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

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

[Oliver O'Halloran]

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@linux.vnet.ibm.com>
Signed-off-by: Oliver O'Halloran <oohall@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)
+{
+	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);
+
+	for (port_no = 0; port_no < ports; port_no++) {
+		np = fsi_i2c_find_port_of_node(dev.of_node, port_no);
+		if (np && !of_device_is_available(np))
 			continue;
 
 		port = kzalloc(sizeof(*port), GFP_KERNEL);
-- 
2.20.1

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

[Eddie James]

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@linux.vnet.ibm.com>
> Signed-off-by: Oliver O'Halloran <oohall@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)
> +{
> +	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.


Thanks,

Eddie


> +
> +	for (port_no = 0; port_no < ports; port_no++) {
> +		np = fsi_i2c_find_port_of_node(dev.of_node, port_no);
> +		if (np && !of_device_is_available(np))
>   			continue;
>
>   		port = kzalloc(sizeof(*port), GFP_KERNEL);

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

[Oliver]

On Tue, Jun 4, 2019 at 12:15 AM Eddie James <eajames@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@linux.vnet.ibm.com>
> > Signed-off-by: Oliver O'Halloran <oohall@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.

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?


> Thanks,
>
> Eddie
>
>
> > +
> > +     for (port_no = 0; port_no < ports; port_no++) {
> > +             np = fsi_i2c_find_port_of_node(dev.of_node, port_no);
> > +             if (np && !of_device_is_available(np))
> >                       continue;
> >
> >               port = kzalloc(sizeof(*port), GFP_KERNEL);

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

[Eddie James]

On 6/4/19 1:14 AM, Oliver wrote:
> On Tue, Jun 4, 2019 at 12:15 AM Eddie James <eajames@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@linux.vnet.ibm.com>
>>> Signed-off-by: Oliver O'Halloran <oohall@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!


Thanks,

Eddie



>
>
>> Thanks,
>>
>> Eddie
>>
>>
>>> +
>>> +     for (port_no = 0; port_no < ports; port_no++) {
>>> +             np = fsi_i2c_find_port_of_node(dev.of_node, port_no);
>>> +             if (np && !of_device_is_available(np))
>>>                        continue;
>>>
>>>                port = kzalloc(sizeof(*port), GFP_KERNEL);

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

[Oliver]

On Wed, Jun 5, 2019 at 8:57 AM Eddie James <eajames@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@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@linux.vnet.ibm.com>
> >>> Signed-off-by: Oliver O'Halloran <oohall@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

Re: [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@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@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@linux.vnet.ibm.com>
>>>>> Signed-off-by: Oliver O'Halloran <oohall@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
>