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

["Markus Schneider-Pargmann" <msp@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

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?

Best
Markus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 289 bytes --]