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

[Oliver]

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?

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.

Oliver