[PATCH v1 6/6] ARM: BCM5301X: Add DT for Meraki MR32

[Ray Jui]

On 8/18/2020 8:34 PM, Florian Fainelli wrote:
> 
> 
> On 8/18/2020 7:46 PM, Christian Lamparter wrote:
>> On 2020-08-19 02:07, Ray Jui wrote:
>>>
>>>
>>> On 8/18/2020 3:52 PM, Christian Lamparter wrote:
>>>> Hello Florian,
>>>>
>>>> On 2020-08-18 22:11, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 8/18/2020 9:39 AM, Christian Lamparter wrote:
>>>>> [snip]
>>>>>> +    i2c-gpio {
>>>>>> +        compatible = "i2c-gpio";
>>>>>> +        sda-gpios = <&chipcommon 5 GPIO_ACTIVE_HIGH>;
>>>>>> +        scl-gpios = <&chipcommon 4 GPIO_ACTIVE_HIGH>;
>>>>>> +        i2c-gpio,delay-us = <10>; /* close to 100 kHz */
>>>>>> +        #address-cells = <1>;
>>>>>> +        #size-cells = <0>;
>>>>>
>>>>> Can you try using the hardware controller here instead of bit banging
>>>>> i2c over gpios? GPIOs 4 and 5 are the default pins for I2C.
>>>>
>>>> Ok. I gave this a try. What I did was:
>>>>
>>>> I removed the i2c-gpio node and went with this in the mr32.dts:
>>>>
>>>> +&i2c0 {
>>>> +       status = "okay";
>>>> +
>>>> +    clock-frequency = <100000>; /* also tried 400KHz */
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&pinmux_i2c>; /* editted bcm5301x.dtsi for this */
>>>> +
>>>> +       cur_mon: ina2xx at 45 {
>>>> +               compatible = "ti,ina219";
>>>> +               reg = <0x45>;
>>>> +               shunt-resistor = <60000>; /* = 60 mOhms */
>>>> +       };
>>>> +
>>>> +       meraki_eeprom: at24 at 50 {
>>>> +               compatible = "atmel,24c64";
>>>> +               reg = <0x50>;
>>>> +               pagesize = <32>;
>>>> +               read-only;
>>>> +       };
>>>> +};
>>>>
>>>> I enabled CONFIG_I2C_BCM_IPROC=y and built the whole thing and moved it
>>>> to the AP.
>>>>
>>>> During boot, I now get:
>>>>
>>>> |[    1.039174] bcm-iproc-i2c 18009000.i2c: bus set to 100000 Hz
>>>> |[    8.918419] i2c /dev entries driver
>>>> [...] (The i2c-bcm-iproc now causes a long "wait" during boot)
>>>> |[   59.385497] bcm-iproc-i2c 18009000.i2c: transaction timed out
>>>> |[   59.391272] ina2xx 0-0045: error configuring the device: -110
>>>> |[  110.585517] bcm-iproc-i2c 18009000.i2c: transaction timed out
>>>>
>>>
>>> The long wait is probably caused by waiting for the I2C transaction to
>>> time out, which it eventually did.
>>
>> Yes.
>>
>>  >
>>>
>>>> Is there a special magic needed to get this working with bcm5301x's
>>>> existing i2c0 node?
>>>>
>>>
>>> Two things to check: 1) if pinmux is configured properly; 
>> Hm, any tips for testing this? The /sys/kernel/debug/pinctrl is
>> populated correctly from what I can tell.
>>
>> What I don't know is if the DTS in bcm5301x.dtsi.
>>
>> |        dmu at 1800c000 {
>> |                compatible = "simple-bus";
>> |                ranges = <0 0x1800c000 0x1000>;
>> |                #address-cells = <1>;
>> |                #size-cells = <1>;
>> |
>> |                cru at 100 {
>> |                        compatible = "simple-bus";
>> |                        reg = <0x100 0x1a4>;
>> |                        ranges;
>> |                        #address-cells = <1>;
>> |                        #size-cells = <1>;
>> |
>> |                        pin-controller at 1c0 {
>> |                                compatible = "brcm,bcm4708-pinmux";
>> |                                reg = <0x1c0 0x24>;
>>
>> Based on my understanding of the DT, the pinctrl register should be at
>> 0x1800c2c0 (that would be outside of the 0x1a4 size though?!). This is
>> because of 0x1800c000 (dmu base) + 0x100 (cru reg) + 0x1c0
>> (pin-controller reg). If so, here are devmem's output of said region
>> (read value is after the "=")
> 
> There is a translation problem here, we are off by 0x100 (need to check
> the bcm4708 data sheet though quite positive it applies there, too), the
> following would be more correct:
> 

It looks like someone made a mistake when creating this file (and
obviously these pinmux configurations were not tested when that someone
upstreamed this).

It looks like 0x100 is done at the CDRU node and got double added again
in the pin-controller node under the CDRU node.

> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
> b/arch/arm/boot/dts/bcm5301x.dtsi
> index 2d9b4dd05830..d73e5151ce51 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -407,9 +407,9 @@
>                         #address-cells = <1>;
>                         #size-cells = <1>;
> 
> -                       pin-controller at 1c0 {
> -                               compatible = "brcm,bcm4708-pinmux";
> -                               reg = <0x1c0 0x24>;
> +                       pin-controller at c0 {
> +                               compatible = "brcm,bcm53012-pinmux";
> +                               reg = <0xc0 0x24>;
>                                 reg-names = "cru_gpio_control";
> 
>                                 spi-pins {
> 
>>
>> devmem 0x1800c2c0 = 0x0
>> devmem 0x1800c2c4 = 0x001D2003
>> devmem 0x1800c2c8 = 0x00000284
>> devmem 0x1800c2cc = 0x00000284
>> devmem 0x1800c2d0 = 0x00000285
>> devmem 0x1800c2d4 = 0x00000284
>> devmem 0x1800c2d8 = 0x00000284
>> devmem 0x1800c2dc = 0x00000284
>> devmem 0x1800c2e0 = 0x00000284
>> devmem 0x1800c2e4 = 0x00000284
>>
>> Just in case, I've also checked 0x1800c1c0.
>> This is because I stumbled over the "Example" in the binding
>> Documentation under
>> Documentation/devicetree/bindings/pinctrl/brcm,bcm4708-pinmux.txt.
>> Because this uses a "offset = <0xc0>"
>> property and seems to be based of the cru at 100 node?!
>>
>> devmem 0x1800c1c0 = 0x00140037
> 
> So here we can see that bits 4 and 5 are set to 1, which means that they
> are still configured as GPIOs, and not as pins for the desired functions
> which explains the timeout.

Yes, that makes sense. Thanks, Florian!