Hello everyone,
Here's yet another version of the cleanup series for the TSE PCS replacement
by PCS Lynx. It includes Kconfig fixups, some missing initialisations
and a slight rework suggested by Russell for the dwmac cleanup sequence,
along with more explicit zeroing of local structures as per MAciej's
review.
V3->V4 :
- Zero mdio_regmap_config objects
- Make regmap config more local in dwmac_socfpga
V2->V3 :
- Fix uninitialized .autoscan field for mdio regmap configuration in
both altera_tse and dwmac_socfpga
V1->V2 :
- Fix a Kconfig inconsistency
- rework the dwmac_socfpga cleanup sequence
Maxime Chevallier (5):
net: altera-tse: Initialize local structs before using it
net: altera_tse: Use the correct Kconfig option for the PCS_LYNX
dependency
net: stmmac: make the pcs_lynx cleanup sequence specific to
dwmac_socfpga
net: altera_tse: explicitly disable autoscan on the regmap-mdio bus
net: dwmac_socfpga: initialize local data for mdio regmap
configuration
drivers/net/ethernet/altera/Kconfig | 2 +-
drivers/net/ethernet/altera/altera_tse_main.c | 3 ++
drivers/net/ethernet/stmicro/stmmac/common.h | 1 -
.../ethernet/stmicro/stmmac/dwmac-socfpga.c | 29 ++++++++++++++-----
.../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 3 --
5 files changed, 26 insertions(+), 12 deletions(-)
--
2.40.1
Explicitely zero-ize the local mdio_regmap_config data, and explicitely
set the .autoscan parameter, as we only have a PCS on this bus.
Fixes: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <[email protected]>
Suggested-by: Maciej Fijalkowski <[email protected]>
Signed-off-by: Maxime Chevallier <[email protected]>
---
V3->V4 : Move pcs_regmap_cfg into a more local block, and zeroize mrc
V2->V3 : New patch
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1fb808be843b..6267bcb60206 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -389,7 +389,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
struct net_device *ndev;
struct stmmac_priv *stpriv;
const struct socfpga_dwmac_ops *ops;
- struct regmap_config pcs_regmap_cfg;
ops = device_get_match_data(&pdev->dev);
if (!ops) {
@@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
if (ret)
goto err_dvr_remove;
- memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
- pcs_regmap_cfg.reg_bits = 16;
- pcs_regmap_cfg.val_bits = 16;
- pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
-
/* Create a regmap for the PCS so that it can be used by the PCS driver,
* if we have such a PCS
*/
if (dwmac->tse_pcs_base) {
+ struct regmap_config pcs_regmap_cfg;
struct mdio_regmap_config mrc;
struct regmap *pcs_regmap;
struct mii_bus *pcs_bus;
+ memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+ memset(&mrc, 0, sizeof(mrc));
+
+ pcs_regmap_cfg.reg_bits = 16;
+ pcs_regmap_cfg.val_bits = 16;
+ pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
+
pcs_regmap = devm_regmap_init_mmio(&pdev->dev, dwmac->tse_pcs_base,
&pcs_regmap_cfg);
if (IS_ERR(pcs_regmap)) {
@@ -470,6 +472,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
mrc.regmap = pcs_regmap;
mrc.parent = &pdev->dev;
mrc.valid_addr = 0x0;
+ mrc.autoscan = false;
snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
--
2.40.1
Use the correct Kconfig dependency for altera_tse as PCS_ALTERA_TSE was
replaced by PCS_LYNX.
Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Signed-off-by: Maxime Chevallier <[email protected]>
---
V3->V4 : No changes
V2->V3 : Fix a typo in the commit title
V1->V2 : New patch
drivers/net/ethernet/altera/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/altera/Kconfig b/drivers/net/ethernet/altera/Kconfig
index 93533ba03429..17985319088c 100644
--- a/drivers/net/ethernet/altera/Kconfig
+++ b/drivers/net/ethernet/altera/Kconfig
@@ -4,7 +4,7 @@ config ALTERA_TSE
depends on HAS_DMA
select PHYLIB
select PHYLINK
- select PCS_ALTERA_TSE
+ select PCS_LYNX
select MDIO_REGMAP
select REGMAP_MMIO
help
--
2.40.1
Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
random uninitialized value. We don't want autoscan in this case as the
mdio device is a PCS and not a PHY.
Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Suggested-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Maxime Chevallier <[email protected]>
---
V3->V4 : No changes
V2->V3 : New patch
drivers/net/ethernet/altera/altera_tse_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index 215f9fb89c5b..2e15800e5310 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -1288,6 +1288,7 @@ static int altera_tse_probe(struct platform_device *pdev)
mrc.regmap = pcs_regmap;
mrc.parent = &pdev->dev;
mrc.valid_addr = 0x0;
+ mrc.autoscan = false;
/* Rx IRQ */
priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
--
2.40.1
So far, only the dwmac_socfpga variant of stmmac uses PCS Lynx. Use a
dedicated cleanup sequence for dwmac_socfpga instead of using the
generic stmmac one.
Fixes: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Maxime Chevallier <[email protected]>
---
V3->V4 : No changes
V2->V3 : Removed extra whiteline
V1->V2 : New patch
drivers/net/ethernet/stmicro/stmmac/common.h | 1 -
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 14 +++++++++++++-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 3 ---
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 52c5ec553276..16e67c18b6f7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -16,7 +16,6 @@
#include <linux/stmmac.h>
#include <linux/phy.h>
#include <linux/pcs/pcs-xpcs.h>
-#include <linux/pcs-lynx.h>
#include <linux/module.h>
#if IS_ENABLED(CONFIG_VLAN_8021Q)
#define STMMAC_VLAN_TAG_USED
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index e399fccbafe5..1fb808be843b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -11,6 +11,7 @@
#include <linux/phy.h>
#include <linux/regmap.h>
#include <linux/mdio/mdio-regmap.h>
+#include <linux/pcs-lynx.h>
#include <linux/reset.h>
#include <linux/stmmac.h>
@@ -494,6 +495,17 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
return ret;
}
+static void socfpga_dwmac_remove(struct platform_device *pdev)
+{
+ struct net_device *ndev = platform_get_drvdata(pdev);
+ struct stmmac_priv *priv = netdev_priv(ndev);
+ struct phylink_pcs *pcs = priv->hw->lynx_pcs;
+
+ stmmac_pltfr_remove(pdev);
+
+ lynx_pcs_destroy(pcs);
+}
+
#ifdef CONFIG_PM_SLEEP
static int socfpga_dwmac_resume(struct device *dev)
{
@@ -565,7 +577,7 @@ MODULE_DEVICE_TABLE(of, socfpga_dwmac_match);
static struct platform_driver socfpga_dwmac_driver = {
.probe = socfpga_dwmac_probe,
- .remove_new = stmmac_pltfr_remove,
+ .remove_new = socfpga_dwmac_remove,
.driver = {
.name = "socfpga-dwmac",
.pm = &socfpga_dwmac_pm_ops,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index c784a6731f08..3db1cb0fd160 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -665,9 +665,6 @@ int stmmac_mdio_unregister(struct net_device *ndev)
if (priv->hw->xpcs)
xpcs_destroy(priv->hw->xpcs);
- if (priv->hw->lynx_pcs)
- lynx_pcs_destroy(priv->hw->lynx_pcs);
-
mdiobus_unregister(priv->mii);
priv->mii->priv = NULL;
mdiobus_free(priv->mii);
--
2.40.1
The regmap_config and mdio_regmap_config objects needs to be zeroed before
using them. This will cause spurious errors at probe time as config->pad_bits
is containing random uninitialized data.
Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Signed-off-by: Maxime Chevallier <[email protected]>
---
V3->V4 : Also zero "mrc" from Maciej's review
V2->V3 : No changes
V1->V2 : No changes
drivers/net/ethernet/altera/altera_tse_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index d866c0f1b503..215f9fb89c5b 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -1255,6 +1255,8 @@ static int altera_tse_probe(struct platform_device *pdev)
if (ret)
goto err_free_netdev;
+ memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+ memset(&mrc, 0, sizeof(mrc));
/* SGMII PCS address space. The location can vary depending on how the
* IP is integrated. We can have a resource dedicated to it at a specific
* address space, but if it's not the case, we fallback to the mdiophy0
--
2.40.1
On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> struct mdio_regmap_config mrc;
> struct regmap *pcs_regmap;
> struct mii_bus *pcs_bus;
>
...
> + memset(&mrc, 0, sizeof(mrc));
...
> mrc.parent = &pdev->dev;
> mrc.valid_addr = 0x0;
> + mrc.autoscan = false;
Isn't this covered by the memset() ?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jun 07, 2023 at 03:59:40PM +0200, Maxime Chevallier wrote:
> Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
> random uninitialized value. We don't want autoscan in this case as the
> mdio device is a PCS and not a PHY.
Isn't this now covered by patch 1's memset of mrc?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hello Russell,
On Wed, 7 Jun 2023 13:26:41 +0100
"Russell King (Oracle)" <[email protected]> wrote:
> On Wed, Jun 07, 2023 at 03:59:40PM +0200, Maxime Chevallier wrote:
> > Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
> > random uninitialized value. We don't want autoscan in this case as the
> > mdio device is a PCS and not a PHY.
>
> Isn't this now covered by patch 1's memset of mrc?
>
Yes it is, however I thought it could be fine keeping it set explicitely
anyway, as we do have a PCS on that bus and we don't want any autoscan
happening. Since these two drivers are the first users of mdio_regmap,
my hope is that we could get these reference usages covering all fields.
Should I drop this ?
Maxime
On Wed, 7 Jun 2023 13:28:03 +0100
"Russell King (Oracle)" <[email protected]> wrote:
> On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> > struct mdio_regmap_config mrc;
> > struct regmap *pcs_regmap;
> > struct mii_bus *pcs_bus;
> >
> ...
> > + memset(&mrc, 0, sizeof(mrc));
> ...
> > mrc.parent = &pdev->dev;
> > mrc.valid_addr = 0x0;
> > + mrc.autoscan = false;
>
> Isn't this covered by the memset() ?
I have the same answer as for the above. It's redundant, but I don't
think there's any harm having it set explicitely ?
Maxime
On Wed, Jun 07, 2023 at 03:59:36PM +0200, Maxime Chevallier wrote:
> Hello everyone,
>
> Here's yet another version of the cleanup series for the TSE PCS replacement
> by PCS Lynx. It includes Kconfig fixups, some missing initialisations
> and a slight rework suggested by Russell for the dwmac cleanup sequence,
> along with more explicit zeroing of local structures as per MAciej's
> review.
For the series, which brings an immediate fix to the problems people
are noticing:
Reviewed-by: Russell King (Oracle) <[email protected]>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jun 07, 2023 at 04:54:09PM +0200, Maxime Chevallier wrote:
> On Wed, 7 Jun 2023 13:28:03 +0100
> "Russell King (Oracle)" <[email protected]> wrote:
>
> > On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> > > struct mdio_regmap_config mrc;
> > > struct regmap *pcs_regmap;
> > > struct mii_bus *pcs_bus;
> > >
> > ...
> > > + memset(&mrc, 0, sizeof(mrc));
> > ...
> > > mrc.parent = &pdev->dev;
> > > mrc.valid_addr = 0x0;
> > > + mrc.autoscan = false;
> >
> > Isn't this covered by the memset() ?
>
> I have the same answer as for the above. It's redundant, but I don't
> think there's any harm having it set explicitely ?
No harm, just redundant. I don't think this is a good enough reason not
to merge it.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> Explicitely zero-ize the local mdio_regmap_config data, and explicitely
nit: Explicitely -> explicitly
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Wed, 7 Jun 2023 15:59:36 +0200 you wrote:
> Hello everyone,
>
> Here's yet another version of the cleanup series for the TSE PCS replacement
> by PCS Lynx. It includes Kconfig fixups, some missing initialisations
> and a slight rework suggested by Russell for the dwmac cleanup sequence,
> along with more explicit zeroing of local structures as per MAciej's
> review.
>
> [...]
Here is the summary with links:
- [net-next,v4,1/5] net: altera-tse: Initialize local structs before using it
https://git.kernel.org/netdev/net-next/c/2d830f7a4134
- [net-next,v4,2/5] net: altera_tse: Use the correct Kconfig option for the PCS_LYNX dependency
https://git.kernel.org/netdev/net-next/c/fae555f5a56f
- [net-next,v4,3/5] net: stmmac: make the pcs_lynx cleanup sequence specific to dwmac_socfpga
https://git.kernel.org/netdev/net-next/c/a8dd7404c214
- [net-next,v4,4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus
https://git.kernel.org/netdev/net-next/c/fa19a5d9dcff
- [net-next,v4,5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration
https://git.kernel.org/netdev/net-next/c/06b9dede1e7d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html