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

[Christopher Bostic]

On 2/17/17 1:42 AM, Jeremy Kerr wrote:
> 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/.

Hi Jeremy,

Ok will add some details here.
>>   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]
I altered the dev size due to the incorrect size of hMFSI links/ports in 
the config table.  More on that below.

>  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

I think 0x1d and 0x1c types are swapped in the overall discussion.
>
>   - 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.

Good idea, will implement that.
> 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.

There isn't much difference conceptually between a cMFSI master and hub 
hMFSI master.   Only things that vary between the two are number of 
links/ports that might be supported off of them and what size address 
space their links/ports span.  The cMFSI master on P9 has a 0x8000 byte 
addressable window per link/port  whereas hMFSI accesses 0x80000 per link.

The P9 CFAM configuration space does not list the hMFSI port size 
correctly.  Each link/port is listed as size *0*.
The cMFSI link/port size is correct, 0x20 1k pages.

Given their similarities it would make sense to me to use the refactored 
approach you describe above for both hMFSI and and in the future, cMFSI.

Thanks for your input,

Chris

>
> Regards,
>
>
> Jeremy
>