2022-07-27 06:45:46

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 0/8] DSA: switch to fwnode_/device_

Hi,

This is a re-spin of the DSA migration to fwnode_/device_ API.
It addresses all comments from the previous iteration - the
details are summarized in the changelog section below.

This time the patchset is based and tested on top of
pure net-next/main branch. Each commit was checked on:
* On EspressoBIN
* On SolidRun CN913x CEx7 Eval Board

Any comments or remarks will be appreciated.

Best regards,
Marcin

Changelog v2 -> v3:
1/8:
* Replace forward declaration s/device_node/fwnode_handle/ in
include/linux/phy_fixed.h
* Add Florian's RB

* 3/8:
* Extend lines width in the commit message.
* While dropping dp->dn fields in the drivers, switch to
fwnode_ API in the updated places.

* 5/8:
* Update routine name to fwnode_find_parent_dev_match()
* Improve comment section
* Move the definition adjacent to a group of fwnode
APIs operating on parents

Changelog v1 -> v2:
1/8
* Drop unnecessary check in fixed_phy_get_gpiod()
* Improve line breaking
* Use device_set_node & dev_fwnode

2/8
* Switch to fwnode_property_count_u32 and fix comparison
in if statement.

3/8
* Drop dn usage entirely and use dp->fwnode only. Update
all dependent drivers to use to_of_node.
* Use device_set_node, dev_fwnode & device_get_named_child_node
* Replace '_of' routines suffix with '_fw'

4/8
* Use device_set_node

5/8
* New patch

6/8
* Use device_match_fwnode
* Restore EXPORT_SYMBOL()

7/8
* Get rid of of_mdiobus_register_device

8/8
* Use dev_fwnode in mv88e6xxx_probe
* Simplify condition checks in mv88e6xxx_probe as suggested by Andy

Marcin Wojtas (8):
net: phy: fixed_phy: switch to fwnode_ API
net: mdio: switch fixed-link PHYs API to fwnode_
net: dsa: switch to device_/fwnode_ APIs
net: mvpp2: initialize port fwnode pointer
device property: introduce fwnode_find_parent_dev_match
net: core: switch to fwnode_find_net_device_by_node()
net: mdio: introduce fwnode_mdiobus_register_device()
net: dsa: mv88e6xxx: switch to device_/fwnode_ APIs

include/linux/etherdevice.h | 1 +
include/linux/fwnode_mdio.h | 22 ++++
include/linux/of_net.h | 6 -
include/linux/phy_fixed.h | 6 +-
include/linux/property.h | 1 +
include/net/dsa.h | 2 +-
net/dsa/dsa_priv.h | 4 +-
drivers/base/property.c | 23 ++++
drivers/net/dsa/mt7530.c | 6 +-
drivers/net/dsa/mv88e6xxx/chip.c | 57 ++++-----
drivers/net/dsa/qca/qca8k.c | 2 +-
drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
drivers/net/mdio/fwnode_mdio.c | 129 ++++++++++++++++++++
drivers/net/mdio/of_mdio.c | 111 +----------------
drivers/net/phy/fixed_phy.c | 39 +++---
net/core/net-sysfs.c | 25 ++--
net/dsa/dsa2.c | 101 ++++++++-------
net/dsa/port.c | 68 +++++------
net/dsa/slave.c | 7 +-
20 files changed, 329 insertions(+), 285 deletions(-)

--
2.29.0


2022-07-27 06:46:00

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer

As a preparation to switch the DSA subsystem from using
of_find_net_device_by_node() to its more generic fwnode_
equivalent, the port's device structure should be updated
with its fwnode pointer, similarly to of_node - see analogous
commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer").

This patch is required to prevent a regression before updating
the DSA API on boards that connect the mvpp2 port to switch,
such as Clearfog GT-8K or CN913x CEx7 Evaluation Board.

Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index b84128b549b4..03d5ff649c47 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6868,7 +6868,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
dev->min_mtu = ETH_MIN_MTU;
/* 9704 == 9728 - 20 and rounding to 8 */
dev->max_mtu = MVPP2_BM_JUMBO_PKT_SIZE;
- dev->dev.of_node = port_node;
+ device_set_node(&dev->dev, port_fwnode);

port->pcs_gmac.ops = &mvpp2_phylink_gmac_pcs_ops;
port->pcs_xlg.ops = &mvpp2_phylink_xlg_pcs_ops;
--
2.29.0

2022-07-27 06:46:25

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 8/8] net: dsa: mv88e6xxx: switch to device_/fwnode_ APIs

In order to support both DT and ACPI in future, modify the
mv88e6xx driver code to use device_/fwnode_ equivalent routines.
No functional change is introduced by this patch.

Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 43 +++++++++-----------
1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index a46ebdfba1c3..e7848b56316f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3891,10 +3891,11 @@ static int mv88e6xxx_mdio_write(struct mii_bus *bus, int phy, int reg, u16 val)
}

static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
- struct device_node *np,
+ struct fwnode_handle *fwnode,
bool external)
{
static int index;
+ struct device_node *np = to_of_node(fwnode);
struct mv88e6xxx_mdio_bus *mdio_bus;
struct mii_bus *bus;
int err;
@@ -3973,18 +3974,18 @@ static void mv88e6xxx_mdios_unregister(struct mv88e6xxx_chip *chip)
}

static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
- struct device_node *np)
+ struct fwnode_handle *fwnode)
{
- struct device_node *child;
+ struct fwnode_handle *child;
int err;

/* Always register one mdio bus for the internal/default mdio
* bus. This maybe represented in the device tree, but is
* optional.
*/
- child = of_get_child_by_name(np, "mdio");
+ child = fwnode_get_named_child_node(fwnode, "mdio");
err = mv88e6xxx_mdio_register(chip, child, false);
- of_node_put(child);
+ fwnode_handle_put(child);
if (err)
return err;

@@ -3992,13 +3993,13 @@ static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
* which say they are compatible with the external mdio
* bus.
*/
- for_each_available_child_of_node(np, child) {
- if (of_device_is_compatible(
- child, "marvell,mv88e6xxx-mdio-external")) {
+ fwnode_for_each_available_child_node(fwnode, child) {
+ if (fwnode_property_match_string(child, "compatible",
+ "marvell,mv88e6xxx-mdio-external") == 0) {
err = mv88e6xxx_mdio_register(chip, child, true);
if (err) {
mv88e6xxx_mdios_unregister(chip);
- of_node_put(child);
+ fwnode_handle_put(child);
return err;
}
}
@@ -6984,20 +6985,16 @@ static SIMPLE_DEV_PM_OPS(mv88e6xxx_pm_ops, mv88e6xxx_suspend, mv88e6xxx_resume);
static int mv88e6xxx_probe(struct mdio_device *mdiodev)
{
struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;
+ struct fwnode_handle *fwnode = dev_fwnode(&mdiodev->dev);
const struct mv88e6xxx_info *compat_info = NULL;
struct device *dev = &mdiodev->dev;
- struct device_node *np = dev->of_node;
struct mv88e6xxx_chip *chip;
int port;
int err;

- if (!np && !pdata)
- return -EINVAL;
-
- if (np)
- compat_info = of_device_get_match_data(dev);
-
- if (pdata) {
+ if (fwnode)
+ compat_info = device_get_match_data(dev);
+ else if (pdata) {
compat_info = pdata_device_get_match_data(dev);

if (!pdata->netdev)
@@ -7054,9 +7051,9 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
mv88e6xxx_phy_init(chip);

if (chip->info->ops->get_eeprom) {
- if (np)
- of_property_read_u32(np, "eeprom-length",
- &chip->eeprom_len);
+ if (fwnode)
+ device_property_read_u32(dev, "eeprom-length",
+ &chip->eeprom_len);
else
chip->eeprom_len = pdata->eeprom_len;
}
@@ -7067,8 +7064,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
if (err)
goto out;

- if (np) {
- chip->irq = of_irq_get(np, 0);
+ if (fwnode) {
+ chip->irq = fwnode_irq_get(fwnode, 0);
if (chip->irq == -EPROBE_DEFER) {
err = chip->irq;
goto out;
@@ -7106,7 +7103,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
if (err)
goto out_g1_atu_prob_irq;

- err = mv88e6xxx_mdios_register(chip, np);
+ err = mv88e6xxx_mdios_register(chip, fwnode);
if (err)
goto out_g1_vtu_prob_irq;

--
2.29.0

2022-07-27 06:46:44

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 5/8] device property: introduce fwnode_find_parent_dev_match

This patch adds a new generic routine fwnode_find_parent_dev_match
that can be used e.g. as a callback for class_find_device().
It searches for the struct device corresponding to a
struct fwnode_handle by iterating over device and
its parents.

Signed-off-by: Marcin Wojtas <[email protected]>
---
include/linux/property.h | 1 +
drivers/base/property.c | 23 ++++++++++++++++++++
2 files changed, 24 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index a5b429d623f6..829254a5ae63 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -95,6 +95,7 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
unsigned int depth);
+int fwnode_find_parent_dev_match(struct device *dev, const void *data);
bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_child_node(
const struct fwnode_handle *fwnode, struct fwnode_handle *child);
diff --git a/drivers/base/property.c b/drivers/base/property.c
index ed6f449f8e5c..ee6a8144f103 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -685,6 +685,29 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
}
EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);

+/**
+ * fwnode_find_parent_dev_match - look for a device matching the struct fwnode_handle
+ * @dev: the struct device to initiate the search
+ * @data: pointer passed for comparison
+ *
+ * Looks up the device structure corresponding with the fwnode by iterating
+ * over @dev and its parents.
+ * The routine can be used e.g. as a callback for class_find_device().
+ *
+ * Returns: %1 - match is found
+ * %0 - match not found
+ */
+int fwnode_find_parent_dev_match(struct device *dev, const void *data)
+{
+ for (; dev; dev = dev->parent) {
+ if (device_match_fwnode(dev, data))
+ return 1;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match);
+
/**
* fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child
* @ancestor: Firmware which is tested for being an ancestor
--
2.29.0

2022-07-27 06:46:48

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 2/8] net: mdio: switch fixed-link PHYs API to fwnode_

fixed-link PHYs API is used by DSA and a number of drivers
and was depending on of_. Switch to fwnode_ so to make it
hardware description agnostic and allow to be used in ACPI
world as well.

Signed-off-by: Marcin Wojtas <[email protected]>
---
include/linux/fwnode_mdio.h | 19 ++++
drivers/net/mdio/fwnode_mdio.c | 100 ++++++++++++++++++++
drivers/net/mdio/of_mdio.c | 79 +---------------
3 files changed, 122 insertions(+), 76 deletions(-)

diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
index faf603c48c86..98755b8c6c8a 100644
--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h
@@ -16,6 +16,11 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct fwnode_handle *child, u32 addr);

+int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode);
+
+void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode);
+
+bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode);
#else /* CONFIG_FWNODE_MDIO */
int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
@@ -30,6 +35,20 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
{
return -EINVAL;
}
+
+static inline int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode)
+{
+ return -ENODEV;
+}
+
+static inline void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode)
+{
+}
+
+static inline bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode)
+{
+ return false;
+}
#endif

#endif /* __LINUX_FWNODE_MDIO_H */
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1c1584fca632..454fdae24150 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -10,6 +10,7 @@
#include <linux/fwnode_mdio.h>
#include <linux/of.h>
#include <linux/phy.h>
+#include <linux/phy_fixed.h>

MODULE_AUTHOR("Calvin Johnson <[email protected]>");
MODULE_LICENSE("GPL");
@@ -147,3 +148,102 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
return 0;
}
EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
+/*
+ * fwnode_phy_is_fixed_link() and fwnode_phy_register_fixed_link() must
+ * support two bindings:
+ * - the old binding, where 'fixed-link' was a property with 5
+ * cells encoding various information about the fixed PHY
+ * - the new binding, where 'fixed-link' is a sub-node of the
+ * Ethernet device.
+ */
+bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *fixed_link_node;
+ const char *managed;
+ int len;
+
+ /* New binding */
+ fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link");
+ if (fixed_link_node) {
+ fwnode_handle_put(fixed_link_node);
+ return true;
+ }
+
+ if (fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&
+ strcmp(managed, "auto") != 0)
+ return true;
+
+ /* Old binding */
+ len = fwnode_property_count_u32(fwnode, "fixed-link");
+ if (len == 5)
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL(fwnode_phy_is_fixed_link);
+
+int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode)
+{
+ struct fixed_phy_status status = {};
+ struct fwnode_handle *fixed_link_node;
+ u32 fixed_link_prop[5];
+ const char *managed;
+
+ if (fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&
+ strcmp(managed, "in-band-status") == 0) {
+ /* status is zeroed, namely its .link member */
+ goto register_phy;
+ }
+
+ /* New binding */
+ fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link");
+ if (fixed_link_node) {
+ status.link = 1;
+ status.duplex = fwnode_property_present(fixed_link_node,
+ "full-duplex");
+ if (fwnode_property_read_u32(fixed_link_node, "speed",
+ &status.speed)) {
+ fwnode_handle_put(fixed_link_node);
+ return -EINVAL;
+ }
+ status.pause = fwnode_property_present(fixed_link_node, "pause");
+ status.asym_pause = fwnode_property_present(fixed_link_node,
+ "asym-pause");
+ fwnode_handle_put(fixed_link_node);
+
+ goto register_phy;
+ }
+
+ /* Old binding */
+ if (fwnode_property_read_u32_array(fwnode, "fixed-link", fixed_link_prop,
+ ARRAY_SIZE(fixed_link_prop)) == 0) {
+ status.link = 1;
+ status.duplex = fixed_link_prop[1];
+ status.speed = fixed_link_prop[2];
+ status.pause = fixed_link_prop[3];
+ status.asym_pause = fixed_link_prop[4];
+ goto register_phy;
+ }
+
+ return -ENODEV;
+
+register_phy:
+ return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, fwnode));
+}
+EXPORT_SYMBOL(fwnode_phy_register_fixed_link);
+
+void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode)
+{
+ struct phy_device *phydev;
+
+ phydev = fwnode_phy_find_device(fwnode);
+ if (!phydev)
+ return;
+
+ fixed_phy_unregister(phydev);
+
+ put_device(&phydev->mdio.dev); /* fwnode_phy_find_device() */
+ phy_device_free(phydev); /* fixed_phy_register() */
+}
+EXPORT_SYMBOL(fwnode_phy_deregister_fixed_link);
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index d755fe1ecdda..409da6e92f7d 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -351,91 +351,18 @@ EXPORT_SYMBOL(of_phy_get_and_connect);
*/
bool of_phy_is_fixed_link(struct device_node *np)
{
- struct device_node *dn;
- int len, err;
- const char *managed;
-
- /* New binding */
- dn = of_get_child_by_name(np, "fixed-link");
- if (dn) {
- of_node_put(dn);
- return true;
- }
-
- err = of_property_read_string(np, "managed", &managed);
- if (err == 0 && strcmp(managed, "auto") != 0)
- return true;
-
- /* Old binding */
- if (of_get_property(np, "fixed-link", &len) &&
- len == (5 * sizeof(__be32)))
- return true;
-
- return false;
+ return fwnode_phy_is_fixed_link(of_fwnode_handle(np));
}
EXPORT_SYMBOL(of_phy_is_fixed_link);

