| Message ID | 20240524140706.359537-1-ramon.nordin.rodriguez@ferroamp.se (mailing list archive) |
|---|---|
| Headers | show |
| Series | phy: microchip_t1s: lan865x rev.b1 support | expand |
> Far as I can tell the phy-driver cannot access some of the regs necessary > for probing the hardware and performing the init/fixup without going > over the spi interface. > The MMDCTRL register (used with indirect access) can address > > * PMA - mms 3 > * PCS - mms 2 > * Vendor specific / PLCA - mms 4 > > This driver needs to access mms (memory map seleector) > * mac registers - mms 1, > * vendor specific / PLCA - mms 4 > * vencor specific - mms 10 In general, a MAC should not be touching the PHY, and the PHY should not be touching the MAC. This rule is because you should not assume you have a specific MAC+PHY pair. However, this is one blob of silicon, so we can relax that a bit if needed. So it sounds like Microchip have mixed up the register address spaces :-( I guess this also means there is no discrete version of this PHY, because where would these registers be? Do any of the registers in the wrong address space need to be poked at runtime? By that i mean config_aneg(), read_status(). Or are they only needed around the time the PHY is probed? How critical is the ordering? Could we have the Microchip MAC driver probe. It instantiates the TC6 framework which registers the MDIO bus and probes the PHY. Can the MAC driver then complete the PHY setup using the registers in the wrong address space? Does it need to access any PHY registers in the correct address space? The MAC driver should be able to do this before phy_start() Does MMS 0 register 1 "PHY Identification Register" give enough information to know it is a B1 PHY? The standard suggests it is a straight copy of PHY registers 2 and 3. So the MAC driver does not need to touch PHY registers, we are not totally violating the layering... Andrew
On Fri, May 24, 2024 at 04:07:05PM +0200, Ramón Nordin Rodriguez wrote: > Hi, > Let me first prepend this submission with 4 points: > > * this is not in a merge-ready state > * some code has been copied from the ongoing oa_tc6 work by Parthiban > * this has to interop with code not yet merged (oa_tc6) > * Microchip is looking into if rev.b0 can use the rev.b1 init procedure > > The ongoing work by Parthiban Veerasooran is probably gonna get at least > one more revision > (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/) > > I'm publishing this early as it could benefit some of the discussions in > the oa_tc6 threads, as well as giving other devs the possibility > massaging things to a state where they can use the rev.b1 chip (rev.b0 > is eol). > And I need feedback on how to wrap this up. > > Far as I can tell the phy-driver cannot access some of the regs necessary > for probing the hardware and performing the init/fixup without going > over the spi interface. > The MMDCTRL register (used with indirect access) can address > > * PMA - mms 3 > * PCS - mms 2 > * Vendor specific / PLCA - mms 4 > > This driver needs to access mms (memory map seleector) > * mac registers - mms 1, > * vendor specific / PLCA - mms 4 > * vencor specific - mms 10 > > Far as I can tell, mms 1 and 10 are only accessible via spi. In the > oa_tc6 patches this is enabled by the oa_tc6 framework by populating the > mdiobus->read/write_c45 funcs. > > In order to access any mms I needed I added the following change in the > oa_tc6.c module > > static int oa_tc6_get_phy_c45_mms(int devnum) > { > + if(devnum & BIT(31)) > + return devnum & GENMASK(30, 0); > > Which corresponds to the 'mms | BIT(31)' snippets in this commit, this > is really not how things should be handled, and I need input on how to > proceed here. So if bit 31 of the devnum is set, then the other bits specify the MMS instead of the MMD. I'm not sure we want to overload the PHY interface in this way. We have been down this path before with the MDIO bus read/write methods being used for both C22 and C45 accesses, and it created problems, so I don't think we want to repeat that mistake by doing the same thing here. There's a comment in the original patches etc about the PHY being discovered via C22, and then not using the direct accesses to the C45 register space. I'm wondering whether we should split phydev->is_c45 to be phydev->probed_c45 / phydev->use_c45. The former gets used during bus scanning and probe time to determine how we match the device driver to the phydev. The latter gets used _only_ to determine whether the read/write_mmd ops use direct mode or indirect mode. Before the driver probe is called, we should do: phydev->use_mmd = phydev->probed_c45; to ensure that todays behaviour is preserved. Then, provide a function, maybe, phy_use_direct_c45(phydev) which will set this phydev->use_mmd flag, and phy_use_indirect_c45(phydev) which will clear it. phy_use_direct_c45() should also check whether the MDIO bus that is attached supports C45 access, and return an error if not. That will give you the ability to use the direct access method where necessary. There's a comment in the referred to code: + /* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and + * C45 registers space. If the PHY is discovered via C22 bus protocol it + * assumes it uses C22 protocol and always uses C22 registers indirect + * access to access C45 registers. This is because, we don't have a + * clean separation between C22/C45 register space and C22/C45 MDIO bus + * protocols. Resulting, PHY C45 registers direct access can't be used + * which can save multiple SPI bus access. To support this feature, PHY + * drivers can set .read_mmd/.write_mmd in the PHY driver to call + * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c + */ which I don't really understand. It claims that C45 direct access "saves" multiple SPI bus accesses. However, C45 indirect access requires: 1. A C22 write to the MMD control register 2. A C22 write to the MMD data register 3. Another C22 write to the MMD control register 4. A c22 read or write to access the actual data. Do four C22 bus transactions over SPI require more or less SPI bus accesses than a single C45 bus transaction over SPI? I suspect not, which makes the comment above factually incorrect. If we have direct C45 access working, does that remove the need to have this special bit-31 to signal MMS access requirement? Thanks.
Hi Ramon, On 24/05/24 7:37 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, > Let me first prepend this submission with 4 points: > > * this is not in a merge-ready state > * some code has been copied from the ongoing oa_tc6 work by Parthiban > * this has to interop with code not yet merged (oa_tc6) > * Microchip is looking into if rev.b0 can use the rev.b1 init procedure I will try to get the info on this as soon as possible. I have asked for it to avoid the below complications only. I don't want to proceed with these patches until getting a clarity to avoid unnecessary confusions. Best regards, Parthiban V > > The ongoing work by Parthiban Veerasooran is probably gonna get at least > one more revision > (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/) > > I'm publishing this early as it could benefit some of the discussions in > the oa_tc6 threads, as well as giving other devs the possibility > massaging things to a state where they can use the rev.b1 chip (rev.b0 > is eol). > And I need feedback on how to wrap this up. > > Far as I can tell the phy-driver cannot access some of the regs necessary > for probing the hardware and performing the init/fixup without going > over the spi interface. > The MMDCTRL register (used with indirect access) can address > > * PMA - mms 3 > * PCS - mms 2 > * Vendor specific / PLCA - mms 4 > > This driver needs to access mms (memory map seleector) > * mac registers - mms 1, > * vendor specific / PLCA - mms 4 > * vencor specific - mms 10 > > Far as I can tell, mms 1 and 10 are only accessible via spi. In the > oa_tc6 patches this is enabled by the oa_tc6 framework by populating the > mdiobus->read/write_c45 funcs. > > In order to access any mms I needed I added the following change in the > oa_tc6.c module > > static int oa_tc6_get_phy_c45_mms(int devnum) > { > + if(devnum & BIT(31)) > + return devnum & GENMASK(30, 0); > > Which corresponds to the 'mms | BIT(31)' snippets in this commit, this > is really not how things should be handled, and I need input on how to > proceed here. > > Here we get into a weird spot, this driver will need changes in the > oa_tc6 submission, but it's weird to submit support for yet another phy > with that patchset (in my opinion). > > This has been tested with a lan8650 rev.b1 chip on one end and a lan8670 > usb eval board on the other end. Performance is rather lacking, the > rev.b0 reaches close to the 10Mbit/s limit, but b.1 only gets about > ~4Mbit/s, with the same results when PLCA enabled or disabled. > > I suggest that this patch is left to brew until the oa_tc6 changes are > accepted, at which time this is fixed up. > > Ramón Nordin Rodriguez (1): > net: phy: microchip_t1s: enable lan865x revb1 > > drivers/net/phy/microchip_t1s.c | 189 ++++++++++++++++++++++++++++---- > 1 file changed, 166 insertions(+), 23 deletions(-) > > -- > 2.43.0 >
Hi Andrew, On 24/05/24 8:20 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Far as I can tell the phy-driver cannot access some of the regs necessary >> for probing the hardware and performing the init/fixup without going >> over the spi interface. >> The MMDCTRL register (used with indirect access) can address >> >> * PMA - mms 3 >> * PCS - mms 2 >> * Vendor specific / PLCA - mms 4 >> >> This driver needs to access mms (memory map seleector) >> * mac registers - mms 1, >> * vendor specific / PLCA - mms 4 >> * vencor specific - mms 10 > > In general, a MAC should not be touching the PHY, and the PHY should > not be touching the MAC. This rule is because you should not assume > you have a specific MAC+PHY pair. However, this is one blob of > silicon, so we can relax that a bit if needed. > > So it sounds like Microchip have mixed up the register address spaces > :-( > > I guess this also means there is no discrete version of this PHY, > because where would these registers be? > > Do any of the registers in the wrong address space need to be poked at > runtime? By that i mean config_aneg(), read_status(). Or are they only > needed around the time the PHY is probed? > > How critical is the ordering? Could we have the Microchip MAC driver > probe. It instantiates the TC6 framework which registers the MDIO bus > and probes the PHY. Can the MAC driver then complete the PHY setup > using the registers in the wrong address space? Does it need to access > any PHY registers in the correct address space? The MAC driver should > be able to do this before phy_start() > > Does MMS 0 register 1 "PHY Identification Register" give enough > information to know it is a B1 PHY? The standard suggests it is a > straight copy of PHY registers 2 and 3. So the MAC driver does not > need to touch PHY registers, we are not totally violating the > layering... I completely agree with all your above points. As I told already, I am in talk with our design team about this complications by the time this Rev.B1 support has been posted. Will try to get the clarity as soon as possible. Sorry for the inconvenience. So I would recommend to go with Rev.B0 support now as "CD disable if PLCA is enabled" fix which gives stable performance until we get the clarity on B1. So that we can evaluate the TC6 framework (oa_tc6.c) to have a initial/basic version in the mainline first. Best regards, Parthiban V > > Andrew >
Hi, On 24/05/24 9:00 pm, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, May 24, 2024 at 04:07:05PM +0200, Ramón Nordin Rodriguez wrote: >> Hi, >> Let me first prepend this submission with 4 points: >> >> * this is not in a merge-ready state >> * some code has been copied from the ongoing oa_tc6 work by Parthiban >> * this has to interop with code not yet merged (oa_tc6) >> * Microchip is looking into if rev.b0 can use the rev.b1 init procedure >> >> The ongoing work by Parthiban Veerasooran is probably gonna get at least >> one more revision >> (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/) >> >> I'm publishing this early as it could benefit some of the discussions in >> the oa_tc6 threads, as well as giving other devs the possibility >> massaging things to a state where they can use the rev.b1 chip (rev.b0 >> is eol). >> And I need feedback on how to wrap this up. >> >> Far as I can tell the phy-driver cannot access some of the regs necessary >> for probing the hardware and performing the init/fixup without going >> over the spi interface. >> The MMDCTRL register (used with indirect access) can address >> >> * PMA - mms 3 >> * PCS - mms 2 >> * Vendor specific / PLCA - mms 4 >> >> This driver needs to access mms (memory map seleector) >> * mac registers - mms 1, >> * vendor specific / PLCA - mms 4 >> * vencor specific - mms 10 >> >> Far as I can tell, mms 1 and 10 are only accessible via spi. In the >> oa_tc6 patches this is enabled by the oa_tc6 framework by populating the >> mdiobus->read/write_c45 funcs. >> >> In order to access any mms I needed I added the following change in the >> oa_tc6.c module >> >> static int oa_tc6_get_phy_c45_mms(int devnum) >> { >> + if(devnum & BIT(31)) >> + return devnum & GENMASK(30, 0); >> >> Which corresponds to the 'mms | BIT(31)' snippets in this commit, this >> is really not how things should be handled, and I need input on how to >> proceed here. > > So if bit 31 of the devnum is set, then the other bits specify the > MMS instead of the MMD. > > I'm not sure we want to overload the PHY interface in this way. We > have been down this path before with the MDIO bus read/write methods > being used for both C22 and C45 accesses, and it created problems, > so I don't think we want to repeat that mistake by doing the same > thing here. > > There's a comment in the original patches etc about the PHY being > discovered via C22, and then not using the direct accesses to the > C45 register space. I'm wondering whether we should split > phydev->is_c45 to be phydev->probed_c45 / phydev->use_c45. > > The former gets used during bus scanning and probe time to determine > how we match the device driver to the phydev. The latter gets used > _only_ to determine whether the read/write_mmd ops use direct mode > or indirect mode. > > Before the driver probe is called, we should do: > > phydev->use_mmd = phydev->probed_c45; > > to ensure that todays behaviour is preserved. Then, provide a > function, maybe, phy_use_direct_c45(phydev) which will set this > phydev->use_mmd flag, and phy_use_indirect_c45(phydev) which will > clear it. > > phy_use_direct_c45() should also check whether the MDIO bus that > is attached supports C45 access, and return an error if not. > > That will give you the ability to use the direct access method > where necessary. > > There's a comment in the referred to code: > > + /* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and > + * C45 registers space. If the PHY is discovered via C22 bus protocol it > + * assumes it uses C22 protocol and always uses C22 registers indirect > + * access to access C45 registers. This is because, we don't have a > + * clean separation between C22/C45 register space and C22/C45 MDIO bus > + * protocols. Resulting, PHY C45 registers direct access can't be used > + * which can save multiple SPI bus access. To support this feature, PHY > + * drivers can set .read_mmd/.write_mmd in the PHY driver to call > + * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c > + */ > > which I don't really understand. It claims that C45 direct access > "saves" multiple SPI bus accesses. However, C45 indirect access Sorry for the misunderstanding here. Probably I should have used "avoid" in the place of "save" might clear the things properly. The intention of this comment is, PHY C45 direct access will be faster than PHY C45 indirect access as PHY C45 indirect access needs 4 SPI bus access. If you agree, I will correct it in the next version. Best regards, Parthiban V > requires: > > 1. A C22 write to the MMD control register > 2. A C22 write to the MMD data register > 3. Another C22 write to the MMD control register > 4. A c22 read or write to access the actual data. > > Do four C22 bus transactions over SPI require more or less SPI bus > accesses than a single C45 bus transaction over SPI? I suspect not, > which makes the comment above factually incorrect. > > If we have direct C45 access working, does that remove the need to > have this special bit-31 to signal MMS access requirement? > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi, Let me first prepend this submission with 4 points: * this is not in a merge-ready state * some code has been copied from the ongoing oa_tc6 work by Parthiban * this has to interop with code not yet merged (oa_tc6) * Microchip is looking into if rev.b0 can use the rev.b1 init procedure The ongoing work by Parthiban Veerasooran is probably gonna get at least one more revision (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/) I'm publishing this early as it could benefit some of the discussions in the oa_tc6 threads, as well as giving other devs the possibility massaging things to a state where they can use the rev.b1 chip (rev.b0 is eol). And I need feedback on how to wrap this up. Far as I can tell the phy-driver cannot access some of the regs necessary for probing the hardware and performing the init/fixup without going over the spi interface. The MMDCTRL register (used with indirect access) can address * PMA - mms 3 * PCS - mms 2 * Vendor specific / PLCA - mms 4 This driver needs to access mms (memory map seleector) * mac registers - mms 1, * vendor specific / PLCA - mms 4 * vencor specific - mms 10 Far as I can tell, mms 1 and 10 are only accessible via spi. In the oa_tc6 patches this is enabled by the oa_tc6 framework by populating the mdiobus->read/write_c45 funcs. In order to access any mms I needed I added the following change in the oa_tc6.c module static int oa_tc6_get_phy_c45_mms(int devnum) { + if(devnum & BIT(31)) + return devnum & GENMASK(30, 0); Which corresponds to the 'mms | BIT(31)' snippets in this commit, this is really not how things should be handled, and I need input on how to proceed here. Here we get into a weird spot, this driver will need changes in the oa_tc6 submission, but it's weird to submit support for yet another phy with that patchset (in my opinion). This has been tested with a lan8650 rev.b1 chip on one end and a lan8670 usb eval board on the other end. Performance is rather lacking, the rev.b0 reaches close to the 10Mbit/s limit, but b.1 only gets about ~4Mbit/s, with the same results when PLCA enabled or disabled. I suggest that this patch is left to brew until the oa_tc6 changes are accepted, at which time this is fixed up. Ramón Nordin Rodriguez (1): net: phy: microchip_t1s: enable lan865x revb1 drivers/net/phy/microchip_t1s.c | 189 ++++++++++++++++++++++++++++---- 1 file changed, 166 insertions(+), 23 deletions(-)