Hi,
This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
First three patches introduce fwnode helpers for obtaining PHY
information from nodes and also MDIO fwnode API for registering
the bus with its PHY/devices.
Following patches update code of the mvmdio and mvpp2 drivers
to support ACPI tables handling. The latter is done in 4 steps,
as can be seen in the commits. For the details, please
refer to the commit messages.
Drivers operation was tested on top of the net-next branch
with both DT and ACPI. Although for compatibility reasons
with older platforms, the mvmdio driver keeps using
of_ MDIO registering, new fwnode_ one proved to fully work
with DT as well (tested on MacchiatoBin board).
mvpp2/mvmdio driver can work with the ACPI representation, as exposed
on a public branch:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
It was compiled together with the most recent Tianocore EDK2 revision.
Please refer to the firmware build instruction on MacchiatoBin board:
http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
Above support configures 1G to use its PHY normally. 10G can work now
only with the link interrupt mode. Somehow reading of the
string property in fwnode_mdiobus_child_is_phy works only with
DT and cannot cope with 10G PHY nodes as in:
https://pastebin.com/3JnYpU0A
Above root cause will be further checked. In the meantime I will
appreciate any comments or remarks for the kernel patches.
Best regards,
Marcin
Marcin Wojtas (8):
device property: Introduce fwnode_get_mac_address()
device property: Introduce fwnode_get_phy_mode()
mdio_bus: Introduce fwnode MDIO helpers
net: mvmdio: add ACPI support
net: mvpp2: simplify maintaining enabled ports' list
net: mvpp2: use device_*/fwnode_* APIs instead of of_*
net: mvpp2: handle PHY with its fwnode
net: mvpp2: enable ACPI support in the driver
drivers/base/property.c | 52 +++--
drivers/net/ethernet/marvell/mvmdio.c | 42 +++-
drivers/net/ethernet/marvell/mvpp2.c | 246 ++++++++++++--------
drivers/net/phy/mdio_bus.c | 218 +++++++++++++++++
include/linux/mdio.h | 3 +
include/linux/property.h | 3 +
6 files changed, 454 insertions(+), 110 deletions(-)
--
2.7.4
This patch introducing ACPI support for the mvmdio driver by adding
acpi_match_table with two entries:
* "MRVL0100" for the SMI operation
* "MRVL0101" for the XSMI mode
Also clk enabling is skipped, because the tables do not contain
such data and clock maintenance relies on the firmware. The
MDIO bus is registered using newly introduced
fwnode_mdiobus_register().
Memory region used by mvmdio driver is usually placed in the middle
of the address space of the network controller (e.g. NETA or PP2).
The MDIO base address is obtained without requesting memory region
(by devm_ioremap() call), later overlapping resources are
requested by the network driver, where care is taken to avoid
concurrent access.
This way of solving problem occurred to be not sufficient with ACPI,
because resources declared in the table and used once, appear as
'in-use' in the OS. This patch also ensures releasing resources by
mvmdio driver prior to initializing the network controller driver.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvmdio.c | 42 +++++++++++++++++++-
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
index 0495487..31d758a 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -17,6 +17,7 @@
* warranty of any kind, whether express or implied.
*/
+#include <linux/acpi.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
@@ -279,9 +280,19 @@ static int orion_mdio_probe(struct platform_device *pdev)
struct resource *r;
struct mii_bus *bus;
struct orion_mdio_dev *dev;
+ const struct acpi_device_id *acpi_id;
+ bool use_acpi = false;
int i, ret;
- type = (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+ if (has_acpi_companion(&pdev->dev)) {
+ acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
+ &pdev->dev);
+ type = (enum orion_mdio_bus_type)acpi_id->driver_data;
+ use_acpi = true;
+ } else {
+ type =
+ (enum orion_mdio_bus_type)of_device_get_match_data(&pdev->dev);
+ }
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!r) {
@@ -319,7 +330,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
init_waitqueue_head(&dev->smi_busy_wait);
- for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
+ for (i = 0; !use_acpi && i < ARRAY_SIZE(dev->clk); i++) {
dev->clk[i] = of_clk_get(pdev->dev.of_node, i);
if (IS_ERR(dev->clk[i]))
break;
@@ -350,6 +361,8 @@ static int orion_mdio_probe(struct platform_device *pdev)
if (pdev->dev.of_node)
ret = of_mdiobus_register(bus, pdev->dev.of_node);
+ else if (use_acpi)
+ ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
else
ret = mdiobus_register(bus);
if (ret < 0) {
@@ -357,6 +370,15 @@ static int orion_mdio_probe(struct platform_device *pdev)
goto out_mdio;
}
+ /* In case of ACPI resources declared in the tables and used
+ * once, appear as 'in-use' in the OS. Make sure they are released,
+ * before the network driver possibly requests it again during
+ * its initialization. The care is taken there to avoid
+ * concurrent access to this memory region.
+ */
+ if (use_acpi)
+ release_resource(r);
+
platform_set_drvdata(pdev, bus);
return 0;
@@ -365,6 +387,11 @@ static int orion_mdio_probe(struct platform_device *pdev)
if (dev->err_interrupt > 0)
writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
+ if (use_acpi) {
+ release_resource(r);
+ return ret;
+ }
+
for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
if (IS_ERR(dev->clk[i]))
break;
@@ -385,6 +412,9 @@ static int orion_mdio_remove(struct platform_device *pdev)
writel(0, dev->regs + MVMDIO_ERR_INT_MASK);
mdiobus_unregister(bus);
+ if (has_acpi_companion(&pdev->dev))
+ return 0;
+
for (i = 0; i < ARRAY_SIZE(dev->clk); i++) {
if (IS_ERR(dev->clk[i]))
break;
@@ -402,12 +432,20 @@ static const struct of_device_id orion_mdio_match[] = {
};
MODULE_DEVICE_TABLE(of, orion_mdio_match);
+static const struct acpi_device_id orion_mdio_acpi_match[] = {
+ { "MRVL0100", BUS_TYPE_SMI },
+ { "MRVL0101", BUS_TYPE_XSMI },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, orion_mdio_acpi_match);
+
static struct platform_driver orion_mdio_driver = {
.probe = orion_mdio_probe,
.remove = orion_mdio_remove,
.driver = {
.name = "orion-mdio",
.of_match_table = orion_mdio_match,
+ .acpi_match_table = ACPI_PTR(orion_mdio_acpi_match),
},
};
--
2.7.4
This patch introduces an alternative way of obtaining resources - via
ACPI tables provided by firmware. Enabling coexistence with the DT
support, in addition to the OF_*->device_*/fwnode_* API replacement,
required following steps to be taken:
* Add mvpp2_acpi_match table
* Omit clock configuration and obtain tclk from the property - in ACPI
world, the firmware is responsible for clock maintenance.
* Disable comphy and syscon handling as they are not available for ACPI.
* Modify way of obtaining interrupts - with ACPI they are resources
bound to struct platform_device and it's not possible to obtain
them directly from the child node. Hence a formula is used, depending
on the port_id and number of possible CPUs.
Moreover when booting with ACPI MVPP2_QDIST_MULTI_MODE is picked by
default, as there is no need to keep any kind of the backward
compatibility.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2.c | 130 +++++++++++++-------
1 file changed, 87 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 4e61ce7..762a44e 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -10,6 +10,7 @@
* warranty of any kind, whether express or implied.
*/
+#include <linux/acpi.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
@@ -7477,7 +7478,8 @@ static int mvpp2_simple_queue_vectors_init(struct mvpp2_port *port,
return 0;
}
-static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port,
+static int mvpp2_multi_queue_vectors_init(struct platform_device *pdev,
+ struct mvpp2_port *port,
struct device_node *port_node)
{
struct mvpp2_queue_vector *v;
@@ -7510,7 +7512,11 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port,
strncpy(irqname, "rx-shared", sizeof(irqname));
}
- v->irq = of_irq_get_byname(port_node, irqname);
+ if (port_node)
+ v->irq = of_irq_get_byname(port_node, irqname);
+ else
+ v->irq = platform_get_irq(pdev, port->id *
+ (port->nqvecs + 2) + i);
if (v->irq <= 0) {
ret = -EINVAL;
goto err;
@@ -7528,11 +7534,12 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port,
return ret;
}
-static int mvpp2_queue_vectors_init(struct mvpp2_port *port,
+static int mvpp2_queue_vectors_init(struct platform_device *pdev,
+ struct mvpp2_port *port,
struct device_node *port_node)
{
if (port->has_tx_irqs)
- return mvpp2_multi_queue_vectors_init(port, port_node);
+ return mvpp2_multi_queue_vectors_init(pdev, port, port_node);
else
return mvpp2_simple_queue_vectors_init(port, port_node);
}
@@ -7753,7 +7760,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
struct fwnode_handle *port_fwnode,
struct mvpp2 *priv)
{
- struct phy *comphy;
+ struct phy *comphy = NULL;
struct mvpp2_port *port;
struct mvpp2_port_pcpu *port_pcpu;
struct device_node *port_node = to_of_node(port_fwnode);
@@ -7772,7 +7779,12 @@ static int mvpp2_port_probe(struct platform_device *pdev,
if (!fwnode_device_is_available(port_fwnode))
return 0;
- has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
+ if (port_node) {
+ has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
+ } else {
+ has_tx_irqs = true;
+ queue_mode = MVPP2_QDIST_MULTI_MODE;
+ }
if (!has_tx_irqs)
queue_mode = MVPP2_QDIST_SINGLE_MODE;
@@ -7794,13 +7806,15 @@ static int mvpp2_port_probe(struct platform_device *pdev,
goto err_free_netdev;
}
- comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
- if (IS_ERR(comphy)) {
- if (PTR_ERR(comphy) == -EPROBE_DEFER) {
- err = -EPROBE_DEFER;
- goto err_free_netdev;
+ if (port_node) {
+ comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
+ if (IS_ERR(comphy)) {
+ if (PTR_ERR(comphy) == -EPROBE_DEFER) {
+ err = -EPROBE_DEFER;
+ goto err_free_netdev;
+ }
+ comphy = NULL;
}
- comphy = NULL;
}
if (fwnode_property_read_u32(port_fwnode, "port-id", &id)) {
@@ -7820,6 +7834,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
port->nrxqs = nrxqs;
port->priv = priv;
port->has_tx_irqs = has_tx_irqs;
+ port->id = id;
err = fwnode_property_get_reference_args(port_fwnode, "phy", NULL,
0, 0, &args);
@@ -7832,11 +7847,16 @@ static int mvpp2_port_probe(struct platform_device *pdev,
goto err_free_netdev;
}
- err = mvpp2_queue_vectors_init(port, port_node);
+ err = mvpp2_queue_vectors_init(pdev, port, port_node);
if (err)
goto err_free_netdev;
- port->link_irq = of_irq_get_byname(port_node, "link");
+ if (port_node)
+ port->link_irq = of_irq_get_byname(port_node, "link");
+ else
+ port->link_irq = platform_get_irq(pdev, port->id *
+ (port->nqvecs + 2) +
+ port->nqvecs + 1);
if (port->link_irq == -EPROBE_DEFER) {
err = -EPROBE_DEFER;
goto err_deinit_qvecs;
@@ -7848,7 +7868,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
if (fwnode_property_read_bool(port_fwnode, "marvell,loopback"))
port->flags |= MVPP2_F_LOOPBACK;
- port->id = id;
if (priv->hw_version == MVPP21)
port->first_rxq = port->id * port->nrxqs;
else
@@ -8218,6 +8237,7 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
static int mvpp2_probe(struct platform_device *pdev)
{
+ const struct acpi_device_id *acpi_id;
struct fwnode_handle *port_fwnode;
struct mvpp2 *priv;
struct resource *res;
@@ -8229,8 +8249,14 @@ static int mvpp2_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
- priv->hw_version =
- (unsigned long)of_device_get_match_data(&pdev->dev);
+ if (has_acpi_companion(&pdev->dev)) {
+ acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
+ &pdev->dev);
+ priv->hw_version = (unsigned long)acpi_id->driver_data;
+ } else {
+ priv->hw_version =
+ (unsigned long)of_device_get_match_data(&pdev->dev);
+ }
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
base = devm_ioremap_resource(&pdev->dev, res);
@@ -8247,7 +8273,9 @@ static int mvpp2_probe(struct platform_device *pdev)
priv->iface_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->iface_base))
return PTR_ERR(priv->iface_base);
+ }
+ if (priv->hw_version == MVPP22 && dev_of_node(&pdev->dev)) {
priv->sysctrl_base =
syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
"marvell,system-controller");
@@ -8273,32 +8301,34 @@ static int mvpp2_probe(struct platform_device *pdev)
else
priv->max_port_rxqs = 32;
- priv->pp_clk = devm_clk_get(&pdev->dev, "pp_clk");
- if (IS_ERR(priv->pp_clk))
- return PTR_ERR(priv->pp_clk);
- err = clk_prepare_enable(priv->pp_clk);
- if (err < 0)
- return err;
-
- priv->gop_clk = devm_clk_get(&pdev->dev, "gop_clk");
- if (IS_ERR(priv->gop_clk)) {
- err = PTR_ERR(priv->gop_clk);
- goto err_pp_clk;
- }
- err = clk_prepare_enable(priv->gop_clk);
- if (err < 0)
- goto err_pp_clk;
+ if (dev_of_node(&pdev->dev)) {
+ priv->pp_clk = devm_clk_get(&pdev->dev, "pp_clk");
+ if (IS_ERR(priv->pp_clk))
+ return PTR_ERR(priv->pp_clk);
+ err = clk_prepare_enable(priv->pp_clk);
+ if (err < 0)
+ return err;
- if (priv->hw_version == MVPP22) {
- priv->mg_clk = devm_clk_get(&pdev->dev, "mg_clk");
- if (IS_ERR(priv->mg_clk)) {
- err = PTR_ERR(priv->mg_clk);
- goto err_gop_clk;
+ priv->gop_clk = devm_clk_get(&pdev->dev, "gop_clk");
+ if (IS_ERR(priv->gop_clk)) {
+ err = PTR_ERR(priv->gop_clk);
+ goto err_pp_clk;
}
-
- err = clk_prepare_enable(priv->mg_clk);
+ err = clk_prepare_enable(priv->gop_clk);
if (err < 0)
- goto err_gop_clk;
+ goto err_pp_clk;
+
+ if (priv->hw_version == MVPP22) {
+ priv->mg_clk = devm_clk_get(&pdev->dev, "mg_clk");
+ if (IS_ERR(priv->mg_clk)) {
+ err = PTR_ERR(priv->mg_clk);
+ goto err_gop_clk;
+ }
+
+ err = clk_prepare_enable(priv->mg_clk);
+ if (err < 0)
+ goto err_gop_clk;
+ }
priv->axi_clk = devm_clk_get(&pdev->dev, "axi_clk");
if (IS_ERR(priv->axi_clk)) {
@@ -8311,10 +8341,14 @@ static int mvpp2_probe(struct platform_device *pdev)
if (err < 0)
goto err_gop_clk;
}
- }
- /* Get system's tclk rate */
- priv->tclk = clk_get_rate(priv->pp_clk);
+ /* Get system's tclk rate */
+ priv->tclk = clk_get_rate(priv->pp_clk);
+ } else if (device_property_read_u32(&pdev->dev, "clock-frequency",
+ &priv->tclk)) {
+ dev_err(&pdev->dev, "missing clock-frequency value\n");
+ return -EINVAL;
+ }
if (priv->hw_version == MVPP22) {
err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(40));
@@ -8418,6 +8452,9 @@ static int mvpp2_remove(struct platform_device *pdev)
aggr_txq->descs_dma);
}
+ if (is_acpi_node(port_fwnode))
+ return 0;
+
clk_disable_unprepare(priv->axi_clk);
clk_disable_unprepare(priv->mg_clk);
clk_disable_unprepare(priv->pp_clk);
@@ -8439,12 +8476,19 @@ static const struct of_device_id mvpp2_match[] = {
};
MODULE_DEVICE_TABLE(of, mvpp2_match);
+static const struct acpi_device_id mvpp2_acpi_match[] = {
+ { "MRVL0110", MVPP22 },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, mvpp2_acpi_match);
+
static struct platform_driver mvpp2_driver = {
.probe = mvpp2_probe,
.remove = mvpp2_remove,
.driver = {
.name = MVPP2_DRIVER_NAME,
.of_match_table = mvpp2_match,
+ .acpi_match_table = ACPI_PTR(mvpp2_acpi_match),
},
};
--
2.7.4
Newly introduced mvmdio driver ACPI support and also fwnode
MDIO helpers allow for switching to PHY handling with its fwnode.
This patch replaces of_* related PHY handling function with
the fwnode_* equivalent and updates mvpp2_port structure
accordingly.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2.c | 41 ++++++++++++++------
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 537474f..4e61ce7 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -963,7 +963,7 @@ struct mvpp2_port {
struct delayed_work stats_work;
phy_interface_t phy_interface;
- struct device_node *phy_node;
+ struct fwnode_handle *phy_fwnode;
struct phy *comphy;
unsigned int link;
unsigned int duplex;
@@ -6890,14 +6890,22 @@ static void mvpp21_get_mac_address(struct mvpp2_port *port, unsigned char *addr)
static int mvpp2_phy_connect(struct mvpp2_port *port)
{
struct phy_device *phy_dev;
+ int ret;
/* No PHY is attached */
- if (!port->phy_node)
+ if (!port->phy_fwnode)
return 0;
- phy_dev = of_phy_connect(port->dev, port->phy_node, mvpp2_link_event, 0,
+ phy_dev = fwnode_phy_find_device(port->phy_fwnode);
+ phy_dev->dev_flags = 0;
+
+ ret = phy_connect_direct(port->dev, phy_dev, mvpp2_link_event,
port->phy_interface);
- if (!phy_dev) {
+
+ /* Refcount is held by phy_connect_direct() on success */
+ put_device(&phy_dev->mdio.dev);
+
+ if (ret) {
netdev_err(port->dev, "cannot connect to phy\n");
return -ENODEV;
}
@@ -7047,7 +7055,7 @@ static int mvpp2_open(struct net_device *dev)
goto err_cleanup_txqs;
}
- if (priv->hw_version == MVPP22 && !port->phy_node && port->link_irq) {
+ if (priv->hw_version == MVPP22 && !port->phy_fwnode && port->link_irq) {
err = request_irq(port->link_irq, mvpp2_link_status_isr, 0,
dev->name, port);
if (err) {
@@ -7082,7 +7090,7 @@ static int mvpp2_open(struct net_device *dev)
return 0;
err_free_link_irq:
- if (priv->hw_version == MVPP22 && !port->phy_node && port->link_irq)
+ if (priv->hw_version == MVPP22 && !port->phy_fwnode && port->link_irq)
free_irq(port->link_irq, port);
err_free_irq:
mvpp2_irqs_deinit(port);
@@ -7107,7 +7115,7 @@ static int mvpp2_stop(struct net_device *dev)
on_each_cpu(mvpp2_interrupts_mask, port, 1);
mvpp2_shared_interrupt_mask_unmask(port, true);
- if (priv->hw_version == MVPP22 && !port->phy_node && port->link_irq)
+ if (priv->hw_version == MVPP22 && !port->phy_fwnode && port->link_irq)
free_irq(port->link_irq, port);
mvpp2_irqs_deinit(port);
@@ -7745,11 +7753,11 @@ static int mvpp2_port_probe(struct platform_device *pdev,
struct fwnode_handle *port_fwnode,
struct mvpp2 *priv)
{
- struct device_node *phy_node;
struct phy *comphy;
struct mvpp2_port *port;
struct mvpp2_port_pcpu *port_pcpu;
struct device_node *port_node = to_of_node(port_fwnode);
+ struct fwnode_reference_args args;
struct net_device *dev;
struct resource *res;
char *mac_from = "";
@@ -7779,7 +7787,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
if (!dev)
return -ENOMEM;
- phy_node = of_parse_phandle(port_node, "phy", 0);
phy_mode = fwnode_get_phy_mode(port_fwnode);
if (phy_mode < 0) {
dev_err(&pdev->dev, "incorrect phy mode\n");
@@ -7814,6 +7821,17 @@ static int mvpp2_port_probe(struct platform_device *pdev,
port->priv = priv;
port->has_tx_irqs = has_tx_irqs;
+ err = fwnode_property_get_reference_args(port_fwnode, "phy", NULL,
+ 0, 0, &args);
+ if (!err) {
+ port->phy_fwnode = args.fwnode;
+ } else if (err == -ENOENT) {
+ port->phy_fwnode = NULL;
+ } else {
+ dev_err(&pdev->dev, "unable to parse \"phy\" node\n");
+ goto err_free_netdev;
+ }
+
err = mvpp2_queue_vectors_init(port, port_node);
if (err)
goto err_free_netdev;
@@ -7836,7 +7854,6 @@ static int mvpp2_port_probe(struct platform_device *pdev,
else
port->first_rxq = port->id * priv->max_port_rxqs;
- port->phy_node = phy_node;
port->phy_interface = phy_mode;
port->comphy = comphy;
@@ -7958,7 +7975,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
err_deinit_qvecs:
mvpp2_queue_vectors_deinit(port);
err_free_netdev:
- of_node_put(phy_node);
+ fwnode_handle_put(args.fwnode);
free_netdev(dev);
return err;
}
@@ -7969,7 +7986,7 @@ static void mvpp2_port_remove(struct mvpp2_port *port)
int i;
unregister_netdev(port->dev);
- of_node_put(port->phy_node);
+ fwnode_handle_put(port->phy_fwnode);
free_percpu(port->pcpu);
free_percpu(port->stats);
for (i = 0; i < port->ntxqs; i++)
--
2.7.4
OF functions can be used only for the driver using DT.
As a preparation for introducing ACPI support in mvpp2
driver, use struct fwnode_handle in order to obtain
properties from the hardware description.
Because there is no equivalent for for_each_available_child_of_node(),
use device_for_each_child_node() and check the port availability
inside the mvpp2_port_probe() routine.
This patch replaces of_* function with device_*/fwnode_*
where possible in the mvpp2.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2.c | 47 +++++++++++---------
1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 7f42d90..537474f 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -932,6 +932,9 @@ struct mvpp2_port {
struct mvpp2 *priv;
+ /* Firmware node associated to the port */
+ struct fwnode_handle *fwnode;
+
/* Per-port registers' base address */
void __iomem *base;
void __iomem *stats_base;
@@ -7711,17 +7714,16 @@ static bool mvpp2_port_has_tx_irqs(struct mvpp2 *priv,
}
static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
- struct device_node *port_node,
+ struct fwnode_handle *fwnode,
char **mac_from)
{
struct mvpp2_port *port = netdev_priv(dev);
char hw_mac_addr[ETH_ALEN] = {0};
- const char *dt_mac_addr;
+ char fw_mac_addr[ETH_ALEN];
- dt_mac_addr = of_get_mac_address(port_node);
- if (dt_mac_addr && is_valid_ether_addr(dt_mac_addr)) {
- *mac_from = "device tree";
- ether_addr_copy(dev->dev_addr, dt_mac_addr);
+ if (fwnode_get_mac_address(fwnode, fw_mac_addr, ETH_ALEN)) {
+ *mac_from = "firmware node";
+ ether_addr_copy(dev->dev_addr, fw_mac_addr);
return;
}
@@ -7740,13 +7742,14 @@ static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
/* Ports initialization */
static int mvpp2_port_probe(struct platform_device *pdev,
- struct device_node *port_node,
+ struct fwnode_handle *port_fwnode,
struct mvpp2 *priv)
{
struct device_node *phy_node;
struct phy *comphy;
struct mvpp2_port *port;
struct mvpp2_port_pcpu *port_pcpu;
+ struct device_node *port_node = to_of_node(port_fwnode);
struct net_device *dev;
struct resource *res;
char *mac_from = "";
@@ -7757,6 +7760,10 @@ static int mvpp2_port_probe(struct platform_device *pdev,
int phy_mode;
int err, i, cpu;
+ /* Silently exit, if the port node turns out to be disabled. */
+ if (!fwnode_device_is_available(port_fwnode))
+ return 0;
+
has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
if (!has_tx_irqs)
@@ -7773,7 +7780,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
return -ENOMEM;
phy_node = of_parse_phandle(port_node, "phy", 0);
- phy_mode = of_get_phy_mode(port_node);
+ phy_mode = fwnode_get_phy_mode(port_fwnode);
if (phy_mode < 0) {
dev_err(&pdev->dev, "incorrect phy mode\n");
err = phy_mode;
@@ -7789,7 +7796,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
comphy = NULL;
}
- if (of_property_read_u32(port_node, "port-id", &id)) {
+ if (fwnode_property_read_u32(port_fwnode, "port-id", &id)) {
err = -EINVAL;
dev_err(&pdev->dev, "missing port-id value\n");
goto err_free_netdev;
@@ -7820,7 +7827,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
/* the link irq is optional */
port->link_irq = 0;
- if (of_property_read_bool(port_node, "marvell,loopback"))
+ if (fwnode_property_read_bool(port_fwnode, "marvell,loopback"))
port->flags |= MVPP2_F_LOOPBACK;
port->id = id;
@@ -7845,8 +7852,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
MVPP21_MIB_COUNTERS_OFFSET +
port->gop_id * MVPP21_MIB_COUNTERS_PORT_SZ;
} else {
- if (of_property_read_u32(port_node, "gop-port-id",
- &port->gop_id)) {
+ if (fwnode_property_read_u32(port_fwnode, "gop-port-id",
+ &port->gop_id)) {
err = -EINVAL;
dev_err(&pdev->dev, "missing gop-port-id value\n");
goto err_deinit_qvecs;
@@ -7876,7 +7883,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
mutex_init(&port->gather_stats_lock);
INIT_DELAYED_WORK(&port->stats_work, mvpp2_gather_hw_statistics);
- mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
+ mvpp2_port_copy_mac_addr(dev, priv, port_fwnode, &mac_from);
port->tx_ring_size = MVPP2_MAX_TXD_DFLT;
port->rx_ring_size = MVPP2_MAX_RXD_DFLT;
@@ -8194,8 +8201,7 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
static int mvpp2_probe(struct platform_device *pdev)
{
- struct device_node *dn = pdev->dev.of_node;
- struct device_node *port_node;
+ struct fwnode_handle *port_fwnode;
struct mvpp2 *priv;
struct resource *res;
void __iomem *base;
@@ -8315,8 +8321,8 @@ static int mvpp2_probe(struct platform_device *pdev)
}
/* Initialize ports */
- for_each_available_child_of_node(dn, port_node) {
- err = mvpp2_port_probe(pdev, port_node, priv);
+ device_for_each_child_node(&pdev->dev, port_fwnode) {
+ err = mvpp2_port_probe(pdev, port_fwnode, priv);
if (err < 0)
goto err_port_probe;
}
@@ -8347,7 +8353,7 @@ static int mvpp2_probe(struct platform_device *pdev)
err_port_probe:
i = 0;
- for_each_available_child_of_node(dn, port_node) {
+ device_for_each_child_node(&pdev->dev, port_fwnode) {
if (priv->port_list[i])
mvpp2_port_remove(priv->port_list[i]);
i++;
@@ -8366,14 +8372,13 @@ static int mvpp2_probe(struct platform_device *pdev)
static int mvpp2_remove(struct platform_device *pdev)
{
struct mvpp2 *priv = platform_get_drvdata(pdev);
- struct device_node *dn = pdev->dev.of_node;
- struct device_node *port_node;
+ struct fwnode_handle *port_fwnode;
int i = 0;
flush_workqueue(priv->stats_queue);
destroy_workqueue(priv->stats_queue);
- for_each_available_child_of_node(dn, port_node) {
+ device_for_each_child_node(&pdev->dev, port_fwnode) {
if (priv->port_list[i]) {
mutex_destroy(&priv->port_list[i]->gather_stats_lock);
mvpp2_port_remove(priv->port_list[i]);
--
2.7.4
'port_count' field of the mvpp2 structure holds an overall amount
of available ports, based on DT nodes status. In order to be prepared
to support other HW description, obtain the value by incrementing it
upon each successful port initialization. This allowed for simplifying
port indexing in the controller's private array, whose size is now not
dynamically allocated, but fixed to MVPP2_MAX_PORTS.
This patch simplifies creating and filling list of enabled ports and
is a part of the preparation for adding ACPI support in the mvpp2 driver.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2.c | 32 +++++++-------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index a197607..7f42d90 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -865,7 +865,7 @@ struct mvpp2 {
/* List of pointers to port structures */
int port_count;
- struct mvpp2_port **port_list;
+ struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
/* Aggregated TXQs */
struct mvpp2_tx_queue *aggr_txqs;
@@ -7741,7 +7741,7 @@ static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
/* Ports initialization */
static int mvpp2_port_probe(struct platform_device *pdev,
struct device_node *port_node,
- struct mvpp2 *priv, int index)
+ struct mvpp2 *priv)
{
struct device_node *phy_node;
struct phy *comphy;
@@ -7934,7 +7934,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
}
netdev_info(dev, "Using %s mac address %pM\n", mac_from, dev->dev_addr);
- priv->port_list[index] = port;
+ priv->port_list[priv->port_count++] = port;
+
return 0;
err_free_port_pcpu:
@@ -8313,28 +8314,17 @@ static int mvpp2_probe(struct platform_device *pdev)
goto err_mg_clk;
}
- priv->port_count = of_get_available_child_count(dn);
- if (priv->port_count == 0) {
- dev_err(&pdev->dev, "no ports enabled\n");
- err = -ENODEV;
- goto err_mg_clk;
- }
-
- priv->port_list = devm_kcalloc(&pdev->dev, priv->port_count,
- sizeof(*priv->port_list),
- GFP_KERNEL);
- if (!priv->port_list) {
- err = -ENOMEM;
- goto err_mg_clk;
- }
-
/* Initialize ports */
- i = 0;
for_each_available_child_of_node(dn, port_node) {
- err = mvpp2_port_probe(pdev, port_node, priv, i);
+ err = mvpp2_port_probe(pdev, port_node, priv);
if (err < 0)
goto err_port_probe;
- i++;
+ }
+
+ if (priv->port_count == 0) {
+ dev_err(&pdev->dev, "no ports enabled\n");
+ err = -ENODEV;
+ goto err_mg_clk;
}
/* Statistics must be gathered regularly because some of them (like
--
2.7.4
This patch introduces fwnode helper for registering MDIO
bus, as well as one for finding the PHY, basing on its
firmware node pointer. Comparing to existing OF equivalent,
fwnode_mdiobus_register() does not support:
* deprecated bindings (device whitelist, nor the PHY ID embedded
in the compatible string)
* MDIO bus auto scanning
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/net/phy/mdio_bus.c | 218 ++++++++++++++++++++
include/linux/mdio.h | 3 +
2 files changed, 221 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a0f34c3..f2b2a94 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -27,6 +27,7 @@
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/skbuff.h>
@@ -662,6 +663,223 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
return 0;
}
+static int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct phy_device *phy;
+ bool is_c45 = false;
+ int rc;
+
+ rc = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c45");
+ if (!rc)
+ is_c45 = true;
+
+ phy = get_phy_device(bus, addr, is_c45);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ phy->irq = bus->irq[addr];
+
+ if (to_of_node(child)) {
+ rc = of_irq_get(to_of_node(child), 0);
+ if (rc == -EPROBE_DEFER) {
+ phy_device_free(phy);
+ return rc;
+ } else if (rc > 0) {
+ phy->irq = rc;
+ bus->irq[addr] = rc;
+ }
+ }
+
+ if (fwnode_property_read_bool(child, "broken-turn-around"))
+ bus->phy_ignore_ta_mask |= 1 << addr;
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ phy->mdio.dev.fwnode = child;
+
+ /* All data is now stored in the phy struct, so register it */
+ rc = phy_device_register(phy);
+ if (rc) {
+ phy_device_free(phy);
+ fwnode_handle_put(child);
+ return rc;
+ }
+
+ dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+ return 0;
+}
+
+static int fwnode_mdiobus_register_device(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct mdio_device *mdiodev;
+ int rc;
+
+ mdiodev = mdio_device_create(bus, addr);
+ if (IS_ERR(mdiodev))
+ return PTR_ERR(mdiodev);
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ mdiodev->dev.fwnode = 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(&bus->dev, "registered mdio device at address %i\n", addr);
+
+ return 0;
+}
+
+static int fwnode_mdio_parse_addr(struct device *dev,
+ const struct fwnode_handle *fwnode)
+{
+ u32 addr;
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", &addr);
+ if (ret < 0) {
+ dev_err(dev, "PHY node has no 'reg' property\n");
+ return ret;
+ }
+
+ /* A PHY must have a reg property in the range [0-31] */
+ if (addr < 0 || addr >= PHY_MAX_ADDR) {
+ dev_err(dev, "PHY address %i is invalid\n", addr);
+ return -EINVAL;
+ }
+
+ return addr;
+}
+
+/**
+ * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
+ * It must either:
+ * o Compatible string of "ethernet-phy-ieee802.3-c45"
+ * o Compatible string of "ethernet-phy-ieee802.3-c22"
+ * Checking "compatible" property is done, in order to follow the DT binding.
+ */
+static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
+{
+ int ret;
+
+ ret = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c45");
+ if (!ret)
+ return true;
+
+ ret = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c22");
+ if (!ret)
+ return true;
+
+ if (!fwnode_property_present(child, "compatible"))
+ return true;
+
+ return false;
+}
+
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from the fwnode
+ * @bus: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode_handle of MDIO bus.
+ *
+ * This function registers the mii_bus structure and registers a phy_device
+ * for each child node of @fwnode.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *child;
+ int addr, rc;
+ int default_gpio_reset_delay_ms = 10;
+
+ /* Do not continue if the node is disabled */
+ if (!fwnode_device_is_available(fwnode))
+ return -ENODEV;
+
+ /* Mask out all PHYs from auto probing. Instead the PHYs listed in
+ * the firmware nodes are populated after the bus has been registered.
+ */
+ bus->phy_mask = ~0;
+
+ bus->dev.fwnode = fwnode;
+
+ /* Get bus level PHY reset GPIO details */
+ bus->reset_delay_us = default_gpio_reset_delay_ms;
+ fwnode_property_read_u32(fwnode, "reset-delay-us",
+ &bus->reset_delay_us);
+
+ /* Register the MDIO bus */
+ rc = mdiobus_register(bus);
+ if (rc)
+ return rc;
+
+ /* Loop over the child nodes and register a phy_device for each PHY */
+ fwnode_for_each_child_node(fwnode, child) {
+ addr = fwnode_mdio_parse_addr(&bus->dev, child);
+ if (addr < 0)
+ continue;
+
+ if (fwnode_mdiobus_child_is_phy(child))
+ rc = fwnode_mdiobus_register_phy(bus, child, addr);
+ else
+ rc = fwnode_mdiobus_register_device(bus, child, addr);
+ if (rc)
+ goto unregister;
+ }
+
+ return 0;
+
+unregister:
+ mdiobus_unregister(bus);
+
+ return rc;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);
+
+/* Helper function for fwnode_phy_find_device */
+static int fwnode_phy_match(struct device *dev, void *phy_fwnode)
+{
+ return dev->fwnode == phy_fwnode;
+}
+
+/**
+ * fwnode_phy_find_device - find the phy_device associated to fwnode
+ * @phy_fwnode: Pointer to the PHY's fwnode
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+ struct device *d;
+ struct mdio_device *mdiodev;
+
+ if (!phy_fwnode)
+ return NULL;
+
+ d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
+ if (d) {
+ mdiodev = to_mdio_device(d);
+ if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+ return to_phy_device(d);
+ put_device(d);
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
#ifdef CONFIG_PM
static int mdio_bus_suspend(struct device *dev)
{
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index e37c21d..286ec12 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -272,6 +272,9 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev);
bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+
/**
* mdio_module_driver() - Helper macro for registering mdio drivers
*
--
2.7.4
Until now there were two almost identical functions for
obtaining network PHY mode - of_get_phy_mode() and,
more generic, device_get_phy_mode(). However it is not uncommon,
that the network interface is represented as a child
of the actual controller, hence it is not associated
directly to any struct device, required by the latter
routine.
This commit allows for getting the PHY mode for
children nodes in the ACPI world by introducing a new function -
fwnode_get_phy_mode(). This commit also changes
device_get_phy_mode() routine to be its wrapper, in order
to prevent unnecessary duplication.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/base/property.c | 24 ++++++++++++++++----
include/linux/property.h | 1 +
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index f261d1a..7c4a53d 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1126,21 +1126,21 @@ enum dev_dma_attr device_get_dma_attr(struct device *dev)
EXPORT_SYMBOL_GPL(device_get_dma_attr);
/**
- * device_get_phy_mode - Get phy mode for given device
- * @dev: Pointer to the given device
+ * fwnode_get_phy_mode - Get phy mode for given firmware node
+ * @fwnode: Pointer to the given node
*
* The function gets phy interface string from property 'phy-mode' or
* 'phy-connection-type', and return its index in phy_modes table, or errno in
* error case.
*/
-int device_get_phy_mode(struct device *dev)
+int fwnode_get_phy_mode(struct fwnode_handle *fwnode)
{
const char *pm;
int err, i;
- err = device_property_read_string(dev, "phy-mode", &pm);
+ err = fwnode_property_read_string(fwnode, "phy-mode", &pm);
if (err < 0)
- err = device_property_read_string(dev,
+ err = fwnode_property_read_string(fwnode,
"phy-connection-type", &pm);
if (err < 0)
return err;
@@ -1151,6 +1151,20 @@ int device_get_phy_mode(struct device *dev)
return -ENODEV;
}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_mode);
+
+/**
+ * device_get_phy_mode - Get phy mode for given device
+ * @dev: Pointer to the given device
+ *
+ * The function gets phy interface string from property 'phy-mode' or
+ * 'phy-connection-type', and return its index in phy_modes table, or errno in
+ * error case.
+ */
+int device_get_phy_mode(struct device *dev)
+{
+ return fwnode_get_phy_mode(dev_fwnode(dev));
+}
EXPORT_SYMBOL_GPL(device_get_phy_mode);
static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index 35620e0..9b13332 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -279,6 +279,7 @@ int device_get_phy_mode(struct device *dev);
void *device_get_mac_address(struct device *dev, char *addr, int alen);
+int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
char *addr, int alen);
struct fwnode_handle *fwnode_graph_get_next_endpoint(
--
2.7.4
Until now there were two almost identical functions for
obtaining MAC address - of_get_mac_address() and, more generic,
device_get_mac_address(). However it is not uncommon,
that the network interface is represented as a child
of the actual controller, hence it is not associated
directly to any struct device, required by the latter
routine.
This commit allows for getting the MAC address for
children nodes in the ACPI world by introducing a new function -
fwnode_get_mac_address(). This commit also changes
device_get_mac_address() routine to be its wrapper, in order
to prevent unnecessary duplication.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/base/property.c | 28 ++++++++++++++------
include/linux/property.h | 2 ++
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 851b1b6..f261d1a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1153,11 +1153,11 @@ int device_get_phy_mode(struct device *dev)
}
EXPORT_SYMBOL_GPL(device_get_phy_mode);
-static void *device_get_mac_addr(struct device *dev,
+static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
const char *name, char *addr,
int alen)
{
- int ret = device_property_read_u8_array(dev, name, addr, alen);
+ int ret = fwnode_property_read_u8_array(fwnode, name, addr, alen);
if (ret == 0 && alen == ETH_ALEN && is_valid_ether_addr(addr))
return addr;
@@ -1165,8 +1165,8 @@ static void *device_get_mac_addr(struct device *dev,
}
/**
- * device_get_mac_address - Get the MAC for a given device
- * @dev: Pointer to the device
+ * fwnode_get_mac_address - Get the MAC from the firmware node
+ * @fwnode: Pointer to the firmware node
* @addr: Address of buffer to store the MAC in
* @alen: Length of the buffer pointed to by addr, should be ETH_ALEN
*
@@ -1187,19 +1187,31 @@ static void *device_get_mac_addr(struct device *dev,
* In this case, the real MAC is in 'local-mac-address', and 'mac-address'
* exists but is all zeros.
*/
-void *device_get_mac_address(struct device *dev, char *addr, int alen)
+void *fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr, int alen)
{
char *res;
- res = device_get_mac_addr(dev, "mac-address", addr, alen);
+ res = fwnode_get_mac_addr(fwnode, "mac-address", addr, alen);
if (res)
return res;
- res = device_get_mac_addr(dev, "local-mac-address", addr, alen);
+ res = fwnode_get_mac_addr(fwnode, "local-mac-address", addr, alen);
if (res)
return res;
- return device_get_mac_addr(dev, "address", addr, alen);
+ return fwnode_get_mac_addr(fwnode, "address", addr, alen);
+}
+EXPORT_SYMBOL(fwnode_get_mac_address);
+
+/**
+ * device_get_mac_address - Get the MAC for a given device
+ * @dev: Pointer to the device
+ * @addr: Address of buffer to store the MAC in
+ * @alen: Length of the buffer pointed to by addr, should be ETH_ALEN
+ */
+void *device_get_mac_address(struct device *dev, char *addr, int alen)
+{
+ return fwnode_get_mac_address(dev_fwnode(dev), addr, alen);
}
EXPORT_SYMBOL(device_get_mac_address);
diff --git a/include/linux/property.h b/include/linux/property.h
index f6189a3..35620e0 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -279,6 +279,8 @@ int device_get_phy_mode(struct device *dev);
void *device_get_mac_address(struct device *dev, char *addr, int alen);
+void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
+ char *addr, int alen);
struct fwnode_handle *fwnode_graph_get_next_endpoint(
const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
struct fwnode_handle *
--
2.7.4
On 18 December 2017 at 10:17, Marcin Wojtas <[email protected]> wrote:
> Hi,
>
> This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> First three patches introduce fwnode helpers for obtaining PHY
> information from nodes and also MDIO fwnode API for registering
> the bus with its PHY/devices.
>
> Following patches update code of the mvmdio and mvpp2 drivers
> to support ACPI tables handling. The latter is done in 4 steps,
> as can be seen in the commits. For the details, please
> refer to the commit messages.
>
> Drivers operation was tested on top of the net-next branch
> with both DT and ACPI. Although for compatibility reasons
> with older platforms, the mvmdio driver keeps using
> of_ MDIO registering, new fwnode_ one proved to fully work
> with DT as well (tested on MacchiatoBin board).
>
> mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> on a public branch:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> It was compiled together with the most recent Tianocore EDK2 revision.
> Please refer to the firmware build instruction on MacchiatoBin board:
> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>
> Above support configures 1G to use its PHY normally. 10G can work now
> only with the link interrupt mode. Somehow reading of the
> string property in fwnode_mdiobus_child_is_phy works only with
> DT and cannot cope with 10G PHY nodes as in:
> https://pastebin.com/3JnYpU0A
>
> Above root cause will be further checked. In the meantime I will
> appreciate any comments or remarks for the kernel patches.
>
Hi Marcin,
I have added linux-acpi and Graeme to cc. I think it makes sense to
discuss the way you describe the device topology before looking at the
patches in more detail.
In particular, I would like to request feedback on the use of
[redundant] 'reg' properties and the use of _DSD + compatible to
describe PHYs. Usually, we try to avoid this, given that it is
essentially a ACPI encapsulated DT dialect that is difficult to
support in drivers unless they are based on DT to begin with. Also,
non-standard _DSD properties require a vendor prefix, it is not
freeform.
For reference, the ACPI description is here (starting at line 175)
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
--
Ard.
On 12/18/2017 10:17 AM, Marcin Wojtas wrote:
> Hi,
>
> This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> First three patches introduce fwnode helpers for obtaining PHY
> information from nodes and also MDIO fwnode API for registering
> the bus with its PHY/devices.
>
> Following patches update code of the mvmdio and mvpp2 drivers
> to support ACPI tables handling. The latter is done in 4 steps,
> as can be seen in the commits. For the details, please
> refer to the commit messages.
>
> Drivers operation was tested on top of the net-next branch
> with both DT and ACPI. Although for compatibility reasons
> with older platforms, the mvmdio driver keeps using
> of_ MDIO registering, new fwnode_ one proved to fully work
> with DT as well (tested on MacchiatoBin board).
>
> mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> on a public branch:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> It was compiled together with the most recent Tianocore EDK2 revision.
> Please refer to the firmware build instruction on MacchiatoBin board:
> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>
> Above support configures 1G to use its PHY normally. 10G can work now
> only with the link interrupt mode. Somehow reading of the
> string property in fwnode_mdiobus_child_is_phy works only with
> DT and cannot cope with 10G PHY nodes as in:
> https://pastebin.com/3JnYpU0A
>
> Above root cause will be further checked. In the meantime I will
> appreciate any comments or remarks for the kernel patches.
>
> Best regards,
> Marcin
>
> Marcin Wojtas (8):
> device property: Introduce fwnode_get_mac_address()
> device property: Introduce fwnode_get_phy_mode()
> mdio_bus: Introduce fwnode MDIO helpers
> net: mvmdio: add ACPI support
> net: mvpp2: simplify maintaining enabled ports' list
> net: mvpp2: use device_*/fwnode_* APIs instead of of_*
> net: mvpp2: handle PHY with its fwnode
> net: mvpp2: enable ACPI support in the driver
>
> drivers/base/property.c | 52 +++--
> drivers/net/ethernet/marvell/mvmdio.c | 42 +++-
> drivers/net/ethernet/marvell/mvpp2.c | 246 ++++++++++++--------
> drivers/net/phy/mdio_bus.c | 218 +++++++++++++++++
> include/linux/mdio.h | 3 +
> include/linux/property.h | 3 +
> 6 files changed, 454 insertions(+), 110 deletions(-)
>
Please CC linux-acpi on all submissions of patches touching ACPI,
property.c or property.h.
Thanks!
Hi Ard
2017-12-18 10:40 GMT+01:00 Ard Biesheuvel <[email protected]>:
> On 18 December 2017 at 10:17, Marcin Wojtas <[email protected]> wrote:
>> Hi,
>>
>> This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
>> First three patches introduce fwnode helpers for obtaining PHY
>> information from nodes and also MDIO fwnode API for registering
>> the bus with its PHY/devices.
>>
>> Following patches update code of the mvmdio and mvpp2 drivers
>> to support ACPI tables handling. The latter is done in 4 steps,
>> as can be seen in the commits. For the details, please
>> refer to the commit messages.
>>
>> Drivers operation was tested on top of the net-next branch
>> with both DT and ACPI. Although for compatibility reasons
>> with older platforms, the mvmdio driver keeps using
>> of_ MDIO registering, new fwnode_ one proved to fully work
>> with DT as well (tested on MacchiatoBin board).
>>
>> mvpp2/mvmdio driver can work with the ACPI representation, as exposed
>> on a public branch:
>> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
>> It was compiled together with the most recent Tianocore EDK2 revision.
>> Please refer to the firmware build instruction on MacchiatoBin board:
>> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>>
>> Above support configures 1G to use its PHY normally. 10G can work now
>> only with the link interrupt mode. Somehow reading of the
>> string property in fwnode_mdiobus_child_is_phy works only with
>> DT and cannot cope with 10G PHY nodes as in:
>> https://pastebin.com/3JnYpU0A
>>
>> Above root cause will be further checked. In the meantime I will
>> appreciate any comments or remarks for the kernel patches.
>>
>
> Hi Marcin,
>
> I have added linux-acpi and Graeme to cc. I think it makes sense to
> discuss the way you describe the device topology before looking at the
> patches in more detail.
>
Thanks. Tomasz Nowicki immediately pointed this to me off the lists.
> In particular, I would like to request feedback on the use of
> [redundant] 'reg' properties and the use of _DSD + compatible to
> describe PHYs. Usually, we try to avoid this, given that it is
> essentially a ACPI encapsulated DT dialect that is difficult to
> support in drivers unless they are based on DT to begin with. Also,
> non-standard _DSD properties require a vendor prefix, it is not
> freeform.
>
Already a lot of drivers use both DT + ACPI. Some have IMO very
fanciful bindings in both, mostly handled within the drivers with
custom functions. OF_ - only drivers can use of_mdio_ helper routines,
that assume a certain hierarchy:
MDIO device node with PHYs as children, which are referenced in the
ports node. I believe such approach could fit ACPI description too.
I'm aware however that my code is pretty much DT transposed into ACPI
environment and I'm of course open to discussion, how to do it in the
most proper way.
My main goal is to provide an fwnode-based glue code, that can be used
among the NIC/MDIO drivers (+ phylink) without multiple ifs
differentiating between ACPI/OF. What I sent has single calls in
mvpp2/mvmdio with a common bottom layers, but I don't see a problem,
that, e.g. when iterating over PHY subnodes, in case of OF
'reg'/'compatible' are used, whereas with ACPI some specific _ADR/_CID
objects.
I'd appreaciate any feedback.
Best regards,
Marcin
> For reference, the ACPI description is here (starting at line 175)
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
From: Marcin Wojtas <[email protected]>
Date: Mon, 18 Dec 2017 10:17:56 +0100
> Above support configures 1G to use its PHY normally. 10G can work now
> only with the link interrupt mode. Somehow reading of the
> string property in fwnode_mdiobus_child_is_phy works only with
> DT and cannot cope with 10G PHY nodes as in:
> https://pastebin.com/3JnYpU0A
>
> Above root cause will be further checked. In the meantime I will
> appreciate any comments or remarks for the kernel patches.
I would like you to figure this out before these changes go in.
Thanks.
Hi David,
2017-12-19 19:48 GMT+01:00 David Miller <[email protected]>:
> From: Marcin Wojtas <[email protected]>
> Date: Mon, 18 Dec 2017 10:17:56 +0100
>
>> Above support configures 1G to use its PHY normally. 10G can work now
>> only with the link interrupt mode. Somehow reading of the
>> string property in fwnode_mdiobus_child_is_phy works only with
>> DT and cannot cope with 10G PHY nodes as in:
>> https://pastebin.com/3JnYpU0A
>>
>> Above root cause will be further checked. In the meantime I will
>> appreciate any comments or remarks for the kernel patches.
>
> I would like you to figure this out before these changes go in.
>
Of course! v2 will not have such problem, I've been waiting however
for the feedback about the ACPI representation. Anyway, I'm strongly
leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so
maybe I'll just issue the v2 in order to push the discussion a bit
forward.
Thanks,
Marcin
> Of course! v2 will not have such problem, I've been waiting however
> for the feedback about the ACPI representation. Anyway, I'm strongly
> leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so
> maybe I'll just issue the v2 in order to push the discussion a bit
> forward.
Hi Marcin
I know ~0 about ACPI. But what seems to be missing for me is
documentation. You are defining a ABI here, which all future MDIO
busses, PHYs, and to some extent Ethernet switches need to follow. So
i would expect this to be documented somewhere.
How does documentation work in the ACPI world?
Andrew
Hi Andrew,
2017-12-19 21:46 GMT+01:00 Andrew Lunn <[email protected]>:
>> Of course! v2 will not have such problem, I've been waiting however
>> for the feedback about the ACPI representation. Anyway, I'm strongly
>> leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so
>> maybe I'll just issue the v2 in order to push the discussion a bit
>> forward.
>
> Hi Marcin
>
> I know ~0 about ACPI. But what seems to be missing for me is
> documentation. You are defining a ABI here, which all future MDIO
> busses, PHYs, and to some extent Ethernet switches need to follow. So
> i would expect this to be documented somewhere.
As the code derives straight from of_mdio.c, there is absolutely no
deviation from Documentation/devicetree/bindings/net/mdio.txt.
>
> How does documentation work in the ACPI world?
>
ACPI has its own specification, the newest one is 6.2
http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
It is pretty likely, that if we establish a generic solution for the
MDIO bus, a short section about it may probably require adding to
above - for now I see none.
Best regards,
Marcin
Hi Marcin,
On Mon, Dec 18, 2017 at 10:17:56AM +0100, Marcin Wojtas wrote:
>
> Marcin Wojtas (8):
> device property: Introduce fwnode_get_mac_address()
> device property: Introduce fwnode_get_phy_mode()
> mdio_bus: Introduce fwnode MDIO helpers
> net: mvmdio: add ACPI support
> net: mvpp2: simplify maintaining enabled ports' list
> net: mvpp2: use device_*/fwnode_* APIs instead of of_*
> net: mvpp2: handle PHY with its fwnode
> net: mvpp2: enable ACPI support in the driver
I tested your series on a mcbin, using the dt way. It still worked. If
it is relevant, you can add on the mvpp2 related patches:
Tested-by: Antoine Tenart <[email protected]>
Thanks!
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
> On 18 December 2017 at 10:17, Marcin Wojtas <[email protected]> wrote:
> > Hi,
> >
> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> > First three patches introduce fwnode helpers for obtaining PHY
> > information from nodes and also MDIO fwnode API for registering
> > the bus with its PHY/devices.
> >
> > Following patches update code of the mvmdio and mvpp2 drivers
> > to support ACPI tables handling. The latter is done in 4 steps,
> > as can be seen in the commits. For the details, please
> > refer to the commit messages.
> >
> > Drivers operation was tested on top of the net-next branch
> > with both DT and ACPI. Although for compatibility reasons
> > with older platforms, the mvmdio driver keeps using
> > of_ MDIO registering, new fwnode_ one proved to fully work
> > with DT as well (tested on MacchiatoBin board).
> >
> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> > on a public branch:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> > It was compiled together with the most recent Tianocore EDK2 revision.
> > Please refer to the firmware build instruction on MacchiatoBin board:
> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
> >
> > Above support configures 1G to use its PHY normally. 10G can work now
> > only with the link interrupt mode. Somehow reading of the
> > string property in fwnode_mdiobus_child_is_phy works only with
> > DT and cannot cope with 10G PHY nodes as in:
> > https://pastebin.com/3JnYpU0A
> >
> > Above root cause will be further checked. In the meantime I will
> > appreciate any comments or remarks for the kernel patches.
> >
>
> Hi Marcin,
>
> I have added linux-acpi and Graeme to cc. I think it makes sense to
> discuss the way you describe the device topology before looking at the
> patches in more detail.
>
> In particular, I would like to request feedback on the use of
> [redundant] 'reg' properties and the use of _DSD + compatible to
> describe PHYs. Usually, we try to avoid this, given that it is
> essentially a ACPI encapsulated DT dialect that is difficult to
> support in drivers unless they are based on DT to begin with. Also,
> non-standard _DSD properties require a vendor prefix, it is not
> freeform.
>
> For reference, the ACPI description is here (starting at line 175)
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
>
So the representation of PHY's with _DSD was kind of formalised here
http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
This is already in use in the kernel, and that DSDT seems to be along the same
lines. So seems ok in that manner.
The "reg", 0 property seems a little odd, it would probably make more
sense to check for the lack of ranges passed in in ACPI manner _CRS.
Graeme
Hi Graeme,
2018-01-03 12:00 GMT+01:00 Graeme Gregory <[email protected]>:
> On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
>> On 18 December 2017 at 10:17, Marcin Wojtas <[email protected]> wrote:
>> > Hi,
>> >
>> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
>> > First three patches introduce fwnode helpers for obtaining PHY
>> > information from nodes and also MDIO fwnode API for registering
>> > the bus with its PHY/devices.
>> >
>> > Following patches update code of the mvmdio and mvpp2 drivers
>> > to support ACPI tables handling. The latter is done in 4 steps,
>> > as can be seen in the commits. For the details, please
>> > refer to the commit messages.
>> >
>> > Drivers operation was tested on top of the net-next branch
>> > with both DT and ACPI. Although for compatibility reasons
>> > with older platforms, the mvmdio driver keeps using
>> > of_ MDIO registering, new fwnode_ one proved to fully work
>> > with DT as well (tested on MacchiatoBin board).
>> >
>> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
>> > on a public branch:
>> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
>> > It was compiled together with the most recent Tianocore EDK2 revision.
>> > Please refer to the firmware build instruction on MacchiatoBin board:
>> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>> >
>> > Above support configures 1G to use its PHY normally. 10G can work now
>> > only with the link interrupt mode. Somehow reading of the
>> > string property in fwnode_mdiobus_child_is_phy works only with
>> > DT and cannot cope with 10G PHY nodes as in:
>> > https://pastebin.com/3JnYpU0A
>> >
>> > Above root cause will be further checked. In the meantime I will
>> > appreciate any comments or remarks for the kernel patches.
>> >
>>
>> Hi Marcin,
>>
>> I have added linux-acpi and Graeme to cc. I think it makes sense to
>> discuss the way you describe the device topology before looking at the
>> patches in more detail.
>>
>> In particular, I would like to request feedback on the use of
>> [redundant] 'reg' properties and the use of _DSD + compatible to
>> describe PHYs. Usually, we try to avoid this, given that it is
>> essentially a ACPI encapsulated DT dialect that is difficult to
>> support in drivers unless they are based on DT to begin with. Also,
>> non-standard _DSD properties require a vendor prefix, it is not
>> freeform.
>>
>> For reference, the ACPI description is here (starting at line 175)
>> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
>>
> So the representation of PHY's with _DSD was kind of formalised here
>
> http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
>
> This is already in use in the kernel, and that DSDT seems to be along the same
> lines. So seems ok in that manner.
>
> The "reg", 0 property seems a little odd, it would probably make more
> sense to check for the lack of ranges passed in in ACPI manner _CRS.
>
I already agreed with 'reg' being awkward in the later emails.
Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
Thanks,
Marcin
> I already agreed with 'reg' being awkward in the later emails.
> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
Also, how do you specify which MDIO bus the PHY is on. To fully
specify a PHY, you need both bits of information.
In DT, the phy-handle phandle can point to any PHY anywhere in the
system. This is particularly important when a Ethernet device has two
MDIO busses.
Andrew
Hi Andrew,
2018-01-03 13:47 GMT+01:00 Andrew Lunn <[email protected]>:
>> I already agreed with 'reg' being awkward in the later emails.
>> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
>
> Also, how do you specify which MDIO bus the PHY is on. To fully
> specify a PHY, you need both bits of information.
>
> In DT, the phy-handle phandle can point to any PHY anywhere in the
> system. This is particularly important when a Ethernet device has two
> MDIO busses.
>
For now, my local MDIO bus description is pretty DT-like, i.e. master
bus with children PHYs:
Device (MDIO)
{
Name (_HID, "MRVL0100") //
_HID: Hardware ID
Name (_UID, 0x00) //
_UID: Unique ID
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
0xf212a200, // Address Base
0x00000010, //
Address Length
)
})
Device (GPHY)
{
Name (_ADR, 0x0)
}
}
Device (XSMI)
{
Name (_HID, "MRVL0101") //
_HID: Hardware ID
Name (_UID, 0x00) //
_UID: Unique ID
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
0xf212a600, // Address Base
0x00000010, //
Address Length
)
})
Device (PHY0)
{
Name (_ADR, 0x0)
Name (_CID, "ethernet-phy-ieee802.3-c45")
}
Device (PHY8)
{
Name (_ADR, 0x8)
Name (_CID, "ethernet-phy-ieee802.3-c45")
}
}
Which is referenced in the port's node:
Package () { "phy", Package (){\_SB.XSMI.PHY0}},
I'm studying an alternatives with graphs, as suggested by Tomasz
Nowicki, but to me above is pretty natural and not complicated.
Best regards,
Marcin
On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote:
> Hi Andrew,
>
> 2018-01-03 13:47 GMT+01:00 Andrew Lunn <[email protected]>:
> >> I already agreed with 'reg' being awkward in the later emails.
> >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> >
> > Also, how do you specify which MDIO bus the PHY is on. To fully
> > specify a PHY, you need both bits of information.
> >
> > In DT, the phy-handle phandle can point to any PHY anywhere in the
> > system. This is particularly important when a Ethernet device has two
> > MDIO busses.
> >
>
> For now, my local MDIO bus description is pretty DT-like, i.e. master
> bus with children PHYs:
> Device (MDIO)
> {
> Name (_HID, "MRVL0100") //
> _HID: Hardware ID
> Name (_UID, 0x00) //
> _UID: Unique ID
> Name (_CRS, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite,
> 0xf212a200, // Address Base
> 0x00000010, //
> Address Length
> )
> })
> Device (GPHY)
> {
> Name (_ADR, 0x0)
> }
> }
>
> Device (XSMI)
> {
> Name (_HID, "MRVL0101") //
> _HID: Hardware ID
> Name (_UID, 0x00) //
> _UID: Unique ID
> Name (_CRS, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite,
> 0xf212a600, // Address Base
> 0x00000010, //
> Address Length
> )
> })
> Device (PHY0)
> {
> Name (_ADR, 0x0)
> Name (_CID, "ethernet-phy-ieee802.3-c45")
> }
> Device (PHY8)
> {
> Name (_ADR, 0x8)
> Name (_CID, "ethernet-phy-ieee802.3-c45")
> }
> }
>
> Which is referenced in the port's node:
>
> Package () { "phy", Package (){\_SB.XSMI.PHY0}},
Hi Marcin
This reference looks good, giving both the bus and the PHY on the bus.
I assume you can use references like this within the Device (PHY8)
node? You need to be able to reference a GPIO used for resetting the
PHY. And you also need to reference a GPIO at the Device (MDIO) level
for resetting all the PHYs on the MDIO bus.
Andrew
2018-01-03 14:33 GMT+01:00 Andrew Lunn <[email protected]>:
> On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote:
>> Hi Andrew,
>>
>> 2018-01-03 13:47 GMT+01:00 Andrew Lunn <[email protected]>:
>> >> I already agreed with 'reg' being awkward in the later emails.
>> >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
>> >
>> > Also, how do you specify which MDIO bus the PHY is on. To fully
>> > specify a PHY, you need both bits of information.
>> >
>> > In DT, the phy-handle phandle can point to any PHY anywhere in the
>> > system. This is particularly important when a Ethernet device has two
>> > MDIO busses.
>> >
>>
>> For now, my local MDIO bus description is pretty DT-like, i.e. master
>> bus with children PHYs:
>> Device (MDIO)
>> {
>> Name (_HID, "MRVL0100") //
>> _HID: Hardware ID
>> Name (_UID, 0x00) //
>> _UID: Unique ID
>> Name (_CRS, ResourceTemplate ()
>> {
>> Memory32Fixed (ReadWrite,
>> 0xf212a200, // Address Base
>> 0x00000010, //
>> Address Length
>> )
>> })
>> Device (GPHY)
>> {
>> Name (_ADR, 0x0)
>> }
>> }
>>
>> Device (XSMI)
>> {
>> Name (_HID, "MRVL0101") //
>> _HID: Hardware ID
>> Name (_UID, 0x00) //
>> _UID: Unique ID
>> Name (_CRS, ResourceTemplate ()
>> {
>> Memory32Fixed (ReadWrite,
>> 0xf212a600, // Address Base
>> 0x00000010, //
>> Address Length
>> )
>> })
>> Device (PHY0)
>> {
>> Name (_ADR, 0x0)
>> Name (_CID, "ethernet-phy-ieee802.3-c45")
>> }
>> Device (PHY8)
>> {
>> Name (_ADR, 0x8)
>> Name (_CID, "ethernet-phy-ieee802.3-c45")
>> }
>> }
>>
>> Which is referenced in the port's node:
>>
>> Package () { "phy", Package (){\_SB.XSMI.PHY0}},
>
> Hi Marcin
>
> This reference looks good, giving both the bus and the PHY on the bus.
>
> I assume you can use references like this within the Device (PHY8)
> node?
Yes.
> You need to be able to reference a GPIO used for resetting the
> PHY. And you also need to reference a GPIO at the Device (MDIO) level
> for resetting all the PHYs on the MDIO bus.
>
Yes, for full support of PHYs the GPIO must be supported, as well as
the PHY's IRQs.
Best regards,
Marcin
On Wed, Jan 03, 2018 at 12:12:06PM +0100, Marcin Wojtas wrote:
> Hi Graeme,
>
> 2018-01-03 12:00 GMT+01:00 Graeme Gregory <[email protected]>:
> > On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
> >> On 18 December 2017 at 10:17, Marcin Wojtas <[email protected]> wrote:
> >> > Hi,
> >> >
> >> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> >> > First three patches introduce fwnode helpers for obtaining PHY
> >> > information from nodes and also MDIO fwnode API for registering
> >> > the bus with its PHY/devices.
> >> >
> >> > Following patches update code of the mvmdio and mvpp2 drivers
> >> > to support ACPI tables handling. The latter is done in 4 steps,
> >> > as can be seen in the commits. For the details, please
> >> > refer to the commit messages.
> >> >
> >> > Drivers operation was tested on top of the net-next branch
> >> > with both DT and ACPI. Although for compatibility reasons
> >> > with older platforms, the mvmdio driver keeps using
> >> > of_ MDIO registering, new fwnode_ one proved to fully work
> >> > with DT as well (tested on MacchiatoBin board).
> >> >
> >> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> >> > on a public branch:
> >> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> >> > It was compiled together with the most recent Tianocore EDK2 revision.
> >> > Please refer to the firmware build instruction on MacchiatoBin board:
> >> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
> >> >
> >> > Above support configures 1G to use its PHY normally. 10G can work now
> >> > only with the link interrupt mode. Somehow reading of the
> >> > string property in fwnode_mdiobus_child_is_phy works only with
> >> > DT and cannot cope with 10G PHY nodes as in:
> >> > https://pastebin.com/3JnYpU0A
> >> >
> >> > Above root cause will be further checked. In the meantime I will
> >> > appreciate any comments or remarks for the kernel patches.
> >> >
> >>
> >> Hi Marcin,
> >>
> >> I have added linux-acpi and Graeme to cc. I think it makes sense to
> >> discuss the way you describe the device topology before looking at the
> >> patches in more detail.
> >>
> >> In particular, I would like to request feedback on the use of
> >> [redundant] 'reg' properties and the use of _DSD + compatible to
> >> describe PHYs. Usually, we try to avoid this, given that it is
> >> essentially a ACPI encapsulated DT dialect that is difficult to
> >> support in drivers unless they are based on DT to begin with. Also,
> >> non-standard _DSD properties require a vendor prefix, it is not
> >> freeform.
> >>
> >> For reference, the ACPI description is here (starting at line 175)
> >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
> >>
> > So the representation of PHY's with _DSD was kind of formalised here
> >
> > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
> >
> > This is already in use in the kernel, and that DSDT seems to be along the same
> > lines. So seems ok in that manner.
> >
> > The "reg", 0 property seems a little odd, it would probably make more
> > sense to check for the lack of ranges passed in in ACPI manner _CRS.
> >
>
> I already agreed with 'reg' being awkward in the later emails.
> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
>
Ah it is an actual address, then yes _ADR is probably more appropriate.
Graeme
> > I already agreed with 'reg' being awkward in the later emails.
> > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> >
> Ah it is an actual address, then yes _ADR is probably more appropriate.
Newbie ACPI question. What is the definition of an address?
In this cause, we are talking about an address of a device on an MDIO
bus. It takes a value between 0 and 31.
How are IC2 device addresses represented in ACPI? MDIO devices and I2C
devices are pretty similar. So it would make sense to use the same as
what I2C uses.
Andrew
On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > I already agreed with 'reg' being awkward in the later emails.
> > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > >
> > Ah it is an actual address, then yes _ADR is probably more appropriate.
>
> Newbie ACPI question. What is the definition of an address?
>
> In this cause, we are talking about an address of a device on an MDIO
> bus. It takes a value between 0 and 31.
>
> How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> devices are pretty similar. So it would make sense to use the same as
> what I2C uses.
>
Too big (and has table) to sensibly quote, but defined in ACPI spec
6.1.1 _ADR (Address)
Ive never though been quite sure if that is just an example list of
address types or its supposed to be canonical (in which case some ECRs
are needed to the spec).
Thanks
Graeme
On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote:
> On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > > I already agreed with 'reg' being awkward in the later emails.
> > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > > >
> > > Ah it is an actual address, then yes _ADR is probably more appropriate.
> >
> > Newbie ACPI question. What is the definition of an address?
> >
> > In this cause, we are talking about an address of a device on an MDIO
> > bus. It takes a value between 0 and 31.
> >
> > How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> > devices are pretty similar. So it would make sense to use the same as
> > what I2C uses.
> >
> Too big (and has table) to sensibly quote, but defined in ACPI spec
>
> 6.1.1 _ADR (Address)
>
> Ive never though been quite sure if that is just an example list of
> address types or its supposed to be canonical (in which case some ECRs
> are needed to the spec).
Hi Graeme
I took a quick look at version 6.2, and noticed i2c devices use
_ADR(). So using it for MDIO seems O.K.
However, i2c, spi and uart devices all seem to be described using
GenericSerialBus. Maybe the correct way to describe MDIO devices is to
also use GenericSerialBus?
Andrew
On Mon, Jan 08, 2018 at 03:53:12PM +0100, Andrew Lunn wrote:
> On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote:
> > On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > > > I already agreed with 'reg' being awkward in the later emails.
> > > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > > > >
> > > > Ah it is an actual address, then yes _ADR is probably more appropriate.
> > >
> > > Newbie ACPI question. What is the definition of an address?
> > >
> > > In this cause, we are talking about an address of a device on an MDIO
> > > bus. It takes a value between 0 and 31.
> > >
> > > How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> > > devices are pretty similar. So it would make sense to use the same as
> > > what I2C uses.
> > >
> > Too big (and has table) to sensibly quote, but defined in ACPI spec
> >
> > 6.1.1 _ADR (Address)
> >
> > Ive never though been quite sure if that is just an example list of
> > address types or its supposed to be canonical (in which case some ECRs
> > are needed to the spec).
>
> Hi Graeme
>
> I took a quick look at version 6.2, and noticed i2c devices use
> _ADR(). So using it for MDIO seems O.K.
>
> However, i2c, spi and uart devices all seem to be described using
> GenericSerialBus. Maybe the correct way to describe MDIO devices is to
> also use GenericSerialBus?
>
I am not familiar with MDIO, but if its similar or a specific
implementation of a serial bus that does sound sane!
Graeme
w> I am not familiar with MDIO, but if its similar or a specific
> implementation of a serial bus that does sound sane!
It is a two wire serial bus. A good overview can be found here:
https://www.totalphase.com/support/articles/200349206-MDIO-Background
Andrew
Hi Andrew,
2018-01-08 16:42 GMT+01:00 Andrew Lunn <[email protected]>:
> w> I am not familiar with MDIO, but if its similar or a specific
>> implementation of a serial bus that does sound sane!
>
Thanks for digging, I will check if and how we can use
GenericSerialBus with MDIO.
Best regards,
Marcin
> It is a two wire serial bus. A good overview can be found here:
>
> https://www.totalphase.com/support/articles/200349206-MDIO-Background
>
On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> Hi Andrew,
>
>
>
> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <[email protected]>:
> > w> I am not familiar with MDIO, but if its similar or a specific
> >> implementation of a serial bus that does sound sane!
> >
>
> Thanks for digging, I will check if and how we can use
> GenericSerialBus with MDIO.
>
Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
across similar on other ARM boards.
Graeme
2018-01-09 11:19 GMT+01:00 Graeme Gregory <[email protected]>:
> On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
>> Hi Andrew,
>>
>>
>>
>> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <[email protected]>:
>> > w> I am not familiar with MDIO, but if its similar or a specific
>> >> implementation of a serial bus that does sound sane!
>> >
>>
>> Thanks for digging, I will check if and how we can use
>> GenericSerialBus with MDIO.
>>
> Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> across similar on other ARM boards.
>
I'm looking forward to their feedback, however, what I've noticed,
each driver handles mdio/phys on its own, not using any generic
solution, which is what I need to actually avoid :)
Best regards,
Marcin
On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote:
> 2018-01-09 11:19 GMT+01:00 Graeme Gregory <[email protected]>:
> > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> >> Hi Andrew,
> >>
> >>
> >>
> >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <[email protected]>:
> >> > w> I am not familiar with MDIO, but if its similar or a specific
> >> >> implementation of a serial bus that does sound sane!
> >> >
> >>
> >> Thanks for digging, I will check if and how we can use
> >> GenericSerialBus with MDIO.
> >>
> > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> > across similar on other ARM boards.
> >
>
> I'm looking forward to their feedback, however, what I've noticed,
> each driver handles mdio/phys on its own, not using any generic
> solution, which is what I need to actually avoid :)
Agreed. Lets define it once for all drivers using phylib/phylink.
Andrew
[+cc Mika]
On Tue, Jan 09, 2018 at 02:00:12PM +0100, Andrew Lunn wrote:
> On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote:
> > 2018-01-09 11:19 GMT+01:00 Graeme Gregory <[email protected]>:
> > > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> > >> Hi Andrew,
> > >>
> > >>
> > >>
> > >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <[email protected]>:
> > >> > w> I am not familiar with MDIO, but if its similar or a specific
> > >> >> implementation of a serial bus that does sound sane!
> > >> >
> > >>
> > >> Thanks for digging, I will check if and how we can use
> > >> GenericSerialBus with MDIO.
> > >>
> > > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> > > across similar on other ARM boards.
> > >
> >
> > I'm looking forward to their feedback, however, what I've noticed,
> > each driver handles mdio/phys on its own, not using any generic
> > solution, which is what I need to actually avoid :)
>
> Agreed. Lets define it once for all drivers using phylib/phylink.
To start with, I am not entirely familiar with MDIO, apologies in
advance. Building something on top of GenericSerialBus sounds correct
but if I am not mistaken you would need a new bus type in the ACPI
specs.
I CC'ed Mika since he is more familiar with handling these bits of ACPI
specs - I wonder whether this is a problem that cropped up on x86
systems too.
I do not think there is one and only answer but there must be a single
set of bindings and if the ACPI specs already cater for some of them
we have to reuse them.
Please take some time to ensure the solution you are pushing is widely
deployable.
Thanks,
Lorenzo
> I CC'ed Mika since he is more familiar with handling these bits of ACPI
> specs - I wonder whether this is a problem that cropped up on x86
> systems too.
Hi Lorenzo
There is nothing about MDIO, PHYs, Ethernet switches, etc in version
6.2 of the spec. If x86 has this, it must be after 6.2 was released.
I would not be too surprised if x86 has none of this. If you look at
the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
are all PCI devices, and hide all this.
> I do not think there is one and only answer but there must be a single
> set of bindings and if the ACPI specs already cater for some of them
> we have to reuse them.
Agreed. Due diligence so far suggests there is nothing already
defined. But im a newbie to ACPI, so could be looking in the wrong
place. I really hope there is somebody like Rob Herring, the DT
maintainer, who keeps an eye on all ACPI talk and would tell us if we
are heading off in the wrong direction.
Andrew
Hi Mika,
2018-01-18 14:00 GMT+01:00 Andrew Lunn <[email protected]>:
>> I CC'ed Mika since he is more familiar with handling these bits of ACPI
>> specs - I wonder whether this is a problem that cropped up on x86
>> systems too.
>
> Hi Lorenzo
>
> There is nothing about MDIO, PHYs, Ethernet switches, etc in version
> 6.2 of the spec. If x86 has this, it must be after 6.2 was released.
> I would not be too surprised if x86 has none of this. If you look at
> the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
> are all PCI devices, and hide all this.
>
>> I do not think there is one and only answer but there must be a single
>> set of bindings and if the ACPI specs already cater for some of them
>> we have to reuse them.
>
> Agreed. Due diligence so far suggests there is nothing already
> defined. But im a newbie to ACPI, so could be looking in the wrong
> place. I really hope there is somebody like Rob Herring, the DT
> maintainer, who keeps an eye on all ACPI talk and would tell us if we
> are heading off in the wrong direction.
>
My initial approach with MDIO bus with PHYs as child nodes was super
easy to describe and handle in Linux - please refer to:
- typical representation of mdio bus with the phys -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8
- my patches in the initial series
However I guess it would be more proper to use the
GenericSerialBus-based description, as advised in ACPI Spec. The
question is, whether we should define new type of a bus or not
(MdioSerialBus, similar to e.g. I2cSerialBus).
Since I have a code that can be tested and easily modified to use
different ACPI approaches with real platform MDIO controller
(mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
some time to prepare a proof of concept based on GenericSerialBus.
Please expect some RFC patches hopefully right after the coming merge
window is closed.
Of course, if I come up on some ACPI - specific implementation
questions, I won't hesitate to ask in this thred. I will also
appreciate any hints. For now my two main concerns are:
- The PHY address on the mdio bus - should it be put into _CRS ->
GenericSerialBus() field or separate _ADR? I'd lean towards first
option.
- The PHY type - in Linux it's resolved basing on two generic
compatible strings
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8).
I'd put it as a sort of ID into GenericSerialBus(). If you agree - any
specific filed that you would try to use?
Do I understand correctly that the MDIO controller node should
comprise OperationRegion() definition of the GenericSerialBus?
I'm looking forward to your feedback.
Thanks,
Marcin
> Since I have a code that can be tested and easily modified to use
> different ACPI approaches with real platform MDIO controller
> (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
> some time to prepare a proof of concept based on GenericSerialBus.
> Please expect some RFC patches hopefully right after the coming merge
> window is closed.
It would also be interesting to know how the standardisation process
works.
I'm sure these is a FAQs, so maybe somebody could point us towards it.
Do we need to submit a proposed extension to the ACPI standard at the
same time as the patch? Should we not accept the code into Linux until
the proposal has been accepted? Or can we accept the code
provisionally, with the understanding that if the ACPI committee
rejects the extension, or suggest alternations, we can take the code
out of Linux without having to worry about backwards compatibility?
Thanks
Andrew
On Fri, Jan 19, 2018 at 07:07:29PM +0100, Marcin Wojtas wrote:
> Hi Mika,
Hi,
> 2018-01-18 14:00 GMT+01:00 Andrew Lunn <[email protected]>:
> >> I CC'ed Mika since he is more familiar with handling these bits of ACPI
> >> specs - I wonder whether this is a problem that cropped up on x86
> >> systems too.
> >
> > Hi Lorenzo
> >
> > There is nothing about MDIO, PHYs, Ethernet switches, etc in version
> > 6.2 of the spec. If x86 has this, it must be after 6.2 was released.
> > I would not be too surprised if x86 has none of this. If you look at
> > the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
> > are all PCI devices, and hide all this.
> >
> >> I do not think there is one and only answer but there must be a single
> >> set of bindings and if the ACPI specs already cater for some of them
> >> we have to reuse them.
> >
> > Agreed. Due diligence so far suggests there is nothing already
> > defined. But im a newbie to ACPI, so could be looking in the wrong
> > place. I really hope there is somebody like Rob Herring, the DT
> > maintainer, who keeps an eye on all ACPI talk and would tell us if we
> > are heading off in the wrong direction.
> >
>
> My initial approach with MDIO bus with PHYs as child nodes was super
> easy to describe and handle in Linux - please refer to:
> - typical representation of mdio bus with the phys -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8
> - my patches in the initial series
> However I guess it would be more proper to use the
> GenericSerialBus-based description, as advised in ACPI Spec. The
> question is, whether we should define new type of a bus or not
> (MdioSerialBus, similar to e.g. I2cSerialBus).
I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
would be to follow what SDIO is doing, e.g have the PHY devices listed
below the MDIO controller and use _ADR to describe their "address" on
that bus. You can see how _ADR applies to SDIO bus from ACPI spec.
Of course ACPI spec should then be updated accordingly to describe what
_ADR means for devices on MDIO bus.
> Since I have a code that can be tested and easily modified to use
> different ACPI approaches with real platform MDIO controller
> (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
> some time to prepare a proof of concept based on GenericSerialBus.
> Please expect some RFC patches hopefully right after the coming merge
> window is closed.
>
> Of course, if I come up on some ACPI - specific implementation
> questions, I won't hesitate to ask in this thred. I will also
> appreciate any hints. For now my two main concerns are:
> - The PHY address on the mdio bus - should it be put into _CRS ->
> GenericSerialBus() field or separate _ADR? I'd lean towards first
> option.
> - The PHY type - in Linux it's resolved basing on two generic
> compatible strings
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8).
> I'd put it as a sort of ID into GenericSerialBus(). If you agree - any
> specific filed that you would try to use?
If you go with the SDIO way then each PHY is described as normal ACPI
device and you can use ACPI _HID/_CID to match the device to the
corresponding driver.
> Do I understand correctly that the MDIO controller node should
> comprise OperationRegion() definition of the GenericSerialBus?
I don't think OpRegions are useful in this case because they are mainly
used to allow BIOS AML code to access the hardware through OS driver.
> I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
> would be to follow what SDIO is doing, e.g have the PHY devices listed
> below the MDIO controller and use _ADR to describe their "address" on
> that bus. You can see how _ADR applies to SDIO bus from ACPI spec.
Hi Mika
SDIO is not a serial bus, well it can be in its simplest form, but
high speed implementations have 4 data lines. So i can understand them
not using GenericSerialBus.
MDIO is a serial bus, very similar to SPI, I2C, and UART.
> If you go with the SDIO way then each PHY is described as normal ACPI
> device and you can use ACPI _HID/_CID to match the device to the
> corresponding driver.
Just some background here. If you have a plain PHY as a device on an
MDIO bus, you don't need to match it to a driver within ACPI.
Registers 2 and 3 contain a vendor and product ID. That is what it
used to match the device to the driver.
What you might need to know is the protocol to talk on the bus. Most
devices use clause 22 protocol. A few devices are clause 45. 22 is the
default in Linux, and you need to indicate if 45 should be used. You
can also indicate 22.
It gets more complex when the device on the bus is not a PHY. It is a
generic bus, you can connect anything to it. Ethernet switches can be
on the bus. They generally cannot be identified using registers 2 and
3. So you do need to match the device to the driver. Most do have ID
registers, so the driver can work out what specific device is on the
bus. However, Marvell moved the ID registers on there newer generation
of devices, so we need to give the driver a hint where to look. So in
device tree, we have two different compatible string.
Broadcom really do use it as a generic bus. They have their USB PHYs
and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to
match the device to the driver, as normal.
We need to ensure what we define for ACPI has the same level of
flexibility.
Andrew
On Sun, Jan 21, 2018 at 02:08:40AM +0100, Andrew Lunn wrote:
> > I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
> > would be to follow what SDIO is doing, e.g have the PHY devices listed
> > below the MDIO controller and use _ADR to describe their "address" on
> > that bus. You can see how _ADR applies to SDIO bus from ACPI spec.
>
> Hi Mika
>
> SDIO is not a serial bus, well it can be in its simplest form, but
> high speed implementations have 4 data lines. So i can understand them
> not using GenericSerialBus.
Well, I think SDIO is more of a serial bus pretty much the same way than
SPI but it can support more data lines as needed (in the same way than
SPI). But I'm not an expert in SDIO.
However, that's not the point here :-) SATA (which is definitely a serial
bus) uses the same mechanism and not GenericSerialBus.
> MDIO is a serial bus, very similar to SPI, I2C, and UART.
>
> > If you go with the SDIO way then each PHY is described as normal ACPI
> > device and you can use ACPI _HID/_CID to match the device to the
> > corresponding driver.
>
> Just some background here. If you have a plain PHY as a device on an
> MDIO bus, you don't need to match it to a driver within ACPI.
> Registers 2 and 3 contain a vendor and product ID. That is what it
> used to match the device to the driver.
>
> What you might need to know is the protocol to talk on the bus. Most
> devices use clause 22 protocol. A few devices are clause 45. 22 is the
> default in Linux, and you need to indicate if 45 should be used. You
> can also indicate 22.
>
> It gets more complex when the device on the bus is not a PHY. It is a
> generic bus, you can connect anything to it. Ethernet switches can be
> on the bus. They generally cannot be identified using registers 2 and
> 3. So you do need to match the device to the driver. Most do have ID
> registers, so the driver can work out what specific device is on the
> bus. However, Marvell moved the ID registers on there newer generation
> of devices, so we need to give the driver a hint where to look. So in
> device tree, we have two different compatible string.
>
> Broadcom really do use it as a generic bus. They have their USB PHYs
> and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to
> match the device to the driver, as normal.
OK, thanks for the explanation.
> We need to ensure what we define for ACPI has the same level of
> flexibility.
Right. So if you need to have some additional "parameters" with the
connection, then I suppose you may want to go with the GenericSerialBus
route. However, looking at the sample device tree description:
davinci_mdio: ethernet@5c030000 {
compatible = "ti,davinci_mdio";
reg = <0x5c030000 0x1000>;
#address-cells = <1>;
#size-cells = <0>;
reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
reset-delay-us = <2>;
ethphy0: ethernet-phy@1 {
reg = <1>;
};
ethphy1: ethernet-phy@3 {
reg = <3>;
};
};
would pretty much translate directly to this in ACPI if you don't need
any additional attributes:
Device (ETH0) {
Name (_ADR, /* PCI address of the NIC */)
Device (PHY0) {
Name (_ADR, 1)
...
}
Device (PHY1) {
Name (_ADR, 3)
...
}
}
which looks pretty simple to me. You can also use _DSM and _DSD here to
pass information (like the protocol number) for the PHY devices to Linux.
> Right. So if you need to have some additional "parameters" with the
> connection, then I suppose you may want to go with the GenericSerialBus
> route. However, looking at the sample device tree description:
>
> davinci_mdio: ethernet@5c030000 {
> compatible = "ti,davinci_mdio";
> reg = <0x5c030000 0x1000>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> reset-delay-us = <2>;
>
> ethphy0: ethernet-phy@1 {
> reg = <1>;
> };
>
> ethphy1: ethernet-phy@3 {
> reg = <3>;
> };
> };
>
> would pretty much translate directly to this in ACPI if you don't need
> any additional attributes:
>
> Device (ETH0) {
> Name (_ADR, /* PCI address of the NIC */)
>
> Device (PHY0) {
> Name (_ADR, 1)
> ...
> }
>
> Device (PHY1) {
> Name (_ADR, 3)
> ...
> }
> }
>
> which looks pretty simple to me. You can also use _DSM and _DSD here to
> pass information (like the protocol number) for the PHY devices to Linux.
I'm not particularly worried about that simple case. Other than, i
don't want people to think that is all that is required.
For a more full example, take a look at vf610-zii-dev-rev-b.dts. The
Freescale FEC Ethernet controller provides the base MDIO device,
mdio1. On top of this is an MDIO mux, using a few GPIO lines to
enable/disable 3 child MDIO busses. Each of these busses has an
Ethernet Switch. The Ethernet switch exports up to two MDIO busses,
and on these busses are Ethernet PHYs which are embedded inside the
switch. The Ethernet switches are also interrupt controllers, with the
PHYs having interrupt properties which point back to the interrupt
controller in the switch.
So i'm interested in an ACPI proposal which supports this board.
Andrew
On 21 January 2018 at 16:13, Andrew Lunn <[email protected]> wrote:
>> Right. So if you need to have some additional "parameters" with the
>> connection, then I suppose you may want to go with the GenericSerialBus
>> route. However, looking at the sample device tree description:
>>
>> davinci_mdio: ethernet@5c030000 {
>> compatible = "ti,davinci_mdio";
>> reg = <0x5c030000 0x1000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> reset-delay-us = <2>;
>>
>> ethphy0: ethernet-phy@1 {
>> reg = <1>;
>> };
>>
>> ethphy1: ethernet-phy@3 {
>> reg = <3>;
>> };
>> };
>>
>> would pretty much translate directly to this in ACPI if you don't need
>> any additional attributes:
>>
>> Device (ETH0) {
>> Name (_ADR, /* PCI address of the NIC */)
>>
>> Device (PHY0) {
>> Name (_ADR, 1)
>> ...
>> }
>>
>> Device (PHY1) {
>> Name (_ADR, 3)
>> ...
>> }
>> }
>>
>> which looks pretty simple to me. You can also use _DSM and _DSD here to
>> pass information (like the protocol number) for the PHY devices to Linux.
>
> I'm not particularly worried about that simple case. Other than, i
> don't want people to think that is all that is required.
>
> For a more full example, take a look at vf610-zii-dev-rev-b.dts. The
> Freescale FEC Ethernet controller provides the base MDIO device,
> mdio1. On top of this is an MDIO mux, using a few GPIO lines to
> enable/disable 3 child MDIO busses. Each of these busses has an
> Ethernet Switch. The Ethernet switch exports up to two MDIO busses,
> and on these busses are Ethernet PHYs which are embedded inside the
> switch. The Ethernet switches are also interrupt controllers, with the
> PHYs having interrupt properties which point back to the interrupt
> controller in the switch.
>
> So i'm interested in an ACPI proposal which supports this board.
>
However interesting as an example, I'm not convinced this is what we
should aim for.
ACPI is not a replacement for DT, and it is unlikely that people would
be interested in running ACPI-only distros such as RHEL on their
Ethernet switch. DT is excellent at describing this, and there is no
need to replace it.
ACPI is about firmware abstractions: you don't need to describe every
stacked interrupt controller in minute detail to the OS if the
firmware configures it sufficiently. That way, the OS does not need to
know all these details, and vendors can update their hardware without
having to update the software as well. (Or that is the idea at least,
how that works out in practice on arm64 systems remains to be seen)
Taking the Marvell 8040 as an example: the ACPI description does not
expose the ICU interrupt controllers to the OS, but the firmware
configures them and describes their configured state as ordinary GIC
interrupts.
Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI
forum, and only members (who are all under a contract regarding
reasonable and non-discriminatory (RAND) licensing terms to IP they
own that is covered by the spec) can contribute. Individuals can
become members for free AFAIR, but that doesn't mean individuals are
typically interested in getting involved in a process that is rather
tedious and bureaucratic.
So my suggestion would be to find out to what extent the latest
version of the spec can describe non-trivial MDIO topologies, and
hopefully that includes the mvpp2 on the Marvell 8040.
> However interesting as an example, I'm not convinced this is what we
> should aim for.
>
> ACPI is not a replacement for DT, and it is unlikely that people would
> be interested in running ACPI-only distros such as RHEL on their
> Ethernet switch. DT is excellent at describing this, and there is no
> need to replace it.
I however do know of somebody who is building an industrial wireless
access point, with a Marvell Ethernet switch. They have chosen to use
an Intel CPU, and want to run CentOS on it, because that is what they
know.
So they will be using ACPI whatever, for the basic board support. They
then have a choice of either ACPI for the switch, or having device
tree as well as ACPI for the switch. And it will be interesting to see
how that works. Can DT reference GPIOs and I2C busses in ACPI?
> Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI
> forum, and only members (who are all under a contract regarding
> reasonable and non-discriminatory (RAND) licensing terms to IP they
> own that is covered by the spec) can contribute. Individuals can
> become members for free AFAIR, but that doesn't mean individuals are
> typically interested in getting involved in a process that is rather
> tedious and bureaucratic.
And this is a bit what i'm worried about. How you represent MDIO
busses, PHYs, Ethernet switches is implemented once in the core Linux
code. So i would be interested in knowing what happens if the Linux
community defines and implements something, boards start using it, but
a year later the tedious and bureaucratic process rejects it,
re-writes it, in an incompatible way.
Andrew