int of_phy_register_fixed_link(struct device_node *np)
{
- struct fixed_phy_status status = {};
- struct device_node *fixed_link_node;
- u32 fixed_link_prop[5];
- const char *managed;
-
- if (of_property_read_string(np, "managed", &managed) == 0 &&
- strcmp(managed, "in-band-status") == 0) {
- /* status is zeroed, namely its .link member */
- goto register_phy;
- }
-
- /* New binding */
- fixed_link_node = of_get_child_by_name(np, "fixed-link");
- if (fixed_link_node) {
- status.link = 1;
- status.duplex = of_property_read_bool(fixed_link_node,
- "full-duplex");
- if (of_property_read_u32(fixed_link_node, "speed",
- &status.speed)) {
- of_node_put(fixed_link_node);
- return -EINVAL;
- }
- status.pause = of_property_read_bool(fixed_link_node, "pause");
- status.asym_pause = of_property_read_bool(fixed_link_node,
- "asym-pause");
- of_node_put(fixed_link_node);
-
- goto register_phy;
- }
-
- /* Old binding */
- if (of_property_read_u32_array(np, "fixed-link", fixed_link_prop,
- ARRAY_SIZE(fixed_link_prop)) == 0) {
- status.link = 1;
- status.duplex = fixed_link_prop[1];
- status.speed = fixed_link_prop[2];
- status.pause = fixed_link_prop[3];
- status.asym_pause = fixed_link_prop[4];
- goto register_phy;
- }
-
- return -ENODEV;
-
-register_phy:
- return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, of_fwnode_handle(np)));
+ return fwnode_phy_register_fixed_link(of_fwnode_handle(np));
}
EXPORT_SYMBOL(of_phy_register_fixed_link);

void of_phy_deregister_fixed_link(struct device_node *np)
{
- struct phy_device *phydev;
-
- phydev = of_phy_find_device(np);
- if (!phydev)
- return;
-
- fixed_phy_unregister(phydev);
-
- put_device(&phydev->mdio.dev); /* of_phy_find_device() */
- phy_device_free(phydev); /* fixed_phy_register() */
+ fwnode_phy_deregister_fixed_link(of_fwnode_handle(np));
}
EXPORT_SYMBOL(of_phy_deregister_fixed_link);
--
2.29.0

2022-07-27 06:46:53

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 7/8] net: mdio: introduce fwnode_mdiobus_register_device()

As a preparation patch to extend MDIO capabilities in the ACPI world,
introduce fwnode_mdiobus_register_device() to register non-PHY
devices on the mdiobus.

Use the newly introduced routine instead of of_mdiobus_register_device().

Signed-off-by: Marcin Wojtas <[email protected]>
---
include/linux/fwnode_mdio.h | 3 ++
drivers/net/mdio/fwnode_mdio.c | 29 ++++++++++++++++++
drivers/net/mdio/of_mdio.c | 32 +-------------------
3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
index 98755b8c6c8a..39d74c5d1bb0 100644
--- a/include/linux/fwnode_mdio.h
+++ b/include/linux/fwnode_mdio.h
@@ -16,6 +16,9 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct fwnode_handle *child, u32 addr);

+int fwnode_mdiobus_register_device(struct mii_bus *mdio,
+ struct fwnode_handle *child, u32 addr);
+
int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode);

void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode);
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 454fdae24150..3743f34e7c2d 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -149,6 +149,35 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
}
EXPORT_SYMBOL(fwnode_mdiobus_register_phy);

+int fwnode_mdiobus_register_device(struct mii_bus *mdio,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct mdio_device *mdiodev;
+ int rc;
+
+ mdiodev = mdio_device_create(mdio, addr);
+ if (IS_ERR(mdiodev))
+ return PTR_ERR(mdiodev);
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ device_set_node(&mdiodev->dev, child);
+
+ /* All data is now stored in the mdiodev struct; register it. */
+ rc = mdio_device_register(mdiodev);
+ if (rc) {
+ mdio_device_free(mdiodev);
+ fwnode_handle_put(child);
+ return rc;
+ }
+
+ dev_dbg(&mdio->dev, "registered mdio device %p fwnode at address %i\n",
+ child, addr);
+ return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_device);
+
/*
* fwnode_phy_is_fixed_link() and fwnode_phy_register_fixed_link() must
* support two bindings:
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 409da6e92f7d..bd941da030bb 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -48,36 +48,6 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
return fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr);
}

-static int of_mdiobus_register_device(struct mii_bus *mdio,
- struct device_node *child, u32 addr)
-{
- struct fwnode_handle *fwnode = of_fwnode_handle(child);
- struct mdio_device *mdiodev;
- int rc;
-
- mdiodev = mdio_device_create(mdio, addr);
- if (IS_ERR(mdiodev))
- return PTR_ERR(mdiodev);
-
- /* Associate the OF node with the device structure so it
- * can be looked up later.
- */
- fwnode_handle_get(fwnode);
- device_set_node(&mdiodev->dev, fwnode);
-
- /* All data is now stored in the mdiodev struct; register it. */
- rc = mdio_device_register(mdiodev);
- if (rc) {
- mdio_device_free(mdiodev);
- of_node_put(child);
- return rc;
- }
-
- dev_dbg(&mdio->dev, "registered mdio device %pOFn at address %i\n",
- child, addr);
- return 0;
-}
-
/* The following is a list of PHY compatible strings which appear in
* some DTBs. The compatible string is never matched against a PHY
* driver, so is pointless. We only expect devices which are not PHYs
@@ -186,7 +156,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
if (of_mdiobus_child_is_phy(child))
rc = of_mdiobus_register_phy(mdio, child, addr);
else
- rc = of_mdiobus_register_device(mdio, child, addr);
+ rc = fwnode_mdiobus_register_device(mdio, of_fwnode_handle(child), addr);

if (rc == -ENODEV)
dev_err(&mdio->dev,
--
2.29.0

2022-07-27 06:59:22

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 1/8] net: phy: fixed_phy: switch to fwnode_ API

This patch allows to use fixed_phy driver and its helper
functions without Device Tree dependency, by swtiching from
of_ to fwnode_ API.

Signed-off-by: Marcin Wojtas <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
include/linux/phy_fixed.h | 6 +--
drivers/net/mdio/of_mdio.c | 2 +-
drivers/net/phy/fixed_phy.c | 39 +++++++-------------
3 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 52bc8e487ef7..e3a03494f218 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -10,7 +10,7 @@ struct fixed_phy_status {
int asym_pause;
};

-struct device_node;
+struct fwnode_handle;
struct gpio_desc;

#if IS_ENABLED(CONFIG_FIXED_PHY)
@@ -19,7 +19,7 @@ extern int fixed_phy_add(unsigned int irq, int phy_id,
struct fixed_phy_status *status);
extern struct phy_device *fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np);
+ struct fwnode_handle *fwnode);

extern struct phy_device *
fixed_phy_register_with_gpiod(unsigned int irq,
@@ -38,7 +38,7 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id,
}
static inline struct phy_device *fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np)
+ struct fwnode_handle *fwnode)
{
return ERR_PTR(-ENODEV);
}
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 9e3c815a070f..d755fe1ecdda 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -421,7 +421,7 @@ int of_phy_register_fixed_link(struct device_node *np)
return -ENODEV;

register_phy:
- return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, np));
+ return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, of_fwnode_handle(np)));
}
EXPORT_SYMBOL(of_phy_register_fixed_link);

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index aef739c20ac4..e59d186f78e6 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -15,9 +15,9 @@
#include <linux/mii.h>
#include <linux/phy.h>
#include <linux/phy_fixed.h>
+#include <linux/property.h>
#include <linux/err.h>
#include <linux/slab.h>
-#include <linux/of.h>
#include <linux/gpio/consumer.h>
#include <linux/idr.h>
#include <linux/netdevice.h>
@@ -186,16 +186,12 @@ static void fixed_phy_del(int phy_addr)
}
}

-#ifdef CONFIG_OF_GPIO
-static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
+static struct gpio_desc *fixed_phy_get_gpiod(struct fwnode_handle *fwnode)
{
- struct device_node *fixed_link_node;
+ struct fwnode_handle *fixed_link_node;
struct gpio_desc *gpiod;

- if (!np)
- return NULL;
-
- fixed_link_node = of_get_child_by_name(np, "fixed-link");
+ fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link");
if (!fixed_link_node)
return NULL;

@@ -204,28 +200,21 @@ static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
* Linux device associated with it, we simply have obtain
* the GPIO descriptor from the device tree like this.
*/
- gpiod = fwnode_gpiod_get_index(of_fwnode_handle(fixed_link_node),
- "link", 0, GPIOD_IN, "mdio");
+ gpiod = fwnode_gpiod_get_index(fixed_link_node, "link", 0, GPIOD_IN, "mdio");
if (IS_ERR(gpiod) && PTR_ERR(gpiod) != -EPROBE_DEFER) {
if (PTR_ERR(gpiod) != -ENOENT)
pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
fixed_link_node);
gpiod = NULL;
}
- of_node_put(fixed_link_node);
+ fwnode_handle_put(fixed_link_node);

return gpiod;
}
-#else
-static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
-{
- return NULL;
-}
-#endif

static struct phy_device *__fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np,
+ struct fwnode_handle *fwnode,
struct gpio_desc *gpiod)
{
struct fixed_mdio_bus *fmb = &platform_fmb;
@@ -238,7 +227,7 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,

/* Check if we have a GPIO associated with this fixed phy */
if (!gpiod) {
- gpiod = fixed_phy_get_gpiod(np);
+ gpiod = fixed_phy_get_gpiod(fwnode);
if (IS_ERR(gpiod))
return ERR_CAST(gpiod);
}
@@ -269,8 +258,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
phy->asym_pause = status->asym_pause;
}

- of_node_get(np);
- phy->mdio.dev.of_node = np;
+ fwnode_handle_get(fwnode);
+ device_set_node(&phy->mdio.dev, fwnode);
phy->is_pseudo_fixed_link = true;

switch (status->speed) {
@@ -299,7 +288,7 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
ret = phy_device_register(phy);
if (ret) {
phy_device_free(phy);
- of_node_put(np);
+ fwnode_handle_put(fwnode);
fixed_phy_del(phy_addr);
return ERR_PTR(ret);
}
@@ -309,9 +298,9 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,

struct phy_device *fixed_phy_register(unsigned int irq,
struct fixed_phy_status *status,
- struct device_node *np)
+ struct fwnode_handle *fwnode)
{
- return __fixed_phy_register(irq, status, np, NULL);
+ return __fixed_phy_register(irq, status, fwnode, NULL);
}
EXPORT_SYMBOL_GPL(fixed_phy_register);

@@ -327,7 +316,7 @@ EXPORT_SYMBOL_GPL(fixed_phy_register_with_gpiod);
void fixed_phy_unregister(struct phy_device *phy)
{
phy_device_remove(phy);
- of_node_put(phy->mdio.dev.of_node);
+ fwnode_handle_put(dev_fwnode(&phy->mdio.dev));
fixed_phy_del(phy->mdio.addr);
}
EXPORT_SYMBOL_GPL(fixed_phy_unregister);
--
2.29.0

