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: manikanda...@microchip.com <manikanda...@microchip.com> > Sent: Monday, 21 October 2024 13:19 > To: ale...@all4bambi.com; eugen.hris...@linaro.org; u-boot@lists.denx.de > Cc: varshini.rajend...@microchip.com; hari.prasat...@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: manikanda...@microchip.com <manikanda...@microchip.com> >> Sent: Friday, 18 October 2024 12:30 >> To: eugen.hris...@linaro.org; ale...@all4bambi.com; >> u-boot@lists.denx.de >> Cc: varshini.rajend...@microchip.com; hari.prasat...@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.hris...@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, manikanda...@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 <ale...@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.
