[PATCH] cpsw_mdio.c: Use correct reg in cpsw_mdio_get_alive

[Siddharth Vadapalli]

On 09/02/23 14:56, Ulf Samuelsson wrote:
> 
> Den 2023-02-09 kl. 08:29, skrev Siddharth Vadapalli:
>> Hello Ulf,
>>
>> On 07/02/23 13:55, Ulf Samuelsson wrote:
>>> cpsw_mdio_get_alive reads the wrong register.
>>> See page 2316 in SPRUH73Q AM335x TRM
>>>
>>> Signed-off-by: Ulf Samuelsson <ulf at emagii.com>
>>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>>> Cc: Ramon Fried <rfried.dev at gmail.com>
>>> ---
>>>   drivers/net/ti/cpsw_mdio.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ti/cpsw_mdio.c b/drivers/net/ti/cpsw_mdio.c
>>> index a5ba73b739..ac791faa81 100644
>>> --- a/drivers/net/ti/cpsw_mdio.c
>>> +++ b/drivers/net/ti/cpsw_mdio.c
>>> @@ -51,7 +51,7 @@ struct cpsw_mdio_regs {
>>>   #define USERACCESS_PHY_REG_SHIFT    (21)
>>>   #define USERACCESS_PHY_ADDR_SHIFT    (16)
>>>   #define USERACCESS_DATA        GENMASK(15, 0)
>>> -    } user[0];
>>> +    } user[2];
>>>   };
>>>     #define CPSW_MDIO_DIV_DEF    0xff
>>> @@ -366,8 +366,8 @@ u32 cpsw_mdio_get_alive(struct mii_dev *bus)
>>>       struct cpsw_mdio *mdio = bus->priv;
>>>       u32 val;
>>>   -    val = readl(&mdio->regs->control);
>>> -    return val & GENMASK(15, 0);
>>> +    val = readl(&mdio->regs->alive);
>>> +    return val & GENMASK(7, 0);
>>
>> Referring to the TRM for AM335x at [0], on page 2401, the MDIOALIVE register
>> reports the presence of Ethernet PHYs up to ADDR 31. Also, the cpsw driver:
>> drivers/net/ti/cpsw.c which uses the cpsw_mdio_get_alive() function, expects the
>> return value to be a 16 bit value. Therefore, I believe that the GENMASK should
>> retain the previous value of "GENMASK(15, 0)", while only the register being
>> read needs to be changed from "control" to "alive". Please let me know if I
>> misunderstood something regarding your implementation.
>>
> We had an unusual H/W where the PHY (actually a switch) was connected to the
> second Ethernet port. All boards I have seen have the PHY connected to the first
> port or have both ports connected.
> 
> IIRC correctly (this patch is > 2 years old),
> we had bad data in the bits (15:8) and this is why only the 8 bits.
> Only bits [1:0] are used anyway.

Thank you for clarifying.

Reviewed-by: Siddharth Vadapalli <s-vadapalli at ti.com>

Regards,
Siddharth.