* [PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config
From: Alexandru Ardelean @ 2019-08-05 16:54 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, hkallweit1, andrew,
Alexandru Ardelean
In-Reply-To: <20190805165453.3989-1-alexandru.ardelean@analog.com>
The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
unconfigured) is RGMII.
This change adds support for configuring these modes via the device
registers.
For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
the default delay is 2 ns. This can be configurable and will be done in
a subsequent change.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 3dd9fe50f4c8..dbdb8f60741c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -33,14 +33,91 @@
ADIN1300_INT_HW_IRQ_EN)
#define ADIN1300_INT_STATUS_REG 0x0019
+#define ADIN1300_GE_RGMII_CFG_REG 0xff23
+#define ADIN1300_GE_RGMII_RXID_EN BIT(2)
+#define ADIN1300_GE_RGMII_TXID_EN BIT(1)
+#define ADIN1300_GE_RGMII_EN BIT(0)
+
+#define ADIN1300_GE_RMII_CFG_REG 0xff24
+#define ADIN1300_GE_RMII_EN BIT(0)
+
+static int adin_config_rgmii_mode(struct phy_device *phydev,
+ phy_interface_t intf)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
+ if (reg < 0)
+ return reg;
+
+ if (!phy_interface_mode_is_rgmii(intf)) {
+ reg &= ~ADIN1300_GE_RGMII_EN;
+ goto write;
+ }
+
+ reg |= ADIN1300_GE_RGMII_EN;
+
+ if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
+ intf == PHY_INTERFACE_MODE_RGMII_RXID) {
+ reg |= ADIN1300_GE_RGMII_RXID_EN;
+ } else {
+ reg &= ~ADIN1300_GE_RGMII_RXID_EN;
+ }
+
+ if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
+ intf == PHY_INTERFACE_MODE_RGMII_TXID) {
+ reg |= ADIN1300_GE_RGMII_TXID_EN;
+ } else {
+ reg &= ~ADIN1300_GE_RGMII_TXID_EN;
+ }
+
+write:
+ return phy_write_mmd(phydev, MDIO_MMD_VEND1,
+ ADIN1300_GE_RGMII_CFG_REG, reg);
+}
+
+static int adin_config_rmii_mode(struct phy_device *phydev,
+ phy_interface_t intf)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
+ if (reg < 0)
+ return reg;
+
+ if (intf != PHY_INTERFACE_MODE_RMII) {
+ reg &= ~ADIN1300_GE_RMII_EN;
+ goto write;
+ }
+
+ reg |= ADIN1300_GE_RMII_EN;
+
+write:
+ return phy_write_mmd(phydev, MDIO_MMD_VEND1,
+ ADIN1300_GE_RMII_CFG_REG, reg);
+}
+
static int adin_config_init(struct phy_device *phydev)
{
- int rc;
+ phy_interface_t interface, rc;
rc = genphy_config_init(phydev);
if (rc < 0)
return rc;
+ interface = phydev->interface;
+
+ rc = adin_config_rgmii_mode(phydev, interface);
+ if (rc < 0)
+ return rc;
+
+ rc = adin_config_rmii_mode(phydev, interface);
+ if (rc < 0)
+ return rc;
+
+ dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
+ phy_modes(phydev->interface));
+
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH 02/16] net: phy: adin: hook genphy_{suspend,resume} into the driver
From: Alexandru Ardelean @ 2019-08-05 16:54 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, hkallweit1, andrew,
Alexandru Ardelean
In-Reply-To: <20190805165453.3989-1-alexandru.ardelean@analog.com>
The chip supports standard suspend/resume via BMCR reg.
Hook these functions into the `adin` driver.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/net/phy/adin.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 6a610d4563c3..c100a0dd95cd 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -34,6 +34,8 @@ static struct phy_driver adin_driver[] = {
.config_init = adin_config_init,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .resume = genphy_resume,
+ .suspend = genphy_suspend,
},
{
.phy_id = PHY_ID_ADIN1300,
@@ -43,6 +45,8 @@ static struct phy_driver adin_driver[] = {
.config_init = adin_config_init,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .resume = genphy_resume,
+ .suspend = genphy_suspend,
},
};
--
2.20.1
^ permalink raw reply related
* [PATCH 06/16] net: phy: adin: support PHY mode converters
From: Alexandru Ardelean @ 2019-08-05 16:54 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, hkallweit1, andrew,
Alexandru Ardelean
In-Reply-To: <20190805165453.3989-1-alexandru.ardelean@analog.com>
Sometimes, the connection between a MAC and PHY is done via a
mode/interface converter. An example is a GMII-to-RGMII converter, which
would mean that the MAC operates in GMII mode while the PHY operates in
RGMII. In this case there is a discrepancy between what the MAC expects &
what the PHY expects and both need to be configured in their respective
modes.
Sometimes, this converter is specified via a board/system configuration (in
the device-tree for example). But, other times it can be left unspecified.
The use of these converters is common in boards that have FPGA on them.
This patch also adds support for a `adi,phy-mode-internal` property that
can be used in these (implicit convert) cases. The internal PHY mode will
be used to specify the correct register settings for the PHY.
`fwnode_handle` is used, since this property may be specified via ACPI as
well in other setups, but testing has been done in DT context.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/net/phy/adin.c | 39 ++++++++++++++++++++++++++++++++++++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index dbdb8f60741c..e3d2ff8cc09c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/mii.h>
#include <linux/phy.h>
+#include <linux/property.h>
#define PHY_ID_ADIN1200 0x0283bc20
#define PHY_ID_ADIN1300 0x0283bc30
@@ -41,6 +42,31 @@
#define ADIN1300_GE_RMII_CFG_REG 0xff24
#define ADIN1300_GE_RMII_EN BIT(0)
+static int adin_get_phy_internal_mode(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ const char *pm;
+ int i;
+
+ if (device_property_read_string(dev, "adi,phy-mode-internal", &pm))
+ return phydev->interface;
+
+ /**
+ * Getting here assumes that there is converter in-between the actual
+ * PHY, for example a GMII-to-RGMII converter. In this case the MAC
+ * talks GMII and PHY talks RGMII, so the PHY needs to be set in RGMII
+ * while the MAC can work in GMII mode.
+ */
+
+ for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
+ if (!strcasecmp(pm, phy_modes(i)))
+ return i;
+
+ dev_err(dev, "Invalid value for 'phy-mode-internal': '%s'\n", pm);
+
+ return -EINVAL;
+}
+
static int adin_config_rgmii_mode(struct phy_device *phydev,
phy_interface_t intf)
{
@@ -105,7 +131,9 @@ static int adin_config_init(struct phy_device *phydev)
if (rc < 0)
return rc;
- interface = phydev->interface;
+ interface = adin_get_phy_internal_mode(phydev);
+ if (interface < 0)
+ return interface;
rc = adin_config_rgmii_mode(phydev, interface);
if (rc < 0)
@@ -115,8 +143,13 @@ static int adin_config_init(struct phy_device *phydev)
if (rc < 0)
return rc;
- dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
- phy_modes(phydev->interface));
+ if (phydev->interface == interface)
+ dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
+ phy_modes(phydev->interface));
+ else
+ dev_info(&phydev->mdio.dev,
+ "PHY is using mode '%s', MAC is using mode '%s'\n",
+ phy_modes(interface), phy_modes(phydev->interface));
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH 00/16] net: phy: adin: add support for Analog Devices PHYs
From: Alexandru Ardelean @ 2019-08-05 16:54 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, robh+dt, mark.rutland, f.fainelli, hkallweit1, andrew,
Alexandru Ardelean
This changeset adds support for Analog Devices Industrial Ethernet PHYs.
Particularly the PHYs this driver adds support for:
* ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
* ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
Ethernet PHY
The 2 chips are pin & register compatible with one another. The main
difference being that ADIN1200 doesn't operate in gigabit mode.
The chips can be operated by the Generic PHY driver as well via the
standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
kernel as well. This assumes that configuration of the PHY has been done
completely in HW, according to spec, i.e. no extra SW configuration
required.
This changeset also implements the ability to configure the chips via SW
registers.
Datasheets:
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Alexandru Ardelean (16):
net: phy: adin: add support for Analog Devices PHYs
net: phy: adin: hook genphy_{suspend,resume} into the driver
net: phy: adin: add support for interrupts
net: phy: adin: add {write,read}_mmd hooks
net: phy: adin: configure RGMII/RMII/MII modes on config
net: phy: adin: support PHY mode converters
net: phy: adin: make RGMII internal delays configurable
net: phy: adin: make RMII fifo depth configurable
net: phy: adin: add support MDI/MDIX/Auto-MDI selection
net: phy: adin: add EEE translation layer for Clause 22
net: phy: adin: PHY reset mechanisms
net: phy: adin: read EEE setting from device-tree
net: phy: adin: implement Energy Detect Powerdown mode
net: phy: adin: make sure down-speed auto-neg is enabled
net: phy: adin: add ethtool get_stats support
dt-bindings: net: add bindings for ADIN PHY driver
.../devicetree/bindings/net/adi,adin.yaml | 93 +++
MAINTAINERS | 9 +
drivers/net/phy/Kconfig | 9 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/adin.c | 752 ++++++++++++++++++
include/dt-bindings/net/adin.h | 26 +
6 files changed, 890 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
create mode 100644 drivers/net/phy/adin.c
create mode 100644 include/dt-bindings/net/adin.h
--
2.20.1
^ permalink raw reply
* Re: [PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock
From: Andrew Lunn @ 2019-08-05 13:58 UTC (permalink / raw)
To: Hubert Feurstein
Cc: netdev, linux-kernel, Richard Cochran, Vivien Didelot,
Florian Fainelli, David S. Miller
In-Reply-To: <20190805082642.12873-1-hubert.feurstein@vahle.at>
On Mon, Aug 05, 2019 at 10:26:42AM +0200, Hubert Feurstein wrote:
> From: Hubert Feurstein <h.feurstein@gmail.com>
Hi Hubert
In your RFC patch, there was some interesting numbers. Can you provide
numbers of just this patch? How much of an improvement does it make?
Your RFC patch pushed these ptp_read_system_{pre|post}ts() calls down
into the MDIO driver. This patch is much less invasive, but i wonder
how much a penalty you paid?
Did you also try moving these calls into global2_avb.c, around the one
write that really matters?
It was speculated that the jitter comes from contention on the mdio
bus lock. Did you investigate this? If you can prove this true, one
thing to try is to take the mdio bus lock in the mv88e6xxx driver,
take the start timestamp, call __mdiobus_write(), and then the end
timestamp. The bus contention is then outside your time snapshot.
We could even think about adding a mdiobus_write variant which does
all this. I'm sure other DSA drivers would find it useful, if it
really does help.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: David Ahern @ 2019-08-05 14:10 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <20190805055422.GA2349@nanopsycho.orion>
On 8/4/19 11:54 PM, Jiri Pirko wrote:
> There was implicit devlink instance creation per-namespace. No relation
> any actual device. It was wrong and misuse of devlink.
>
> Now you have 1 devlink instance per 1 device as it should be. Also, you
> have fib resource control for this device, also as it is done for real
> devices, like mlxsw.
>
> Could you please describe your usecase? Perhaps we can handle
> it differently.
I have described this before, multiple times.
It is documented in the commit log for the initial fib.c in netdevsim
(37923ed6b8cea94d7d76038e2f72c57a0b45daab) and
https://lore.kernel.org/netdev/20180328012200.15175-7-dsa@cumulusnetworks.com/
And this comment in the discussion thread:
https://lore.kernel.org/netdev/e9c59b0c-328e-d343-6e8d-d19f643d2e9d@cumulusnetworks.com/:
"The intention is to treat the kernel's tables *per namespace* as a
standalone entity that can be managed very similar to ASIC resources."
So, to state this again, the fib.c in the RFC version
https://lore.kernel.org/netdev/20180322225757.10377-8-dsa@cumulusnetworks.com/
targeted this:
namespace 1 | namespace 2 | ... | namespace N
| | |
| | |
devlink 1 | devlink 2 | ... | devlink N
and each devlink instance has resource limits for the number of fib
rules and fib entries *for that namespace* only.
You objected to how the devlink instances per namespace was implemented,
so the non-RFC version limited the devlink instance and resource
controller to init_net only. Fine. I accepted that limitation until
someone had time to work on devlink instances per network namespace
which you are doing now. So, the above goal will be achievable but first
you need to fix the breakage you put into v5.2 and forward.
Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
per-namepace accounting to all namespaces managed by a single devlink
instance in init_net - which is completely wrong.
Move the fib accounting back to per namespace as the original code
intended. If you now want the devlink instance to be namespace based
then it should be trivial for you to fix it and will work going forward.
^ permalink raw reply
* Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver
From: Andrew Lunn @ 2019-08-05 14:11 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-17-alexandru.ardelean@analog.com>
> + adi,rx-internal-delay:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> + is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> + default value is 0 (which represents 2 ns)
> + enum: [ 0, 1, 2, 6, 7 ]
We want these numbers to be in ns. So the default value would actually
be 2. The driver needs to convert the number in DT to a value to poke
into a PHY register. Please rename the property adi,rx-internal-delay-ns.
> +
> + adi,tx-internal-delay:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> + is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> + default value is 0 (which represents 2 ns)
> + enum: [ 0, 1, 2, 6, 7 ]
Same here.
> +
> + adi,fifo-depth:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + When operating in RMII mode, this option configures the FIFO depth.
> + See `dt-bindings/net/adin.h`.
> + enum: [ 0, 1, 2, 3, 4, 5 ]
Units? You should probably rename this adi,fifo-depth-bits and list
the valid values in bits.
> +
> + adi,eee-enabled:
> + description: |
> + Advertise EEE capabilities on power-up/init (default disabled)
> + type: boolean
It is not the PHY which decides this. The MAC indicates if it is EEE
capable to phylib. phylib looks into the PHY registers to determine if
the PHY supports EEE. phylib will then enable EEE
advertisement. Please remove this, and ensure EEE is disabled by
default.
Andrew
^ permalink raw reply
* Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs
From: Andrew Lunn @ 2019-08-05 14:16 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-2-alexandru.ardelean@analog.com>
> +static int adin_config_init(struct phy_device *phydev)
> +{
> + int rc;
> +
> + rc = genphy_config_init(phydev);
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
Why not just
return genphy_config_init(phydev);
Andrew
^ permalink raw reply
* Re: [PATCH 02/16] net: phy: adin: hook genphy_{suspend,resume} into the driver
From: Andrew Lunn @ 2019-08-05 14:17 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-3-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:39PM +0300, Alexandru Ardelean wrote:
> The chip supports standard suspend/resume via BMCR reg.
> Hook these functions into the `adin` driver.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 03/16] net: phy: adin: add support for interrupts
From: Andrew Lunn @ 2019-08-05 14:21 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-4-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:40PM +0300, Alexandru Ardelean wrote:
> This change adds support for enabling PHY interrupts that can be used by
> the PHY framework to get signal for link/speed/auto-negotiation changes.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index c100a0dd95cd..b75c723bda79 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,22 @@
> #define PHY_ID_ADIN1200 0x0283bc20
> #define PHY_ID_ADIN1300 0x0283bc30
>
> +#define ADIN1300_INT_MASK_REG 0x0018
> +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9)
> +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8)
> +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6)
> +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5)
> +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4)
> +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3)
> +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2)
> +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1)
> +#define ADIN1300_INT_HW_IRQ_EN BIT(0)
> +#define ADIN1300_INT_MASK_EN \
> + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> + ADIN1300_INT_HW_IRQ_EN)
> +#define ADIN1300_INT_STATUS_REG 0x0019
> +
> static int adin_config_init(struct phy_device *phydev)
> {
> int rc;
> @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
> return 0;
> }
>
> +static int adin_phy_ack_intr(struct phy_device *phydev)
> +{
> + int ret;
> +
> + /* Clear pending interrupts. */
> + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
Please go through the whole driver and throw out all the needless
if (ret < 0)
return ret;
return 0;
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH 04/16] net: phy: adin: add {write,read}_mmd hooks
From: Andrew Lunn @ 2019-08-05 14:25 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-5-alexandru.ardelean@analog.com>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index b75c723bda79..3dd9fe50f4c8 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,9 @@
> #define PHY_ID_ADIN1200 0x0283bc20
> #define PHY_ID_ADIN1300 0x0283bc30
>
> +#define ADIN1300_MII_EXT_REG_PTR 0x10
> +#define ADIN1300_MII_EXT_REG_DATA 0x11
> +
> #define ADIN1300_INT_MASK_REG 0x0018
Please be consistent with registers. Either use 4 digits, or 2 digits.
Andrew
^ permalink raw reply
* Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver
From: Andrew Lunn @ 2019-08-05 14:27 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-17-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:53PM +0300, Alexandru Ardelean wrote:
> This change adds bindings for the Analog Devices ADIN PHY driver, detailing
> all the properties implemented by the driver.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> .../devicetree/bindings/net/adi,adin.yaml | 93 +++++++++++++++++++
> MAINTAINERS | 2 +
> include/dt-bindings/net/adin.h | 26 ++++++
> 3 files changed, 121 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
> create mode 100644 include/dt-bindings/net/adin.h
>
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> new file mode 100644
> index 000000000000..fcf884bb86f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/adi,adin.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADIN1200/ADIN1300 PHY
> +
> +maintainers:
> + - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> + Bindings for Analog Devices Industrial Ethernet PHYs
> +
> +properties:
> + compatible:
> + description: |
> + Compatible list, may contain "ethernet-phy-ieee802.3-c45" in which case
> + Clause 45 will be used to access device management registers. If
> + unspecified, Clause 22 will be used. Use this only when MDIO supports
> + Clause 45 access, but there is no other way to determine this.
> + enum:
> + - ethernet-phy-ieee802.3-c45
It is valid to list ethernet-phy-ieee802.3-c22, it is just not
required. So maybe you should list it here to keep the DT validater happy?
Andrew
^ permalink raw reply
* RE: [PATCH net-next v3 2/2] qed: Add driver API for flashing the config attributes.
From: Ariel Elior @ 2019-08-05 14:30 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru, David Miller
Cc: netdev@vger.kernel.org, Michal Kalderon
In-Reply-To: <MN2PR18MB2528F3206069A06618CBCCAFD3DC0@MN2PR18MB2528.namprd18.prod.outlook.com>
> From: Sudarsana Reddy Kalluru
> Sent: Tuesday, July 30, 2019 6:36 AM
> To: David Miller <davem@davemloft.net>
>
> > -----Original Message-----
> > From: David Miller <davem@davemloft.net>
> > Sent: Monday, July 29, 2019 11:34 PM
> > To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> > Cc: netdev@vger.kernel.org; Michal Kalderon <mkalderon@marvell.com>;
> > Ariel Elior <aelior@marvell.com>
> > Subject: Re: [PATCH net-next v3 2/2] qed: Add driver API for flashing
> > the config attributes.
> >
> > From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> > Date: Sat, 27 Jul 2019 18:55:49 -0700
> >
> > > @@ -2268,6 +2330,9 @@ static int qed_nvm_flash(struct qed_dev *cdev,
> > const char *name)
> > > rc = qed_nvm_flash_image_access(cdev, &data,
> > > &check_resp);
> > > break;
> > > + case QED_NVM_FLASH_CMD_NVM_CFG_ID:
> > > + rc = qed_nvm_flash_cfg_write(cdev, &data);
> > > + break;
>
> > > default:
> > > DP_ERR(cdev, "Unknown command %08x\n",
> > cmd_type);
> >
> > I don't see how any existing portable interface can cause this new
> > code to actually be used.
> >
> > You have to explain this to me.
> The API qed_nvm_flash() is used to flash the user provided data (e.g.,
> Management FW) to the required partitions of the adapter.
> - Format of the input file would be - file signature info, followed by one or
> more data sets.
> - Each data set is represented with the header followed by its contents.
> Header captures info such as command name (e.g., FILE_START), data size
> etc., which specifies how to handle the data.
> The API qed_nvm_flash() validates the user provided input file, parses the
> data sets and handles each accordingly. Here one of the data sets (preferably
> the last one) could be nvm-attributes page (with cmd-id =
> QED_NVM_FLASH_CMD_NVM_CHANGE).
This is basically an expansion of our existing ethtool -f implementation.
The management FW has exposed an additional method of configuring
some of the nvram options, and this makes use of that. The new code will
come into use when newer FW files which contain configuration directives
employing this API will be provided to ethtool -f.
thanks,
Ariel
^ permalink raw reply
* Re: [RFC PATCH 1/2] dt-bindings: net: macb: Add new property for PS SGMII only
From: Harini Katakam @ 2019-08-05 14:36 UTC (permalink / raw)
To: Andrew Lunn
Cc: Harini Katakam, Nicolas Ferre, David Miller, Claudiu Beznea,
Rob Herring, Mark Rutland, netdev, linux-kernel, Michal Simek,
devicetree
In-Reply-To: <20190805132045.GC24275@lunn.ch>
Hi Andrew,
On Mon, Aug 5, 2019 at 7:00 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Aug 05, 2019 at 11:45:05AM +0530, Harini Katakam wrote:
> > Hi Andrew,
> >
> > On Sun, Aug 4, 2019 at 8:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Wed, Jul 31, 2019 at 03:10:32PM +0530, Harini Katakam wrote:
> > > > Add a new property to indicate when PS SGMII is used with NO
> > > > external PHY on board.
> > >
> > > Hi Harini
> > >
> > > What exactly is you use case? Are you connecting to a Ethernet switch?
> > > To an SFP cage with a copper module?
> >
> > Yes, an SFP cage is the common HW target for this patch.
>
> Hi Harini
>
> So you have a copper PHY in the SFP cage. It will talk SGMII
> signalling to your PS SGMII. When that signalling is complete i would
> expect the MAC to raise an interrupt, just as if the SGMII PHY was
> soldered on the board. So i don't see why you need this polling?
>
Thanks. Sorry, I overlooked this interrupt. Let me try that.
Regards,
Harini
^ permalink raw reply
* Re: [PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config
From: Andrew Lunn @ 2019-08-05 14:39 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-6-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:42PM +0300, Alexandru Ardelean wrote:
> The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
> unconfigured) is RGMII.
> This change adds support for configuring these modes via the device
> registers.
>
> For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
It would be nice to add the missing space.
> the default delay is 2 ns. This can be configurable and will be done in
> a subsequent change.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 3dd9fe50f4c8..dbdb8f60741c 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -33,14 +33,91 @@
> ADIN1300_INT_HW_IRQ_EN)
> #define ADIN1300_INT_STATUS_REG 0x0019
>
> +#define ADIN1300_GE_RGMII_CFG_REG 0xff23
> +#define ADIN1300_GE_RGMII_RXID_EN BIT(2)
> +#define ADIN1300_GE_RGMII_TXID_EN BIT(1)
> +#define ADIN1300_GE_RGMII_EN BIT(0)
> +
> +#define ADIN1300_GE_RMII_CFG_REG 0xff24
> +#define ADIN1300_GE_RMII_EN BIT(0)
> +
> +static int adin_config_rgmii_mode(struct phy_device *phydev,
> + phy_interface_t intf)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
> + if (reg < 0)
> + return reg;
> +
> + if (!phy_interface_mode_is_rgmii(intf)) {
> + reg &= ~ADIN1300_GE_RGMII_EN;
> + goto write;
> + }
> +
> + reg |= ADIN1300_GE_RGMII_EN;
> +
> + if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> + intf == PHY_INTERFACE_MODE_RGMII_RXID) {
> + reg |= ADIN1300_GE_RGMII_RXID_EN;
> + } else {
> + reg &= ~ADIN1300_GE_RGMII_RXID_EN;
> + }
> +
> + if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> + intf == PHY_INTERFACE_MODE_RGMII_TXID) {
> + reg |= ADIN1300_GE_RGMII_TXID_EN;
> + } else {
> + reg &= ~ADIN1300_GE_RGMII_TXID_EN;
> + }
Nice. Often driver writers forget to clear the delay, they only set
it. Not so here.
However, is checkpatch happy with this? Each half of the if/else is a
single statement, so the {} are not needed.
> +
> +write:
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> + ADIN1300_GE_RGMII_CFG_REG, reg);
> +}
> +
> +static int adin_config_rmii_mode(struct phy_device *phydev,
> + phy_interface_t intf)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
> + if (reg < 0)
> + return reg;
> +
> + if (intf != PHY_INTERFACE_MODE_RMII) {
> + reg &= ~ADIN1300_GE_RMII_EN;
> + goto write;
goto? Really?
> + }
> +
> + reg |= ADIN1300_GE_RMII_EN;
> +
> +write:
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> + ADIN1300_GE_RMII_CFG_REG, reg);
> +}
> +
> static int adin_config_init(struct phy_device *phydev)
> {
> - int rc;
> + phy_interface_t interface, rc;
genphy_config_init() does not return a phy_interface_t!
>
> rc = genphy_config_init(phydev);
> if (rc < 0)
> return rc;
>
> + interface = phydev->interface;
> +
> + rc = adin_config_rgmii_mode(phydev, interface);
> + if (rc < 0)
> + return rc;
> +
> + rc = adin_config_rmii_mode(phydev, interface);
> + if (rc < 0)
> + return rc;
> +
> + dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
> + phy_modes(phydev->interface));
phydev_dbg(), or not at all.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: Jiri Pirko @ 2019-08-05 14:49 UTC (permalink / raw)
To: David Ahern; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <796ba97c-9915-9a44-e933-4a7e22aaef2e@gmail.com>
Mon, Aug 05, 2019 at 04:10:39PM CEST, dsahern@gmail.com wrote:
>On 8/4/19 11:54 PM, Jiri Pirko wrote:
>> There was implicit devlink instance creation per-namespace. No relation
>> any actual device. It was wrong and misuse of devlink.
>>
>> Now you have 1 devlink instance per 1 device as it should be. Also, you
>> have fib resource control for this device, also as it is done for real
>> devices, like mlxsw.
>>
>> Could you please describe your usecase? Perhaps we can handle
>> it differently.
>
>I have described this before, multiple times.
>
>It is documented in the commit log for the initial fib.c in netdevsim
>(37923ed6b8cea94d7d76038e2f72c57a0b45daab) and
>https://lore.kernel.org/netdev/20180328012200.15175-7-dsa@cumulusnetworks.com/
>
>And this comment in the discussion thread:
>
>https://lore.kernel.org/netdev/e9c59b0c-328e-d343-6e8d-d19f643d2e9d@cumulusnetworks.com/:
>"The intention is to treat the kernel's tables *per namespace* as a
>standalone entity that can be managed very similar to ASIC resources."
>
>
>So, to state this again, the fib.c in the RFC version
>https://lore.kernel.org/netdev/20180322225757.10377-8-dsa@cumulusnetworks.com/
>
>targeted this:
>
> namespace 1 | namespace 2 | ... | namespace N
> | | |
> | | |
> devlink 1 | devlink 2 | ... | devlink N
>
>and each devlink instance has resource limits for the number of fib
>rules and fib entries *for that namespace* only.
>
>You objected to how the devlink instances per namespace was implemented,
>so the non-RFC version limited the devlink instance and resource
>controller to init_net only. Fine. I accepted that limitation until
>someone had time to work on devlink instances per network namespace
>which you are doing now. So, the above goal will be achievable but first
>you need to fix the breakage you put into v5.2 and forward.
>
>Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>per-namepace accounting to all namespaces managed by a single devlink
>instance in init_net - which is completely wrong.
No. Not "all namespaces". Only the one where the devlink is. And that is
always init_net, until this patchset.
>
>Move the fib accounting back to per namespace as the original code
>intended. If you now want the devlink instance to be namespace based
>then it should be trivial for you to fix it and will work going forward.
With this patchset, you can create netdevsim instance in a namespace,
set the resources limits on the devlink instance for this netdevsim
and you have what you need and what you had before. You just need to
create one netdevsim instance per network namespace.
Or multiple netdevsim instances in one namespace with different
limitations. Up to you.
^ permalink raw reply
* Re: [RFC] net: dsa: mv88e6xxx: ptp: improve phc2sys precision for mv88e6xxx switch in combination with imx6-fec
From: Hubert Feurstein @ 2019-08-05 14:50 UTC (permalink / raw)
To: Vladimir Oltean
Cc: mlichvar, Richard Cochran, Andrew Lunn, netdev, lkml,
Vivien Didelot, Florian Fainelli, David S. Miller
In-Reply-To: <CA+h21hr835sdvtgVOA2M9SWeCXDOrDG1S3FnNgJd_9NP2X_TaQ@mail.gmail.com>
Hi Vladimir,
Am Mo., 5. Aug. 2019 um 11:55 Uhr schrieb Vladimir Oltean <olteanv@gmail.com>:
[...]
> You guessed correctly (since you copied me) that I'm battling much of
> the same issues with the sja1105 and its spi-fsl-dspi controller
> driver.
I've copied you, because of this discussion on github:
https://github.com/openil/linuxptp/issues/5
There you said: "In fact any MDIO access will make the latency
unpredictable and hence
throw off the time."
I thought you might be interested in how to make MDIO latency predictable.
[...]
> - You said you patched linuxptp master. Could you share the patch? Is
> there anything else that's needed except compiling against the board's
> real kernel headers (aka point KBUILD_OUTPUT to the extracted location
> of /sys/kernel/kheaders.tar.xz)? I've done that and I do see phc2sys
> probing and using the new ioctl, but I don't see a big improvement in
> my case (that's probably because my SPI interface has real jitter
> caused by peripheral clock instability, but I really need to put a
> scope on it to be sure, so that's a discussion for another time).
My compiler used kernel headers where the ioctl was not yet defined.
So I simply defined it in the linuxptp source directly:
diff --git a/sysoff.c b/sysoff.c
index b993ee9..b23ad2f 100644
--- a/sysoff.c
+++ b/sysoff.c
@@ -27,6 +27,20 @@
#define NS_PER_SEC 1000000000LL
+#ifndef PTP_SYS_OFFSET_EXTENDED
+struct ptp_sys_offset_extended {
+ unsigned int n_samples; /* Desired number of measurements. */
+ unsigned int rsv[3]; /* Reserved for future use. */
+ /*
+ * Array of [system, phc, system] time stamps. The kernel will provide
+ * 3*n_samples time stamps.
+ */
+ struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
+};
+#define PTP_SYS_OFFSET_EXTENDED \
+ _IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
+#endif
+
#ifdef PTP_SYS_OFFSET
static int64_t pctns(struct ptp_clock_time *t)
> - I really wonder what your jitter is caused by. Maybe it is just
> contention on the mii_bus->mdio_lock? If that's the case, maybe you
> don't need to go all the way down to the driver level, and taking the
> ptp_sts at the subsystem (MDIO, SPI, I2C, etc) level is "good enough".
I would say there are many places, where we introduce unpredictable jitter:
- The busy-flag polling
- The locking of the chip- and mdio-bus-mutex
- The mdio_done completion in the imx_fec
- Scheduling latencies
> - Along the lines of the above, I wonder how badly would the
> maintainers shout at the proposal of adding a struct
> ptp_system_timestamp pointer and its associated lock in struct device.
> That way at least you don't have to change the API, like you did with
> mdiobus_write_nested_ptp. Relatively speaking, this is the least
> amount of intrusion (although, again, far from beautiful).
It is important to make sure to hook up the right mdio_write access, that is
why the ptp_sts pointer is passed under the mdio_lock. Of course It would
be nicer, to pass through the pointer as an argument, instead of bypassing it to
the mii_bus struct. In the case of SPI it could be added to the spi_transfer
struct.
> - The software timestamps help you in the particular case of phc2sys,
> but are they enough to cover all your needs?
For now it's all I need.
> I haven't spent even 1
> second looking at Marvell switch datasheets, but is its free-running
> timer only used for PTP timestamping? At least the sja1105 does also
> support time-based egress scheduling and ingress policing, so for that
> scenario, the timecounter/cyclecounter implementation will no longer
> cut it (you do need to synchronize the hardware clock). If your
> hardware supports these PTP-based features as well, I can only assume
> the reason why mv88e6xxx went for a timecounter is the same as the
> reason why I did: the MDIO/SPI jitter while accessing the frequency
> and offset correction registers is bad enough that the servo loop goes
> haywire. And implementing gettimex64 will not solve that.
>
[...]
Hubert
^ permalink raw reply related
* Re: [PATCH] staging: isdn: remove unnecessary parentheses
From: Dan Carpenter @ 2019-08-05 14:50 UTC (permalink / raw)
To: Thiago Bonotto
Cc: Karsten Keil, Greg Kroah-Hartman, netdev, devel, linux-kernel,
lkcamp
In-Reply-To: <20190802202323.27117-1-thbonotto@gmail.com>
This driver is obselete so we're just keeping it around for a couple
kernel releases and then deleting it. We're not taking cleanups for it.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
From: Andrew Lunn @ 2019-08-05 14:51 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-7-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> Sometimes, the connection between a MAC and PHY is done via a
> mode/interface converter. An example is a GMII-to-RGMII converter, which
> would mean that the MAC operates in GMII mode while the PHY operates in
> RGMII. In this case there is a discrepancy between what the MAC expects &
> what the PHY expects and both need to be configured in their respective
> modes.
>
> Sometimes, this converter is specified via a board/system configuration (in
> the device-tree for example). But, other times it can be left unspecified.
> The use of these converters is common in boards that have FPGA on them.
>
> This patch also adds support for a `adi,phy-mode-internal` property that
> can be used in these (implicit convert) cases. The internal PHY mode will
> be used to specify the correct register settings for the PHY.
>
> `fwnode_handle` is used, since this property may be specified via ACPI as
> well in other setups, but testing has been done in DT context.
Looking at the patch, you seems to assume phy-mode is what the MAC is
using? That seems rather odd, given the name. It seems like a better
solution would be to add a mac-mode, which the MAC uses to configure
its side of the link. The MAC driver would then implement this
property.
I don't see a need for this. phy-mode indicates what the PHY should
use. End of story.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: David Ahern @ 2019-08-05 14:51 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <20190805144927.GD2349@nanopsycho.orion>
On 8/5/19 8:49 AM, Jiri Pirko wrote:
>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>> per-namepace accounting to all namespaces managed by a single devlink
>> instance in init_net - which is completely wrong.
> No. Not "all namespaces". Only the one where the devlink is. And that is
> always init_net, until this patchset.
>
>
Jiri: your change to fib.c does not take into account namespace when
doing rules and routes accounting. you broke it. fix it.
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: dump nested registers
From: Vivien Didelot @ 2019-08-05 14:52 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev, f.fainelli, andrew, davem, linville, cphealy
In-Reply-To: <20190805080448.GA31971@unicorn.suse.cz>
Hi Michal!
On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > Usually kernel drivers set the regs->len value to the same length as
> > info->regdump_len, which was used for the allocation. In case where
> > regs->len is smaller than the allocated info->regdump_len length,
> > we may assume that the dump contains a nested set of registers.
> >
> > This becomes handy for kernel drivers to expose registers of an
> > underlying network conduit unfortunately not exposed to userspace,
> > as found in network switching equipment for example.
> >
> > This patch adds support for recursing into the dump operation if there
> > is enough room for a nested ethtool_drvinfo structure containing a
> > valid driver name, followed by a ethtool_regs structure like this:
> >
> > 0 regs->len info->regdump_len
> > v v v
> > +--------------+-----------------+--------------+-- - --+
> > | ethtool_regs | ethtool_drvinfo | ethtool_regs | |
> > +--------------+-----------------+--------------+-- - --+
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > ---
>
> I'm not sure about this approach. If these additional objects with their
> own registers are represented by a network device, we can query their
> registers directly. If they are not (which, IIUC, is the case in your
> use case), we should use an appropriate interface. AFAIK the CPU ports
> are already represented in devlink, shouldn't devlink be also used to
> query their registers?
Yet another interface wasn't that much appropriate for DSA, making the stack
unnecessarily complex. In fact we are already glueing the statistics of the CPU
port into the master's ethtool ops (both physical ports are wired together).
Adding support for nested registers dump in ethtool makes it simple to
(pretty) dump CPU port's registers without too much userspace addition.
>
> > ethtool.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/ethtool.c b/ethtool.c
> > index 05fe05a08..c0e2903c5 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -1245,7 +1245,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >
> > if (gregs_dump_raw) {
> > fwrite(regs->data, regs->len, 1, stdout);
> > - return 0;
> > + goto nested;
> > }
You're right regarding your comment about raw output. I can keep the return
0 here.
> >
> > if (!gregs_dump_hex)
> > @@ -1253,7 +1253,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> > if (!strncmp(driver_list[i].name, info->driver,
> > ETHTOOL_BUSINFO_LEN)) {
> > if (driver_list[i].func(info, regs) == 0)
> > - return 0;
> > + goto nested;
> > /* This version (or some other
> > * variation in the dump format) is
> > * not handled; fall back to hex
> > @@ -1263,6 +1263,15 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >
> > dump_hex(stdout, regs->data, regs->len, 0);
> >
> > +nested:
> > + /* Recurse dump if some drvinfo and regs structures are nested */
> > + if (info->regdump_len > regs->len + sizeof(*info) + sizeof(*regs)) {
> > + info = (struct ethtool_drvinfo *)(®s->data[0] + regs->len);
> > + regs = (struct ethtool_regs *)(®s->data[0] + regs->len + sizeof(*info));
> > +
> > + return dump_regs(gregs_dump_raw, gregs_dump_hex, info, regs);
> > + }
> > +
> > return 0;
> > }
> >
>
> For raw and hex dumps, this will dump only the payloads without any
> metadata allowing to identify what are the additional blocks for the
> other related objects, i.e. where they start, how long they are and what
> they belong to. That doesn't seem very useful.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH 1/2] usb: serial: option: Add the BroadMobi BM818 card
From: Bob Ham @ 2019-08-05 14:44 UTC (permalink / raw)
To: Johan Hovold, Angus Ainslie (Purism)
Cc: kernel, Bjørn Mork, David S. Miller, Greg Kroah-Hartman,
netdev, linux-usb, linux-kernel
In-Reply-To: <20190805114711.GF3574@localhost>
[-- Attachment #1.1: Type: text/plain, Size: 950 bytes --]
On 05/08/2019 12:47, Johan Hovold wrote:
> On Wed, Jul 24, 2019 at 07:52:26AM -0700, Angus Ainslie (Purism) wrote:
>> From: Bob Ham <bob.ham@puri.sm>
>>
>> Add a VID:PID for the BroadModi BM818 M.2 card
>
> Would you mind posting the output of usb-devices (or lsusb -v) for this
> device?
T: Bus=01 Lev=03 Prnt=40 Port=03 Cnt=01 Dev#= 44 Spd=480 MxCh= 0
D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
P: Vendor=2020 ProdID=2060 Rev=00.00
S: Manufacturer=Qualcomm, Incorporated
S: Product=Qualcomm CDMA Technologies MSM
C: #Ifs= 5 Cfg#= 1 Atr=e0 MxPwr=500mA
I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=fe Prot=ff Driver=(none)
I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH]][next] selftests: nettest: fix spelling mistake: "potocol" -> "protocol"
From: shuah @ 2019-08-05 15:04 UTC (permalink / raw)
To: Colin King, David S . Miller, netdev, linux-kselftest
Cc: kernel-janitors, linux-kernel, shuah
In-Reply-To: <20190805105211.27229-1-colin.king@canonical.com>
On 8/5/19 4:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in an error messgae. Fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> tools/testing/selftests/net/nettest.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/nettest.c b/tools/testing/selftests/net/nettest.c
> index 9278f8460d75..83515e5ea4dc 100644
> --- a/tools/testing/selftests/net/nettest.c
> +++ b/tools/testing/selftests/net/nettest.c
> @@ -1627,7 +1627,7 @@ int main(int argc, char *argv[])
> args.protocol = pe->p_proto;
> } else {
> if (str_to_uint(optarg, 0, 0xffff, &tmp) != 0) {
> - fprintf(stderr, "Invalid potocol\n");
> + fprintf(stderr, "Invalid protocol\n");
> return 1;
> }
> args.protocol = tmp;
>
Assuming this will go through net tree
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH bpf-next 1/3] selftests/bpf: test_progs: switch to open_memstream
From: Stanislav Fomichev @ 2019-08-05 15:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Stanislav Fomichev, netdev@vger.kernel.org,
bpf@vger.kernel.org, davem@davemloft.net, ast@kernel.org,
daniel@iogearbox.net
In-Reply-To: <CAEf4BzYV31v6ch-k+ZCQr1RaBuGComt9C0dQjFV1Es42qXz-8Q@mail.gmail.com>
On 08/02, Andrii Nakryiko wrote:
> On Fri, Aug 2, 2019 at 1:14 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 08/02, Andrii Nakryiko wrote:
> > > On 8/2/19 10:17 AM, Stanislav Fomichev wrote:
> > > > Use open_memstream to override stdout during test execution.
> > > > The copy of the original stdout is held in env.stdout and used
> > > > to print subtest info and dump failed log.
> > >
> > > I really like the idea. I didn't know about open_memstream, it's awesome. Thanks!
> > One possible downside of using open_memstream is that it's glibc
> > specific. I probably need to wrap it in #ifdef __GLIBC__ to make
> > it work with other libcs and just print everything as it was before :-(.
> > I'm not sure we care though.
>
> Given this is selftests/bpf, it is probably OK.
>
> >
> > > > test_{v,}printf are now simple wrappers around stdout and will be
> > > > removed in the next patch.
> > > >
> > > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ---
> > > > tools/testing/selftests/bpf/test_progs.c | 100 ++++++++++-------------
> > > > tools/testing/selftests/bpf/test_progs.h | 2 +-
> > > > 2 files changed, 46 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > > index db00196c8315..00d1565d01a3 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > > @@ -40,14 +40,22 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
> > > >
> > > > static void dump_test_log(const struct prog_test_def *test, bool failed)
> > > > {
> > > > - if (env.verbose || test->force_log || failed) {
> > > > - if (env.log_cnt) {
> > > > - fprintf(stdout, "%s", env.log_buf);
> > > > - if (env.log_buf[env.log_cnt - 1] != '\n')
> > > > - fprintf(stdout, "\n");
> > > > + if (stdout == env.stdout)
> > > > + return;
> > > > +
> > > > + fflush(stdout); /* exports env.log_buf & env.log_cap */
> > > > +
> > > > + if (env.log_cap && (env.verbose || test->force_log || failed)) {
> > > > + int len = strlen(env.log_buf);
> > >
> > > env.log_cap is not really a capacity, it's actual number of bytes (without terminating zero), so there is no need to do strlen and it's probably better to rename env.log_cap into env.log_cnt.
> > I'll rename it to log_size to match open_memstream args.
> > We probably still need to do strlen because open_memstream can allocate
> > bigger buffer to hold the data.
>
> If I read man page correctly, env.log_cnt will be exactly the value
> that strlen will return - number of actual bytes written (omitting
> terminal zero), not number of pre-allocated bytes, thus I'm saying
> that strlen is redundant. Please take a look again.
Yeah, you're right, I've played with it a bit and it does return the
length of the data, not the (possibly bigger) size of the buffer.
Will fix in a v3 and use log_cnt.
> >
> > > > +
> > > > + if (len) {
> > > > + fprintf(env.stdout, "%s", env.log_buf);
> > > > + if (env.log_buf[len - 1] != '\n')
> > > > + fprintf(env.stdout, "\n");
> > > > +
> > > > + fseeko(stdout, 0, SEEK_SET);
> > > Same bug as I already fixed with env.log_cnt = 0 being inside this if. You want to do seek always, not just when you print output log.
> > SG, will move to where we currently clear log_cnt, thanks!
> >
> > > > /* rewind */
> > > > }
> > > > }
> > > > - env.log_cnt = 0;
> > > > }
> > > >
> > > > void test__end_subtest()
> > > > @@ -62,7 +70,7 @@ void test__end_subtest()
> > > >
> > > > dump_test_log(test, sub_error_cnt);
> > > >
> > > > - printf("#%d/%d %s:%s\n",
> > > > + fprintf(env.stdout, "#%d/%d %s:%s\n",
> > > > test->test_num, test->subtest_num,
> > > > test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
> > > > }
> > > > @@ -100,53 +108,7 @@ void test__force_log() {
> > > >
> > > > void test__vprintf(const char *fmt, va_list args)
> > > > {
> > > > - size_t rem_sz;
> > > > - int ret = 0;
> > > > -
> > > > - if (env.verbose || (env.test && env.test->force_log)) {
> > > > - vfprintf(stderr, fmt, args);
> > > > - return;
> > > > - }
> > > > -
> > > > -try_again:
> > > > - rem_sz = env.log_cap - env.log_cnt;
> > > > - if (rem_sz) {
> > > > - va_list ap;
> > > > -
> > > > - va_copy(ap, args);
> > > > - /* we reserved extra byte for \0 at the end */
> > > > - ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz + 1, fmt, ap);
> > > > - va_end(ap);
> > > > -
> > > > - if (ret < 0) {
> > > > - env.log_buf[env.log_cnt] = '\0';
> > > > - fprintf(stderr, "failed to log w/ fmt '%s'\n", fmt);
> > > > - return;
> > > > - }
> > > > - }
> > > > -
> > > > - if (!rem_sz || ret > rem_sz) {
> > > > - size_t new_sz = env.log_cap * 3 / 2;
> > > > - char *new_buf;
> > > > -
> > > > - if (new_sz < 4096)
> > > > - new_sz = 4096;
> > > > - if (new_sz < ret + env.log_cnt)
> > > > - new_sz = ret + env.log_cnt;
> > > > -
> > > > - /* +1 for guaranteed space for terminating \0 */
> > > > - new_buf = realloc(env.log_buf, new_sz + 1);
> > > > - if (!new_buf) {
> > > > - fprintf(stderr, "failed to realloc log buffer: %d\n",
> > > > - errno);
> > > > - return;
> > > > - }
> > > > - env.log_buf = new_buf;
> > > > - env.log_cap = new_sz;
> > > > - goto try_again;
> > > > - }
> > > > -
> > > > - env.log_cnt += ret;
> > > > + vprintf(fmt, args);
> > > > }
> > > >
> > > > void test__printf(const char *fmt, ...)
> > > > @@ -477,6 +439,32 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > > > return 0;
> > > > }
> > > >
> > > > +static void stdout_hijack(void)
> > > > +{
> > > > + if (env.verbose || (env.test && env.test->force_log)) {
> > > > + /* nothing to do, output to stdout by default */
> > > > + return;
> > > > + }
> > > > +
> > > > + /* stdout -> buffer */
> > > > + fflush(stdout);
> > > > + stdout = open_memstream(&env.log_buf, &env.log_cap);
> > > Check errors and restore original stdout if something went wrong? (And emit some warning to stderr).
> > Good point, will do.
> >
> > > > +}
> > > > +
> > > > +static void stdout_restore(void)
> > > > +{
> > > > + if (stdout == env.stdout)
> > > > + return;
> > > > +
> > > > + fclose(stdout);
> > > > + free(env.log_buf);
> > > > +
> > > > + env.log_buf = NULL;
> > > > + env.log_cap = 0;
> > > > +
> > > > + stdout = env.stdout;
> > > > +}
> > > > +
> > > > int main(int argc, char **argv)
> > > > {
> > > > static const struct argp argp = {
> > > > @@ -495,6 +483,7 @@ int main(int argc, char **argv)
> > > > srand(time(NULL));
> > > >
> > > > env.jit_enabled = is_jit_enabled();
> > > > + env.stdout = stdout;
> > > >
> > > > for (i = 0; i < prog_test_cnt; i++) {
> > > > struct prog_test_def *test = &prog_test_defs[i];
> > > > @@ -508,6 +497,7 @@ int main(int argc, char **argv)
> > > > test->test_num, test->test_name))
> > > > continue;
> > > >
> > > > + stdout_hijack();
> > > Why do you do this for every test? Just do once before all the tests and restore after?
> > We can do that, my thinking was to limit the area of hijacking :-)
>
> But why? We actually want to hijack stdout/stderr for entire duration
> of all the tests. If test_progs needs some "infrastructural" mandatory
> output, we have env.stdout/env.stderr for that.
Oh, I agree, I've done just that for v2 that's already out.
I was just trying to justify my initial thinking :-)
> > But that would work as well, less allocations per test, I guess. Will
> > do.
> >
> > > > test->run_test();
> > > > /* ensure last sub-test is finalized properly */
> > > > if (test->subtest_name)
> > > > @@ -522,6 +512,7 @@ int main(int argc, char **argv)
> > > > env.succ_cnt++;
> > > >
> > > > dump_test_log(test, test->error_cnt);
> > > > + stdout_restore();
> > > >
> > > > printf("#%d %s:%s\n", test->test_num, test->test_name,
> > > > test->error_cnt ? "FAIL" : "OK");
> > > > @@ -529,7 +520,6 @@ int main(int argc, char **argv)
> > > > printf("Summary: %d/%d PASSED, %d FAILED\n",
> > > > env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> > > >
> > > > - free(env.log_buf);
> > > > free(env.test_selector.num_set);
> > > > free(env.subtest_selector.num_set);
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > > index afd14962456f..9fd89078494f 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > > @@ -56,8 +56,8 @@ struct test_env {
> > > > bool jit_enabled;
> > > >
> > > > struct prog_test_def *test;
> > > > + FILE *stdout;
> > > > char *log_buf;
> > > > - size_t log_cnt;
> > > > size_t log_cap;
> > > So it's actually log_cnt that's assigned on fflush for memstream, according to man page, so probably keep log_cnt, delete log_cap.
> > Ack. See above, will rename to log_size, let me know if you disagree.
> >
> > > > int succ_cnt; /* successful tests */
^ permalink raw reply
* Re: [PATCH 11/16] net: phy: adin: PHY reset mechanisms
From: Andrew Lunn @ 2019-08-05 15:15 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-12-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:48PM +0300, Alexandru Ardelean wrote:
> The ADIN PHYs supports 4 types of reset:
> 1. The standard PHY reset via BMCR_RESET bit in MII_BMCR reg
> 2. Reset via GPIO
> 3. Reset via reg GeSftRst (0xff0c) & reload previous pin configs
> 4. Reset via reg GeSftRst (0xff0c) & request new pin configs
>
> Resets 2 & 4 are almost identical, with the exception that the crystal
> oscillator is available during reset for 2.
>
> Resetting via GeSftRst or via GPIO is useful when doing a warm reboot. If
> doing various settings via phytool or ethtool, the sub-system registers
> don't reset just via BMCR_RESET.
>
> This change implements resetting the entire PHY subsystem during probe.
> During PHY HW init (phy_hw_init() logic) the PHY core regs will be reset
> again via BMCR_RESET. This will also need to happen during a PM resume.
phylib already has support for GPIO reset. So if possible, you should
not repeat that code here.
What is the difference between a GPIO reset, and a GPIO reset followed
by a subsystem soft reset?
Andrew
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox