[PATCH] i2c: fsi: Create busses for all ports

[Eddie James]

On 6/4/19 10:17 PM, Oliver wrote:
> On Wed, Jun 5, 2019 at 8:57 AM Eddie James <eajames at linux.ibm.com> wrote:
>>
>> On 6/4/19 1:14 AM, Oliver wrote:
>>> On Tue, Jun 4, 2019 at 12:15 AM Eddie James <eajames at linux.ibm.com> wrote:
>>>> On 6/3/19 12:57 AM, Oliver O'Halloran wrote:
>>>>> Currently we only create an I2C bus for the ports listed in the
>>>>> device-tree for that master. There's no real reason for this since
>>>>> we can discover the number of ports the master supports by looking
>>>>> at the port_max field of the status register.
>>>>>
>>>>> This patch re-works the bus add logic so that we always create buses
>>>>> for each port, unless the bus is marked as unavailable in the DT. This
>>>>> is useful since it ensures that all the buses provided by the CFAM I2C
>>>>> master are accessible to debug tools.
>>>>>
>>>>> Cc: Eddie James <eajames at linux.vnet.ibm.com>
>>>>> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
>>>>> ---
>>>>>     drivers/i2c/busses/i2c-fsi.c | 30 +++++++++++++++++++++++++-----
>>>>>     1 file changed, 25 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
>>>>> index 1e2be2219a60..59a76c6e31ad 100644
>>>>> --- a/drivers/i2c/busses/i2c-fsi.c
>>>>> +++ b/drivers/i2c/busses/i2c-fsi.c
>>>>> @@ -658,13 +658,27 @@ static const struct i2c_algorithm fsi_i2c_algorithm = {
>>>>>         .functionality = fsi_i2c_functionality,
>>>>>     };
>>>>>
>>>>> +static device_node *fsi_i2c_find_port_of_node(struct device_node *master,
>>>>> +                                           int port)
>>> Turns out I had a pile of compile fixes staged but not committed so
>>> this patch is totally broken. Oops.
>>>
>>>>> +{
>>>>> +     struct device_node *np;
>>>>> +
>>>>> +     for_each_child_of_node(fsi, np) {
>>>>> +             rc = of_property_read_u32(np, "reg", &port_no);
>>>>> +             if (!rc && port_no == port)
>>>>> +                     return np;
>>>>> +     }
>>>>> +
>>>>> +     return NULL;
>>>>> +}
>>>>> +
>>>>>     static int fsi_i2c_probe(struct device *dev)
>>>>>     {
>>>>>         struct fsi_i2c_master *i2c;
>>>>>         struct fsi_i2c_port *port;
>>>>>         struct device_node *np;
>>>>> +     u32 port_no, ports, stat;
>>>>>         int rc;
>>>>> -     u32 port_no;
>>>>>
>>>>>         i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>>>>>         if (!i2c)
>>>>> @@ -678,10 +692,16 @@ static int fsi_i2c_probe(struct device *dev)
>>>>>         if (rc)
>>>>>                 return rc;
>>>>>
>>>>> -     /* Add adapter for each i2c port of the master. */
>>>>> -     for_each_available_child_of_node(dev->of_node, np) {
>>>>> -             rc = of_property_read_u32(np, "reg", &port_no);
>>>>> -             if (rc || port_no > USHRT_MAX)
>>>>> +     rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &state);
>>>>> +     if (rc)
>>>>> +             return rc;
>>>>> +
>>>>> +     ports = FIELD_GET(I2C_STAT_MAX_PORT, stat);
>>>>> +     dev_dbg(dev, "I2C master has %d ports\n", ports);
>>>> Thanks for the patch Oliver. This looks great except some older CFAM
>>>> types don't report the max port number, in which case this would not
>>>> probe up any ports. So we probably need a fallback to dts if the max
>>>> ports is 0.
>>> Hmm, The oldest CFAM spec I could find was v1.2 which is from the p6
>>> era and it includes the MAX_PORT field. When I was checking the spec I
>>> noticed that I mis-interpreted the meaning of MAX_PORT. It's actually
>>> the largest value you can write into the port field of the mode
>>> register rather than the number of ports the master supports. So zero
>>> is a valid value for MAX_PORT that you would see if the master only
>>> has one port.
>>
>> Yep, now that I look at the specs too, that is correct.
>>
>>
>>> Do you know if the old masters only had one port? If not, do you know
>>> what version (from the ext status reg) of the master doesn't support
>>> the max_port field?
>>
>> I used to have some more up-to-date specs but I can't seem to find
>> them... I think I see what's going on. Some versions of the CFAM have
>> the max port, or "upper threshold for ports" at bits 16-19, while others
>> have that information at 9-15 or 12-15... I'm not sure we can reliably
>> determine where/what that number will be. I'm open to suggestions!
> I had a look at the various docs I've got and they say:
>
> CFAM 1.2:      9 - 11 b ‘000’
>                12 - 15 Upper threshold for I2C ports (Port number - 1)
> p7 pervasive:  9 - 11 b ‘000’
>                12 - 15 Upper threshold for I2C ports (Port number - 1)
> p8 pervasive:  9 - 15 Upper threshold for I2C ports (Port number - 1)
> p9 pervasive:  9 - 15 Upper threshold for I2C ports (Port number - 1)
>
> Keep in mind these docs use IBM bit numbering. Translating to normal bits:
>
>    binary: 01111111 00000000 00000000
> bits set: 22, 21, 20, 19, 18, 17, 16 (7)
>   IBM 32b:  9, 10, 11, 12, 13, 14, 15
>
> And dropping the upper 3 bits gives you 16 - 19. Are you sure it's
> actually different or is this IBM bit ordering just screwing us again?


Ah, good point, I forgot it was backwards, I was puzzling over where I 
had originally gotten the 16-19. So it should be good to check 9-15 
(real 16-22) as the upper bits will be 0 in the older case.


>
> Anyway, while I was looking I noticed that between p7 and p8 they did
> change the layout of the mode register. The baud rate divider was
> extended from 8 to 16 bits and the port select field was moved from
> IBM(8,15) to IBM(16,21) to make room. If we need to support the older
> masters we'll need to fix that too.


Probably not worth it, I don't think anyone will try older processors 
with this driver now.

Thanks,

Eddie


>
> Oliver
>