2022-07-27 07:00:28

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 3/8] net: dsa: switch to device_/fwnode_ APIs

In order to support both DT and ACPI in future, modify the generic DSA
code to use device_/fwnode_ equivalent routines. Drop using port's 'dn'
field and use only fwnode - update all dependent drivers.

Because support for more generic fwnode is added, replace '_of' suffix
with '_fw' in related routines. No functional change is introduced by
this patch.

Signed-off-by: Marcin Wojtas <[email protected]>
---
include/net/dsa.h | 2 +-
net/dsa/dsa_priv.h | 4 +-
drivers/net/dsa/mt7530.c | 6 +-
drivers/net/dsa/mv88e6xxx/chip.c | 14 +--
drivers/net/dsa/qca/qca8k.c | 2 +-
drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
net/dsa/dsa2.c | 100 +++++++++++---------
net/dsa/port.c | 68 +++++++------
net/dsa/slave.c | 7 +-
9 files changed, 103 insertions(+), 102 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b902b31bebce..0a328c0073ec 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -302,7 +302,7 @@ struct dsa_port {

u8 setup:1;

- struct device_node *dn;
+ struct fwnode_handle *fwnode;
unsigned int ageing_time;

struct dsa_bridge *bridge;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d9722e49864b..2c0034a915ee 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -285,8 +285,8 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
const struct switchdev_obj_ring_role_mrp *mrp);
int dsa_port_phylink_create(struct dsa_port *dp);
-int dsa_port_link_register_of(struct dsa_port *dp);
-void dsa_port_link_unregister_of(struct dsa_port *dp);
+int dsa_port_link_register_fw(struct dsa_port *dp);
+void dsa_port_link_unregister_fw(struct dsa_port *dp);
int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 835807911be0..427b66342493 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2109,8 +2109,8 @@ mt7530_setup(struct dsa_switch *ds)
struct device_node *phy_node;
struct device_node *mac_np;
struct mt7530_dummy_poll p;
- phy_interface_t interface;
struct dsa_port *cpu_dp;
+ int interface;
u32 id, val;
int ret, i;

@@ -2232,8 +2232,8 @@ mt7530_setup(struct dsa_switch *ds)

if (!dsa_is_unused_port(ds, 5)) {
priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
- ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
- if (ret && ret != -ENODEV)
+ interface = fwnode_get_phy_mode(dsa_to_port(ds, 5)->fwnode);
+ if (interface < 0)
return ret;
} else {
/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 07e9a4da924c..a46ebdfba1c3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3275,7 +3275,7 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)

static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
{
- struct device_node *phy_handle = NULL;
+ struct fwnode_handle *phy_handle = NULL;
struct dsa_switch *ds = chip->ds;
phy_interface_t mode;
struct dsa_port *dp;
@@ -3499,15 +3499,15 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)

if (chip->info->ops->serdes_set_tx_amplitude) {
if (dp)
- phy_handle = of_parse_phandle(dp->dn, "phy-handle", 0);
+ phy_handle = fwnode_find_reference(dp->fwnode, "phy-handle", 0);

- if (phy_handle && !of_property_read_u32(phy_handle,
- "tx-p2p-microvolt",
- &tx_amp))
+ if (!IS_ERR(phy_handle) && !fwnode_property_read_u32(phy_handle,
+ "tx-p2p-microvolt",
+ &tx_amp))
err = chip->info->ops->serdes_set_tx_amplitude(chip,
port, tx_amp);
- if (phy_handle) {
- of_node_put(phy_handle);
+ if (!IS_ERR(phy_handle)) {
+ fwnode_handle_put(phy_handle);
if (err)
return err;
}
diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
index 1cbb05b0323f..77b14ade0828 100644
--- a/drivers/net/dsa/qca/qca8k.c
+++ b/drivers/net/dsa/qca/qca8k.c
@@ -1517,7 +1517,7 @@ qca8k_parse_port_config(struct qca8k_priv *priv)
continue;

dp = dsa_to_port(priv->ds, port);
- port_dn = dp->dn;
+ port_dn = to_of_node(dp->fwnode);
cpu_port_index++;

if (!of_device_is_available(port_dn))
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index da31d8b839ac..d61da012451f 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -887,7 +887,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
return -ENODEV;

dp = dsa_to_port(priv->ds, port);
- dn = dp->dn;
+ dn = to_of_node(dp->fwnode);

/* Set the RGMII TX/RX delay
*
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cac48a741f27..82fb3b009fb4 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -296,12 +296,12 @@ static void dsa_tree_put(struct dsa_switch_tree *dst)
}

static struct dsa_port *dsa_tree_find_port_by_node(struct dsa_switch_tree *dst,
- struct device_node *dn)
+ struct fwnode_handle *fwnode)
{
struct dsa_port *dp;

list_for_each_entry(dp, &dst->ports, list)
- if (dp->dn == dn)
+ if (dp->fwnode == fwnode)
return dp;

return NULL;
@@ -337,14 +337,13 @@ static bool dsa_port_setup_routing_table(struct dsa_port *dp)
{
struct dsa_switch *ds = dp->ds;
struct dsa_switch_tree *dst = ds->dst;
- struct device_node *dn = dp->dn;
struct of_phandle_iterator it;
struct dsa_port *link_dp;
struct dsa_link *dl;
int err;

- of_for_each_phandle(&it, err, dn, "link", NULL, 0) {
- link_dp = dsa_tree_find_port_by_node(dst, it.node);
+ of_for_each_phandle(&it, err, to_of_node(dp->fwnode), "link", NULL, 0) {
+ link_dp = dsa_tree_find_port_by_node(dst, of_fwnode_handle(it.node));
if (!link_dp) {
of_node_put(it.node);
return false;
@@ -469,7 +468,7 @@ static int dsa_port_setup(struct dsa_port *dp)
dsa_port_disable(dp);
break;
case DSA_PORT_TYPE_CPU:
- err = dsa_port_link_register_of(dp);
+ err = dsa_port_link_register_fw(dp);
if (err)
break;
dsa_port_link_registered = true;
@@ -481,7 +480,7 @@ static int dsa_port_setup(struct dsa_port *dp)

break;
case DSA_PORT_TYPE_DSA:
- err = dsa_port_link_register_of(dp);
+ err = dsa_port_link_register_fw(dp);
if (err)
break;
dsa_port_link_registered = true;
@@ -493,7 +492,7 @@ static int dsa_port_setup(struct dsa_port *dp)

break;
case DSA_PORT_TYPE_USER:
- of_get_mac_address(dp->dn, dp->mac);
+ fwnode_get_mac_address(dp->fwnode, dp->mac);
err = dsa_slave_create(dp);
if (err)
break;
@@ -505,7 +504,7 @@ static int dsa_port_setup(struct dsa_port *dp)
if (err && dsa_port_enabled)
dsa_port_disable(dp);
if (err && dsa_port_link_registered)
- dsa_port_link_unregister_of(dp);
+ dsa_port_link_unregister_fw(dp);
if (err) {
if (ds->ops->port_teardown)
ds->ops->port_teardown(ds, dp->index);
@@ -577,11 +576,11 @@ static void dsa_port_teardown(struct dsa_port *dp)
break;
case DSA_PORT_TYPE_CPU:
dsa_port_disable(dp);
- dsa_port_link_unregister_of(dp);
+ dsa_port_link_unregister_fw(dp);
break;
case DSA_PORT_TYPE_DSA:
dsa_port_disable(dp);
- dsa_port_link_unregister_of(dp);
+ dsa_port_link_unregister_fw(dp);
break;
case DSA_PORT_TYPE_USER:
if (dp->slave) {
@@ -853,7 +852,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
static int dsa_switch_setup(struct dsa_switch *ds)
{
struct dsa_devlink_priv *dl_priv;
- struct device_node *dn;
+ struct fwnode_handle *fwnode;
struct dsa_port *dp;
int err;

@@ -909,10 +908,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)

dsa_slave_mii_bus_init(ds);

- dn = of_get_child_by_name(ds->dev->of_node, "mdio");
+ fwnode = device_get_named_child_node(ds->dev, "mdio");

- err = of_mdiobus_register(ds->slave_mii_bus, dn);
- of_node_put(dn);
+ err = of_mdiobus_register(ds->slave_mii_bus, to_of_node(fwnode));
+ fwnode_handle_put(fwnode);
if (err < 0)
goto free_slave_mii_bus;
}
@@ -1482,24 +1481,33 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
return 0;
}

-static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
+static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode)
{
- struct device_node *ethernet = of_parse_phandle(dn, "ethernet", 0);
- const char *name = of_get_property(dn, "label", NULL);
- bool link = of_property_read_bool(dn, "link");
+ struct fwnode_handle *ethernet = fwnode_find_reference(fwnode, "ethernet", 0);
+ bool link = fwnode_property_present(fwnode, "link");
+ const char *name;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "label", &name);
+ if (ret)
+ return ret;

- dp->dn = dn;
+ dp->fwnode = fwnode;

- if (ethernet) {
+ if (!IS_ERR(ethernet)) {
struct net_device *master;
const char *user_protocol;

- master = of_find_net_device_by_node(ethernet);
- of_node_put(ethernet);
+ master = of_find_net_device_by_node(to_of_node(ethernet));
+ fwnode_handle_put(ethernet);
if (!master)
return -EPROBE_DEFER;

- user_protocol = of_get_property(dn, "dsa-tag-protocol", NULL);
+ ret = fwnode_property_read_string(fwnode, "dsa-tag-protocol",
+ &user_protocol);
+ if (ret)
+ user_protocol = NULL;
+
return dsa_port_parse_cpu(dp, master, user_protocol);
}

@@ -1509,61 +1517,61 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
return dsa_port_parse_user(dp, name);
}

-static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
- struct device_node *dn)
+static int dsa_switch_parse_ports_fw(struct dsa_switch *ds,
+ struct fwnode_handle *fwnode)
{
- struct device_node *ports, *port;
+ struct fwnode_handle *ports, *port;
struct dsa_port *dp;
int err = 0;
u32 reg;

- ports = of_get_child_by_name(dn, "ports");
+ ports = fwnode_get_named_child_node(fwnode, "ports");
if (!ports) {
/* The second possibility is "ethernet-ports" */
- ports = of_get_child_by_name(dn, "ethernet-ports");
+ ports = fwnode_get_named_child_node(fwnode, "ethernet-ports");
if (!ports) {
dev_err(ds->dev, "no ports child node found\n");
return -EINVAL;
}
}

- for_each_available_child_of_node(ports, port) {
- err = of_property_read_u32(port, "reg", &reg);
+ fwnode_for_each_available_child_node(ports, port) {
+ err = fwnode_property_read_u32(port, "reg", &reg);
if (err) {
- of_node_put(port);
+ fwnode_handle_put(port);
goto out_put_node;
}

if (reg >= ds->num_ports) {
dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%u)\n",
port, reg, ds->num_ports);
- of_node_put(port);
+ fwnode_handle_put(port);
err = -EINVAL;
goto out_put_node;
}

dp = dsa_to_port(ds, reg);

- err = dsa_port_parse_of(dp, port);
+ err = dsa_port_parse_fw(dp, port);
if (err) {
- of_node_put(port);
+ fwnode_handle_put(port);
goto out_put_node;
}
}

out_put_node:
- of_node_put(ports);
+ fwnode_handle_put(ports);
return err;
}

-static int dsa_switch_parse_member_of(struct dsa_switch *ds,
- struct device_node *dn)
+static int dsa_switch_parse_member_fw(struct dsa_switch *ds,
+ struct fwnode_handle *fwnode)
{
u32 m[2] = { 0, 0 };
int sz;

/* Don't error out if this optional property isn't found */
- sz = of_property_read_variable_u32_array(dn, "dsa,member", m, 2, 2);
+ sz = fwnode_property_read_u32_array(fwnode, "dsa,member", m, 2);
if (sz < 0 && sz != -EINVAL)
return sz;

@@ -1600,11 +1608,11 @@ static int dsa_switch_touch_ports(struct dsa_switch *ds)
return 0;
}

-static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn)
+static int dsa_switch_parse_fw(struct dsa_switch *ds, struct fwnode_handle *fwnode)
{
int err;

- err = dsa_switch_parse_member_of(ds, dn);
+ err = dsa_switch_parse_member_fw(ds, fwnode);
if (err)
return err;

@@ -1612,7 +1620,7 @@ static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn)
if (err)
return err;

- return dsa_switch_parse_ports_of(ds, dn);
+ return dsa_switch_parse_ports_fw(ds, fwnode);
}

