Re: [PATCH] board: ti: am335x: Conditional MDIO PAD configuration instead of static for AM335_ICE

[Parvathi Pudi <parvathi@couthit.com>]

Hi Markus,

> Hi Parvathi,
> 
> On Fri Apr 17, 2026 at 2:06 PM CEST, Parvathi Pudi wrote:
>> Hi Markus,
>>  
>>> On Tue Apr 7, 2026 at 10:22 AM CEST, Parvathi Pudi wrote:
>>>> This patch removes the static MDIO pinmux configuration from
>>>> rmii1_pin_mux[] and instead configures the MDIO pins conditionally
>>>> during board_init(). Previously, the MDIO_CLK and MDIO_DATA pins
>>>> were always configured for CPSW in mux.c, which could lead to
>>>> unnecessary pin ownership and conflicts in scenarios where CPSW
>>>> is not used.
>>>>
>>>> With this change, the MDIO pins are configured only when required,
>>>> ensuring that CPSW Ethernet functionality in U-Boot remains unaffected.
>>>> This approach keeps Ethernet boot behavior intact and provides cleaner
>>>> separation between CPSW and other Ethernet use cases.
>>> 
>>> Do you have a specific use case here?
>>> 
>>
>> We have an ICSSM PRUETH Ethernet use case. The existing static MDIO pinmux
>> configuration has been hard-coded for CPSW. Based on the hardware jumper
>> setting we configure the MDIO either for CPSW or for the ICSSM PRUETH use case.
>>
>>>>
>>>> Signed-off-by: Parvathi Pudi <parvathi@couthit.com>
>>>> ---
>>>>  board/ti/am335x/board.c | 22 ++++++++++++++++++++++
>>>>  board/ti/am335x/mux.c   |  2 --
>>>>  2 files changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/board/ti/am335x/board.c b/board/ti/am335x/board.c
>>>> index 90e37a8d913..abeab809387 100644
>>>> --- a/board/ti/am335x/board.c
>>>> +++ b/board/ti/am335x/board.c
>>>> @@ -61,6 +61,18 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>  #define GPIO_ETH0_MODE		GPIO_TO_PIN(0, 11)
>>>>  #define GPIO_ETH1_MODE		GPIO_TO_PIN(1, 26)
>>>>  
>>>> +#define AM335X_PIN_MDIO	0x948
>>>> +#define AM335X_PIN_MDC		0x94c
>>> 
>>> These two are not aligned when applied.
>>> 
>>
>> We will fix this in the next version.
>>
>>>> +
>>>> +#define GPIO_MDIO_DATA		CTRL_BASE + AM335X_PIN_MDIO
>>>> +#define GPIO_MDIO_CLK		CTRL_BASE + AM335X_PIN_MDC
>>>> +
>>>> +/* Enabling MDIO_DATA by setting MUX_MODE to 0, RXACTIVE, PULLUP_EN bits */
>>>> +#define PAD_CONFIG_MDIO_DATA	0x30
>>>> +
>>>> +/* Enabling MDIO_CLK by setting MUX_MODE to 0, PULLUP_EN bit */
>>>> +#define PAD_CONFIG_MDIO_CLK	0x10
>>>> +
>>>>  static struct ctrl_dev *cdev = (struct ctrl_dev *)CTRL_DEVICE_BASE;
>>>>  
>>>>  #define GPIO0_RISINGDETECT	(AM33XX_GPIO0_BASE + OMAP_GPIO_RISINGDETECT)
>>>> @@ -779,6 +791,16 @@ int board_init(void)
>>>>  			hang();
>>>>  		}
>>>>  
>>>> +		if (!eth0_is_mii || !eth1_is_mii) {
>>> 
>>> eth0_is_mii and eth1_is_mii is the same, as it was checked in the
>>> if condition above this one. You could make this an else if as well if
>>> you like.
>>> 
>>
>> Sure, we will clean it up in the next version.
>>
>>>> +			/* Set the Mux Mode to MDIO_DATA */
>>>> +			reg = readl(GPIO_MDIO_DATA);
>>>> +			writel(reg & PAD_CONFIG_MDIO_DATA, GPIO_MDIO_DATA);
>>>> +
>>>> +			/* Set the Mux Mode to MDIO_CLK */
>>> 
>>> Please remove comments for code that shows the same thing.
>>> 
>>
>> We will fix this in the next version.
>>
>>>> +			reg = readl(GPIO_MDIO_CLK);
>>>> +			writel(reg & PAD_CONFIG_MDIO_CLK, GPIO_MDIO_CLK);
>>> 
>>> Could you do the same thing in enable_board_pin_mux()? Or would it be
>>> possible to reuse mux.h here to avoid redefining values etc.?
>>> 
>>> Why do you use reg & PAD_CONFIG_MDIO_CLK instead of writing
>>> PAD_CONFIG_MDIO_CLK directly and skip the read?
>>> 
>>
>> enable_board_pin_mux() runs before the jumper state is known, so we can't
>> use it here. We'll reuse the existing macros from mux.h instead to avoid
>> the redefinition's.
>>
>>>> +		}
>>>> +
>>> 
>>> This snippet is in the section guarded by these:
>>> 
>>>  #if defined(CONFIG_CLOCK_SYNTHESIZER) && (!defined(CONFIG_XPL_BUILD) || \
>>>    (defined(CONFIG_SPL_ETH) && defined(CONFIG_XPL_BUILD)))
>>>    if (board_is_icev2()) {
>>> 
>>> Is this the same as for the original pinmux setup?
>>> 
>>
>> We have not changed the original pinmux setup.
>> Can you please elaborate bit more on this?
> 
> Yes, I specifically meant the #if defined(...) conditions that are
> present for the if block in which you added the new code. The MDIO
> configuration is now only executed when CONFIG_CLOCK_SYNTHESIZER is set
> (and CONFIG_SPL_ETH for SPL builds). Is this intentional and the same
> condition as it was running in the pinmux code?
> 

CONFIG_CLOCK_SYNTHESIZER is needed for Ethernet functionality. Currently,
this is the only place where the jumper settings are being read and the
appropriate RMII/MII mode is set. If the jumper is set to CPSW (RMII) mode
then the MDIO pin mux changes proposed in our patch will retain the existing
Ethernet functionality.

Thanks and Regards,
Parvathi.