[PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
If the interrupt configuration isn't set and we are using the internal phy, then we need to poll the phy to reliably detect phy state changes. Signed-off-by: Jeremy Linton --- drivers/net/ethernet/smsc/smsc911x.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 8af2556..369dc7d 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev) return -ENODEV; } + if ((!phydev->irq) && (!pdata->using_extphy)) + phydev->irq = PHY_POLL; + SMSC_TRACE(pdata, probe, "PHY: addr %d, phy_id 0x%08X", phydev->mdio.addr, phydev->phy_id); -- 2.5.5
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 02:49 PM, Sergei Shtylyov wrote: On 06/14/2016 10:27 PM, Sergei Shtylyov wrote: If the interrupt configuration isn't set and we are using the internal phy, then we need to poll the phy to reliably detect phy state changes. Signed-off-by: Jeremy Linton --- drivers/net/ethernet/smsc/smsc911x.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 8af2556..369dc7d 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev) return -ENODEV; } +if ((!phydev->irq) && (!pdata->using_extphy)) Inner parens aren't needed at all. Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()... And looking at that array, I doubt it's really useful for anything... And the memcpy() there seems buggy as well -- it copies just 4 bytes of this array to 'pdata->mii_bus->irq'. I do care about this driver, so might be a good idea to clean it up a bit... The use of phy_connect_direct() in the driver probe is incorrect, and keeps the driver from being unloaded. Also, some portion of smsc's can deliver mii state change interrupts via the smsc interrupt, but that code is no longer in this driver. I suspect a portion of the problem, besides all the strange hardware configurations this driver supports are the emulated hardware like QEMU that also uses it.
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 01:42 PM, Andrew Lunn wrote: On Tue, Jun 14, 2016 at 11:16:02AM -0500, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the internal phy, then we need to poll the phy to reliably detect phy state changes. Hi Jeremy Why does the external phy not have the exact same problem? Hello, It may... but I've only got a fairly limited hardware testing setup, and this driver is used across a pretty broad set of hardware configured in various ways. So, I'm trying to limit the scope of things I might break. Initially I thought it was added to avoid a QEMU issue, but now I don't think that is the case. So, if 0 can't be an interrupt why not update phy_interrupt_is_valid() to check for it? That would fix the problem too...
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote: On 06/14/2016 07:16 PM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) Looks like your phy ends up polling (-1==IRQ_POLL)... I'm using the device tree on my board. This was DT as well with a recent fedora/NetworkManager. It actually seems to be timing related to how fast the device gets configured after the initial phy probe. There is something like a 1 second window or so where it will work, but if network manager takes longer than that, the link state drops and cannot be brought back up unless the cable is pulled, replugged while the netdevice is being restarted.
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote: On 06/14/2016 07:16 PM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) BTW, the phy that is causing the problem is the one labeled "SMSC LAN911x Internal PHY" in phy/smsc.c.
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote: On 06/14/2016 07:16 PM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. AFAIK, I think that its set when the device is configured as a platform device, or there is an external phy/interrupt setup in DT. I might be wrong on that.. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) I'm using the device tree on my board. MBR, Sergei
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote: On 06/14/2016 07:16 PM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) I'm using the device tree on my board. Ok, I'm back on the machine, this is what mine says without that patch. SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0)
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 04:34 PM, Sergei Shtylyov wrote: On 06/15/2016 12:29 AM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) I'm using the device tree on my board. Ok, I'm back on the machine, this is what mine says without that patch. SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0) Hum, that's unexpected... things are probably more complex that I thought. Do you have extra patches to this driver by changce? No, the initial kernel where the problem was discovered is 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the driver with the same effect. Although, now that I'm looking closer at phy_irq, I'm curious how it works for anyone else...
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 04:42 PM, Sergei Shtylyov wrote: On 06/15/2016 12:40 AM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) I'm using the device tree on my board. Ok, I'm back on the machine, this is what mine says without that patch. SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0) Hum, that's unexpected... things are probably more complex that I thought. Do you have extra patches to this driver by changce? No, the initial kernel where the problem was discovered is 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the driver with the same effect. Although, now that I'm looking closer at phy_irq, I'm curious how it works for anyone else... Does anything change when you comment out that memcpy()? It shouldn't probably... Well that should change the irq to PHY_POLL by default rather than the 0's in the structure, which may be a better patch.
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 05:26 PM, Andrew Lunn wrote: This was DT as well with a recent fedora/NetworkManager. It actually seems to be timing related to how fast the device gets configured after the initial phy probe. There is something like a 1 second window or so where it will work, but if network manager takes longer than that, the link state drops and cannot be brought back up unless the cable is pulled, replugged while the netdevice is being restarted. Ah! There is another bug in the driver. The phy is connected to the netdev after calling register_netdev(). You are supposed to do it before, because the interface is usable, and can be used, directly after the register. Move the call to smsc911x_mii_init() before the register_netdev(). Yah, I buy that, and will move it an see what happens. But it doesn't solve the problem of the module use count being bumped in the probe rather than the ndo_open(). The users of phy_connect_direct() seem to be split between using it in the probe, and using it in the ndo_open (pxa168, and ax88796 for two examples of using it in the open).
Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
On 06/14/2016 04:56 PM, Sergei Shtylyov wrote: On 06/15/2016 12:53 AM, Jeremy Linton wrote: If the interrupt configuration isn't set and we are using the It's never set, judging by the driver code. internal phy, then we need to poll the phy to reliably detect phy state changes. What address your internal PHY is at? Mine is at 1, and things seem to work reliably after probing: SMSC LAN8700 1800.etherne:01: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=1800.etherne:01, irq=-1) I'm using the device tree on my board. Ok, I'm back on the machine, this is what mine says without that patch. SMSC LAN911x Internal PHY 1800.etherne:01: attached PHY driver [SMSC LAN911x Internal PHY] (mii_bus:phy_addr=1800.etherne:01, irq=0) Hum, that's unexpected... things are probably more complex that I thought. Do you have extra patches to this driver by changce? No, the initial kernel where the problem was discovered is 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the driver with the same effect. Although, now that I'm looking closer at phy_irq, I'm curious how it works for anyone else... Does anything change when you comment out that memcpy()? It shouldn't probably... Well that should change the irq to PHY_POLL by default rather than the 0's in the structure, which may be a better patch. It shouldn't due to the wrong size. It should only overwrite IRQ and index 0, unless I'm mistaken. Oh, sizeof(pointer)==8 on arm64, yah in the arm32 case you dodge the bullet. I think the memcpy removal solves the problem, i'm also going to test moving the mii_probe and will post an updated patch. Thanks!
[PATCH 2/2] net: smsc911x: Fix register_netdev, phy startup ordering and driver unload
Previously the mdio and phy's were started in the drv probe following
register_netdev(). This could lead to a situation where if the netdev was
opened before the mdio/phys were configured, it would fail with a
EAGAIN.
Also, the use of phy_connect_direct() in the drv_probe
routine results in a situation where the module use count would never
decrease sufficiently to unload the driver.
With this patch the mdio bus is allocated/configured before
register_netdev(), and the phy's are brought online/started in the
ndo_open and stopped in the ndo_stop. Because of this, the behavior
of ethtool changes a little if the interface is stopped. Before the
phy's would remain up, and their last state would be displayed with
ethtool. Now ethtool reports link has been severed/Link detected: no
when the net dev is stopped.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 48 +++-
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index b5ab5e1..abb1842 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1100,15 +1100,8 @@ static int smsc911x_mii_init(struct platform_device
*pdev,
goto err_out_free_bus_2;
}
- if (smsc911x_mii_probe(dev) < 0) {
- SMSC_WARN(pdata, probe, "Error registering mii bus");
- goto err_out_unregister_bus_3;
- }
-
return 0;
-err_out_unregister_bus_3:
- mdiobus_unregister(pdata->mii_bus);
err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
err_out_1:
@@ -1516,10 +1509,12 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
- /* if the phy is not yet registered, retry later*/
+ /* find and start the given phy */
if (!pdata->phy_dev) {
- SMSC_WARN(pdata, hw, "phy_dev is NULL");
- return -EAGAIN;
+ if (smsc911x_mii_probe(dev) < 0) {
+ SMSC_WARN(pdata, probe, "Error starting phy");
+ return -EAGAIN;
+ }
}
/* Reset the LAN911x */
@@ -1663,8 +1658,11 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
/* Bring the PHY down */
- if (pdata->phy_dev)
+ if (pdata->phy_dev) {
phy_stop(pdata->phy_dev);
+ phy_disconnect(pdata->phy_dev);
+ pdata->phy_dev = NULL;
+ }
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -1917,8 +1915,12 @@ smsc911x_ethtool_getsettings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
struct smsc911x_data *pdata = netdev_priv(dev);
+ if (!netif_running(dev) || !pdata->phy_dev)
+ return -ENOLINK;
+
cmd->maxtxpkt = 1;
cmd->maxrxpkt = 1;
+
return phy_ethtool_gset(pdata->phy_dev, cmd);
}
@@ -1927,6 +1929,9 @@ smsc911x_ethtool_setsettings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
struct smsc911x_data *pdata = netdev_priv(dev);
+ if (!netif_running(dev) || !pdata->phy_dev)
+ return -ENOLINK;
+
return phy_ethtool_sset(pdata->phy_dev, cmd);
}
@@ -1943,6 +1948,9 @@ static int smsc911x_ethtool_nwayreset(struct net_device
*dev)
{
struct smsc911x_data *pdata = netdev_priv(dev);
+ if (!netif_running(dev) || !pdata->phy_dev)
+ return -ENOLINK;
+
return phy_start_aneg(pdata->phy_dev);
}
@@ -2308,12 +2316,10 @@ static int smsc911x_drv_remove(struct platform_device
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
- BUG_ON(!pdata->phy_dev);
+ WARN_ON(pdata->phy_dev);
SMSC_TRACE(pdata, ifdown, "Stopping driver");
- phy_disconnect(pdata->phy_dev);
- pdata->phy_dev = NULL;
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
@@ -2512,6 +2518,12 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
netif_carrier_off(dev);
+ retval = smsc911x_mii_init(pdev, dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+ goto out_free_irq;
+ }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2521,12 +2533,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
"Network interface: \"%s\"", dev->name);
}
- retval = smsc911x_mii_init(pdev, dev);
- if (retval) {
- SMSC_WARN(pdata, probe, "Error
[PATCH 1/2] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0
By default, mdiobus_alloc() sets the PHY's to polling mode, but a
pointer size memcpy means that couple IRQs (depending
on 32-bit or 64-bit kernels) end up being overwritten with
a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHYs location on the mdio
bus. Remove that memcpy and the now unused phy_irq member to force
SMSC911x PHY's into polling mode 100% of the time.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
- int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
- memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
pdata->mii_bus->parent = &pdev->dev;
--
2.5.5
[PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0
By default, mdiobus_alloc() sets the PHYs to polling mode, but a
pointer size memcpy means that a couple IRQs end up being overwritten
with a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHY's location on the
MDIO bus. Remove that memcpy and the now unused phy_irq member to
force the SMSC911x PHYs into polling mode 100% of the time.
Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core")
Signed-off-by: Jeremy Linton
Reviewed-by: Andrew Lunn
Acked-by: Sergei Shtylyov
---
drivers/net/ethernet/smsc/smsc911x.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
- int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
- memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
pdata->mii_bus->parent = &pdev->dev;
--
2.5.5
[PATCH] net: smc91x: ACPI Enable lan91x adapters
Enable lan91x adapters in some ARM machines and models
when booted with an ACPI kernel.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smc91x.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc91x.c
b/drivers/net/ethernet/smsc/smc91x.c
index 18ac52d..fcf69f9 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2203,6 +2203,12 @@ static const struct of_device_id smc91x_match[] = {
};
MODULE_DEVICE_TABLE(of, smc91x_match);
+static const struct acpi_device_id smsc91x_acpi_match[] = {
+ { "LNRO0003", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, smsc91x_acpi_match);
+
/**
* of_try_set_control_gpio - configure a gpio if it exists
*/
@@ -2274,7 +2280,6 @@ static int smc_drv_probe(struct platform_device *pdev)
#if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), &pdev->dev);
if (match) {
- struct device_node *np = pdev->dev.of_node;
u32 val;
/* Optional pwrdwn GPIO configured? */
@@ -2300,7 +2305,8 @@ static int smc_drv_probe(struct platform_device *pdev)
usleep_range(750, 1000);
/* Combination of IO widths supported, default to 16-bit */
- if (!of_property_read_u32(np, "reg-io-width", &val)) {
+ if (!device_property_read_u32(&pdev->dev, "reg-io-width",
+ &val)) {
if (val & 1)
lp->cfg.flags |= SMC91X_USE_8BIT;
if ((val == 0) || (val & 2))
@@ -2479,6 +2485,7 @@ static struct platform_driver smc_driver = {
.name = CARDNAME,
.pm = &smc_drv_pm_ops,
.of_match_table = of_match_ptr(smc91x_match),
+ .acpi_match_table = smsc91x_acpi_match,
},
};
--
2.5.5
Re: [PATCH 2/2] Convert smsc911x to use ACPI as well as DT
On 09/09/2015 11:10 AM, Marc Zyngier wrote: Jeremy, I can see two issues here: we have a screaming interrupt, and we seem to corrupt some workqueue. How did you get this to work? Firmware release? Marc, I'm responding because its been a month or so since my last response, and I haven't forgotten about this issue. First, any custom tianocore build (*) should work. The required changes have been in the last few linaro snapshots as well (http://snapshots.linaro.org/components/kernel/linaro-edk2/, currently at 40) but I personally haven't had a lot of luck with the prebuilt images due to problems unrelated to this change. Others may have more luck. * For those that don't know, tianno core is at: https://github.com/tianocore/edk2.git Use the master branch After setting the environment variables/dependencies appropriately: make -f ArmPlatformPkg/ArmJunoPkg/Makefile all Will create a functional ACPI firmware image for all recent kernels, including ACPI/PCIe ones. Thanks for everyone's patience on this, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Convert smsc911x to use ACPI as well as DT
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 48 +---
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 959aeea..0f21aa3 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -59,7 +59,9 @@
#include
#include
#include
+#include
#include
+#include
#include "smsc911x.h"
@@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops =
{
.tx_writefifo = smsc911x_tx_writefifo_shift,
};
-#ifdef CONFIG_OF
-static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config,
- struct device_node *np)
+static int smsc911x_probe_config(struct smsc911x_platform_config *config,
+struct device *dev)
{
- const char *mac;
u32 width = 0;
- if (!np)
+ if (!dev)
return -ENODEV;
- config->phy_interface = of_get_phy_mode(np);
+ config->phy_interface = device_get_phy_mode(dev);
- mac = of_get_mac_address(np);
- if (mac)
- memcpy(config->mac, mac, ETH_ALEN);
+ device_get_mac_address(dev, config->mac, ETH_ALEN);
- of_property_read_u32(np, "reg-shift", &config->shift);
+ device_property_read_u32(dev, "reg-shift", &config->shift);
- of_property_read_u32(np, "reg-io-width", &width);
+ device_property_read_u32(dev, "reg-io-width", &width);
if (width == 4)
config->flags |= SMSC911X_USE_32BIT;
else
config->flags |= SMSC911X_USE_16BIT;
- if (of_get_property(np, "smsc,irq-active-high", NULL))
+ if (device_property_present(dev, "smsc,irq-active-high"))
config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH;
- if (of_get_property(np, "smsc,irq-push-pull", NULL))
+ if (device_property_present(dev, "smsc,irq-push-pull"))
config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL;
- if (of_get_property(np, "smsc,force-internal-phy", NULL))
+ if (device_property_present(dev, "smsc,force-internal-phy"))
config->flags |= SMSC911X_FORCE_INTERNAL_PHY;
- if (of_get_property(np, "smsc,force-external-phy", NULL))
+ if (device_property_present(dev, "smsc,force-external-phy"))
config->flags |= SMSC911X_FORCE_EXTERNAL_PHY;
- if (of_get_property(np, "smsc,save-mac-address", NULL))
+ if (device_property_present(dev, "smsc,save-mac-address"))
config->flags |= SMSC911X_SAVE_MAC_ADDRESS;
return 0;
}
-#else
-static inline int smsc911x_probe_config_dt(
- struct smsc911x_platform_config *config,
- struct device_node *np)
-{
- return -ENODEV;
-}
-#endif /* CONFIG_OF */
static int smsc911x_drv_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
struct net_device *dev;
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev);
@@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
goto out_disable_resources;
}
- retval = smsc911x_probe_config_dt(&pdata->config, np);
+ retval = smsc911x_probe_config(&pdata->config, &pdev->dev);
if (retval && config) {
/* copy config parameters across to pdata */
memcpy(&pdata->config, config, sizeof(pdata->config));
@@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = {
MODULE_DEVICE_TABLE(of, smsc911x_dt_ids);
#endif
+static const struct acpi_device_id smsc911x_acpi_match[] = {
+ { "ARMH9118", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match);
+
static struct platform_driver smsc911x_driver = {
.probe = smsc911x_drv_probe,
.remove = smsc911x_drv_remove,
@@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = {
.name = SMSC_CHIPNAME,
.pm = SMSC911X_PM_OPS,
.of_match_table = of_match_ptr(smsc911x_dt_ids),
+ .acpi_match_table = ACPI_PTR(smsc911x_acpi_match),
},
};
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Enable smsc911x for use with ACPI
This set of patches enables the front Ethernet port on the ARM Juno development platform when used with an ACPI enabled kernel. These patches covert the of_property* calls in the driver to the DT/ACPI agnostic device_property* calls, and add the arm hardware id to the acpi_match_table. To support the above changes I copied a couple routines from of_net into the properties.c file, and modified them to be ACPI/DT agnostic. I'm not 100% sure this is the correct location for these functions. But I think they are required to avoid having a dozen different implementations scattered across assorted Ethernet adapters that are being enabled to use ACPI properties. Jeremy Linton (2): Add a matching set of device_ functions for determining mac/phy Convert smsc911x to use ACPI as well as DT drivers/base/property.c | 73 drivers/net/ethernet/smsc/smsc911x.c | 48 +++- include/linux/property.h | 4 ++ 3 files changed, 99 insertions(+), 26 deletions(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy
OF has some helper functions for parsing MAC and PHY settings.
In cases where the platform is providing this information rather
than the device itself, there needs to be similar functions for ACPI.
These functions are slightly modified versions of the ones in
of_net which can use information provided via DT or ACPI.
Signed-off-by: Jeremy Linton
---
drivers/base/property.c | 73
include/linux/property.h | 4 +++
2 files changed, 77 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index f3f6d16..2e8cd14 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -16,6 +16,8 @@
#include
#include
#include
+#include
+#include
/**
* device_add_property_set - Add a collection of properties to a device object.
@@ -533,3 +535,74 @@ bool device_dma_is_coherent(struct device *dev)
return coherent;
}
EXPORT_SYMBOL_GPL(device_dma_is_coherent);
+
+/**
+ * device_get_phy_mode - Get phy mode for given device_node
+ * @dev: Pointer to the given device
+ *
+ * The function gets phy interface string from property 'phy-mode' or
+ * 'phy-connection-type', and return its index in phy_modes table, or errno in
+ * error case.
+ */
+int device_get_phy_mode(struct device *dev)
+{
+ const char *pm;
+ int err, i;
+
+ err = device_property_read_string(dev, "phy-mode", &pm);
+ if (err < 0)
+ err = device_property_read_string(dev,
+ "phy-connection-type", &pm);
+ if (err < 0)
+ return err;
+
+ for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
+ if (!strcasecmp(pm, phy_modes(i)))
+ return i;
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(device_get_phy_mode);
+
+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
+}
+
+/**
+ * Search the device tree for the best MAC address to use. 'mac-address' is
+ * checked first, because that is supposed to contain to "most recent" MAC
+ * address. If that isn't set, then 'local-mac-address' is checked next,
+ * because that is the default address. If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree.
+ *
+ * Note that the 'address' property is supposed to contain a virtual address of
+ * the register set, but some DTS files have redefined that property to be the
+ * MAC address.
+ *
+ * All-zero MAC addresses are rejected, because those could be properties that
+ * exist in the device tree, but were not set by U-Boot. For example, the
+ * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
+ * addresses. Some older U-Boots only initialized 'local-mac-address'. In
+ * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
+ * but is all zeros.
+*/
+void *device_get_mac_address(struct device *dev, char *addr, int alen)
+{
+ addr = device_get_mac_addr(dev, "mac-address", addr, alen);
+ if (addr)
+ return addr;
+
+ addr = device_get_mac_addr(dev, "local-mac-address", addr, alen);
+ if (addr)
+ return addr;
+
+ return device_get_mac_addr(dev, "address", addr, alen);
+}
+EXPORT_SYMBOL(device_get_mac_address);
diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..a59c6ee 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -166,4 +166,8 @@ void device_add_property_set(struct device *dev, struct
property_set *pset);
bool device_dma_is_coherent(struct device *dev);
+int device_get_phy_mode(struct device *dev);
+
+void *device_get_mac_address(struct device *dev, char *addr, int alen);
+
#endif /* _LINUX_PROPERTY_H_ */
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy
Hello Robin,
On 08/13/2015 06:57 AM, Robin Murphy wrote:
+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
+}
Not sure I understand the logic here - "return the same thing we were
given if we updated it, or null if we didn't". It's only indicating
success/failure (the caller can perfectly well cast its own buffer to a
void * if it needs to), so why wouldn't you just return a normal int
error code?
No particular reason, other than initially I was trying to keep the
function as similar as possible to the one in of_net. AKA copy paste
job. I can convert the return types, but I was trying for a simple
function rename. That way the users of the of version could be converted
with relative ease, and the drivers which invented their own version of
these functions could be changed to use this instead. Of course, that
plan took a blow, when I added the addr/alen parameters.
Same thing applies for the other function.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a matching set of device_ functions for determining mac/phy
On 08/12/2015 05:13 PM, Florian Fainelli wrote:
On 12/08/15 15:06, Jeremy Linton wrote:
+
+static void *device_get_mac_addr(struct device *dev,
+const char *name, char *addr,
+int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
The DT counterpart has an additional check on the properly length to be
ETH_ALEN, you might want to have such check here for consistency and
correctness.
I will add it back,
Thanks,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used
On 08/17/2015 03:45 PM, Guenter Roeck wrote:
Commit 0b50dc4fc971 ("Convert smsc911x to use ACPI as well as DT") makes
the call to smsc911x_probe_config() unconditional, and no longer fails if
there is no device node. device_get_phy_mode() is called unconditionally,
and if there is no phy node configured returns an error code. This error
code is assigned to phy_interface, and interpreted elsewhere in the code
as valid phy mode. This in turn causes qemu to crash when running a
variant of realview_pb_defconfig.
Thanks for catching that! I have a couple other minor cleanups per the
reviewers.
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used
On 08/17/2015 05:14 PM, Guenter Roeck wrote: One nitpick I noticed after sending my patch: dev can never be NULL in smsc911x_probe_config(), so it does not really make sense to check if it is NULL. No, it doesn't... it should really be something like if (dev_fwnode(dev)) But dev_fwnode is static inline in property.c, and i'm pretty sure that still isn't 100% correct.The best plan might just be to remove the check, and abort on failure to find the phy property per your patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Small cleanups for smsc and device property
These patches are against net-next. This patch set adds a length check to device_get_mac_addr() before calling is_valid_ether_addr(), it also removes an unisssary dev==null check. The remainder is updates to the comments. Jeremy Linton (2): device property: Add ETH_ALEN check, update comments. smsc911x: Remove dev==NULL check. drivers/base/property.c | 21 + drivers/net/ethernet/smsc/smsc911x.c | 3 --- 2 files changed, 13 insertions(+), 11 deletions(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] device property: Add ETH_ALEN check, update comments.
This patch adds MAC address length check back into
the device_get_mac_addr() function before calling
is_valid_ether_addr() similar to the way the OF
routine does it.
Update the comments for the two new functions.
Signed-off-by: Jeremy Linton
---
drivers/base/property.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2e8cd14..4c20828 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -537,7 +537,7 @@ bool device_dma_is_coherent(struct device *dev)
EXPORT_SYMBOL_GPL(device_dma_is_coherent);
/**
- * device_get_phy_mode - Get phy mode for given device_node
+ * device_get_phy_mode - Get phy mode for given device
* @dev: Pointer to the given device
*
* The function gets phy interface string from property 'phy-mode' or
@@ -570,13 +570,18 @@ static void *device_get_mac_addr(struct device *dev,
{
int ret = device_property_read_u8_array(dev, name, addr, alen);
- if (ret == 0 && is_valid_ether_addr(addr))
+ if (ret == 0 && alen == ETH_ALEN && is_valid_ether_addr(addr))
return addr;
return NULL;
}
/**
- * Search the device tree for the best MAC address to use. 'mac-address' is
+ * device_get_mac_address - Get the MAC for a given device
+ * @dev: Pointer to the device
+ * @addr: Address of buffer to store the MAC in
+ * @alen: Length of the buffer pointed to by addr, should be ETH_ALEN
+ *
+ * Search the firmware node for the best MAC address to use. 'mac-address' is
* checked first, because that is supposed to contain to "most recent" MAC
* address. If that isn't set, then 'local-mac-address' is checked next,
* because that is the default address. If that isn't set, then the obsolete
@@ -587,11 +592,11 @@ static void *device_get_mac_addr(struct device *dev,
* MAC address.
*
* All-zero MAC addresses are rejected, because those could be properties that
- * exist in the device tree, but were not set by U-Boot. For example, the
- * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
- * addresses. Some older U-Boots only initialized 'local-mac-address'. In
- * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
- * but is all zeros.
+ * exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'mac-address' and 'local-mac-address', with
+ * zero MAC addresses. Some older U-Boots only initialized
'local-mac-address'.
+ * In this case, the real MAC is in 'local-mac-address', and 'mac-address'
+ * exists but is all zeros.
*/
void *device_get_mac_address(struct device *dev, char *addr, int alen)
{
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] smsc911x: Remove dev==NULL check.
The dev==NULL check in smsc911x_probe_config is useless and isn't providing any additional protection. If a fwnode doesn't exist then an appropriate error should be returned by device_get_phy_mode() covering the original case of a missing of/fwnode. Signed-off-by: Jeremy Linton --- drivers/net/ethernet/smsc/smsc911x.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 34f9768..6eef325 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2370,9 +2370,6 @@ static int smsc911x_probe_config(struct smsc911x_platform_config *config, int phy_interface; u32 width = 0; - if (!dev) - return -ENODEV; - phy_interface = device_get_phy_mode(dev); if (phy_interface < 0) return phy_interface; -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] xgene mac/phy cleanup
This patch converts the xgene driver to use some common ACPI/DT agnostic functions for retrieving MAC and PHY settings. I don't have a way to test them at the moment, so if someone can verify they work that would be great. BTW: These patches are against net-next. Thanks, Jeremy Linton (1): net: xgene Remove xgene specific phy and MAC lookup functions drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 38 ++-- 1 file changed, 2 insertions(+), 36 deletions(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: xgene Remove xgene specific phy and MAC lookup functions
Convert the xgene_get_mac_address to device_get_mac_address(), and
xgene_get_phy_mode() to device_get_phy_mode().
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 38 ++--
1 file changed, 2 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 299eb43..4f68d19 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -905,40 +905,6 @@ static int xgene_get_port_id_dt(struct device *dev, struct
xgene_enet_pdata *pda
return ret;
}
-static int xgene_get_mac_address(struct device *dev,
-unsigned char *addr)
-{
- int ret;
-
- ret = device_property_read_u8_array(dev, "local-mac-address", addr, 6);
- if (ret)
- ret = device_property_read_u8_array(dev, "mac-address",
- addr, 6);
- if (ret)
- return -ENODEV;
-
- return ETH_ALEN;
-}
-
-static int xgene_get_phy_mode(struct device *dev)
-{
- int i, ret;
- char *modestr;
-
- ret = device_property_read_string(dev, "phy-connection-type",
- (const char **)&modestr);
- if (ret)
- ret = device_property_read_string(dev, "phy-mode",
- (const char **)&modestr);
- if (ret)
- return -ENODEV;
-
- for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
- if (!strcasecmp(modestr, phy_modes(i)))
- return i;
- }
- return -ENODEV;
-}
static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
{
@@ -998,12 +964,12 @@ static int xgene_enet_get_resources(struct
xgene_enet_pdata *pdata)
if (ret)
return ret;
- if (xgene_get_mac_address(dev, ndev->dev_addr) != ETH_ALEN)
+ if (!device_get_mac_address(dev, ndev->dev_addr, ETH_ALEN))
eth_hw_addr_random(ndev);
memcpy(ndev->perm_addr, ndev->dev_addr, ndev->addr_len);
- pdata->phy_mode = xgene_get_phy_mode(dev);
+ pdata->phy_mode = device_get_phy_mode(dev);
if (pdata->phy_mode < 0) {
dev_err(dev, "Unable to get phy-connection-type\n");
return pdata->phy_mode;
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used
On 08/26/2015 12:04 PM, Tony Lindgren wrote:
* Guenter Roeck [150817 13:48]:
Commit 0b50dc4fc971 ("Convert smsc911x to use ACPI as well as DT") makes
Looks like this change makes at least omap boards using smsc911x
fail with -22 for me in Linux next.
Do any of the the device tree configured smsc911x devices actually
have a phy configured?
Tony,
Looks like all the ones in the kernel boot/dts directory have a phy
including the omap3-lilly except for the ste-snowball.dts.
Do you have smsc,force-internal-phy set instead?
Thanks,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] smsc911x: Ignore error return from device_get_phy_mode()
On 08/26/2015 01:49 PM, Guenter Roeck wrote:
Check the return value from device_property_read_u32() to see if there
is a suitable firmware interface to read the data, and abort if not.
The function should return -ENXIO in that case; however, it returns
-ENODATA. Check for both.
Fixes: 62ee783bf1f8 ("smsc911x: Fix crash seen if neither ACPI nor OF is configured
or used")
Signed-off-by: Guenter Roeck
---
Needs testing. RFC because I am not sure if the -ENODATA check is acceptable.
I'm not really sure about it myself. I can think of cases where it might
cause problems. That said it does work in an ACPI environment with or
without the _DSD block. If the DSD/property isn't set, obviously the
device doesn't configure (but it doesn't crash either) so that is good
and an overall improvement for ACPI.
Also, I personally might have hoisted the reg-io-width ahead of the
device_get_phy_mode() and removed the phy checks, but I don't imagine
there is much functional difference at this point.
Tested-by: Jeremy Linton
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next v3 1/2] device property: Return -ENXIO if there is no suitable FW interface
On 08/26/2015 10:27 PM, Guenter Roeck wrote: Return -ENXIO if device property array access functions don't find a suitable firmware interface. This lets drivers decide if they should use available platform data instead. Works fine on an ACPI based ARM system. Thanks, for taking care of this. Tested-by: Jeremy Linton -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] Convert smsc911x to use ACPI as well as DT
Marc, |FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a |horrible death: Sorry about the delay, I didn't see this message. |How did you get this to work? Firmware release? Yes, it's a firmware problem with the ACPI tables. It is likely you need _DSD changes for the SMSC. If your trying to run juno with ACPI there are a few other ACPI table changes you will need beyond what was in the last linaro release. I wanted to just provide a direct link to the correct firmware in this response, but it seems the required version is sort of up in the air at the moment. Be assured I will really push on this next week, and will provide a follow up here. Originally, the lack of DSD wasn't a problem, but as this patch evolved the fact that it consolidates a number of configuration code paths balancing that got tricky, and ACPI machines with "bad" tables were the victims (rather than breaking something I can't control). In the meantime, if this is blocking anyone I can provide instructions on how to update the required ACPI tables. Thanks, -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] device property: Don't overwrite addr when failing in device_get_mac_address
On 09/03/2015 05:59 PM, Julien Grall wrote: The function device_get_mac_address is trying different property names in order to get the mac address. To check the return value, the variable addr (which contain the buffer pass by the caller) will be re-used. This means that if the previous property is not found, the next property will be read using a NULL buffer. Thanks for catching that! I checked the OF version to see if it has the same problem, but of course it doesn't because I added the logic to pass the buffer into the routine. Reviewed-by: Jeremy Linton -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: smsc911x: Fix unload crash when link is up
The smsc911x driver will crash if it is rmmod'ed while the netdev is up like: Call trace: phy_detach+0x94/0x150 phy_disconnect+0x40/0x50 smsc911x_stop+0x104/0x128 [smsc911x] __dev_close_many+0xb4/0x138 dev_close_many+0xbc/0x190 rollback_registered_many+0x140/0x460 rollback_registered+0x68/0xb0 unregister_netdevice_queue+0x100/0x118 unregister_netdev+0x28/0x38 smsc911x_drv_remove+0x58/0x130 [smsc911x] platform_drv_remove+0x30/0x50 device_release_driver_internal+0x15c/0x1f8 driver_detach+0x54/0x98 bus_remove_driver+0x64/0xe8 driver_unregister+0x34/0x60 platform_driver_unregister+0x20/0x30 smsc911x_cleanup_module+0x14/0xbca8 [smsc911x] SyS_delete_module+0x1e8/0x238 __sys_trace_return+0x0/0x4 This is caused by the mdiobus being unregistered/free'd and the code in phy_detach() attempting to manipulate mdio related structures from unregister_netdev() calling close() To fix this, we delay the mdiobus teardown until after the netdev is deregistered. Reported-by: Matt Sealey Signed-off-by: Jeremy Linton --- drivers/net/ethernet/smsc/smsc911x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 012fb66eed8d..f0afb88d7bc2 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device *pdev) pdata = netdev_priv(dev); BUG_ON(!pdata); BUG_ON(!pdata->ioaddr); - WARN_ON(dev->phydev); SMSC_TRACE(pdata, ifdown, "Stopping driver"); + unregister_netdev(dev); + mdiobus_unregister(pdata->mii_bus); mdiobus_free(pdata->mii_bus); - unregister_netdev(dev); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "smsc911x-memory"); if (!res) -- 2.13.6
Re: [PATCH] net: smsc911x: Fix unload crash when link is up
Hi, On 03/06/2018 09:23 AM, Andrew Lunn wrote: This is caused by the mdiobus being unregistered/free'd and the code in phy_detach() attempting to manipulate mdio related structures from unregister_netdev() calling close() To fix this, we delay the mdiobus teardown until after the netdev is deregistered. Reported-by: Matt Sealey Signed-off-by: Jeremy Linton --- drivers/net/ethernet/smsc/smsc911x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 012fb66eed8d..f0afb88d7bc2 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2335,14 +2335,14 @@ static int smsc911x_drv_remove(struct platform_device *pdev) pdata = netdev_priv(dev); BUG_ON(!pdata); BUG_ON(!pdata->ioaddr); - WARN_ON(dev->phydev); Hi Jeremy I assume this WARN_ON() also fired? It would be good to comment about why you removed it, that the code now handles that case. Yes, the phydev is started and assigned in the netdev _open and stopped/set to null in the _stop. Since the module remove is not blocked by having the netdev active, and unregister_netdev closes out active connections, the WARN_ON would needlessly trigger if the netdev was still open. Apart from that Reviewed-by: Andrew Lunn Thanks for looking at this.
Re: mvpp2 crash under load.
Hi, First, thanks for taking a look at this. On 01/23/2018 01:53 AM, Antoine Tenart wrote: Hi Jeremy, On Mon, Jan 22, 2018 at 05:14:27PM -0600, Jeremy Linton wrote: I'm running 4.15rc7 and hitting the following crash on the MACCHIATObin. This is 100% reproducible once the adapter is given any load. Within a few seconds of starting a scp or nfs copies inbound to the machine it dies like this: [12544.192436] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx processing [12548.513734] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx processing [12548.623574] mvpp2 f400.ethernet eth2: wrong cpu on the end of Tx processing I believe this is the root cause of this issue: txq_done() is scheduled on the wrong CPU and we know it can't run on 2 CPUs at the same time. We had a similar issue (same stack trace, different root cause): 082297e61480c4d72ed75b31077e74aca0e7c799 I'm pretty sure I already had that patch, I've rebased to 4.15rc9 and it continues. I also cherry picked "net: mvpp2: only free the TSO header buffers when it was allocated" from net-next which didn't appear to fix it either. Thanks, Thanks for reporting this! Antoine [12548.630943] Unable to handle kernel paging request at virtual address 97ffd6fdd28000e8 [12548.638897] Mem abort info: [12548.641703] ESR = 0x9604 [12548.644775] Exception class = DABT (current EL), IL = 32 bits [12548.650720] SET = 0, FnV = 0 [12548.653795] EA = 0, S1PTW = 0 [12548.656952] Data abort info: [12548.659846] ISV = 0, ISS = 0x0004 [12548.663700] CM = 0, WnR = 0 [12548.84] [97ffd6fdd28000e8] address between user and kernel address ranges [12548.673855] Internal error: Oops: 9604 [#1] SMP [12548.678757] Modules linked in: ax88179_178a usbnet ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_brox [12548.749992] xhci_plat_hcd ahci_platform [last unloaded: usbnet] [12548.756034] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.15.0-0.rc7.git0.1.fc28.aarch64 #1 [12548.764249] Hardware name: Marvell Armada 8040 MacchiatoBin/Armada 8040 MacchiatoBin, BIOS EDK II Oct 2 2017 [12548.774210] pstate: 4045 (nZcv daif +PAN -UAO) [12548.779033] pc : consume_skb+0x1c/0xd8 [12548.782802] lr : __dev_kfree_skb_any+0x58/0x68 [12548.787264] sp : 0801bc30 [12548.790594] x29: 0801bc30 x28: 831bed412a40 [12548.795934] x27: 831bf7ce8000 x26: 0001 [12548.801273] x25: 27e28d746120 x24: 831bed412948 [12548.806612] x23: 0018 x22: 27e28d746120 [12548.811950] x21: 0007 x20: 0001 [12548.817289] x19: 97ffd6fdd284 x18: 0010 [12548.822627] x17: x16: 27e28d5bb4a0 [12548.827966] x15: x14: 737365636f727020 [12548.833305] x13: 785420666f20646e x12: 6520656874206e6f [12548.838643] x11: 27e28e07b448 x10: 27e28d35eb00 [12548.843981] x9 : 2074656e72656874 x8 : 0005 [12548.849319] x7 : b26f x6 : b66f [12548.854658] x5 : 0001 x4 : [12548.859995] x3 : 0001 x2 : 97ffd6fdd284 [12548.865333] x1 : 0001 x0 : 27e28d5bb4f8 [12548.870673] Process swapper/3 (pid: 0, stack limit = 0x71feb006) [12548.877404] Call trace: [12548.879863] consume_skb+0x1c/0xd8 [12548.883281] __dev_kfree_skb_any+0x58/0x68 [12548.887411] mvpp2_txq_bufs_free.isra.53+0xd0/0x118 [mvpp2] [12548.893017] mvpp2_txq_done.isra.68+0xb0/0xf8 [mvpp2] [12548.898100] mvpp2_tx_done+0xb4/0x118 [mvpp2] [12548.902484] mvpp2_poll+0x5c4/0x658 [mvpp2] [12548.906688] net_rx_action+0x160/0x3f8 [12548.910456] __do_softirq+0x138/0x344 [12548.914137] irq_exit+0xd0/0x100 [12548.917381] __handle_domain_irq+0x6c/0xc0 [12548.921497] gic_handle_irq+0x60/0xb0 [12548.925175] el1_irq+0xd8/0x180 [12548.928331] arch_cpu_idle+0x30/0x188 [12548.932011] do_idle+0x138/0x1f8 [12548.935255] cpu_startup_entry+0x2c/0x30 [12548.939197] secondary_start_kernel+0x11c/0x130 [12548.943750] Code: aa0003f3 aa1e03e0 d503201f b4000153 (b940e660) [12548.949876] ---[ end trace c9cfd11479961f0c ]--- [12548.954515] Kernel panic - not syncing: Fatal exception in interrupt [12548.960900] SMP: stopping secondary CPUs [12548.964845] Kernel Offset: 0x27e284d5 from 0x0800 [12548.970967] CPU features: 0x002000 [12548.974384] Memory Limit: none Its interesting that the wrong CPU messages are still appearing despite the irqbalance change from MarkZ. I disabled irqbalance and tried starting it in single queue mode and it did the same thing.
[RESEND/BUG PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0
By default, mdiobus_alloc() sets the PHYs to polling mode, but a
pointer size memcpy means that a couple IRQs end up being overwritten
with a value of 0. This means that PHY_POLL is disabled and results
in unpredictable behavior depending on the PHY's location on the
MDIO bus. Remove that memcpy and the now unused phy_irq member to
force the SMSC911x PHYs into polling mode 100% of the time.
Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core")
Signed-off-by: Jeremy Linton
Reviewed-by: Andrew Lunn
Acked-by: Sergei Shtylyov
---
drivers/net/ethernet/smsc/smsc911x.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..b5ab5e1 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -116,7 +116,6 @@ struct smsc911x_data {
struct phy_device *phy_dev;
struct mii_bus *mii_bus;
- int phy_irq[PHY_MAX_ADDR];
unsigned int using_extphy;
int last_duplex;
int last_carrier;
@@ -1073,7 +1072,6 @@ static int smsc911x_mii_init(struct platform_device *pdev,
pdata->mii_bus->priv = pdata;
pdata->mii_bus->read = smsc911x_mii_read;
pdata->mii_bus->write = smsc911x_mii_write;
- memcpy(pdata->mii_bus->irq, pdata->phy_irq, sizeof(pdata->mii_bus));
pdata->mii_bus->parent = &pdev->dev;
--
2.5.5
Re: [RESEND/BUG PATCH v3] net: smsc911x: Fix bug where PHY interrupts are overwritten by 0
On 07/05/2016 01:45 PM, Sergei Shtylyov wrote: The patch has been merged to 4.7-rc6, why resend it? Sorry, I must have missed the merge. Thanks,
[PATCH v2] net: smc91x: ACPI Enable lan91x adapters
Enable lan91x adapters in some ARM machines and models
when booted with an ACPI kernel.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smc91x.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc91x.c
b/drivers/net/ethernet/smsc/smc91x.c
index 18ac52d..726b80f 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2195,6 +2195,12 @@ static void smc_release_datacs(struct platform_device
*pdev, struct net_device *
}
}
+static const struct acpi_device_id smc91x_acpi_match[] = {
+ { "LNRO0003", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, smc91x_acpi_match);
+
#if IS_BUILTIN(CONFIG_OF)
static const struct of_device_id smc91x_match[] = {
{ .compatible = "smsc,lan91c94", },
@@ -2274,7 +2280,6 @@ static int smc_drv_probe(struct platform_device *pdev)
#if IS_BUILTIN(CONFIG_OF)
match = of_match_device(of_match_ptr(smc91x_match), &pdev->dev);
if (match) {
- struct device_node *np = pdev->dev.of_node;
u32 val;
/* Optional pwrdwn GPIO configured? */
@@ -2300,7 +2305,8 @@ static int smc_drv_probe(struct platform_device *pdev)
usleep_range(750, 1000);
/* Combination of IO widths supported, default to 16-bit */
- if (!of_property_read_u32(np, "reg-io-width", &val)) {
+ if (!device_property_read_u32(&pdev->dev, "reg-io-width",
+ &val)) {
if (val & 1)
lp->cfg.flags |= SMC91X_USE_8BIT;
if ((val == 0) || (val & 2))
@@ -2478,7 +2484,8 @@ static struct platform_driver smc_driver = {
.driver = {
.name = CARDNAME,
.pm = &smc_drv_pm_ops,
- .of_match_table = of_match_ptr(smc91x_match),
+ .of_match_table = of_match_ptr(smc91x_match),
+ .acpi_match_table = smc91x_acpi_match,
},
};
--
2.5.5
Re: [PATCH 2/3 v2] net: smsc911x: request and deassert optional RESET GPIO
Hi Linus,
On 08/24/2016 07:59 AM, Linus Walleij wrote:
On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor but
connected to a GPIO line, so that the operating system must
explicitly deassert RESET before use.
Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.
Hmm, at least in our hardware case AFAIK (juno/lan9118) the hardware
reset line on the lan9118 is active low, but the chip itself is
documented as having internal pullups so that it may be left unconnected.
Which microchip/smsc chip are we talking about? because it seems that
you probably want the GPIO pin to be in any state (hi-Z, or just high)
besides the one you selected here.
Beyond that, is it not possible for the firmware to get the reset pin in
the correct configuration, so that linux doesn't have to mess with it?
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Use devm_gpiod_request_optiona() and request the line with
GPIOD_OUT_LOW so it is deasserted immediately if active.
---
drivers/net/ethernet/smsc/smsc911x.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index ca3134540d2d..8ab8d4b9614b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -62,6 +62,7 @@
#include
#include
#include
+#include
#include "smsc911x.h"
@@ -147,6 +148,9 @@ struct smsc911x_data {
/* regulators */
struct regulator_bulk_data supplies[SMSC911X_NUM_SUPPLIES];
+ /* Reset GPIO */
+ struct gpio_desc *reset_gpiod;
+
/* clock */
struct clk *clk;
};
@@ -438,6 +442,11 @@ static int smsc911x_request_resources(struct
platform_device *pdev)
netdev_err(ndev, "couldn't get regulators %d\n",
ret);
+ /* Request optional RESET GPIO */
+ pdata->reset_gpiod = devm_gpiod_get_optional(&pdev->dev,
+"reset",
+GPIOD_OUT_LOW);
+
/* Request clock */
pdata->clk = clk_get(&pdev->dev, NULL);
if (IS_ERR(pdata->clk))
Re: [PATCH 3/3 v2] net: smsc911x: add wake-up event interrupt support
Hi Linus,
On 08/24/2016 07:59 AM, Linus Walleij wrote:
The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.
This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.
The patch is inspired by the wakeup code in the RTC
subsystem.
The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.
Cc: Sudeep Holla
Cc: Tony Lindgren
Cc: Florian Fainelli
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
this is handled from the irq core when you call
dev_pm_set_wake_irq() as we do.
---
drivers/net/ethernet/smsc/smsc911x.c | 42
1 file changed, 42 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8ab8d4b9614b..8fffc1dc2bdd 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -63,6 +63,7 @@
#include
#include
#include
+#include
#include "smsc911x.h"
@@ -151,6 +152,9 @@ struct smsc911x_data {
/* Reset GPIO */
struct gpio_desc *reset_gpiod;
+ /* PME interrupt */
+ int pme_irq;
+
/* clock */
struct clk *clk;
};
@@ -1881,6 +1885,19 @@ static irqreturn_t smsc911x_irqhandler(int irq, void
*dev_id)
return serviced;
}
+static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
+{
+ struct net_device *dev = dev_id;
+ struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
+
+ SMSC_TRACE(pdata, pm, "wakeup event");
+ pm_wakeup_event(&dev->dev, 50);
+ /* This signal is active for 50 ms, wait for it to deassert */
+ usleep_range(5, 10);
+
+ return IRQ_HANDLED;
+}
+
#ifdef CONFIG_NET_POLL_CONTROLLER
static void smsc911x_poll_controller(struct net_device *dev)
{
@@ -2501,6 +2518,31 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
goto out_disable_resources;
}
+ irq = platform_get_irq(pdev, 1);
+ if (irq == -EPROBE_DEFER) {
+ retval = -EPROBE_DEFER;
+ goto out_disable_resources;
+ /* It's perfectly fine to not have a PME IRQ */
+ } else if (irq > 0) {
+ /*
+* The Power Management Event (PME) IRQ appears as
+* a pulse waking up the system from sleep in response to a
+* network event.
+*/
+ retval = request_threaded_irq(irq, NULL,
+ smsc911x_pme_irq_thread,
+ IRQF_ONESHOT, "smsc911x-pme",
+ dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe,
+ "Unable to claim requested PME irq: %d", irq);
+ goto out_disable_resources;
+ }
+ pdata->pme_irq = irq;
+ device_init_wakeup(&pdev->dev, true);
+ dev_pm_set_wake_irq(&pdev->dev, irq);
+ }
+
netif_carrier_off(dev);
retval = register_netdev(dev);
The cleanup code in drv_remove seems to be missing, am I missing something?
Also, do you want the wake-up to be active if the interface is downed?
Thanks,
Re: [PATCH 3/3 v2] net: smsc911x: add wake-up event interrupt support
Hi,
On 08/24/2016 07:59 AM, Linus Walleij wrote:
The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.
This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.
The patch is inspired by the wakeup code in the RTC
subsystem.
The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.
Having looked at a couple of the supported smsc chips, it seems they can
route the wakeup through the chip's interrupt as well. If you add code
to support this, it should work on a lot of the smsc911x devices rather
than just the dragonboard.
Thanks,
Cc: Sudeep Holla
Cc: Tony Lindgren
Cc: Florian Fainelli
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
this is handled from the irq core when you call
dev_pm_set_wake_irq() as we do.
---
drivers/net/ethernet/smsc/smsc911x.c | 42
1 file changed, 42 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 8ab8d4b9614b..8fffc1dc2bdd 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -63,6 +63,7 @@
#include
#include
#include
+#include
#include "smsc911x.h"
@@ -151,6 +152,9 @@ struct smsc911x_data {
/* Reset GPIO */
struct gpio_desc *reset_gpiod;
+ /* PME interrupt */
+ int pme_irq;
+
/* clock */
struct clk *clk;
};
@@ -1881,6 +1885,19 @@ static irqreturn_t smsc911x_irqhandler(int irq, void
*dev_id)
return serviced;
}
+static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
+{
+ struct net_device *dev = dev_id;
+ struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
+
+ SMSC_TRACE(pdata, pm, "wakeup event");
+ pm_wakeup_event(&dev->dev, 50);
+ /* This signal is active for 50 ms, wait for it to deassert */
+ usleep_range(5, 10);
+
+ return IRQ_HANDLED;
+}
+
#ifdef CONFIG_NET_POLL_CONTROLLER
static void smsc911x_poll_controller(struct net_device *dev)
{
@@ -2501,6 +2518,31 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
goto out_disable_resources;
}
+ irq = platform_get_irq(pdev, 1);
+ if (irq == -EPROBE_DEFER) {
+ retval = -EPROBE_DEFER;
+ goto out_disable_resources;
+ /* It's perfectly fine to not have a PME IRQ */
+ } else if (irq > 0) {
+ /*
+* The Power Management Event (PME) IRQ appears as
+* a pulse waking up the system from sleep in response to a
+* network event.
+*/
+ retval = request_threaded_irq(irq, NULL,
+ smsc911x_pme_irq_thread,
+ IRQF_ONESHOT, "smsc911x-pme",
+ dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe,
+ "Unable to claim requested PME irq: %d", irq);
+ goto out_disable_resources;
+ }
+ pdata->pme_irq = irq;
+ device_init_wakeup(&pdev->dev, true);
+ dev_pm_set_wake_irq(&pdev->dev, irq);
+ }
+
netif_carrier_off(dev);
retval = register_netdev(dev);
[PATCH 1/2] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.
The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 51
1 file changed, 28 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..6d6dcfe 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1099,15 +1099,8 @@ static int smsc911x_mii_init(struct platform_device
*pdev,
goto err_out_free_bus_2;
}
- if (smsc911x_mii_probe(dev) < 0) {
- SMSC_WARN(pdata, probe, "Error registering mii bus");
- goto err_out_unregister_bus_3;
- }
-
return 0;
-err_out_unregister_bus_3:
- mdiobus_unregister(pdata->mii_bus);
err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
err_out_1:
@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+ int retval;
- /* if the phy is not yet registered, retry later*/
+ /* find and start the given phy */
if (!dev->phydev) {
- SMSC_WARN(pdata, hw, "phy_dev is NULL");
- return -EAGAIN;
+ if (smsc911x_mii_probe(dev) < 0) {
+ SMSC_WARN(pdata, probe, "Error starting phy");
+ retval = -EAGAIN;
+ goto out;
+ }
}
/* Reset the LAN911x */
if (smsc911x_soft_reset(pdata)) {
SMSC_WARN(pdata, hw, "soft reset failed");
- return -EIO;
+ retval = -EIO;
+ goto mii_free_out;
}
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1600,7 +1598,8 @@ static int smsc911x_open(struct net_device *dev)
if (!pdata->software_irq_signal) {
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
- return -ENODEV;
+ retval = -ENODEV;
+ goto mii_free_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1646,6 +1645,12 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
+
+mii_free_out:
+ phy_disconnect(dev->phydev);
+ dev->phydev = NULL;
+out:
+ return retval;
}
/* Entry point for stopping the interface */
@@ -1668,8 +1673,11 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
/* Bring the PHY down */
- if (dev->phydev)
+ if (dev->phydev) {
phy_stop(dev->phydev);
+ phy_disconnect(dev->phydev);
+ dev->phydev = NULL;
+ }
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2291,11 +2299,10 @@ static int smsc911x_drv_remove(struct platform_device
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
- BUG_ON(!dev->phydev);
+ WARN_ON(dev->phydev);
SMSC_TRACE(pdata, ifdown, "Stopping driver");
- phy_disconnect(dev->phydev);
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
@@ -2494,6 +2501,12 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
netif_carrier_off(dev);
+ retval = smsc911x_mii_init(pdev, dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+ goto out_free_irq;
+ }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2503,12 +2516,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
"Network interface: \"%s\"", dev->name);
}
- retval = smsc911x_mii_init(pdev, dev);
- if (retval) {
- SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
- goto out_unregister_netdev_5;
- }
-
spin_lock_irq(&pdata->mac_lock);
/* Check if mac address has been specified when bringing interface up */
@@ -2544,8 +255
[PATCH 0/2] net: smsc911x: Move phy and interrupt config
Hi, The smsc911x driver is doing a number of things in its probe routine that should be delayed until the interface is started. Because of this, the module cannot be unloaded, the phy states are incorrect/stale if the interface isn't running, open's unnecessarily fail causing network configuration problems, and the /proc/irq nodes are incorrectly named. Clean up a number of these problems by moving the mdio and interrupt configuration into the smsc911x_open routine. Jeremy Linton (2): net: smsc911x: Fix register_netdev, phy startup, driver unload ordering net/smsc911x: Move interrupt allocation to open/stop drivers/net/ethernet/smsc/smsc911x.c | 89 +--- 1 file changed, 42 insertions(+), 47 deletions(-) -- 2.5.5
[PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop
The /proc/irq/xx information is incorrect for smsc911x because
the request_irq is happening before the register_netdev has the
proper device name. Moving it to the open also fixes the case
of when the device is renamed.
Reported-by: Will Deacon
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 50 +++-
1 file changed, 20 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 6d6dcfe..8ef9776 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -154,6 +154,8 @@ struct smsc911x_data {
/* Easy access to information */
#define __smsc_shift(pdata, reg) ((reg) << ((pdata)->config.shift))
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id);
+
static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
{
if (pdata->config.flags & SMSC911X_USE_32BIT)
@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+ int irq_flags;
/* find and start the given phy */
if (!dev->phydev) {
@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
+ irq_flags = irq_get_trigger_type(dev->irq);
+ if (request_irq(dev->irq, smsc911x_irqhandler,
+ irq_flags | IRQF_SHARED, dev->name, dev)) {
+ SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+ goto mii_free_out;
+ }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
- goto mii_free_out;
+ goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
-
+irq_stop_out:
+ free_irq(dev->irq, dev);
mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);
+ free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
phy_disconnect(dev->phydev);
dev->phydev = NULL;
}
+ netif_carrier_off(dev);
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2307,7 +2322,6 @@ static int smsc911x_drv_remove(struct platform_device
*pdev)
mdiobus_free(pdata->mii_bus);
unregister_netdev(dev);
- free_irq(dev->irq, dev);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"smsc911x-memory");
if (!res)
@@ -2392,8 +2406,7 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev);
struct resource *res;
- unsigned int intcfg = 0;
- int res_size, irq, irq_flags;
+ int res_size, irq;
int retval;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -2432,7 +2445,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
pdata = netdev_priv(dev);
dev->irq = irq;
- irq_flags = irq_get_trigger_type(irq);
pdata->ioaddr = ioremap_nocache(res->start, res_size);
pdata->dev = dev;
@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
if (retval < 0)
goto out_disable_resources;
- /* configure irq polarity and type before connecting isr */
- if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
- intcfg |= INT_CFG_IRQ_POL_;
-
- if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
- intcfg |= INT_CFG_IRQ_TYPE_;
-
- smsc911x_reg_write(pdata, INT_CFG, intcfg);
-
- /* Ensure interrupts are globally disabled before connecting ISR */
- smsc911x_disable_irq_chip(dev);
-
- retval = request_irq(dev->irq, smsc911x_irqhandler,
-irq_flags | IRQF_SHARED, dev->name, dev);
- if (retval) {
- SMSC_WARN(pdata, probe
Re: [PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop
Hi, On 09/01/2016 02:45 PM, Andrew Lunn wrote: On Thu, Sep 01, 2016 at 01:47:44PM -0500, Jeremy Linton wrote: Hi Andrew, Thanks for taking a look at this! On 09/01/2016 12:06 PM, Andrew Lunn wrote: Hi Jeremy Please don't add forward references. Move the function earlier in the file. Ok, but I thought it was a fairly large move due to further dependent functions.. There are a few other options, like moving smsc911x_open() rather than the interrupt handler. And i would suggest what ever you do, make it a separate patch. A patch which says: No functional changes, just move functions around as needed by later patches, is going to be quick and easy to review. Yes I put it in another patch, I was busy blasting it out rather than checking my email... + netif_carrier_off(dev); What has this change got to do with interrupt handling? This is a whoops, it should be in the previous patch.. Or a patch of its own? You also needs to be careful with ordering against the phy_connect. @@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device *pdev) if (retval < 0) goto out_disable_resources; - /* configure irq polarity and type before connecting isr */ - if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH) - intcfg |= INT_CFG_IRQ_POL_; - - if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL) - intcfg |= INT_CFG_IRQ_TYPE_; - - smsc911x_reg_write(pdata, INT_CFG, intcfg); I see these removes, but where are the adds? The functionality is duplicated in open, when the IRQ handler is tested. Ah, it is obfusticated by SMC_SET_IRQ_CFG(). If you say it is duplicated, how about a separate patch removing it, with a clear pointer to where the duplicate is. ? Well, I didn't do that part, but I'm confused by your SMC_SET_IRQ_CFG(). AFAIK, that is smc911x not smsc911x. The code in question is in smsc911x_open() following "Set interrupt deassertion to 100uS". It looks a little different but its reprogramming the INT_CFG preceding the interrupt being enabled. Really, if I were feeling brave (because this driver is used in so many strange pieces of hardware) I would rewrite a large portion of the interrupt management in this driver. I started looking at it last month, while looking for the mdio polling issue, because I wanted to get the link state changes directly. While at it I noticed the WOL functionality could use some attention, etc, one problem after another.
Re: [PATCH 1/2] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
On 09/01/2016 11:58 AM, Andrew Lunn wrote:
@@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+ int retval;
- /* if the phy is not yet registered, retry later*/
+ /* find and start the given phy */
if (!dev->phydev) {
- SMSC_WARN(pdata, hw, "phy_dev is NULL");
- return -EAGAIN;
+ if (smsc911x_mii_probe(dev) < 0) {
+ SMSC_WARN(pdata, probe, "Error starting phy");
+ retval = -EAGAIN;
smsc911x_mii_probe() returns an error code. It is better to use that,
than -EAGAIN, which is rather odd to start with.
Thats a good point, I was just maintaining the existing behavior, but
its definitely better to just return the actual error.
+ goto out;
+ }
}
/* Reset the LAN911x */
if (smsc911x_soft_reset(pdata)) {
SMSC_WARN(pdata, hw, "soft reset failed");
- return -EIO;
+ retval = -EIO;
+ goto mii_free_out;
smsc911x_soft_reset() also returns an error code you should use.
This patch also seems to do quite a few different things. Please can
you break it up into multiple patches.
That was the comment from the previous patch, but It confused me
because it was only really moving the MDIO startup, the rest was a side
effect of that and atomic to it (AKA it wasn't clear how to make it
smaller). I read it as Steve misinterpreting the laundry list of fixes
as things being individually fixed, rather than one thing fixing a bunch
of stuff.
This patch does add additional code I overlooked to cleanup the phy if
it fails, I guess in theory that portion could be a prereq patch, I will
break that portion out. I'm still not sure how to partially move the
MDIO startup...
Re: [PATCH 2/2] net/smsc911x: Move interrupt allocation to open/stop
Hi Andrew,
Thanks for taking a look at this!
On 09/01/2016 12:06 PM, Andrew Lunn wrote:
Hi Jeremy
Please don't add forward references. Move the function earlier in the
file.
Ok, but I thought it was a fairly large move due to further dependent
functions..
static inline u32 __smsc911x_reg_read(struct smsc911x_data *pdata, u32 reg)
{
if (pdata->config.flags & SMSC911X_USE_32BIT)
@@ -1514,6 +1516,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+ int irq_flags;
/* find and start the given phy */
if (!dev->phydev) {
@@ -1584,6 +1587,14 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
+ irq_flags = irq_get_trigger_type(dev->irq);
+ if (request_irq(dev->irq, smsc911x_irqhandler,
+ irq_flags | IRQF_SHARED, dev->name, dev)) {
+ SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+ goto mii_free_out;
+ }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1599,7 +1610,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
- goto mii_free_out;
+ goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1645,7 +1656,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
-
+irq_stop_out:
+ free_irq(dev->irq, dev);
mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1672,12 +1684,15 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);
+ free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
phy_disconnect(dev->phydev);
dev->phydev = NULL;
}
+ netif_carrier_off(dev);
What has this change got to do with interrupt handling?
This is a whoops, it should be in the previous patch..
@@ -2479,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
if (retval < 0)
goto out_disable_resources;
- /* configure irq polarity and type before connecting isr */
- if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
- intcfg |= INT_CFG_IRQ_POL_;
-
- if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
- intcfg |= INT_CFG_IRQ_TYPE_;
-
- smsc911x_reg_write(pdata, INT_CFG, intcfg);
I see these removes, but where are the adds?
The functionality is duplicated in open, when the IRQ handler is tested.
[PATCH v3 0/4] net: smsc911x: Move phy and interrupt config
v2-v3: Move error handing into separate patch, replace a couple cases of fixed errors with the errors being returned from the failing functions. Hoist irq handler. The smsc911x driver is doing a number of things in its probe routine that should be delayed until the interface is started. Because of this, the module cannot be unloaded, the phy states are incorrect/stale if the interface isn't running, open's unnecessarily fail causing network configuration problems, and the /proc/irq nodes are incorrectly named. Clean up a number of these problems by moving the mdio and interrupt configuration into the smsc911x_open routine. Jeremy Linton (4): net: smsc911x: Remove multiple exit points from smsc911x_open net: smsc911x: Fix register_netdev, phy startup, driver unload ordering net: smsc911x: Move interrupt handler before open net: smsc911x: Move interrupt allocation to open/stop drivers/net/ethernet/smsc/smsc911x.c | 213 +-- 1 file changed, 104 insertions(+), 109 deletions(-) -- 2.5.5
[PATCH 3/4] net: smsc911x: Move interrupt handler before open
In preparation for the allocating/enabling interrupts
in the ndo_open routine move the irq handler before it.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 122 +--
1 file changed, 61 insertions(+), 61 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index 823ad3f..c2e56f0 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1507,6 +1507,67 @@ static void smsc911x_disable_irq_chip(struct net_device
*dev)
smsc911x_reg_write(pdata, INT_STS, 0x);
}
+static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
+{
+ struct net_device *dev = dev_id;
+ struct smsc911x_data *pdata = netdev_priv(dev);
+ u32 intsts = smsc911x_reg_read(pdata, INT_STS);
+ u32 inten = smsc911x_reg_read(pdata, INT_EN);
+ int serviced = IRQ_NONE;
+ u32 temp;
+
+ if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
+ temp = smsc911x_reg_read(pdata, INT_EN);
+ temp &= (~INT_EN_SW_INT_EN_);
+ smsc911x_reg_write(pdata, INT_EN, temp);
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
+ pdata->software_irq_signal = 1;
+ smp_wmb();
+ serviced = IRQ_HANDLED;
+ }
+
+ if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
+ /* Called when there is a multicast update scheduled and
+* it is now safe to complete the update */
+ SMSC_TRACE(pdata, intr, "RX Stop interrupt");
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
+ if (pdata->multicast_update_pending)
+ smsc911x_rx_multicast_update_workaround(pdata);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (intsts & inten & INT_STS_TDFA_) {
+ temp = smsc911x_reg_read(pdata, FIFO_INT);
+ temp |= FIFO_INT_TX_AVAIL_LEVEL_;
+ smsc911x_reg_write(pdata, FIFO_INT, temp);
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_TDFA_);
+ netif_wake_queue(dev);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (unlikely(intsts & inten & INT_STS_RXE_)) {
+ SMSC_TRACE(pdata, intr, "RX Error interrupt");
+ smsc911x_reg_write(pdata, INT_STS, INT_STS_RXE_);
+ serviced = IRQ_HANDLED;
+ }
+
+ if (likely(intsts & inten & INT_STS_RSFL_)) {
+ if (likely(napi_schedule_prep(&pdata->napi))) {
+ /* Disable Rx interrupts */
+ temp = smsc911x_reg_read(pdata, INT_EN);
+ temp &= (~INT_EN_RSFL_EN_);
+ smsc911x_reg_write(pdata, INT_EN, temp);
+ /* Schedule a NAPI poll */
+ __napi_schedule(&pdata->napi);
+ } else {
+ SMSC_WARN(pdata, rx_err, "napi_schedule_prep failed");
+ }
+ serviced = IRQ_HANDLED;
+ }
+
+ return serviced;
+}
+
static int smsc911x_open(struct net_device *dev)
{
struct smsc911x_data *pdata = netdev_priv(dev);
@@ -1820,67 +1881,6 @@ static void smsc911x_set_multicast_list(struct
net_device *dev)
spin_unlock_irqrestore(&pdata->mac_lock, flags);
}
-static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
-{
- struct net_device *dev = dev_id;
- struct smsc911x_data *pdata = netdev_priv(dev);
- u32 intsts = smsc911x_reg_read(pdata, INT_STS);
- u32 inten = smsc911x_reg_read(pdata, INT_EN);
- int serviced = IRQ_NONE;
- u32 temp;
-
- if (unlikely(intsts & inten & INT_STS_SW_INT_)) {
- temp = smsc911x_reg_read(pdata, INT_EN);
- temp &= (~INT_EN_SW_INT_EN_);
- smsc911x_reg_write(pdata, INT_EN, temp);
- smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_);
- pdata->software_irq_signal = 1;
- smp_wmb();
- serviced = IRQ_HANDLED;
- }
-
- if (unlikely(intsts & inten & INT_STS_RXSTOP_INT_)) {
- /* Called when there is a multicast update scheduled and
-* it is now safe to complete the update */
- SMSC_TRACE(pdata, intr, "RX Stop interrupt");
- smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_);
- if (pdata->multicast_update_pending)
- smsc911x_rx_multicast_update_workaround(pdata);
- serviced = IRQ_HANDLED;
- }
-
- if (intsts & inten & INT_STS_TDFA_) {
- temp = smsc911x_reg_read(pdata, FIFO_INT);
- temp |= FIFO_INT_TX_AVAIL_LEVEL_;
- smsc911x_reg_write(pdata
[PATCH 4/4] net: smsc911x: Move interrupt allocation to open/stop
The /proc/irq/xx information is incorrect for smsc911x because
the request_irq is happening before the register_netdev has the
proper device name. Moving it to the open also fixes the case
of when the device is renamed.
Reported-by: Will Deacon
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 47 ++--
1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index c2e56f0..4f8910b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1575,6 +1575,7 @@ static int smsc911x_open(struct net_device *dev)
unsigned int temp;
unsigned int intcfg;
int retval;
+ int irq_flags;
/* find and start the given phy */
if (!dev->phydev) {
@@ -1645,6 +1646,15 @@ static int smsc911x_open(struct net_device *dev)
pdata->software_irq_signal = 0;
smp_wmb();
+ irq_flags = irq_get_trigger_type(dev->irq);
+ retval = request_irq(dev->irq, smsc911x_irqhandler,
+irq_flags | IRQF_SHARED, dev->name, dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe,
+ "Unable to claim requested irq: %d", dev->irq);
+ goto mii_free_out;
+ }
+
temp = smsc911x_reg_read(pdata, INT_EN);
temp |= INT_EN_SW_INT_EN_;
smsc911x_reg_write(pdata, INT_EN, temp);
@@ -1660,7 +1670,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
- goto mii_free_out;
+ goto irq_stop_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1707,6 +1717,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
+irq_stop_out:
+ free_irq(dev->irq, dev);
mii_free_out:
phy_disconnect(dev->phydev);
dev->phydev = NULL;
@@ -1733,6 +1745,8 @@ static int smsc911x_stop(struct net_device *dev)
dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
smsc911x_tx_update_txcounters(dev);
+ free_irq(dev->irq, dev);
+
/* Bring the PHY down */
if (dev->phydev) {
phy_stop(dev->phydev);
@@ -2308,7 +2322,6 @@ static int smsc911x_drv_remove(struct platform_device
*pdev)
mdiobus_free(pdata->mii_bus);
unregister_netdev(dev);
- free_irq(dev->irq, dev);
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"smsc911x-memory");
if (!res)
@@ -2393,8 +2406,7 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev);
struct resource *res;
- unsigned int intcfg = 0;
- int res_size, irq, irq_flags;
+ int res_size, irq;
int retval;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -2433,7 +2445,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
pdata = netdev_priv(dev);
dev->irq = irq;
- irq_flags = irq_get_trigger_type(irq);
pdata->ioaddr = ioremap_nocache(res->start, res_size);
pdata->dev = dev;
@@ -2480,38 +2491,18 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
if (retval < 0)
goto out_disable_resources;
- /* configure irq polarity and type before connecting isr */
- if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
- intcfg |= INT_CFG_IRQ_POL_;
-
- if (pdata->config.irq_type == SMSC911X_IRQ_TYPE_PUSH_PULL)
- intcfg |= INT_CFG_IRQ_TYPE_;
-
- smsc911x_reg_write(pdata, INT_CFG, intcfg);
-
- /* Ensure interrupts are globally disabled before connecting ISR */
- smsc911x_disable_irq_chip(dev);
-
- retval = request_irq(dev->irq, smsc911x_irqhandler,
-irq_flags | IRQF_SHARED, dev->name, dev);
- if (retval) {
- SMSC_WARN(pdata, probe,
- "Unable to claim requested irq: %d", dev->irq);
- goto out_disable_resources;
- }
-
netif_carrier_off(dev);
retval = smsc911x_mii_init(pdev, dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
- goto out_free_irq;
+ goto out_disable_resources;
}
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering
[PATCH 1/4] net: smsc911x: Remove multiple exit points from smsc911x_open
Rework the error handling in smsc911x open in preparation
for the mdio startup being moved here.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index ca31345..c9b0e05 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1520,17 +1520,20 @@ static int smsc911x_open(struct net_device *dev)
unsigned int timeout;
unsigned int temp;
unsigned int intcfg;
+ int retval;
/* if the phy is not yet registered, retry later*/
if (!dev->phydev) {
SMSC_WARN(pdata, hw, "phy_dev is NULL");
- return -EAGAIN;
+ retval = -EAGAIN;
+ goto out;
}
/* Reset the LAN911x */
- if (smsc911x_soft_reset(pdata)) {
+ retval = smsc911x_soft_reset(pdata);
+ if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
- return -EIO;
+ goto out;
}
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1600,7 +1603,8 @@ static int smsc911x_open(struct net_device *dev)
if (!pdata->software_irq_signal) {
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
- return -ENODEV;
+ retval = -ENODEV;
+ goto out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1646,6 +1650,8 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
+out:
+ return retval;
}
/* Entry point for stopping the interface */
--
2.5.5
[PATCH 2/4] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering
Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.
The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/smsc/smsc911x.c | 48 ++--
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index c9b0e05..823ad3f 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1099,15 +1099,8 @@ static int smsc911x_mii_init(struct platform_device
*pdev,
goto err_out_free_bus_2;
}
- if (smsc911x_mii_probe(dev) < 0) {
- SMSC_WARN(pdata, probe, "Error registering mii bus");
- goto err_out_unregister_bus_3;
- }
-
return 0;
-err_out_unregister_bus_3:
- mdiobus_unregister(pdata->mii_bus);
err_out_free_bus_2:
mdiobus_free(pdata->mii_bus);
err_out_1:
@@ -1522,18 +1515,20 @@ static int smsc911x_open(struct net_device *dev)
unsigned int intcfg;
int retval;
- /* if the phy is not yet registered, retry later*/
+ /* find and start the given phy */
if (!dev->phydev) {
- SMSC_WARN(pdata, hw, "phy_dev is NULL");
- retval = -EAGAIN;
- goto out;
+ retval = smsc911x_mii_probe(dev);
+ if (retval < 0) {
+ SMSC_WARN(pdata, probe, "Error starting phy");
+ goto out;
+ }
}
/* Reset the LAN911x */
retval = smsc911x_soft_reset(pdata);
if (retval) {
SMSC_WARN(pdata, hw, "soft reset failed");
- goto out;
+ goto mii_free_out;
}
smsc911x_reg_write(pdata, HW_CFG, 0x0005);
@@ -1604,7 +1599,7 @@ static int smsc911x_open(struct net_device *dev)
netdev_warn(dev, "ISR failed signaling test (IRQ %d)\n",
dev->irq);
retval = -ENODEV;
- goto out;
+ goto mii_free_out;
}
SMSC_TRACE(pdata, ifup, "IRQ handler passed test using IRQ %d",
dev->irq);
@@ -1650,6 +1645,10 @@ static int smsc911x_open(struct net_device *dev)
netif_start_queue(dev);
return 0;
+
+mii_free_out:
+ phy_disconnect(dev->phydev);
+ dev->phydev = NULL;
out:
return retval;
}
@@ -1674,8 +1673,12 @@ static int smsc911x_stop(struct net_device *dev)
smsc911x_tx_update_txcounters(dev);
/* Bring the PHY down */
- if (dev->phydev)
+ if (dev->phydev) {
phy_stop(dev->phydev);
+ phy_disconnect(dev->phydev);
+ dev->phydev = NULL;
+ }
+ netif_carrier_off(dev);
SMSC_TRACE(pdata, ifdown, "Interface stopped");
return 0;
@@ -2297,11 +2300,10 @@ static int smsc911x_drv_remove(struct platform_device
*pdev)
pdata = netdev_priv(dev);
BUG_ON(!pdata);
BUG_ON(!pdata->ioaddr);
- BUG_ON(!dev->phydev);
+ WARN_ON(dev->phydev);
SMSC_TRACE(pdata, ifdown, "Stopping driver");
- phy_disconnect(dev->phydev);
mdiobus_unregister(pdata->mii_bus);
mdiobus_free(pdata->mii_bus);
@@ -2500,6 +2502,12 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
netif_carrier_off(dev);
+ retval = smsc911x_mii_init(pdev, dev);
+ if (retval) {
+ SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
+ goto out_free_irq;
+ }
+
retval = register_netdev(dev);
if (retval) {
SMSC_WARN(pdata, probe, "Error %i registering device", retval);
@@ -2509,12 +2517,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
"Network interface: \"%s\"", dev->name);
}
- retval = smsc911x_mii_init(pdev, dev);
- if (retval) {
- SMSC_WARN(pdata, probe, "Error %i initialising mii", retval);
- goto out_unregister_netdev_5;
- }
-
spin_lock_irq(&pdata->mac_lock);
/* Check if mac address has been specified when bringing interface up */
@@ -2550,8 +2552,6 @@ static int smsc911x_drv_probe(struct platform_device
*pdev)
return 0;
-out_unregister_netdev_5:
- unregister_netdev(dev);
out_free_irq:
free_irq(dev->irq, dev);
out_disable_resources:
--
2.5.5
Re: [PATCH 2/2 v3] net: smsc911x: request and deassert optional RESET GPIO
Hi, On 09/07/2016 08:53 AM, Linus Walleij wrote: On some systems (such as the Qualcomm APQ8060 Dragonboard) the RESET signal of the SMSC911x is not pulled up by a resistor (or the internal pull-up that will pull it up if the pin is not even connected) but instead connected to a GPIO line, so that the operating system must explicitly deassert RESET before use. Support this in the SMSC911x driver so this ethernet connector can be used on such targets. Notice that we request the line to go logical low (deassert) whilst the line on the actual component is active low. This is managed in the respective hardware description when specifying the GPIO line with e.g. device tree or ACPI. With device tree it looks like this in one case: reset-gpios = <&tlmm 30 GPIO_ACTIVE_LOW>; Which means that logically requesting the RESET line to be deasserted will result in the line being driven high, taking the device out of reset. Cc: Jeremy Linton Signed-off-by: Linus Walleij Thanks for clarifying the GPIO_ACTIVE_LOW state on this pin. I've reviewed this patch, and tested that it doesn't have any affect on a JunoR2 (ACPI or DT) system. I've got some personal reservations about making changes for a single board that might better be handled in firmware. But, I don't think those should be an excuse to keep that hardware from working. Particularly since this is a fairly trivial change. So FWIW, Reviewed-by: Jeremy Linton Thanks,
Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms
Hi Robert, On 10/03/2016 04:05 AM, Robert Jarzmik wrote: Add a workaround for mainstone, idp and stargate2 boards, for u16 writes which must be aligned on 32 bits addresses. Signed-off-by: Robert Jarzmik --- Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++ 1 file changed, 2 insertions(+) I think this might be the wrong doc file. I think you want the smsc-lan91c111.txt file. Thanks, diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt index 3fed3c124411..224965b7453c 100644 --- a/Documentation/devicetree/bindings/net/smsc911x.txt +++ b/Documentation/devicetree/bindings/net/smsc911x.txt @@ -13,6 +13,8 @@ Optional properties: - reg-io-width : Specify the size (in bytes) of the IO accesses that should be performed on the device. Valid value for SMSC LAN is 2 or 4. If it's omitted or invalid, the size would be 2. +- reg-u16-align4 : Boolean, put in place the workaround the force all + u16 writes to be 32 bits aligned - smsc,irq-active-high : Indicates the IRQ polarity is active-high - smsc,irq-push-pull : Indicates the IRQ type is push-pull - smsc,force-internal-phy : Forces SMSC LAN controller to use
Re: [RFC] net: phy: smsc: Disable auto-negotiation on startup
On 10/10/2016 12:41 PM, Kyle Roeschley wrote:
Because the SMSC PHY completes auto-negotiation before the driver is
ready to handle interrupts, the PHY state machine never realizes that we
have a link. Clear the ANENABLE bit on initialization, which lets
genphy_config_aneg do its thing when that code is hit later.
While this patch does fix the problem we see (no link on boot without
re-plugging the cable), it seems like the generic PHY code should be
able to handle auto-negotiation completing before interrupts are
enabled. Submitted as an RFC in the hopes that someone has an idea as to
how that could be done.
Hi,
Which smsc chip/driver? Maybe assuring the device interrupts are
enabled before the phy is started is a solution?
The whole problem sounds similar to what was recently happening in the
smsc911x driver, but AFAIK that driver is basically only polling at this
point so connecting the phy before the interrupts are enabled shouldn't
be a problem.
This fix is copied from commit 99f81afc139c ("phy: micrel: Disable auto
negotiation on startup").
Signed-off-by: Kyle Roeschley
---
drivers/net/phy/smsc.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b62c4aa..8de8011 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -62,6 +62,16 @@ static int smsc_phy_config_init(struct phy_device *phydev)
return rc;
}
+ if (phy_interrupt_is_valid(phydev)) {
+ rc = phy_read(phydev, MII_BMCR);
+ if (rc < 0)
+ return rc;
+
+ rc = phy_write(phydev, MII_BMCR, rc & ~BMCR_ANENABLE);
+ if (rc < 0)
+ return rc;
+ }
+
return smsc_phy_ack_interrupt(phydev);
}
Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend
Hi,
On 10/27/2016 06:23 AM, Ulf Hansson wrote:
The smsc911c driver puts its device into low power state when entering
system suspend. Although it doesn't update the device's runtime PM status
to RPM_SUSPENDED, which causes problems for a parent device.
In particular, when the runtime PM status of the parent is requested to be
updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
forbidden to runtime suspend a device, which has an active child.
Fix this by updating the runtime PM status of the smsc911x device to
RPM_SUSPENDED during system suspend. In system resume, let's reverse that
action by runtime resuming the device and thus also the parent.
Signed-off-by: Ulf Hansson
Tested-by: Geert Uytterhoeven
Cc: Steve Glendinning
Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an active
child")
---
Note that the commit this change fixes is currently queued for 4.10 via
Rafael's linux-pm tree. So this fix should go via that tree as well.
---
drivers/net/ethernet/smsc/smsc911x.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index e9b8579..65fca9c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+
return 0;
}
@@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
struct smsc911x_data *pdata = netdev_priv(ndev);
unsigned int to = 100;
+ pm_runtime_enable(dev);
+ pm_runtime_resume(dev);
+
/* Note 3.11 from the datasheet:
* "When the LAN9220 is in a power saving state, a write of any
* data to the BYTE_TEST register will wake-up the device."
This seems an unusual change/sequence. I thought a successful return
from the suspend callback would set the device state to suspended.
I just checked a few other ethernet drivers suspend/resume sequences and
directly calling the pm_runtime seems a little unusual. Most of the
other drivers are checking to see if the interface is running then doing
a netif_device_detach()/attach() sequence which is missing from this
drivers suspend/resume path. Could that be part of the problem?
Of course my knowledge of the power management system is a little thin
so I could be really off base.
[PATCH] net: sky2: Fix shutdown crash
The sky2 frequently crashes during machine shutdown with:
sky2_get_stats+0x60/0x3d8 [sky2]
dev_get_stats+0x68/0xd8
rtnl_fill_stats+0x54/0x140
rtnl_fill_ifinfo+0x46c/0xc68
rtmsg_ifinfo_build_skb+0x7c/0xf0
rtmsg_ifinfo.part.22+0x3c/0x70
rtmsg_ifinfo+0x50/0x5c
netdev_state_change+0x4c/0x58
linkwatch_do_dev+0x50/0x88
__linkwatch_run_queue+0x104/0x1a4
linkwatch_event+0x30/0x3c
process_one_work+0x140/0x3e0
worker_thread+0x60/0x44c
kthread+0xdc/0xf0
ret_from_fork+0x10/0x50
This is caused by the sky2 being called after it has been shutdown.
A previous thread about this can be found here:
https://lkml.org/lkml/2016/4/12/410
An alternative fix is to assure that IFF_UP gets cleared by
calling dev_close() during shutdown. This is similar to what the
bnx2/tg3/xgene and maybe others are doing to assure that the driver
isn't being called following _shutdown().
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/marvell/sky2.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/net/ethernet/marvell/sky2.c
b/drivers/net/ethernet/marvell/sky2.c
index f05ea56..941c8e2 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -5220,6 +5220,19 @@ static SIMPLE_DEV_PM_OPS(sky2_pm_ops, sky2_suspend,
sky2_resume);
static void sky2_shutdown(struct pci_dev *pdev)
{
+ struct sky2_hw *hw = pci_get_drvdata(pdev);
+ int port;
+
+ for (port = 0; port < hw->ports; port++) {
+ struct net_device *ndev = hw->dev[port];
+
+ rtnl_lock();
+ if (netif_running(ndev)) {
+ dev_close(ndev);
+ netif_device_detach(ndev);
+ }
+ rtnl_unlock();
+ }
sky2_suspend(&pdev->dev);
pci_wake_from_d3(pdev, device_may_wakeup(&pdev->dev));
pci_set_power_state(pdev, PCI_D3hot);
--
2.5.5
xgbe unbalanced enable for IRQ XX in 4.11-rc1
Hi, I have a softiron 3k and under network load (nfs copies, vnc with gnome, etc) it is now throwing these messages as fast as the console will accept them. This is booted DT mode. [ 430.111324] Unbalanced enable for IRQ 33 [ 430.115239] [ cut here ] [ 430.119849] WARNING: CPU: 0 PID: 6 at kernel/irq/manage.c:529 __enable_irq+0x7c/0x88 [ 430.127583] Modules linked in: fuse xt_CHECKSUM ipt_MASQUERADE nf_nat_masquerade_ipv4 tun ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_raw iptable_security ebtable_filter ebtables ip6table_filter ip6_tables vfat fat crc32_ce crct10dif_ce amd_xgbe ghash_ce ptp pps_core spi_pl022 ipmi_si i2c_designware_platform ipmi_devintf i2c_designware_core ccp ipmi_msghandler nfsd auth_rpcgss nfs_acl lockd grace sunrpc xfs libcrc32c [ 430.188350] [ 430.189833] CPU: 0 PID: 6 Comm: ksoftirqd/0 Tainted: GW I 4.11.0-0.rc1.git0.1.fc27.aarch64 #1 [ 430.199391] Hardware name: AMD Overdrive/Supercharger/Default string, BIOS ROD1002C 04/08/2016 [ 430.207994] task: 8003e4057900 task.stack: 8003f459 [ 430.213904] PC is at __enable_irq+0x7c/0x88 [ 430.218078] LR is at __enable_irq+0x7c/0x88 [ 430.52] pc : [] lr : [] pstate: 01c5 [ 430.229639] sp : 8003f4593c50 [ 430.232944] x29: 8003f4593c50 x28: 8003dfeddce0 [ 430.238248] x27: 0002 x26: 0001 [ 430.243551] x25: 0040 x24: 8003f60a [ 430.248855] x23: 8003d5d0a900 x22: 0040 [ 430.254158] x21: 0001 x20: 0021 [ 430.259462] x19: 8003dc507200 x18: [ 430.264765] x17: x16: [ 430.270069] x15: 0010 x14: 89005e7f [ 430.275372] x13: 09005e8d x12: 08e8d000 [ 430.280676] x11: 08e65458 x10: 085adce8 [ 430.285979] x9 : ffd0 x8 : 0005 [ 430.291283] x7 : 636e616c61626e55 x6 : 8003fee74d98 [ 430.296586] x5 : 8003fee74d98 x4 : [ 430.301890] x3 : 8003fee88730 x2 : 8003fee74d98 [ 430.307193] x1 : 8003f60a x0 : 001c [ 430.312496] [ 430.313978] ---[ end trace 5664787410723389 ]--- [ 430.318586] Call trace: [ 430.321023] Exception stack(0x8003f4593a80 to 0x8003f4593bb0) [ 430.327454] 3a80: 8003dc507200 0001 8003f4593c50 08137b14 [ 430.335277] 3aa0: 8003f4593c50 8003f4593c50 8003f4593c10 ffc8 [ 430.343099] 3ac0: 8003f4593b00 08134838 08b7a570 8003f4593bd0 [ 430.350921] 3ae0: 8003f4593c50 8003f4593c50 8003f4593c10 ffc8 [ 430.358743] 3b00: 8003f4593bb0 081f8130 0001 0021 [ 430.366566] 3b20: 001c 8003f60a 8003fee74d98 8003fee88730 [ 430.374388] 3b40: 8003fee74d98 8003fee74d98 636e616c61626e55 [ 430.382210] 3b60: 0005 ffd0 085adce8 08e65458 [ 430.390033] 3b80: 08e8d000 09005e8d 89005e7f 0010 [ 430.397854] 3ba0: [ 430.402723] [] __enable_irq+0x7c/0x88 [ 430.407939] [] enable_irq+0x40/0x78 [ 430.412999] [] xgbe_one_poll+0xc0/0xe8 [amd_xgbe] [ 430.419257] [] net_rx_action+0x150/0x3c8 [ 430.424734] [] __do_softirq+0x138/0x358 [ 430.430123] [] run_ksoftirqd+0x4c/0x78 [ 430.435426] [] smpboot_thread_fn+0x184/0x1b8 [ 430.441249] [] kthread+0x12c/0x130 [ 430.446205] [] ret_from_fork+0x10/0x20 [ 430.451600] Unbalanced enable for IRQ 33 [ 430.455516] [ cut here ]
Re: xgbe unbalanced enable for IRQ XX in 4.11-rc1
Hi, On 03/09/2017 03:39 PM, Tom Lendacky wrote: On 3/9/2017 3:26 PM, Jeremy Linton wrote: Hi, Hi Jeremy, I'll have a look at it. Can you send me your kernel config just in case? Sure, i will send it to you off list to avoid spamming everyone with a 43k gziped file. Thanks, Thanks, Tom I have a softiron 3k and under network load (nfs copies, vnc with gnome, etc) it is now throwing these messages as fast as the console will accept them. This is booted DT mode. [ 430.111324] Unbalanced enable for IRQ 33 [ 430.115239] [ cut here ] [ 430.119849] WARNING: CPU: 0 PID: 6 at kernel/irq/manage.c:529 __enable_irq+0x7c/0x88 [ 430.127583] Modules linked in: fuse xt_CHECKSUM ipt_MASQUERADE nf_nat_masquerade_ipv4 tun ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_raw iptable_security ebtable_filter ebtables ip6table_filter ip6_tables vfat fat crc32_ce crct10dif_ce amd_xgbe ghash_ce ptp pps_core spi_pl022 ipmi_si i2c_designware_platform ipmi_devintf i2c_designware_core ccp ipmi_msghandler nfsd auth_rpcgss nfs_acl lockd grace sunrpc xfs libcrc32c [ 430.188350] [ 430.189833] CPU: 0 PID: 6 Comm: ksoftirqd/0 Tainted: GW I 4.11.0-0.rc1.git0.1.fc27.aarch64 #1 [ 430.199391] Hardware name: AMD Overdrive/Supercharger/Default string, BIOS ROD1002C 04/08/2016 [ 430.207994] task: 8003e4057900 task.stack: 8003f459 [ 430.213904] PC is at __enable_irq+0x7c/0x88 [ 430.218078] LR is at __enable_irq+0x7c/0x88 [ 430.52] pc : [] lr : [] pstate: 01c5 [ 430.229639] sp : 8003f4593c50 [ 430.232944] x29: 8003f4593c50 x28: 8003dfeddce0 [ 430.238248] x27: 0002 x26: 0001 [ 430.243551] x25: 0040 x24: 8003f60a [ 430.248855] x23: 8003d5d0a900 x22: 0040 [ 430.254158] x21: 0001 x20: 0021 [ 430.259462] x19: 8003dc507200 x18: [ 430.264765] x17: x16: [ 430.270069] x15: 0010 x14: 89005e7f [ 430.275372] x13: 09005e8d x12: 08e8d000 [ 430.280676] x11: 08e65458 x10: 085adce8 [ 430.285979] x9 : ffd0 x8 : 0005 [ 430.291283] x7 : 636e616c61626e55 x6 : 8003fee74d98 [ 430.296586] x5 : 8003fee74d98 x4 : [ 430.301890] x3 : 8003fee88730 x2 : 8003fee74d98 [ 430.307193] x1 : 8003f60a x0 : 001c [ 430.312496] [ 430.313978] ---[ end trace 5664787410723389 ]--- [ 430.318586] Call trace: [ 430.321023] Exception stack(0x8003f4593a80 to 0x8003f4593bb0) [ 430.327454] 3a80: 8003dc507200 0001 8003f4593c50 08137b14 [ 430.335277] 3aa0: 8003f4593c50 8003f4593c50 8003f4593c10 ffc8 [ 430.343099] 3ac0: 8003f4593b00 08134838 08b7a570 8003f4593bd0 [ 430.350921] 3ae0: 8003f4593c50 8003f4593c50 8003f4593c10 ffc8 [ 430.358743] 3b00: 8003f4593bb0 081f8130 0001 0021 [ 430.366566] 3b20: 001c 8003f60a 8003fee74d98 8003fee88730 [ 430.374388] 3b40: 8003fee74d98 8003fee74d98 636e616c61626e55 [ 430.382210] 3b60: 0005 ffd0 085adce8 08e65458 [ 430.390033] 3b80: 08e8d000 09005e8d 89005e7f 0010 [ 430.397854] 3ba0: [ 430.402723] [] __enable_irq+0x7c/0x88 [ 430.407939] [] enable_irq+0x40/0x78 [ 430.412999] [] xgbe_one_poll+0xc0/0xe8 [amd_xgbe] [ 430.419257] [] net_rx_action+0x150/0x3c8 [ 430.424734] [] __do_softirq+0x138/0x358 [ 430.430123] [] run_ksoftirqd+0x4c/0x78 [ 430.435426] [] smpboot_thread_fn+0x184/0x1b8 [ 430.441249] [] kthread+0x12c/0x130 [ 430.446205] [] ret_from_fork+0x10/0x20 [ 430.451600] Unbalanced enable for IRQ 33 [ 430.455516] [ cut here ]
Re: [PATCH net] amd-xgbe: Enable IRQs only if napi_complete_done() is true
On 03/09/2017 05:48 PM, Tom Lendacky wrote:
Depending on the hardware, the amd-xgbe driver may use disable_irq_nosync()
and enable_irq() when an interrupt is received to process Rx packets. If
the napi_complete_done() return value isn't checked an unbalanced enable
for the IRQ could result, generating a warning stack trace.
Update the driver to only enable interrupts if napi_complete_done() returns
true.
I've been running this for a few hours now and haven't seen the warning.
So it appears to be fixed. Thanks!
I guess Dave M picked it up already, but I will toss this in anyway.
Tested-by: Jeremy Linton
Reported-by: Jeremy Linton
Signed-off-by: Tom Lendacky
---
drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 248f60d..ffea985 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2272,10 +2272,7 @@ static int xgbe_one_poll(struct napi_struct *napi, int
budget)
processed = xgbe_rx_poll(channel, budget);
/* If we processed everything, we are done */
- if (processed < budget) {
- /* Turn off polling */
- napi_complete_done(napi, processed);
-
+ if ((processed < budget) && napi_complete_done(napi, processed)) {
/* Enable Tx and Rx interrupts */
if (pdata->channel_irq_mode)
xgbe_enable_rx_tx_int(pdata, channel);
@@ -2317,10 +2314,7 @@ static int xgbe_all_poll(struct napi_struct *napi, int
budget)
} while ((processed < budget) && (processed != last_processed));
/* If we processed everything, we are done */
- if (processed < budget) {
- /* Turn off polling */
- napi_complete_done(napi, processed);
-
+ if ((processed < budget) && napi_complete_done(napi, processed)) {
/* Enable Tx and Rx interrupts */
xgbe_enable_rx_tx_ints(pdata);
}
Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio
Hi,
On 6/17/20 12:34 PM, Andrew Lunn wrote:
On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
From: Jeremy Linton
+static const struct acpi_device_id xgmac_acpi_match[] = {
+ { "NXP0006", (kernel_ulong_t)NULL },
Hi Jeremy
What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some
NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP
MDIO bus master? A cluster of Ethernet controllers?
Strictly speaking its a NXP defined (they own the "NXP" prefix per
https://uefi.org/pnp_id_list) id. So, they have tied it to a specific
bit of hardware. In this case it appears to be a shared MDIO master
which isn't directly contained in an Ethernet controller. Its somewhat
similar to a "nxp,x" compatible id, depending on how they are using
it to identify an ACPI device object (_HID()/_CID()).
So AFAIK, this is all valid ACPI usage as long as the ID maps to a
unique device/object.
Is this documented somewhere? In the DT world we have a clear
documentation for all the compatible strings. Is there anything
similar in the ACPI world for these magic numbers?
Sadly not fully. The mentioned PNP and ACPI
(https://uefi.org/acpi_id_list) ids lists are requested and registered
to a given organization. But, once the prefix is owned, it becomes the
responsibility of that organization to assign & manage the ID's with
their prefix. There are various individuals/etc which have collected
lists, though like PCI ids, there aren't any formal publishing requirements.
Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
On 5/8/20 6:42 PM, Andrew Lunn wrote: On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote: Hi, On 5/8/20 3:27 PM, Andrew Lunn wrote: There is a very small number of devices where the vendor messed up, and did not put valid contents in the ID registers. In such cases, we can read the IDs from device tree. These are then used in exactly the same way as if they were read from the device. Is that the case here? Sorry, I don't understand the question? I was asking in general, does this machine report the ID's correctly. Very likely, it does. The embedded single mac:mdio per nic case seems like the normal case, and most of the existing ACPI described devices are setup that way. Somebody in this thread pointed to ACPI patches for the MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet interfaces, and two MDIO bus masters. One of the bus masters can only do C22 and the other can only do C45. It is expected that the busses are shared, not a nice one to one mapping. Thats whats going on with the NXP too AFAIK. But the mcbin is one of the ones with the "compatible" embedded in the DSD properties.. AKA, they buried the entire DT node there. But at the same time, that shifts the c22/45 question to the nic driver, where use of a DSD property before instantiating/probing MDIO isn't really a problem if needed. This in fact does not help you. The MAC driver has no idea what PHY is connected to it. The MAC does not know if it is C22 or C45. It uses Thats all I was trying to say (the mac side capability is implied by the HID/CID that instantiates it). the phylib abstraction which hides all this. Even if you assume 1:1, use phy_find_first(), it will not find a C45 PHY because without knowing there is a C45 PHY, we don't scan for it. And we should expect C45 PHYs to become more popular in the next few years. In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement for passthrough into a generic VM, otherwise someone has to create a virtual mdio, and pass the phy in for the nic/mac. AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy state never leaves the mgmt side of the picture. It monitors the state and then feeds that back into their nic mgmt complex rather than using it directly. That is the other model. Don't use Linux to drive the PHY, use firmware in the MAC. A number of MACs do that, but it has the usual problems of firmware. It limits you on your choice of PHYs, bugs in the firmware cannot be fixed by the community, no sharing of drivers because firmware is generally proprietary, no 'for free features' because somebody else added features to the linux PHY driver etc. But it will make ACPI support simple, this whole discussion goes away, no ACPI needed at all. Open source firmware! :)
[PATCH] net: phy: Fix c45 no phy detected logic
The commit "disregard Clause 22 registers present bit..." clears
the low bit of the devices_in_package data which is being used
in get_phy_c45_ids() to determine if a phy/register is responding
correctly. That check is against 0x1FFF, but since the low
bit is always cleared, the check can never be true. This leads to
detecting c45 phy devices where none exist.
Lets fix this by also clearing the low bit in the mask to 0x1FFE.
This allows us to continue to autoprobe standards compliant devices
without also gaining a large number of bogus ones.
Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in
get_phy_c45_devs_in_pkg")
Cc: Heiner Kallweit
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472..b93d984d35cc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
/* If mostly Fs, there is no device there,
* then let's continue to probe more, as some
* 10G PHYs have zero Devices In package,
@@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
/* no device there, let's get out of here */
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
*phy_id = 0x;
return 0;
} else {
--
2.24.1
Re: [PATCH] net: phy: Fix c45 no phy detected logic
Hi,
On 5/14/20 2:59 PM, Heiner Kallweit wrote:
On 14.05.2020 19:00, Jeremy Linton wrote:
The commit "disregard Clause 22 registers present bit..." clears
the low bit of the devices_in_package data which is being used
in get_phy_c45_ids() to determine if a phy/register is responding
correctly. That check is against 0x1FFF, but since the low
bit is always cleared, the check can never be true. This leads to
detecting c45 phy devices where none exist.
Lets fix this by also clearing the low bit in the mask to 0x1FFE.
This allows us to continue to autoprobe standards compliant devices
without also gaining a large number of bogus ones.
Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in
get_phy_c45_devs_in_pkg")
Cc: Heiner Kallweit
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472..b93d984d35cc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
/* If mostly Fs, there is no device there,
Looks good to me, it would just be good to extend the comment and explain
why we exclude bit 0 here.
Sure, sounds good.
* then let's continue to probe more, as some
* 10G PHYs have zero Devices In package,
@@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
/* no device there, let's get out of here */
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
*phy_id = 0x;
return 0;
} else {
Re: [PATCH] net: phy: Fix c45 no phy detected logic
Hi,
On 5/14/20 12:00 PM, Jeremy Linton wrote:
The commit "disregard Clause 22 registers present bit..." clears
the low bit of the devices_in_package data which is being used
in get_phy_c45_ids() to determine if a phy/register is responding
correctly. That check is against 0x1FFF, but since the low
bit is always cleared, the check can never be true. This leads to
detecting c45 phy devices where none exist.
Lets fix this by also clearing the low bit in the mask to 0x1FFE.
This allows us to continue to autoprobe standards compliant devices
without also gaining a large number of bogus ones.
So, I've been reworking the c45 ID detection logic, with an aim to
hinting to the scanner that it should fallback to c22 for a given phy
address (as well as giving it some additional standardized areas to
probe for phy ids). It turns out that the c22 registers present bit is a
pretty useful signal that this needs to happen. So, I think this patch
really should move the BIT(0) sanitation after the MMD detection loop in
get_phy_c45_ids().
But having dug into this code for a while now, I'm hard pressed to
understand the case that the original 3b5e74e0afe3 commit fixed. The
only thing I can see is that the "bug" i'm fixing here was intentionally
creating bogus phy nodes when the MMDs weren't responding.
Thanks,
Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in
get_phy_c45_devs_in_pkg")
Cc: Heiner Kallweit
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472..b93d984d35cc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
/* If mostly Fs, there is no device there,
* then let's continue to probe more, as some
* 10G PHYs have zero Devices In package,
@@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (phy_reg < 0)
return -EIO;
/* no device there, let's get out of here */
- if ((*devs & 0x1fff) == 0x1fff) {
+ if ((*devs & 0x1ffe) == 0x1ffe) {
*phy_id = 0x;
return 0;
} else {
Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO PHY
Hi,
On 7/24/20 8:39 AM, Andrew Lunn wrote:
Otherwise the MDIO bus and its phy should be a
child of the nic/mac using it, with standardized behaviors/etc left up to
the OSPM when it comes to MDIO bus enumeration/etc.
Hi Jeremy
Could you be a bit more specific here please.
DT allows
macb0: ethernet@fffc4000 {
compatible = "cdns,at32ap7000-macb";
reg = <0xfffc4000 0x4000>;
interrupts = <21>;
phy-mode = "rmii";
local-mac-address = [3a 0e 03 04 05 06];
clock-names = "pclk", "hclk", "tx_clk";
clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
ethernet-phy@1 {
reg = <0x1>;
reset-gpios = <&pioE 6 1>;
};
};
So the PHY is a direct child of the MAC. The MDIO bus is not modelled
at all. Although this is allowed, it is deprecated, because it results > in
problems with advanced systems which have multiple different
children, and the need to differentiate them. So drivers are slowly
I don't think i'm suggesting that, because AFAIK in ACPI you would have
to specify the DEVICE() for mdio, in order to nest a further set of
phy's via _ADR(). I think in general what I was describing would look
more like what you have below. But..
migrating to always modelling the MDIO bus. In that case, the
phy-handle is always used to point to the PHY:
eth0: ethernet@522d {
compatible = "socionext,synquacer-netsec";
reg = <0 0x522d 0x0 0x1>, <0 0x1000 0x0 0x1>;
interrupts = ;
clocks = <&clk_netsec>;
clock-names = "phy_ref_clk";
phy-mode = "rgmii";
max-speed = <1000>;
max-frame-size = <9000>;
phy-handle = <&phy1>;
mdio {
#address-cells = <1>;
#size-cells = <0>;
phy1: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <1>;
};
};
"mdio-handle" is just half of phy-handle.
What you seem to be say is that although we have defined a generic
solution for ACPI which should work in all cases, it is suggested to
not use it? What exactly are you suggesting in its place?
When you put it that way, what i'm saying sounds crazy.
In this case what are are doing isn't as clean as what you have
described above, its more like:
mdio: {
phy1: {}
phy2: {}
}
...
// somewhere else
dmac1: {
phy-handle = <&phy1>;
}
... //somewhere else
eth0: {
//another device talking to the mgmt controller
}
Which is special in a couple ways.
Lets rewind for a moment and say for ARM/ACPI, what we are talking about
are "edge/server class" devices (with RAS statements/etc) where the
expectation is that they will be running virtualized workloads using LTS
distros, or non linux OSes. DT/etc remains an option for networking
devices which are more "embedded", aren't SBSA, etc. So an Arm
based/ACPI machine should be more regular and share more in the way of
system architecture with other SBSA/SBBR/ACPI/etc machines than has been
the case for DT machines.
A concern is then how we punch networking devices into an arbitrary VM
in a standardized way using libvirt/whatever. If the networking device
doesn't look like a simple self contained memory mapped resource with an
IOMMU domain, I think everything becomes more complicated and you have
to start going down the path of special caseing the VM firmware beyond
how its done for self contained PCIe/SRIOV devices. The latter manage to
pull this all off with a PCIe id, and a couple BARs fed into the VM.
So, I would hope an ACPI nic representation is closer to just a minimal
resource list like:
eth0: {
compatible = "cdns,at32ap7000-macb";
reg = <0xfffc4000 0x4000>;
interrupts = <21>;
}
or in ACPI speak:
Device (ETH0)
{
Name (_HID, "CDNSXXX")
Method (_CRS, 0x0, Serialized)
{
Name (RBUF, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0xfffc4000, 0x4000, )
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive)
{
21
}
})
Return (RBUF)
}
}
(Plus methods for pwr mgmt/etc as needed, the iommu info comes from
another table).
Returning to the NXP part. They avoid the entirety of the above
discussion because all this MDIO/PHY mgmt is just feeding the data into
the mgmt controller, and the bits that are punched into the VM are
fairly standalone.
Anyway, I think this set is generally fine, I would like to see this
part working well with ACPI given its what we have available today. For
the future, we also need to continue pushing everyone towards common
hardware
Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO PHY
Hi, On 7/28/20 3:06 AM, Dan Callaghan wrote: Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: Now i could be wrong, but are Ethernet switches something you expect to see on ACPI/SBSA platforms? Or is this a legitimate use of the escape hatch? As an extra data point: right now I am working on an x86 embedded appliance (ACPI not Device Tree) with 3x integrated Marvell switches. I have been watching this patch series with great interest, because right now there is no way for me to configure a complex switch topology in DSA without Device Tree. DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring whether that NIC/MAC is actually hug off PCIe for the moment). It just has a bunch of phy devices strung out on that single MAC/MDIO. So in ACPI land it would still have a relationship similar to the one Andrew pointed out with his DT example where the eth0->mdio->phy are all contained in their physical parent. The phy in that case associated with the parent adapter would be the first direct decedent of the mdio, the switch itself could then be represented with another device, with a further string of device/phys representing the devices. (I dislike drawing acsii art, but if this isn't clear I will, its also worthwhile to look at the dpaa2 docs for how the mac/phys work on this device for contrast.). If so, then its probably possible to represent with a fairly regular looking set of ACPI objects and avoids part of the core discussion here about whether we need a standardized way to pick phy's out of arbitrary parts of the hierarchy using a part of the spec intended for one off problems. Am I missing something? For the device I am working on, we will have units shipping before these questions about how to represent Ethernet switches in ACPI can be resolved. So realistically, we will have to actually configure the switches using software_node structures supplied by an out-of-tree platform driver, or some hackery like that, rather than configuring them through ACPI. An approach I have been toying with is to port all of DSA to use the fwnode_handle abstraction instead of Device Tree nodes, but that is obviously a large task, and frankly I was not sure whether such a patch series would be welcomed.
Re: [net-next PATCH v7 1/6] Documentation: ACPI: DSD: Document MDIO PHY
Hi, On 7/28/20 7:39 PM, Florian Fainelli wrote: On 7/28/2020 3:30 PM, Jeremy Linton wrote: Hi, On 7/28/20 3:06 AM, Dan Callaghan wrote: Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: Now i could be wrong, but are Ethernet switches something you expect to see on ACPI/SBSA platforms? Or is this a legitimate use of the escape hatch? As an extra data point: right now I am working on an x86 embedded appliance (ACPI not Device Tree) with 3x integrated Marvell switches. I have been watching this patch series with great interest, because right now there is no way for me to configure a complex switch topology in DSA without Device Tree. DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring whether that NIC/MAC is actually hug off PCIe for the moment). There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches all supported within the driver framework right now. It just has a bunch of phy devices strung out on that single MAC/MDIO. It has a number of built-in PHYs that typically appear on a MDIO bus, whether that bus is the switch's internal MDIO bus, or another MDIO bus (which could be provided with just two GPIOs) depends on how the switch is connected to its management host. When the switch is interfaced via MDIO the switch also typically has a MDIO interface called the pseudo-PHY which is how you can actually tap into the control interface of the switch, as opposed to reading its internal PHYs from the MDIO bus. So in ACPI land it would still have a relationship similar to the one Andrew pointed out with his DT example where the eth0->mdio->phy are all contained in their physical parent. The phy in that case associated with the parent adapter would be the first direct decedent of the mdio, the switch itself could then be represented with another device, with a further string of device/phys representing the devices. (I dislike drawing acsii art, but if this isn't clear I will, its also worthwhile to look at the dpaa2 docs for how the mac/phys work on this device for contrast.). The eth0->mdio->phy relationship you describe is the simple case that you are well aware of which is say what we have on the Raspberry Pi 4 with GENET and the external Broadcom PHY. For an Ethernet switch connected to an Ethernet MAC, we have 4 different types of objects: - the Ethernet MAC which sits on its specific bus - the Ethernet switch which also sits on its specific bus - the built-in PHYs of the Ethernet switch which sit on whatever bus/interface the switch provides to make them accessible - the specific bus controller that provides access to the Ethernet switch and this is a simplification that does not take into account Physical Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules. Which is why I've stayed away from much of the switch discussion. There are a lot of edge cases to fall into, because for whatever reason networking seems to be unique in all this non-enumerable customization vs many other areas of the system. Storage, being an example i'm more familiar with which has very similar problems yet, somehow has managed to avoid much of this, despite having run on fabrics significantly more complex than basic ethernet (including running on a wide range of hot pluggable GBIC/SFP/QSFP/etc media layers). ACPI's "problem" here is that its strongly influenced by the "Plug and Play" revolution of the 1990's where the industry went from having humans describing hardware using machine readable languages, to hardware which was enumerable using standard protocols. ACPI's device descriptions are there as a crutch for the remaining non plug an play hardware in the system. So at a basic level, if your describing hardware in ACPI rather than abstracting it, that is a problem. Thanks,
[RFC 10/11] net: example acpize xgmac_mdio
Signed-off-by: Jeremy Linton
---
drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c
b/drivers/net/ethernet/freescale/xgmac_mdio.c
index c82c85ef5fb3..96ee3bd89983 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -245,14 +245,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct mii_bus *bus;
- struct resource res;
+ struct resource *res;
struct mdio_fsl_priv *priv;
int ret;
- ret = of_address_to_resource(np, 0, &res);
- if (ret) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
dev_err(&pdev->dev, "could not obtain address\n");
- return ret;
+ return -EINVAL;
}
bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv));
@@ -263,21 +263,21 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
bus->read = xgmac_mdio_read;
bus->write = xgmac_mdio_write;
bus->parent = &pdev->dev;
- snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long
long)res.start);
+ snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long
long)res->start);
/* Set the PHY base address */
priv = bus->priv;
- priv->mdio_base = of_iomap(np, 0);
+ priv->mdio_base = devm_platform_ioremap_resource(pdev, 0);
if (!priv->mdio_base) {
ret = -ENOMEM;
goto err_ioremap;
}
- priv->is_little_endian = of_property_read_bool(pdev->dev.of_node,
- "little-endian");
+ priv->is_little_endian = device_property_read_bool(&pdev->dev,
+ "little-endian");
- priv->has_a011043 = of_property_read_bool(pdev->dev.of_node,
- "fsl,erratum-a011043");
+ priv->has_a011043 = device_property_read_bool(&pdev->dev,
+ "fsl,erratum-a011043");
ret = of_mdiobus_register(bus, np);
if (ret) {
@@ -320,10 +320,17 @@ static const struct of_device_id xgmac_mdio_match[] = {
};
MODULE_DEVICE_TABLE(of, xgmac_mdio_match);
+static const struct acpi_device_id xgmac_acpi_match[] = {
+ { "NXP0006", (kernel_ulong_t)NULL },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, xgmac_acpi_match);
+
static struct platform_driver xgmac_mdio_driver = {
.driver = {
.name = "fsl-fman_xmdio",
.of_match_table = xgmac_mdio_match,
+ .acpi_match_table = xgmac_acpi_match,
},
.probe = xgmac_mdio_probe,
.remove = xgmac_mdio_remove,
--
2.26.2
[RFC 09/11] net: phy: Refuse to consider phy_id=0 a valid phy
This is another one of those questionable commits. In this case a bus tagged C45_FIRST refuses to create phys where the phy id is invalid. In general this is probably a good idea, but might cause problems. Another idea might be to create an additional flag (MDIOBUS_STRICT_ID?) for this case. Or we just ignore it and accept that the probe logic as it stands potentially creates bogus phy devices, to avoid the case where an actual phy exists but isn't responding correctly. Signed-off-by: Jeremy Linton --- drivers/net/phy/phy_device.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index acdada865864..e74f2ef6f12b 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -789,7 +789,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, if (!valid_id && c22_present) return 0; - *phy_id = 0; + if (valid_id || bus->probe_capabilities != MDIOBUS_C45_FIRST) + *phy_id = 0; + return 0; } @@ -853,6 +855,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) if ((phy_id & 0x1fff) == 0x1fff) return ERR_PTR(-ENODEV); + /* Strict scanning should also ignore phy_id = 0 */ + if (phy_id == 0 && bus->probe_capabilities == MDIOBUS_C45_FIRST) + return ERR_PTR(-ENODEV); + return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids); } EXPORT_SYMBOL(get_phy_device); -- 2.26.2
[RFC 00/11] Make C45 autoprobe more robust
It would be nice if we could depend on the c45 scanner to identify standards complaint phys or fail cleanly enough that we can turn around and continue probing the bus for c22 devices. In order to pull this off we should be looking at a larger range of MMD addresses, as well as doing a better job of judging if a phy appears to be responding correctly. Once that is in place we then allow a MDIO bus to report that its c45 capable with a c22 fallback. So this set is really a heavy RFC, and I have my own set of questions about it. First, it seems like its ok to scan reserved parts of the MMD space, given the existing code is scanning MMD 0. Should we do a better job of blocking out the reserved areas? Or was this really what the commit to sanitize the c22 capability was fixing (avoid a probe at location 0). Secondly, are there parts of the system that are depending on "stub" MDIO devices being created?The DT code looks ok, but I think the existing code path left open a number possibilities where devices are created without valid IDs. The commit which cleared the c22 capability registers from the device id list doesn't make much sense to me except to create bugus devices by avoiding breaking out of the devices loop early. There were a couple other cases (all 0 device lists, device lists reporting devices that respond as 0x to the id registers). Do we want to probe some of the additional package id registers? Do we want a more "strict" flag for fully compliant MDIO/PHY combinations or are we ok with extra stub devices? The 3rd to last set is just using the C45_FIRST flag for it. What have I missed? Jeremy Linton (11): net: phy: Don't report success if devices weren't found net: phy: Simplify MMD device list termination net: phy: refactor c45 phy identification sequence net: phy: Handle c22 regs presence better net: phy: Scan the entire MMD device space net: phy: Hoist no phy detected state net: phy: reset invalid phy reads of 0 back to 0x net: phy: Allow mdio buses to auto-probe c45 devices net: phy: Refuse to consider phy_id=0 a valid phy net: example acpize xgmac_mdio net: example xgmac enable extended scanning drivers/net/ethernet/freescale/xgmac_mdio.c | 28 +++-- drivers/net/phy/mdio_bus.c | 9 +- drivers/net/phy/phy_device.c| 112 include/linux/phy.h | 7 +- 4 files changed, 100 insertions(+), 56 deletions(-) -- 2.26.2
[RFC 05/11] net: phy: Scan the entire MMD device space
The spec identifies devices in the top of
the 32-bit device space. Some phys are actually
responding that high. Lets try and capture their
information as well.
Starting at the reserved address 0, lets scan
every single possible MMD address. The spec
seems to indicate that every MMD should respond
with the same devices list. But it seems this
is being interpreted that only implemented MMDs
need respond. Since it doesn't appear to hurt to
scan reserved addresses, and the spec says that
access to unimplemented registers should return 0
(despite this some devices appear to be returning
0x) we are just going to ignore anything
that doesn't look like a valid return.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 8 +++-
include/linux/phy.h | 2 +-
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2d677490ecab..360c3a72c498 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -743,7 +743,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
-* for 802.3 c45 complied PHYs, so don't probe it at first.
+* for 802.3 c45 complied PHYs, We will ask it for a devices list,
+* but later we won't ask for identification from it.
*/
for (i = 0; i < num_ids && *devs == 0; i++) {
ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
@@ -756,10 +757,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
* 10G PHYs have zero Devices In package,
* e.g. Cortina CS4315/CS4340 PHY.
*/
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
- if (phy_reg < 0)
- return -EIO;
- break;
+ *devs = 0;
}
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc..480a6b153227 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -346,7 +346,7 @@ enum phy_state {
*/
struct phy_c45_device_ids {
u32 devices_in_package;
- u32 device_ids[8];
+ u32 device_ids[32];
};
struct macsec_context;
--
2.26.2
[RFC 08/11] net: phy: Allow mdio buses to auto-probe c45 devices
The mdiobus_scan logic is currently hardcoded to only
work with c22 devices. This works fairly well in most
cases, but its possible a c45 device doesn't respond
despite being a standard phy. If the parent hardware
is capable, it makes sense to scan for c45 devices before
falling back to c22.
As we want this to reflect the capabilities of the STA,
lets add a field to the mii_bus structure to represent
the capability. That way devices can opt into the extended
scanning. Existing users should continue to default to c22
only scanning as long as they are zero'ing the structure
before use.
At the moment its a yes/no option, but in the future
it may be useful to extend this to c45 only policy, or
add additional classes and policies.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/mdio_bus.c | 9 +++--
include/linux/phy.h| 5 +
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 7a4eb3f2cb74..3ab618e15f2c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -732,10 +732,15 @@ EXPORT_SYMBOL(mdiobus_free);
*/
struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
{
- struct phy_device *phydev;
+ struct phy_device *phydev = ERR_PTR(-ENODEV);
int err;
- phydev = get_phy_device(bus, addr, false);
+ if (bus->probe_capabilities == MDIOBUS_C45_FIRST)
+ phydev = get_phy_device(bus, addr, true);
+
+ if (IS_ERR(phydev))
+ phydev = get_phy_device(bus, addr, false);
+
if (IS_ERR(phydev))
return phydev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 480a6b153227..d68e1ebab484 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -275,6 +275,11 @@ struct mii_bus {
int reset_delay_us;
/* RESET GPIO descriptor pointer */
struct gpio_desc *reset_gpiod;
+ /* bus capabilities, used for probing */
+ enum {
+ MDIOBUS_C22_ONLY = 0,
+ MDIOBUS_C45_FIRST,
+ } probe_capabilities;
};
#define to_mii_bus(d) container_of(d, struct mii_bus, dev)
--
2.26.2
[RFC 11/11] net: example xgmac enable extended scanning
Since we know the xgmac hardware always has a c45 complaint bus, lets try scanning for c45 capable phys first. If we fail to find any, then it with fall back to c22 automatically. Signed-off-by: Jeremy Linton --- drivers/net/ethernet/freescale/xgmac_mdio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index 96ee3bd89983..1d7313031be6 100644 --- a/drivers/net/ethernet/freescale/xgmac_mdio.c +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c @@ -263,6 +263,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev) bus->read = xgmac_mdio_read; bus->write = xgmac_mdio_write; bus->parent = &pdev->dev; + bus->probe_capabilities = MDIOBUS_C45_FIRST; snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start); /* Set the PHY base address */ -- 2.26.2
[RFC 01/11] net: phy: Don't report success if devices weren't found
C45 devices are to return 0 for registers they haven't
implemented. This means in theory we can terminate the
device search loop without finding any MMDs. In that
case we want to immediately return indicating that
nothing was found rather than continuing to probe
and falling into the success state at the bottom.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472..245899b58a7d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -742,6 +742,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
}
}
+ /* no reported devices */
+ if (*devs == 0) {
+ *phy_id = 0x;
+ return 0;
+ }
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
--
2.26.2
[RFC 06/11] net: phy: Hoist no phy detected state
Default initializing the phy_id to "invalid" allows
us to avoid setting it on the error returns.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 360c3a72c498..b2cd22d6315c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -742,6 +742,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
bool c22_present = false;
bool valid_id = false;
+ *phy_id = 0x;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, We will ask it for a devices list,
* but later we won't ask for identification from it.
@@ -762,10 +763,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
}
/* no reported devices */
- if (!valid_phy_id(*devs)) {
- *phy_id = 0x;
+ if (!valid_phy_id(*devs))
return 0;
- }
/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
c22_present = *devs & BIT(0);
@@ -783,10 +782,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
valid_id = true;
}
- if (!valid_id && c22_present) {
- *phy_id = 0x;
+ if (!valid_id && c22_present)
return 0;
- }
+
*phy_id = 0;
return 0;
}
--
2.26.2
[RFC 04/11] net: phy: Handle c22 regs presence better
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
+ }
+
+ if (!valid_id && c22_present) {
+ *phy_id = 0x;
+ return 0;
}
*phy_id = 0;
return 0;
--
2.26.2
[RFC 02/11] net: phy: Simplify MMD device list termination
Since we are already checking for *devs == 0 after
the loop terminates, we can add a mostly F's check
as well. With that change we can simplify the return/break
sequence inside the loop.
Add a valid_phy_id() macro for this, since we will be using it
in a couple other places.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 245899b58a7d..7746c07b97fe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,6 +695,11 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus,
int addr, int dev_addr,
return 0;
}
+static bool valid_phy_id(int val)
+{
+ return (val > 0 && ((val & 0x1fff) != 0x1fff));
+}
+
/**
* get_phy_c45_ids - reads the specified addr for its 802.3-c45 IDs.
* @bus: the target MII bus
@@ -732,18 +737,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
if (phy_reg < 0)
return -EIO;
- /* no device there, let's get out of here */
- if ((*devs & 0x1fff) == 0x1fff) {
- *phy_id = 0x;
- return 0;
- } else {
- break;
- }
+ break;
}
}
/* no reported devices */
- if (*devs == 0) {
+ if (!valid_phy_id(*devs)) {
*phy_id = 0x;
return 0;
}
--
2.26.2
[RFC 07/11] net: phy: reset invalid phy reads of 0 back to 0xffffffff
MMD's in the device list sometimes return 0 for their id. When that happens lets reset the id back to 0xfff so that we don't get a stub device created for it. This is a questionable commit, but i'm tossing it out there along with the comment that reading the spec seems to indicate that maybe there are further registers that could be probed in an attempt to resolve some futher "bad" phys. It sort of comes down to do we want unused phy devices floating around (potentially unmatched in dt) or do we want to cut them off early and let DT create them directly. For the ACPI case, we don't really have a good way to match them, and since it hasn't been working I think its perfectly reasonable at this point to expect phy's to implement enough of the standard that we can detect them and attach a phy specific driver. Signed-off-by: Jeremy Linton --- drivers/net/phy/phy_device.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index b2cd22d6315c..acdada865864 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -780,6 +780,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, return ret; if (valid_phy_id(c45_ids->device_ids[i])) valid_id = true; + else + c45_ids->device_ids[i] = 0x; + + /* consider probing PKGID per 45.2.12.2.1? */ } if (!valid_id && c22_present) -- 2.26.2
[RFC 03/11] net: phy: refactor c45 phy identification sequence
Lets factor out the phy id logic, and make it generic
so that it can be used for c22 and c45.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 65 +++-
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7746c07b97fe..f0761fa5e40b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus,
int addr, int dev_addr,
return 0;
}
+static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr,
+ u32 *phy_id, bool c45)
+{
+ int phy_reg, reg_addr;
+
+ int reg_base = c45 ? (MII_ADDR_C45 | dev_addr << 16) : 0;
+
+ reg_addr = reg_base | MII_PHYSID1;
+ phy_reg = mdiobus_read(bus, addr, reg_addr);
+ if (phy_reg < 0)
+ return -EIO;
+
+ *phy_id = phy_reg << 16;
+
+ reg_addr = reg_base | MII_PHYSID2;
+ phy_reg = mdiobus_read(bus, addr, reg_addr);
+ if (phy_reg < 0)
+ return -EIO;
+ *phy_id |= phy_reg;
+
+ return 0;
+}
+
static bool valid_phy_id(int val)
{
return (val > 0 && ((val & 0x1fff) != 0x1fff));
@@ -715,17 +738,17 @@ static bool valid_phy_id(int val)
*/
static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
struct phy_c45_device_ids *c45_ids) {
- int phy_reg;
- int i, reg_addr;
+ int ret;
+ int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
*/
- for (i = 1; i < num_ids && *devs == 0; i++) {
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
- if (phy_reg < 0)
+ for (i = 0; i < num_ids && *devs == 0; i++) {
+ ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
+ if (ret < 0)
return -EIO;
if ((*devs & 0x1fff) == 0x1fff) {
@@ -752,17 +775,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (!(c45_ids->devices_in_package & (1 << i)))
continue;
- reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1;
- phy_reg = mdiobus_read(bus, addr, reg_addr);
- if (phy_reg < 0)
- return -EIO;
- c45_ids->device_ids[i] = phy_reg << 16;
-
- reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;
- phy_reg = mdiobus_read(bus, addr, reg_addr);
- if (phy_reg < 0)
- return -EIO;
- c45_ids->device_ids[i] |= phy_reg;
+ ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
+ if (ret < 0)
+ return ret;
}
*phy_id = 0;
return 0;
@@ -787,27 +802,17 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
bool is_c45, struct phy_c45_device_ids *c45_ids)
{
- int phy_reg;
+ int ret;
if (is_c45)
return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
- /* Grab the bits from PHYIR1, and put them in the upper half */
- phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
- if (phy_reg < 0) {
+ ret = _get_phy_id(bus, addr, 0, phy_id, false);
+ if (ret < 0) {
/* returning -ENODEV doesn't stop bus scanning */
- return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
+ return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO;
}
- *phy_id = phy_reg << 16;
-
- /* Grab the bits from PHYIR2, and put them in the lower half */
- phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
- if (phy_reg < 0)
- return -EIO;
-
- *phy_id |= phy_reg;
-
return 0;
}
--
2.26.2
Re: [RFC 03/11] net: phy: refactor c45 phy identification sequence
Hi,
Thanks for taking a look at this!
On 5/23/20 10:28 AM, Andrew Lunn wrote:
On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote:
Lets factor out the phy id logic, and make it generic
so that it can be used for c22 and c45.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 65 +++-
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7746c07b97fe..f0761fa5e40b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus,
int addr, int dev_addr,
return 0;
}
+static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr,
+ u32 *phy_id, bool c45)
Hi Jeremy
How about read_phy_id() so you can avoid the _ prefix.
Yes, that sounds good.
static bool valid_phy_id(int val)
{
return (val > 0 && ((val & 0x1fff) != 0x1fff));
@@ -715,17 +738,17 @@ static bool valid_phy_id(int val)
*/
static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
struct phy_c45_device_ids *c45_ids) {
- int phy_reg;
- int i, reg_addr;
+ int ret;
+ int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
*/
- for (i = 1; i < num_ids && *devs == 0; i++) {
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
- if (phy_reg < 0)
+ for (i = 0; i < num_ids && *devs == 0; i++) {
+ ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
+ if (ret < 0)
return -EIO;
Renaming reg_addr to ret does not belong in this patch.
Sure, that makes sense. The rename was a last min change when I was
shuffling the args around.
Thanks,
Re: [RFC 03/11] net: phy: refactor c45 phy identification sequence
Hi,
On 5/23/20 10:28 AM, Andrew Lunn wrote:
On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote:
Lets factor out the phy id logic, and make it generic
so that it can be used for c22 and c45.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 65 +++-
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7746c07b97fe..f0761fa5e40b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus,
int addr, int dev_addr,
return 0;
}
+static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr,
+ u32 *phy_id, bool c45)
Hi Jeremy
How about read_phy_id() so you can avoid the _ prefix.
static bool valid_phy_id(int val)
{
return (val > 0 && ((val & 0x1fff) != 0x1fff));
@@ -715,17 +738,17 @@ static bool valid_phy_id(int val)
*/
static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
struct phy_c45_device_ids *c45_ids) {
- int phy_reg;
- int i, reg_addr;
+ int ret;
+ int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
*/
- for (i = 1; i < num_ids && *devs == 0; i++) {
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
- if (phy_reg < 0)
+ for (i = 0; i < num_ids && *devs == 0; i++) {
+ ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
+ if (ret < 0)
return -EIO;
Renaming reg_addr to ret does not belong in this patch.
Looks like I changed the loop index in this patch while shuffling things
around yesterday too. The "for (i = 1/0.." change belongs in 5/11 as well.
Re: [RFC 03/11] net: phy: refactor c45 phy identification sequence
Hi,
On 5/23/20 3:01 PM, Russell King - ARM Linux admin wrote:
On Sat, May 23, 2020 at 09:51:31PM +0200, Andrew Lunn wrote:
static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
struct phy_c45_device_ids *c45_ids) {
- int phy_reg;
- int i, reg_addr;
+ int ret;
+ int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
I feel a "reverse christmas tree" complaint brewing... yes, the original
code didn't follow it. Maybe a tidy up while touching this code?
At minimum, a patch should not make it worse. ret and i should clearly
be after devs.
static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
bool is_c45, struct phy_c45_device_ids *c45_ids)
{
- int phy_reg;
+ int ret;
if (is_c45)
return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
- /* Grab the bits from PHYIR1, and put them in the upper half */
- phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
- if (phy_reg < 0) {
+ ret = _get_phy_id(bus, addr, 0, phy_id, false);
+ if (ret < 0) {
/* returning -ENODEV doesn't stop bus scanning */
- return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
+ return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO;
Since ret will only ever be -EIO here, this can only ever return
-ENODEV, which is a functional change in the code (probably unintended.)
Nevertheless, it's likely introducing a bug if the intention is for
some other return from mdiobus_read() to be handled differently.
}
- *phy_id = phy_reg << 16;
-
- /* Grab the bits from PHYIR2, and put them in the lower half */
- phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
- if (phy_reg < 0)
- return -EIO;
... whereas this one always returns -EIO on any error.
So, I think you have the potential in this patch to introduce a subtle
change of behaviour, which may lead to problems - have you closely
analysed why the code was the way it was, and whether your change of
behaviour is actually valid?
I could be remembering this wrongly, but i think this is to do with
orion_mdio_xsmi_read() returning -ENODEV, not 0xff, if there
is no device on the bus at the given address. -EIO is fatal to the
scan, everything stops with the assumption the bus is broken. -ENODEV
should not be fatal to the scan.
Maybe orion_mdio_xsmi_read() should be fixed then? Also, maybe
adding return code documentation for mdiobus_read() / mdiobus_write()
would help MDIO driver authors have some consistency in what
errors they are expected to return (does anyone know for certain?)
My understanding at this point (which is mostly based on the xgmac
here), is that 0x is equivalent to "bus responding correctly,
phy failed to respond at this register location" while any -Eerror is
understood as "something wrong with bus", and the mdio core then makes a
choice about terminating just the current phy search (ENODEV), or
terminating the entire mdio bus (basically everything else) registration.
I will see about clarifying the docs. I need to see if that will end up
being a bit of a rabbit hole before committing to including that in this
set.
Which brings up the problem that at least xgmac_mdio doesn't appear to
handle being told "your bus registration failed" without OOPSing the
probe routine. I think Calvin is aware of this, and I believe he has
some additional xgmac/etc patches on top of this set. Although he pinged
me offline the other day to say that apparently all my hunk shuffling
broke some of the c45 phy detection I had working earlier in the week.
Re: [RFC 01/11] net: phy: Don't report success if devices weren't found
Hi, Thanks for taking a look at this. On 5/23/20 1:20 PM, Russell King - ARM Linux admin wrote: On Fri, May 22, 2020 at 04:30:49PM -0500, Jeremy Linton wrote: C45 devices are to return 0 for registers they haven't implemented. This means in theory we can terminate the device search loop without finding any MMDs. In that case we want to immediately return indicating that nothing was found rather than continuing to probe and falling into the success state at the bottom. This is a little confusing when you read this comment: /* If mostly Fs, there is no device there, * then let's continue to probe more, as some * 10G PHYs have zero Devices In package, * e.g. Cortina CS4315/CS4340 PHY. */ Since it appears to be talking about the case of a PHY where *devs will be zero. However, tracking down the original submission, it seems this is not the case at all, and the comment is grossly misleading. It seems these PHYs only report a valid data in the Devices In Package registers for devad=0 - it has nothing to do with a zero Devices In Package. Yes, this ended up being my understanding of this commit, and is part of my justification for starting the devices search at the reserved address 0 rather than 1. Can I suggest that this comment is fixed while we're changing the code to explicitly reject this "zero Devices In package" so that it's not confusing? Its probably better to kill it in 5/11 with a mention that we are starting at a reserved address? OTOH, I'm a bit concerned that reading at 0 as the first address will cause problems because the original code was only triggering it after a read returning 0x at a valid MMD address. It does simplify/clarify the loop though. If it weren't for this 0 read, I would have tried to avoid some of the additional MMD reserved addresses.
Re: [RFC 02/11] net: phy: Simplify MMD device list termination
Hi,
On 5/23/20 1:36 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:50PM -0500, Jeremy Linton wrote:
Since we are already checking for *devs == 0 after
the loop terminates, we can add a mostly F's check
as well. With that change we can simplify the return/break
sequence inside the loop.
Add a valid_phy_id() macro for this, since we will be using it
in a couple other places.
I'm not sure you have the name of this correct, and your usage layer
in your patch series is correct.
Or the name is poor..
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 245899b58a7d..7746c07b97fe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,6 +695,11 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus,
int addr, int dev_addr,
return 0;
}
+static bool valid_phy_id(int val)
+{
+ return (val > 0 && ((val & 0x1fff) != 0x1fff));
+}
+
/**
* get_phy_c45_ids - reads the specified addr for its 802.3-c45 IDs.
* @bus: the target MII bus
@@ -732,18 +737,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
if (phy_reg < 0)
return -EIO;
- /* no device there, let's get out of here */
- if ((*devs & 0x1fff) == 0x1fff) {
- *phy_id = 0x;
- return 0;
- } else {
- break;
- }
+ break;
}
}
/* no reported devices */
- if (*devs == 0) {
+ if (!valid_phy_id(*devs)) {
You are using this to validate the "devices in package" value, not the
PHY ID value. So, IMHO this should be called "valid_devs_in_package()"
or similar.
Hmmm, its more "valid_phy_reg()" since it ends up being used to validate
both the devs in package as well as phy id.
*phy_id = 0x;
return 0;
}
--
2.26.2
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x
returns that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases
seems to overlap sufficiently to cause this to throw them out. Plus a
phy would also have to have model+revision set to 'F's. So while might
be possible, if unlikely, at the moment I think the OUI registration
keeps this from being a problem. Particularly, if i'm reading the
mapping correctly, the OUI mapping guarantees that the field cannot be
all '1's due to the OUI having X & M bits cleared. It sort of looks like
the mapping is trying to lose those bits, by tossing bit 1 & 2, but the
X & M are in the wrong octet (AFAIK, I just read it three times cause it
didn't make any sense).
Re: [RFC 07/11] net: phy: reset invalid phy reads of 0 back to 0xffffffff
Hi, On 5/23/20 1:44 PM, Russell King - ARM Linux admin wrote: On Fri, May 22, 2020 at 04:30:55PM -0500, Jeremy Linton wrote: MMD's in the device list sometimes return 0 for their id. When that happens lets reset the id back to 0xfff so that we don't get a stub device created for it. This is a questionable commit, but i'm tossing it out there along with the comment that reading the spec seems to indicate that maybe there are further registers that could be probed in an attempt to resolve some futher "bad" phys. It sort of comes down to do we want unused phy devices floating around (potentially unmatched in dt) or do we want to cut them off early and let DT create them directly. I'm not sure what you mean "stub device" or "unused phy devices floating around" - the individual MMDs are not treated as separate "phy devices", but as one PHY device as a whole. Well, I guess its clearer to say phy/mmd devices with a phy_id=0. Which is a problem if we don't have DT overriding the phy_id for a given address. Although AFAIK given a couple of the /sys/bus/mdio_bus/devices lists I've seen, and after studying this code for a while now, I think "bogus" phy's might be getting created*. I was far to easy, to upset the cart when I was hacking on this set, and end up with a directory chuck full of phys. So this gets close to one of the questions I asked in the cover letter. This patch and 09/11 serve to cut off possibly valid phy's which are failing to identify themselves using the standard registers. Which per the 802.3 spec there is a blurb about 0 in the id registers for some cases. Its not really a critical problem for ACPI machines to have these phys around (OTOH, there might be issues with c22 phys on c45 electrical buses that respond to c45 reg requests but don't set the c22 regs flag, I haven't seen that yet.). I considered dropping this patch, and 9/11 was a last minute addition. I kept it because I was worried all those extra "reserved" MMDs would end up with id = 0's in there and break something. * In places where there isn't actually a phy, likely a large part of the problem was clearing the c22 bit, which allowed 0x returns to slip through the devices list.
Re: [RFC 08/11] net: phy: Allow mdio buses to auto-probe c45 devices
Hi,
On 5/24/20 9:44 AM, Andrew Lunn wrote:
+++ b/include/linux/phy.h
@@ -275,6 +275,11 @@ struct mii_bus {
int reset_delay_us;
/* RESET GPIO descriptor pointer */
struct gpio_desc *reset_gpiod;
+ /* bus capabilities, used for probing */
+ enum {
+ MDIOBUS_C22_ONLY = 0,
+ MDIOBUS_C45_FIRST,
+ } probe_capabilities;
};
I'm not so keen on _FIRST. It suggest _LAST would also be valid. But
that then suggests this is not a bus property, but a PHY property, and
some PHYs might need _FIRST and other phys need _LAST, and then you
have a bus which has both sorts of PHY on it, and you have a problem.
So i think it would be better to have
enum {
MDIOBUS_UNKNOWN = 0,
MDIOBUS_C22,
MDIOBUS_C45,
MDIOBUS_C45_C22,
} bus_capabilities;
Describe just what the bus master can support.
Yes, the naming is reasonable and I will update it in the next patch. I
went around a bit myself with this naming early on, and the problem I
saw was that a C45 capable master, can have C45 electrical phy's that
only respond to c22 requests (AFAIK). So the MDIOBUS_C45 (I think I was
calling it C45_ONLY) is an invalid selection. Not, that it wouldn't be
helpful to have a C45_ONLY case, but that the assumption is that you
wouldn't try and probe c22 registers, which I thought was a mistake.
Thanks,
Re: [RFC 01/11] net: phy: Don't report success if devices weren't found
Hi, On 5/25/20 4:45 AM, Russell King - ARM Linux admin wrote: On Sun, May 24, 2020 at 09:46:55PM -0500, Jeremy Linton wrote: Hi, Thanks for taking a look at this. On 5/23/20 1:20 PM, Russell King - ARM Linux admin wrote: On Fri, May 22, 2020 at 04:30:49PM -0500, Jeremy Linton wrote: C45 devices are to return 0 for registers they haven't implemented. This means in theory we can terminate the device search loop without finding any MMDs. In that case we want to immediately return indicating that nothing was found rather than continuing to probe and falling into the success state at the bottom. This is a little confusing when you read this comment: /* If mostly Fs, there is no device there, * then let's continue to probe more, as some * 10G PHYs have zero Devices In package, * e.g. Cortina CS4315/CS4340 PHY. */ Since it appears to be talking about the case of a PHY where *devs will be zero. However, tracking down the original submission, it seems this is not the case at all, and the comment is grossly misleading. It seems these PHYs only report a valid data in the Devices In Package registers for devad=0 - it has nothing to do with a zero Devices In Package. Yes, this ended up being my understanding of this commit, and is part of my justification for starting the devices search at the reserved address 0 rather than 1. Can I suggest that this comment is fixed while we're changing the code to explicitly reject this "zero Devices In package" so that it's not confusing? Its probably better to kill it in 5/11 with a mention that we are starting at a reserved address? OTOH, I'm a bit concerned that reading at 0 as the first address will cause problems because the original code was only triggering it after a read returning 0x at a valid MMD address. It does simplify/clarify the loop though. If it weren't for this 0 read, I would have tried to avoid some of the additional MMD reserved addresses. Yes, that is the worry, and as MMD 0 is marked as reserved, I don't think we should routinely probe it. Hmm, ok, so it gets a bit more complex then. The loop could probe all the valid MMD addresses, then if that doesn't appear to have yielded anything try the reserved ones? Its actually not a big code change because we could have a hardcoded bitfield of valid MMD addresses which we check before trying the probe. Then make one pass through the loop hitting the valid ones, and if we still didn't find anything, try the reserved addresses. As I've already mentioned, note that bit 0 of devices-in-package does not mean that there is a MMD 0 implemented, even if bit 0 is set. Bit 0 means that the clause 22 register set is available through clause 22 cycles. So, simplifying the loop to start at 0 and removing the work- around could end up breaking Cortina PHYs if they don't set bit 0 in their devices in package - but I don't see why we should depend on bit 0 being set for their workaround. Just to be clear this set probes MMD 0 except to ask for the device list. That is the same behavior as before. That is because all the subsequent id/etc loops are indexed to start at MMD 1. The primary difference for the cortiana PHY's AFAIK, is the order we ask for the devices list. Before it had to fail at a valid addr before reading 0, now it just starts at 0 and continues to probe if that fails. Some of this is required (continuing scan on failure) due to phys that "fail" reading the devices list for the lower MMD's addresses but work on the higher ones. So, I think you're going to have to add a work-around to ignore bit 0, which brings up the question whether this is worth it or not. It does ignore bit 0, it gets turned into the C22 regs flag, and cleared/ignored in the remainder of the code (do to MMD loop indexes starting at 1). Hence, I think this is a "simplifcation" too far. Ok, if i'm understanding correctly, avoid the reserved addresses unless we fail to get a device list as before. That isn't a problem, I will include that in the next revision. Thanks,
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi,
On 5/25/20 5:06 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 10:34:13PM -0500, Jeremy Linton wrote:
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x returns
that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases seems to
overlap sufficiently to cause this to throw them out. Plus a phy would also
have to have model+revision set to 'F's. So while might be possible, if
unlikely, at the moment I think the OUI registration keeps this from being a
problem. Particularly, if i'm reading the mapping correctly, the OUI mapping
guarantees that the field cannot be all '1's due to the OUI having X & M
bits cleared. It sort of looks like the mapping is trying to lose those
bits, by tossing bit 1 & 2, but the X & M are in the wrong octet (AFAIK, I
just read it three times cause it didn't make any sense).
I should also note that we have at least one supported PHY where one
of the MMDs returns 0xfffe for even numbered registers and 0x for
odd numbered registers in one of the vendor MMDs for addresses 0
through 0xefff - which has a bit set in the devices-in-package.
It also returns 0x0082 for almost every register in MMD 2, but MMD 2's
devices-in-package bit is clear in most of the valid MMDs, so we
shouldn't touch it.
These reveal the problem of randomly probing MMDs - they can return
unexpected values and not be as well behaved as we would like them to
be. Using register 8 to detect presence may be beneficial, but that
may also introduce problems as we haven't used that before (and we
don't know whether any PHY that wrong.) I know at least the 88x3310
gets it right for all except the vendor MMDs, where the low addresses
appear non-confromant to the 802.3 specs. Both vendor MMDs are
definitely implemented, just not with anything conforming to 802.3.
Yes, we know even for the NXP reference hardware, one of the phy's
doesn't probe out correctly because it doesn't respond to the ieee
defined registers
Re: [RFC 01/11] net: phy: Don't report success if devices weren't found
Hi, On 5/25/20 4:07 PM, Russell King - ARM Linux admin wrote: On Mon, May 25, 2020 at 04:02:13PM -0500, Jeremy Linton wrote: So, I think you're going to have to add a work-around to ignore bit 0, which brings up the question whether this is worth it or not. It does ignore bit 0, it gets turned into the C22 regs flag, and cleared/ignored in the remainder of the code (do to MMD loop indexes starting at 1). However, I've already pointed out that that isn't the case in a number of functions that I listed in another email, and I suspect was glossed over. Hmm, right, I might not be understanding, I'm still considering your comments in 4/11 and a couple others.. OTOH, the mmd 0 logic could be completely removed, as its actually been broken for a year or so in linux (AFAIK) because the code triggering it was disabled when the C22 sanitation patch was merged. OTOH, this patch is still clearing the C22 flag from devices, so anything dependent entirely on that should have the same behavior as before. So, there is a bug in the is_valid_phy/device macro, because I messed it up when I converted it to a function because its using a signed val, when it should be unsigned. I don't think that is what you were hinting in 4/11 though. Thanks,
Re: [RFC 08/11] net: phy: Allow mdio buses to auto-probe c45 devices
Hi,
On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote:
Hi,
On 5/24/20 9:44 AM, Andrew Lunn wrote:
+++ b/include/linux/phy.h
@@ -275,6 +275,11 @@ struct mii_bus {
int reset_delay_us;
/* RESET GPIO descriptor pointer */
struct gpio_desc *reset_gpiod;
+ /* bus capabilities, used for probing */
+ enum {
+ MDIOBUS_C22_ONLY = 0,
+ MDIOBUS_C45_FIRST,
+ } probe_capabilities;
};
I'm not so keen on _FIRST. It suggest _LAST would also be valid. But
that then suggests this is not a bus property, but a PHY property, and
some PHYs might need _FIRST and other phys need _LAST, and then you
have a bus which has both sorts of PHY on it, and you have a problem.
So i think it would be better to have
enum {
MDIOBUS_UNKNOWN = 0,
MDIOBUS_C22,
MDIOBUS_C45,
MDIOBUS_C45_C22,
} bus_capabilities;
Describe just what the bus master can support.
Yes, the naming is reasonable and I will update it in the next patch. I went
around a bit myself with this naming early on, and the problem I saw was
that a C45 capable master, can have C45 electrical phy's that only respond
to c22 requests (AFAIK).
If you have a master that can only generate clause 45 cycles, and
someone is daft enough to connect a clause 22 only PHY to it, the
result is hardware that doesn't work - there's no getting around
that. The MDIO interface can't generate the appropriate cycles to
access the clause 22 PHY. So, this is not something we need care
about.
So the MDIOBUS_C45 (I think I was calling it
C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
a C45_ONLY case, but that the assumption is that you wouldn't try and probe
c22 registers, which I thought was a mistake.
MDIOBUS_C45 means "I can generate clause 45 cycles".
MDIOBUS_C22 means "I can generate clause 22 cycles".
MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
cycles."
Hi, to be clear, we are talking about c45 controllers that can access
the c22 register space via c45, or controllers which are
electrically/level shifting to be compatible with c22 voltages/etc?
The nxp hardware in question has 1, 10 and 40Gbit phys on the same MDIO,
the 1gbit we fall back to c22 registers because it doesn't respond
correctly to c45 registers. Which is AFAIK what the bit0 C22 regs bit is
for..
The general logic right now for a C45_FIRST is attempt to detect a c45
phy and if nothing is detected fall back and attempt c22 register
access. That is whats picking up the 1G phys. If for whatever reason the
MDIO controller can't do the right thing to access the c22 regs, I guess
there really isn't anything we can do about it.
Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0),
MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1). I suspect
that was no coincidence in Andrew's suggestion.
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi, On 5/25/20 5:06 PM, Andrew Lunn wrote: Yes, we know even for the NXP reference hardware, one of the phy's doesn't probe out correctly because it doesn't respond to the ieee defined registers. I think at this point, there really isn't anything we can do about that unless we involve the (ACPI) firmware in currently nonstandard behaviors. So, my goals here have been to first, not break anything, and then do a slightly better job finding phy's that are (mostly?) responding correctly to the 802.3 spec. So we can say "if you hardware is ACPI conformant, and you have IEEE conformant phy's you should be ok". So, for your example phy, I guess the immediate answer is "use DT" or "find a conformant phy", or even "abstract it in the firmware and use a mailbox interface". Hi Jeremy You need to be careful here, when you say "use DT". With a c45 PHY of_mdiobus_register_phy() calls get_phy_device() to see if the device exists on the bus. So your changes to get_phy_device() etc, needs to continue to find devices it used to find, even if they are not fully complient to 802.3. Yes, that is my "don't break anything". But, in a number of cases I can't tell if something is an intentional "bug", or what exactly the intended side effect actually was. The c22 bit0 sanitation is in this bucket, because its basically disabling the MMD0 probe.. I know for sure we find phys that previously weren't found. OTOH, i'm not sure how many that were previously "found" are now getting kicked out by because they are doing something "bad" that looked like a bug.
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
On 5/25/20 5:01 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 04:51:16PM -0500, Jeremy Linton wrote:
Hi,
On 5/25/20 5:06 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 10:34:13PM -0500, Jeremy Linton wrote:
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x returns
that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases seems to
overlap sufficiently to cause this to throw them out. Plus a phy would also
have to have model+revision set to 'F's. So while might be possible, if
unlikely, at the moment I think the OUI registration keeps this from being a
problem. Particularly, if i'm reading the mapping correctly, the OUI mapping
guarantees that the field cannot be all '1's due to the OUI having X & M
bits cleared. It sort of looks like the mapping is trying to lose those
bits, by tossing bit 1 & 2, but the X & M are in the wrong octet (AFAIK, I
just read it three times cause it didn't make any sense).
I should also note that we have at least one supported PHY where one
of the MMDs returns 0xfffe for even numbered registers and 0x for
odd numbered registers in one of the vendor MMDs for addresses 0
through 0xefff - which has a bit set in the devices-in-package.
It also returns 0x0082 for almost every register in MMD 2, but MMD 2's
devices-in-package bit is clear in most of the valid MMDs, so we
shouldn't touch it.
These reveal the problem of randomly probing MMDs - they can return
unexpected values and not be as well behaved as we would like them to
be. Using register 8 to detect presence may be beneficial, but that
may also introduce problems as we haven't used that before (and we
don't know whether any PHY that wrong.) I know at least the 88x3310
gets it right for all except the vendor MMDs, where the low addresses
appear non-confromant to the 802.3 specs. Both vendor MMDs are
definitely implemented, just not with anything conforming to 802.3.
Yes, we know even for the NXP reference hardwa
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi,
On 5/25/20 5:01 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 04:51:16PM -0500, Jeremy Linton wrote:
Hi,
On 5/25/20 5:06 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 10:34:13PM -0500, Jeremy Linton wrote:
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x returns
that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases seems to
overlap sufficiently to cause this to throw them out. Plus a phy would also
have to have model+revision set to 'F's. So while might be possible, if
unlikely, at the moment I think the OUI registration keeps this from being a
problem. Particularly, if i'm reading the mapping correctly, the OUI mapping
guarantees that the field cannot be all '1's due to the OUI having X & M
bits cleared. It sort of looks like the mapping is trying to lose those
bits, by tossing bit 1 & 2, but the X & M are in the wrong octet (AFAIK, I
just read it three times cause it didn't make any sense).
I should also note that we have at least one supported PHY where one
of the MMDs returns 0xfffe for even numbered registers and 0x for
odd numbered registers in one of the vendor MMDs for addresses 0
through 0xefff - which has a bit set in the devices-in-package.
It also returns 0x0082 for almost every register in MMD 2, but MMD 2's
devices-in-package bit is clear in most of the valid MMDs, so we
shouldn't touch it.
These reveal the problem of randomly probing MMDs - they can return
unexpected values and not be as well behaved as we would like them to
be. Using register 8 to detect presence may be beneficial, but that
may also introduce problems as we haven't used that before (and we
don't know whether any PHY that wrong.) I know at least the 88x3310
gets it right for all except the vendor MMDs, where the low addresses
appear non-confromant to the 802.3 specs. Both vendor MMDs are
definitely implemented, just not with anything conforming to 802.3.
Yes, we know even for the NXP reference h
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
On 5/25/20 6:09 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 05:22:07PM -0500, Jeremy Linton wrote:
On 5/25/20 5:01 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 04:51:16PM -0500, Jeremy Linton wrote:
Hi,
On 5/25/20 5:06 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 10:34:13PM -0500, Jeremy Linton wrote:
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x returns
that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases seems to
overlap sufficiently to cause this to throw them out. Plus a phy would also
have to have model+revision set to 'F's. So while might be possible, if
unlikely, at the moment I think the OUI registration keeps this from being a
problem. Particularly, if i'm reading the mapping correctly, the OUI mapping
guarantees that the field cannot be all '1's due to the OUI having X & M
bits cleared. It sort of looks like the mapping is trying to lose those
bits, by tossing bit 1 & 2, but the X & M are in the wrong octet (AFAIK, I
just read it three times cause it didn't make any sense).
I should also note that we have at least one supported PHY where one
of the MMDs returns 0xfffe for even numbered registers and 0x for
odd numbered registers in one of the vendor MMDs for addresses 0
through 0xefff - which has a bit set in the devices-in-package.
It also returns 0x0082 for almost every register in MMD 2, but MMD 2's
devices-in-package bit is clear in most of the valid MMDs, so we
shouldn't touch it.
These reveal the problem of randomly probing MMDs - they can return
unexpected values and not be as well behaved as we would like them to
be. Using register 8 to detect presence may be beneficial, but that
may also introduce problems as we haven't used that before (and we
don't know whether any PHY that wrong.) I know at least the 88x3310
gets it right for all except the vendor MMDs, where the low addresses
appear non-confromant to the 802.3 specs. Both ve
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi,
On 5/25/20 6:33 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 06:22:19PM -0500, Jeremy Linton wrote:
On 5/25/20 6:09 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 05:22:07PM -0500, Jeremy Linton wrote:
On 5/25/20 5:01 PM, Russell King - ARM Linux admin wrote:
On Mon, May 25, 2020 at 04:51:16PM -0500, Jeremy Linton wrote:
Hi,
On 5/25/20 5:06 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 10:34:13PM -0500, Jeremy Linton wrote:
Hi,
On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
Until this point, we have been sanitizing the c22
regs presence bit out of all the MMD device lists.
This is incorrect as it causes the 0x checks
to incorrectly fail. Further, it turns out that we
want to utilize this flag to make a determination that
there is actually a phy at this location and we should
be accessing it using c22.
Signed-off-by: Jeremy Linton
---
drivers/net/phy/phy_device.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f0761fa5e40b..2d677490ecab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int
addr, int dev_addr,
return -EIO;
*devices_in_package |= phy_reg;
- /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
- *devices_in_package &= ~BIT(0);
-
return 0;
}
@@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
int i;
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 *devs = &c45_ids->devices_in_package;
+ bool c22_present = false;
+ bool valid_id = false;
/* Find first non-zero Devices In package. Device zero is reserved
* for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
return 0;
}
+ /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+ c22_present = *devs & BIT(0);
+ *devs &= ~BIT(0);
+
/* Now probe Device Identifiers for each device present. */
for (i = 1; i < num_ids; i++) {
if (!(c45_ids->devices_in_package & (1 << i)))
@@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
if (ret < 0)
return ret;
+ if (valid_phy_id(c45_ids->device_ids[i]))
+ valid_id = true;
Here you are using your "devices in package" validator to validate the
PHY ID value. One of the things it does is mask this value with
0x1fff. That means you lose some of the vendor OUI. To me, this
looks completely wrong.
I think in this case I was just using it like the comment in
get_phy_device() "if the phy_id is mostly F's, there is no device here".
My understanding is that the code is trying to avoid the 0x returns
that seem to indicate "bus ok, phy didn't respond".
I just checked the OUI registration, and while there are a couple OUI's
registered that have a number of FFF's in them, none of those cases seems to
overlap sufficiently to cause this to throw them out. Plus a phy would also
have to have model+revision set to 'F's. So while might be possible, if
unlikely, at the moment I think the OUI registration keeps this from being a
problem. Particularly, if i'm reading the mapping correctly, the OUI mapping
guarantees that the field cannot be all '1's due to the OUI having X & M
bits cleared. It sort of looks like the mapping is trying to lose those
bits, by tossing bit 1 & 2, but the X & M are in the wrong octet (AFAIK, I
just read it three times cause it didn't make any sense).
I should also note that we have at least one supported PHY where one
of the MMDs returns 0xfffe for even numbered registers and 0x for
odd numbered registers in one of the vendor MMDs for addresses 0
through 0xefff - which has a bit set in the devices-in-package.
It also returns 0x0082 for almost every register in MMD 2, but MMD 2's
devices-in-package bit is clear in most of the valid MMDs, so we
shouldn't touch it.
These reveal the problem of randomly probing MMDs - they can return
unexpected values and not be as well behaved as we would like them to
be. Using register 8 to detect presence may be beneficial, but that
may also introduce problems as we haven't used that before (and we
don't know whether any PHY that wrong.) I know at least the 8
Re: [RFC 04/11] net: phy: Handle c22 regs presence better
Hi, On 5/25/20 6:12 PM, Andrew Lunn wrote: arch/powerpc/boot/dts/fsl/t4240rdb.dts: compatible = "ethernet-phy-id13e5.1002"; arch/powerpc/boot/dts/fsl/t4240rdb.dts: compatible = "ethernet-phy-id13e5.1002"; arch/powerpc/boot/dts/fsl/t4240rdb.dts: compatible = "ethernet-phy-id13e5.1002"; arch/powerpc/boot/dts/fsl/t4240rdb.dts: compatible = "ethernet-phy-id13e5.1002"; arch/powerpc/boot/dts/fsl/t2080rdb.dts: compatible = "ethernet-phy-id13e5.1002"; arch/powerpc/boot/dts/fsl/t2080rdb.dts: compatible = "ethernet-phy-id13e5.1002"; Hi Jeremy You are doing this work for NXP right? Maybe you can ask them to go searching in the cellar and find you one of these boards? Yes, thats a good idea. I've been talking to various parties to let me run this code on some of their "odd" devices.
Re: [PATCH RFC 7/7] net: phy: read MMD ID from all present MMDs
Hi,
On 5/26/20 9:31 AM, Russell King wrote:
Expand the device_ids[] array to allow all MMD IDs to be read rather
than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
register reports that a device really is present here for these new
devices to maintain compatibility with our current behaviour.
88X3310 PHY vendor MMDs do are marked as present in the
devices_in_package, but do not contain IEE 802.3 compatible register
sets in their lower space. This avoids reading incorrect values as MMD
identifiers.
Signed-off-by: Russell King
---
drivers/net/phy/phy_device.c | 14 ++
include/linux/phy.h | 2 +-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1c948bbf4fa0..92742c7be80f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -773,6 +773,20 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
u32 *phy_id,
if (!(devs_in_pkg & (1 << i)))
continue;
+ if (i >= 8) {
+ /* Only probe the MMD ID for MMDs >= 8 if they report
+* that they are present. We have at least one PHY that
+* reports MMD presence in devs_in_pkg, but does not
+* contain valid IEEE 802.3 ID registers in some MMDs.
+*/
+ ret = phy_c45_probe_present(bus, addr, i);
+ if (ret < 0)
+ return ret;
+
+ if (!ret)
+ continue;
+ }
+
phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
if (phy_reg < 0)
return -EIO;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0d41c710339a..3325dd8fb9ac 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -361,7 +361,7 @@ enum phy_state {
struct phy_c45_device_ids {
u32 devices_in_package;
u32 mmds_present;
- u32 device_ids[8];
+ u32 device_ids[MDIO_MMD_NUM];
You have a array overflow/invalid access if you don't do this earlier in
4/7.
};
struct macsec_context;
