| Message ID | 20210406141819.1025864-1-dqfext@gmail.com |
|---|---|
| Headers | show |
| Series | MT7530 interrupt support | expand |
On Tue, Apr 06, 2021 at 10:18:16PM +0800, DENG Qingfang wrote: > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. Do you know if this PHY is available standalone? > +static int mt7531_phy_config_init(struct phy_device *phydev) > +{ > + mtk_phy_config_init(phydev); > + > + /* PHY link down power saving enable */ > + phy_set_bits(phydev, 0x17, BIT(4)); > + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300); > + > + /* Set TX Pair delay selection */ > + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404); > + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404); This gets me worried about RGMII delays. We have had bad backwards compatibility problems with PHY drivers which get RGMII delays wrong. Since this is an internal PHY, i suggest you add a test to the beginning of mt7531_phy_config_init(): if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL) return -EINVAL; We can then solve RGMII problems when somebody actually needs RGMII. Andrew
On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote:
> Add support for MT7530 interrupt controller to handle internal PHYs.
Are the interrupts purely PHY interrupts? Or are there some switch
operation interrupts, which are currently not used?
I'm just wondering if it is correct to so closely tie interrupts and
MDIO together.
Andrew
On Tue, Apr 6, 2021 at 11:21 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Apr 06, 2021 at 10:18:16PM +0800, DENG Qingfang wrote: > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > > Do you know if this PHY is available standalone? Not that I know of. > > > +static int mt7531_phy_config_init(struct phy_device *phydev) > > +{ > > + mtk_phy_config_init(phydev); > > + > > + /* PHY link down power saving enable */ > > + phy_set_bits(phydev, 0x17, BIT(4)); > > + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, 0xc6, 0x300); > > + > > + /* Set TX Pair delay selection */ > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x13, 0x404); > > + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x14, 0x404); > > This gets me worried about RGMII delays. We have had bad backwards > compatibility problems with PHY drivers which get RGMII delays wrong. > > Since this is an internal PHY, i suggest you add a test to the > beginning of mt7531_phy_config_init(): > > if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL) > return -EINVAL; Okay. Will add it to v2. > > We can then solve RGMII problems when somebody actually needs RGMII. > > Andrew
On Tue, Apr 6, 2021 at 11:30 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote: > > Add support for MT7530 interrupt controller to handle internal PHYs. > > Are the interrupts purely PHY interrupts? Or are there some switch > operation interrupts, which are currently not used? There are other switch operations interrupts as well, such as VLAN member violation, switch ACL hit. > > I'm just wondering if it is correct to so closely tie interrupts and > MDIO together. > > Andrew
Hi, Qingfang: DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > The initialization procedure is from the vendor driver, but due to lack > of documentation, the function of some register values remains unknown. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > drivers/net/phy/Kconfig | 5 ++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 115 insertions(+) > create mode 100644 drivers/net/phy/mediatek.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index a615b3660b05..edd858cec9ec 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > Transceiver. > > +config MEDIATEK_PHY There are many Mediatek phy drivers in [1], so use a specific name. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6 Regards, Chun-Kuang. > + tristate "MediaTek PHYs" > + help > + Supports the MediaTek switch integrated PHYs. > + > config MICREL_PHY > tristate "Micrel PHYs" > help
On Tue, Apr 06, 2021 at 11:39:12PM +0800, DENG Qingfang wrote: > On Tue, Apr 6, 2021 at 11:30 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Tue, Apr 06, 2021 at 10:18:17PM +0800, DENG Qingfang wrote: > > > Add support for MT7530 interrupt controller to handle internal PHYs. > > > > Are the interrupts purely PHY interrupts? Or are there some switch > > operation interrupts, which are currently not used? > > There are other switch operations interrupts as well, such as VLAN > member violation, switch ACL hit. O.K. So that makes it similar to the mv88e6xxx. With that driver, i kept interrupt setup and mdio setup separate. I add the interrupt controller first, and then do mdio setup, calling a helper to map the PHY interrupts and assign them to bus->irq[]. That gives you a cleaner structure when you start using the other interrupts. Andrew
On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote: > > Hi, Qingfang: > > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > > Transceiver. > > > > +config MEDIATEK_PHY > > There are many Mediatek phy drivers in [1], so use a specific name. So "MEDIATEK_MT7530_PHY" should be okay? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6 > > Regards, > Chun-Kuang.
On Tue, Apr 6, 2021 at 11:49 PM Andrew Lunn <andrew@lunn.ch> wrote: > O.K. So that makes it similar to the mv88e6xxx. With that driver, i > kept interrupt setup and mdio setup separate. I add the interrupt > controller first, and then do mdio setup, calling a helper to map the > PHY interrupts and assign them to bus->irq[]. > > That gives you a cleaner structure when you start using the other > interrupts. Okay. Will split the function in v2. Thanks. > > Andrew
On Tue, Apr 06, 2021 at 11:47:08PM +0800, Chun-Kuang Hu wrote: > Hi, Qingfang: > > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > > The initialization procedure is from the vendor driver, but due to lack > > of documentation, the function of some register values remains unknown. > > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > --- > > drivers/net/phy/Kconfig | 5 ++ > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 115 insertions(+) > > create mode 100644 drivers/net/phy/mediatek.c > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > index a615b3660b05..edd858cec9ec 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > > Transceiver. > > > > +config MEDIATEK_PHY > > There are many Mediatek phy drivers in [1], so use a specific name. Those are generic PHY drivers, where as this patch is add a PHY driver. The naming used in this patch is consistent with other PHY drivers. So i'm happy with this patch in this respect. PHY drivers have been around a lot longer than generic PHY drivers. So i would actually say the generic PHY driver naming should make it clear they are generic PHYs, not PHYs. But lets not bike shed about this too much. Andrew
On Tue, Apr 06, 2021 at 11:57:14PM +0800, DENG Qingfang wrote: > On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote: > > > > Hi, Qingfang: > > > > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > > > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > > > Transceiver. > > > > > > +config MEDIATEK_PHY > > > > There are many Mediatek phy drivers in [1], so use a specific name. > > So "MEDIATEK_MT7530_PHY" should be okay? No. MEDIATEK_PHY is consistent with other PHY drivers. Please leave it as it is. And with time, we expect other devices will be supported by this driver, so having MT7530 in the name would be confusing. Andrew
DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午11:57寫道： > > On Tue, Apr 6, 2021 at 11:47 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote: > > > > Hi, Qingfang: > > > > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > > > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > > > Transceiver. > > > > > > +config MEDIATEK_PHY > > > > There are many Mediatek phy drivers in [1], so use a specific name. > > So "MEDIATEK_MT7530_PHY" should be okay? This is ok, but this name looks only for one SoC. MEDIATEK_ETHERNET_PHY could support more SoC, how do you think? Regards, Chun-Kuang. > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/mediatek?h=v5.12-rc6 > > > > Regards, > > Chun-Kuang.
Andrew Lunn <andrew@lunn.ch> 於 2021年4月7日 週三 上午12:02寫道： > > On Tue, Apr 06, 2021 at 11:47:08PM +0800, Chun-Kuang Hu wrote: > > Hi, Qingfang: > > > > DENG Qingfang <dqfext@gmail.com> 於 2021年4月6日 週二 下午10:19寫道： > > > > > > Add support for MediaTek PHYs found in MT7530 and MT7531 switches. > > > The initialization procedure is from the vendor driver, but due to lack > > > of documentation, the function of some register values remains unknown. > > > > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > > --- > > > drivers/net/phy/Kconfig | 5 ++ > > > drivers/net/phy/Makefile | 1 + > > > drivers/net/phy/mediatek.c | 109 +++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 115 insertions(+) > > > create mode 100644 drivers/net/phy/mediatek.c > > > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > > index a615b3660b05..edd858cec9ec 100644 > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -207,6 +207,11 @@ config MARVELL_88X2222_PHY > > > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > > > Transceiver. > > > > > > +config MEDIATEK_PHY > > > > There are many Mediatek phy drivers in [1], so use a specific name. > > Those are generic PHY drivers, where as this patch is add a PHY > driver. The naming used in this patch is consistent with other PHY > drivers. So i'm happy with this patch in this respect. > > PHY drivers have been around a lot longer than generic PHY drivers. So > i would actually say the generic PHY driver naming should make it > clear they are generic PHYs, not PHYs. > OK, so just ignore my comment. > But lets not bike shed about this too much. > > Andrew