RE: [PATCH 1/1] Fixed sama5d3 dts file so PIO sections are inside pinctrl as in kernel dts

[Alexey Tsirlin <alexey@all4bambi.com>]

No problem!

Thanks!
Alexey.

-----Original Message-----
From: Manikandan.M@microchip.com <Manikandan.M@microchip.com>
Sent: Tuesday, 22 October 2024 11:47
To: alexey@all4bambi.com; eugen.hristev@linaro.org; u-boot@lists.denx.de
Cc: Varshini.Rajendran@microchip.com; Hari.PrasathGE@microchip.com
Subject: Re: [PATCH 1/1] Fixed sama5d3 dts file so PIO sections are inside
pinctrl as in kernel dts

Hi Alexey,

Thank you for confirming the u-boot tree used for testing your patch.
This suggested modification is a fix-up patch for the pinctrl driver changes
present in the uboot-mcho git tree. However, because those changes are not
yet present in the mainline U-Boot, this patch will cause regression on all
sama5d3 SoC boards.

I appreciate your work in finding and resolving the issue for sama5d3.Please
hold on to this patch; you can share a version 2 that resolves the comments
once the driver changes are in.

Also, fyi, this change is already present in the u-boot-mchp tree
linux4microchip-2024.10-rc2 version,
https://github.com/linux4microchip/u-boot-mchp/commit/a25b31c2558f9fe303a3b97dd660b87476446301
On 22/10/24 11:14 am, Alexey Tsirlin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> Hi Manikandan,
>
> I am using buildroot 2024.04 sama5d3_eds_headless_defconfig config
> setup which leads to the following:
> 1. uboot from https://github.com/linux4microchip/u-boot-mchp.git
> 2. uboot version linux4microchip-2024.10-rc2 3. uboot config
> sama5d3_xplained_mmc
>
> My board is sama5d3 EDS with DP83630 PHY connected to EMAC.
>
> I confirm that using configuration above, I was not able to make
> Ethernet
> (EMAC) in uboot work because it was failing to configure EMAC pins to
> the required peripheral mode. The at91_pin_check_config function in
> pinctrl-at91 returned -EINVAL because of nbanks=0. After applying the
> patch, the Ethernet is working fine.
>
> I also confirm that after applying the patch, I was able to toggle IO
> pin with gpio command (tried PIOC18 which is accessible through IO
> Connector #1), although I cannot tell you if this functionality also
> worked before applying the patch.
>
> Regards,
> Alexey.
>
> -----Original Message-----
> From: Manikandan.M@microchip.com <Manikandan.M@microchip.com>
> Sent: Monday, 21 October 2024 13:19
> To: alexey@all4bambi.com; eugen.hristev@linaro.org;
> u-boot@lists.denx.de
> Cc: Varshini.Rajendran@microchip.com; Hari.PrasathGE@microchip.com
> Subject: Re: [PATCH 1/1] Fixed sama5d3 dts file so PIO sections are
> inside pinctrl as in kernel dts
>
> Hi Alexey,
>
> Can you confirm if you are using the u-boot-mchp repo from GitHub [1]
> to test the sama5d3_eds board where the driver changes to align U-Boot
> and Kernel DT are already present.
> The pinctrl driver probe fails with the proposed change and will cause
> regression on other boards that uses the same pinctrl PIO3 driver in
> the mainline repo.
>
> [1] --> https://github.com/linux4microchip/u-boot-mchp
>
> On 20/10/24 10:44 am, Alexey Tsirlin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>> know the content is safe
>>
>> Hi Manikandan,
>>
>> I have tested gpio cmd option on SAMA5D3 EDS board (BTW it wasn't
>> enabled by default in sama5d3_xplained_mmc_defconfig which is used by
>> this board) in u-boot with the DT change as I proposed and it seems
>> to work fine, at least it detects all the 5 GPIO ports (A-E). pinmux
>> cmd does not work too much, but this is because pinctrl-at91 does not
>> implement get_pins_count function.
>> Without the proposed change I was not able to make the Ethernet
>> (EMAC) detect the PHY because MDIO interface was not working - the
>> correct peripheral mode for the EMAC pins was not set as defined in DT.
>>
>> Regards,
>> Alexey.
>>
>> -----Original Message-----
>> From: Manikandan.M@microchip.com <Manikandan.M@microchip.com>
>> Sent: Friday, 18 October 2024 12:30
>> To: eugen.hristev@linaro.org; alexey@all4bambi.com;
>> u-boot@lists.denx.de
>> Cc: Varshini.Rajendran@microchip.com; Hari.PrasathGE@microchip.com
>> Subject: Re: [PATCH 1/1] Fixed sama5d3 dts file so PIO sections are
>> inside pinctrl as in kernel dts
>>
>> Hi Eugen,
>>
>> On 18/10/24 12:45 pm, Eugen Hristev wrote:
>>> [You don't often get email from eugen.hristev@linaro.org. Learn why
>>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> know the content is safe
>>>
>>> Hello Alexey,
>>>
>>> Please fix the subject to adhere to the rules ARM: dts: .... etc, if
>>> you are unsure, please follow previous commits that touched this file.
>>>
>>> On 10/17/24 11:51, Manikandan.M@microchip.com wrote:
>>>> Hi Alexey,
>>>>
>>>> On 15/10/24 8:23 pm, Alexey Tsirlin wrote:
>>>>> This allows setting the GPIO parameters from device tree,
>>>>> otherwise the at91_pin_check_config will fail because the
>>>>> priv->nbanks equal to zero
>>>
>>> I remember these pin banks are outside of the pinctrl because the
>>> driver fails to probe them if they are inside.
>>> Is this no longer true ?
>> Indeed, you are correct.With the current code base the pinctrl fails
>> to probe if they are defined inside.
>> I started to review this code with an intention that my changes for
>> pinctrl driver and DT to align U-Boot pinctrl DT node with the kernel
>> had already been made upstream, however that is not the case.
>> This patch is valid and necessary only after when my changes are
>> up-streamed otherwise driver probe will fail
>>
>> Since I don't own a board with this SoC, Alexey, could you kindly
>> check the GPIO functions and determine whether this patch is actually
>> necessary for the problem you're experiencing?If not, after
>> incorporating the driver changes, you can send this as part of the DT
>> alignment
>>>
>>> Manikandan, is it possible to test this on the board? and use the
>>> gpio command in U-boot to toggle the pins , like e.g. for the LEDs
>>> and see if there is no regression ?
>>>
>>> Thanks,
>>> Eugen
>>>>>
>>>>> Signed-off-by: Alexey Tsirlin <alexey@all4bambi.com>
>>>>> ---
>>>>>
>>>>>      arch/arm/dts/sama5d3.dtsi | 111
>>>>> +++++++++++++++++++-------------------
>>>>>      1 file changed, 56 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/dts/sama5d3.dtsi b/arch/arm/dts/sama5d3.dtsi
>>>>> index 4c03a302ec..c671ea42f2 100644
>>>>> --- a/arch/arm/dts/sama5d3.dtsi
>>>>> +++ b/arch/arm/dts/sama5d3.dtsi
>>>>> @@ -873,66 +873,67 @@
>>>>>                                                         AT91_PIOE
>>>>> 17 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* PE17 periph B, conflicts
>>>>> with
>>>>> A17 */
>>>>>                                        };
>>>>>                                };
>>>>> -                    };
>>>>>
>>>>> -                    pioA: gpio@fffff200 {
>>>>> -                            compatible = "atmel,at91sam9x5-gpio",
>>>>> "atmel,at91rm9200-gpio";
>>>>> -                            reg = <0xfffff200 0x100>;
>>>>> -                            interrupts = <6 IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> -                            #gpio-cells = <2>;
>>>>> -                            gpio-controller;
>>>>> -                            interrupt-controller;har
>>>>> -                            #interrupt-cells = <2>;
>>>>> -                            clocks = <&pioA_clk>;
>>>>> -                            bootph-all;
>>>>> -                    };
>>>>> +                            pioA: gpio@fffff200 {
>>>>> +                                  compatible =
>>>>> "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>>>> Spaces instead of tab before 'compatible', should be consistent
>>>> with the remaining properties of pioA node.
>>>>> +                                    reg = <0xfffff200 0x100>;
>>>>> +                                    interrupts = <6
>>>>> IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> +                                    #gpio-cells = <2>;
>>>>> +                                    gpio-controller;
>>>>> +                                    interrupt-controller;
>>>>> +                                    #interrupt-cells = <2>;
>>>>> +                                    clocks = <&pioA_clk>;
>>>>> +                                    bootph-all;
>>>>> +                            };
>>>>>
>>>>> -                    pioB: gpio@fffff400 {
>>>>> -                            compatible = "atmel,at91sam9x5-gpio",
>>>>> "atmel,at91rm9200-gpio";
>>>>> -                            reg = <0xfffff400 0x100>;
>>>>> -                            interrupts = <7 IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> -                            #gpio-cells = <2>;
>>>>> -                            gpio-controller;
>>>>> -                            interrupt-controller;
>>>>> -                            #interrupt-cells = <2>;
>>>>> -                            clocks = <&pioB_clk>;
>>>>> -                            bootph-all;
>>>>> -                    };
>>>>> +                            pioB: gpio@fffff400 {
>>>>> +                                  compatible =
>>>>> "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>>>> Ditto
>>>>> +                                    reg = <0xfffff400 0x100>;
>>>>> +                                    interrupts = <7
>>>>> IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> +                                    #gpio-cells = <2>;
>>>>> +                                    gpio-controller;
>>>>> +                                    interrupt-controller;
>>>>> +                                    #interrupt-cells = <2>;
>>>>> +                                    clocks = <&pioB_clk>;
>>>>> +                                    bootph-all;
>>>>> +                            };
>>>>>
>>>>> -                    pioC: gpio@fffff600 {
>>>>> -                            compatible = "atmel,at91sam9x5-gpio",
>>>>> "atmel,at91rm9200-gpio";
>>>>> -                            reg = <0xfffff600 0x100>;
>>>>> -                            interrupts = <8 IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> -                            #gpio-cells = <2>;
>>>>> -                            gpio-controller;
>>>>> -                            interrupt-controller;
>>>>> -                            #interrupt-cells = <2>;
>>>>> -                            clocks = <&pioC_clk>;
>>>>> -                            bootph-all;
>>>>> -                    };
>>>>> +                            pioC: gpio@fffff600 {
>>>>> +                                  compatible =
>>>>> "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>>>> Ditto
>>>>> +                                    reg = <0xfffff600 0x100>;
>>>>> +                                    interrupts = <8
>>>>> IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> +                                    #gpio-cells = <2>;
>>>>> +                                    gpio-controller;
>>>>> +                                    interrupt-controller;
>>>>> +                                    #interrupt-cells = <2>;
>>>>> +                                    clocks = <&pioC_clk>;
>>>>> +                                    bootph-all;
>>>>> +                            };
>>>>>
>>>>> -                    pioD: gpio@fffff800 {
>>>>> -                            compatible = "atmel,at91sam9x5-gpio",
>>>>> "atmel,at91rm9200-gpio";
>>>>> -                            reg = <0xfffff800 0x100>;
>>>>> -                            interrupts = <9 IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> -                            #gpio-cells = <2>;
>>>>> -                            gpio-controller;
>>>>> -                            interrupt-controller;
>>>>> -                            #interrupt-cells = <2>;
>>>>> -                            clocks = <&pioD_clk>;
>>>>> -                            bootph-all;
>>>>> -                    };
>>>>> +                            pioD: gpio@fffff800 {
>>>>> +                                  compatible =
>>>>> "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>>>> Ditto
>>>>> +                                    reg = <0xfffff800 0x100>;
>>>>> +                                    interrupts = <9
>>>>> IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> +                                    #gpio-cells = <2>;
>>>>> +                                    gpio-controller;
>>>>> +                                    interrupt-controller;
>>>>> +                                    #interrupt-cells = <2>;
>>>>> +                                    clocks = <&pioD_clk>;
>>>>> +                                    bootph-all;
>>>>> +                            };
>>>>> +
>>>>> +                            pioE: gpio@fffffa00 {
>>>>> +                                  compatible =
>>>>> "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
>>>> Ditto
>>>>> +                                    reg = <0xfffffa00 0x100>;
>>>>> +                                    interrupts = <10
>>>>> IRQ_TYPE_LEVEL_HIGH 1>;
>>>>> +                                    #gpio-cells = <2>;
>>>>> +                                    gpio-controller;
>>>>> +                                    interrupt-controller;
>>>>> +                                    #interrupt-cells = <2>;
>>>>> +                                    clocks = <&pioE_clk>;
>>>>> +                                    bootph-all;
>>>>> +                            };
>>>>>
>>>> Extra line
>>>>> -                    pioE: gpio@fffffa00 {
>>>>> -                            compatible = "atmel,at91sam9x5-gpio",
>>>>> "atmel,at91rm9200-gpio";
>>>>> -                            reg = <0xfffffa00 0x100>;
>>>>> -                            interrupts = <10 IRQ_TYPE_LEVEL_HIGH
>>>>> 1>;
>>>>> -                            #gpio-cells = <2>;
>>>>> -                            gpio-controller;
>>>>> -                            interrupt-controller;
>>>>> -                            #interrupt-cells = <2>;
>>>>> -                            clocks = <&pioE_clk>;
>>>>> -                            bootph-all;
>>>>>                        };
>>>>>
>>>>>                        pmc: pmc@fffffc00 {
>>>>
>>>
>>
>> --
>> Thanks and Regards,
>> Manikandan M.
>
> --
> Thanks and Regards,
> Manikandan M.

--
Thanks and Regards,
Manikandan M.