static int dsa_port_parse(struct dsa_port *dp, const char *name,
@@ -1705,20 +1713,20 @@ static int dsa_switch_probe(struct dsa_switch *ds)
{
struct dsa_switch_tree *dst;
struct dsa_chip_data *pdata;
- struct device_node *np;
+ struct fwnode_handle *fwnode;
int err;

if (!ds->dev)
return -ENODEV;

pdata = ds->dev->platform_data;
- np = ds->dev->of_node;
+ fwnode = dev_fwnode(ds->dev);

if (!ds->num_ports)
return -EINVAL;

- if (np) {
- err = dsa_switch_parse_of(ds, np);
+ if (fwnode) {
+ err = dsa_switch_parse_fw(ds, fwnode);
if (err)
dsa_switch_release_ports(ds);
} else if (pdata) {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2dd76eb1621c..40c7d1d9b488 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -6,10 +6,9 @@
* Vivien Didelot <[email protected]>
*/

+#include <linux/fwnode_mdio.h>
#include <linux/if_bridge.h>
#include <linux/notifier.h>
-#include <linux/of_mdio.h>
-#include <linux/of_net.h>

#include "dsa_priv.h"

@@ -1380,20 +1379,20 @@ void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,

static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
{
- struct device_node *phy_dn;
+ struct fwnode_handle *phy_handle;
struct phy_device *phydev;

- phy_dn = of_parse_phandle(dp->dn, "phy-handle", 0);
- if (!phy_dn)
+ phy_handle = fwnode_find_reference(dp->fwnode, "phy-handle", 0);
+ if (IS_ERR(phy_handle))
return NULL;

- phydev = of_phy_find_device(phy_dn);
+ phydev = fwnode_phy_find_device(phy_handle);
if (!phydev) {
- of_node_put(phy_dn);
+ fwnode_handle_put(phy_handle);
return ERR_PTR(-EPROBE_DEFER);
}

- of_node_put(phy_dn);
+ fwnode_handle_put(phy_handle);
return phydev;
}

@@ -1525,11 +1524,10 @@ static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
int dsa_port_phylink_create(struct dsa_port *dp)
{
struct dsa_switch *ds = dp->ds;
- phy_interface_t mode;
- int err;
+ int mode;

- err = of_get_phy_mode(dp->dn, &mode);
- if (err)
+ mode = fwnode_get_phy_mode(dp->fwnode);
+ if (mode < 0)
mode = PHY_INTERFACE_MODE_NA;

/* Presence of phylink_mac_link_state or phylink_mac_an_restart is
@@ -1542,7 +1540,7 @@ int dsa_port_phylink_create(struct dsa_port *dp)
if (ds->ops->phylink_get_caps)
ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);

- dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
+ dp->pl = phylink_create(&dp->pl_config, dp->fwnode,
mode, &dsa_port_phylink_mac_ops);
if (IS_ERR(dp->pl)) {
pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
@@ -1552,7 +1550,7 @@ int dsa_port_phylink_create(struct dsa_port *dp)
return 0;
}

-static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
+static int dsa_port_setup_phy_fw(struct dsa_port *dp, bool enable)
{
struct dsa_switch *ds = dp->ds;
struct phy_device *phydev;
@@ -1590,16 +1588,15 @@ static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
return err;
}

-static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
+static int dsa_port_fixed_link_register_fw(struct dsa_port *dp)
{
- struct device_node *dn = dp->dn;
struct dsa_switch *ds = dp->ds;
struct phy_device *phydev;
int port = dp->index;
- phy_interface_t mode;
+ int mode;
int err;

- err = of_phy_register_fixed_link(dn);
+ err = fwnode_phy_register_fixed_link(dp->fwnode);
if (err) {
dev_err(ds->dev,
"failed to register the fixed PHY of port %d\n",
@@ -1607,10 +1604,10 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
return err;
}

- phydev = of_phy_find_device(dn);
+ phydev = fwnode_phy_find_device(dp->fwnode);

- err = of_get_phy_mode(dn, &mode);
- if (err)
+ mode = fwnode_get_phy_mode(dp->fwnode);
+ if (mode < 0)
mode = PHY_INTERFACE_MODE_NA;
phydev->interface = mode;

@@ -1627,7 +1624,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
static int dsa_port_phylink_register(struct dsa_port *dp)
{
struct dsa_switch *ds = dp->ds;
- struct device_node *port_dn = dp->dn;
int err;

dp->pl_config.dev = ds->dev;
@@ -1637,7 +1633,7 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
if (err)
return err;

- err = phylink_of_phy_connect(dp->pl, port_dn, 0);
+ err = phylink_fwnode_phy_connect(dp->pl, dp->fwnode, 0);
if (err && err != -ENODEV) {
pr_err("could not attach to PHY: %d\n", err);
goto err_phy_connect;
@@ -1650,35 +1646,35 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
return err;
}

-int dsa_port_link_register_of(struct dsa_port *dp)
+int dsa_port_link_register_fw(struct dsa_port *dp)
{
+ struct fwnode_handle *phy_handle;
struct dsa_switch *ds = dp->ds;
- struct device_node *phy_np;
int port = dp->index;

if (!ds->ops->adjust_link) {
- phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
- if (of_phy_is_fixed_link(dp->dn) || phy_np) {
+ phy_handle = fwnode_find_reference(dp->fwnode, "phy-handle", 0);
+ if (fwnode_phy_is_fixed_link(dp->fwnode) || !IS_ERR(phy_handle)) {
if (ds->ops->phylink_mac_link_down)
ds->ops->phylink_mac_link_down(ds, port,
MLO_AN_FIXED, PHY_INTERFACE_MODE_NA);
- of_node_put(phy_np);
+ fwnode_handle_put(dp->fwnode);
return dsa_port_phylink_register(dp);
}
- of_node_put(phy_np);
+ fwnode_handle_put(dp->fwnode);
return 0;
}

dev_warn(ds->dev,
"Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");

- if (of_phy_is_fixed_link(dp->dn))
- return dsa_port_fixed_link_register_of(dp);
+ if (fwnode_phy_is_fixed_link(dp->fwnode))
+ return dsa_port_fixed_link_register_fw(dp);
else
- return dsa_port_setup_phy_of(dp, true);
+ return dsa_port_setup_phy_fw(dp, true);
}

-void dsa_port_link_unregister_of(struct dsa_port *dp)
+void dsa_port_link_unregister_fw(struct dsa_port *dp)
{
struct dsa_switch *ds = dp->ds;

@@ -1691,10 +1687,10 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
return;
}

- if (of_phy_is_fixed_link(dp->dn))
- of_phy_deregister_fixed_link(dp->dn);
+ if (fwnode_phy_is_fixed_link(dp->fwnode))
+ fwnode_phy_deregister_fixed_link(dp->fwnode);
else
- dsa_port_setup_phy_of(dp, false);
+ dsa_port_setup_phy_fw(dp, false);
}

int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ad6a6663feeb..209e24cb1477 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -10,8 +10,6 @@
#include <linux/phy.h>
#include <linux/phy_fixed.h>
#include <linux/phylink.h>
-#include <linux/of_net.h>
-#include <linux/of_mdio.h>
#include <linux/mdio.h>
#include <net/rtnetlink.h>
#include <net/pkt_cls.h>
@@ -2228,7 +2226,6 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr,
static int dsa_slave_phy_setup(struct net_device *slave_dev)
{
struct dsa_port *dp = dsa_slave_to_port(slave_dev);
- struct device_node *port_dn = dp->dn;
struct dsa_switch *ds = dp->ds;
u32 phy_flags = 0;
int ret;
@@ -2252,7 +2249,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
if (ds->ops->get_phy_flags)
phy_flags = ds->ops->get_phy_flags(ds, dp->index);

- ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
+ ret = phylink_fwnode_phy_connect(dp->pl, dp->fwnode, phy_flags);
if (ret == -ENODEV && ds->slave_mii_bus) {
/* We could not connect to a designated PHY or SFP, so try to
* use the switch internal MDIO bus instead
@@ -2364,7 +2361,7 @@ int dsa_slave_create(struct dsa_port *port)
SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);

SET_NETDEV_DEV(slave_dev, port->ds->dev);
- slave_dev->dev.of_node = port->dn;
+ device_set_node(&slave_dev->dev, port->fwnode);
slave_dev->vlan_features = master->vlan_features;

p = netdev_priv(slave_dev);
--
2.29.0

2022-07-27 07:07:57

by Marcin Wojtas

[permalink] [raw]
Subject: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

A helper function which allows getting the struct net_device pointer
associated with a given device tree node can be more generic and
also support alternative hardware description. Switch to fwnode_
and update the only existing caller in DSA subsystem.
For that purpose use newly added fwnode_dev_node_match helper routine.

Signed-off-by: Marcin Wojtas <[email protected]>
---
include/linux/etherdevice.h | 1 +
include/linux/of_net.h | 6 -----
net/core/net-sysfs.c | 25 ++++++--------------
net/dsa/dsa2.c | 3 ++-
4 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 92b10e67d5f8..a335775af244 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -35,6 +35,7 @@ int nvmem_get_mac_address(struct device *dev, void *addrbuf);
int device_get_mac_address(struct device *dev, char *addr);
int device_get_ethdev_address(struct device *dev, struct net_device *netdev);
int fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr);
+struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode);

u32 eth_get_headlen(const struct net_device *dev, const void *data, u32 len);
__be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 0484b613ca64..f672f831292d 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -15,7 +15,6 @@ struct net_device;
extern int of_get_phy_mode(struct device_node *np, phy_interface_t *interface);
extern int of_get_mac_address(struct device_node *np, u8 *mac);
int of_get_ethdev_address(struct device_node *np, struct net_device *dev);
-extern struct net_device *of_find_net_device_by_node(struct device_node *np);
#else
static inline int of_get_phy_mode(struct device_node *np,
phy_interface_t *interface)
@@ -32,11 +31,6 @@ static inline int of_get_ethdev_address(struct device_node *np, struct net_devic
{
return -ENODEV;
}
-
-static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
-{
- return NULL;
-}
#endif

#endif /* __LINUX_OF_NET_H */
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d61afd21aab5..fc972545aaea 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -6,6 +6,7 @@
*/

#include <linux/capability.h>
+#include <linux/etherdevice.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/if_arp.h>
@@ -1935,38 +1936,26 @@ static struct class net_class __ro_after_init = {
.get_ownership = net_get_ownership,
};

-#ifdef CONFIG_OF
-static int of_dev_node_match(struct device *dev, const void *data)
-{
- for (; dev; dev = dev->parent) {
- if (dev->of_node == data)
- return 1;
- }
-
- return 0;
-}
-
/*
- * of_find_net_device_by_node - lookup the net device for the device node
- * @np: OF device node
+ * fwnode_find_net_device_by_node - lookup the net device for the device fwnode
+ * @fwnode: firmware node
*
- * Looks up the net_device structure corresponding with the device node.
+ * Looks up the net_device structure corresponding with the fwnode.
* If successful, returns a pointer to the net_device with the embedded
* struct device refcount incremented by one, or NULL on failure. The
* refcount must be dropped when done with the net_device.
*/
-struct net_device *of_find_net_device_by_node(struct device_node *np)
+struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode)
{
struct device *dev;

- dev = class_find_device(&net_class, NULL, np, of_dev_node_match);
+ dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match);
if (!dev)
return NULL;

return to_net_dev(dev);
}
-EXPORT_SYMBOL(of_find_net_device_by_node);
-#endif
+EXPORT_SYMBOL(fwnode_find_net_device_by_node);

/* Delete sysfs entries but hold kobject reference until after all
* netdev references are gone.
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 82fb3b009fb4..bba416eba9c2 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -7,6 +7,7 @@
*/

#include <linux/device.h>
+#include <linux/etherdevice.h>
#include <linux/err.h>
#include <linux/list.h>
#include <linux/netdevice.h>
@@ -1498,7 +1499,7 @@ static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode)
struct net_device *master;
const char *user_protocol;

- master = of_find_net_device_by_node(to_of_node(ethernet));
+ master = fwnode_find_net_device_by_node(ethernet);
fwnode_handle_put(ethernet);
if (!master)
return -EPROBE_DEFER;
--
2.29.0

2022-07-27 10:38:55

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 2/8] net: mdio: switch fixed-link PHYs API to fwnode_

On Wed, Jul 27, 2022 at 08:43:15AM +0200, Marcin Wojtas wrote:
> fixed-link PHYs API is used by DSA and a number of drivers
> and was depending on of_. Switch to fwnode_ so to make it
> hardware description agnostic and allow to be used in ACPI
> world as well.

...

> + /* Old binding */
> + len = fwnode_property_count_u32(fwnode, "fixed-link");
> + if (len == 5)
> + return true;
> +
> + return false;

Can be

return len == 5;

or

return fwnode_...(...) == 5;

Original also good, so up to you,

...

> + if (fwnode_property_read_u32(fixed_link_node, "speed",
> + &status.speed)) {
> + fwnode_handle_put(fixed_link_node);
> + return -EINVAL;
> + }

Why shadowing actual error code?

Either

ret = fwnode_...(...);
if (ret) {
...
return ret;
}

or add a comment explaining the above magic transformations.

...

> + /* Old binding */
> + if (fwnode_property_read_u32_array(fwnode, "fixed-link", fixed_link_prop,
> + ARRAY_SIZE(fixed_link_prop)) == 0) {
> + status.link = 1;
> + status.duplex = fixed_link_prop[1];
> + status.speed = fixed_link_prop[2];
> + status.pause = fixed_link_prop[3];
> + status.asym_pause = fixed_link_prop[4];
> + goto register_phy;
> + }
> +
> + return -ENODEV;

Ditto.

--
With Best Regards,
Andy Shevchenko


2022-07-27 10:42:46

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 1/8] net: phy: fixed_phy: switch to fwnode_ API

On Wed, Jul 27, 2022 at 08:43:14AM +0200, Marcin Wojtas wrote:
> This patch allows to use fixed_phy driver and its helper

