Message ID | 20211012103402.21438-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 67ca5159dbe2edb5dae7544447b8677d2596933a |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: phy: micrel: make *-skew-ps check more lenient | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | No Fixes tag |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/header_inline | success | No static functions without inline keyword in header files |
On Tue, 2021-10-12 at 12:34 +0200, Matthias Schiffer wrote: > It seems reasonable to fine-tune only some of the skew values when > using > one of the rgmii-*id PHY modes, and even when all skew values are > specified, using the correct ID PHY mode makes sense for documentation > purposes. Such a configuration also appears in the binding docs in > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the > driver > should not warn about it. I don't think your commit message is right. The rgmii-*id PHY modes are no longer just for documentation purposes on KSZ9031 PHY. They are used to set the skew-registers according to . The warning is there, that in case you override the skew registers of one of the modes rgmii-id, rgmii-txid, rgmii-rxid with *-skew-ps settings in DT. Therefore I also think the warning is valuable and should be kept. We may want to reword it though. Philippe > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/net/phy/micrel.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index c330a5a9f665..03e58ebf68af 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -863,9 +863,9 @@ static int ksz9031_config_init(struct phy_device > *phydev) > MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, > tx_data_skews, 4, &update); > > - if (update && phydev->interface != > PHY_INTERFACE_MODE_RGMII) > + if (update && !phy_interface_is_rgmii(phydev)) > phydev_warn(phydev, > - "*-skew-ps values should be used > only with phy-mode = \"rgmii\"\n"); > + "*-skew-ps values should be used > only with RGMII PHY modes\n"); > > /* Silicon Errata Sheet (DS80000691D or DS80000692D): > * When the device links in the 1000BASE-T slave mode > only,
On Wed, 2021-10-13 at 08:25 +0000, Philippe Schenker wrote: > On Tue, 2021-10-12 at 12:34 +0200, Matthias Schiffer wrote: > > It seems reasonable to fine-tune only some of the skew values when > > using > > one of the rgmii-*id PHY modes, and even when all skew values are > > specified, using the correct ID PHY mode makes sense for documentation > > purposes. Such a configuration also appears in the binding docs in > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the > > driver > > should not warn about it. > > I don't think your commit message is right. The rgmii-*id PHY modes are > no longer just for documentation purposes on KSZ9031 PHY. They are used > to set the skew-registers according to . Yes, this was implemented in [1]. The commit message explicitly states that fine-tuning is still possible using *-skew-ps. > > The warning is there, that in case you override the skew registers of > one of the modes rgmii-id, rgmii-txid, rgmii-rxid with *-skew-ps > settings in DT. The "rgmii" mode should not be handled differently from "rgmii-*id" in my opinion. Otherwise for a device that is basically "rgmii-id", but requires slight fine-tuning, you have to set the mode to the incorrect value "rgmii" in the DTS to avoid this warning. > > Therefore I also think the warning is valuable and should be kept. We > may want to reword it though. > > Philippe [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/micrel.c?id=bcf3440c6dd78bfe5836ec0990fe36d7b4bb7d20 > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > --- > > drivers/net/phy/micrel.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index c330a5a9f665..03e58ebf68af 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -863,9 +863,9 @@ static int ksz9031_config_init(struct phy_device > > *phydev) > > MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, > > tx_data_skews, 4, &update); > > > > - if (update && phydev->interface != > > PHY_INTERFACE_MODE_RGMII) > > + if (update && !phy_interface_is_rgmii(phydev)) > > phydev_warn(phydev, > > - "*-skew-ps values should be used > > only with phy-mode = \"rgmii\"\n"); > > + "*-skew-ps values should be used > > only with RGMII PHY modes\n"); > > > > /* Silicon Errata Sheet (DS80000691D or DS80000692D): > > * When the device links in the 1000BASE-T slave mode > > only, > >
On Wed, 2021-10-13 at 10:46 +0200, Matthias Schiffer wrote: > On Wed, 2021-10-13 at 08:25 +0000, Philippe Schenker wrote: > > On Tue, 2021-10-12 at 12:34 +0200, Matthias Schiffer wrote: > > > It seems reasonable to fine-tune only some of the skew values when > > > using > > > one of the rgmii-*id PHY modes, and even when all skew values are > > > specified, using the correct ID PHY mode makes sense for > > > documentation > > > purposes. Such a configuration also appears in the binding docs in > > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the > > > driver > > > should not warn about it. > > > > I don't think your commit message is right. The rgmii-*id PHY modes > > are > > no longer just for documentation purposes on KSZ9031 PHY. They are > > used > > to set the skew-registers according to . > > Yes, this was implemented in [1]. The commit message explicitly states > that fine-tuning is still possible using *-skew-ps. > > > > > The warning is there, that in case you override the skew registers > > of > > one of the modes rgmii-id, rgmii-txid, rgmii-rxid with *-skew-ps > > settings in DT. > > The "rgmii" mode should not be handled differently from "rgmii-*id" in > my opinion. Otherwise for a device that is basically "rgmii-id", but > requires slight fine-tuning, you have to set the mode to the incorrect > value "rgmii" in the DTS to avoid this warning. Now I have understood your argument. But then I suggest to delete the warning entirely as it completely changes its meaning with that patch. Philippe > > > > > > Therefore I also think the warning is valuable and should be kept. > > We > > may want to reword it though. > > > > Philippe > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/micrel.c?id=bcf3440c6dd78bfe5836ec0990fe36d7b4bb7d20 > > > > > > > > > > Signed-off-by: Matthias Schiffer > > > <matthias.schiffer@ew.tq-group.com> > > > --- > > > drivers/net/phy/micrel.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > > index c330a5a9f665..03e58ebf68af 100644 > > > --- a/drivers/net/phy/micrel.c > > > +++ b/drivers/net/phy/micrel.c > > > @@ -863,9 +863,9 @@ static int ksz9031_config_init(struct > > > phy_device > > > *phydev) > > > MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, > > > tx_data_skews, 4, &update); > > > > > > - if (update && phydev->interface != > > > PHY_INTERFACE_MODE_RGMII) > > > + if (update && !phy_interface_is_rgmii(phydev)) > > > phydev_warn(phydev, > > > - "*-skew-ps values should be > > > used > > > only with phy-mode = \"rgmii\"\n"); > > > + "*-skew-ps values should be > > > used > > > only with RGMII PHY modes\n"); > > > > > > /* Silicon Errata Sheet (DS80000691D or > > > DS80000692D): > > > * When the device links in the 1000BASE-T slave > > > mode > > > only, > > > > >
On Wed, 2021-10-13 at 10:18 +0000, Philippe Schenker wrote: > On Wed, 2021-10-13 at 10:46 +0200, Matthias Schiffer wrote: > > On Wed, 2021-10-13 at 08:25 +0000, Philippe Schenker wrote: > > > On Tue, 2021-10-12 at 12:34 +0200, Matthias Schiffer wrote: > > > > It seems reasonable to fine-tune only some of the skew values when > > > > using > > > > one of the rgmii-*id PHY modes, and even when all skew values are > > > > specified, using the correct ID PHY mode makes sense for > > > > documentation > > > > purposes. Such a configuration also appears in the binding docs in > > > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the > > > > driver > > > > should not warn about it. > > > > > > I don't think your commit message is right. The rgmii-*id PHY modes > > > are > > > no longer just for documentation purposes on KSZ9031 PHY. They are > > > used > > > to set the skew-registers according to . > > > > Yes, this was implemented in [1]. The commit message explicitly states > > that fine-tuning is still possible using *-skew-ps. > > > > > > > > The warning is there, that in case you override the skew registers > > > of > > > one of the modes rgmii-id, rgmii-txid, rgmii-rxid with *-skew-ps > > > settings in DT. > > > > The "rgmii" mode should not be handled differently from "rgmii-*id" in > > my opinion. Otherwise for a device that is basically "rgmii-id", but > > requires slight fine-tuning, you have to set the mode to the incorrect > > value "rgmii" in the DTS to avoid this warning. > > Now I have understood your argument. But then I suggest to delete the > warning entirely as it completely changes its meaning with that patch. > > Philippe The KSZ9031 also supports MII and GMII though. I think it makes sense to keep the warning for these cases (which is why I reworded the warning the way I did). > > > > > > > > > > > Therefore I also think the warning is valuable and should be kept. > > > We > > > may want to reword it though. > > > > > > Philippe > > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/micrel.c?id=bcf3440c6dd78bfe5836ec0990fe36d7b4bb7d20 > > > > > > > > > > > > > > > Signed-off-by: Matthias Schiffer > > > > <matthias.schiffer@ew.tq-group.com> > > > > --- > > > > drivers/net/phy/micrel.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > > > index c330a5a9f665..03e58ebf68af 100644 > > > > --- a/drivers/net/phy/micrel.c > > > > +++ b/drivers/net/phy/micrel.c > > > > @@ -863,9 +863,9 @@ static int ksz9031_config_init(struct > > > > phy_device > > > > *phydev) > > > > MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, > > > > tx_data_skews, 4, &update); > > > > > > > > - if (update && phydev->interface != > > > > PHY_INTERFACE_MODE_RGMII) > > > > + if (update && !phy_interface_is_rgmii(phydev)) > > > > phydev_warn(phydev, > > > > - "*-skew-ps values should be > > > > used > > > > only with phy-mode = \"rgmii\"\n"); > > > > + "*-skew-ps values should be > > > > used > > > > only with RGMII PHY modes\n"); > > > > > > > > /* Silicon Errata Sheet (DS80000691D or > > > > DS80000692D): > > > > * When the device links in the 1000BASE-T slave > > > > mode > > > > only, > > > > > > > >
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 12 Oct 2021 12:34:02 +0200 you wrote: > It seems reasonable to fine-tune only some of the skew values when using > one of the rgmii-*id PHY modes, and even when all skew values are > specified, using the correct ID PHY mode makes sense for documentation > purposes. Such a configuration also appears in the binding docs in > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the driver > should not warn about it. > > [...] Here is the summary with links: - net: phy: micrel: make *-skew-ps check more lenient https://git.kernel.org/netdev/net-next/c/67ca5159dbe2 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index c330a5a9f665..03e58ebf68af 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -863,9 +863,9 @@ static int ksz9031_config_init(struct phy_device *phydev) MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4, tx_data_skews, 4, &update); - if (update && phydev->interface != PHY_INTERFACE_MODE_RGMII) + if (update && !phy_interface_is_rgmii(phydev)) phydev_warn(phydev, - "*-skew-ps values should be used only with phy-mode = \"rgmii\"\n"); + "*-skew-ps values should be used only with RGMII PHY modes\n"); /* Silicon Errata Sheet (DS80000691D or DS80000692D): * When the device links in the 1000BASE-T slave mode only,
It seems reasonable to fine-tune only some of the skew values when using one of the rgmii-*id PHY modes, and even when all skew values are specified, using the correct ID PHY mode makes sense for documentation purposes. Such a configuration also appears in the binding docs in Documentation/devicetree/bindings/net/micrel-ksz90x1.txt, so the driver should not warn about it. Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/net/phy/micrel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)