Re: [PATCH v3 3/7] mv643xx.c: Add basic device tree support.

Re: [PATCH v3 3/7] mv643xx.c: Add basic device tree support.

[Arnd Bergmann]

On Tuesday 07 August 2012, Ian Molton wrote:
> > I think it documents some of the same thing,
> 
> Not really. It documents some godawful hack that recycled the platform
> device -based driver and provided a DT binding for it, just for PPC.
> 
> I cant even find anything that implements code for whatever
> "marvell,mv64360-mdio" might be. I'm sure it might exist somewhere.

The code dates back to when we had separate buses for platform devices
and of devices, and then it was decided not to add support for both
bus types to each of the marvell drivers. In hindsight it would have
been better to do that, but that was impossible to tell back then.

> > We might also want to move some of the
> > code from arch/powerpc/sysdev/mv64x60_dev.c to live in the same place
> > as the device driver.
> 
> I hope not. I don't really want to touch that stuff at all. If it works
> the way it
> is, then it can stay that way. If the PPC folk want to send patches to
> add the
> properties they use to the driver, then they can do. I'll send an email
> their
> way and see if they want to join in.
> 
> From my perspective, the next thing that needs to happen to the driver is
> for it to be broken up into ethernet and mdio drivers, so that we can
> get rid
> of all this shared_smi craziness... But that's for another patch series.

Adding devicetree-discuss and linuxppc-dev, as well as Dale Farnsworth,
who initially added the bindings for mv643xx.

I would hope that at least some of the properties that are used on
powerpc can be reused in the same way for other architectures.
The method to find the phy address on powerpc does indeed make
more sense than the "port_number" property you suggested, and the
phandle for the phy node is usually called "phy" not "mdio".

I'm not sure if the ethernet-group is required on ARM as well, but
it does sound a lot like what you actually want instead of the
shared_smi property.

	Arnd

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Ian Molton]

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@2000 {
>>>                 #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@2400 {
>>>                 compatible = "marvell,mv643xx-eth";
>>>                 reg = <0x2400 0x1c00>
>>>                 interrupt_parent = <&mpic>;
>>>                 ports = <3>;
>>>                 interrupts = <4>, <5>, <6>;
>>>                 phys = <&phy0>, <&phy1>, <&phy2>;
>>> };
>>>
>>> ethernet@6400 {
>>>                 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@2400 {
>>>         #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@6400 {
>>>         #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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Arnd Bergmann]

 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:
 >>> 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.

Depends on how you count the numbers. I see at least three machines
supported in the kernel with the old binding and none with the new one
so far ;-)

 >> 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.

This could theoretically be dealt with by having 5 register ranges
per device, but that would cause extra overhead and also be
incompatible with the existing binding. I think showing one
parent device with children at address 0, 1 and 2 is ok. The driver
already knows all those offsets and they are always the same
for all variants of mv643xx, right?

	Arnd

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Ian Molton]

On 09/08/12 12:43, Arnd Bergmann wrote:
>  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:
>  >>> 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.
>
> Depends on how you count the numbers. I see at least three machines
> supported in the kernel with the old binding and none with the new one
> so far ;-)

I'm curious as to how any of those actually work, given the
apparent total lack of a mv64360-mdio device binding...
>  > 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.
>
> This could theoretically be dealt with by having 5 register ranges

I make that three...

> per device, but that would cause extra overhead and also be
> incompatible with the existing binding.

Indeed.

>  I think showing one
> parent device with children at address 0, 1 and 2 is ok.
Is it acceptable for the child devices to directly access the
parents register space? because there would be no other
way for that to work.

>  The driver
> already knows all those offsets and they are always the same
> for all variants of mv643xx, right?
Yes, but its not clean. And no amount of refactoring is
really going to make a nice driver that also fits the ancient
(and badly thought out) OF bindings.

If we have to break things, we can at least go for a nice
clean design, surely?

The ports arent really child devices of the MAC. The MAC
just has 3 ports.

