| Message ID | 20211126103833.3609945-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: micrel: Add config_init for LAN8814 | expand |
On 26.11.2021 11:38, Horatiu Vultur wrote: > Add config_init for LAN8814. This function is required for the following > reasons: > - we need to make sure that the PHY is reset, > - disable ANEG with QSGMII PCS Host side > - swap the MDI-X A,B transmit so that there will not be any link flip-flaps > when the PHY gets a link. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > drivers/net/phy/micrel.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 44a24b99c894..f080312032cf 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1565,6 +1565,14 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, > #define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA 0x17 > #define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC 0x4000 > > +#define LAN8814_QSGMII_SOFT_RESET 0x43 > +#define LAN8814_QSGMII_SOFT_RESET_BIT BIT(0) > +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG 0x13 > +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA BIT(3) > +#define LAN8814_ALIGN_SWAP 0x4a > +#define LAN8814_ALIGN_TX_A_B_SWAP 0x1 > +#define LAN8814_ALIGN_TX_A_B_SWAP_MASK GENMASK(2, 0) > + > #define LAN8804_ALIGN_SWAP 0x4a > #define LAN8804_ALIGN_TX_A_B_SWAP 0x1 > #define LAN8804_ALIGN_TX_A_B_SWAP_MASK GENMASK(2, 0) > @@ -1601,6 +1609,29 @@ static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr, > return 0; > } > > +static int lan8814_config_init(struct phy_device *phydev) > +{ > + int val; > + > + /* Reset the PHY */ > + val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET); > + val |= LAN8814_QSGMII_SOFT_RESET_BIT; > + lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val); > + > + /* Disable ANEG with QSGMII PCS Host side */ > + val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG); > + val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA; > + lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val); > + > + /* MDI-X setting for swap A,B transmit */ > + val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP); > + val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK; > + val |= LAN8814_ALIGN_TX_A_B_SWAP; > + lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val); > + Not directly related to just this patch: Did you consider implementing the read_page and write_page PHY driver callbacks? Then you could use phylib functions like phy_modify_paged et al and you wouldn't have to open-code the paged register operations. I think write_page would just be phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); and read_page phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL); > + return 0; > +} > + > static int lan8804_config_init(struct phy_device *phydev) > { > int val; > @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = { > .phy_id = PHY_ID_LAN8814, > .phy_id_mask = MICREL_PHY_ID_MASK, > .name = "Microchip INDY Gigabit Quad PHY", > + .config_init = lan8814_config_init, > .driver_data = &ksz9021_type, > .probe = kszphy_probe, > .soft_reset = genphy_soft_reset, >
On Fri, Nov 26, 2021 at 12:57:33PM +0100, Heiner Kallweit wrote: > Not directly related to just this patch: > Did you consider implementing the read_page and write_page PHY driver > callbacks? Then you could use phylib functions like phy_modify_paged et al > and you wouldn't have to open-code the paged register operations. > > I think write_page would just be > phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); > phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); > phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); > > and read_page > phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL); Remember that read_page() and write_page() must be implemented using the unlocked accessors since the MDIO bus lock is held prior to calling them. So these should be __phy_write() and __phy_read(). The use of the helpers you mention above also bring greater safety to the read-modify-write accesses, since with these accessors, the whole set of accesses are done while holding the bus lock. Thanks.
> - swap the MDI-X A,B transmit so that there will not be any link flip-flaps > when the PHY gets a link. Isn't this a board issue, rather than generic? Or does the datasheet have the pins labelled wrongly and all boards following the datasheet are wrong? Andrew
The 11/26/2021 12:57, Heiner Kallweit wrote: Hi Heiner, > > On 26.11.2021 11:38, Horatiu Vultur wrote: > > > > +static int lan8814_config_init(struct phy_device *phydev) > > +{ > > + int val; > > + > > + /* Reset the PHY */ > > + val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET); > > + val |= LAN8814_QSGMII_SOFT_RESET_BIT; > > + lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val); > > + > > + /* Disable ANEG with QSGMII PCS Host side */ > > + val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG); > > + val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA; > > + lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val); > > + > > + /* MDI-X setting for swap A,B transmit */ > > + val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP); > > + val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK; > > + val |= LAN8814_ALIGN_TX_A_B_SWAP; > > + lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val); > > + > > Not directly related to just this patch: > Did you consider implementing the read_page and write_page PHY driver > callbacks? Then you could use phylib functions like phy_modify_paged et al > and you wouldn't have to open-code the paged register operations. > > I think write_page would just be > phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); > phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); > phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); > > and read_page > phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL); Thanks for the suggestion, but unfortunately it would not work. The reason is that in the callback 'write_page' I don't actually get also the address in the page that is needed to be read/write. If this issue would be fixed, then there is another problem. To read/write the data in the extended page is required to access the register LAN_EXT_PAGE_ACCESS_ADDRESS_DATA. But that would not happen when using the phy_read_paged, it would read actually the register in page 0. If I have missed something, please let me know. > > > + return 0; > > +} > > + > > static int lan8804_config_init(struct phy_device *phydev) > > { > > int val; > > @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = { > > .phy_id = PHY_ID_LAN8814, > > .phy_id_mask = MICREL_PHY_ID_MASK, > > .name = "Microchip INDY Gigabit Quad PHY", > > + .config_init = lan8814_config_init, > > .driver_data = &ksz9021_type, > > .probe = kszphy_probe, > > .soft_reset = genphy_soft_reset, > > >
The 11/26/2021 16:15, Andrew Lunn wrote: Hi Andrew, > > > - swap the MDI-X A,B transmit so that there will not be any link flip-flaps > > when the PHY gets a link. > > Isn't this a board issue, rather than generic? Or does the datasheet > have the pins labelled wrongly and all boards following the datasheet > are wrong? From my understanding it is not a board issue. This is needed in case the link partners can't do the A/B swapping based on MDI/MDIX. It would not harm if this is set also for link partners that can do this. > > Andrew
On 29.11.2021 09:29, Horatiu Vultur wrote: > The 11/26/2021 12:57, Heiner Kallweit wrote: > > Hi Heiner, > >> >> On 26.11.2021 11:38, Horatiu Vultur wrote: >>> >>> +static int lan8814_config_init(struct phy_device *phydev) >>> +{ >>> + int val; >>> + >>> + /* Reset the PHY */ >>> + val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET); >>> + val |= LAN8814_QSGMII_SOFT_RESET_BIT; >>> + lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val); >>> + >>> + /* Disable ANEG with QSGMII PCS Host side */ >>> + val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG); >>> + val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA; >>> + lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val); >>> + >>> + /* MDI-X setting for swap A,B transmit */ >>> + val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP); >>> + val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK; >>> + val |= LAN8814_ALIGN_TX_A_B_SWAP; >>> + lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val); >>> + >> >> Not directly related to just this patch: >> Did you consider implementing the read_page and write_page PHY driver >> callbacks? Then you could use phylib functions like phy_modify_paged et al >> and you wouldn't have to open-code the paged register operations. >> >> I think write_page would just be >> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page); >> phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr); >> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC)); >> >> and read_page >> phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL); > > Thanks for the suggestion, but unfortunately it would not work. > The reason is that in the callback 'write_page' I don't actually get > also the address in the page that is needed to be read/write. > > If this issue would be fixed, then there is another problem. > To read/write the data in the extended page is required to access the > register LAN_EXT_PAGE_ACCESS_ADDRESS_DATA. But that would not happen > when using the phy_read_paged, it would read actually the register in > page 0. > > If I have missed something, please let me know. > Right, after reading the sequence in lanphy_read_page_reg() more carefully I agree. This PHY re-uses the more complex MMD access mechanism for paged access. >> >>> + return 0; >>> +} >>> + >>> static int lan8804_config_init(struct phy_device *phydev) >>> { >>> int val; >>> @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = { >>> .phy_id = PHY_ID_LAN8814, >>> .phy_id_mask = MICREL_PHY_ID_MASK, >>> .name = "Microchip INDY Gigabit Quad PHY", >>> + .config_init = lan8814_config_init, >>> .driver_data = &ksz9021_type, >>> .probe = kszphy_probe, >>> .soft_reset = genphy_soft_reset, >>> >> >
The 11/26/2021 11:38, Horatiu Vultur wrote: Hi, Sorry for reviving this old thread. I can see this patch was marked as "Changes Requested" [1]. The change that Heiner proposed, will not worked as we already discussed. It is using a different mechanism to access extend pages. Should I just try to resend the patch or is possible to get this one? [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211126103833.3609945-1-horatiu.vultur@microchip.com/
On Wed, 22 Dec 2021 12:48:20 +0100 Horatiu Vultur wrote: > The 11/26/2021 11:38, Horatiu Vultur wrote: > > Sorry for reviving this old thread. I can see this patch was marked as > "Changes Requested" [1]. The change that Heiner proposed, will not worked as > we already discussed. It is using a different mechanism to access extend > pages. > Should I just try to resend the patch or is possible to get this one? > > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211126103833.3609945-1-horatiu.vultur@microchip.com/ Please resend (preferably with a short note on why in the change log).
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 44a24b99c894..f080312032cf 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1565,6 +1565,14 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, #define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA 0x17 #define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC 0x4000 +#define LAN8814_QSGMII_SOFT_RESET 0x43 +#define LAN8814_QSGMII_SOFT_RESET_BIT BIT(0) +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG 0x13 +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA BIT(3) +#define LAN8814_ALIGN_SWAP 0x4a +#define LAN8814_ALIGN_TX_A_B_SWAP 0x1 +#define LAN8814_ALIGN_TX_A_B_SWAP_MASK GENMASK(2, 0) + #define LAN8804_ALIGN_SWAP 0x4a #define LAN8804_ALIGN_TX_A_B_SWAP 0x1 #define LAN8804_ALIGN_TX_A_B_SWAP_MASK GENMASK(2, 0) @@ -1601,6 +1609,29 @@ static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr, return 0; } +static int lan8814_config_init(struct phy_device *phydev) +{ + int val; + + /* Reset the PHY */ + val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET); + val |= LAN8814_QSGMII_SOFT_RESET_BIT; + lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val); + + /* Disable ANEG with QSGMII PCS Host side */ + val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG); + val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA; + lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val); + + /* MDI-X setting for swap A,B transmit */ + val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP); + val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK; + val |= LAN8814_ALIGN_TX_A_B_SWAP; + lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val); + + return 0; +} + static int lan8804_config_init(struct phy_device *phydev) { int val; @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = { .phy_id = PHY_ID_LAN8814, .phy_id_mask = MICREL_PHY_ID_MASK, .name = "Microchip INDY Gigabit Quad PHY", + .config_init = lan8814_config_init, .driver_data = &ksz9021_type, .probe = kszphy_probe, .soft_reset = genphy_soft_reset,
Add config_init for LAN8814. This function is required for the following reasons: - we need to make sure that the PHY is reset, - disable ANEG with QSGMII PCS Host side - swap the MDI-X A,B transmit so that there will not be any link flip-flaps when the PHY gets a link. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- drivers/net/phy/micrel.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)