[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <12394ae6-6a2d-4575-9ba1-1b39ca983264@lunn.ch>
Date: Wed, 8 May 2024 20:25:43 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Hans-Frieder Vogt <hfdevel@....net>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
horms@...nel.org, kuba@...nel.org, jiri@...nulli.us,
pabeni@...hat.com
Subject: Re: [PATCH net-next v5 5/6] net: tn40xx: add mdio bus support
> > + writel(((1 << 15) | i), regs + TN40_REG_MDIO_CMD);
>
> similarly here:
>
> writel((MDIO_PHY_ID_C45 | i), regs + TN40_REG_MDIO_CMD);
This one i don't agree with. It happens to work, but there is no
reason to think the hardware has been designed around how Linux
combines the different parts of a C45 address into one word, using the
top bit to indicate it is actually a C45 address, not a C22.
I would much prefer a TN40_ define is added for this bit.
> > + writel(((device & 0x1F) | ((port & 0x1F) << 5)),
>
> and also here, similarly:
>
> writel((device & MDIO_PHY_ID_DEVAD) | ((port << 5) & MDIO_PHY_ID_PRTAD),
Similarly here, this happens to work, but that is just because the
hardware matches a software construct Linux uses. It would be better
to add TN40_ macros to describe the hardware.
Andrew
Powered by blists - more mailing lists