[PATCH 1/1] Fixed sama5d3 dts file so PIO sections are inside pinctrl as in kernel dts
Manikandan.M at microchip.com
Manikandan.M at microchip.com
Tue Oct 22 10:46:41 CEST 2024
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 at microchip.com <Manikandan.M at microchip.com>
> Sent: Monday, 21 October 2024 13:19
> To: alexey at all4bambi.com; eugen.hristev at linaro.org; u-boot at lists.denx.de
> Cc: Varshini.Rajendran at microchip.com; Hari.PrasathGE at 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 at microchip.com <Manikandan.M at microchip.com>
>> Sent: Friday, 18 October 2024 12:30
>> To: eugen.hristev at linaro.org; alexey at all4bambi.com;
>> u-boot at lists.denx.de
>> Cc: Varshini.Rajendran at microchip.com; Hari.PrasathGE at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at fffffc00 {
>>>>
>>>
>>
>> --
>> Thanks and Regards,
>> Manikandan M.
>
> --
> Thanks and Regards,
> Manikandan M.
--
Thanks and Regards,
Manikandan M.
More information about the U-Boot
mailing list