From: florian@openwrt•org (Florian Fainelli)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/5 v2] mv643xx_eth: add Device Tree bindings
Date: Fri, 05 Apr 2013 16:23:17 +0200 [thread overview]
Message-ID: <515EDE55.9030609@openwrt.org> (raw)
In-Reply-To: <CABJ1b_TdzE_Q9qLsdSGZy8byFpCTtVBUy5FtxiZAx7QqgG-rVg@mail.gmail.com>
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
next prev parent reply other threads:[~2013-04-05 14:23 UTC|newest]
Thread overview: 131+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-04 10:27 [PATCH 0/5 v2] mv643xx_eth: device tree bindings Florian Fainelli
2013-04-04 10:27 ` [PATCH 1/5 v2] mv643xx_eth: add Device Tree bindings Florian Fainelli
2013-04-04 21:29 ` Simon Baatz
2013-04-05 9:56 ` Florian Fainelli
2013-04-05 13:58 ` Sebastian Hesselbarth
2013-04-05 14:23 ` Florian Fainelli [this message]
2013-04-05 18:04 ` Jason Gunthorpe
2013-04-05 20:35 ` Sebastian Hesselbarth
2013-04-04 10:27 ` [PATCH 2/5] mv643xx_eth: update Device Tree bindings documentation Florian Fainelli
2013-04-04 10:27 ` [PATCH 3/5 v2] ARM: kirkwood: add device node entries for the gigabit interfaces Florian Fainelli
2013-04-04 21:35 ` Simon Baatz
2013-04-05 9:21 ` Florian Fainelli
2013-04-04 10:27 ` [PATCH 4/5 v2] ARM: orion5x: add gigabit ethernet device tree node Florian Fainelli
2013-04-04 10:27 ` [PATCH 5/5 v2] ARM: dove: add gigabit device tree nodes to dove.dtsi Florian Fainelli
2013-04-11 16:53 ` [PATCH 0/5 v2] mv643xx_eth: device tree bindings Jason Cooper
2013-04-11 17:09 ` Sebastian Hesselbarth
2013-04-13 15:21 ` Thomas Petazzoni
2013-04-13 19:00 ` Jason Cooper
2013-04-15 3:07 ` Ben Hutchings
2013-04-15 13:43 ` Jason Cooper
2013-04-15 10:00 ` Thomas Petazzoni
2013-05-06 15:33 ` [PATCH 0/7] " Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 1/7] net: mv643xx_eth: add Device Tree bindings Sebastian Hesselbarth
2013-05-20 21:19 ` Simon Baatz
2013-05-20 21:34 ` Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 2/7] net: mv643xx_eth: update Device Tree bindings documentation Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 3/7] ARM: dove: remove legacy mv643xx_eth setup Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 4/7] ARM: dove: add gigabit ethernet and mvmdio device tree nodes Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 5/7] ARM: kirkwood: remove legacy mv643xx_eth board setup Sebastian Hesselbarth
2013-05-06 15:39 ` Thomas Petazzoni
2013-05-06 15:33 ` [PATCH v3 6/7] ARM: kirkwood: add gigabit ethernet and mvmdio device tree nodes Sebastian Hesselbarth
2013-05-20 21:27 ` Simon Baatz
2013-05-20 21:38 ` Sebastian Hesselbarth
2013-05-06 15:33 ` [PATCH v3 7/7] ARM: kirkwood: remove legacy clk alias for mv643xx_eth Sebastian Hesselbarth
2013-05-06 15:38 ` [PATCH 0/7] mv643xx_eth: device tree bindings David Miller
2013-05-06 16:07 ` Jason Cooper
2013-05-06 16:21 ` Sebastian Hesselbarth
2013-05-07 9:10 ` Valentin Longchamp
2013-05-21 16:41 ` [PATCH v4 00/12] net: mv643xx_eth DT support and fixes Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 01/12] net: mv643xx_eth: use phy_disconnect instead of phy_detach Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 02/12] net: mv643xx_eth: use managed devm_ioremap for port registers Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 03/12] net: mv643xx_eth: add phy_node to platform_data struct Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 04/12] net: mv643xx_eth: use of_phy_connect if phy_node present Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 05/12] net: mv643xx_eth: add DT parsing support Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 06/12] ARM: dove: add gigabit ethernet and mvmdio device tree nodes Sebastian Hesselbarth
2013-05-21 17:48 ` Andrew Lunn
2013-05-22 9:43 ` Sebastian Hesselbarth
2013-05-22 10:04 ` tiejun.chen
2013-05-22 10:13 ` Sebastian Hesselbarth
2013-05-22 13:10 ` Jason Cooper
2013-05-22 16:59 ` Jason Gunthorpe
2013-05-22 17:01 ` Jason Cooper
2013-05-22 17:32 ` Sebastian Hesselbarth
2013-05-22 17:35 ` Jason Cooper
2013-05-22 17:42 ` Sebastian Hesselbarth
2013-05-22 17:48 ` Jason Cooper
2013-05-22 18:44 ` Sebastian Hesselbarth
2013-05-22 18:49 ` Jason Cooper
2013-05-22 18:55 ` Sebastian Hesselbarth
2013-05-22 18:58 ` Jason Cooper
2013-05-22 19:52 ` Sebastian Hesselbarth
2013-05-22 18:24 ` Jason Gunthorpe
2013-05-22 18:51 ` Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 07/12] ARM: kirkwood: " Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 08/12] ARM: orion5x: " Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 09/12] ARM: dove: remove legacy mv643xx_eth setup Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 10/12] ARM: kirkwood: remove legacy clk alias for mv643xx_eth Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 11/12] ARM: kirkwood: remove redundant DT board files Sebastian Hesselbarth
2013-05-22 20:36 ` Simon Baatz
2013-05-22 20:55 ` Sebastian Hesselbarth
2013-05-22 21:02 ` Jason Cooper
2013-05-22 21:17 ` Sebastian Hesselbarth
2013-05-21 16:41 ` [PATCH v4 12/12] ARM: orion5x: remove legacy mv643xx_eth board setup Sebastian Hesselbarth
2013-05-22 16:16 ` [PATCH v4 00/12] net: mv643xx_eth DT support and fixes Andrew Lunn
2013-05-22 20:04 ` [PATCH 1/2] ARM: kirkwood: proper retain MAC address workaround on DT ethernet Sebastian Hesselbarth
2013-05-22 20:04 ` [PATCH 2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs Sebastian Hesselbarth
2013-05-22 20:16 ` Jason Gunthorpe
2013-05-22 21:02 ` Sebastian Hesselbarth
2013-05-23 16:01 ` Jason Cooper
2013-05-23 17:11 ` Jason Gunthorpe
2013-05-23 17:23 ` Jason Cooper
2013-05-23 17:53 ` Jason Gunthorpe
2013-05-23 18:40 ` Jason Cooper
2013-05-23 19:01 ` Jason Gunthorpe
2013-05-24 16:46 ` Jason Cooper
2013-05-24 16:53 ` Andrew Lunn
2013-05-24 17:03 ` Jason Cooper
2013-05-24 17:33 ` Jason Gunthorpe
2013-05-28 18:02 ` Jason Cooper
2013-05-23 22:40 ` Sebastian Hesselbarth
2013-05-24 11:03 ` Linus Walleij
2013-05-24 17:01 ` Jason Cooper
2013-05-24 17:13 ` Russell King - ARM Linux
2013-05-24 17:25 ` Sebastian Hesselbarth
2013-05-24 16:53 ` Jason Cooper
2013-05-26 4:04 ` [PATCH 1/2] ARM: kirkwood: proper retain MAC address workaround on DT ethernet David Miller
2013-05-26 20:06 ` Sebastian Hesselbarth
2013-05-27 9:23 ` David Miller
2013-05-27 9:39 ` Benjamin Herrenschmidt
2013-05-27 10:24 ` Sebastian Hesselbarth
2013-05-27 11:50 ` Benjamin Herrenschmidt
2013-05-27 12:47 ` Arnd Bergmann
2013-05-27 21:50 ` Benjamin Herrenschmidt
2013-05-27 22:12 ` Sebastian Hesselbarth
2013-05-27 22:17 ` David Miller
2013-05-27 20:18 ` David Miller
2013-05-27 21:48 ` Benjamin Herrenschmidt
2013-05-27 9:38 ` Benjamin Herrenschmidt
2013-05-29 19:32 ` [PATCH v5 00/13] net: mv643xx_eth DT support and fixes Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 01/13] net: mv643xx_eth: use phy_disconnect instead of phy_detach Sebastian Hesselbarth
2013-05-29 20:00 ` Jason Cooper
2013-05-29 19:32 ` [PATCH v5 02/13] net: mv643xx_eth: use managed devm_ioremap for port registers Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 03/13] net: mv643xx_eth: add phy_node to platform_data struct Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 04/13] net: mv643xx_eth: use of_phy_connect if phy_node present Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 05/13] net: mv643xx_eth: proper initialization for Kirkwood SoCs Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 06/13] net: mv643xx_eth: add DT parsing support Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 07/13] ARM: dove: add gigabit ethernet and mvmdio device tree nodes Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 08/13] ARM: kirkwood: " Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 09/13] ARM: orion5x: " Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 10/13] ARM: dove: remove legacy mv643xx_eth setup Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 11/13] ARM: kirkwood: remove legacy clk alias for mv643xx_eth Sebastian Hesselbarth
2013-05-29 19:32 ` [PATCH v5 12/13] ARM: kirkwood: remove redundant DT board files Sebastian Hesselbarth
2013-05-30 9:06 ` Arnaud Ebalard
2013-05-30 9:08 ` Sebastian Hesselbarth
2013-05-30 19:37 ` Jason Cooper
2013-05-30 22:28 ` Arnaud Ebalard
2013-05-31 11:54 ` Jason Cooper
2013-05-29 19:32 ` [PATCH v5 13/13] ARM: orion5x: remove legacy mv643xx_eth board setup Sebastian Hesselbarth
2013-05-31 0:55 ` [PATCH v5 00/13] net: mv643xx_eth DT support and fixes David Miller
2013-05-31 6:28 ` Sebastian Hesselbarth
2013-05-31 9:32 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=515EDE55.9030609@openwrt.org \
--to=florian@openwrt$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox