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

[Ulf Samuelsson]

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.



> [0] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> 
> Regards,
> Siddharth.
> 
>>   }
>>   
>>   struct mii_dev *cpsw_mdio_init(const char *name, phys_addr_t mdio_base,

Best Regards
Ulf Samuelsson