Luckily, it looks like the existing users don't actually use
the device tree to set up the driver at all, preferring to
translate their D-T bindings to calls to
platform_device_register() so all we'd need to do to
support them is completely ignore them.

We're going to have to maintain a legacy
platform_device -> DT bindings hack somewhere anyway,
at least until the remaining other users of the driver
convert to D-T.

-Ian

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Arnd Bergmann]

On Thursday 09 August 2012, Ian Molton wrote:
> >  I think showing one
> > parent device with children at address 0, 1 and 2 is ok.
> Is it acceptable for the child devices to directly access the
> parents register space? because there would be no other
> way for that to work.

Yes, I see no problem with that. As long as all the drivers
agree on who can access what.

> >  The driver
> > already knows all those offsets and they are always the same
> > for all variants of mv643xx, right?
> Yes, but its not clean. And no amount of refactoring is
> really going to make a nice driver that also fits the ancient
> (and badly thought out) OF bindings.

In what way is it badly though out, or not clean? The use of
underscores in the properties, and the way that the sram
is configured is problematic, I agree. But The way that
the three ports are addressed and how the PHY is found
seems quite clever.

> If we have to break things, we can at least go for a nice
> clean design, surely?
> 
> The ports arent really child devices of the MAC. The MAC
> just has 3 ports.

I don't see the difference between those two things.

> Luckily, it looks like the existing users don't actually use
> the device tree to set up the driver at all, preferring to
> translate their D-T bindings to calls to
> platform_device_register() so all we'd need to do to
> support them is completely ignore them.
> 
> We're going to have to maintain a legacy
> platform_device -> DT bindings hack somewhere anyway,
> at least until the remaining other users of the driver
> convert to D-T.

I don't understand why you describe the method used in
powerpc as a hack. It was the normal way to introduce
DT support for platform devices back when it was implemented.
It also had the advantage of not requiring any modifications
to the generic driver, because it was shared between one
architecture using DT (powerpc) and one that didn't (ARM).

	Arnd

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Ian Molton]

On 10/08/12 11:49, Arnd Bergmann wrote:
> On Thursday 09 August 2012, Ian Molton wrote:
>>>  The driver
>>> already knows all those offsets and they are always the same
>>> for all variants of mv643xx, right?
>> Yes, but its not clean. And no amount of refactoring is
>> really going to make a nice driver that also fits the ancient
>> (and badly thought out) OF bindings.
> In what way is it badly though out, or not clean? The use of
> underscores in the properties, and the way that the sram
> is configured is problematic, I agree. But The way that
> the three ports are addressed and how the PHY is found
> seems quite clever.

It forces one to load the MDIO driver first, because it maps ALL the
registers for both itself and all the ports, and the MDIO driver has no
way of knowing how many ethernet blocks are present (I have a device
here with two, and another with four). Thats anywhere from 1 to 12
ports, split across 1 to 4 address ranges, and theres a big gap in the
address range between controllers 0,1 and 2,3. *ALL* the devices on the
board are sharing ethernet block 0's MDIO bus. By pure luck it happens
to work, because the blocks 2,3 have an alias of the MDIO registers from
blocks 0,1.

Having the MDIO driver map the ethernet drivers memory is a terrible
solution, IMO. Ethernet drivers should map their own memory, and that
introduces the n-ports-per-block problem, because their address ranges
overlap.

I think the best solution is to make each ethernet block register 3 ports.

the PPC code can simply generate different fixups so that instead of
creating 3 devices, it creates one, with three ports.

>> If we have to break things, we can at least go for a nice
>> clean design, surely?
>>
>> The ports arent really child devices of the MAC. The MAC
>> just has 3 ports.
> I don't see the difference between those two things.

The ports are at best 'pseudodevices'. Real devices have registers of
their own.

