Hi!
This patchset introduces the support for DSA in ACPI world. A couple of
words about the background and motivation behind those changes:
The DSA code is strictly dependent on the Device Tree and Open Firmware
(of_*) interface, both in the drivers and the common high-level net/dsa API.
The only alternative is to pass the information about the topology via
platform data - a legacy approach used by older systems that compiled the
board description into the kernel.
The above constraint is problematic for the embedded devices based e.g. on
x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks
and workarounds have to be applied. Addition of switch description to
DSDT/SSDT tables would help to solve many similar cases and use unmodified
kernel modules. It also enables this feature for ARM64 ACPI users.
The key enablements allowing for adding ACPI support for DSA in Linux were
NIC drivers, MDIO, PHY, and phylink modifications – the latter three merged
in 2021. I thought it would be worth to experiment with DSA, which seemed
to be a natural follow-up challenge.
It turned out that without much hassle it is possible to describe
DSA-compliant switches as child devices of the MDIO busses, which are
responsible for their enumeration based on the standard _ADR fields and
description in _DSD objects under 'device properties' UUID [1].
The vast majority of required changes were simple of_* to fwnode_*
transition, as the DT and ACPI topolgies are analogous, except for
'ports' and 'mdio' subnodes naming, as they don't conform ACPI
namespace constraints [2].
The patchset can be logically split to subsets of commits:
* Move a couple of missing routines to fwnode_ equivalents
* Rework net/dsa to use fwnode_*/device_* API
* Introduce fwnode_mdiobus_register_device() and allow MDIO device probing
in ACPI world.
* Add necessary ACPI-related modifications to net/dsa and add Documentation
entry.
* Shift example mv88e6xxx driver to fwnode_*/device_* and add ACPI hooks.
The changes details can be found in the commit messages.
Note that for now cascade topology remains unsupported in ACPI world
(based on "dsa" label and "link" property values). It seems to be feasible,
but would extend this patchset due to necessity of of_phandle_iterator
migration to fwnode_. Leave it as a possible future step.
Testing:
* EACH commit was tested against regression with device tree on EspressoBIN
and SolidRun CN913x CEx7 Evaluation board. It works as expected throughout
entire patchset.
* The latter board was used as example ACPI user of the feature - it's 1:1
to what's available when booting with DT. Please check [3] and [4] to
compare the DT/ACPI description.
For convenience, this patchset is also available on a public branch [5].
I am looking forward to any comments or remarks, your review will be
appreciated.
Best regards,
Marcin
[1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
[2] https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#acpi-namespace
[3] https://github.com/semihalf-wojtas-marcin/edk2-platforms/commit/6368ee09a232c1348e19729f21c05e9c5410cdb9
[4] https://github.com/tianocore/edk2-non-osi/blob/master/Silicon/Marvell/OcteonTx/DeviceTree/T91/cn9130-cex7.dts#L252
[5] https://github.com/semihalf-wojtas-marcin/Linux-Kernel/commits/dsa-acpi-v1
Marcin Wojtas (12):
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
net: core: switch to fwnode_find_net_device_by_node()
net: mdio: introduce fwnode_mdiobus_register_device()
net: mdio: allow registering non-PHY devices in ACPI world
ACPI: scan: prevent double enumeration of MDIO bus children
Documentation: ACPI: DSD: introduce DSA description
net: dsa: add ACPI support
net: dsa: mv88e6xxx: switch to device_/fwnode_ APIs
net: dsa: mv88e6xxx: add ACPI support
include/linux/etherdevice.h | 1 +
include/linux/fwnode_mdio.h | 22 ++
include/linux/of_net.h | 6 -
include/linux/phy_fixed.h | 4 +-
include/net/dsa.h | 1 +
drivers/acpi/scan.c | 15 +
drivers/net/dsa/mv88e6xxx/chip.c | 76 +++--
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 +
drivers/net/mdio/acpi_mdio.c | 40 ++-
drivers/net/mdio/fwnode_mdio.c | 129 +++++++
drivers/net/mdio/of_mdio.c | 105 +-----
drivers/net/phy/fixed_phy.c | 37 +-
drivers/net/phy/mdio_bus.c | 4 +
net/core/net-sysfs.c | 18 +-
net/dsa/dsa2.c | 104 ++++--
net/dsa/port.c | 54 ++-
net/dsa/slave.c | 6 +-
Documentation/firmware-guide/acpi/dsd/dsa.rst | 359 ++++++++++++++++++++
Documentation/firmware-guide/acpi/index.rst | 1 +
19 files changed, 748 insertions(+), 235 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/dsa.rst
--
2.29.0
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.
Signed-off-by: Marcin Wojtas <[email protected]>
---
include/linux/etherdevice.h | 1 +
include/linux/of_net.h | 6 ------
net/core/net-sysfs.c | 18 ++++++++----------
net/dsa/dsa2.c | 3 ++-
4 files changed, 11 insertions(+), 17 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 d49fc974e630..837f67f1f3a4 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>
@@ -1934,11 +1935,10 @@ 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)
+static int fwnode_dev_node_match(struct device *dev, const void *data)
{
for (; dev; dev = dev->parent) {
- if (dev->of_node == data)
+ if (dev->fwnode == data)
return 1;
}
@@ -1946,26 +1946,24 @@ static int of_dev_node_match(struct device *dev, const void *data)
}
/*
- * 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 node
+ * @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_dev_node_match);
if (!dev)
return NULL;
return to_net_dev(dev);
}
-EXPORT_SYMBOL(of_find_net_device_by_node);
-#endif
/* 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 039022bf914b..5e11d66f9057 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>
@@ -1500,7 +1501,7 @@ static int dsa_port_parse_of(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
This patch utilizes the newly added fwnode_mdiobus_register_device
function and enables registration of non-PHY MDIO devices.
For that purpose a helper routine is added, allowing to determine,
whether the device associated to ACPI node is a PHY.
In addition to that update, allow matching child devices' drivers
based on their ACPI ID.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/mdio/acpi_mdio.c | 40 +++++++++++++++++++-
drivers/net/phy/mdio_bus.c | 4 ++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/net/mdio/acpi_mdio.c b/drivers/net/mdio/acpi_mdio.c
index d77c987fda9c..b5d7404afc5e 100644
--- a/drivers/net/mdio/acpi_mdio.c
+++ b/drivers/net/mdio/acpi_mdio.c
@@ -17,6 +17,41 @@
MODULE_AUTHOR("Calvin Johnson <[email protected]>");
MODULE_LICENSE("GPL");
+/**
+ * acpi_mdiobus_child_is_phy - check if device associated with fwnode is a PHY.
+ * @fwnode: pointer to MDIO bus child fwnode and is expected to represent ACPI
+ * device object.
+ *
+ * The function returns true if the child node is for a PHY.
+ * It must comprise either:
+ * o Compatible string of "ethernet-phy-idX.X"
+ * o Compatible string of "ethernet-phy-ieee802.3-c45"
+ * o Compatible string of "ethernet-phy-ieee802.3-c22"
+ * o No _HID or _CID fields.
+ */
+static bool acpi_mdiobus_child_is_phy(struct fwnode_handle *child)
+{
+ struct acpi_device *adev = to_acpi_device_node(child);
+ u32 phy_id;
+
+ if (fwnode_get_phy_id(child, &phy_id) != -EINVAL)
+ return true;
+
+ if (fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c45") == 0)
+ return true;
+
+ if (fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c22") == 0)
+ return true;
+
+ /* Default to PHY if no _HID or _CID found in the fwnode. */
+ if (list_empty(&adev->pnp.ids))
+ return true;
+
+ return false;
+}
+
/**
* acpi_mdiobus_register - Register mii_bus and create PHYs from the ACPI ASL.
* @mdio: pointer to mii_bus structure
@@ -47,7 +82,10 @@ int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
if (ret || addr >= PHY_MAX_ADDR)
continue;
- ret = fwnode_mdiobus_register_phy(mdio, child, addr);
+ if (acpi_mdiobus_child_is_phy(child))
+ ret = fwnode_mdiobus_register_phy(mdio, child, addr);
+ else
+ ret = fwnode_mdiobus_register_device(mdio, child, addr);
if (ret == -ENODEV)
dev_err(&mdio->dev,
"MDIO device at address %d is missing.\n",
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8a2dbe849866..b3c2f966be4b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -8,6 +8,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/acpi.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/errno.h>
@@ -989,6 +990,9 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
if (of_driver_match_device(dev, drv))
return 1;
+ if (acpi_driver_match_device(dev, drv))
+ return 1;
+
if (mdio->bus_match)
return mdio->bus_match(dev, drv);
--
2.29.0
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.
While at it, also use the newly introduced fwnode operation in
of_mdiobus_phy_device_register().
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 | 26 +-----------------
3 files changed, 33 insertions(+), 25 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 b1c20c48b6cb..97abfaf88030 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..522dbee419fe 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -51,31 +51,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
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;
+ return fwnode_mdiobus_register_device(mdio, of_fwnode_handle(child), addr);
}
/* The following is a list of PHY compatible strings which appear in
--
2.29.0
On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote:
> Hi!
>
> This patchset introduces the support for DSA in ACPI world. A couple of
> words about the background and motivation behind those changes:
>
> The DSA code is strictly dependent on the Device Tree and Open Firmware
> (of_*) interface, both in the drivers and the common high-level net/dsa API.
> The only alternative is to pass the information about the topology via
> platform data - a legacy approach used by older systems that compiled the
> board description into the kernel.
>
> The above constraint is problematic for the embedded devices based e.g. on
> x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks
> and workarounds have to be applied. Addition of switch description to
> DSDT/SSDT tables would help to solve many similar cases and use unmodified
> kernel modules. It also enables this feature for ARM64 ACPI users.
>
> The key enablements allowing for adding ACPI support for DSA in Linux were
> NIC drivers, MDIO, PHY, and phylink modifications – the latter three merged
> in 2021. I thought it would be worth to experiment with DSA, which seemed
> to be a natural follow-up challenge.
>
> It turned out that without much hassle it is possible to describe
> DSA-compliant switches as child devices of the MDIO busses, which are
> responsible for their enumeration based on the standard _ADR fields and
> description in _DSD objects under 'device properties' UUID [1].
> The vast majority of required changes were simple of_* to fwnode_*
> transition, as the DT and ACPI topolgies are analogous, except for
> 'ports' and 'mdio' subnodes naming, as they don't conform ACPI
> namespace constraints [2].
...
> Note that for now cascade topology remains unsupported in ACPI world
> (based on "dsa" label and "link" property values). It seems to be feasible,
> but would extend this patchset due to necessity of of_phandle_iterator
> migration to fwnode_. Leave it as a possible future step.
Wondering if this can be done using fwnode graph.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 05:02:18PM +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.
...
> +static int fwnode_dev_node_match(struct device *dev, const void *data)
> {
> for (; dev; dev = dev->parent) {
> - if (dev->of_node == data)
> + if (dev->fwnode == data)
We have a helper in device/bus.h (?) device_match_fwnode().
> return 1;
> }
But this all sounds like a good candidate to be generic. Do we have more users
in the kernel of a such?
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 05:02:19PM +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.
>
> While at it, also use the newly introduced fwnode operation in
> of_mdiobus_phy_device_register().
...
> static int of_mdiobus_register_device(struct mii_bus *mdio,
> struct device_node *child, u32 addr)
> {
> + return fwnode_mdiobus_register_device(mdio, of_fwnode_handle(child), addr);
> }
Since it's static one-liner you probably may ger rid of it completely.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 07:55:44PM +0200, Andrew Lunn wrote:
> On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote:
...
> > It turned out that without much hassle it is possible to describe
> > DSA-compliant switches as child devices of the MDIO busses, which are
> > responsible for their enumeration based on the standard _ADR fields and
> > description in _DSD objects under 'device properties' UUID [1].
>
> No surprises there. That is how the DT binding works. And the current
> ACPI concept is basically DT in different words. Maybe the more
> important question is, is rewording DT in ACPI the correct approach,
> or should you bo doing a more native ACPI implementation? I cannot
> answer that, you need to ask the ACPI maintainers.
You beat me up to this. I also was about to mention that the problem with such
conversions (like this series does) is not in the code. It's simplest part. The
problem is bindings and how you get them to be a standard (at least de facto).
> > Note that for now cascade topology remains unsupported in ACPI world
> > (based on "dsa" label and "link" property values). It seems to be feasible,
> > but would extend this patchset due to necessity of of_phandle_iterator
> > migration to fwnode_. Leave it as a possible future step.
>
> We really do need to ensure this is possible. You are setting an ABI
> here, which everybody else in the ACPI world needs to follow. Cascaded
> switches is fundamental to DSA, it is the D in DSA. So i would prefer
> that you at least define and document the binding for D in DSA and get
> it sanity checked by the ACPI people.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote:
> Hi!
>
> This patchset introduces the support for DSA in ACPI world. A couple of
> words about the background and motivation behind those changes:
>
> The DSA code is strictly dependent on the Device Tree and Open Firmware
> (of_*) interface, both in the drivers and the common high-level net/dsa API.
> The only alternative is to pass the information about the topology via
> platform data - a legacy approach used by older systems that compiled the
> board description into the kernel.
Not true. There are deployed x86 systems which do this, and they are
fully up to date, not legacy. There are however limitations in what
you can do. So please drop this wording.
> The above constraint is problematic for the embedded devices based e.g. on
> x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks
> and workarounds have to be applied.
It would be good to describe the limitations. As i said, there are x86
systems running with marvell 6390 switches.
> It turned out that without much hassle it is possible to describe
> DSA-compliant switches as child devices of the MDIO busses, which are
> responsible for their enumeration based on the standard _ADR fields and
> description in _DSD objects under 'device properties' UUID [1].
No surprises there. That is how the DT binding works. And the current
ACPI concept is basically DT in different words. Maybe the more
important question is, is rewording DT in ACPI the correct approach,
or should you bo doing a more native ACPI implementation? I cannot
answer that, you need to ask the ACPI maintainers.
> Note that for now cascade topology remains unsupported in ACPI world
> (based on "dsa" label and "link" property values). It seems to be feasible,
> but would extend this patchset due to necessity of of_phandle_iterator
> migration to fwnode_. Leave it as a possible future step.
We really do need to ensure this is possible. You are setting an ABI
here, which everybody else in the ACPI world needs to follow. Cascaded
switches is fundamental to DSA, it is the D in DSA. So i would prefer
that you at least define and document the binding for D in DSA and get
it sanity checked by the ACPI people.
Andrew
> You beat me up to this. I also was about to mention that the problem with such
> conversions (like this series does) is not in the code. It's simplest part. The
> problem is bindings and how you get them to be a standard (at least de facto).
De facto is easy. Get it merged. After that, i will simply refuse
anything else, the same way i and other Maintainers would refuse a
different DT binding.
If the ACPI committee approve and publish a binding, we will naturally
accept that as well. So in the end we might have two bindings. But so
far in this whole ACPI for networking story, i've not heard anybody
say they are going to submit anything for standardisation. So this
might be a mute point.
Andrew
Hi Marcin,
I love your patch! Yet something to improve:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on robh/for-next linus/master v5.19-rc2 next-20220617]
[cannot apply to horms-ipvs/master]
[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]
url: https://github.com/intel-lab-lkp/linux/commits/Marcin-Wojtas/ACPI-support-for-DSA/20220620-231646
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20220621/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/68a7a52989207bfe8640877c512c77ca233c3bba
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marcin-Wojtas/ACPI-support-for-DSA/20220620-231646
git checkout 68a7a52989207bfe8640877c512c77ca233c3bba
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm 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 >>, old ones prefixed by <<):
ERROR: modpost: missing MODULE_LICENSE() in drivers/watchdog/gxp-wdt.o
>> ERROR: modpost: "fwnode_find_net_device_by_node" [net/dsa/dsa_core.ko] undefined!
--
0-DAY CI Kernel Test Service
https://01.org/lkp
pon., 20 cze 2022 o 19:46 Andy Shevchenko
<[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:18PM +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.
>
> ...
>
> > +static int fwnode_dev_node_match(struct device *dev, const void *data)
> > {
> > for (; dev; dev = dev->parent) {
> > - if (dev->of_node == data)
>
> > + if (dev->fwnode == data)
>
>
> We have a helper in device/bus.h (?) device_match_fwnode().
>
That's true, thanks.
> > return 1;
> > }
>
> But this all sounds like a good candidate to be generic. Do we have more users
> in the kernel of a such?
>
Do you mean fwnode_dev_node_match? I haven't noticed. Indeed, it may
be worth to move this one to drivers/base/property.c - what do you
think?
Thanks,
Marcin
pon., 20 cze 2022 o 19:49 Andy Shevchenko
<[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:19PM +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.
> >
> > While at it, also use the newly introduced fwnode operation in
> > of_mdiobus_phy_device_register().
>
> ...
>
> > static int of_mdiobus_register_device(struct mii_bus *mdio,
> > struct device_node *child, u32 addr)
> > {
>
> > + return fwnode_mdiobus_register_device(mdio, of_fwnode_handle(child), addr);
> > }
>
> Since it's static one-liner you probably may ger rid of it completely.
>
Good point, will do in v2.
Thanks,
Marcin
pon., 20 cze 2022 o 19:21 Andy Shevchenko
<[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote:
> > Hi!
> >
> > This patchset introduces the support for DSA in ACPI world. A couple of
> > words about the background and motivation behind those changes:
> >
> > The DSA code is strictly dependent on the Device Tree and Open Firmware
> > (of_*) interface, both in the drivers and the common high-level net/dsa API.
> > The only alternative is to pass the information about the topology via
> > platform data - a legacy approach used by older systems that compiled the
> > board description into the kernel.
> >
> > The above constraint is problematic for the embedded devices based e.g. on
> > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks
> > and workarounds have to be applied. Addition of switch description to
> > DSDT/SSDT tables would help to solve many similar cases and use unmodified
> > kernel modules. It also enables this feature for ARM64 ACPI users.
> >
> > The key enablements allowing for adding ACPI support for DSA in Linux were
> > NIC drivers, MDIO, PHY, and phylink modifications – the latter three merged
> > in 2021. I thought it would be worth to experiment with DSA, which seemed
> > to be a natural follow-up challenge.
> >
> > It turned out that without much hassle it is possible to describe
> > DSA-compliant switches as child devices of the MDIO busses, which are
> > responsible for their enumeration based on the standard _ADR fields and
> > description in _DSD objects under 'device properties' UUID [1].
> > The vast majority of required changes were simple of_* to fwnode_*
> > transition, as the DT and ACPI topolgies are analogous, except for
> > 'ports' and 'mdio' subnodes naming, as they don't conform ACPI
> > namespace constraints [2].
>
> ...
>
> > Note that for now cascade topology remains unsupported in ACPI world
> > (based on "dsa" label and "link" property values). It seems to be feasible,
> > but would extend this patchset due to necessity of of_phandle_iterator
> > migration to fwnode_. Leave it as a possible future step.
>
> Wondering if this can be done using fwnode graph.
>
Probably yes. It's a general question whether to follow iterating over
phandles pointed by properties, like DT with a minimal code change or
do something completely different.
Best regards,
Marcin
pon., 20 cze 2022 o 19:56 Andrew Lunn <[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote:
> > Hi!
> >
> > This patchset introduces the support for DSA in ACPI world. A couple of
> > words about the background and motivation behind those changes:
> >
> > The DSA code is strictly dependent on the Device Tree and Open Firmware
> > (of_*) interface, both in the drivers and the common high-level net/dsa API.
> > The only alternative is to pass the information about the topology via
> > platform data - a legacy approach used by older systems that compiled the
> > board description into the kernel.
>
> Not true. There are deployed x86 systems which do this, and they are
> fully up to date, not legacy. There are however limitations in what
> you can do. So please drop this wording.
>
Ok, thanks for clarification, agree for rewording. Afair pdata was a
legacy derived from the Orion/Dove times, but indeed it can be used in
the new systems that lack other switch description.
> > The above constraint is problematic for the embedded devices based e.g. on
> > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks
> > and workarounds have to be applied.
>
> It would be good to describe the limitations. As i said, there are x86
> systems running with marvell 6390 switches.
I'm aware of that and even saw some x86_64 + Marvell switch
contemporary examples of how lack of DT in system was worked around:
- out of tree updates to the module code
- keep small DT blob on the system storage, from where the mv88e6xxx
Those could be poor-coding / anecdotic showcases. I'd be happy to
learn if there is a proper and recommended way, how to do it properly.
>
> > It turned out that without much hassle it is possible to describe
> > DSA-compliant switches as child devices of the MDIO busses, which are
> > responsible for their enumeration based on the standard _ADR fields and
> > description in _DSD objects under 'device properties' UUID [1].
>
> No surprises there. That is how the DT binding works. And the current
> ACPI concept is basically DT in different words. Maybe the more
> important question is, is rewording DT in ACPI the correct approach,
> or should you bo doing a more native ACPI implementation? I cannot
> answer that, you need to ask the ACPI maintainers.
This is why I added linux-acpi list and the ACPI Maintainers to discuss
>
> > Note that for now cascade topology remains unsupported in ACPI world
> > (based on "dsa" label and "link" property values). It seems to be feasible,
> > but would extend this patchset due to necessity of of_phandle_iterator
> > migration to fwnode_. Leave it as a possible future step.
>
> We really do need to ensure this is possible. You are setting an ABI
> here, which everybody else in the ACPI world needs to follow. Cascaded
> switches is fundamental to DSA, it is the D in DSA. So i would prefer
> that you at least define and document the binding for D in DSA and get
> it sanity checked by the ACPI people.
>
I'm aware of the "D" importance, just kept it aside for now due to
lack of access to relevant HW and willing to discuss the overall
approach first.
WRT the technical side: multiple-phandle property is for sure
supported in _DSD, so the most straightforward would be to follow that
and simply migrate to fwnode_. The thing is in arm64 it's not widely
used and (testing that with ACPI is making it even harder). There is
also an alternative brought by Andy - definitely a thing to discuss
further. I think we seem to have a quorum for that among recipents of
this thread.
Thanks,
Marcin
pon., 20 cze 2022 o 20:45 Andrew Lunn <[email protected]> napisał(a):
>
> > You beat me up to this. I also was about to mention that the problem with such
> > conversions (like this series does) is not in the code. It's simplest part. The
> > problem is bindings and how you get them to be a standard (at least de facto).
>
> De facto is easy. Get it merged. After that, i will simply refuse
> anything else, the same way i and other Maintainers would refuse a
> different DT binding.
>
> If the ACPI committee approve and publish a binding, we will naturally
> accept that as well. So in the end we might have two bindings. But so
> far in this whole ACPI for networking story, i've not heard anybody
> say they are going to submit anything for standardisation. So this
> might be a mute point.
>
I understand your concern and of course it's better to be on a safe
side from the beginning. Based on the hitherto discussion under this
patchset, I would split the question about standardization to 2
orthogonal topics:
1. Relation to the bus and enumeration:
* As pointed out in another patch some switches can be attached to
SPI or I2C. In such a case this is simple - SPISerialBus /
I2CSerialBus structures
in _CRS are included in the ACPI Spec. They allow to comprise more
bus specific
information and the code in acpi/scan.c marks those child devices
as to be enumerated
by parent bus.
* MDIO bus doesn't have its own _CRS macro in the Spec, on the other
hand the _ADR
seems to be the only object required for proper operation - this
was my base for
proposed solution in patch 06/12.
2. The device description (unrelated to which bus it is attached)
* In Linux and other OS's there is a great amount of devices
conforming the guidelines
and using only the standard device identification/configuration
objects as per [1].
* Above do not contain custom items and entire information can be obtained by
existing, generic ACPI accessors - those devices (e.g. NICs,
SD/MMC controllers and
many others) are not explicitly mentioned in official standards.
* The question, also related to this DSA case - is the ACPI device()
hierarchical
structure of this kind a subject for standardization for including
in official ACPI specification?
* In case not, where to document it? Is Linux' Documentation enough?
I agree that in the moment of merge it becomes de facto standard ABI and
it's worth to sort it out.
Rafael, Len, any other ACPI expert - I would appreciate your inputs
and clarification
of the above. Your recommendation would be extremely helpful.
Best regards,
Marcin
Hi,
wt., 21 cze 2022 o 12:46 Marcin Wojtas <[email protected]> napisał(a):
>
> pon., 20 cze 2022 o 20:45 Andrew Lunn <[email protected]> napisał(a):
> >
> > > You beat me up to this. I also was about to mention that the problem with such
> > > conversions (like this series does) is not in the code. It's simplest part. The
> > > problem is bindings and how you get them to be a standard (at least de facto).
> >
> > De facto is easy. Get it merged. After that, i will simply refuse
> > anything else, the same way i and other Maintainers would refuse a
> > different DT binding.
> >
> > If the ACPI committee approve and publish a binding, we will naturally
> > accept that as well. So in the end we might have two bindings. But so
> > far in this whole ACPI for networking story, i've not heard anybody
> > say they are going to submit anything for standardisation. So this
> > might be a mute point.
> >
>
> I understand your concern and of course it's better to be on a safe
> side from the beginning. Based on the hitherto discussion under this
> patchset, I would split the question about standardization to 2
> orthogonal topics:
>
> 1. Relation to the bus and enumeration:
> * As pointed out in another patch some switches can be attached to
> SPI or I2C. In such a case this is simple - SPISerialBus /
> I2CSerialBus structures
> in _CRS are included in the ACPI Spec. They allow to comprise more
> bus specific
> information and the code in acpi/scan.c marks those child devices
> as to be enumerated
> by parent bus.
> * MDIO bus doesn't have its own _CRS macro in the Spec, on the other
> hand the _ADR
> seems to be the only object required for proper operation - this
> was my base for
> proposed solution in patch 06/12.
>
> 2. The device description (unrelated to which bus it is attached)
> * In Linux and other OS's there is a great amount of devices
> conforming the guidelines
> and using only the standard device identification/configuration
> objects as per [1].
> * Above do not contain custom items and entire information can be obtained by
> existing, generic ACPI accessors - those devices (e.g. NICs,
> SD/MMC controllers and
> many others) are not explicitly mentioned in official standards.
> * The question, also related to this DSA case - is the ACPI device()
> hierarchical
> structure of this kind a subject for standardization for including
> in official ACPI specification?
> * In case not, where to document it? Is Linux' Documentation enough?
> I agree that in the moment of merge it becomes de facto standard ABI and
> it's worth to sort it out.
>
> Rafael, Len, any other ACPI expert - I would appreciate your inputs
> and clarification
> of the above. Your recommendation would be extremely helpful.
>
Thank you all for vivid discussions. As it may take some time for the
MDIOSerialBus _CRS macro review and approval, for now I plan to submit
v2 of_ -> fwnode_/device_ migration (patches 1-7,11/12) and skip
ACPI-specific additions until it is unblocked by spec extension.
Best regards,
Marcin
On Wed, Jun 22, 2022 at 5:44 PM Marcin Wojtas <[email protected]> wrote:
>
> Hi,
>
> wt., 21 cze 2022 o 12:46 Marcin Wojtas <[email protected]> napisał(a):
> >
> > pon., 20 cze 2022 o 20:45 Andrew Lunn <[email protected]> napisał(a):
> > >
> > > > You beat me up to this. I also was about to mention that the problem with such
> > > > conversions (like this series does) is not in the code. It's simplest part. The
> > > > problem is bindings and how you get them to be a standard (at least de facto).
> > >
> > > De facto is easy. Get it merged. After that, i will simply refuse
> > > anything else, the same way i and other Maintainers would refuse a
> > > different DT binding.
> > >
> > > If the ACPI committee approve and publish a binding, we will naturally
> > > accept that as well. So in the end we might have two bindings. But so
> > > far in this whole ACPI for networking story, i've not heard anybody
> > > say they are going to submit anything for standardisation. So this
> > > might be a mute point.
> > >
> >
> > I understand your concern and of course it's better to be on a safe
> > side from the beginning. Based on the hitherto discussion under this
> > patchset, I would split the question about standardization to 2
> > orthogonal topics:
> >
> > 1. Relation to the bus and enumeration:
> > * As pointed out in another patch some switches can be attached to
> > SPI or I2C. In such a case this is simple - SPISerialBus /
> > I2CSerialBus structures
> > in _CRS are included in the ACPI Spec. They allow to comprise more
> > bus specific
> > information and the code in acpi/scan.c marks those child devices
> > as to be enumerated
> > by parent bus.
> > * MDIO bus doesn't have its own _CRS macro in the Spec, on the other
> > hand the _ADR
> > seems to be the only object required for proper operation - this
> > was my base for
> > proposed solution in patch 06/12.
> >
> > 2. The device description (unrelated to which bus it is attached)
> > * In Linux and other OS's there is a great amount of devices
> > conforming the guidelines
> > and using only the standard device identification/configuration
> > objects as per [1].
> > * Above do not contain custom items and entire information can be obtained by
> > existing, generic ACPI accessors - those devices (e.g. NICs,
> > SD/MMC controllers and
> > many others) are not explicitly mentioned in official standards.
> > * The question, also related to this DSA case - is the ACPI device()
> > hierarchical
> > structure of this kind a subject for standardization for including
> > in official ACPI specification?
> > * In case not, where to document it? Is Linux' Documentation enough?
> > I agree that in the moment of merge it becomes de facto standard ABI and
> > it's worth to sort it out.
> >
> > Rafael, Len, any other ACPI expert - I would appreciate your inputs
> > and clarification
> > of the above. Your recommendation would be extremely helpful.
> >
>
> Thank you all for vivid discussions. As it may take some time for the
> MDIOSerialBus _CRS macro review and approval, for now I plan to submit
> v2 of_ -> fwnode_/device_ migration (patches 1-7,11/12) and skip
> ACPI-specific additions until it is unblocked by spec extension.
Sounds good to me (as from fwnode perspective).
--
With Best Regards,
Andy Shevchenko