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

[Christopher Bostic]

On 2/17/17 11:24 AM, Christopher Bostic wrote:
>
>
> 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.

Hi Jeremy,

Since the hMFSI link type does have a number of 1k slots like cMFSI 
would this influence your proposed changes?
Sounded like you were inclined to keep the slave engine driver model for 
cMFSI.

Thanks,
> Thanks for your input,
>
> Chris
>
>>
>> Regards,
>>
>>
>> Jeremy
>>
>