The code in sja1105_mdio.c does the same thing as the one added by Serge
Semin in drivers/net/pcs/pcs-xpcs-plat.c (implements a virtual MDIO bus,
backed by either a direct or an indirect register access method), except
the latter is generic after the conversion to regmap.
Except for just one problem: pcs-xpcs-plat.c expects to probe on a
device tree node, and the SJA1105 and SJA1110 doesn't describe its XPCS
on the SGMII ports in the device tree.
This is both a problem and a design intention. I've long held the view
that for SPI devices which don't have a common .dtsi but whose bindings
have to be written many times by many people for many boards, less is
more, and all the details like internal subdevices can probably stay
hidden even if those devices are being configured by the kernel.
I've also held the view that DSA should delegate its responsibility for
configuring non-essential subdevices to other drivers in their
respective subsystems, and that the switching IP core (the one with the
DSA driver) should merely be one of the MFD children of a MFD parent
that is in charge of the spi_device.
This would mean something like the "mscc,vsc7512" example from
Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml, but:
- retrofitting that model onto the SJA1105 bindings (and any other
switch on a non-MMIO bus, where the ethernet-switch node coincides
with the SPI/I2C bus device node) is a huge pain for backwards and
especially forward compatibility, as it implies that the switch can be
either a spi_device or a platform_device after the conversion
- if it sounds like it is conflicting with the first requirement of
"less is more", yes it is
I've tried various ways of using the xpcs-plat driver while
avoiding major DT bindings changes for this switch, like
fwnode_create_software_node() and custom platform data.
Platform data was ugly and software nodes didn't work at all, for
reasons explained here:
https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
I have to give huge credits to Andy Shevchenko, who after more than one
year remembered the discussion and referenced Hervé Codina's work on PCI
DT overlays, as well as a presentation from Lizhi Hou and Rob Herring.
I think I found the compromise solution that allows me to make progress,
which is to create a dynamic OF changeset that attaches the PCS node to
the live device tree, if it wasn't described already in the DTS, or use
the one from the DTS if it's already there. With a proper OF node, the
xpcs-plat driver probes just fine.
As for where to attach the XPCS node? Memory-mapped devices (in the
SPI address space, mind you) don't naturally sit well in the
"ethernet-switch" node, because that has #address-cells = <0>
and #size-cells = <0>.
We can't modify #address-cells and #size-cells, because "ethernet-switch"
has a reg-less "ethernet-ports" child node as per dsa.yaml, and the PCS
would sit on the same hierarchical level as that. Essentially this is
another angle of the argument that the DSA OF node shouldn't coincide
with the SPI bus device node, as this implies it is in control of the
entire address space.
The compromise, retrofit-ready solution here is to create a "regs"
container node which is a child of the ethernet-switch and
has #address-cells = <1> and #size-cells = <1>, then attach the XPCS to
that.
There also exists a use case where the XPCS is manually described
in the device tree, and that is when we need to describe SGMII lane
polarity inversion (not yet supported, TBD). In that case,
sja1105_fill_device_tree() simply backs off for the already present PCS
nodes, and sja1105_mfd_add_pcs_cells() works all the same.
A small implementation note in sja1105_create_pcs():
xpcs_create_fwnode() is NULL-tolerant via fwnode_device_is_available(),
so we don't need to explicitly handle the case where
priv->pcs_fwnode[port] wasn't assigned by sja1105_create_pcs_nodes()
(because the OF node exists in the DTS). This case currently returns
-ENODEV and will be handled by the next change.
Cc: Serge Semin <fancer.lancer@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Herve Codina <herve.codina@bootlin.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/Kconfig | 1 +
drivers/net/dsa/sja1105/Makefile | 1 -
drivers/net/dsa/sja1105/sja1105.h | 29 ++-
drivers/net/dsa/sja1105/sja1105_main.c | 82 +++++++--
drivers/net/dsa/sja1105/sja1105_mdio.c | 233 -------------------------
drivers/net/dsa/sja1105/sja1105_mfd.c | 228 +++++++++++++++++++++++-
drivers/net/dsa/sja1105/sja1105_mfd.h | 2 +
drivers/net/dsa/sja1105/sja1105_spi.c | 49 ++++--
8 files changed, 342 insertions(+), 283 deletions(-)
delete mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c
diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
index 932bca545d69..eef06e419559 100644
--- a/drivers/net/dsa/sja1105/Kconfig
+++ b/drivers/net/dsa/sja1105/Kconfig
@@ -8,6 +8,7 @@ tristate "NXP SJA1105 Ethernet switch family support"
select PACKING
select CRC32
select MFD_CORE
+ select OF_DYNAMIC
help
This is the driver for the NXP SJA1105 (5-port) and SJA1110 (10-port)
automotive Ethernet switch family. These are managed over an SPI
diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
index 3ac2d77dbe6c..94907116c1cc 100644
--- a/drivers/net/dsa/sja1105/Makefile
+++ b/drivers/net/dsa/sja1105/Makefile
@@ -4,7 +4,6 @@ obj-$(CONFIG_NET_DSA_SJA1105) += sja1105.o
sja1105-objs := \
sja1105_spi.o \
sja1105_main.o \
- sja1105_mdio.o \
sja1105_mfd.o \
sja1105_flower.o \
sja1105_ethtool.o \
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 22fce143cb76..96954f1f5bcf 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -44,6 +44,8 @@
#define SJA1105_RGMII_DELAY_MAX_PS \
SJA1105_RGMII_DELAY_PHASE_TO_PS(1017)
+#define SJA1105_SGMII_PORT 4
+
typedef enum {
SPI_READ = 0,
SPI_WRITE = 1,
@@ -91,7 +93,6 @@ struct sja1105_regs {
u64 rmii_ref_clk[SJA1105_MAX_NUM_PORTS];
u64 rmii_ext_tx_clk[SJA1105_MAX_NUM_PORTS];
u64 stats[__MAX_SJA1105_STATS_AREA][SJA1105_MAX_NUM_PORTS];
- u64 pcs_base[SJA1105_MAX_NUM_PORTS];
};
enum {
@@ -109,6 +110,13 @@ enum sja1105_internal_phy_t {
SJA1105_PHY_BASE_T1,
};
+struct sja1105_pcs_resource {
+ struct resource res;
+ int port;
+ const char *cell_name;
+ const char *compatible;
+};
+
struct sja1105_info {
u64 device_id;
/* Needed for distinction between P and R, and between Q and S
@@ -148,10 +156,6 @@ struct sja1105_info {
bool (*rxtstamp)(struct dsa_switch *ds, int port, struct sk_buff *skb);
void (*txtstamp)(struct dsa_switch *ds, int port, struct sk_buff *skb);
int (*clocking_setup)(struct sja1105_private *priv);
- int (*pcs_mdio_read_c45)(struct mii_bus *bus, int phy, int mmd,
- int reg);
- int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd,
- int reg, u16 val);
int (*disable_microcontroller)(struct sja1105_private *priv);
const char *name;
bool supports_mii[SJA1105_MAX_NUM_PORTS];
@@ -161,6 +165,8 @@ struct sja1105_info {
bool supports_2500basex[SJA1105_MAX_NUM_PORTS];
enum sja1105_internal_phy_t internal_phy[SJA1105_MAX_NUM_PORTS];
const u64 port_speed[SJA1105_SPEED_MAX];
+ const struct sja1105_pcs_resource *pcs_resources;
+ size_t num_pcs_resources;
};
enum sja1105_key_type {
@@ -273,8 +279,9 @@ struct sja1105_private {
struct regmap *regmap;
struct devlink_region **regions;
struct sja1105_cbs_entry *cbs;
- struct mii_bus *mdio_pcs;
struct phylink_pcs *pcs[SJA1105_MAX_NUM_PORTS];
+ struct fwnode_handle *pcs_fwnode[SJA1105_MAX_NUM_PORTS];
+ struct of_changeset of_cs;
struct sja1105_ptp_data ptp_data;
struct sja1105_tas_data tas_data;
};
@@ -302,16 +309,6 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
struct netlink_ext_ack *extack);
void sja1105_frame_memory_partitioning(struct sja1105_private *priv);
-/* From sja1105_mdio.c */
-int sja1105_mdiobus_register(struct dsa_switch *ds);
-void sja1105_mdiobus_unregister(struct dsa_switch *ds);
-int sja1105_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg);
-int sja1105_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
- u16 val);
-int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg);
-int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
- u16 val);
-
/* From sja1105_devlink.c */
int sja1105_devlink_setup(struct dsa_switch *ds);
void sja1105_devlink_teardown(struct dsa_switch *ds);
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 6da5c655dae7..70aecdf9fd0e 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -15,6 +15,7 @@
#include <linux/of.h>
#include <linux/of_net.h>
#include <linux/of_mdio.h>
+#include <linux/pcs/pcs-xpcs.h>
#include <linux/netdev_features.h>
#include <linux/netdevice.h>
#include <linux/if_bridge.h>
@@ -3032,6 +3033,44 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
return 0;
}
+static int sja1105_create_pcs(struct dsa_switch *ds, int port)
+{
+ struct sja1105_private *priv = ds->priv;
+ struct phylink_pcs *pcs;
+
+ if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
+ priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
+ return 0;
+
+ pcs = xpcs_create_pcs_fwnode(priv->pcs_fwnode[port]);
+ if (IS_ERR(pcs))
+ return PTR_ERR(pcs);
+
+ priv->pcs[port] = pcs;
+
+ return 0;
+}
+
+static void sja1105_destroy_pcs(struct dsa_switch *ds, int port)
+{
+ struct sja1105_private *priv = ds->priv;
+
+ if (priv->pcs[port]) {
+ xpcs_destroy_pcs(priv->pcs[port]);
+ priv->pcs[port] = NULL;
+ }
+}
+
+static int sja1105_port_setup(struct dsa_switch *ds, int port)
+{
+ return sja1105_create_pcs(ds, port);
+}
+
+static void sja1105_port_teardown(struct dsa_switch *ds, int port)
+{
+ sja1105_destroy_pcs(ds, port);
+}
+
/* The programming model for the SJA1105 switch is "all-at-once" via static
* configuration tables. Some of these can be dynamically modified at runtime,
* but not the xMII mode parameters table.
@@ -3086,16 +3125,9 @@ static int sja1105_setup(struct dsa_switch *ds)
goto out_flower_teardown;
}
- rc = sja1105_mdiobus_register(ds);
- if (rc < 0) {
- dev_err(ds->dev, "Failed to register MDIO bus: %pe\n",
- ERR_PTR(rc));
- goto out_ptp_clock_unregister;
- }
-
rc = sja1105_devlink_setup(ds);
if (rc < 0)
- goto out_mdiobus_unregister;
+ goto out_ptp_clock_unregister;
rtnl_lock();
rc = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
@@ -3125,8 +3157,6 @@ static int sja1105_setup(struct dsa_switch *ds)
out_devlink_teardown:
sja1105_devlink_teardown(ds);
-out_mdiobus_unregister:
- sja1105_mdiobus_unregister(ds);
out_ptp_clock_unregister:
sja1105_ptp_clock_unregister(ds);
out_flower_teardown:
@@ -3147,7 +3177,6 @@ static void sja1105_teardown(struct dsa_switch *ds)
rtnl_unlock();
sja1105_devlink_teardown(ds);
- sja1105_mdiobus_unregister(ds);
sja1105_ptp_clock_unregister(ds);
sja1105_flower_teardown(ds);
sja1105_tas_teardown(ds);
@@ -3166,6 +3195,8 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
.connect_tag_protocol = sja1105_connect_tag_protocol,
.setup = sja1105_setup,
.teardown = sja1105_teardown,
+ .port_setup = sja1105_port_setup,
+ .port_teardown = sja1105_port_teardown,
.set_ageing_time = sja1105_set_ageing_time,
.port_change_mtu = sja1105_change_mtu,
.port_max_mtu = sja1105_get_max_mtu,
@@ -3362,32 +3393,51 @@ static int sja1105_probe(struct spi_device *spi)
return rc;
}
+ rc = sja1105_fill_device_tree(ds);
+ if (rc) {
+ dev_err(ds->dev, "Failed to fill device tree: %pe\n",
+ ERR_PTR(rc));
+ return rc;
+ }
+
rc = sja1105_mfd_add_devices(ds);
if (rc) {
dev_err(ds->dev, "Failed to create child devices: %pe\n",
ERR_PTR(rc));
- return rc;
+ goto restore_device_tree;
}
if (IS_ENABLED(CONFIG_NET_SCH_CBS)) {
priv->cbs = devm_kcalloc(dev, priv->info->num_cbs_shapers,
sizeof(struct sja1105_cbs_entry),
GFP_KERNEL);
- if (!priv->cbs)
- return -ENOMEM;
+ if (!priv->cbs) {
+ rc = -ENOMEM;
+ goto restore_device_tree;
+ }
}
- return dsa_register_switch(priv->ds);
+ rc = dsa_register_switch(priv->ds);
+ if (rc)
+ goto restore_device_tree;
+
+ return 0;
+
+restore_device_tree:
+ sja1105_restore_device_tree(ds);
+ return rc;
}
static void sja1105_remove(struct spi_device *spi)
{
struct sja1105_private *priv = spi_get_drvdata(spi);
+ struct dsa_switch *ds = priv->ds;
if (!priv)
return;
- dsa_unregister_switch(priv->ds);
+ dsa_unregister_switch(ds);
+ sja1105_restore_device_tree(ds);
}
static void sja1105_shutdown(struct spi_device *spi)
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
deleted file mode 100644
index d5577c702902..000000000000
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ /dev/null
@@ -1,233 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright 2021 NXP
- */
-#include <linux/pcs/pcs-xpcs.h>
-#include <linux/of_mdio.h>
-#include "sja1105.h"
-
-#define SJA1110_PCS_BANK_REG SJA1110_SPI_ADDR(0x3fc)
-
-int sja1105_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg)
-{
- struct sja1105_private *priv = bus->priv;
- u64 addr;
- u32 tmp;
- int rc;
-
- addr = (mmd << 16) | reg;
-
- if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
- return 0xffff;
-
- if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID1)
- return NXP_SJA1105_XPCS_ID >> 16;
- if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID2)
- return NXP_SJA1105_XPCS_ID & GENMASK(15, 0);
-
- rc = sja1105_xfer_u32(priv, SPI_READ, addr, &tmp, NULL);
- if (rc < 0)
- return rc;
-
- return tmp & 0xffff;
-}
-
-int sja1105_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd,
- int reg, u16 val)
-{
- struct sja1105_private *priv = bus->priv;
- u64 addr;
- u32 tmp;
-
- addr = (mmd << 16) | reg;
- tmp = val;
-
- if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
- return -EINVAL;
-
- return sja1105_xfer_u32(priv, SPI_WRITE, addr, &tmp, NULL);
-}
-
-int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg)
-{
- struct sja1105_private *priv = bus->priv;
- const struct sja1105_regs *regs = priv->info->regs;
- int offset, bank;
- u64 addr;
- u32 tmp;
- int rc;
-
- if (regs->pcs_base[phy] == SJA1105_RSV_ADDR)
- return -ENODEV;
-
- addr = (mmd << 16) | reg;
-
- if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID1)
- return NXP_SJA1110_XPCS_ID >> 16;
- if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID2)
- return NXP_SJA1110_XPCS_ID & GENMASK(15, 0);
-
- bank = addr >> 8;
- offset = addr & GENMASK(7, 0);
-
- /* This addressing scheme reserves register 0xff for the bank address
- * register, so that can never be addressed.
- */
- if (WARN_ON(offset == 0xff))
- return -ENODEV;
-
- tmp = bank;
-
- rc = sja1105_xfer_u32(priv, SPI_WRITE,
- regs->pcs_base[phy] + SJA1110_PCS_BANK_REG,
- &tmp, NULL);
- if (rc < 0)
- return rc;
-
- rc = sja1105_xfer_u32(priv, SPI_READ, regs->pcs_base[phy] + offset,
- &tmp, NULL);
- if (rc < 0)
- return rc;
-
- return tmp & 0xffff;
-}
-
-int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
- u16 val)
-{
- struct sja1105_private *priv = bus->priv;
- const struct sja1105_regs *regs = priv->info->regs;
- int offset, bank;
- u64 addr;
- u32 tmp;
- int rc;
-
- if (regs->pcs_base[phy] == SJA1105_RSV_ADDR)
- return -ENODEV;
-
- addr = (mmd << 16) | reg;
-
- bank = addr >> 8;
- offset = addr & GENMASK(7, 0);
-
- /* This addressing scheme reserves register 0xff for the bank address
- * register, so that can never be addressed.
- */
- if (WARN_ON(offset == 0xff))
- return -ENODEV;
-
- tmp = bank;
-
- rc = sja1105_xfer_u32(priv, SPI_WRITE,
- regs->pcs_base[phy] + SJA1110_PCS_BANK_REG,
- &tmp, NULL);
- if (rc < 0)
- return rc;
-
- tmp = val;
-
- return sja1105_xfer_u32(priv, SPI_WRITE, regs->pcs_base[phy] + offset,
- &tmp, NULL);
-}
-
-static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
-{
- struct dsa_switch *ds = priv->ds;
- struct mii_bus *bus;
- int rc = 0;
- int port;
-
- if (!priv->info->pcs_mdio_read_c45 || !priv->info->pcs_mdio_write_c45)
- return 0;
-
- bus = mdiobus_alloc_size(0);
- if (!bus)
- return -ENOMEM;
-
- bus->name = "SJA1105 PCS MDIO bus";
- snprintf(bus->id, MII_BUS_ID_SIZE, "%s-pcs",
- dev_name(ds->dev));
- bus->read_c45 = priv->info->pcs_mdio_read_c45;
- bus->write_c45 = priv->info->pcs_mdio_write_c45;
- bus->parent = ds->dev;
- /* There is no PHY on this MDIO bus => mask out all PHY addresses
- * from auto probing.
- */
- bus->phy_mask = ~0;
- bus->priv = priv;
-
- rc = mdiobus_register(bus);
- if (rc) {
- mdiobus_free(bus);
- return rc;
- }
-
- for (port = 0; port < ds->num_ports; port++) {
- struct phylink_pcs *pcs;
-
- if (dsa_is_unused_port(ds, port))
- continue;
-
- if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
- priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
- continue;
-
- pcs = xpcs_create_pcs_mdiodev(bus, port);
- if (IS_ERR(pcs)) {
- rc = PTR_ERR(pcs);
- goto out_pcs_free;
- }
-
- priv->pcs[port] = pcs;
- }
-
- priv->mdio_pcs = bus;
-
- return 0;
-
-out_pcs_free:
- for (port = 0; port < ds->num_ports; port++) {
- if (priv->pcs[port]) {
- xpcs_destroy_pcs(priv->pcs[port]);
- priv->pcs[port] = NULL;
- }
- }
-
- mdiobus_unregister(bus);
- mdiobus_free(bus);
-
- return rc;
-}
-
-static void sja1105_mdiobus_pcs_unregister(struct sja1105_private *priv)
-{
- struct dsa_switch *ds = priv->ds;
- int port;
-
- if (!priv->mdio_pcs)
- return;
-
- for (port = 0; port < ds->num_ports; port++) {
- if (priv->pcs[port]) {
- xpcs_destroy_pcs(priv->pcs[port]);
- priv->pcs[port] = NULL;
- }
- }
-
- mdiobus_unregister(priv->mdio_pcs);
- mdiobus_free(priv->mdio_pcs);
- priv->mdio_pcs = NULL;
-}
-
-int sja1105_mdiobus_register(struct dsa_switch *ds)
-{
- struct sja1105_private *priv = ds->priv;
-
- return sja1105_mdiobus_pcs_register(priv);
-}
-
-void sja1105_mdiobus_unregister(struct dsa_switch *ds)
-{
- struct sja1105_private *priv = ds->priv;
-
- sja1105_mdiobus_pcs_unregister(priv);
-}
diff --git a/drivers/net/dsa/sja1105/sja1105_mfd.c b/drivers/net/dsa/sja1105/sja1105_mfd.c
index 9e60cd3b5d01..7785e7d33c3d 100644
--- a/drivers/net/dsa/sja1105/sja1105_mfd.c
+++ b/drivers/net/dsa/sja1105/sja1105_mfd.c
@@ -7,12 +7,40 @@
#include "sja1105.h"
#include "sja1105_mfd.h"
+#define SJA1105_MAX_NUM_MDIOS 2
+#define SJA1105_MAX_NUM_PCS 4
+#define SJA1105_MAX_NUM_CELLS (SJA1105_MAX_NUM_MDIOS + \
+ SJA1105_MAX_NUM_PCS + \
+ 1) /* sentinel */
+
static const struct resource sja1110_mdio_cbtx_res =
DEFINE_RES_REG_NAMED(0x709000, 0x1000, "mdio_cbtx");
static const struct resource sja1110_mdio_cbt1_res =
DEFINE_RES_REG_NAMED(0x704000, 0x4000, "mdio_cbt1");
+static void sja1105_mfd_add_pcs_cells(struct sja1105_private *priv,
+ struct device_node *regs_node,
+ struct mfd_cell *cells,
+ int *num_cells)
+{
+ for (int i = 0; i < priv->info->num_pcs_resources; i++) {
+ const struct sja1105_pcs_resource *pcs_res;
+
+ pcs_res = &priv->info->pcs_resources[i];
+
+ cells[(*num_cells)++] = (struct mfd_cell) {
+ .name = pcs_res->cell_name,
+ .of_compatible = pcs_res->compatible,
+ .of_reg = pcs_res->res.start,
+ .use_of_reg = true,
+ .resources = &pcs_res->res,
+ .num_resources = 1,
+ .parent_of_node = regs_node,
+ };
+ }
+}
+
static void sja1105_mfd_add_mdio_cells(struct sja1105_private *priv,
struct device_node *mdio_node,
struct mfd_cell *cells,
@@ -50,12 +78,16 @@ static void sja1105_mfd_add_mdio_cells(struct sja1105_private *priv,
int sja1105_mfd_add_devices(struct dsa_switch *ds)
{
struct device_node *switch_node = dev_of_node(ds->dev);
+ struct mfd_cell cells[SJA1105_MAX_NUM_CELLS] = {};
+ struct device_node *regs_node, *mdio_node;
struct sja1105_private *priv = ds->priv;
- struct device_node *mdio_node;
- struct mfd_cell cells[2] = {};
int num_cells = 0;
int rc = 0;
+ regs_node = of_get_available_child_by_name(switch_node, "regs");
+ if (regs_node)
+ sja1105_mfd_add_pcs_cells(priv, regs_node, cells, &num_cells);
+
mdio_node = of_get_available_child_by_name(switch_node, "mdios");
if (mdio_node)
sja1105_mfd_add_mdio_cells(priv, mdio_node, cells, &num_cells);
@@ -64,6 +96,198 @@ int sja1105_mfd_add_devices(struct dsa_switch *ds)
rc = devm_mfd_add_devices(ds->dev, PLATFORM_DEVID_AUTO, cells,
num_cells, NULL, 0, NULL);
+ of_node_put(regs_node);
of_node_put(mdio_node);
return rc;
}
+
+static bool sja1105_child_node_exists(struct device_node *node,
+ const char *name,
+ const struct resource *res)
+{
+ struct device_node *child = of_get_child_by_name(node, name);
+ u32 reg[2];
+
+ for_each_child_of_node(node, child) {
+ if (!of_node_name_eq(child, name))
+ continue;
+
+ if (of_property_read_u32_array(child, "reg", reg, ARRAY_SIZE(reg)))
+ continue;
+
+ if (reg[0] == res->start && reg[1] == resource_size(res))
+ return true;
+ }
+
+ return false;
+}
+
+static int sja1105_create_pcs_nodes(struct sja1105_private *priv,
+ struct device_node *regs_node)
+{
+ struct dsa_switch *ds = priv->ds;
+ struct device *dev = ds->dev;
+ struct device_node *pcs_node;
+ const u32 reg_io_width = 4;
+ char node_name[32];
+ u32 reg_props[2];
+ int rc;
+
+ for (int i = 0; i < priv->info->num_pcs_resources; i++) {
+ const struct sja1105_pcs_resource *pcs_res;
+
+ pcs_res = &priv->info->pcs_resources[i];
+
+ if (sja1105_child_node_exists(regs_node, "ethernet-pcs",
+ &pcs_res->res))
+ continue;
+
+ snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
+ pcs_res->res.start);
+
+ pcs_node = of_changeset_create_node(&priv->of_cs, regs_node,
+ node_name);
+ if (!pcs_node) {
+ dev_err(dev, "Failed to create PCS node %s\n", node_name);
+ return -ENOMEM;
+ }
+
+ rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
+ "compatible",
+ pcs_res->compatible);
+ if (rc) {
+ dev_err(dev, "Failed to add compatible property to %s: %pe\n",
+ node_name, ERR_PTR(rc));
+ return rc;
+ }
+
+ reg_props[0] = pcs_res->res.start;
+ reg_props[1] = resource_size(&pcs_res->res);
+ rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
+ "reg", reg_props, 2);
+ if (rc) {
+ dev_err(dev, "Failed to add reg property to %s: %pe\n",
+ node_name, ERR_PTR(rc));
+ return rc;
+ }
+
+ rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
+ "reg-names",
+ pcs_res->res.name);
+ if (rc) {
+ dev_err(dev, "Failed to add reg-names property to %s: %pe\n",
+ node_name, ERR_PTR(rc));
+ return rc;
+ }
+
+ rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
+ "reg-io-width",
+ ®_io_width, 1);
+ if (rc) {
+ dev_err(dev, "Failed to add reg-io-width property to %s: %pe\n",
+ node_name, ERR_PTR(rc));
+ return rc;
+ }
+
+ dev_dbg(dev, "Created OF node %pOF\n", pcs_node);
+ priv->pcs_fwnode[pcs_res->port] = of_fwnode_handle(pcs_node);
+ }
+
+ return 0;
+}
+
+static struct device_node *sja1105_create_regs_node(struct sja1105_private *priv,
+ struct device_node *switch_node)
+{
+ struct device *dev = priv->ds->dev;
+ struct device_node *regs_node;
+ const u32 addr_size_cells = 1;
+ int rc;
+
+ regs_node = of_changeset_create_node(&priv->of_cs, switch_node, "regs");
+ if (!regs_node) {
+ dev_err(dev, "Failed to create 'regs' device tree node\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ rc = of_changeset_add_prop_u32_array(&priv->of_cs, regs_node,
+ "#address-cells",
+ &addr_size_cells, 1);
+ if (rc) {
+ dev_err(dev, "Failed to add #address-cells property: %pe\n",
+ ERR_PTR(rc));
+ return ERR_PTR(rc);
+ }
+
+ rc = of_changeset_add_prop_u32_array(&priv->of_cs, regs_node,
+ "#size-cells",
+ &addr_size_cells, 1);
+ if (rc) {
+ dev_err(dev, "Failed to add #size-cells property: %pe\n",
+ ERR_PTR(rc));
+ return ERR_PTR(rc);
+ }
+
+ return regs_node;
+}
+
+int sja1105_fill_device_tree(struct dsa_switch *ds)
+{
+ struct device_node *switch_node, *regs_node;
+ struct sja1105_private *priv = ds->priv;
+ bool regs_node_created = false;
+ struct device *dev = ds->dev;
+ int rc;
+
+ switch_node = dev_of_node(dev);
+ of_changeset_init(&priv->of_cs);
+
+ regs_node = of_get_child_by_name(switch_node, "regs");
+ if (!regs_node) {
+ regs_node = sja1105_create_regs_node(priv, switch_node);
+ if (IS_ERR(regs_node)) {
+ rc = PTR_ERR(regs_node);
+ goto out_destroy_changeset;
+ }
+
+ regs_node_created = true;
+ dev_dbg(dev, "Created OF node %pOF\n", regs_node);
+ }
+
+ rc = sja1105_create_pcs_nodes(priv, regs_node);
+ if (rc)
+ goto out_destroy_changeset;
+
+ rc = of_changeset_apply(&priv->of_cs);
+ if (rc) {
+ dev_err(dev, "Failed to apply device tree changeset: %pe\n",
+ ERR_PTR(rc));
+ goto out_destroy_changeset;
+ }
+
+ /* Don't destroy the changeset - we need it for reverting later */
+ goto out_put_regs_node;
+
+out_destroy_changeset:
+ of_changeset_destroy(&priv->of_cs);
+out_put_regs_node:
+ if (!regs_node_created)
+ of_node_put(regs_node);
+
+ return rc;
+}
+
+void sja1105_restore_device_tree(struct dsa_switch *ds)
+{
+ struct sja1105_private *priv = ds->priv;
+ struct device *dev = ds->dev;
+ int rc;
+
+ rc = of_changeset_revert(&priv->of_cs);
+ if (rc) {
+ dev_err(dev, "Failed to revert device tree changeset: %pe\n",
+ ERR_PTR(rc));
+ }
+
+ of_changeset_destroy(&priv->of_cs);
+}
diff --git a/drivers/net/dsa/sja1105/sja1105_mfd.h b/drivers/net/dsa/sja1105/sja1105_mfd.h
index c33c8ff24e25..7195c3aa1437 100644
--- a/drivers/net/dsa/sja1105/sja1105_mfd.h
+++ b/drivers/net/dsa/sja1105/sja1105_mfd.h
@@ -5,5 +5,7 @@
#define _SJA1105_MFD_H
int sja1105_mfd_add_devices(struct dsa_switch *ds);
+int sja1105_fill_device_tree(struct dsa_switch *ds);
+void sja1105_restore_device_tree(struct dsa_switch *ds);
#endif
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 087acded7827..8af3d01d0f5c 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -617,9 +617,28 @@ static const struct sja1105_regs sja1110_regs = {
.ptpclkrate = SJA1110_SPI_ADDR(0x74),
.ptpclkcorp = SJA1110_SPI_ADDR(0x80),
.ptpsyncts = SJA1110_SPI_ADDR(0x84),
- .pcs_base = {SJA1105_RSV_ADDR, 0x1c1400, 0x1c1800, 0x1c1c00, 0x1c2000,
- SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR,
- SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR},
+};
+
+/* See port compatibility matrix in Documentation/networking/dsa/sja1105.rst */
+static const struct sja1105_pcs_resource sja1105rs_pcs_resources[] = {
+ { DEFINE_RES_REG_NAMED(0x0, 0x800000, "direct"),
+ SJA1105_SGMII_PORT, "sja1105-pcs", "nxp,sja1105-pcs"
+ },
+};
+
+static const struct sja1105_pcs_resource sja1110_pcs_resources[] = {
+ { DEFINE_RES_REG_NAMED(0x705000, 0x1000, "indirect"),
+ 1, "sja1110-pcs", "nxp,sja1110-pcs"
+ },
+ { DEFINE_RES_REG_NAMED(0x706000, 0x1000, "indirect"),
+ 2, "sja1110-pcs", "nxp,sja1110-pcs"
+ },
+ { DEFINE_RES_REG_NAMED(0x707000, 0x1000, "indirect"),
+ 3, "sja1110-pcs", "nxp,sja1110-pcs"
+ },
+ { DEFINE_RES_REG_NAMED(0x708000, 0x1000, "indirect"),
+ 4, "sja1110-pcs", "nxp,sja1110-pcs"
+ },
};
const struct sja1105_info sja1105e_info = {
@@ -771,8 +790,6 @@ const struct sja1105_info sja1105r_info = {
.ptp_cmd_packing = sja1105pqrs_ptp_cmd_packing,
.rxtstamp = sja1105_rxtstamp,
.clocking_setup = sja1105_clocking_setup,
- .pcs_mdio_read_c45 = sja1105_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1105_pcs_mdio_write_c45,
.regs = &sja1105pqrs_regs,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
@@ -785,6 +802,8 @@ const struct sja1105_info sja1105r_info = {
.supports_rmii = {true, true, true, true, true},
.supports_rgmii = {true, true, true, true, true},
.supports_sgmii = {false, false, false, false, true},
+ .pcs_resources = sja1105rs_pcs_resources,
+ .num_pcs_resources = ARRAY_SIZE(sja1105rs_pcs_resources),
.name = "SJA1105R",
};
@@ -808,8 +827,6 @@ const struct sja1105_info sja1105s_info = {
.ptp_cmd_packing = sja1105pqrs_ptp_cmd_packing,
.rxtstamp = sja1105_rxtstamp,
.clocking_setup = sja1105_clocking_setup,
- .pcs_mdio_read_c45 = sja1105_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1105_pcs_mdio_write_c45,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
[SJA1105_SPEED_10MBPS] = 3,
@@ -821,6 +838,8 @@ const struct sja1105_info sja1105s_info = {
.supports_rmii = {true, true, true, true, true},
.supports_rgmii = {true, true, true, true, true},
.supports_sgmii = {false, false, false, false, true},
+ .pcs_resources = sja1105rs_pcs_resources,
+ .num_pcs_resources = ARRAY_SIZE(sja1105rs_pcs_resources),
.name = "SJA1105S",
};
@@ -847,8 +866,6 @@ const struct sja1105_info sja1110a_info = {
.rxtstamp = sja1110_rxtstamp,
.txtstamp = sja1110_txtstamp,
.disable_microcontroller = sja1110_disable_microcontroller,
- .pcs_mdio_read_c45 = sja1110_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1110_pcs_mdio_write_c45,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
[SJA1105_SPEED_10MBPS] = 4,
@@ -872,6 +889,8 @@ const struct sja1105_info sja1110a_info = {
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_PHY_BASE_T1},
+ .pcs_resources = sja1110_pcs_resources,
+ .num_pcs_resources = ARRAY_SIZE(sja1110_pcs_resources),
.name = "SJA1110A",
};
@@ -898,8 +917,6 @@ const struct sja1105_info sja1110b_info = {
.rxtstamp = sja1110_rxtstamp,
.txtstamp = sja1110_txtstamp,
.disable_microcontroller = sja1110_disable_microcontroller,
- .pcs_mdio_read_c45 = sja1110_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1110_pcs_mdio_write_c45,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
[SJA1105_SPEED_10MBPS] = 4,
@@ -923,6 +940,8 @@ const struct sja1105_info sja1110b_info = {
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_NO_PHY},
+ .pcs_resources = &sja1110_pcs_resources[2], /* ports 3 and 4 */
+ .num_pcs_resources = ARRAY_SIZE(sja1110_pcs_resources) - 2,
.name = "SJA1110B",
};
@@ -949,8 +968,6 @@ const struct sja1105_info sja1110c_info = {
.rxtstamp = sja1110_rxtstamp,
.txtstamp = sja1110_txtstamp,
.disable_microcontroller = sja1110_disable_microcontroller,
- .pcs_mdio_read_c45 = sja1110_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1110_pcs_mdio_write_c45,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
[SJA1105_SPEED_10MBPS] = 4,
@@ -974,6 +991,8 @@ const struct sja1105_info sja1110c_info = {
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_NO_PHY, SJA1105_NO_PHY,
SJA1105_NO_PHY},
+ .pcs_resources = &sja1110_pcs_resources[3], /* port 4 */
+ .num_pcs_resources = ARRAY_SIZE(sja1110_pcs_resources) - 3,
.name = "SJA1110C",
};
@@ -1000,8 +1019,6 @@ const struct sja1105_info sja1110d_info = {
.rxtstamp = sja1110_rxtstamp,
.txtstamp = sja1110_txtstamp,
.disable_microcontroller = sja1110_disable_microcontroller,
- .pcs_mdio_read_c45 = sja1110_pcs_mdio_read_c45,
- .pcs_mdio_write_c45 = sja1110_pcs_mdio_write_c45,
.port_speed = {
[SJA1105_SPEED_AUTO] = 0,
[SJA1105_SPEED_10MBPS] = 4,
@@ -1025,5 +1042,7 @@ const struct sja1105_info sja1110d_info = {
SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
SJA1105_NO_PHY, SJA1105_NO_PHY,
SJA1105_NO_PHY},
+ .pcs_resources = sja1110_pcs_resources,
+ .num_pcs_resources = ARRAY_SIZE(sja1110_pcs_resources),
.name = "SJA1110D",
};
--
2.34.1
Hi Vladimir,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-sja1105-let-phylink-help-with-the-replay-of-link-callbacks/20251119-031300
base: net-next/main
patch link: https://lore.kernel.org/r/20251118190530.580267-15-vladimir.oltean%40nxp.com
patch subject: [PATCH net-next 14/15] net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver
config: s390-randconfig-002-20251120 (https://download.01.org/0day-ci/archive/20251120/202511200752.hGkPwqbU-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251120/202511200752.hGkPwqbU-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511200752.hGkPwqbU-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/cpufreq.h:17,
from kernel/sched/sched.h:31,
from kernel/sched/rq-offsets.c:5:
include/linux/of.h: In function 'of_changeset_attach_node':
>> include/linux/of.h:1623:34: error: 'OF_RECONFIG_ATTACH_NODE' undeclared (first use in this function); did you mean 'OF_RECONFIG_NO_CHANGE'?
return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
^~~~~~~~~~~~~~~~~~~~~~~
OF_RECONFIG_NO_CHANGE
include/linux/of.h:1623:34: note: each undeclared identifier is reported only once for each function it appears in
include/linux/of.h: In function 'of_changeset_detach_node':
>> include/linux/of.h:1629:34: error: 'OF_RECONFIG_DETACH_NODE' undeclared (first use in this function); did you mean 'OF_RECONFIG_NO_CHANGE'?
return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
^~~~~~~~~~~~~~~~~~~~~~~
OF_RECONFIG_NO_CHANGE
include/linux/of.h: In function 'of_changeset_add_property':
>> include/linux/of.h:1635:34: error: 'OF_RECONFIG_ADD_PROPERTY' undeclared (first use in this function); did you mean 'OF_RECONFIG_NO_CHANGE'?
return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
^~~~~~~~~~~~~~~~~~~~~~~~
OF_RECONFIG_NO_CHANGE
include/linux/of.h: In function 'of_changeset_remove_property':
>> include/linux/of.h:1641:34: error: 'OF_RECONFIG_REMOVE_PROPERTY' undeclared (first use in this function); did you mean 'OF_RECONFIG_CHANGE_REMOVE'?
return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
OF_RECONFIG_CHANGE_REMOVE
include/linux/of.h: In function 'of_changeset_update_property':
>> include/linux/of.h:1647:34: error: 'OF_RECONFIG_UPDATE_PROPERTY' undeclared (first use in this function); did you mean 'OF_RECONFIG_CHANGE_REMOVE'?
return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
OF_RECONFIG_CHANGE_REMOVE
make[3]: *** [scripts/Makefile.build:182: kernel/sched/rq-offsets.s] Error 1 shuffle=1571759522
make[3]: Target 'prepare' not remade because of errors.
make[2]: *** [Makefile:1280: prepare0] Error 2 shuffle=1571759522
make[2]: Target 'prepare' not remade because of errors.
make[1]: *** [Makefile:248: __sub-make] Error 2 shuffle=1571759522
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:248: __sub-make] Error 2 shuffle=1571759522
make: Target 'prepare' not remade because of errors.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for OF_DYNAMIC
Depends on [n]: OF [=n]
Selected by [m]:
- NET_DSA_SJA1105 [=m] && NETDEVICES [=y] && NET_DSA [=m] && SPI [=y] && PTP_1588_CLOCK_OPTIONAL [=m]
vim +1623 include/linux/of.h
2e8fff668dc14e Rob Herring 2023-03-29 1604
201c910bd6898d Pantelis Antoniou 2014-07-04 1605 #ifdef CONFIG_OF_DYNAMIC
f6892d193fb9d6 Grant Likely 2014-11-21 1606 extern int of_reconfig_notifier_register(struct notifier_block *);
f6892d193fb9d6 Grant Likely 2014-11-21 1607 extern int of_reconfig_notifier_unregister(struct notifier_block *);
f5242e5a883bf1 Grant Likely 2014-11-24 1608 extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
f5242e5a883bf1 Grant Likely 2014-11-24 1609 extern int of_reconfig_get_state_change(unsigned long action,
f5242e5a883bf1 Grant Likely 2014-11-24 1610 struct of_reconfig_data *arg);
f6892d193fb9d6 Grant Likely 2014-11-21 1611
201c910bd6898d Pantelis Antoniou 2014-07-04 1612 extern void of_changeset_init(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04 1613 extern void of_changeset_destroy(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04 1614 extern int of_changeset_apply(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04 1615 extern int of_changeset_revert(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04 1616 extern int of_changeset_action(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1617 unsigned long action, struct device_node *np,
201c910bd6898d Pantelis Antoniou 2014-07-04 1618 struct property *prop);
201c910bd6898d Pantelis Antoniou 2014-07-04 1619
201c910bd6898d Pantelis Antoniou 2014-07-04 1620 static inline int of_changeset_attach_node(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1621 struct device_node *np)
201c910bd6898d Pantelis Antoniou 2014-07-04 1622 {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1623 return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
201c910bd6898d Pantelis Antoniou 2014-07-04 1624 }
201c910bd6898d Pantelis Antoniou 2014-07-04 1625
201c910bd6898d Pantelis Antoniou 2014-07-04 1626 static inline int of_changeset_detach_node(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1627 struct device_node *np)
201c910bd6898d Pantelis Antoniou 2014-07-04 1628 {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1629 return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
201c910bd6898d Pantelis Antoniou 2014-07-04 1630 }
201c910bd6898d Pantelis Antoniou 2014-07-04 1631
201c910bd6898d Pantelis Antoniou 2014-07-04 1632 static inline int of_changeset_add_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1633 struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04 1634 {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1635 return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04 1636 }
201c910bd6898d Pantelis Antoniou 2014-07-04 1637
201c910bd6898d Pantelis Antoniou 2014-07-04 1638 static inline int of_changeset_remove_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1639 struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04 1640 {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1641 return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04 1642 }
201c910bd6898d Pantelis Antoniou 2014-07-04 1643
201c910bd6898d Pantelis Antoniou 2014-07-04 1644 static inline int of_changeset_update_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04 1645 struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04 1646 {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1647 return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04 1648 }
b544fc2b8606d7 Lizhi Hou 2023-08-15 1649
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Vladimir,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-sja1105-let-phylink-help-with-the-replay-of-link-callbacks/20251119-031300
base: net-next/main
patch link: https://lore.kernel.org/r/20251118190530.580267-15-vladimir.oltean%40nxp.com
patch subject: [PATCH net-next 14/15] net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver
config: hexagon-randconfig-002-20251119 (https://download.01.org/0day-ci/archive/20251119/202511191946.DzxNhLVl-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 0bba1e76581bad04e7d7f09f5115ae5e2989e0d9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251119/202511191946.DzxNhLVl-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511191946.DzxNhLVl-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/net/dsa/sja1105/sja1105_mfd.c:146:5: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
145 | snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
| ~~~~
| %x
146 | pcs_res->res.start);
| ^~~~~~~~~~~~~~~~~~
1 warning generated.
vim +146 drivers/net/dsa/sja1105/sja1105_mfd.c
124
125 static int sja1105_create_pcs_nodes(struct sja1105_private *priv,
126 struct device_node *regs_node)
127 {
128 struct dsa_switch *ds = priv->ds;
129 struct device *dev = ds->dev;
130 struct device_node *pcs_node;
131 const u32 reg_io_width = 4;
132 char node_name[32];
133 u32 reg_props[2];
134 int rc;
135
136 for (int i = 0; i < priv->info->num_pcs_resources; i++) {
137 const struct sja1105_pcs_resource *pcs_res;
138
139 pcs_res = &priv->info->pcs_resources[i];
140
141 if (sja1105_child_node_exists(regs_node, "ethernet-pcs",
142 &pcs_res->res))
143 continue;
144
145 snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
> 146 pcs_res->res.start);
147
148 pcs_node = of_changeset_create_node(&priv->of_cs, regs_node,
149 node_name);
150 if (!pcs_node) {
151 dev_err(dev, "Failed to create PCS node %s\n", node_name);
152 return -ENOMEM;
153 }
154
155 rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
156 "compatible",
157 pcs_res->compatible);
158 if (rc) {
159 dev_err(dev, "Failed to add compatible property to %s: %pe\n",
160 node_name, ERR_PTR(rc));
161 return rc;
162 }
163
164 reg_props[0] = pcs_res->res.start;
165 reg_props[1] = resource_size(&pcs_res->res);
166 rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
167 "reg", reg_props, 2);
168 if (rc) {
169 dev_err(dev, "Failed to add reg property to %s: %pe\n",
170 node_name, ERR_PTR(rc));
171 return rc;
172 }
173
174 rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
175 "reg-names",
176 pcs_res->res.name);
177 if (rc) {
178 dev_err(dev, "Failed to add reg-names property to %s: %pe\n",
179 node_name, ERR_PTR(rc));
180 return rc;
181 }
182
183 rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
184 "reg-io-width",
185 ®_io_width, 1);
186 if (rc) {
187 dev_err(dev, "Failed to add reg-io-width property to %s: %pe\n",
188 node_name, ERR_PTR(rc));
189 return rc;
190 }
191
192 dev_dbg(dev, "Created OF node %pOF\n", pcs_node);
193 priv->pcs_fwnode[pcs_res->port] = of_fwnode_handle(pcs_node);
194 }
195
196 return 0;
197 }
198
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Vladimir,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-sja1105-let-phylink-help-with-the-replay-of-link-callbacks/20251119-031300
base: net-next/main
patch link: https://lore.kernel.org/r/20251118190530.580267-15-vladimir.oltean%40nxp.com
patch subject: [PATCH net-next 14/15] net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20251119/202511191835.rwfD48SW-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251119/202511191835.rwfD48SW-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511191835.rwfD48SW-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/net/dsa/sja1105/sja1105_mfd.c: In function 'sja1105_create_pcs_nodes':
>> drivers/net/dsa/sja1105/sja1105_mfd.c:145:73: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
145 | snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
| ~~~^
| |
| long long unsigned int
| %x
146 | pcs_res->res.start);
| ~~~~~~~~~~~~~~~~~~
| |
| resource_size_t {aka unsigned int}
vim +145 drivers/net/dsa/sja1105/sja1105_mfd.c
124
125 static int sja1105_create_pcs_nodes(struct sja1105_private *priv,
126 struct device_node *regs_node)
127 {
128 struct dsa_switch *ds = priv->ds;
129 struct device *dev = ds->dev;
130 struct device_node *pcs_node;
131 const u32 reg_io_width = 4;
132 char node_name[32];
133 u32 reg_props[2];
134 int rc;
135
136 for (int i = 0; i < priv->info->num_pcs_resources; i++) {
137 const struct sja1105_pcs_resource *pcs_res;
138
139 pcs_res = &priv->info->pcs_resources[i];
140
141 if (sja1105_child_node_exists(regs_node, "ethernet-pcs",
142 &pcs_res->res))
143 continue;
144
> 145 snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
146 pcs_res->res.start);
147
148 pcs_node = of_changeset_create_node(&priv->of_cs, regs_node,
149 node_name);
150 if (!pcs_node) {
151 dev_err(dev, "Failed to create PCS node %s\n", node_name);
152 return -ENOMEM;
153 }
154
155 rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
156 "compatible",
157 pcs_res->compatible);
158 if (rc) {
159 dev_err(dev, "Failed to add compatible property to %s: %pe\n",
160 node_name, ERR_PTR(rc));
161 return rc;
162 }
163
164 reg_props[0] = pcs_res->res.start;
165 reg_props[1] = resource_size(&pcs_res->res);
166 rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
167 "reg", reg_props, 2);
168 if (rc) {
169 dev_err(dev, "Failed to add reg property to %s: %pe\n",
170 node_name, ERR_PTR(rc));
171 return rc;
172 }
173
174 rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
175 "reg-names",
176 pcs_res->res.name);
177 if (rc) {
178 dev_err(dev, "Failed to add reg-names property to %s: %pe\n",
179 node_name, ERR_PTR(rc));
180 return rc;
181 }
182
183 rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
184 "reg-io-width",
185 ®_io_width, 1);
186 if (rc) {
187 dev_err(dev, "Failed to add reg-io-width property to %s: %pe\n",
188 node_name, ERR_PTR(rc));
189 return rc;
190 }
191
192 dev_dbg(dev, "Created OF node %pOF\n", pcs_node);
193 priv->pcs_fwnode[pcs_res->port] = of_fwnode_handle(pcs_node);
194 }
195
196 return 0;
197 }
198
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Nov 19, 2025 at 07:19:59PM +0800, kernel test robot wrote:
> Hi Vladimir,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on net-next/main]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-dsa-sja1105-let-phylink-help-with-the-replay-of-link-callbacks/20251119-031300
> base: net-next/main
> patch link: https://lore.kernel.org/r/20251118190530.580267-15-vladimir.oltean%40nxp.com
> patch subject: [PATCH net-next 14/15] net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20251119/202511191835.rwfD48SW-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 15.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251119/202511191835.rwfD48SW-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202511191835.rwfD48SW-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> drivers/net/dsa/sja1105/sja1105_mfd.c: In function 'sja1105_create_pcs_nodes':
> >> drivers/net/dsa/sja1105/sja1105_mfd.c:145:73: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
> 145 | snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
> | ~~~^
> | |
> | long long unsigned int
> | %x
> 146 | pcs_res->res.start);
> | ~~~~~~~~~~~~~~~~~~
> | |
> | resource_size_t {aka unsigned int}
I do wonder how to print resource_size_t (typedef to phys_addr_t, which
is typedeffed to u64 or u32 depending on CONFIG_PHYS_ADDR_T_64BIT).
Using %pa should be fine, like drivers/irqchip/irq-gic-v3-its.c does?
On Wed, Nov 19, 2025 at 02:01:11PM +0200, Vladimir Oltean wrote:
> I do wonder how to print resource_size_t (typedef to phys_addr_t, which
> is typedeffed to u64 or u32 depending on CONFIG_PHYS_ADDR_T_64BIT).
From the now hard to find Documentation/core-api/printk-formats.rst:
Physical address types phys_addr_t
----------------------------------
::
%pa[p] 0x01234567 or 0x0123456789abcdef
For printing a phys_addr_t type (and its derivatives, such as
resource_size_t) which can vary based on build options, regardless of the
width of the CPU data path.
Passed by reference.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 19, 2025 at 12:03:29PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 19, 2025 at 02:01:11PM +0200, Vladimir Oltean wrote: > > I do wonder how to print resource_size_t (typedef to phys_addr_t, which > > is typedeffed to u64 or u32 depending on CONFIG_PHYS_ADDR_T_64BIT). > > From the now hard to find Documentation/core-api/printk-formats.rst: > > Physical address types phys_addr_t > ---------------------------------- > > :: > > %pa[p] 0x01234567 or 0x0123456789abcdef > > For printing a phys_addr_t type (and its derivatives, such as > resource_size_t) which can vary based on build options, regardless of the > width of the CPU data path. > > Passed by reference. Hmm, but I guess you don't want the 0x prefix. Maybe print it to a separate buffer and then lop off the first two characters? Another solution would be to always cast it to a u64 and use %llx as suggested in the "Integer types" section. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 19, 2025 at 12:05:43PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 19, 2025 at 12:03:29PM +0000, Russell King (Oracle) wrote: > > On Wed, Nov 19, 2025 at 02:01:11PM +0200, Vladimir Oltean wrote: > > > I do wonder how to print resource_size_t (typedef to phys_addr_t, which > > > is typedeffed to u64 or u32 depending on CONFIG_PHYS_ADDR_T_64BIT). > > > > From the now hard to find Documentation/core-api/printk-formats.rst: I really wasn't aware of this, and I was reading the inline docs from lib/vsprintf.c all this time... Thanks! > > Physical address types phys_addr_t > > ---------------------------------- > > > > :: > > > > %pa[p] 0x01234567 or 0x0123456789abcdef > > > > For printing a phys_addr_t type (and its derivatives, such as > > resource_size_t) which can vary based on build options, regardless of the > > width of the CPU data path. > > > > Passed by reference. > > Hmm, but I guess you don't want the 0x prefix. Maybe print it to a > separate buffer and then lop off the first two characters? > > Another solution would be to always cast it to a u64 and use %llx as > suggested in the "Integer types" section. Yeah, although I see the ePAPR section on node names doesn't disallow the 0x prefix for the unit-address, it is just that the established convention is without it. By far the most unassuming option seems to be to do explicit type casting to unsigned long long. I've started a new test build with the changes made so far.
On Tue, 18 Nov 2025 21:05:29 +0200 Vladimir Oltean wrote:
> +static bool sja1105_child_node_exists(struct device_node *node,
> + const char *name,
> + const struct resource *res)
> +{
> + struct device_node *child = of_get_child_by_name(node, name);
> + u32 reg[2];
> +
> + for_each_child_of_node(node, child) {
> + if (!of_node_name_eq(child, name))
> + continue;
> +
> + if (of_property_read_u32_array(child, "reg", reg, ARRAY_SIZE(reg)))
> + continue;
> +
> + if (reg[0] == res->start && reg[1] == resource_size(res))
> + return true;
coccicheck says you're likely leaking the reference on the child here
> + }
> +
> + return false;
> +}
--
pw-bot: cr
On Tue, Nov 18, 2025 at 04:41:30PM -0800, Jakub Kicinski wrote:
> On Tue, 18 Nov 2025 21:05:29 +0200 Vladimir Oltean wrote:
> > +static bool sja1105_child_node_exists(struct device_node *node,
> > + const char *name,
> > + const struct resource *res)
> > +{
> > + struct device_node *child = of_get_child_by_name(node, name);
> > + u32 reg[2];
> > +
> > + for_each_child_of_node(node, child) {
> > + if (!of_node_name_eq(child, name))
> > + continue;
> > +
> > + if (of_property_read_u32_array(child, "reg", reg, ARRAY_SIZE(reg)))
> > + continue;
> > +
> > + if (reg[0] == res->start && reg[1] == resource_size(res))
> > + return true;
>
> coccicheck says you're likely leaking the reference on the child here
Ok, one item added to the change list for v2.
Why is cocci-check.sh part of the "contest" test suite that runs on
remote executors? This test didn't run when I tested this series locally
with ingest_mdir.py.
> > + }
> > +
> > + return false;
> > +}
> --
> pw-bot: cr
On Wed, Nov 19, 2025 at 11:59:42AM +0200, Vladimir Oltean wrote:
> On Tue, Nov 18, 2025 at 04:41:30PM -0800, Jakub Kicinski wrote:
> > On Tue, 18 Nov 2025 21:05:29 +0200 Vladimir Oltean wrote:
...
> > > + for_each_child_of_node(node, child) {
> > > + if (!of_node_name_eq(child, name))
> > > + continue;
> > > +
> > > + if (of_property_read_u32_array(child, "reg", reg, ARRAY_SIZE(reg)))
> > > + continue;
> > > +
> > > + if (reg[0] == res->start && reg[1] == resource_size(res))
> > > + return true;
> >
> > coccicheck says you're likely leaking the reference on the child here
>
> Ok, one item added to the change list for v2.
Note, we have __free() and _scoped() variants of the respective APIs to make it
easier to not forget.
> Why is cocci-check.sh part of the "contest" test suite that runs on
> remote executors? This test didn't run when I tested this series locally
> with ingest_mdir.py.
I believe it's due to heavy load it makes. Running it on a (whole) kernel make
take hours.
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 19, 2025 at 12:31:27PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 19, 2025 at 11:59:42AM +0200, Vladimir Oltean wrote:
> > On Tue, Nov 18, 2025 at 04:41:30PM -0800, Jakub Kicinski wrote:
> > > On Tue, 18 Nov 2025 21:05:29 +0200 Vladimir Oltean wrote:
>
> ...
>
> > > > + for_each_child_of_node(node, child) {
> > > > + if (!of_node_name_eq(child, name))
> > > > + continue;
> > > > +
> > > > + if (of_property_read_u32_array(child, "reg", reg, ARRAY_SIZE(reg)))
> > > > + continue;
> > > > +
> > > > + if (reg[0] == res->start && reg[1] == resource_size(res))
> > > > + return true;
> > >
> > > coccicheck says you're likely leaking the reference on the child here
> >
> > Ok, one item added to the change list for v2.
>
> Note, we have __free() and _scoped() variants of the respective APIs to make it
> easier to not forget.
Ok, I'll use for_each_child_of_node_scoped(), as it's a good fit, thanks.
> > Why is cocci-check.sh part of the "contest" test suite that runs on
> > remote executors? This test didn't run when I tested this series locally
> > with ingest_mdir.py.
>
> I believe it's due to heavy load it makes. Running it on a (whole) kernel make
> take hours.
For context, the "local" NIPA tests for this set took me 3 hours and 34 minutes
to run on 32 AMD EPYC 9R14 CPU cores. So if cocci-check.sh increases that time
by a few additional hours, yeah, it's bad, but it was bad before too, so
the "heavy load" argument doesn't satisfactorily explain things.
Plus, looking deeper at the NIPA code, it only submits the patch set
under test to a "contest" branch if it already passed the slow
gate_checks=build_clang,build_32bit,build_allmodconfig_warn. So it's not
like the "local" build tests run in parallel with the remote worker, and
we're not saving any time for a single build.
I think it's due to the fact that the "contest" checks are fundamentally
so slow, that they can't be run on individual patch sets, and are run on
batches of patch sets merged into a single branch (of which there seem
to be 8 per day). I didn't get this from NIPA documentation, though.
On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > I think it's due to the fact that the "contest" checks are fundamentally > so slow, that they can't be run on individual patch sets, and are run on > batches of patch sets merged into a single branch (of which there seem > to be 8 per day). Correct, looking at the logs AFAICT coccicheck takes 25min on a relatively beefy machine, and we only run it on path that were actually modified by pending changes. We get 100+ patches a day, and 40+ series, and coccicheck fails relatively rarely. So on the NIPA side it's not worth it. But, we can certainly integrate it into ingest_mdir. FTR make htmldocs and of course selftests are also not executed by ingest_mdir.
On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote: > On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > > I think it's due to the fact that the "contest" checks are fundamentally > > so slow, that they can't be run on individual patch sets, and are run on > > batches of patch sets merged into a single branch (of which there seem > > to be 8 per day). > > Correct, looking at the logs AFAICT coccicheck takes 25min on a > relatively beefy machine, and we only run it on path that were actually > modified by pending changes. We get 100+ patches a day, and 40+ series, > and coccicheck fails relatively rarely. So on the NIPA side it's not > worth it. On "contest" I find when looking at patchwork, it's just best to ignore the result that NIPA posts for that, because more often than not it reports "fail" when there's nothing wrong. For example, the qcom-ethqos patches - v1 passed contest, and this morning I submitted v2. The only change was removing the double space in patch 2. What I see in v2 is that _all_ the patches failed contest, even those that are unchanged and previously passed. This makes contest unreliable and IMHO misleading - and as such I hate it. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, 20 Nov 2025 12:32:37 +0000 Russell King (Oracle) wrote: > On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote: > > On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > > > I think it's due to the fact that the "contest" checks are fundamentally > > > so slow, that they can't be run on individual patch sets, and are run on > > > batches of patch sets merged into a single branch (of which there seem > > > to be 8 per day). > > > > Correct, looking at the logs AFAICT coccicheck takes 25min on a > > relatively beefy machine, and we only run it on path that were actually > > modified by pending changes. We get 100+ patches a day, and 40+ series, > > and coccicheck fails relatively rarely. So on the NIPA side it's not > > worth it. > > On "contest" I find when looking at patchwork, it's just best to ignore > the result that NIPA posts for that, because more often than not it > reports "fail" when there's nothing wrong. > > For example, the qcom-ethqos patches - v1 passed contest, and this > morning I submitted v2. The only change was removing the double space > in patch 2. What I see in v2 is that _all_ the patches failed contest, > even those that are unchanged and previously passed. This makes > contest unreliable and IMHO misleading - and as such I hate it. Fair, I'll fix it over the weekend. tl;dr it shows up as failing until we get a clean run because of patchwork UI shortcomings. Long version is that unfortunately patchwork UI does not show "pending" tests on the main page. So when we eyeball the queue to get a sense of patches which are fully validated its hard to tell "done" from "in progress". I believe BPF's KPD system also uses "fail until finished", I'm guessing for the same reason. That said I stopped using the patchwork UI completely now, and switch to my own UIs within NIPA. So patchwork shortcomings are no longer a concern.
On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote: > On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > > I think it's due to the fact that the "contest" checks are fundamentally > > so slow, that they can't be run on individual patch sets, and are run on > > batches of patch sets merged into a single branch (of which there seem > > to be 8 per day). > > Correct, looking at the logs AFAICT coccicheck takes 25min on a > relatively beefy machine, and we only run it on path that were actually > modified by pending changes. We get 100+ patches a day, and 40+ series, > and coccicheck fails relatively rarely. So on the NIPA side it's not > worth it. > > But, we can certainly integrate it into ingest_mdir. > > FTR make htmldocs and of course selftests are also not executed by > ingest_mdir. Btw, do you do `make W=1` while having CONFIG_WERROR=y? And if so, do you also compile with clang? Sorry if it's documented / answered already, please point me to that discussion / documentation. -- With Best Regards, Andy Shevchenko
On Wed, Nov 19, 2025 at 06:17:48PM +0200, Andy Shevchenko wrote: > On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote: > > On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > > > I think it's due to the fact that the "contest" checks are fundamentally > > > so slow, that they can't be run on individual patch sets, and are run on > > > batches of patch sets merged into a single branch (of which there seem > > > to be 8 per day). > > > > Correct, looking at the logs AFAICT coccicheck takes 25min on a > > relatively beefy machine, and we only run it on path that were actually > > modified by pending changes. We get 100+ patches a day, and 40+ series, > > and coccicheck fails relatively rarely. So on the NIPA side it's not > > worth it. > > > > But, we can certainly integrate it into ingest_mdir. > > > > FTR make htmldocs and of course selftests are also not executed by > > ingest_mdir. > > Btw, do you do `make W=1` while having CONFIG_WERROR=y? > And if so, do you also compile with clang? If you look at any patch in patchwork, e.g. https://patchwork.kernel.org/project/netdevbpf/patch/E1vLgSZ-0000000FMrW-0cEu@rmk-PC.armlinux.org.uk/ it gives a list of the tests performed. In answer to your questions: netdev/build_clang success Errors and warnings before: 1 this patch: 1 So yes, building with clang is tested. I can say that stuff such as unused const variables gets found by nipa, via -Wunused-const-variable, which as I understand it is a W=1 thing. I suspect CONFIG_WERROR=y isn't used (I don't know) as that would stop the build on warnings, whereas what is done instead is that the output of the build is analysed, and the number of warnings counted and reported in patchwork. Note that the "build" stuff reports number of errors/warnings before and after the patch. Hope this answers your question. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 19, 2025 at 05:23:48PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 19, 2025 at 06:17:48PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote: > > > On Wed, 19 Nov 2025 13:25:22 +0200 Vladimir Oltean wrote: > > > > I think it's due to the fact that the "contest" checks are fundamentally > > > > so slow, that they can't be run on individual patch sets, and are run on > > > > batches of patch sets merged into a single branch (of which there seem > > > > to be 8 per day). > > > > > > Correct, looking at the logs AFAICT coccicheck takes 25min on a > > > relatively beefy machine, and we only run it on path that were actually > > > modified by pending changes. We get 100+ patches a day, and 40+ series, > > > and coccicheck fails relatively rarely. So on the NIPA side it's not > > > worth it. > > > > > > But, we can certainly integrate it into ingest_mdir. > > > > > > FTR make htmldocs and of course selftests are also not executed by > > > ingest_mdir. > > > > Btw, do you do `make W=1` while having CONFIG_WERROR=y? > > And if so, do you also compile with clang? > > If you look at any patch in patchwork, e.g. > > https://patchwork.kernel.org/project/netdevbpf/patch/E1vLgSZ-0000000FMrW-0cEu@rmk-PC.armlinux.org.uk/ > > it gives a list of the tests performed. In answer to your questions: > > netdev/build_clang success Errors and warnings before: 1 this patch: 1 > > So yes, building with clang is tested. > > I can say that stuff such as unused const variables gets found by > nipa, via -Wunused-const-variable, which as I understand it is a > W=1 thing. > > I suspect CONFIG_WERROR=y isn't used (I don't know) as that would > stop the build on warnings, whereas what is done instead is that > the output of the build is analysed, and the number of warnings > counted and reported in patchwork. Note that the "build" stuff > reports number of errors/warnings before and after the patch. > > Hope this answers your question. Pretty much, thanks! -- With Best Regards, Andy Shevchenko
On Wed, 19 Nov 2025 19:39:32 +0200 Andy Shevchenko wrote:
> On Wed, Nov 19, 2025 at 05:23:48PM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 19, 2025 at 06:17:48PM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote:
> > I can say that stuff such as unused const variables gets found by
> > nipa, via -Wunused-const-variable, which as I understand it is a
> > W=1 thing.
> >
> > I suspect CONFIG_WERROR=y isn't used (I don't know) as that would
> > stop the build on warnings, whereas what is done instead is that
> > the output of the build is analysed, and the number of warnings
> > counted and reported in patchwork. Note that the "build" stuff
> > reports number of errors/warnings before and after the patch.
> >
> > Hope this answers your question.
>
> Pretty much, thanks!
We do:
prep_config() {
make LLVM=1 O=$output_dir allmodconfig
./scripts/config --file $output_dir/.config -d werror
./scripts/config --file $output_dir/.config -d drm_werror
./scripts/config --file $output_dir/.config -d kvm_werror
}
I have strong feelings about people who think Werror is an acceptable
practice in 2025, but let me not violate the CoC here..
On Wed, Nov 19, 2025 at 10:35:45AM -0800, Jakub Kicinski wrote:
> On Wed, 19 Nov 2025 19:39:32 +0200 Andy Shevchenko wrote:
> > On Wed, Nov 19, 2025 at 05:23:48PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 19, 2025 at 06:17:48PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 19, 2025 at 08:11:12AM -0800, Jakub Kicinski wrote:
> > > I can say that stuff such as unused const variables gets found by
> > > nipa, via -Wunused-const-variable, which as I understand it is a
> > > W=1 thing.
> > >
> > > I suspect CONFIG_WERROR=y isn't used (I don't know) as that would
> > > stop the build on warnings, whereas what is done instead is that
> > > the output of the build is analysed, and the number of warnings
> > > counted and reported in patchwork. Note that the "build" stuff
> > > reports number of errors/warnings before and after the patch.
> > >
> > > Hope this answers your question.
> >
> > Pretty much, thanks!
>
> We do:
A-ha, thanks!
> prep_config() {
> make LLVM=1 O=$output_dir allmodconfig
> ./scripts/config --file $output_dir/.config -d werror
> ./scripts/config --file $output_dir/.config -d drm_werror
> ./scripts/config --file $output_dir/.config -d kvm_werror
> }
>
> I have strong feelings about people who think Werror is an acceptable
> practice in 2025, but let me not violate the CoC here..
WERROR=y by default was introduced you-know-by-whom :-)
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.