| Message ID | 20220313194748.1705385-1-hauke@hauke-m.de |
|---|---|
| State | Under Review |
| Delegated to: | Hauke Mehrtens |
| Headers | show |
| Series | realtek: Fix kernel dependencies on CONFIG_MDIO_SMBUS | expand |
Hi, CONFIG_SFP should only depend on CONFIG_MDIO_SMBUS on the RTL93xx platforms. This is because only on those platforms there is hardware support for an SMBus controller which is used for the MDIO of the SFP ports. If we had known about the worning, then we would have tried to fix it by making CONFIG_SFP only on the 93xx platforms dependent on CONFIG_MDIO_SMBUS. How to do this is however not clear to me. Cheers, Birger On 13.03.22 20:47, Hauke Mehrtens wrote: > CONFIG_SFP depends on CONFIG_MDIO_SMBUS now and this depends on > CONFIG_I2C_SMBUS. Activate the missing kernel compile options. > > This fixes the following warning: > WARNING: unmet direct dependencies detected for MDIO_SMBUS > Depends on [n]: NETDEVICES [=y] && MDIO_DEVICE [=y] && MDIO_BUS [=y] && I2C_SMBUS [=n] > Selected by [y]: > - SFP [=y] && NETDEVICES [=y] && PHYLIB [=y] && I2C [=y] && PHYLINK [=y] && (HWMON [=y] || HWMON [=y]=n) > > Fixes: 45053b507c66 ("realtek: Add support for SFP EEPROM-access over SMBus") > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > target/linux/realtek/rtl838x/config-5.10 | 2 ++ > target/linux/realtek/rtl839x/config-5.10 | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/target/linux/realtek/rtl838x/config-5.10 b/target/linux/realtek/rtl838x/config-5.10 > index 8e27af95ba46..16964d94c967 100644 > --- a/target/linux/realtek/rtl838x/config-5.10 > +++ b/target/linux/realtek/rtl838x/config-5.10 > @@ -90,6 +90,7 @@ CONFIG_I2C_GPIO=y > CONFIG_I2C_MUX=y > # CONFIG_I2C_RTL9300 is not set > # CONFIG_I2C_MUX_RTL9300 is not set > +CONFIG_I2C_SMBUS=y > CONFIG_INITRAMFS_SOURCE="" > CONFIG_IRQCHIP=y > CONFIG_IRQ_DOMAIN=y > @@ -107,6 +108,7 @@ CONFIG_MDIO_BUS=y > CONFIG_MDIO_DEVICE=y > CONFIG_MDIO_DEVRES=y > CONFIG_MDIO_I2C=y > +CONFIG_MDIO_SMBUS=y > CONFIG_MEMFD_CREATE=y > CONFIG_MFD_SYSCON=y > CONFIG_MIGRATION=y > diff --git a/target/linux/realtek/rtl839x/config-5.10 b/target/linux/realtek/rtl839x/config-5.10 > index 5704b4265a3c..b3192d1866da 100644 > --- a/target/linux/realtek/rtl839x/config-5.10 > +++ b/target/linux/realtek/rtl839x/config-5.10 > @@ -86,6 +86,7 @@ CONFIG_I2C_BOARDINFO=y > CONFIG_I2C_GPIO=y > # CONFIG_I2C_RTL9300 is not set > # CONFIG_I2C_MUX_RTL9300 is not set > +CONFIG_I2C_SMBUS=y > CONFIG_INITRAMFS_SOURCE="" > CONFIG_IRQCHIP=y > CONFIG_IRQ_DOMAIN=y > @@ -104,6 +105,7 @@ CONFIG_MDIO_BUS=y > CONFIG_MDIO_DEVICE=y > CONFIG_MDIO_DEVRES=y > CONFIG_MDIO_I2C=y > +CONFIG_MDIO_SMBUS=y > CONFIG_MEMFD_CREATE=y > CONFIG_MFD_SYSCON=y > CONFIG_MIGRATION=y
On 3/14/22 06:56, Birger Koblitz wrote: > Hi, > > CONFIG_SFP should only depend on CONFIG_MDIO_SMBUS on the RTL93xx platforms. > This is because only on those platforms there is hardware support for an SMBus > controller which is used for the MDIO of the SFP ports. > If we had known about the worning, then we would have tried to fix it by > making CONFIG_SFP only on the 93xx platforms dependent on CONFIG_MDIO_SMBUS. > How to do this is however not clear to me. We should probably remove the dependency to CONFIG_MDIO_SMBUS from CONFIG_SFP. This is added in target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch You could add the dependency to the CONFIG_I2C_RTL9300 in target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch Hauke
Hi, On 14.03.22 23:53, Hauke Mehrtens wrote: > On 3/14/22 06:56, Birger Koblitz wrote: >> Hi, >> >> CONFIG_SFP should only depend on CONFIG_MDIO_SMBUS on the RTL93xx platforms. >> This is because only on those platforms there is hardware support for an SMBus >> controller which is used for the MDIO of the SFP ports. >> If we had known about the worning, then we would have tried to fix it by >> making CONFIG_SFP only on the 93xx platforms dependent on CONFIG_MDIO_SMBUS. >> How to do this is however not clear to me. > > We should probably remove the dependency to CONFIG_MDIO_SMBUS from CONFIG_SFP. This is added in target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch > > You could add the dependency to the CONFIG_I2C_RTL9300 in target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch > > Hauke > I would propose the following patch, which compiles on the 839x and 930x platforms: From 7fb7bb16fdd0a66fdd8dbd7cd071420d48df230a Mon Sep 17 00:00:00 2001 From: Birger Koblitz <git@birger-koblitz.de> Date: Tue, 15 Mar 2022 10:53:21 +0100 Subject: [PATCH] realtek: fix CONFIG_MDIO_SMBUS dependency on RTL83xx CONFIG_SFP depends on CONFIG_MDIO_SMBUS now and this depends on CONFIG_I2C_SMBUS. This created the following warnings on the RTL83xx platforms: WARNING: unmet direct dependencies detected for MDIO_SMBUS Depends on [n]: NETDEVICES [=y] && MDIO_DEVICE [=y] && MDIO_BUS [=y] && I2C_SMBUS [=n] Selected by [y]: - SFP [=y] && NETDEVICES [=y] && PHYLIB [=y] && I2C [=y] && PHYLINK [=y] && (HWMON [=y] || HWMON [=y]=n) We remove the overall dependency to CONFIG_MDIO_SMBUS from CONFIG_SFP and instead add it to CONFIG_I2C_RTL9300 since only the RTL93xx platforms make use of the SMBus capable I2C controller. Reported-by: Hauke Mehrtens <hauke@hauke-m.de> Signed-off-by: Birger Koblitz <git@birger-koblitz.de> --- .../patches-5.10/310-add-i2c-rtl9300-support.patch | 3 ++- .../711-net-phy-add-an-MDIO-SMBus-library.patch | 10 ---------- .../712-net-phy-sfp-add-support-for-SMBus.patch | 8 +++++++- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch b/target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch index c4d23d90e1..96be87e08e 100644 --- a/target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch +++ b/target/linux/realtek/patches-5.10/310-add-i2c-rtl9300-support.patch @@ -1,12 +1,13 @@ --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig -@@ -954,6 +954,16 @@ config I2C_RK3X +@@ -954,6 +954,17 @@ config I2C_RK3X This driver can also be built as a module. If so, the module will be called i2c-rk3x. +config I2C_RTL9300 + tristate "Realtek RTL9300 I2C adapter" + depends on OF ++ select MDIO_SMBUS + help + Say Y here to include support for the I2C adapter in Realtek RTL9300 + and RTL9310 SoCs. diff --git a/target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch b/target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch index 5e1053ca6c..aad70d8d6a 100644 --- a/target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch +++ b/target/linux/realtek/patches-5.10/711-net-phy-add-an-MDIO-SMBus-library.patch @@ -108,16 +108,6 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> +MODULE_AUTHOR("Antoine Tenart"); +MODULE_DESCRIPTION("MDIO SMBus bridge library"); +MODULE_LICENSE("GPL"); ---- a/drivers/net/phy/Kconfig -+++ b/drivers/net/phy/Kconfig -@@ -60,6 +60,7 @@ config SFP - depends on I2C && PHYLINK - depends on HWMON || HWMON=n - select MDIO_I2C -+ select MDIO_SMBUS - - comment "Switch configuration API + drivers" - --- a/include/linux/mdio/mdio-i2c.h +++ b/include/linux/mdio/mdio-i2c.h @@ -12,5 +12,21 @@ struct i2c_adapter; diff --git a/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch b/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch index 8eef1885ab..30085a8068 100644 --- a/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch +++ b/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch @@ -10,10 +10,11 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c -@@ -412,32 +412,72 @@ static int sfp_i2c_write(struct sfp *sfp +@@ -412,32 +412,78 @@ static int sfp_i2c_write(struct sfp *sfp return ret == ARRAY_SIZE(msgs) ? len : 0; } ++#ifdef CONFIG_MDIO_SMBUS +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, + size_t len) +{ @@ -42,6 +43,7 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> + + return sfp->i2c_mii->write(sfp->i2c_mii, bus_addr, dev_addr, val); +} ++#endif + static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c) { @@ -69,6 +71,7 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> + + mii->name = "SFP I2C Bus"; + } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) { ++#ifdef CONFIG_MDIO_SMBUS + sfp->read = sfp_smbus_read; + sfp->write = sfp_smbus_write; + @@ -79,6 +82,9 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> - i2c_mii->name = "SFP I2C Bus"; - i2c_mii->phy_mask = ~0; + mii->name = "SFP SMBus"; ++#else ++ return -EINVAL; ++#endif + } else { + return -EINVAL; + }
Birger Koblitz <mail@birger-koblitz.de> writes: > --- a/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch > +++ b/target/linux/realtek/patches-5.10/712-net-phy-sfp-add-support-for-SMBus.patch > @@ -10,10 +10,11 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > -@@ -412,32 +412,72 @@ static int sfp_i2c_write(struct sfp *sfp > +@@ -412,32 +412,78 @@ static int sfp_i2c_write(struct sfp *sfp > return ret == ARRAY_SIZE(msgs) ? len : 0; > } > > ++#ifdef CONFIG_MDIO_SMBUS > +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, > + size_t len) > +{ > @@ -42,6 +43,7 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > + > + return sfp->i2c_mii->write(sfp->i2c_mii, bus_addr, dev_addr, val); > +} > ++#endif > + > static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c) > { > @@ -69,6 +71,7 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > + > + mii->name = "SFP I2C Bus"; > + } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) { > ++#ifdef CONFIG_MDIO_SMBUS > + sfp->read = sfp_smbus_read; > + sfp->write = sfp_smbus_write; > + > @@ -79,6 +82,9 @@ Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > - i2c_mii->name = "SFP I2C Bus"; > - i2c_mii->phy_mask = ~0; > + mii->name = "SFP SMBus"; > ++#else > ++ return -EINVAL; > ++#endif > + } else { > + return -EINVAL; > + } Yuck! Why the heck can't this be made generic, auto-configured by DTS props and upstreamed? There is a reason for this, you know: bjorn@miraculix:/usr/local/src/git/linux$ git grep -E '^#ifdef' drivers/net/phy/sfp.c|wc -c 0 Bjørn
Hi, On 15.03.22 13:32, Bjørn Mork wrote: > Birger Koblitz <mail@birger-koblitz.de> writes: > Yuck! > > Why the heck can't this be made generic, auto-configured by DTS props > and upstreamed? There is a reason for this, you know: > > bjorn@miraculix:/usr/local/src/git/linux$ git grep -E '^#ifdef' drivers/net/phy/sfp.c|wc -c > 0 > Well, we can of course always compile SMBus support in, i.e. make CONFIG_SFP depend on CONFIG_MDIO_SMBUS, this is already done for the I2C equivalent and this is where we started: config SFP tristate "SFP cage support" depends on I2C && PHYLINK depends on HWMON || HWMON=n select MDIO_I2C + select CONFIG_MDIO_SMBUS But if we do not want to do that then #ifdefs are the only solution to prevent compiler errors due to missing symbols. There is no need to do anything in a dts BTW. The code is already auto-configuring, it first tries I2C which is more powerful and then falls back to SMBus before it gives up. Cheers, Birger
Birger Koblitz <mail@birger-koblitz.de> writes: > Well, we can of course always compile SMBus support in, i.e. make CONFIG_SFP depend on CONFIG_MDIO_SMBUS, > this is already done for the I2C equivalent and this is where we started: > config SFP > tristate "SFP cage support" > depends on I2C && PHYLINK > depends on HWMON || HWMON=n > select MDIO_I2C > + select CONFIG_MDIO_SMBUS > > But if we do not want to do that then #ifdefs are the only solution to prevent compiler errors > due to missing symbols. There is no need to do anything in a dts BTW. The code is already > auto-configuring, it first tries I2C which is more powerful and then > falls back to SMBus before it gives up. Just drop the unnecssary I2C_SMBUS dependency. AFAICS, you're only using i2c_smbus_xfer() which is part of the i2c core. Bjørn
Bjørn Mork <bjorn@mork.no> writes: > Just drop the unnecssary I2C_SMBUS dependency. AFAICS, you're only > using i2c_smbus_xfer() which is part of the i2c core. Looking further at this, I believe there are good reasons to simply merge the mdio-smbus implementation into mdio-i2c: You seem to have lost some clause 45 specific stuff from commit 6912b712cd75 ("net: mdio-i2c: add support for Clause 45 accesses") And the i2c_mii_valid_phy_id() tests to avoid eeprom access are also missing. Maybe not important, but demonstrating the problems with copying code. Even modules as small as these tend to grow apart over time. I guess the clause 45 support could be important for multigig copper SFPs? Bjørn
Hi Bjørn, On 15.03.22 15:10, Bjørn Mork wrote: > Bjørn Mork <bjorn@mork.no> writes: > >> Just drop the unnecssary I2C_SMBUS dependency. AFAICS, you're only >> using i2c_smbus_xfer() which is part of the i2c core. The reason for the ifdefs were mdio_smbus_alloc(). > > Looking further at this, I believe there are good reasons to simply > merge the mdio-smbus implementation into mdio-i2c: You seem to have lost > some clause 45 specific stuff from commit 6912b712cd75 ("net: mdio-i2c: > add support for Clause 45 accesses") > > And the i2c_mii_valid_phy_id() tests to avoid eeprom access are also > missing. Maybe not important, but demonstrating the problems with > copying code. Even modules as small as these tend to grow apart over > time. > > I guess the clause 45 support could be important for multigig copper > SFPs? Absolutely. I just hope that we will soon have this support. The 10G SFPs on the RTL93xx are a nightmare to understand. I'll pick up your suggestion with merging into mdio-i2c and also ask the original author of the patch why it was not upstreamed. Maybe we can learn what still would have been necessary. Cheers, Birger
Birger Koblitz <mail@birger-koblitz.de> writes: > On 15.03.22 15:10, Bjørn Mork wrote: >> Bjørn Mork <bjorn@mork.no> writes: >> >>> Just drop the unnecssary I2C_SMBUS dependency. AFAICS, you're only >>> using i2c_smbus_xfer() which is part of the i2c core. > The reason for the ifdefs were mdio_smbus_alloc(). I still can't see anything depending on I2C_SMBUS there? This is the troublesome one. Depending on MDIO_SMBUS is fine if you decide to keep that as a separate module. But then the sfp driver should select MDIO_SMBUS along with MDIO_I2C and drop the ifdefs. In my humble opinion, I should add... >> Looking further at this, I believe there are good reasons to simply >> merge the mdio-smbus implementation into mdio-i2c: You seem to have lost >> some clause 45 specific stuff from commit 6912b712cd75 ("net: mdio-i2c: >> add support for Clause 45 accesses") >> >> And the i2c_mii_valid_phy_id() tests to avoid eeprom access are also >> missing. Maybe not important, but demonstrating the problems with >> copying code. Even modules as small as these tend to grow apart over >> time. >> >> I guess the clause 45 support could be important for multigig copper >> SFPs? > Absolutely. I just hope that we will soon have this support. The 10G SFPs > on the RTL93xx are a nightmare to understand. > > I'll pick up your suggestion with merging into mdio-i2c and also ask the original > author of the patch why it was not upstreamed. Maybe we can learn what still > would have been necessary. Great! This is really stuff that should Just Work, without having to think too much about driver implementation details. Varying dependencies are messy. (I started playing with the thought that both the sfp driver and the mdio-i2c bus driver could use SMBus transfers unconditionally, depending on the automatic fallback to emulated SMBus. Probably not a good idea...) Bjørn
diff --git a/target/linux/realtek/rtl838x/config-5.10 b/target/linux/realtek/rtl838x/config-5.10 index 8e27af95ba46..16964d94c967 100644 --- a/target/linux/realtek/rtl838x/config-5.10 +++ b/target/linux/realtek/rtl838x/config-5.10 @@ -90,6 +90,7 @@ CONFIG_I2C_GPIO=y CONFIG_I2C_MUX=y # CONFIG_I2C_RTL9300 is not set # CONFIG_I2C_MUX_RTL9300 is not set +CONFIG_I2C_SMBUS=y CONFIG_INITRAMFS_SOURCE="" CONFIG_IRQCHIP=y CONFIG_IRQ_DOMAIN=y @@ -107,6 +108,7 @@ CONFIG_MDIO_BUS=y CONFIG_MDIO_DEVICE=y CONFIG_MDIO_DEVRES=y CONFIG_MDIO_I2C=y +CONFIG_MDIO_SMBUS=y CONFIG_MEMFD_CREATE=y CONFIG_MFD_SYSCON=y CONFIG_MIGRATION=y diff --git a/target/linux/realtek/rtl839x/config-5.10 b/target/linux/realtek/rtl839x/config-5.10 index 5704b4265a3c..b3192d1866da 100644 --- a/target/linux/realtek/rtl839x/config-5.10 +++ b/target/linux/realtek/rtl839x/config-5.10 @@ -86,6 +86,7 @@ CONFIG_I2C_BOARDINFO=y CONFIG_I2C_GPIO=y # CONFIG_I2C_RTL9300 is not set # CONFIG_I2C_MUX_RTL9300 is not set +CONFIG_I2C_SMBUS=y CONFIG_INITRAMFS_SOURCE="" CONFIG_IRQCHIP=y CONFIG_IRQ_DOMAIN=y @@ -104,6 +105,7 @@ CONFIG_MDIO_BUS=y CONFIG_MDIO_DEVICE=y CONFIG_MDIO_DEVRES=y CONFIG_MDIO_I2C=y +CONFIG_MDIO_SMBUS=y CONFIG_MEMFD_CREATE=y CONFIG_MFD_SYSCON=y CONFIG_MIGRATION=y
CONFIG_SFP depends on CONFIG_MDIO_SMBUS now and this depends on CONFIG_I2C_SMBUS. Activate the missing kernel compile options. This fixes the following warning: WARNING: unmet direct dependencies detected for MDIO_SMBUS Depends on [n]: NETDEVICES [=y] && MDIO_DEVICE [=y] && MDIO_BUS [=y] && I2C_SMBUS [=n] Selected by [y]: - SFP [=y] && NETDEVICES [=y] && PHYLIB [=y] && I2C [=y] && PHYLINK [=y] && (HWMON [=y] || HWMON [=y]=n) Fixes: 45053b507c66 ("realtek: Add support for SFP EEPROM-access over SMBus") Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- target/linux/realtek/rtl838x/config-5.10 | 2 ++ target/linux/realtek/rtl839x/config-5.10 | 2 ++ 2 files changed, 4 insertions(+)