[PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Jeremy Kerr]

Hi Chris,

> Define hub master probe and add definitions for general
> master registers and bitfields.

I think some background on what a hub master is (and how it differs from
a cascaded master) would be useful - either here or in Documentation/.

>  struct fsi_slave {
>  	struct list_head	list_link;	/* Master's list of slaves */
>  	struct list_head	my_engines;
>  	struct device		dev;
> -	struct fsi_master	*master;
> +	struct fsi_master	*master;	/* Upstream master */
> +	struct fsi_master	*next_master;	/* Downstream master */
>  	int			link;
>  	uint8_t			id;
> +	uint32_t		base;		/* Base address */
>  };

[...]

> @@ -26,6 +75,10 @@ struct fsi_master {
>  	int		idx;
>  	int		n_links;
>  	uint32_t	ipoll;
> +	struct fsi_device *engine;
> +	struct fsi_device *link;
> +	uint8_t		type;
> +	uint32_t	link_size;
>  	int		(*read)(struct fsi_master *, int link,
>  				uint8_t slave, uint32_t addr,
>  				void *val, size_t size);

It seems to me that you've added a lot of hub-specific definitions to
multiple parts of the generic FSI core code. I would much rather see
this contained to code that supports just the hub.

However, the hub functionality doesn't seem to fit within the
slave-engine device model - as a slave-engine driver (binding on the
0x1c/0x1d entries in the configuration table), the hub driver violates
the device model in that it performs accesses to addresses *way* outside
of the allocations described in the configuration table.

[that's why you've altered fsi_dev->size in the master driver, and can
potentially lead to conflicts with other drivers in future]

>From what I can glean from the docs, the hub master has a couple of
interfaces:

 - the control register set described in the slave configuration table
   as type 0x1d

 - a description of the links, as entries in the slave configuration
   table as type 0x1c

 - the actual FSI master address space, provided underneath a *slave*
   (and not a *slave engine* - that distinction is important).

To me, it sounds like we should not make this a slave engine driver, but
a part of the core *slave* code, because the hub masters are attached
directly to the slaves. This would involve:

  - struct fsi_master_hub {
        struct fsi_master master;
        struct fsi_slave *slave;
	uint32_t          control_regs;  /* slave-relative addr of regs */
	uint32_t          base;          /* slave-relative addr of
					    master address space */
    }

  - in fsi_slave_scan():

     - suppressing the 0x1c and 0x1d devices from creating slave engines

     - if we find an 0x1d type in the configuration table:
        - struct fsi_master_hub *hub = kzalloc(...);
	  hub->base = /* pre-defined */
	  hub->control_regs = /* parsed from config table */
	  hub->slave = slave;

     - if (hub) {
          use a count of the 0x1c types to populate n_links
	  fsi_master_register(&hub->master);
       }

  - the read() and write() callbacks of fsi_master_hub just pass through
    to the slave (at offset ->base), and the enable/break will access the
    control_regs.

This means that we keep the hub code encapsulated, and the hub
implementation doesn't leak out to the core fsi_master or fsi_slave
structs.

For cascaded masters, we can probably keep this model of using a slave
engine driver, but I just don't think it's suitable for the hubs.

Regards,


Jeremy