| Message ID | 20180528174752.6806-2-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Realtek SMI RTL836x DSA driver | expand |
On Mon, May 28, 2018 at 07:47:49PM +0200, Linus Walleij wrote: > The RTL8366RB is an ASIC with five internal PHYs for > LAN0..LAN3 and WAN. The PHYs are spawn off the main > device so they can be handled in a distributed manner > by the Realtek PHY driver. All that is really needed > is the power save feature enablement and letting the > PHY driver core pick up the IRQ from the switch chip. > > Cc: Antti Seppälä <a.seppala@gmail.com> > Cc: Roman Yeryomin <roman@advem.lv> > Cc: Colin Leitner <colin.leitner@googlemail.com> > Cc: Gabor Juhos <juhosg@openwrt.org> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog RFCv1->RFCv2: > - No real changes. > --- > drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 9f48ecf9c627..21624d1c9d38 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -37,6 +37,9 @@ > #define RTL8201F_ISR 0x1e > #define RTL8201F_IER 0x13 > > +#define RTL8366RB_POWER_SAVE 0x21 > +#define RTL8366RB_POWER_SAVE_ON 0x1000 > + > MODULE_DESCRIPTION("Realtek PHY driver"); > MODULE_AUTHOR("Johnson Leung"); > MODULE_LICENSE("GPL"); > @@ -145,6 +148,22 @@ static int rtl8211f_config_init(struct phy_device *phydev) > return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); > } > > +static int rtl8366rb_config_init(struct phy_device *phydev) > +{ > + int ret; > + u16 reg; > + > + ret = genphy_config_init(phydev); > + if (ret < 0) > + return ret; > + > + reg = phy_read(phydev, RTL8366RB_POWER_SAVE); > + reg |= RTL8366RB_POWER_SAVE_ON; > + phy_write(phydev, RTL8366RB_POWER_SAVE, reg); > + > + return 0; > +} > + > static struct phy_driver realtek_drvs[] = { > { > .phy_id = 0x00008201, > @@ -207,6 +226,18 @@ static struct phy_driver realtek_drvs[] = { > .resume = genphy_resume, > .read_page = rtl821x_read_page, > .write_page = rtl821x_write_page, > + }, { > + /* The main part of this DSA is in drivers/net/dsa */ Hi Linus I would not bother with this comment. We don't say, The main part of this driver is in drivers/net/ethernet/... PHY drivers should be completely separate to MAC drivers. Otherwise this looks good. Andrew > + .phy_id = 0x001cc961, > + .name = "RTL8366RB Gigabit Ethernet", > + .phy_id_mask = 0x001fffff, > + .features = PHY_GBIT_FEATURES, > + .flags = PHY_HAS_INTERRUPT, > + .config_aneg = &genphy_config_aneg, > + .config_init = &rtl8366rb_config_init, > + .read_status = &genphy_read_status, > + .suspend = genphy_suspend, > + .resume = genphy_resume, > }, > }; > > @@ -218,6 +249,7 @@ static struct mdio_device_id __maybe_unused realtek_tbl[] = { > { 0x001cc914, 0x001fffff }, > { 0x001cc915, 0x001fffff }, > { 0x001cc916, 0x001fffff }, > + { 0x001cc961, 0x001fffff }, > { } > }; > > -- > 2.17.0 >
Am 28.05.2018 um 19:47 schrieb Linus Walleij: > The RTL8366RB is an ASIC with five internal PHYs for > LAN0..LAN3 and WAN. The PHYs are spawn off the main > device so they can be handled in a distributed manner > by the Realtek PHY driver. All that is really needed > is the power save feature enablement and letting the > PHY driver core pick up the IRQ from the switch chip. > > Cc: Antti Seppälä <a.seppala@gmail.com> > Cc: Roman Yeryomin <roman@advem.lv> > Cc: Colin Leitner <colin.leitner@googlemail.com> > Cc: Gabor Juhos <juhosg@openwrt.org> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog RFCv1->RFCv2: > - No real changes. > --- > drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 9f48ecf9c627..21624d1c9d38 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -37,6 +37,9 @@ > #define RTL8201F_ISR 0x1e > #define RTL8201F_IER 0x13 > > +#define RTL8366RB_POWER_SAVE 0x21 Typically PHY register addresses are 5 bits wide, is 0x21 correct and I miss something? > +#define RTL8366RB_POWER_SAVE_ON 0x1000 Use BIT(12) instead? > + > MODULE_DESCRIPTION("Realtek PHY driver"); > MODULE_AUTHOR("Johnson Leung"); > MODULE_LICENSE("GPL"); > @@ -145,6 +148,22 @@ static int rtl8211f_config_init(struct phy_device *phydev) > return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); > } > > +static int rtl8366rb_config_init(struct phy_device *phydev) > +{ > + int ret; > + u16 reg; > + > + ret = genphy_config_init(phydev); > + if (ret < 0) > + return ret; > + > + reg = phy_read(phydev, RTL8366RB_POWER_SAVE); > + reg |= RTL8366RB_POWER_SAVE_ON; > + phy_write(phydev, RTL8366RB_POWER_SAVE, reg); return phy_set_bits(phydev, RTL8366RB_POWER_SAVE, RTL8366RB_POWER_SAVE_ON); would be easier. > + > + return 0; > +} > + > static struct phy_driver realtek_drvs[] = { > { > .phy_id = 0x00008201, > @@ -207,6 +226,18 @@ static struct phy_driver realtek_drvs[] = { > .resume = genphy_resume, > .read_page = rtl821x_read_page, > .write_page = rtl821x_write_page, > + }, { > + /* The main part of this DSA is in drivers/net/dsa */ > + .phy_id = 0x001cc961, > + .name = "RTL8366RB Gigabit Ethernet", > + .phy_id_mask = 0x001fffff, > + .features = PHY_GBIT_FEATURES, > + .flags = PHY_HAS_INTERRUPT, > + .config_aneg = &genphy_config_aneg, Can be omitted, genphy_config_aneg is the default since 00fde79532d6 "net: phy: core: use genphy version of callbacks read_status and config_aneg per default". > + .config_init = &rtl8366rb_config_init, > + .read_status = &genphy_read_status, Can be omitted, genphy_read_status is the default. > + .suspend = genphy_suspend, > + .resume = genphy_resume, > }, > }; > > @@ -218,6 +249,7 @@ static struct mdio_device_id __maybe_unused realtek_tbl[] = { > { 0x001cc914, 0x001fffff }, > { 0x001cc915, 0x001fffff }, > { 0x001cc916, 0x001fffff }, > + { 0x001cc961, 0x001fffff }, > { } > }; > >
On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> +#define RTL8366RB_POWER_SAVE 0x21 > Typically PHY register addresses are 5 bits wide, is 0x21 correct > and I miss something? If it is correct I don't know, but it appears in the vendor code: /*Power Saving*/ #define RTL8368S_POWER_SAVING_PAGE 0 #define RTL8368S_POWER_SAVING_REG 21 #define RTL8368S_POWER_SAVING_BIT_MSK 0x1000 Then: phySmiAddr = 0x8000 | (1<<(phyNo +RTL8368S_PHY_NO_OFFSET)) | ((RTL8368S_POWER_SAVING_PAGE<<RTL8368S_PHY_PAGE_OFFSET)&RTL8368S_PHY_PAGE_MASK) | (RTL8368S_POWER_SAVING_REG&RTL8368S_PHY_REG_MASK); retVal = rtl8368s_setAsicReg(phySmiAddr, 0); if (retVal != SUCCESS) return retVal; The PHYs are accessed here in memory area 0x8000. Fixed the rest, thanks! Yours, Linus Walleij
On Tue, May 29, 2018 at 10:01:14PM +0200, Linus Walleij wrote: > On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > >> +#define RTL8366RB_POWER_SAVE 0x21 > > > Typically PHY register addresses are 5 bits wide, is 0x21 correct > > and I miss something? Heiner is correct, MDIO only supports 32 register, when using clause 22. However, your device is not clause 22, it is its own thing. So one danger you have is that we put some checks in the generic code testing for values > 31, and return -EINVAL. I think you have two choices: 1) A comment explaining what is going on here, how 0x21 is valid in this context. And check the return value and give out a good warning which will point somebody in the right direction to notice this 0x21. 2) Move this into the DSA driver, which does not have this restriction. Andrew
Am 29.05.2018 um 22:01 schrieb Linus Walleij: > On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >>> +#define RTL8366RB_POWER_SAVE 0x21 > >> Typically PHY register addresses are 5 bits wide, is 0x21 correct >> and I miss something? > > If it is correct I don't know, but it appears in the vendor > code: > > /*Power Saving*/ > #define RTL8368S_POWER_SAVING_PAGE 0 > #define RTL8368S_POWER_SAVING_REG 21 This is a decimal number .. You use 0x21. > #define RTL8368S_POWER_SAVING_BIT_MSK 0x1000 > > Then: > > phySmiAddr = 0x8000 | (1<<(phyNo +RTL8368S_PHY_NO_OFFSET)) | > > ((RTL8368S_POWER_SAVING_PAGE<<RTL8368S_PHY_PAGE_OFFSET)&RTL8368S_PHY_PAGE_MASK) > | > (RTL8368S_POWER_SAVING_REG&RTL8368S_PHY_REG_MASK); > > retVal = rtl8368s_setAsicReg(phySmiAddr, 0); > if (retVal != SUCCESS) > return retVal; > > The PHYs are accessed here in memory area 0x8000. > > Fixed the rest, thanks! > > Yours, > Linus Walleij >
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 9f48ecf9c627..21624d1c9d38 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -37,6 +37,9 @@ #define RTL8201F_ISR 0x1e #define RTL8201F_IER 0x13 +#define RTL8366RB_POWER_SAVE 0x21 +#define RTL8366RB_POWER_SAVE_ON 0x1000 + MODULE_DESCRIPTION("Realtek PHY driver"); MODULE_AUTHOR("Johnson Leung"); MODULE_LICENSE("GPL"); @@ -145,6 +148,22 @@ static int rtl8211f_config_init(struct phy_device *phydev) return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); } +static int rtl8366rb_config_init(struct phy_device *phydev) +{ + int ret; + u16 reg; + + ret = genphy_config_init(phydev); + if (ret < 0) + return ret; + + reg = phy_read(phydev, RTL8366RB_POWER_SAVE); + reg |= RTL8366RB_POWER_SAVE_ON; + phy_write(phydev, RTL8366RB_POWER_SAVE, reg); + + return 0; +} + static struct phy_driver realtek_drvs[] = { { .phy_id = 0x00008201, @@ -207,6 +226,18 @@ static struct phy_driver realtek_drvs[] = { .resume = genphy_resume, .read_page = rtl821x_read_page, .write_page = rtl821x_write_page, + }, { + /* The main part of this DSA is in drivers/net/dsa */ + .phy_id = 0x001cc961, + .name = "RTL8366RB Gigabit Ethernet", + .phy_id_mask = 0x001fffff, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .config_aneg = &genphy_config_aneg, + .config_init = &rtl8366rb_config_init, + .read_status = &genphy_read_status, + .suspend = genphy_suspend, + .resume = genphy_resume, }, }; @@ -218,6 +249,7 @@ static struct mdio_device_id __maybe_unused realtek_tbl[] = { { 0x001cc914, 0x001fffff }, { 0x001cc915, 0x001fffff }, { 0x001cc916, 0x001fffff }, + { 0x001cc961, 0x001fffff }, { } };
The RTL8366RB is an ASIC with five internal PHYs for LAN0..LAN3 and WAN. The PHYs are spawn off the main device so they can be handled in a distributed manner by the Realtek PHY driver. All that is really needed is the power save feature enablement and letting the PHY driver core pick up the IRQ from the switch chip. Cc: Antti Seppälä <a.seppala@gmail.com> Cc: Roman Yeryomin <roman@advem.lv> Cc: Colin Leitner <colin.leitner@googlemail.com> Cc: Gabor Juhos <juhosg@openwrt.org> Cc: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog RFCv1->RFCv2: - No real changes. --- drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) -- 2.17.0