| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50239815.6020108@codethink.co.uk>
Date: Thu, 09 Aug 2012 11:59:33 +0100
From: Ian Molton <ian.molton@...ethink.co.uk>
To: Arnd Bergmann <arnd@...db.de>
CC: David Miller <davem@...emloft.net>,
linux-arm-kernel@...ts.infradead.org, andrew@...n.ch,
thomas.petazzoni@...e-electrons.com, ben.dooks@...ethink.co.uk,
netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
linuxppc-dev@...ts.ozlabs.org, dale@...nsworth.org
Subject: Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
Adding devicetree-discuss and linuxppc-dev, as well as Dale Farnsworth,
who initially added the bindings for mv643xx.
On 08/08/12 14:19, Ian Molton wrote:
> On 08/08/12 13:39, Arnd Bergmann wrote:
>> On Wednesday 08 August 2012, Ian Molton wrote:
>>> The SMI / PHY stuff should look very similar, so I'm happy with something
>>> like:
>>>
>>> mdio@...0 {
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> device_type = "mdio";
>>> compatible = "marvell,mv643xx-mdio";
>>> phy0: ethernet-phy@0 {
>>> device_type = "ethernet-phy";
>>> compatible = "marvell,whatever";
>>> interrupts = <76>;
>>> interrupt-parent = <&mpic>;
>>> reg = <0 32>; // Auto probed phy addr
>>> };
>>>
>>> phy1: ethernet-phy@3 {
>>> device_type = "ethernet-phy";
>>> compatible = "marvell,whatever";
>>> interrupts = <77>;
>>> interrupt-parent = <&mpic>;
>>> reg = <3 1>; // specified phy addr
>>> };
>>>
>>> ... and so on.
>>> }
>>>
>>> Where we can use the reg parameter to allow auto-probing, by
>>> specifying a size of 32 (32 phy addrs max).
>> I don't understand the auto-probed phy address. What is the purpose of that?
> Personally, I think it should die - but the existing driver and a number
> of its users actually scan the bus for their PHY.
>
> I doubt the PHY really moves about or is hotplugged by any of them,
> and its actually quite a slow process.
>
>> If possible, I think we should keep using #size-cells=<0>, which would
>> make the method you describe impossible. It might still work if you just
>> leave out the "reg" property for that node.
> I can certainly investigate that. I couldn't see any good evidence that
> it was a supported mechanism when I looked.
>
>> I also don't understand how the phy driver would locate ethernet-phy@0
>> on the bus if it does not know the address.
>>
>>> The ethernet driver itself is more complicated:
>>>
>>> We have the following considerations:
>>>
>>> * we have one MDIO bus, typically, shared between all the MACs / PHYs.
>>> * each ethernet device can multiple ports (up to three), each with its
>>> own MAC/PHY.
>>> * MAC <-> PHY mapping can be specified, probed (ugh!) or a (gah!)
>>> mix of the two.
>>> * existing D-T users, albeit not well documented / code complete.
>>> * some port address ranges overlap (MIB counters, MCAST / UNICAST
>>> tables, etc.
>>>
>>> The existing ethernet-group idea only works because the current
>>> platform-device based driver doesnt really do proper resource
>>> management, and thus the MAC registers are actually mapped by
>>> the MDIO driver.
>>>
>>> I don't think that preserving this bad behaviour is a good idea, which
>>> leaves us with two choices:
>>>
>>> 1) My preferred solution - allow each device to specify up to three
>>> interrupts, MACs, and PHYs. This is clean in that it doesnt require
>>> multiply instantiating a driver three times over the same address
>>> space.
>>>
>>> ethernet@...0 {
>>> compatible = "marvell,mv643xx-eth";
>>> reg = <0x2400 0x1c00>
>>> interrupt_parent = <&mpic>;
>>> ports = <3>;
>>> interrupts = <4>, <5>, <6>;
>>> phys = <&phy0>, <&phy1>, <&phy2>;
>>> };
>>>
>>> ethernet@...0 {
>>> compatible = "marvell,mv643xx-eth";
>>> reg = <0x6400 0x1c00>
>>> interrupt_parent = <&mpic>;
>>> ports = <1>;
>>> interrupts = <4>;
>>> phys = <&phy3>;
>>> };
>>>
>>> Note that the address is 2400, not 2000 - since this driver no longer
>>> would share its address range with the MDIO driver.
>>>
>>> This method would require a small amount of rework in the driver to
>>> set up <n> ports, rather than just one.
>> This looks quite nice, but it is still very much incompatible with the
>> existing binding. Obviously we can abandon an existing binding and
>> introduce a second one for the same hardware, but that should not
>> be taken lightly.
> Fair, however the existing users aren't anywhere near as
> numerous as the new ones.
>
>>> 2) Create some kind of pseudo-ethernet group device that manages
>>> all the work for some sort of lightweight ethernet device, one per
>>> port. This can never be done cleanly since the port address ranges
>>> overlap:
>>>
>>> pseudo_eth@...0 {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> compatible = "marvell,mv643xx-shared-eth"
>>> reg = <0x2400 0x1c00>;
>>>
>>> ethernet@0 {
>>> compatible = "marvell,mv643xx-port";
>>> interrupts = <4>;
>>> interrupt_parent = <&mpic>;
>>> phy = <&phy0>;
>>> };
>>>
>>> ethernet@1 {
>>> compatible = "marvell,mv643xx-port";
>>> interrupts = <5>;
>>> interrupt_parent = <&mpic>;
>>> phy = <&phy1>;
>>> };
>>>
>>> ethernet@2 {
>>> compatible = "marvell,mv643xx-port";
>>> interrupts = <6>;
>>> interrupt_parent = <&mpic>;
>>> phy = <&phy2>;
>>> };
>>> }
>>> pseudo_eth@...0 {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> compatible = "marvell,mv643xx-shared-eth"
>>> reg = <0x6400 0x1c00>;
>>>
>>> ethernet@0 {
>>> compatible = "marvell,mv643xx-port";
>>> interrupts = <4>;
>>> interrupt_parent = <&mpic>;
>>> phy = <&phy3>;
>>> };
>>> };
>> This looks almost compatible with the existing binding, which is
>> good.
> Well, I'm not sure about that - if the existing bindings are really
> baked into firmware, then "almost" wont be any use at all.
>
>> I would in fact recommend to use the actual "compatible"
>> strings from the binding. More generally speaking, you should not
>> use wildcards in those strings anyway, so always use
>> "marvell,mv64360-eth" instead of "marvell,mv64x60-eth" or
>> "marvell,mv643xx-eth". If you have multiple chips that are
>> completely compatible, put use the identifier for the older one.
> Noted.
>
>> I don't fully understand your concern with the overlapping
>> registers, mostly because I still don't know all the combinations
>> that are actually valid here. Let me try to say what I understood
>> so far, and you can correct me if that's wrong:
>>
>> * A system can have multiple instances of an mv64360 ethernet
>> block, with a register area of 0x2000 bytes.
>> * Each such block can have three MACs and three PHYs.
>> * The first 0x400 bytes in the register space control the three
>> PHYs and the remaining registers control the MACs.
>> * While this is meant to be used in a way that you assign
>> the each of the three PHYs to one of the MACs, this is not
>> always done, and sometimes you use a different PHY (?), or
>> one from a different instance of the mv64360 ethernet block
>> on the same SoC?.
> Nearly - the whole block is 0x2000 in size, yes. And each one
> can have 3 MACs and PHYs, as you say.
>
> There is SMI @ 0x2000 - just one for all ports, and in many
> (all?) cases, for all all the other controllers on the SoC to
> share. On the armadaXP SoC, for example, each ethernet
> block has its own alias of the same bas SMI reg. (there are
> 4 blocks)
>
> ethernet0@ 0x2400
> ## regs in order: Main regs, MIB counters, Special mcast table, Mcast
> table, Unicast table.
> port0 has regs at +0x0000 *0x1000 +0x1400 +0x1500 +0x1600
> port1 has regs at +0x0400 *0x1080 +0x1800 +0x1900 +0x1a00
> port2 has regs at +0x0800 *0x1100 +0x1c00 +0x1d00 +0x1e00
> ethernet1@ 0x6400
> port0 has regs at +0x0000 *0x1000 +0x1400 +0x1500 +0x1600
> ...
>
> As you can see, instead of putting port1 at +0x1700 or so,
> marvell have overlapped the register files - in fact, doubly
> so, since port1 + 0x1080 is right in the middle of
> (port0 + 0x1000) -> (port0 + 0x16ff), so one cant simply map two
> sets of regs like 0x0000->0x03ff and 0x1000->0x16ff for port one
> either.
>
> -Ian
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists