On Sat, Dec 09, 2017 at 10:22:58AM -0800, Florian Fainelli wrote:
> On 12/08/2017 08:44 AM, Russell King - ARM Linux wrote:
> > On Fri, Dec 08, 2017 at 05:17:14PM +0100, Andrew Lunn wrote:
> >> Hi Russell
> >>
> >>> There is an open question whether there should be generic helpers for
> >>> this. Generic helpers would mean:
> >>>
> >>> - Additional couple of function pointers in phy_driver to read/write the
> >>> paging register. This has the restriction that there must only be one
> >>> paging register.
> >>
> >> I must be missing something. I don't see why there is this
> >> restriction. Don't we just need
> >>
> >> int phy_get_page(phydev);
> >> int phy_set_page(phydev, page);
> >
> > The restriction occurs because a PHY may have several different
> > registers, and knowing which of the registers need touching becomes an
> > issue. We wouldn't want these accessors to needlessly access several
> > registers each and every time we requested an access to the page
> > register.
> >
> > There's also the issue of whether an "int" or whatever type we choose to
> > pass the "page" around is enough bits. I haven't surveyed all the PHY
> > drivers yet to know the answer to that.
>
> I have not come across a PHY yet that required writing a page across two
> 16-bit quantities, in general, the page fits within less than 16-bit
> actually to fit within one MDIO write. That does not mean it cannot
> exist obviously, but having about 32-bit x pages of address space within
> a PHY sounds a bit extreme.
True, and phylib at the moment contains nothing beyond a single register.
I was thinking more of paging bits across several registers - such a case
would not lend itself well to this implementation as you'd have to read
every paging-capable register and write every paging capable register in
the phy_driver page accessor methods.
The good news is, having read through several drivers that contain the
caseless "page" string, there are no drivers that need anything but a
simple paging case, so it's not a concern. Those which seem to use
page accesses are:
at803x: this only uses a single bit in a register for one access.
dp83640: looks like it implements its own locking and banks registers
0x10-0x1e. Multiple accesses throughout the driver.
marvell: we know about this one which is the problem case.
microchip: looks like it banks the registers 0x10-0x1e, and uses this
for mdix control.
mscc: looks like it banks the registers 0x10-0x1e. Several accesses
throughout the driver, some under the phydev lock but others
unclear whether they are locked. Could be a problem.
realtek: looks like it banks the registers 0x10-0x1e. Probably racy -
interrupt handling uses paged accesses which may run in a
threaded interrupt handler.
vitesse: "/* map extended registers set 0x10 - 0x1e */" in one place
for mdix control via config_aneg.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
