[PATCH linux dev-4.10 0/4] i2c fsi fixes

[Eddie James]

On 07/06/2017 09:34 AM, Joel Stanley wrote:
> Hi Eddie,
>
> These are some work in progress cleanups that I have created for the fsi i2c
> driver.
>
> Patch 4 causes the kernel to crash when scanning for devices. To reproduce,
> boot the system and do 'obmcutil poweron'. According to the device tree
> bindings, his change is correct:
>
>   https://github.com/openbmc/linux/blob/dev-4.10/Documentation/devicetree/bindings/fsi/fsi.txt#L56
>
>   > Since the master nodes describe the top-level of the FSI topology, they also
>   > need to declare the FSI-standard addressing scheme. This requires two cells for
>   > addresses (link index and slave ID), and no size:
>   >
>   >  #address-cells = <2>;
>   >  #size-cells = <0>;
>
> The code suggests the same:
>
>   https://github.com/openbmc/linux/blob/dev-4.10/drivers/fsi/fsi-core.c#L628
>
>   > na = of_n_addr_cells(np);
>   > ns = of_n_size_cells(np);
>   >
>   > /* Ensure we have the correct format for addresses and sizes in
>   >  * reg properties
>   >  */
>   > if (na != 2 || ns != 0)
>   >	return false;
>
> If you don't apply that fix, we do get one of the chips probing correctly (I
> assume the hub, as it has the correct format).

Ah, I see the confusion now. The cfam at 0,0 node is a slave node. The base 
master node (gpio-fsi) is defined in the machine-specific dts files, and 
has the correct format of 2 address-cells and 0 size-cells.

fsi: gpio-fsi {
         compatible = "fsi-master-gpio", "fsi-master";
         #address-cells = <2>;
         #size-cells = <0>;

The cfam at 0,0 should stick with 1 address and 1 size cell for it's 
children, as it's describing the slave.

The hub master is already correct, as you mentioned, and correctly has a 
slave cfam node with 1 address and 1 size cell.

>
> Another issue is patch 2, which removes the statically allocated port numbers,
> where you added 100 or 200 the port number. To retain the old behaviour, I
> suggest you send a patch that adds aliases instead:
>
>   aliases {
> 	 i2c100 = &cfam0_i2c1;
> 	 i2c101 = &cfam0_i2c2;
>   }

Yep, this appears to work. Though it starts at 101 even though i aliased 
i2c100. Strange.

>
> That's a workaround though. We should get the people using these interfaces to
> discover them properly, instead of assuming they exist at a magic offset. sysfs
> is a good way to do this.
>
>   /sys/devices/platform/gpio-fsi/fsi0/slave at 00:00/00:00:00:03/i2c-19/

Agreed, but using this there is still no guarantee about which i2c port 
you're actually on unless you read the "name" property.

# cat /sys/bus/fsi/drivers/fsi_i2c_master/00\:00\:00\:03/i2c-14/name
fsi_i2c0

^ indicating port 0.

Anyway, the three I2C patches look good, and we can add aliasing.

Thanks!
Eddie

>
> Joel Stanley (4):
>    i2c: fsi: Remove ida counter
>    i2c: fsi: Dynamically allocate port numbers
>    i2c: fsi: Remove idx counter
>    ARM: dts: cfam: Fix hub node
>
>   arch/arm/boot/dts/ibm-power9-cfam.dtsi |  4 ++--
>   drivers/i2c/busses/i2c-fsi.c           | 28 ++++------------------------
>   2 files changed, 6 insertions(+), 26 deletions(-)
>