| Message ID | 1365071235-11611-2-git-send-email-florian@openwrt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Florian On Thu, Apr 04, 2013 at 12:27:11PM +0200, Florian Fainelli wrote: > This patch adds Device Tree bindings following the already defined > bindings at Documentation/devicetree/bindings/marvell.txt. The binding > documentation is also enhanced with new optionnal properties required > for supporting certain devices (RX/TX queue and SRAM). Since we now have > proper support for the orion MDIO bus driver, there is no need to fiddle > around with device tree phandles. PHY-less (MAC connected to switch) > configurations are supported by not specifying any phy phandle for an > ethernet node. > > Signed-off-by: Florian Fainelli <florian@openwrt.org> > --- > - properly ifdef of_platform_bus_probe with CONFIG_OF > - handle of_platform_bus_probe errors and cleanup accordingly > - use of_property_read_u32 where applicable > - parse "duplex" and "speed" property in PHY-less configuration > > Documentation/devicetree/bindings/marvell.txt | 25 +++++- > drivers/net/ethernet/marvell/mv643xx_eth.c | 120 ++++++++++++++++++++++++- > 2 files changed, 140 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/marvell.txt b/Documentation/devicetree/bindings/marvell.txt > index f1533d9..e70a013 100644 > --- a/Documentation/devicetree/bindings/marvell.txt > +++ b/Documentation/devicetree/bindings/marvell.txt > @@ -112,9 +112,14 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. > Required properties: > - #address-cells : <1> > - #size-cells : <0> > - - compatible : "marvell,mv64360-eth-block" > + - compatible : "marvell,mv64360-eth-block", "marvell,mv64360-eth-group", > + "marvell,mv643xx-eth-block" > - reg : Offset and length of the register set for this block > > + Optional properties: > + - tx-csum-limit : Hardware limit above which transmit checksumming > + is disabled. > + > Example Discovery Ethernet block node: > ethernet-block@2000 { > #address-cells = <1>; > @@ -130,7 +135,7 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. > > Required properties: > - device_type : Should be "network". > - - compatible : Should be "marvell,mv64360-eth". > + - compatible : Should be "marvell,mv64360-eth", "marvell,mv643xx-eth". > - reg : Should be <0>, <1>, or <2>, according to which registers > within the silicon block the device uses. > - interrupts : <a> where a is the interrupt number for the port. > @@ -140,6 +145,22 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. > controller. > - local-mac-address : 6 bytes, MAC address > > + Optional properties: > + - clocks : Phandle to the clock control device and gate bit > + - clock-names : String describing the clock gate bit > + - speed : Speed to force the link (10, 100, 1000), used when no > + phy property is defined > + - duplex : Duplex to force the link (0: half, 1: full), used when no > + phy property is defined > + - rx-queue-count : number of RX queues to use > + - tx-queue-count : number of TX queues to use > + - rx-queue-size : size of the RX queue (in bytes) > + - tx-queue-size : size of the TX queue (in bytes) > + - rx-sram-addr : address of the SRAM for RX path (non 0 means used) > + - rx-sram-size : size of the SRAM for RX path (non 0 means used) > + - tx-sram-addr : address of the SRAM for TX path (non 0 means used) > + - tx-sram-size : size of the SRAM for TX path (non 0 means used) > + > Example Discovery Ethernet port node: > ethernet@0 { > device_type = "network"; > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > index aedbd82..75599a8 100644 > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > @@ -60,6 +60,10 @@ > #include <linux/inet_lro.h> > #include <linux/slab.h> > #include <linux/clk.h> > +#include <linux/stringify.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/of_net.h> > > static char mv643xx_eth_driver_name[] = "mv643xx_eth"; > static char mv643xx_eth_driver_version[] = "1.4"; > @@ -2542,14 +2546,23 @@ static void infer_hw_params(struct mv643xx_eth_shared_private *msp) > } > } > > +static const struct of_device_id mv643xx_eth_match[] = { > + { .compatible = "marvell,mv64360-eth" }, > + { .compatible = "marvell,mv643xx-eth" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, mv643xx_eth_match); > + > static int mv643xx_eth_shared_probe(struct platform_device *pdev) > { > static int mv643xx_eth_version_printed; > + struct device_node *np = pdev->dev.of_node; > struct mv643xx_eth_shared_platform_data *pd = pdev->dev.platform_data; > struct mv643xx_eth_shared_private *msp; > const struct mbus_dram_target_info *dram; > struct resource *res; > int ret; > + int tx_csum_limit = 0; > > if (!mv643xx_eth_version_printed++) > pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n", This is not related to your change, but there is a problem in this function that has already been discussed in the past if I remember correctly: The respective clock needs to be enabled here (at least on Kirkwood), since accesses to the hardware are done below. Enabling the clock only in mv643xx_eth_probe() is too late. As said, this is not a problem introduced by your changes (and which is currently circumvented by enabling the respective clocks in kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might want to fix this now to get rid of unconditionally enabling the GE clocks in the DT case. > @@ -2576,13 +2589,23 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev) > if (dram) > mv643xx_eth_conf_mbus_windows(msp, dram); > > - msp->tx_csum_limit = (pd != NULL && pd->tx_csum_limit) ? > - pd->tx_csum_limit : 9 * 1024; > + if (np) > + of_property_read_u32(np, "tx-csum-limit", &tx_csum_limit); > + else > + tx_csum_limit = pd->tx_csum_limit; > + > + msp->tx_csum_limit = tx_csum_limit ? tx_csum_limit : 9 * 1024; > infer_hw_params(msp); > > platform_set_drvdata(pdev, msp); > + ret = 0; > > - return 0; > +#ifdef CONFIG_OF > + ret = of_platform_bus_probe(np, mv643xx_eth_match, &pdev->dev); > + if (ret) > + goto out_free; > +#endif > + return ret; > > out_free: > kfree(msp); > @@ -2600,12 +2623,22 @@ static int mv643xx_eth_shared_remove(struct platform_device *pdev) > return 0; > } > > +static const struct of_device_id mv643xx_eth_shared_match[] = { > + { .compatible = "marvell,mv64360-eth-group" }, > + { .compatible = "marvell,mv64360-eth-block" }, > + { .compatible = "marvell,mv643xx-eth-group" }, > + { .compatible = "marvell,mv643xx-eth-block" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, mv643xx_eth_shared_match); > + > static struct platform_driver mv643xx_eth_shared_driver = { > .probe = mv643xx_eth_shared_probe, > .remove = mv643xx_eth_shared_remove, > .driver = { > .name = MV643XX_ETH_SHARED_NAME, > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(mv643xx_eth_shared_match), > }, > }; > > @@ -2764,6 +2797,74 @@ static const struct net_device_ops mv643xx_eth_netdev_ops = { > #endif > }; > > +#ifdef CONFIG_OF > +static int mv643xx_eth_of_probe(struct platform_device *pdev) > +{ > + struct mv643xx_eth_platform_data *pd; > + struct device_node *np = pdev->dev.of_node; > + struct device_node *shared = of_get_parent(np); > + struct device_node *phy_node; > + const int *prop; > + const char *mac_addr; > + > + if (!pdev->dev.of_node) > + return 0; > + > + pd = kzalloc(sizeof(*pd), GFP_KERNEL); > + if (!pd) > + return -ENOMEM; > + > + pdev->dev.platform_data = pd; > + > + pd->shared = of_find_device_by_node(shared); > + if (!pd->shared) > + return -ENODEV; > + > + prop = of_get_property(np, "reg", NULL); > + if (!prop) > + return -EINVAL; > + > + pd->port_number = be32_to_cpup(prop); > + > + phy_node = of_parse_phandle(np, "phy", 0); > + if (!phy_node) { > + pd->phy_addr = MV643XX_ETH_PHY_NONE; > + > + of_property_read_u32(np, "speed", &pd->speed); > + of_property_read_u32(np, "duplex", &pd->duplex); > + } else { > + prop = of_get_property(phy_node, "reg", NULL); > + if (prop) > + pd->phy_addr = be32_to_cpup(prop); > + } > + > + mac_addr = of_get_mac_address(np); > + if (mac_addr) > + memcpy(pd->mac_addr, mac_addr, ETH_ALEN); > + > +#define rx_tx_queue_sram_property(_name) \ > + prop = of_get_property(np, __stringify(_name), NULL); \ > + if (prop) \ > + pd->_name = be32_to_cpup(prop); > + > + rx_tx_queue_sram_property(rx_queue_count); > + rx_tx_queue_sram_property(tx_queue_count); > + rx_tx_queue_sram_property(rx_queue_size); > + rx_tx_queue_sram_property(tx_queue_size); > + rx_tx_queue_sram_property(rx_sram_addr); > + rx_tx_queue_sram_property(rx_sram_size); > + rx_tx_queue_sram_property(tx_sram_addr); > + rx_tx_queue_sram_property(rx_sram_size); > + > + return 0; > +} > +#else > +static inline int mv643xx_eth_of_probe(struct platform_device *dev) > +{ > + return 0; > +} > +#endif > + > static int mv643xx_eth_probe(struct platform_device *pdev) > { > struct mv643xx_eth_platform_data *pd; > @@ -2772,7 +2873,12 @@ static int mv643xx_eth_probe(struct platform_device *pdev) > struct resource *res; > int err; > > + err = mv643xx_eth_of_probe(pdev); > + if (err) > + return err; > + > pd = pdev->dev.platform_data; > + > if (pd == NULL) { > dev_err(&pdev->dev, "no mv643xx_eth_platform_data\n"); > return -ENODEV; You don't change the clk initialization here: #if defined(CONFIG_HAVE_CLK) mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0")); if (!IS_ERR(mp->clk)) { clk_prepare_enable(mp->clk); mp->t_clk = clk_get_rate(mp->clk); } #endif Which, if I understand correctly, works in the DT case because you assign "clock-names" to the clocks in the DTS. However, I wonder whether this works for any but the first Ethernet device. In the old platform device setup, the pdev->id was set when initialiazing the platform_device structure in common.c. Where is this done in the DT case? In phy_scan(), the phy is searched like this: snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, "orion-mdio-mii", addr); phydev = phy_connect(mp->dev, phy_id, mv643xx_eth_adjust_link, PHY_INTERFACE_MODE_GMII); But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a platform_device. I could not get this to work if the MDIO device is setup via DT. Am I doing something wrong? Additionally, in phy_scan() there is this: if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) { start = phy_addr_get(mp) & 0x1f; num = 32; } else { ... MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood devices use "MV643XX_ETH_PHY_ADDR(0)". If the module probe is deferred in mv643xx_eth because the MDIO driver is not yet loaded, all 32 PHY addresses are scanned without success. This is not needed and clutters the log. - Simon
Hello Simon, First of all, thanks for getting these patches a try! Le 04/04/13 23:29, Simon Baatz a écrit : > Hi Florian > [snip] >> if (!mv643xx_eth_version_printed++) >> pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n", > > This is not related to your change, but there is a problem in this > function that has already been discussed in the past if I remember > correctly: The respective clock needs to be enabled here (at least > on Kirkwood), since accesses to the hardware are done below. > Enabling the clock only in mv643xx_eth_probe() is too late. > > As said, this is not a problem introduced by your changes (and which > is currently circumvented by enabling the respective clocks in > kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might > want to fix this now to get rid of unconditionally enabling the GE > clocks in the DT case. I think there may have been some confusion between the "ethernet-group" clock and the actual Ethernet port inside the "ethernet-group". The mv643xx_eth driver assumes we have a per-port clock gating scheme, while I think we have a per "ethernet-group" clock gating scheme instead. Like you said, I think this should be addressed separately. [snip] > > You don't change the clk initialization here: > > #if defined(CONFIG_HAVE_CLK) > mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0")); > if (!IS_ERR(mp->clk)) { > clk_prepare_enable(mp->clk); > mp->t_clk = clk_get_rate(mp->clk); > } > #endif > > Which, if I understand correctly, works in the DT case because you > assign "clock-names" to the clocks in the DTS. However, I wonder > whether this works for any but the first Ethernet device. > > In the old platform device setup, the pdev->id was set when > initialiazing the platform_device structure in common.c. Where is > this done in the DT case? Looks like you are right, in the DT case, I assume that we should lookup the clock using NULL instead of "1" or "0" so we match any clock instead of a specific one. [snip] > > > In phy_scan(), the phy is searched like this: > > snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, > "orion-mdio-mii", addr); > > phydev = phy_connect(mp->dev, phy_id, mv643xx_eth_adjust_link, > PHY_INTERFACE_MODE_GMII); > > But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a > platform_device. I could not get this to work if the MDIO device is > setup via DT. Am I doing something wrong? I just missed updating this part of the code to probe for PHYs. The board I tested with uses a "PHY_NONE" configuration. I will add the missing bits for of_phy_connect() to be called here. > > > Additionally, in phy_scan() there is this: > > if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) { > start = phy_addr_get(mp) & 0x1f; > num = 32; > } else { > ... > > MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood > devices use "MV643XX_ETH_PHY_ADDR(0)". If the module probe is > deferred in mv643xx_eth because the MDIO driver is not yet loaded, > all 32 PHY addresses are scanned without success. This is not needed > and clutters the log. Ok, I am not sure how we can circumvent the log cluttering that happens, what would be your suggestion? -- Florian
On Fri, Apr 5, 2013 at 11:56 AM, Florian Fainelli <florian@openwrt.org> wrote: > [snip] Florian, took me a while to try you patches out on Dove but now I fixed all issues. I will comment on all related patches but first I want to comment here. One general note for Dove related patches: You didn't remove the registration of ge platform_device from mach-dove/board-dt.c. That will lead to double registration of mdio and mv643xx_eth/shared, so you'll never be sure if DT or non-DT code is executed. I haven't checked mach-kirkwood/board-dt.c or orion5x code. >>> if (!mv643xx_eth_version_printed++) >>> pr_notice("MV-643xx 10/100/1000 ethernet driver version >>> %s\n", >> >> >> This is not related to your change, but there is a problem in this >> function that has already been discussed in the past if I remember >> correctly: The respective clock needs to be enabled here (at least >> on Kirkwood), since accesses to the hardware are done below. >> Enabling the clock only in mv643xx_eth_probe() is too late. >> >> As said, this is not a problem introduced by your changes (and which >> is currently circumvented by enabling the respective clocks in >> kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might >> want to fix this now to get rid of unconditionally enabling the GE >> clocks in the DT case. > > > I think there may have been some confusion between the "ethernet-group" > clock and the actual Ethernet port inside the "ethernet-group". The > mv643xx_eth driver assumes we have a per-port clock gating scheme, while I > think we have a per "ethernet-group" clock gating scheme instead. Like you > said, I think this should be addressed separately. IMHO, there should be a clocks property where ever you try to access registers, i.e. in all three "parts" mv643xx_eth_shared (group), mv643xx_eth (port) and mdio. Since port depends on shared it would be ok to have it per group but that may collide with other SoCs than Dove/Kirkwood that have per port clocks. Is that separation (group/port) really required for any SoC? > > [snip] >> >> You don't change the clk initialization here: >> >> #if defined(CONFIG_HAVE_CLK) >> mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0")); >> if (!IS_ERR(mp->clk)) { >> clk_prepare_enable(mp->clk); >> mp->t_clk = clk_get_rate(mp->clk); >> } >> #endif >> >> Which, if I understand correctly, works in the DT case because you >> assign "clock-names" to the clocks in the DTS. However, I wonder >> whether this works for any but the first Ethernet device. Yes, it does. Assigned clocks from clocks property get a clock alias for that device name (node name). Using anything else than NULL here is IMHO just wrong. We should rather provide proper clock aliases for non-DT case. >> In the old platform device setup, the pdev->id was set when >> initialiazing the platform_device structure in common.c. Where is >> this done in the DT case? > > Looks like you are right, in the DT case, I assume that we should lookup the > clock using NULL instead of "1" or "0" so we match any clock instead of a > specific one. Yes. > [snip] >> >> >> In phy_scan(), the phy is searched like this: >> >> snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, >> "orion-mdio-mii", addr); >> >> phydev = phy_connect(mp->dev, phy_id, >> mv643xx_eth_adjust_link, >> PHY_INTERFACE_MODE_GMII); >> >> But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a >> platform_device. I could not get this to work if the MDIO device is >> setup via DT. Am I doing something wrong? > > I just missed updating this part of the code to probe for PHYs. The board I > tested with uses a "PHY_NONE" configuration. I will add the missing bits for > of_phy_connect() to be called here. I don't think that the ethernet controller should probe the PHY's on mdio-bus at all. At least not for DT enabled platforms. I had a look at DT and non-DT mdio-bus sources, and realized that there is a bus scan for non-DT only. of_mdiobus_register requires you to set (and know) the PHY address. I prepared a patch for of_mdio_register that will allow you to probe mdio and assign phy addresses to each node found. Currently, the heuristic for probing is: assign each phy node the next probed phy_addr starting with 0. But that will not allow to e.g. set some PHY addresses and probe the rest. We had a similar discussion whether to probe or not for DT nodes, and I guess there also will be some discussion about the above patch. OTOH we could just (again) ask users of every kirkwood/orion5x/dove board to tell their phy addresses and fail to probe the phy for new boards... I will prepare a proper patch soon and post it on the corresponding lists. >> Additionally, in phy_scan() there is this: >> >> if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) { >> start = phy_addr_get(mp) & 0x1f; >> num = 32; >> } else { >> ... >> >> MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood >> devices use "MV643XX_ETH_PHY_ADDR(0)". If the module probe is >> deferred in mv643xx_eth because the MDIO driver is not yet loaded, >> all 32 PHY addresses are scanned without success. This is not needed >> and clutters the log. > > > Ok, I am not sure how we can circumvent the log cluttering that happens, > what would be your suggestion? My suggestion is to change MV643XX_ETH_PHY_ADDR_DEFAULT from a valid phy address (0) to something invalid (32). I understand that using 0 helps if you don't want to set it in mv643xx's platform_data but it is always difficult to rely on that if 0 is a valid number. Changing the above to 32 should just work because most (all?) boards using phy_scan should also already use MV643XX_ETH_PHY_ADDR_DEFAULT. I also suggest to rename current define to a better name, e.g. MV643XX_ETH_PHY_ADDR_AUTOSCAN. Sebastian
Hello Sebastian, Le 04/05/13 15:58, Sebastian Hesselbarth a écrit : > On Fri, Apr 5, 2013 at 11:56 AM, Florian Fainelli <florian@openwrt.org> wrote: >> [snip] > > Florian, > > took me a while to try you patches out on Dove but now I fixed all > issues. I will > comment on all related patches but first I want to comment here. > > One general note for Dove related patches: You didn't remove the registration of > ge platform_device from mach-dove/board-dt.c. That will lead to double > registration > of mdio and mv643xx_eth/shared, so you'll never be sure if DT or non-DT code is > executed. I haven't checked mach-kirkwood/board-dt.c or orion5x code. This was intentional, this patchset is just preparatory in the sense that it does no conversion of the existing users of the mv643xx_eth platform driver over DT (have some patches to that though). I wanted to resume the discussion on these bindings first, then proceed with the conversion. > >>>> if (!mv643xx_eth_version_printed++) >>>> pr_notice("MV-643xx 10/100/1000 ethernet driver version >>>> %s\n", >>> >>> >>> This is not related to your change, but there is a problem in this >>> function that has already been discussed in the past if I remember >>> correctly: The respective clock needs to be enabled here (at least >>> on Kirkwood), since accesses to the hardware are done below. >>> Enabling the clock only in mv643xx_eth_probe() is too late. >>> >>> As said, this is not a problem introduced by your changes (and which >>> is currently circumvented by enabling the respective clocks in >>> kirkwood_legacy_clk_init() and kirkwood_ge0x_init()), but we might >>> want to fix this now to get rid of unconditionally enabling the GE >>> clocks in the DT case. >> >> >> I think there may have been some confusion between the "ethernet-group" >> clock and the actual Ethernet port inside the "ethernet-group". The >> mv643xx_eth driver assumes we have a per-port clock gating scheme, while I >> think we have a per "ethernet-group" clock gating scheme instead. Like you >> said, I think this should be addressed separately. > > IMHO, there should be a clocks property where ever you try to access registers, > i.e. in all three "parts" mv643xx_eth_shared (group), mv643xx_eth > (port) and mdio. > Since port depends on shared it would be ok to have it per group but that may > collide with other SoCs than Dove/Kirkwood that have per port clocks. Ok, which means that we should also teach mv643xx_eth_shared_probe() about it, as well as the orion-mdio driver. I don't have any particular objections since it should just make things safer with respect to clocking. > > Is that separation (group/port) really required for any SoC? Probably not, it was not clear when I looked at mv78xx0 if it uses two ports per group or 4 groups and 1 port. Anyway, since we are re-using the existing Device Tree binding definition and that the hardware present itself as ethernet groups and ports, I don't see any problem with keeping that difference since it allows for fine-grained representation of the hardware. > >> >> [snip] >>> >>> You don't change the clk initialization here: >>> >>> #if defined(CONFIG_HAVE_CLK) >>> mp->clk = clk_get(&pdev->dev, (pdev->id ? "1" : "0")); >>> if (!IS_ERR(mp->clk)) { >>> clk_prepare_enable(mp->clk); >>> mp->t_clk = clk_get_rate(mp->clk); >>> } >>> #endif >>> >>> Which, if I understand correctly, works in the DT case because you >>> assign "clock-names" to the clocks in the DTS. However, I wonder >>> whether this works for any but the first Ethernet device. > > Yes, it does. Assigned clocks from clocks property get a clock alias for > that device name (node name). Using anything else than NULL here is > IMHO just wrong. We should rather provide proper clock aliases for non-DT case. > >>> In the old platform device setup, the pdev->id was set when >>> initialiazing the platform_device structure in common.c. Where is >>> this done in the DT case? >> >> Looks like you are right, in the DT case, I assume that we should lookup the >> clock using NULL instead of "1" or "0" so we match any clock instead of a >> specific one. > > Yes. > >> [snip] >>> >>> >>> In phy_scan(), the phy is searched like this: >>> >>> snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT, >>> "orion-mdio-mii", addr); >>> >>> phydev = phy_connect(mp->dev, phy_id, >>> mv643xx_eth_adjust_link, >>> PHY_INTERFACE_MODE_GMII); >>> >>> But "orion-mdio-mii:xx" is the name of the PHY if MDIO is setup via a >>> platform_device. I could not get this to work if the MDIO device is >>> setup via DT. Am I doing something wrong? >> >> I just missed updating this part of the code to probe for PHYs. The board I >> tested with uses a "PHY_NONE" configuration. I will add the missing bits for >> of_phy_connect() to be called here. > > I don't think that the ethernet controller should probe the PHY's on mdio-bus > at all. At least not for DT enabled platforms. I had a look at DT and non-DT > mdio-bus sources, and realized that there is a bus scan for non-DT only. > of_mdiobus_register requires you to set (and know) the PHY address. One reason the Ethernet controller could do the probing is in the case we need to apply quirks (e.g: using phydev->flags) for instance. This can be done even after the MDIO bus driver did probe PHY devices though. > > I prepared a patch for of_mdio_register that will allow you to probe mdio and > assign phy addresses to each node found. Currently, the heuristic for probing > is: assign each phy node the next probed phy_addr starting with 0. But that > will not allow to e.g. set some PHY addresses and probe the rest. Ok, we just need to make sure that this does not break any specific use case, I don't think it does, since it seems to be more accurate or equivalent to Ethernet driver doing the probing. > > We had a similar discussion whether to probe or not for DT nodes, and I guess > there also will be some discussion about the above patch. OTOH we could just > (again) ask users of every kirkwood/orion5x/dove board to tell their > phy addresses > and fail to probe the phy for new boards... > > I will prepare a proper patch soon and post it on the corresponding lists. Cool, thanks! > >>> Additionally, in phy_scan() there is this: >>> >>> if (phy_addr == MV643XX_ETH_PHY_ADDR_DEFAULT) { >>> start = phy_addr_get(mp) & 0x1f; >>> num = 32; >>> } else { >>> ... >>> >>> MV643XX_ETH_PHY_ADDR_DEFAULT is defined as 0. However, many Kirkwood >>> devices use "MV643XX_ETH_PHY_ADDR(0)". If the module probe is >>> deferred in mv643xx_eth because the MDIO driver is not yet loaded, >>> all 32 PHY addresses are scanned without success. This is not needed >>> and clutters the log. >> >> >> Ok, I am not sure how we can circumvent the log cluttering that happens, >> what would be your suggestion? > > My suggestion is to change MV643XX_ETH_PHY_ADDR_DEFAULT from a valid > phy address (0) > to something invalid (32). I understand that using 0 helps if you > don't want to set it in mv643xx's platform_data > but it is always difficult to rely on that if 0 is a valid number. > > Changing the above to 32 should just work because most (all?) boards > using phy_scan should also > already use MV643XX_ETH_PHY_ADDR_DEFAULT. I also suggest to rename > current define to a > better name, e.g. MV643XX_ETH_PHY_ADDR_AUTOSCAN. Sounds good to me. -- Florian
On Fri, Apr 05, 2013 at 03:58:03PM +0200, Sebastian Hesselbarth wrote: > I don't think that the ethernet controller should probe the PHY's on mdio-bus > at all. At least not for DT enabled platforms. I had a look at DT and non-DT > mdio-bus sources, and realized that there is a bus scan for non-DT only. > of_mdiobus_register requires you to set (and know) the PHY address. DT platforms should have the option to use the standard phy-phandle connection: mdio@72004 { #address-cells = <1>; #size-cells = <0>; compatible = "marvell,orion-mdio"; reg = <0x72004 0x84>; status = "disabled"; + PHY1: ethernet-phy@1 { + reg = <1>; + device_type = "ethernet-phy"; + }; }; ethernet-group@72000 { #address-cells = <1>; #size-cells = <0>; compatible = "marvell,mv643xx-eth-block"; reg = <0x72000 0x4000>; tx-csum-limit = <1600>; status = "disabled"; egiga0: ethernet@0 { device_type = "network"; compatible = "marvell,mv643xx-eth"; reg = <0>; interrupts = <29>; clocks = <&gate_clk 2>; + phy-handle = <&PHY1>; }; }; When phy-handle is present the ethernet driver should not probe/scan for phys. There is standard code to handle all of this - an important gain is that the phy driver now has access to a DT node and can apply phy-specific properties. > We had a similar discussion whether to probe or not for DT nodes, > and I guess there also will be some discussion about the above > patch. OTOH we could just (again) ask users of every > kirkwood/orion5x/dove board to tell their phy addresses and fail to > probe the phy for new boards... Maybe print a warning and call the no-DT phy probe code if phy-handle is nor present? Not sure this should be in the common code, phy probing is sketchy, it shouldn't be encouraged, IMHO.. Jason
On 04/05/2013 08:04 PM, Jason Gunthorpe wrote: > On Fri, Apr 05, 2013 at 03:58:03PM +0200, Sebastian Hesselbarth wrote: >> I don't think that the ethernet controller should probe the PHY's on mdio-bus >> at all. At least not for DT enabled platforms. I had a look at DT and non-DT >> mdio-bus sources, and realized that there is a bus scan for non-DT only. >> of_mdiobus_register requires you to set (and know) the PHY address. > > DT platforms should have the option to use the standard phy-phandle > connection: > > > mdio@72004 { > #address-cells =<1>; > #size-cells =<0>; > compatible = "marvell,orion-mdio"; > reg =<0x72004 0x84>; > status = "disabled"; > > + PHY1: ethernet-phy@1 { > + reg =<1>; > + device_type = "ethernet-phy"; > + }; > }; > > ethernet-group@72000 { > #address-cells =<1>; > #size-cells =<0>; > compatible = "marvell,mv643xx-eth-block"; > reg =<0x72000 0x4000>; > tx-csum-limit =<1600>; > status = "disabled"; > > egiga0: ethernet@0 { > device_type = "network"; > compatible = "marvell,mv643xx-eth"; > reg =<0>; > interrupts =<29>; > clocks =<&gate_clk 2>; > + phy-handle =<&PHY1>; > }; > }; > > When phy-handle is present the ethernet driver should not probe/scan for > phys. > > There is standard code to handle all of this - an important gain is > that the phy driver now has access to a DT node and can apply > phy-specific properties. The above is what I use as a modification of Florian's patches. I compared of_mdiobus_register against mdiobus_register. The difference is that DT version does not scan the bus for attached PHYs. That is ok, if you know the phy_address of the PHY that you pass the handle of. But in most cases there will be only one PHY on the mdiobus and especially for new boards probing will help here. >> We had a similar discussion whether to probe or not for DT nodes, >> and I guess there also will be some discussion about the above >> patch. OTOH we could just (again) ask users of every >> kirkwood/orion5x/dove board to tell their phy addresses and fail to >> probe the phy for new boards... > > Maybe print a warning and call the no-DT phy probe code if phy-handle > is nor present? I think it would be best to spam each probing result during mdiobus scan to encourage people to edit the DT and put in the phy_addr directly. IMHO it will be best to have bus scan on mdiobus rather than ethernet driver. > Not sure this should be in the common code, phy probing is sketchy, it > shouldn't be encouraged, IMHO.. Actually, it is in common code (non-DT case) and I think passing the phy_addr by DT node like above is best. But for boards you don't know the address (yet) probing helps a lot. If all phy nodes have their reg property set, no probing will be performed. For testing mdio bus probe, I used mdio { ... ethphy: ethernet-phy { reg = <32>; }; }; And 32 should be defined as OF_PHY_ADDR_AUTOSCAN. It is an invalid address as max phy_addr is 31. I also thought about a bool property but passing an invalid reg property in SoC file allows to overwrite it in board file with the actual address easily. And AFAIK bool properties cannot be removed later on by board specific nodes. Sebastian
diff --git a/Documentation/devicetree/bindings/marvell.txt b/Documentation/devicetree/bindings/marvell.txt index f1533d9..e70a013 100644 --- a/Documentation/devicetree/bindings/marvell.txt +++ b/Documentation/devicetree/bindings/marvell.txt @@ -112,9 +112,14 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. Required properties: - #address-cells : <1> - #size-cells : <0> - - compatible : "marvell,mv64360-eth-block" + - compatible : "marvell,mv64360-eth-block", "marvell,mv64360-eth-group", + "marvell,mv643xx-eth-block" - reg : Offset and length of the register set for this block + Optional properties: + - tx-csum-limit : Hardware limit above which transmit checksumming + is disabled. + Example Discovery Ethernet block node: ethernet-block@2000 { #address-cells = <1>; @@ -130,7 +135,7 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. Required properties: - device_type : Should be "network". - - compatible : Should be "marvell,mv64360-eth". + - compatible : Should be "marvell,mv64360-eth", "marvell,mv643xx-eth". - reg : Should be <0>, <1>, or <2>, according to which registers within the silicon block the device uses. - interrupts : <a> where a is the interrupt number for the port. @@ -140,6 +145,22 @@ prefixed with the string "marvell,", for Marvell Technology Group Ltd. controller. - local-mac-address : 6 bytes, MAC address + Optional properties: + - clocks : Phandle to the clock control device and gate bit + - clock-names : String describing the clock gate bit + - speed : Speed to force the link (10, 100, 1000), used when no + phy property is defined + - duplex : Duplex to force the link (0: half, 1: full), used when no + phy property is defined + - rx-queue-count : number of RX queues to use + - tx-queue-count : number of TX queues to use + - rx-queue-size : size of the RX queue (in bytes) + - tx-queue-size : size of the TX queue (in bytes) + - rx-sram-addr : address of the SRAM for RX path (non 0 means used) + - rx-sram-size : size of the SRAM for RX path (non 0 means used) + - tx-sram-addr : address of the SRAM for TX path (non 0 means used) + - tx-sram-size : size of the SRAM for TX path (non 0 means used) + Example Discovery Ethernet port node: ethernet@0 { device_type = "network"; diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c index aedbd82..75599a8 100644 --- a/drivers/net/ethernet/marvell/mv643xx_eth.c +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c @@ -60,6 +60,10 @@ #include <linux/inet_lro.h> #include <linux/slab.h> #include <linux/clk.h> +#include <linux/stringify.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/of_net.h> static char mv643xx_eth_driver_name[] = "mv643xx_eth"; static char mv643xx_eth_driver_version[] = "1.4"; @@ -2542,14 +2546,23 @@ static void infer_hw_params(struct mv643xx_eth_shared_private *msp) } } +static const struct of_device_id mv643xx_eth_match[] = { + { .compatible = "marvell,mv64360-eth" }, + { .compatible = "marvell,mv643xx-eth" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, mv643xx_eth_match); + static int mv643xx_eth_shared_probe(struct platform_device *pdev) { static int mv643xx_eth_version_printed; + struct device_node *np = pdev->dev.of_node; struct mv643xx_eth_shared_platform_data *pd = pdev->dev.platform_data; struct mv643xx_eth_shared_private *msp; const struct mbus_dram_target_info *dram; struct resource *res; int ret; + int tx_csum_limit = 0; if (!mv643xx_eth_version_printed++) pr_notice("MV-643xx 10/100/1000 ethernet driver version %s\n", @@ -2576,13 +2589,23 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev) if (dram) mv643xx_eth_conf_mbus_windows(msp, dram); - msp->tx_csum_limit = (pd != NULL && pd->tx_csum_limit) ? - pd->tx_csum_limit : 9 * 1024; + if (np) + of_property_read_u32(np, "tx-csum-limit", &tx_csum_limit); + else + tx_csum_limit = pd->tx_csum_limit; + + msp->tx_csum_limit = tx_csum_limit ? tx_csum_limit : 9 * 1024; infer_hw_params(msp); platform_set_drvdata(pdev, msp); + ret = 0; - return 0; +#ifdef CONFIG_OF + ret = of_platform_bus_probe(np, mv643xx_eth_match, &pdev->dev); + if (ret) + goto out_free; +#endif + return ret; out_free: kfree(msp); @@ -2600,12 +2623,22 @@ static int mv643xx_eth_shared_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id mv643xx_eth_shared_match[] = { + { .compatible = "marvell,mv64360-eth-group" }, + { .compatible = "marvell,mv64360-eth-block" }, + { .compatible = "marvell,mv643xx-eth-group" }, + { .compatible = "marvell,mv643xx-eth-block" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, mv643xx_eth_shared_match); + static struct platform_driver mv643xx_eth_shared_driver = { .probe = mv643xx_eth_shared_probe, .remove = mv643xx_eth_shared_remove, .driver = { .name = MV643XX_ETH_SHARED_NAME, .owner = THIS_MODULE, + .of_match_table = of_match_ptr(mv643xx_eth_shared_match), }, }; @@ -2764,6 +2797,74 @@ static const struct net_device_ops mv643xx_eth_netdev_ops = { #endif }; +#ifdef CONFIG_OF +static int mv643xx_eth_of_probe(struct platform_device *pdev) +{ + struct mv643xx_eth_platform_data *pd; + struct device_node *np = pdev->dev.of_node; + struct device_node *shared = of_get_parent(np); + struct device_node *phy_node; + const int *prop; + const char *mac_addr; + + if (!pdev->dev.of_node) + return 0; + + pd = kzalloc(sizeof(*pd), GFP_KERNEL); + if (!pd) + return -ENOMEM; + + pdev->dev.platform_data = pd; + + pd->shared = of_find_device_by_node(shared); + if (!pd->shared) + return -ENODEV; + + prop = of_get_property(np, "reg", NULL); + if (!prop) + return -EINVAL; + + pd->port_number = be32_to_cpup(prop); + + phy_node = of_parse_phandle(np, "phy", 0); + if (!phy_node) { + pd->phy_addr = MV643XX_ETH_PHY_NONE; + + of_property_read_u32(np, "speed", &pd->speed); + of_property_read_u32(np, "duplex", &pd->duplex); + } else { + prop = of_get_property(phy_node, "reg", NULL); + if (prop) + pd->phy_addr = be32_to_cpup(prop); + } + + mac_addr = of_get_mac_address(np); + if (mac_addr) + memcpy(pd->mac_addr, mac_addr, ETH_ALEN); + +#define rx_tx_queue_sram_property(_name) \ + prop = of_get_property(np, __stringify(_name), NULL); \ + if (prop) \ + pd->_name = be32_to_cpup(prop); + + rx_tx_queue_sram_property(rx_queue_count); + rx_tx_queue_sram_property(tx_queue_count); + rx_tx_queue_sram_property(rx_queue_size); + rx_tx_queue_sram_property(tx_queue_size); + rx_tx_queue_sram_property(rx_sram_addr); + rx_tx_queue_sram_property(rx_sram_size); + rx_tx_queue_sram_property(tx_sram_addr); + rx_tx_queue_sram_property(rx_sram_size); + + return 0; +} +#else +static inline int mv643xx_eth_of_probe(struct platform_device *dev) +{ + return 0; +} +#endif + static int mv643xx_eth_probe(struct platform_device *pdev) { struct mv643xx_eth_platform_data *pd; @@ -2772,7 +2873,12 @@ static int mv643xx_eth_probe(struct platform_device *pdev) struct resource *res; int err; + err = mv643xx_eth_of_probe(pdev); + if (err) + return err; + pd = pdev->dev.platform_data; + if (pd == NULL) { dev_err(&pdev->dev, "no mv643xx_eth_platform_data\n"); return -ENODEV; @@ -2896,6 +3002,8 @@ out: } #endif free_netdev(dev); + if (pdev->dev.of_node) + kfree(pd); return err; } @@ -2903,6 +3011,7 @@ out: static int mv643xx_eth_remove(struct platform_device *pdev) { struct mv643xx_eth_private *mp = platform_get_drvdata(pdev); + struct mv643xx_eth_platform_data *pd = pdev->dev.platform_data; unregister_netdev(mp->dev); if (mp->phy != NULL) @@ -2918,6 +3027,9 @@ static int mv643xx_eth_remove(struct platform_device *pdev) free_netdev(mp->dev); + if (pdev->dev.of_node) + kfree(pd); + platform_set_drvdata(pdev, NULL); return 0; @@ -2935,6 +3047,7 @@ static void mv643xx_eth_shutdown(struct platform_device *pdev) port_reset(mp); } + static struct platform_driver mv643xx_eth_driver = { .probe = mv643xx_eth_probe, .remove = mv643xx_eth_remove, @@ -2942,6 +3055,7 @@ static struct platform_driver mv643xx_eth_driver = { .driver = { .name = MV643XX_ETH_NAME, .owner = THIS_MODULE, + .of_match_table = of_match_ptr(mv643xx_eth_match), }, };
This patch adds Device Tree bindings following the already defined bindings at Documentation/devicetree/bindings/marvell.txt. The binding documentation is also enhanced with new optionnal properties required for supporting certain devices (RX/TX queue and SRAM). Since we now have proper support for the orion MDIO bus driver, there is no need to fiddle around with device tree phandles. PHY-less (MAC connected to switch) configurations are supported by not specifying any phy phandle for an ethernet node. Signed-off-by: Florian Fainelli <florian@openwrt.org> --- - properly ifdef of_platform_bus_probe with CONFIG_OF - handle of_platform_bus_probe errors and cleanup accordingly - use of_property_read_u32 where applicable - parse "duplex" and "speed" property in PHY-less configuration Documentation/devicetree/bindings/marvell.txt | 25 +++++- drivers/net/ethernet/marvell/mv643xx_eth.c | 120 ++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 5 deletions(-)