>> We're going to have to maintain a legacy
>> platform_device -> DT bindings hack somewhere anyway,
>> at least until the remaining other users of the driver
>> convert to D-T.
> I don't understand why you describe the method used in
> powerpc as a hack. It was the normal way to introduce
> DT support for platform devices back when it was implemented.

Just because its normal doesn't mean its not a hack :)

> It also had the advantage of not requiring any modifications
> to the generic driver, because it was shared between one
> architecture using DT (powerpc) and one that didn't (ARM).

It /did/ spawn a pretty hideous driver, though...

-Ian

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Ian Molton]

Ping :)

Can we get some consensus on the right approach here? I'm loathe to code
this if its going to be rejected.

I'd prefer the driver to be properly split so we dont have the MDIO
driver mapping the ethernet drivers address spaces, but if thats not
going to be merged, I'm not feeling like doing the work for nothing.

If the driver is to use the overlapping-address mapped-by-the-mdio
scheme, then so be it, but I could do with knowing.

Another point against the latter scheme is that the MDIO driver could
sensibly be used (the block is identical) on the ArmadaXP, which has 4
ethernet blocks rather than two, yet grouped in two pairs with a
discontiguous address range.

I'd like to get this moved along as soon as possible though.

-Ian

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Arnd Bergmann]

On Thursday 16 August 2012, Ian Molton wrote:
> Ping :)
> 
> Can we get some consensus on the right approach here? I'm loathe to code
> this if its going to be rejected.
> 
> I'd prefer the driver to be properly split so we dont have the MDIO
> driver mapping the ethernet drivers address spaces, but if thats not
> going to be merged, I'm not feeling like doing the work for nothing.
> 
> If the driver is to use the overlapping-address mapped-by-the-mdio
> scheme, then so be it, but I could do with knowing.
> 
> Another point against the latter scheme is that the MDIO driver could
> sensibly be used (the block is identical) on the ArmadaXP, which has 4
> ethernet blocks rather than two, yet grouped in two pairs with a
> discontiguous address range.
> 
> I'd like to get this moved along as soon as possible though.

Following up on the old discussion, I talked briefly about this
issue with BenH at the kernel summit. The outcome basically is that
it's a bit sad to have incompatible bindings, but it's not the end
of the world,and it's more important to do it right this time.

Just make sure that you use different values for the 'compatible'
strings and then do what you need to get the ARM hardware working.

Ideally, the new binding should be written in a way that powerpc
machines can use the same one, but the existing ones all use
an version of Open Firmware that is not going to get updated
and it's also not too likely that we are going to see new
powerpc machines based on this chip.

	Arnd

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Benjamin Herrenschmidt]

On Mon, 2012-09-10 at 14:22 +0000, Arnd Bergmann wrote:
> Following up on the old discussion, I talked briefly about this
> issue with BenH at the kernel summit. The outcome basically is that
> it's a bit sad to have incompatible bindings, but it's not the end
> of the world,and it's more important to do it right this time.
> 
> Just make sure that you use different values for the 'compatible'
> strings and then do what you need to get the ARM hardware working.
> 
> Ideally, the new binding should be written in a way that powerpc
> machines can use the same one, but the existing ones all use
> an version of Open Firmware that is not going to get updated
> and it's also not too likely that we are going to see new
> powerpc machines based on this chip.

Right, mostly these machines where the Pegasos. Those came with a fairly
busted variant of Open Firmware which generated a pretty gross
device-tree.

For some reason, the manufacturer of those things was never willing to
fix anything in their firmware (despite the distributor providing
patches etc...), seemingly on the assumption that whatever they were
doing was perfect and operating system people like us didn't matter one
little bit :-)

So I don't care much about it. It would be nice to keep them working
since people in the community still have them but if it goes through
some "compat" code that detects old/broken device-trees and eventually
disappears when we finally drop support, then so be it.

Cheers,
Ben.

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Jason Cooper]

Pong.  ;-)