Check "Submitting Patches" for "This patch..." and fix your message
accordingly.

> functions without Device Tree dependency, by swtiching from
> of_ to fwnode_ API.

--
With Best Regards,
Andy Shevchenko


2022-07-27 11:35:40

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 3/8] net: dsa: switch to device_/fwnode_ APIs

On Wed, Jul 27, 2022 at 08:43:16AM +0200, Marcin Wojtas wrote:
> In order to support both DT and ACPI in future, modify the generic DSA
> code to use device_/fwnode_ equivalent routines. Drop using port's 'dn'
> field and use only fwnode - update all dependent drivers.
>
> Because support for more generic fwnode is added, replace '_of' suffix
> with '_fw' in related routines. No functional change is introduced by
> this patch.

...

> static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> {
> - struct device_node *phy_handle = NULL;
> + struct fwnode_handle *phy_handle = NULL;
> struct dsa_switch *ds = chip->ds;
> phy_interface_t mode;
> struct dsa_port *dp;
> @@ -3499,15 +3499,15 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>
> if (chip->info->ops->serdes_set_tx_amplitude) {
> if (dp)
> - phy_handle = of_parse_phandle(dp->dn, "phy-handle", 0);
> + phy_handle = fwnode_find_reference(dp->fwnode, "phy-handle", 0);
>
> - if (phy_handle && !of_property_read_u32(phy_handle,
> - "tx-p2p-microvolt",
> - &tx_amp))
> + if (!IS_ERR(phy_handle) && !fwnode_property_read_u32(phy_handle,
> + "tx-p2p-microvolt",
> + &tx_amp))
> err = chip->info->ops->serdes_set_tx_amplitude(chip,
> port, tx_amp);
> - if (phy_handle) {
> - of_node_put(phy_handle);
> + if (!IS_ERR(phy_handle)) {
> + fwnode_handle_put(phy_handle);
> if (err)
> return err;
> }

I believe after 002752af7b89 ("device property: Allow error pointer to be
passed to fwnode APIs") you may simplify above like:

if (!fwnode_property_read_u32(phy_handle, "tx-p2p-microvolt",
&tx_amp))
err = chip->info->ops->serdes_set_tx_amplitude(chip,
port, tx_amp);
else
err = 0;
fwnode_handle_put(phy_handle);
if (err)
return err;

It also possible you can do refactoring before/after this one.

--
With Best Regards,
Andy Shevchenko


2022-07-27 11:36:26

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 5/8] device property: introduce fwnode_find_parent_dev_match

On Wed, Jul 27, 2022 at 08:43:18AM +0200, Marcin Wojtas wrote:
> This patch adds a new generic routine fwnode_find_parent_dev_match

"This patch..."

> that can be used e.g. as a callback for class_find_device().
> It searches for the struct device corresponding to a
> struct fwnode_handle by iterating over device and
> its parents.

--
With Best Regards,
Andy Shevchenko


2022-07-27 13:46:51

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 7/8] net: mdio: introduce fwnode_mdiobus_register_device()

On Wed, Jul 27, 2022 at 08:43:20AM +0200, Marcin Wojtas wrote:
> As a preparation patch to extend MDIO capabilities in the ACPI world,
> introduce fwnode_mdiobus_register_device() to register non-PHY
> devices on the mdiobus.

...

> + dev_dbg(&mdio->dev, "registered mdio device %p fwnode at address %i\n",
> + child, addr);

"%p" makes a little sense (and we have hashed pointers). I think the "%pfw"
would be much better for the user to see.

...

> - dev_dbg(&mdio->dev, "registered mdio device %pOFn at address %i\n",
> - child, addr);

Exactly my point above.

--
With Best Regards,
Andy Shevchenko


2022-07-27 13:47:42

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 8/8] net: dsa: mv88e6xxx: switch to device_/fwnode_ APIs

On Wed, Jul 27, 2022 at 08:43:21AM +0200, Marcin Wojtas wrote:
> In order to support both DT and ACPI in future, modify the
> mv88e6xx driver code to use device_/fwnode_ equivalent routines.
> No functional change is introduced by this patch.

...

> static int mv88e6xxx_probe(struct mdio_device *mdiodev)
> {
> struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;

> + struct fwnode_handle *fwnode = dev_fwnode(&mdiodev->dev);

Move this...

> const struct mv88e6xxx_info *compat_info = NULL;
> struct device *dev = &mdiodev->dev;
> - struct device_node *np = dev->of_node;

...here as

struct fwnode_handle *fwnode = dev_fwnode(dev);

> struct mv88e6xxx_chip *chip;
> int port;
> int err;

> + if (fwnode)

Redundant check.

> + compat_info = device_get_match_data(dev);
> + else if (pdata) {
> compat_info = pdata_device_get_match_data(dev);

compat_info - device_get_match_data(dev);
if (!compat_info && pdata)
compat_info = pdata_device_get_match_data(dev);

...

> + if (fwnode)
> + device_property_read_u32(dev, "eeprom-length",
> + &chip->eeprom_len);
> else
> chip->eeprom_len = pdata->eeprom_len;

Can be done w/o conditional

chip->eeprom_len = pdata->eeprom_len;
device_property_read_u32(dev, "eeprom-length", &chip->eeprom_len);

--
With Best Regards,
Andy Shevchenko


2022-07-27 15:10:28

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote:
> A helper function which allows getting the struct net_device pointer
> associated with a given device tree node can be more generic and
> also support alternative hardware description. Switch to fwnode_
> and update the only existing caller in DSA subsystem.
> For that purpose use newly added fwnode_dev_node_match helper routine.
>
> Signed-off-by: Marcin Wojtas <[email protected]>
> ---
> -struct net_device *of_find_net_device_by_node(struct device_node *np)
> +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode)
> {
> struct device *dev;
>
> - dev = class_find_device(&net_class, NULL, np, of_dev_node_match);
> + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match);

This needs to maintain compatibility with DSA masters that have
dev->of_node but don't have dev->fwnode populated.

> if (!dev)
> return NULL;
>
> return to_net_dev(dev);
> }
> -EXPORT_SYMBOL(of_find_net_device_by_node);
> -#endif
> +EXPORT_SYMBOL(fwnode_find_net_device_by_node);

2022-07-27 15:47:46

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

śr., 27 lip 2022 o 16:31 Vladimir Oltean <[email protected]> napisał(a):
>
> On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote:
> > A helper function which allows getting the struct net_device pointer
> > associated with a given device tree node can be more generic and
> > also support alternative hardware description. Switch to fwnode_
> > and update the only existing caller in DSA subsystem.
> > For that purpose use newly added fwnode_dev_node_match helper routine.
> >
> > Signed-off-by: Marcin Wojtas <[email protected]>
> > ---
> > -struct net_device *of_find_net_device_by_node(struct device_node *np)
> > +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode)
> > {
> > struct device *dev;
> >
> > - dev = class_find_device(&net_class, NULL, np, of_dev_node_match);
> > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match);
>
> This needs to maintain compatibility with DSA masters that have
> dev->of_node but don't have dev->fwnode populated.
>

Do you mean a situation analogous to what I addressed in:
[net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
?

I found indeed a couple of drivers that may require a similar change
(e.g. dpaa2).

IMO we have 2 options:
- update these drivers
- add some kind of fallback? If yes, I am wondering about an elegant
solution - maybe add an extra check inside
fwnode_find_parent_dev_match?

What would you suggest?

Best regards,
Marcin

> > if (!dev)
> > return NULL;
> >
> > return to_net_dev(dev);
> > }
> > -EXPORT_SYMBOL(of_find_net_device_by_node);
> > -#endif
> > +EXPORT_SYMBOL(fwnode_find_net_device_by_node);

2022-07-27 18:17:36

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote:
> Do you mean a situation analogous to what I addressed in:
> [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
> ?

Not sure if "analogous" is the right word. My estimation is that the
overwhelmingly vast majority of DSA masters can be found by DSA simply
due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make
anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2:
initialize port of_node pointer"), but that isn't needed in general, and
I can't tell you exactly why it is needed there, I don't know enough
about the mvpp2 driver.

> I found indeed a couple of drivers that may require a similar change
> (e.g. dpaa2).

There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node,
but I'd rather not go into an explanation that essentially doesn't matter.
The point is that you'd be mistaken to think that only the drivers which
touch the net device's ->dev->of_node are the ones that need updating
for your series to not cause regressions.

> IMO we have 2 options:
> - update these drivers
> - add some kind of fallback? If yes, I am wondering about an elegant
> solution - maybe add an extra check inside
> fwnode_find_parent_dev_match?
>
> What would you suggest?

Fixing fwnode_find_parent_dev_match(), of course. This change broke DSA
on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/)
and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c).

2022-07-27 18:35:18

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Wed, Jul 27, 2022 at 5:24 PM Marcin Wojtas <[email protected]> wrote:
> śr., 27 lip 2022 o 16:31 Vladimir Oltean <[email protected]> napisał(a):
> > On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote:

...

> > > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match);
> >
> > This needs to maintain compatibility with DSA masters that have
> > dev->of_node but don't have dev->fwnode populated.
>
> Do you mean a situation analogous to what I addressed in:
> [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
> ?
>
> I found indeed a couple of drivers that may require a similar change
> (e.g. dpaa2).
>
> IMO we have 2 options:
> - update these drivers

Not Vladimir here, but my 2cents that update is best and elegant, it
can be done even before this series.

--
With Best Regards,
Andy Shevchenko

2022-07-27 19:00:27

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

śr., 27 lip 2022 o 18:38 Vladimir Oltean <[email protected]> napisał(a):
>
> On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote:
> > Do you mean a situation analogous to what I addressed in:
> > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
> > ?
>
> Not sure if "analogous" is the right word. My estimation is that the
> overwhelmingly vast majority of DSA masters can be found by DSA simply
> due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make
> anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2:
> initialize port of_node pointer"), but that isn't needed in general, and
> I can't tell you exactly why it is needed there, I don't know enough
> about the mvpp2 driver.

SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev
and in most cases it is sufficient apparently it is sufficient for
fwnode_find_parent_dev_match (at least tests with mvneta case proves
it's fine).

We have some corner cases though:
* mvpp2 -> single controller can handle up to 3 net_devices and
therefore we need device_set_node() to make this work. I think dpaa2
is a similar case
* PCIE drivers with extra DT description (I think that's the case of enetc).

>
> > I found indeed a couple of drivers that may require a similar change
> > (e.g. dpaa2).
>
> There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node,
> but I'd rather not go into an explanation that essentially doesn't matter.
> The point is that you'd be mistaken to think that only the drivers which
> touch the net device's ->dev->of_node are the ones that need updating
> for your series to not cause regressions.

As above - SET_NETDEV_DEV() should be fine in most cases, but we can
never be 100% sure untils it's verified.

>
> > IMO we have 2 options:
> > - update these drivers
> > - add some kind of fallback? If yes, I am wondering about an elegant
> > solution - maybe add an extra check inside
> > fwnode_find_parent_dev_match?
> >
> > What would you suggest?
>
> Fixing fwnode_find_parent_dev_match(), of course. This change broke DSA
> on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/)
> and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c).

Can you please check applying following diff:

--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
* The routine can be used e.g. as a callback for class_find_device().
*
* Returns: %1 - match is found
* %0 - match not found
*/
int fwnode_find_parent_dev_match(struct device *dev, const void *data)
{
for (; dev; dev = dev->parent) {
if (device_match_fwnode(dev, data))
return 1;
+ else if (device_match_of_node(dev, to_of_node(data))
+ return 1;
}

return 0;
}
EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match);

Thanks for the review and test.
Marcin

2022-07-27 19:24:50

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

śr., 27 lip 2022 o 19:00 Andy Shevchenko <[email protected]> napisał(a):
>
> On Wed, Jul 27, 2022 at 5:24 PM Marcin Wojtas <[email protected]> wrote:
> > śr., 27 lip 2022 o 16:31 Vladimir Oltean <[email protected]> napisał(a):
> > > On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote:
>
> ...
>
> > > > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match);
> > >
> > > This needs to maintain compatibility with DSA masters that have
> > > dev->of_node but don't have dev->fwnode populated.
> >
> > Do you mean a situation analogous to what I addressed in:
> > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
> > ?
> >
> > I found indeed a couple of drivers that may require a similar change
> > (e.g. dpaa2).
> >
> > IMO we have 2 options:
> > - update these drivers
>
> Not Vladimir here, but my 2cents that update is best and elegant, it
> can be done even before this series.
>

In general I agree it's desired, but I'm not sure if we can catch all
cases just by reading code or rather base on regression reports
later...

Best regards,
Marcin

2022-07-27 21:33:48

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Wed, Jul 27, 2022 at 07:40:00PM +0200, Marcin Wojtas wrote:
> SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev
> and in most cases it is sufficient apparently it is sufficient for
> fwnode_find_parent_dev_match (at least tests with mvneta case proves
> it's fine).

Indeed, mvneta works, which is a plain old platform device that hasn't
even been converted to fwnode, so why don't the others?

Well, as it turns out, it's one of the cases where I spoke to soon,
thinking I knew what was the problem why probing failed, before actually
debugging.

I thought there was no dmesg output from DSA at all, which would have
indicated an eternal -EPROBE_DEFER situation. But there's one tiny line
I had overlooked:

[ 5.094013] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch

This comes from here:

static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode)
{
struct fwnode_handle *ethernet = fwnode_find_reference(fwnode, "ethernet", 0);
bool link = fwnode_property_present(fwnode, "link");
const char *name = NULL;
int ret;

ret = fwnode_property_read_string(fwnode, "label", &name);
// if (ret)
// return ret;

dp->fwnode = fwnode;

The 'label' property of a port was optional, you've made it mandatory by accident.
It is used only by DSA drivers that register using platform data.

(side note, I can't believe you actually have a 'label' property for the
CPU port and how many people are in the same situation as you; you know
it isn't used for anything, right? how do we stop the cargo cult?)

> Can you please check applying following diff:
>
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
> * The routine can be used e.g. as a callback for class_find_device().
> *
> * Returns: %1 - match is found
> * %0 - match not found
> */
> int fwnode_find_parent_dev_match(struct device *dev, const void *data)
> {
> for (; dev; dev = dev->parent) {
> if (device_match_fwnode(dev, data))
> return 1;
> + else if (device_match_of_node(dev, to_of_node(data))
> + return 1;
> }
>
> return 0;
> }
> EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match);

So, nothing to do with device_match_fwnode() failing, that would have
been strange, come to think about it. Sorry for the noise here.

But looking at the deeper implementation of dev_fwnode() as:

struct fwnode_handle *dev_fwnode(struct device *dev)
{
return IS_ENABLED(CONFIG_OF) && dev->of_node ?
of_fwnode_handle(dev->of_node) : dev->fwnode;
}

I wonder, why did you have to modify mvpp2? It looks at dev->of_node
prior to returning dev->fwnode. It should work.

2022-07-27 22:01:33

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Thu, Jul 28, 2022 at 12:11:12AM +0300, Vladimir Oltean wrote:
> The 'label' property of a port was optional, you've made it mandatory by accident.
> It is used only by DSA drivers that register using platform data.

I didn't mean to say exactly this, the second phrase was an addition
through which I tried to make it clear that the "cpu" label *is* used,
but only by the drivers using platform data. With OF it's not, neither
that nor label = "dsa". We have other means of recognizing a CPU or DSA
port, by the 'ethernet' and/or 'dsa' phandles.

Additionally, the label is optional even for user port. One can have
udev rules that assign names to Ethernet ports. I think that is even
encouraged; some of the things in DSA predate the establishment of some
best practices.

2022-07-28 06:52:28

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

śr., 27 lip 2022 o 23:11 Vladimir Oltean <[email protected]> napisał(a):
>
> On Wed, Jul 27, 2022 at 07:40:00PM +0200, Marcin Wojtas wrote:
> > SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev
> > and in most cases it is sufficient apparently it is sufficient for
> > fwnode_find_parent_dev_match (at least tests with mvneta case proves
> > it's fine).
>
> Indeed, mvneta works, which is a plain old platform device that hasn't
> even been converted to fwnode, so why don't the others?
>
> Well, as it turns out, it's one of the cases where I spoke to soon,
> thinking I knew what was the problem why probing failed, before actually
> debugging.
>
> I thought there was no dmesg output from DSA at all, which would have
> indicated an eternal -EPROBE_DEFER situation. But there's one tiny line
> I had overlooked:
>
> [ 5.094013] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch
>
> This comes from here:
>
> static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode)
> {
> struct fwnode_handle *ethernet = fwnode_find_reference(fwnode, "ethernet", 0);
> bool link = fwnode_property_present(fwnode, "link");
> const char *name = NULL;
> int ret;
>
> ret = fwnode_property_read_string(fwnode, "label", &name);
> // if (ret)
> // return ret;
>
> dp->fwnode = fwnode;
>
> The 'label' property of a port was optional, you've made it mandatory by accident.
> It is used only by DSA drivers that register using platform data.

Thanks for spotting that, I will make it optional again.

>
> (side note, I can't believe you actually have a 'label' property for the
> CPU port and how many people are in the same situation as you; you know
> it isn't used for anything, right? how do we stop the cargo cult?)
>

Well, given these results:
~/git/linux : git grep 'label = \"cpu\"' arch/arm/boot/dts/ | wc -l
79
~/git/linux : git grep 'label = \"cpu\"' arch/arm64/boot/dts/ | wc -l
14

It's not a surprise I also have it defined in the platforms I test. I
agree it doesn't serve any purpose in terms of creating the devices in
DSA with DT, but it IMO increases readability of the description at
least.

> > Can you please check applying following diff:
> >
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
> > * The routine can be used e.g. as a callback for class_find_device().
> > *
> > * Returns: %1 - match is found
> > * %0 - match not found
> > */
> > int fwnode_find_parent_dev_match(struct device *dev, const void *data)
> > {
> > for (; dev; dev = dev->parent) {
> > if (device_match_fwnode(dev, data))
> > return 1;
> > + else if (device_match_of_node(dev, to_of_node(data))
> > + return 1;
> > }
> >
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match);
>
> So, nothing to do with device_match_fwnode() failing, that would have
> been strange, come to think about it. Sorry for the noise here.
>

Great, thank you for confirmation.

> But looking at the deeper implementation of dev_fwnode() as:
>
> struct fwnode_handle *dev_fwnode(struct device *dev)
> {
> return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> of_fwnode_handle(dev->of_node) : dev->fwnode;
> }
>
> I wonder, why did you have to modify mvpp2? It looks at dev->of_node
> prior to returning dev->fwnode. It should work.

When I boot with ACPI, the dev->of_node becomes NULL. With DT it's fine as-is.

Best regards,
Marcin

2022-07-28 07:04:42

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

śr., 27 lip 2022 o 23:15 Andy Shevchenko <[email protected]> napisał(a):
>
>
>
> On Wednesday, July 27, 2022, Marcin Wojtas <[email protected]> wrote:
>>
>> śr., 27 lip 2022 o 18:38 Vladimir Oltean <[email protected]> napisał(a):
>> >
>> > On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote:
>> > > Do you mean a situation analogous to what I addressed in:
>> > > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer
>> > > ?
>> >
>> > Not sure if "analogous" is the right word. My estimation is that the
>> > overwhelmingly vast majority of DSA masters can be found by DSA simply
>> > due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make
>> > anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2:
>> > initialize port of_node pointer"), but that isn't needed in general, and
>> > I can't tell you exactly why it is needed there, I don't know enough
>> > about the mvpp2 driver.
>>
>> SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev
>> and in most cases it is sufficient apparently it is sufficient for
>> fwnode_find_parent_dev_match (at least tests with mvneta case proves
>> it's fine).
>>
>> We have some corner cases though:
>> * mvpp2 -> single controller can handle up to 3 net_devices and
>> therefore we need device_set_node() to make this work. I think dpaa2
>> is a similar case
>> * PCIE drivers with extra DT description (I think that's the case of enetc).
>>
>> >
>> > > I found indeed a couple of drivers that may require a similar change
>> > > (e.g. dpaa2).
>> >
>> > There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node,
>> > but I'd rather not go into an explanation that essentially doesn't matter.
>> > The point is that you'd be mistaken to think that only the drivers which
>> > touch the net device's ->dev->of_node are the ones that need updating
>> > for your series to not cause regressions.
>>
>> As above - SET_NETDEV_DEV() should be fine in most cases, but we can
>> never be 100% sure untils it's verified.
>>
>> >
>> > > IMO we have 2 options:
>> > > - update these drivers
>> > > - add some kind of fallback? If yes, I am wondering about an elegant
>> > > solution - maybe add an extra check inside
>> > > fwnode_find_parent_dev_match?
>> > >
>> > > What would you suggest?
>> >
>> > Fixing fwnode_find_parent_dev_match(), of course.
>
>
>
> Fixing how?!
>
>
>>
>>
>> This change broke DSA
>> > on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/)
>> > and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c).
>>
>> Can you please check applying following diff:
>>
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
>> * The routine can be used e.g. as a callback for class_find_device().
>> *
>> * Returns: %1 - match is found
>> * %0 - match not found
>> */
>> int fwnode_find_parent_dev_match(struct device *dev, const void *data)
>> {
>> for (; dev; dev = dev->parent) {
>> if (device_match_fwnode(dev, data))
>> return 1;
>> + else if (device_match_of_node(dev, to_of_node(data))
>> + return 1;
>> }
>>
>
> This adds a piece of dead code. device_match_fwnode() covers this already.
>

Yes, indeed. After recent update, I think we can assume the current
implementation of fwnode_find_parent_dev_match should work fine with
all existing cases.

Thank you for all remarks and comments, I'll address them in v4 later today.

Best regards,
Marcin

2022-07-28 08:46:11

by Chen, Rong A

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 3/8] net: dsa: switch to device_/fwnode_ APIs

Hi Marcin,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20220726]
[cannot apply to driver-core/driver-core-testing robh/for-next
horms-ipvs/master linus/master v5.19-rc8 v5.19-rc7 v5.19-rc6 v5.19-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Marcin-Wojtas/DSA-switch-to-fwnode_-device_/20220727-144515
base: 058affafc65a74cf54499fb578b66ad0b18f939b
:::::: branch date: 24 hours ago
:::::: commit date: 24 hours ago
config: i386-allyesconfig
(https://download.01.org/0day-ci/archive/20220728/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
#
https://github.com/intel-lab-lkp/linux/commit/0cd0cba4df268433a47eb7d7e6c4b657dac14cbc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Marcin-Wojtas/DSA-switch-to-fwnode_-device_/20220727-144515
git checkout 0cd0cba4df268433a47eb7d7e6c4b657dac14cbc
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/net/dsa/mt7530.c: In function 'mt7530_setup':
>> drivers/net/dsa/mt7530.c:2254:63: error: passing argument 2 of 'of_get_phy_mode' from incompatible pointer type [-Werror=incompatible-pointer-types]
2254 | ret =
of_get_phy_mode(mac_np, &interface);
|
^~~~~~~~~~
| |
|
int *
In file included from drivers/net/dsa/mt7530.c:15:
include/linux/of_net.h:15:69: note: expected 'phy_interface_t *' but
argument is of type 'int *'
15 | extern int of_get_phy_mode(struct device_node *np,
phy_interface_t *interface);
|
~~~~~~~~~~~~~~~~~^~~~~~~~~
cc1: some warnings being treated as errors


vim +/of_get_phy_mode +2254 drivers/net/dsa/mt7530.c

ba751e28d442557 DENG Qingfang 2021-05-19 2103 b8f126a8d54318b
Sean Wang 2017-04-07 2104 static int
b8f126a8d54318b Sean Wang 2017-04-07 2105
mt7530_setup(struct dsa_switch *ds)
b8f126a8d54318b Sean Wang 2017-04-07 2106 {
b8f126a8d54318b Sean Wang 2017-04-07 2107 struct
mt7530_priv *priv = ds->priv;
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2108 struct
device_node *dn = NULL;
38f790a805609b2 Ren? van Dorst 2019-09-02 2109 struct
device_node *phy_node;
38f790a805609b2 Ren? van Dorst 2019-09-02 2110 struct
device_node *mac_np;
b8f126a8d54318b Sean Wang 2017-04-07 2111 struct
mt7530_dummy_poll p;
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2112 struct dsa_port
*cpu_dp;
0cd0cba4df26843 Marcin Wojtas 2022-07-27 2113 int interface;
ca366d6c889b5d3 Ren? van Dorst 2019-09-02 2114 u32 id, val;
ca366d6c889b5d3 Ren? van Dorst 2019-09-02 2115 int ret, i;
b8f126a8d54318b Sean Wang 2017-04-07 2116 0abfd494deefdba
Vivien Didelot 2017-09-20 2117 /* The parent node of master
netdev which holds the common system
b8f126a8d54318b Sean Wang 2017-04-07 2118 * controller
also is the container for two GMACs nodes representing
b8f126a8d54318b Sean Wang 2017-04-07 2119 * as two netdev
instances.
b8f126a8d54318b Sean Wang 2017-04-07 2120 */
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2121
dsa_switch_for_each_cpu_port(cpu_dp, ds) {
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2122 dn =
cpu_dp->master->dev.of_node->parent;
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2123 /* It doesn't
matter which CPU port is found first,
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2124 * their masters
should share the same parent OF node
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2125 */
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2126 break;
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2127 }
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2128 6e19bc26cccdd34
Frank Wunderlich 2022-06-10 2129 if (!dn) {
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2130 dev_err(ds->dev,
"parent OF node of DSA master not found");
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2131 return -EINVAL;
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2132 }
6e19bc26cccdd34 Frank Wunderlich 2022-06-10 2133 0b69c54c74bcb60
DENG Qingfang 2021-08-04 2134 ds->assisted_learning_on_cpu_port
= true;
771c8901568dd87 DENG Qingfang 2020-12-11 2135
ds->mtu_enforcement_ingress = true;
ddda1ac116c852b Greg Ungerer 2019-01-30 2136 ddda1ac116c852b
Greg Ungerer 2019-01-30 2137 if (priv->id == ID_MT7530) {
b8f126a8d54318b Sean Wang 2017-04-07 2138
regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
b8f126a8d54318b Sean Wang 2017-04-07 2139 ret =
regulator_enable(priv->core_pwr);
b8f126a8d54318b Sean Wang 2017-04-07 2140 if (ret < 0) {
b8f126a8d54318b Sean Wang 2017-04-07 2141 dev_err(priv->dev,
b8f126a8d54318b Sean Wang 2017-04-07 2142 "Failed to
enable core power: %d\n", ret);
b8f126a8d54318b Sean Wang 2017-04-07 2143 return ret;
b8f126a8d54318b Sean Wang 2017-04-07 2144 }
b8f126a8d54318b Sean Wang 2017-04-07 2145 b8f126a8d54318b
Sean Wang 2017-04-07 2146
regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
b8f126a8d54318b Sean Wang 2017-04-07 2147 ret =
regulator_enable(priv->io_pwr);
b8f126a8d54318b Sean Wang 2017-04-07 2148 if (ret < 0) {
b8f126a8d54318b Sean Wang 2017-04-07 2149
dev_err(priv->dev, "Failed to enable io pwr: %d\n",
b8f126a8d54318b Sean Wang 2017-04-07 2150 ret);
b8f126a8d54318b Sean Wang 2017-04-07 2151 return ret;
b8f126a8d54318b Sean Wang 2017-04-07 2152 }
ddda1ac116c852b Greg Ungerer 2019-01-30 2153 }
b8f126a8d54318b Sean Wang 2017-04-07 2154 b8f126a8d54318b
Sean Wang 2017-04-07 2155 /* Reset whole chip through gpio
pin or memory-mapped registers for
b8f126a8d54318b Sean Wang 2017-04-07 2156 * different type
of hardware
b8f126a8d54318b Sean Wang 2017-04-07 2157 */
b8f126a8d54318b Sean Wang 2017-04-07 2158 if (priv->mcm) {
b8f126a8d54318b Sean Wang 2017-04-07 2159
reset_control_assert(priv->rstc);
b8f126a8d54318b Sean Wang 2017-04-07 2160
usleep_range(1000, 1100);
b8f126a8d54318b Sean Wang 2017-04-07 2161
reset_control_deassert(priv->rstc);
b8f126a8d54318b Sean Wang 2017-04-07 2162 } else {
b8f126a8d54318b Sean Wang 2017-04-07 2163
gpiod_set_value_cansleep(priv->reset, 0);
b8f126a8d54318b Sean Wang 2017-04-07 2164
usleep_range(1000, 1100);
b8f126a8d54318b Sean Wang 2017-04-07 2165
gpiod_set_value_cansleep(priv->reset, 1);
b8f126a8d54318b Sean Wang 2017-04-07 2166 }
b8f126a8d54318b Sean Wang 2017-04-07 2167 b8f126a8d54318b
Sean Wang 2017-04-07 2168 /* Waiting for MT7530 got to
stable */
b8f126a8d54318b Sean Wang 2017-04-07 2169
INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
b8f126a8d54318b Sean Wang 2017-04-07 2170 ret =
readx_poll_timeout(_mt7530_read, &p, val, val != 0,
b8f126a8d54318b Sean Wang 2017-04-07 2171 20, 1000000);
b8f126a8d54318b Sean Wang 2017-04-07 2172 if (ret < 0) {
b8f126a8d54318b Sean Wang 2017-04-07 2173
dev_err(priv->dev, "reset timeout\n");
b8f126a8d54318b Sean Wang 2017-04-07 2174 return ret;
b8f126a8d54318b Sean Wang 2017-04-07 2175 }
b8f126a8d54318b Sean Wang 2017-04-07 2176 b8f126a8d54318b
Sean Wang 2017-04-07 2177 id = mt7530_read(priv, MT7530_CREV);
b8f126a8d54318b Sean Wang 2017-04-07 2178 id >>=
CHIP_NAME_SHIFT;
b8f126a8d54318b Sean Wang 2017-04-07 2179 if (id !=
MT7530_ID) {
b8f126a8d54318b Sean Wang 2017-04-07 2180
dev_err(priv->dev, "chip %x can't be supported\n", id);
b8f126a8d54318b Sean Wang 2017-04-07 2181 return -ENODEV;
b8f126a8d54318b Sean Wang 2017-04-07 2182 }
b8f126a8d54318b Sean Wang 2017-04-07 2183 b8f126a8d54318b
Sean Wang 2017-04-07 2184 /* Reset the switch through
internal reset */
b8f126a8d54318b Sean Wang 2017-04-07 2185
mt7530_write(priv, MT7530_SYS_CTRL,
b8f126a8d54318b Sean Wang 2017-04-07 2186
SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
b8f126a8d54318b Sean Wang 2017-04-07 2187
SYS_CTRL_REG_RST);
b8f126a8d54318b Sean Wang 2017-04-07 2188 b8f126a8d54318b
Sean Wang 2017-04-07 2189 /* Enable Port 6 only; P5 as
GMAC5 which currently is not supported */
b8f126a8d54318b Sean Wang 2017-04-07 2190 val =
mt7530_read(priv, MT7530_MHWTRAP);
b8f126a8d54318b Sean Wang 2017-04-07 2191 val &=
~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
b8f126a8d54318b Sean Wang 2017-04-07 2192 val |=
MHWTRAP_MANUAL;
b8f126a8d54318b Sean Wang 2017-04-07 2193
mt7530_write(priv, MT7530_MHWTRAP, val);
b8f126a8d54318b Sean Wang 2017-04-07 2194 ca366d6c889b5d3
Ren? van Dorst 2019-09-02 2195 priv->p6_interface =
PHY_INTERFACE_MODE_NA;
ca366d6c889b5d3 Ren? van Dorst 2019-09-02 2196 b8f126a8d54318b
Sean Wang 2017-04-07 2197 /* Enable and reset MIB counters */
b8f126a8d54318b Sean Wang 2017-04-07 2198 mt7530_mib_reset(ds);
b8f126a8d54318b Sean Wang 2017-04-07 2199 b8f126a8d54318b
Sean Wang 2017-04-07 2200 for (i = 0; i < MT7530_NUM_PORTS;
i++) {
b8f126a8d54318b Sean Wang 2017-04-07 2201 /* Disable
forwarding by default on all ports */
b8f126a8d54318b Sean Wang 2017-04-07 2202 mt7530_rmw(priv,
MT7530_PCR_P(i), PCR_MATRIX_MASK,
b8f126a8d54318b Sean Wang 2017-04-07 2203 PCR_MATRIX_CLR);
b8f126a8d54318b Sean Wang 2017-04-07 2204 0b69c54c74bcb60
DENG Qingfang 2021-08-04 2205 /* Disable learning by default
on all ports */
0b69c54c74bcb60 DENG Qingfang 2021-08-04 2206 mt7530_set(priv,
MT7530_PSC_P(i), SA_DIS);
0b69c54c74bcb60 DENG Qingfang 2021-08-04 2207 0ce0c3cd2239502
Alex Dewar 2020-09-19 2208 if (dsa_is_cpu_port(ds, i)) {
0ce0c3cd2239502 Alex Dewar 2020-09-19 2209 ret =
mt753x_cpu_port_enable(ds, i);
0ce0c3cd2239502 Alex Dewar 2020-09-19 2210 if (ret)
0ce0c3cd2239502 Alex Dewar 2020-09-19 2211 return ret;
5a30833b9a16f8d DENG Qingfang 2021-03-16 2212 } else {
75104db0cb353ec Andrew Lunn 2019-02-24 2213
mt7530_port_disable(ds, i);
6087175b7991a90 DENG Qingfang 2021-08-04 2214 6087175b7991a90
DENG Qingfang 2021-08-04 2215 /* Set default PVID to 0 on all
user ports */
6087175b7991a90 DENG Qingfang 2021-08-04 2216
mt7530_rmw(priv, MT7530_PPBV1_P(i), G0_PORT_VID_MASK,
6087175b7991a90 DENG Qingfang 2021-08-04 2217
G0_PORT_VID_DEF);
5a30833b9a16f8d DENG Qingfang 2021-03-16 2218 }
e045124e93995fe DENG Qingfang 2020-04-14 2219 /* Enable
consistent egress tag */
e045124e93995fe DENG Qingfang 2020-04-14 2220 mt7530_rmw(priv,
MT7530_PVC_P(i), PVC_EG_TAG_MASK,
e045124e93995fe DENG Qingfang 2020-04-14 2221
PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
b8f126a8d54318b Sean Wang 2017-04-07 2222 }
b8f126a8d54318b Sean Wang 2017-04-07 2223 1ca8a193cade7f4
DENG Qingfang 2021-08-25 2224 /* Setup VLAN ID 0 for
VLAN-unaware bridges */
1ca8a193cade7f4 DENG Qingfang 2021-08-25 2225 ret =
mt7530_setup_vlan0(priv);
1ca8a193cade7f4 DENG Qingfang 2021-08-25 2226 if (ret)
1ca8a193cade7f4 DENG Qingfang 2021-08-25 2227 return ret;
1ca8a193cade7f4 DENG Qingfang 2021-08-25 2228 38f790a805609b2
Ren? van Dorst 2019-09-02 2229 /* Setup port 5 */
38f790a805609b2 Ren? van Dorst 2019-09-02 2230 priv->p5_intf_sel
= P5_DISABLED;
38f790a805609b2 Ren? van Dorst 2019-09-02 2231 interface =
PHY_INTERFACE_MODE_NA;
38f790a805609b2 Ren? van Dorst 2019-09-02 2232 38f790a805609b2
Ren? van Dorst 2019-09-02 2233 if (!dsa_is_unused_port(ds, 5)) {
38f790a805609b2 Ren? van Dorst 2019-09-02 2234
priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
0cd0cba4df26843 Marcin Wojtas 2022-07-27 2235 interface =
fwnode_get_phy_mode(dsa_to_port(ds, 5)->fwnode);
0cd0cba4df26843 Marcin Wojtas 2022-07-27 2236 if (interface < 0)
0c65b2b90d13c1d Andrew Lunn 2019-11-04 2237 return ret;
38f790a805609b2 Ren? van Dorst 2019-09-02 2238 } else {
38f790a805609b2 Ren? van Dorst 2019-09-02 2239 /* Scan the
ethernet nodes. look for GMAC1, lookup used phy */
38f790a805609b2 Ren? van Dorst 2019-09-02 2240
for_each_child_of_node(dn, mac_np) {
38f790a805609b2 Ren? van Dorst 2019-09-02 2241 if
(!of_device_is_compatible(mac_np,
38f790a805609b2 Ren? van Dorst 2019-09-02 2242
"mediatek,eth-mac"))
38f790a805609b2 Ren? van Dorst 2019-09-02 2243 continue;
38f790a805609b2 Ren? van Dorst 2019-09-02 2244 38f790a805609b2
Ren? van Dorst 2019-09-02 2245 ret =
of_property_read_u32(mac_np, "reg", &id);
38f790a805609b2 Ren? van Dorst 2019-09-02 2246 if (ret < 0 ||
id != 1)
38f790a805609b2 Ren? van Dorst 2019-09-02 2247 continue;
38f790a805609b2 Ren? van Dorst 2019-09-02 2248 38f790a805609b2
Ren? van Dorst 2019-09-02 2249 phy_node =
of_parse_phandle(mac_np, "phy-handle", 0);
0452800f6db4ed0 Chuanhong Guo 2020-04-03 2250 if (!phy_node)
0452800f6db4ed0 Chuanhong Guo 2020-04-03 2251 continue;
0452800f6db4ed0 Chuanhong Guo 2020-04-03 2252 38f790a805609b2
Ren? van Dorst 2019-09-02 2253 if (phy_node->parent ==
priv->dev->of_node->parent) {
0c65b2b90d13c1d Andrew Lunn 2019-11-04 @2254 ret =
of_get_phy_mode(mac_np, &interface);
8e4efd4706f77d7 Sumera Priyadarsini 2020-08-25 2255 if (ret && ret
!= -ENODEV) {
8e4efd4706f77d7 Sumera Priyadarsini 2020-08-25 2256
of_node_put(mac_np);
a9e9b091a1c14ec Yang Yingliang 2022-04-28 2257
of_node_put(phy_node);
0c65b2b90d13c1d Andrew Lunn 2019-11-04 2258 return ret;
8e4efd4706f77d7 Sumera Priyadarsini 2020-08-25 2259 }
38f790a805609b2 Ren? van Dorst 2019-09-02 2260 id =
of_mdio_parse_addr(ds->dev, phy_node);
38f790a805609b2 Ren? van Dorst 2019-09-02 2261 if (id == 0)
38f790a805609b2 Ren? van Dorst 2019-09-02 2262
priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
38f790a805609b2 Ren? van Dorst 2019-09-02 2263 if (id == 4)
38f790a805609b2 Ren? van Dorst 2019-09-02 2264
priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
38f790a805609b2 Ren? van Dorst 2019-09-02 2265 }
8e4efd4706f77d7 Sumera Priyadarsini 2020-08-25 2266
of_node_put(mac_np);
38f790a805609b2 Ren? van Dorst 2019-09-02 2267
of_node_put(phy_node);
38f790a805609b2 Ren? van Dorst 2019-09-02 2268 break;
38f790a805609b2 Ren? van Dorst 2019-09-02 2269 }
38f790a805609b2 Ren? van Dorst 2019-09-02 2270 }
38f790a805609b2 Ren? van Dorst 2019-09-02 2271
--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-07-28 10:01:20

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Thu, Jul 28, 2022 at 08:52:04AM +0200, Marcin Wojtas wrote:
> Yes, indeed. After recent update, I think we can assume the current
> implementation of fwnode_find_parent_dev_match should work fine with
> all existing cases.

What you should really be fixing is the commit message of patch 4,
that's what threw me off:

| As a preparation to switch the DSA subsystem from using
| of_find_net_device_by_node() to its more generic fwnode_
| equivalent, the port's device structure should be updated
| with its fwnode pointer, similarly to of_node - see analogous
| commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer").
|
| This patch is required to prevent a regression before updating
| the DSA API on boards that connect the mvpp2 port to switch,
| such as Clearfog GT-8K or CN913x CEx7 Evaluation Board.

There's no regression to speak of. DSA didn't work with ACPI before, and
fwnode_find_net_device_by_node() still works with the plain dev->of_node
assignment.

2022-07-28 17:43:19

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

czw., 28 lip 2022 o 11:16 Vladimir Oltean <[email protected]> napisał(a):
>
> On Thu, Jul 28, 2022 at 08:52:04AM +0200, Marcin Wojtas wrote:
> > Yes, indeed. After recent update, I think we can assume the current
> > implementation of fwnode_find_parent_dev_match should work fine with
> > all existing cases.
>
> What you should really be fixing is the commit message of patch 4,
> that's what threw me off:
>
> | As a preparation to switch the DSA subsystem from using
> | of_find_net_device_by_node() to its more generic fwnode_
> | equivalent, the port's device structure should be updated
> | with its fwnode pointer, similarly to of_node - see analogous
> | commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer").
> |
> | This patch is required to prevent a regression before updating
> | the DSA API on boards that connect the mvpp2 port to switch,
> | such as Clearfog GT-8K or CN913x CEx7 Evaluation Board.
>
> There's no regression to speak of. DSA didn't work with ACPI before, and
> fwnode_find_net_device_by_node() still works with the plain dev->of_node
> assignment.

There was a regression even for OF in v1, but after switching to
device_match_fwnode() it works indeed. Anyway patch v4 is imo useful,
I'll only reword the commit message.

Thanks,
Marcin

2022-07-28 19:52:49

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Thu, Jul 28, 2022 at 06:56:48PM +0200, Marcin Wojtas wrote:
> There was a regression even for OF in v1, but after switching to
> device_match_fwnode() it works indeed. Anyway patch v4 is imo useful,
> I'll only reword the commit message.

Do you mean patch 4 or patch v4? If patch 4, of course it's useful, but
not for avoiding a regression with OF (case in which I drop all my claims
made earlier about fw_find_net_device_by_node), but rather to actually
get something working with actual ACPI (although perhaps not in this
series, you'll need to add ACPI IDs in the mv88e6xxx driver some time
later as well, maybe you could focus this series just on converting DSA
to play nice with fwnodes). If you're already thinking about the v4 of
this patch set, I'll respond to that in a separate email shortly.

2022-07-28 19:56:29

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

czw., 28 lip 2022 o 21:16 Vladimir Oltean <[email protected]> napisał(a):
>
> On Thu, Jul 28, 2022 at 06:56:48PM +0200, Marcin Wojtas wrote:
> > There was a regression even for OF in v1, but after switching to
> > device_match_fwnode() it works indeed. Anyway patch v4 is imo useful,
> > I'll only reword the commit message.
>
> Do you mean patch 4 or patch v4? If patch 4, of course it's useful, but

Patch 4/8 in v4 :) I'm working on it right now to submit asap.

> not for avoiding a regression with OF (case in which I drop all my claims
> made earlier about fw_find_net_device_by_node), but rather to actually

Change in the mvpp2 driver:
- dev->dev.of_node = port_node;
+ device_set_node(&dev->dev, port_fwnode);
is desired and correct anyway, so as a low-cost change I think it can
be included in this series (which is in fact preparation-to-ACPI
support). I will update the commit message. accordingly.

> get something working with actual ACPI (although perhaps not in this
> series, you'll need to add ACPI IDs in the mv88e6xxx driver some time

v1 added all of this, but we agreed that ACPI-specific bits should be
sent separately later, after extending the ACPI Specification.

> later as well, maybe you could focus this series just on converting DSA
> to play nice with fwnodes). If you're already thinking about the v4 of
> this patch set, I'll respond to that in a separate email shortly.

2022-07-28 19:57:07

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Thu, Jul 28, 2022 at 08:47:58AM +0200, Marcin Wojtas wrote:
> > The 'label' property of a port was optional, you've made it mandatory by accident.
> > It is used only by DSA drivers that register using platform data.
>
> Thanks for spotting that, I will make it optional again.
>
> > (side note, I can't believe you actually have a 'label' property for the
> > CPU port and how many people are in the same situation as you; you know
> > it isn't used for anything, right? how do we stop the cargo cult?)
>
> Well, given these results:
> ~/git/linux : git grep 'label = \"cpu\"' arch/arm/boot/dts/ | wc -l
> 79
> ~/git/linux : git grep 'label = \"cpu\"' arch/arm64/boot/dts/ | wc -l
> 14
>
> It's not a surprise I also have it defined in the platforms I test. I
> agree it doesn't serve any purpose in terms of creating the devices in
> DSA with DT, but it IMO increases readability of the description at
> least.

We've glided over this way too easily, so I'll repeat this thing I've said:

| One can have udev rules that assign names to Ethernet ports. I think
| that is even encouraged; some of the things in DSA predate the
| establishment of some best practices.

I know I'm not exactly "upfront" by saying this at v3 rather than earlier,
but I haven't had the time and I still don't have as much as I'd like.
Sorry for that.

Please don't jump to sending v4 just yet, and please don't expect that
this patch set will make it for the upcoming 5.20 release candidates.

ACPI is a whole new world and I don't think we want to mass-migrate each
and every OF binding that DSA has to the generic fwnode form, at least
not without having a serious discussion about it.

The 'label' thing is actually one of the things that I'm seriously
considering skipping parsing if this is an ACPI system, simply because
best practices are different today than they were when the OF bindings
were created.

There's also the change that validates that phylink has the fwnode
properties it needs to work properly:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

Please don't even think that the DSA fwnode conversion will be merged
before the validation patch (sorry, I'm not saying this to block you,
I'm saying this because I don't want DSA to start with zero-day baggage
on ACPI).

And even when the validation patch gets merged, you'll need to adapt it
to fwnode because that's what will be required syntactically, but we'll
only go through the motions of calling of_device_compatible_match() for
the OF case. With ACPI, every driver will opt into strict validation,
that's non negotiable.

And then there are some other issues we've learned about, with the DT
bindings that specific drivers such as mv88e6xxx and realtek-smi have.
I'll give you more details once we get to the actual mv88e6xxx
conversion to ACPI; currently my memory lacks some of the precise
details of how come mv88e6xxx came to not observe the issue but
realtek-smi did. Anyway, the issue was that fw_devlink causes the
internal PHYs to probe with the generic rather than the specific PHY
driver, if interrupts are being used (and provided by the switch as
'interrupt-controller').

It can be debated what exactly is at fault there, although one
interpretation can be that the DT bindings themselves are to blame,
for describing a circular dependency between a parent and a child.
I've been suggesting the authors of new drivers to take an alternative
approach and describe the switch chip as a MFD, with only the actual
switching component being probed by DSA and the rest being separate
drivers:
https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/

You'll say, ok but don't we have to keep maintaining mv8e6xxx OF bindings
functional with fw_devlink too anyway, so what benefit would there be if
for ACPI we'd split the exact same monolithic driver into a MFD?
And maybe you have a point, I don't know, I haven't actually tried to
look at the code and see if it could be restructured cleanly to probe
and work in both cases.

The bottom line is that if you haven't received too much review for your
series until now, I suspect it's because none of the DSA maintainers
cropped a large enough chunk of time yet to actually clarify things for
themselves. I don't consider the things I've pointed out here to be a
'review' in the proper sense, they're just cases I'm thinking about in
the back of my mind where we should learn from past mistakes.
I'll revisit when I will have come to some conclusions.

2022-07-28 20:22:50

by Andrew Lunn

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

> The 'label' thing is actually one of the things that I'm seriously
> considering skipping parsing if this is an ACPI system, simply because
> best practices are different today than they were when the OF bindings
> were created.

Agreed. We want the ACPI binding to learn from what has worked and not
worked in DT. We should clean up some of the historical mess. And
enforce things we don't in DT simply because there is too much
history.

So a straight one to one conversion is not going to happen.

> It can be debated what exactly is at fault there, although one
> interpretation can be that the DT bindings themselves are to blame,
> for describing a circular dependency between a parent and a child.

DT describes hardware. I'm not sure hardware can have a circular
dependency. It is more about how software make use of that hardware
description that ends up in circular dependencies.

Andrew

2022-07-28 21:25:39

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

czw., 28 lip 2022 o 22:18 Andrew Lunn <[email protected]> napisał(a):
>
> > The 'label' thing is actually one of the things that I'm seriously
> > considering skipping parsing if this is an ACPI system, simply because
> > best practices are different today than they were when the OF bindings
> > were created.
>
> Agreed. We want the ACPI binding to learn from what has worked and not
> worked in DT. We should clean up some of the historical mess. And
> enforce things we don't in DT simply because there is too much
> history.
>
> So a straight one to one conversion is not going to happen.

I understand your standpoint - there is a long history, possible
clean-ups, backward compatibility considerations, etc. that should not
be zero-day baggage of ACPI. Otoh, we don't need to be worried about
the ACPI binding too much now - as agreed it was removed from this
series, beginning from v2. IMO it may be better to return to that once
the ACPI Spec is updated with the MDIOSerialBus and the patches are
resubmitted on whatever shape of the DSA subsystem is established
within the next weeks/months from now.

In v1 we discussed also the resubmission of the non-ACPI-related
patches, which would pave the way to dropping the explicit OF_
dependency in the DSA and moving to a generic hardware description
kernel API - without any functional change. Modifying DT bindings and
clean-ups could be done on top this patchset as well. Of course, it is
the subsystems' Maintainers call and I'll adjust accordingly - if you
wish me to wait and rebase after the 'validation patch' lands in
net-next, I'll do that.

A side note: I was of course aware that making it for the v5.20 would
be extremely hard, but I decided to give it a try anyway - I had to
wait for some time, as this series was gated by fate of the eventually
abandoned phylink-related changes.

Thanks,
Marcin

2022-07-28 22:29:44

by Andrew Lunn

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Thu, Jul 28, 2022 at 11:23:31PM +0200, Marcin Wojtas wrote:
> czw., 28 lip 2022 o 22:18 Andrew Lunn <[email protected]> napisał(a):
> >
> > > The 'label' thing is actually one of the things that I'm seriously
> > > considering skipping parsing if this is an ACPI system, simply because
> > > best practices are different today than they were when the OF bindings
> > > were created.
> >
> > Agreed. We want the ACPI binding to learn from what has worked and not
> > worked in DT. We should clean up some of the historical mess. And
> > enforce things we don't in DT simply because there is too much
> > history.
> >
> > So a straight one to one conversion is not going to happen.
>
> I understand your standpoint - there is a long history, possible
> clean-ups, backward compatibility considerations, etc. that should not
> be zero-day baggage of ACPI. Otoh, we don't need to be worried about
> the ACPI binding too much now - as agreed it was removed from this
> series, beginning from v2. IMO it may be better to return to that once
> the ACPI Spec is updated with the MDIOSerialBus and the patches are
> resubmitted on whatever shape of the DSA subsystem is established
> within the next weeks/months from now.
>
> In v1 we discussed also the resubmission of the non-ACPI-related
> patches, which would pave the way to dropping the explicit OF_
> dependency in the DSA and moving to a generic hardware description
> kernel API - without any functional change.

Ideally, we want to keep all the ugly DT stuff in DT. We want to
ensure that any "generic hardware description kernel API" does not
inherit all the ugly DT stuff.

ACPI and DT are different things, so i don't see why they need to
share code.

Andrew

2022-07-29 10:23:10

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next: PATCH v3 6/8] net: core: switch to fwnode_find_net_device_by_node()

On Fri, Jul 29, 2022 at 12:29 AM Andrew Lunn <[email protected]> wrote:
> On Thu, Jul 28, 2022 at 11:23:31PM +0200, Marcin Wojtas wrote:

...

> ACPI and DT are different things, so i don't see why they need to
> share code.

Yes and no.

ACPI _DSD extension allows us to share a lot of code when it comes to
specific device properties (that are not standardized anyhow by ACPI
specification, and for the record many of them even may not, but some
are, like MIPI camera). Maybe you want to have something like a
property standard for DSA that can be published as MIPI or so and then
that part of the code of course may very well be shared. Discussed
MDIOSerialBus() resource type is pure ACPI thingy if going to be
standardized and indeed, that part can't be shared.

Entire exercise with fwnodes allows to make drivers (most of or most
of the parts of) to be shared between different resource providers.
And this is a cool part about it.

--
With Best Regards,
Andy Shevchenko