On Tue, Sep 11, 2012 at 04:03:31PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-09-10 at 14:22 +0000, Arnd Bergmann wrote:
> > Following up on the old discussion, I talked briefly about this
> > issue with BenH at the kernel summit. The outcome basically is that
> > it's a bit sad to have incompatible bindings, but it's not the end
> > of the world,and it's more important to do it right this time.
> > 
> > Just make sure that you use different values for the 'compatible'
> > strings and then do what you need to get the ARM hardware working.
> > 
> > Ideally, the new binding should be written in a way that powerpc
> > machines can use the same one, but the existing ones all use
> > an version of Open Firmware that is not going to get updated
> > and it's also not too likely that we are going to see new
> > powerpc machines based on this chip.
> 
> Right, mostly these machines where the Pegasos. Those came with a fairly
> busted variant of Open Firmware which generated a pretty gross
> device-tree.
> 
> For some reason, the manufacturer of those things was never willing to
> fix anything in their firmware (despite the distributor providing
> patches etc...), seemingly on the assumption that whatever they were
> doing was perfect and operating system people like us didn't matter one
> little bit :-)
> 
> So I don't care much about it. It would be nice to keep them working
> since people in the community still have them but if it goes through
> some "compat" code that detects old/broken device-trees and eventually
> disappears when we finally drop support, then so be it.

Ian,

What is the status of this work?

thx,

Jason.

Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.

[Arnd Bergmann]

On Monday 13 August 2012, Ian Molton wrote:
> On 10/08/12 11:49, Arnd Bergmann wrote:
> > On Thursday 09 August 2012, Ian Molton wrote:
> >>>  The driver
> >>> already knows all those offsets and they are always the same
> >>> for all variants of mv643xx, right?
> >> Yes, but its not clean. And no amount of refactoring is
> >> really going to make a nice driver that also fits the ancient
> >> (and badly thought out) OF bindings.
> > In what way is it badly though out, or not clean? The use of
> > underscores in the properties, and the way that the sram
> > is configured is problematic, I agree. But The way that
> > the three ports are addressed and how the PHY is found
> > seems quite clever.
> 
> It forces one to load the MDIO driver first, because it maps ALL the
> registers for both itself and all the ports, and the MDIO driver has no
> way of knowing how many ethernet blocks are present (I have a device
> here with two, and another with four). Thats anywhere from 1 to 12
> ports, split across 1 to 4 address ranges, and theres a big gap in the
> address range between controllers 0,1 and 2,3. *ALL* the devices on the
> board are sharing ethernet block 0's MDIO bus. By pure luck it happens
> to work, because the blocks 2,3 have an alias of the MDIO registers from
> blocks 0,1.
> 
> Having the MDIO driver map the ethernet drivers memory is a terrible
> solution, IMO. Ethernet drivers should map their own memory, and that
> introduces the n-ports-per-block problem, because their address ranges
> overlap.
> 
> I think the best solution is to make each ethernet block register 3 ports.
> 
> the PPC code can simply generate different fixups so that instead of
> creating 3 devices, it creates one, with three ports.

Ok.

> Can we get some consensus on the right approach here? I'm loathe to code
> this if its going to be rejected.
> 
> I'd prefer the driver to be properly split so we dont have the MDIO
> driver mapping the ethernet drivers address spaces, but if thats not
> going to be merged, I'm not feeling like doing the work for nothing.
> 
> If the driver is to use the overlapping-address mapped-by-the-mdio
> scheme, then so be it, but I could do with knowing.
> 
> Another point against the latter scheme is that the MDIO driver could
> sensibly be used (the block is identical) on the ArmadaXP, which has 4
> ethernet blocks rather than two, yet grouped in two pairs with a
> discontiguous address range.
> 
> I'd like to get this moved along as soon as possible though.

I don't object to any device driver changes, but I do want to make
sure that the bindings are sensible and can coexist with the
ones that have been used for the past 5 years.

Maybe you can move the binding for the ethernet parts out of the
marvell.txt file into the place you want to use for the new
bindings and then extend it to cover both the old and the new style.

	Arnd