Patchwork [2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs

login
register
mail settings
Submitter Sebastian Hesselbarth
Date May 22, 2013, 8:04 p.m.
Message ID <1369253042-15082-2-git-send-email-sebastian.hesselbarth@gmail.com>
Download mbox | patch
Permalink /patch/245707/
State Not Applicable
Headers show

Comments

Sebastian Hesselbarth - May 22, 2013, 8:04 p.m.
Ethernet controllers found on Kirkwood SoCs not only suffer from loosing
MAC address register contents on clock gating but also some important
registers are reset to values that would break ethernet. This patch
clears the CLK125_BYPASS_EN bit for DT enabled Kirkwood only by using
of_machine_is_compatible() instead of #ifdefs. Non-DT Kirkwood is not
affected as it installs a clock gating workaround because of the MAC
address issue above. Other Orion SoCs do not suffer from register reset,
do not have the bit in question, or do not have the register at all.
Moreover, system controllers on PPC using this driver should also be
protected from clearing that bit.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Note: In contrast to the reset value of 0 for CLK125_BYPASS_EN bit as
stated in Kirkwood datasheet, we confirmed that reset value is 1 instead.
Either datasheet is wrong about it, there is some post-boot self-check or
BootROM flips that bit.

Cc: David Miller <davem@davemloft.net>
Cc: Lennert Buytenhek <buytenh@wantstofly.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
Jason Gunthorpe - May 22, 2013, 8:16 p.m.
On Wed, May 22, 2013 at 10:04:02PM +0200, Sebastian Hesselbarth wrote:

> Ethernet controllers found on Kirkwood SoCs not only suffer from loosing
> MAC address register contents on clock gating but also some important
> registers are reset to values that would break ethernet. This patch

FWIW, we found that the bootloader has to write to PSC1, the driver
doesn't work with the power on/reset value of the register. So I think
it is safe to assume that all kirkwood bootloaders alter the value.

Our systems write the value 0x00638488 to PSC1.

I looked at patching mv643xx_eth, but ran into the same complexity you
did, it isn't clear what variants of this IP block have the
register/etc.

> +	/* Kirkwood resets some registers on gated clocks. Especially
> +	 * CLK125_BYPASS_EN must be cleared but is not available on
> +	 * all other SoCs/System Controllers using this driver.
> +	 */
> +	if (of_machine_is_compatible("marvell,kirkwood"))
> +		wrlp(mp, PORT_SERIAL_CONTROL1,
> +		     rdlp(mp, PORT_SERIAL_CONTROL1) & ~CLK125_BYPASS_EN);

of_machine_is_compatible seems heavy handed, I would expect this to be
based on the compatible string of the ethernet node itself, not the
machine??

Jason
Sebastian Hesselbarth - May 22, 2013, 9:02 p.m.
On 05/22/2013 10:16 PM, Jason Gunthorpe wrote:
> On Wed, May 22, 2013 at 10:04:02PM +0200, Sebastian Hesselbarth wrote:
>> Ethernet controllers found on Kirkwood SoCs not only suffer from loosing
>> MAC address register contents on clock gating but also some important
>> registers are reset to values that would break ethernet. This patch
>
> FWIW, we found that the bootloader has to write to PSC1, the driver
> doesn't work with the power on/reset value of the register. So I think
> it is safe to assume that all kirkwood bootloaders alter the value.

It is safe to assume the bootloader alters it, but that modification is
lost when clocks get gated. I assume on clock ungate, the controller is
reset. Saying this, I will double check Dove's reset value but looks 
like reset mess has been fixed in that later SoC.

> Our systems write the value 0x00638488 to PSC1.
>
> I looked at patching mv643xx_eth, but ran into the same complexity you
> did, it isn't clear what variants of this IP block have the
> register/etc.

For Orion SoCs it is quite clear to me, with Gregory Clement and Thomas
Petazzoni we could also confirm if it does any harm there if we
unconditionally clear it. But for PPC system controllers I have no
idea...

>> +	/* Kirkwood resets some registers on gated clocks. Especially
>> +	 * CLK125_BYPASS_EN must be cleared but is not available on
>> +	 * all other SoCs/System Controllers using this driver.
>> +	 */
>> +	if (of_machine_is_compatible("marvell,kirkwood"))
>> +		wrlp(mp, PORT_SERIAL_CONTROL1,
>> +		     rdlp(mp, PORT_SERIAL_CONTROL1)&  ~CLK125_BYPASS_EN);
>
> of_machine_is_compatible seems heavy handed, I would expect this to be
> based on the compatible string of the ethernet node itself, not the
> machine??

I have no strong opinion about checking the machine compatible or have
an extra compatible string for Kirkwood ethernet. Both would work fine
and are checked once upon probe anyway.

Sebastian
Jason - May 23, 2013, 4:01 p.m.
Sebastian,

On Wed, May 22, 2013 at 02:16:07PM -0600, Jason Gunthorpe wrote:
> On Wed, May 22, 2013 at 10:04:02PM +0200, Sebastian Hesselbarth wrote:
> 
> > Ethernet controllers found on Kirkwood SoCs not only suffer from loosing
> > MAC address register contents on clock gating but also some important
> > registers are reset to values that would break ethernet. This patch
> 
> FWIW, we found that the bootloader has to write to PSC1, the driver
> doesn't work with the power on/reset value of the register. So I think
> it is safe to assume that all kirkwood bootloaders alter the value.
> 
> Our systems write the value 0x00638488 to PSC1.
> 
> I looked at patching mv643xx_eth, but ran into the same complexity you
> did, it isn't clear what variants of this IP block have the
> register/etc.
> 
> > +	/* Kirkwood resets some registers on gated clocks. Especially
> > +	 * CLK125_BYPASS_EN must be cleared but is not available on
> > +	 * all other SoCs/System Controllers using this driver.
> > +	 */
> > +	if (of_machine_is_compatible("marvell,kirkwood"))
> > +		wrlp(mp, PORT_SERIAL_CONTROL1,
> > +		     rdlp(mp, PORT_SERIAL_CONTROL1) & ~CLK125_BYPASS_EN);
> 
> of_machine_is_compatible seems heavy handed, I would expect this to be
> based on the compatible string of the ethernet node itself, not the
> machine??

Is there a model number variation between IP that needs this and IP that
doesn't?  If not, I'm fine with of_machine_is_compatible().

thx,

Jason.
Jason Gunthorpe - May 23, 2013, 5:11 p.m.
On Thu, May 23, 2013 at 12:01:11PM -0400, Jason Cooper wrote:
> > > +	/* Kirkwood resets some registers on gated clocks. Especially
> > > +	 * CLK125_BYPASS_EN must be cleared but is not available on
> > > +	 * all other SoCs/System Controllers using this driver.
> > > +	 */
> > > +	if (of_machine_is_compatible("marvell,kirkwood"))
> > > +		wrlp(mp, PORT_SERIAL_CONTROL1,
> > > +		     rdlp(mp, PORT_SERIAL_CONTROL1) & ~CLK125_BYPASS_EN);
> > 
> > of_machine_is_compatible seems heavy handed, I would expect this to be
> > based on the compatible string of the ethernet node itself, not the
> > machine??
> 
> Is there a model number variation between IP that needs this and IP that
> doesn't?  If not, I'm fine with of_machine_is_compatible().

Well the name 'mv643xx' is a family of system controller SOC's
from ages ago, it seems reasonble to continue the trend and label the
IP variations with the SOC name:

 compatible = "marvell,kirwood,ethernet", "marvell,mv643xx_eth"

Jason
Jason - May 23, 2013, 5:23 p.m.
On Thu, May 23, 2013 at 11:11:12AM -0600, Jason Gunthorpe wrote:
> On Thu, May 23, 2013 at 12:01:11PM -0400, Jason Cooper wrote:
> > > > +	/* Kirkwood resets some registers on gated clocks. Especially
> > > > +	 * CLK125_BYPASS_EN must be cleared but is not available on
> > > > +	 * all other SoCs/System Controllers using this driver.
> > > > +	 */
> > > > +	if (of_machine_is_compatible("marvell,kirkwood"))
> > > > +		wrlp(mp, PORT_SERIAL_CONTROL1,
> > > > +		     rdlp(mp, PORT_SERIAL_CONTROL1) & ~CLK125_BYPASS_EN);
> > > 
> > > of_machine_is_compatible seems heavy handed, I would expect this to be
> > > based on the compatible string of the ethernet node itself, not the
> > > machine??
> > 
> > Is there a model number variation between IP that needs this and IP that
> > doesn't?  If not, I'm fine with of_machine_is_compatible().
> 
> Well the name 'mv643xx' is a family of system controller SOC's
> from ages ago, it seems reasonble to continue the trend and label the
> IP variations with the SOC name:
> 
>  compatible = "marvell,kirwood,ethernet", "marvell,mv643xx_eth"

Shouldn't it rather be

	compatible = "marvell,kirkwood-eth", "marvell,orion-eth";

I'm inclined to go with of_machine_is_compatible() since the only
concrete difference we know is that the tweak is needed on kirkwood and
nowhere else.

If we had an errata, or a datasheet saying specifically flavor X needs
this and none other does, then we could trigger on the ethernet node
compatible string or a boolean in the node.  But we don't have that...

thx,

Jason.
Jason Gunthorpe - May 23, 2013, 5:53 p.m.
On Thu, May 23, 2013 at 01:23:39PM -0400, Jason Cooper wrote:

> Shouldn't it rather be
> 
> 	compatible = "marvell,kirkwood-eth", "marvell,orion-eth";

Not sure about orion-eth?
 
> I'm inclined to go with of_machine_is_compatible() since the only
> concrete difference we know is that the tweak is needed on kirkwood and
> nowhere else.

But there is a larger problem here then just this one bit.

The PSC1 register must be set properly for the board layout, and today
we rely on the bootloader to set it. In fact, even with Sebastian's
change the ethernet port won't work without bootloader
intervention. The PortReset bit should also be cleared by the driver
(and it is only present on some variants of this IP block,
apparently).

We know that some Marvell SOC's wack the ethernet registers when they
clock gate, and the flip of Clk125Bypass is another symptom of this
general problem.

So, long term, the PSC1 must be fully set by the driver, based on DT
information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy
type), and the layout of this register seems to vary on a SOC by SOC
basis.

Thus, I think it is appropriate to call this variant of the eth IP
'marvell,kirkwood-eth' which indicates that the register block follows
the kirkwood manual and the PSC1 register specifically has the
kirkwood layout.

The question is what other Marvell SOCs have the same PSC1 layout as
kirkwood?

Jason
Jason - May 23, 2013, 6:40 p.m.
On Thu, May 23, 2013 at 11:53:57AM -0600, Jason Gunthorpe wrote:
> On Thu, May 23, 2013 at 01:23:39PM -0400, Jason Cooper wrote:
> 
> > Shouldn't it rather be
> > 
> > 	compatible = "marvell,kirkwood-eth", "marvell,orion-eth";
> 
> Not sure about orion-eth?
>  
> > I'm inclined to go with of_machine_is_compatible() since the only
> > concrete difference we know is that the tweak is needed on kirkwood and
> > nowhere else.
> 
> But there is a larger problem here then just this one bit.
> 
> The PSC1 register must be set properly for the board layout, and today
> we rely on the bootloader to set it. In fact, even with Sebastian's
> change the ethernet port won't work without bootloader
> intervention. The PortReset bit should also be cleared by the driver
> (and it is only present on some variants of this IP block,
> apparently).
> 
> We know that some Marvell SOC's wack the ethernet registers when they
> clock gate, and the flip of Clk125Bypass is another symptom of this
> general problem.
> 
> So, long term, the PSC1 must be fully set by the driver, based on DT
> information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy
> type), and the layout of this register seems to vary on a SOC by SOC
> basis.
> 
> Thus, I think it is appropriate to call this variant of the eth IP
> 'marvell,kirkwood-eth' which indicates that the register block follows
> the kirkwood manual and the PSC1 register specifically has the
> kirkwood layout.

Ok, so mv643xx_eth would match both "marvell,orion-eth" and
"marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching
"marvell,kirkwood-eth".  I'm not too keen on that, however, the matching
of the machine doesn't look to good, either.

Perhaps a better answer is to add a boolean, "marvell,kirkwood_psc1" and
check for that?

Or, marvell,psc1_reset = <0xWWXXYYZZ>;

> The question is what other Marvell SOCs have the same PSC1 layout as
> kirkwood?

I think marvell,psc1_reset = <>; gives us the most flexibility in
accurately describing the hardware.

thx,

Jason.
Jason Gunthorpe - May 23, 2013, 7:01 p.m.
On Thu, May 23, 2013 at 02:40:28PM -0400, Jason Cooper wrote:

> > But there is a larger problem here then just this one bit.
> > 
> > The PSC1 register must be set properly for the board layout, and today
> > we rely on the bootloader to set it. In fact, even with Sebastian's
> > change the ethernet port won't work without bootloader
> > intervention. The PortReset bit should also be cleared by the driver
> > (and it is only present on some variants of this IP block,
> > apparently).
> > 
> > We know that some Marvell SOC's wack the ethernet registers when they
> > clock gate, and the flip of Clk125Bypass is another symptom of this
> > general problem.
> > 
> > So, long term, the PSC1 must be fully set by the driver, based on DT
> > information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy
> > type), and the layout of this register seems to vary on a SOC by SOC
> > basis.
> > 
> > Thus, I think it is appropriate to call this variant of the eth IP
> > 'marvell,kirkwood-eth' which indicates that the register block follows
> > the kirkwood manual and the PSC1 register specifically has the
> > kirkwood layout.
> 
> Ok, so mv643xx_eth would match both "marvell,orion-eth" and
> "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching
> "marvell,kirkwood-eth".  I'm not too keen on that, however, the matching
> of the machine doesn't look to good, either.

Why are you not keen on this? It seems like normal device driver
practice, that is what the data field of of_device_id is typically
used for..

There are more compatible strings than just kirkwood and orion in this
driver, the whole TX_BW_CONTROL_OLD_LAYOUT/TX_BW_CONTROL_NEW_LAYOUT
buisness (affecting PPC/MIPS) should also someday be captured with
compatible strings rather than auto-detection too..

> > The question is what other Marvell SOCs have the same PSC1 layout as
> > kirkwood?
> 
> I think marvell,psc1_reset = <>; gives us the most flexibility in
> accurately describing the hardware.

Agree, providing psc1_reset value is a good idea to setup the phy
modes. If all 'orion' SOCs have the PSC1 value then we don't need the
kirkwood differentiators, especially if things like the reset bit are
in the same place.

The same trick Sebastian used to capture the mac address could be used
to capture the PSC1 value from the bootloader.

Basically, I think any IP variants that have idential register layouts
can share a compatible string, otherwise different layouts need
different compatible strings, so the general format:

 compatible = "marvell,SOCNAME-eth", "marvell,<something>-eth";

Seems very sane to me. At least this way if we discover more changes
then the driver can match on the SOCNAME compatible string to find
them.

<someting> = orion for TX_BW_CONTROL_NEW_LAYOUT variants also seems
reasonable..

No idea what to call TX_BW_CONTROL_OLD_LAYOUT variants, or the PPC
variants, not important right now it seems.

(BTW, I wonder if the driver should ideally toggle PSC1 reset at some
point????)

Jason
Sebastian Hesselbarth - May 23, 2013, 10:40 p.m.
On 05/23/2013 08:40 PM, Jason Cooper wrote:
> On Thu, May 23, 2013 at 11:53:57AM -0600, Jason Gunthorpe wrote:
>> On Thu, May 23, 2013 at 01:23:39PM -0400, Jason Cooper wrote:
>>> Shouldn't it rather be
>>>
>>> 	compatible = "marvell,kirkwood-eth", "marvell,orion-eth";
>>
>> Not sure about orion-eth?

Jason, Jason,

sorry I didn't came back to this conversation earlier. I already
reworked the patch to rely on 
of_device_is_compatible(.."marvell,kirkwood-eth"..). This is a
kirkwood only thing as other Orions cannot do clock gating or
retain critcal register content (Dove). I will stick with orion-eth
for all other and maybe introduce new compatible strings (and new
fixes) as soon as issues surface.

>>> I'm inclined to go with of_machine_is_compatible() since the only
>>> concrete difference we know is that the tweak is needed on kirkwood and
>>> nowhere else.
>>
>> But there is a larger problem here then just this one bit.
>>
>> The PSC1 register must be set properly for the board layout, and today
>> we rely on the bootloader to set it. In fact, even with Sebastian's
>> change the ethernet port won't work without bootloader
>> intervention. The PortReset bit should also be cleared by the driver
>> (and it is only present on some variants of this IP block,
>> apparently).

Actually, fixing modular scenarios is only for the sake of multiarch
someday. I don't see the point in running current kernel without eth
compiled in _on a NAS SoC_ ;)

On Dockstar which I tested, clearing CLK125_BYPASS_EN to make it work
after clock gating, it might be a coincidence that bootloader's PSC1
setup matches reset value here - so please test the patch on other
Kirkwood boards also.

But, as long as no other issue arise, I will not start to modifiy
mv643xx_eth out of the blue. I has been working for ages and breaking
PPC is not my intention. There are other things David Miller already
requested to get fixed and honestly I even thought about a fresh start
for it. Maybe I'll come back to it when barebox gets it's driver
someday.

>> We know that some Marvell SOC's wack the ethernet registers when they
>> clock gate, and the flip of Clk125Bypass is another symptom of this
>> general problem.

Which SoCs except Kirkwood? I cannot reproduce any of this behavior on
Dove - and from what I can see from the FS of Orion5x or MV78x00 there
are no clock gating registers.

>> So, long term, the PSC1 must be fully set by the driver, based on DT
>> information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy
>> type), and the layout of this register seems to vary on a SOC by SOC
>> basis.

Agree, but I tend to not go at it now. mv643xx_eth has never set up that
registers and actually it never connects anything else than GMII phy (or 
no phy at all). The latter is easy but the for the other, I like to
give up that brain-dead multi-device driver and stick with one device
for both shared and up to three ports. From what I can see from e.g.
ixgbe or any other multi-port eth drivers they all attach the network
device to a single (pci) device.

>> Thus, I think it is appropriate to call this variant of the eth IP
>> 'marvell,kirkwood-eth' which indicates that the register block follows
>> the kirkwood manual and the PSC1 register specifically has the
>> kirkwood layout.
>
> Ok, so mv643xx_eth would match both "marvell,orion-eth" and
> "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching
> "marvell,kirkwood-eth".  I'm not too keen on that, however, the matching
> of the machine doesn't look to good, either.

I didn't choose "marvell,mv643xx-eth" for two reasons
(a) The DT layout is slightly different with phy-handle instead of phy
and marvell prefixed properties. Choosing a compatible string that
matches any PPC compatible string will cause driver racing with sysdev
code to set up platform_data.

(b) I chose to name the controller "orion-eth" and the port
"orion-eth-port" .. PPC has "mv64360-eth" for the port and some 
"mv64360-eth-block" or "-group" for the controller. IMHO not intuitive,
but it just a name anyway.

> Perhaps a better answer is to add a boolean, "marvell,kirkwood_psc1" and
> check for that?
>
> Or, marvell,psc1_reset =<0xWWXXYYZZ>;

For the _long_ run: Exploit either already present phy properties for
MII and friends or introduce new marvell prefixed .. but not misuse DT
for register values here. Each SoC should setup mv643xx_eth properly,
but that should be based on a clean approach _and_ enough people willing
to test that.

I just have a Dockstar and Topkick which is running 24/7. I didn't even
check if only 6281 suffers from it or also 6282 or maybe only some
revisions of 6281. This patch is a fix, nothing more nothing
less. If you have Kirkwoods around, please test if it suffers from
loosing the MAC address and if it works after insmod with the fix
installed.

>> The question is what other Marvell SOCs have the same PSC1 layout as
>> kirkwood?
>
> I think marvell,psc1_reset =<>; gives us the most flexibility in
> accurately describing the hardware.

IMHO using that is just another workaround for a broken driver. We
could hack the whole register setup in DT as it would still accurately
describe HW. Don't get me wrong, but I don't like it.

Haven't checked how happy Linus Walleij is about pinctrl drivers with
reg values hacked in lately.

Sebastian
Linus Walleij - May 24, 2013, 11:03 a.m.
On Fri, May 24, 2013 at 12:40 AM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 05/23/2013 08:40 PM, Jason Cooper wrote:

>> I think marvell,psc1_reset =<>; gives us the most flexibility in
>> accurately describing the hardware.
>
>
> IMHO using that is just another workaround for a broken driver. We
> could hack the whole register setup in DT as it would still accurately
> describe HW. Don't get me wrong, but I don't like it.
>
> Haven't checked how happy Linus Walleij is about pinctrl drivers with
> reg values hacked in lately.

One of the things I've been ranting about lately is that Linux
subsystem maintainers have become de-facto device tree
standard commite chairs. :-(

So to the actual question:

In general I think we need to draw a line and define what we
mean with "describing the hardware" in a device tree.

We have some consensus:
- <reg> properties to describe regsiter BASE offset in physical
  memory and size.
- Resources like IRQ, DMA channel, regulator, GPIO pin control
  handles, are passed using <&ampersand> notation.

And so it goes on.

When it comes to defining different registers and their individual
bits and the meaning of these and/or default values, I personally
think that is making things harder for developers rather than
simplifying things. I know that pinctrl-single is anyway doing this
and I was talked into accepting it under circumstances where
developers are being passed opaque machine-generated
data that would otherwise be translated into unreadable header
files littering the kernel.

For a coder it is definately better if the *driver* know these
details, but whether that is possible seems to depend on things
like hardware development process.

IMO: if you want to go down that road, what you really want is not
ever more expressible device trees, but real open firmware,
or ACPI or UEFI that can interpret and run bytecode as some
"bios" for you. With DT coming from OF maybe this is a natural
progression of things, but one has to realize when we reach the
point where what we really want is a bios. Then your time is
likely better spent with Tianocore or something than with the
kernel.

Yours,
Linus Walleij
Jason - May 24, 2013, 4:46 p.m.
On Thu, May 23, 2013 at 01:01:40PM -0600, Jason Gunthorpe wrote:
> On Thu, May 23, 2013 at 02:40:28PM -0400, Jason Cooper wrote:
> 
> > > But there is a larger problem here then just this one bit.
> > > 
> > > The PSC1 register must be set properly for the board layout, and today
> > > we rely on the bootloader to set it. In fact, even with Sebastian's
> > > change the ethernet port won't work without bootloader
> > > intervention. The PortReset bit should also be cleared by the driver
> > > (and it is only present on some variants of this IP block,
> > > apparently).
> > > 
> > > We know that some Marvell SOC's wack the ethernet registers when they
> > > clock gate, and the flip of Clk125Bypass is another symptom of this
> > > general problem.
> > > 
> > > So, long term, the PSC1 must be fully set by the driver, based on DT
> > > information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy
> > > type), and the layout of this register seems to vary on a SOC by SOC
> > > basis.
> > > 
> > > Thus, I think it is appropriate to call this variant of the eth IP
> > > 'marvell,kirkwood-eth' which indicates that the register block follows
> > > the kirkwood manual and the PSC1 register specifically has the
> > > kirkwood layout.
> > 
> > Ok, so mv643xx_eth would match both "marvell,orion-eth" and
> > "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching
> > "marvell,kirkwood-eth".  I'm not too keen on that, however, the matching
> > of the machine doesn't look to good, either.
> 
> Why are you not keen on this? It seems like normal device driver
> practice, that is what the data field of of_device_id is typically
> used for..

I'm not keen on it because we don't have a document saying "All kirkwood
SoCs need PSC1 set to X after reset."  We know it, but have we tested
the 6282?

That being said, if "marvell,kirkwood-eth" is the best we can do for
now, I'm all for it.  I would just like to be reasonably certain that
the binding we are creating doesn't lock us into a difficult decision
later.

> There are more compatible strings than just kirkwood and orion in this
> driver, the whole TX_BW_CONTROL_OLD_LAYOUT/TX_BW_CONTROL_NEW_LAYOUT
> buisness (affecting PPC/MIPS) should also someday be captured with
> compatible strings rather than auto-detection too..

Agreed.

> > > The question is what other Marvell SOCs have the same PSC1 layout as
> > > kirkwood?
> > 
> > I think marvell,psc1_reset = <>; gives us the most flexibility in
> > accurately describing the hardware.
> 
> Agree, providing psc1_reset value is a good idea to setup the phy
> modes. If all 'orion' SOCs have the PSC1 value then we don't need the
> kirkwood differentiators, especially if things like the reset bit are
> in the same place.
> 
> The same trick Sebastian used to capture the mac address could be used
> to capture the PSC1 value from the bootloader.
> 
> Basically, I think any IP variants that have idential register layouts
> can share a compatible string, otherwise different layouts need
> different compatible strings, so the general format:
> 
>  compatible = "marvell,SOCNAME-eth", "marvell,<something>-eth";
> 
> Seems very sane to me. At least this way if we discover more changes
> then the driver can match on the SOCNAME compatible string to find
> them.

After glancing a LinusW's email, I'm thinking this isn't the correct
path.  I'll write more in my response to him.

thx,

Jason.
Andrew Lunn - May 24, 2013, 4:53 p.m.
> > Why are you not keen on this? It seems like normal device driver
> > practice, that is what the data field of of_device_id is typically
> > used for..
> 
> I'm not keen on it because we don't have a document saying "All kirkwood
> SoCs need PSC1 set to X after reset."  We know it, but have we tested
> the 6282?

6282 looses its MAC address, that much i know. I've no idea about
PSC1, but if its MAC address behaviour is the same as 6281, is expect
PSC1 is the same.

	Andrew
Jason - May 24, 2013, 4:53 p.m.
On Fri, May 24, 2013 at 12:40:26AM +0200, Sebastian Hesselbarth wrote:
> On 05/23/2013 08:40 PM, Jason Cooper wrote:
> >On Thu, May 23, 2013 at 11:53:57AM -0600, Jason Gunthorpe wrote:
> >>On Thu, May 23, 2013 at 01:23:39PM -0400, Jason Cooper wrote:
> >>>Shouldn't it rather be
> >>>
> >>>	compatible = "marvell,kirkwood-eth", "marvell,orion-eth";
> >>
> >>Not sure about orion-eth?

Sorry, yep, one or the other.

> Jason, Jason,

For a second, I read this as "tsk tsk tsk..." ;-)

> sorry I didn't came back to this conversation earlier. I already
> reworked the patch to rely on
> of_device_is_compatible(.."marvell,kirkwood-eth"..). This is a
> kirkwood only thing as other Orions cannot do clock gating or
> retain critcal register content (Dove). I will stick with orion-eth
> for all other and maybe introduce new compatible strings (and new
> fixes) as soon as issues surface.

Okay, as I mentioned to Jason, I would like to test 6282 before we
settle on this path.  Other than that, I'm fine with it.

> >>>I'm inclined to go with of_machine_is_compatible() since the only
> >>>concrete difference we know is that the tweak is needed on kirkwood and
> >>>nowhere else.
> >>
> >>But there is a larger problem here then just this one bit.
> >>
> >>The PSC1 register must be set properly for the board layout, and today
> >>we rely on the bootloader to set it. In fact, even with Sebastian's
> >>change the ethernet port won't work without bootloader
> >>intervention. The PortReset bit should also be cleared by the driver
> >>(and it is only present on some variants of this IP block,
> >>apparently).
> 
> Actually, fixing modular scenarios is only for the sake of multiarch
> someday. I don't see the point in running current kernel without eth
> compiled in _on a NAS SoC_ ;)

Good point, but if the eth can be gated to save power, we shouldn't
limit the user's ability just because the SoC is primarily in NAS's.

thx,

Jason.
Jason - May 24, 2013, 5:01 p.m.
On Fri, May 24, 2013 at 01:03:25PM +0200, Linus Walleij wrote:
> On Fri, May 24, 2013 at 12:40 AM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com> wrote:
> > On 05/23/2013 08:40 PM, Jason Cooper wrote:
> 
> >> I think marvell,psc1_reset =<>; gives us the most flexibility in
> >> accurately describing the hardware.
> >
> >
> > IMHO using that is just another workaround for a broken driver. We
> > could hack the whole register setup in DT as it would still accurately
> > describe HW. Don't get me wrong, but I don't like it.
> >
> > Haven't checked how happy Linus Walleij is about pinctrl drivers with
> > reg values hacked in lately.
> 
> One of the things I've been ranting about lately is that Linux
> subsystem maintainers have become de-facto device tree
> standard commite chairs. :-(

This is the first I've heard this rant.  :(

To that end, I agree with you.  My frustration boiled down to trying to
predict the future, which is futile. :-P

For our scenario, once we can confirm our least popular kirkwood
variant, the 6282, behaves the same as we've seen so far, then
"marvell,kirkwood-eth" is fine by me.

> So to the actual question:
> 
> In general I think we need to draw a line and define what we
> mean with "describing the hardware" in a device tree.
> 
> We have some consensus:
> - <reg> properties to describe regsiter BASE offset in physical
>   memory and size.
> - Resources like IRQ, DMA channel, regulator, GPIO pin control
>   handles, are passed using <&ampersand> notation.
> 
> And so it goes on.
> 
> When it comes to defining different registers and their individual
> bits and the meaning of these and/or default values, I personally
> think that is making things harder for developers rather than
> simplifying things. I know that pinctrl-single is anyway doing this
> and I was talked into accepting it under circumstances where
> developers are being passed opaque machine-generated
> data that would otherwise be translated into unreadable header
> files littering the kernel.
> 
> For a coder it is definately better if the *driver* know these
> details, but whether that is possible seems to depend on things
> like hardware development process.

Agree.

> IMO: if you want to go down that road, what you really want is not
> ever more expressible device trees, but real open firmware,
> or ACPI or UEFI that can interpret and run bytecode as some
> "bios" for you. With DT coming from OF maybe this is a natural
> progression of things, but one has to realize when we reach the
> point where what we really want is a bios. Then your time is
> likely better spent with Tianocore or something than with the
> kernel.

shudder.  I like embedded systems because the *don't* have a bios.

thx,

Jason.
Jason - May 24, 2013, 5:03 p.m.
On Fri, May 24, 2013 at 06:53:15PM +0200, Andrew Lunn wrote:
> > > Why are you not keen on this? It seems like normal device driver
> > > practice, that is what the data field of of_device_id is typically
> > > used for..
> > 
> > I'm not keen on it because we don't have a document saying "All kirkwood
> > SoCs need PSC1 set to X after reset."  We know it, but have we tested
> > the 6282?
> 
> 6282 looses its MAC address, that much i know. I've no idea about
> PSC1, but if its MAC address behaviour is the same as 6281, is expect
> PSC1 is the same.

Do you have a board set up for testing you could try Sebastian's
forthcoming series on (with "marvell,kirkwood-eth")?

thx,

Jason.
Russell King - ARM Linux - May 24, 2013, 5:13 p.m.
On Fri, May 24, 2013 at 01:01:25PM -0400, Jason Cooper wrote:
> On Fri, May 24, 2013 at 01:03:25PM +0200, Linus Walleij wrote:
> > IMO: if you want to go down that road, what you really want is not
> > ever more expressible device trees, but real open firmware,
> > or ACPI or UEFI that can interpret and run bytecode as some
> > "bios" for you. With DT coming from OF maybe this is a natural
> > progression of things, but one has to realize when we reach the
> > point where what we really want is a bios. Then your time is
> > likely better spent with Tianocore or something than with the
> > kernel.
> 
> shudder.  I like embedded systems because the *don't* have a bios.

Then you're into scenarios like I have with my laptop, where - those
of you who check the nightly build results will have noticed - one
of my serial ports doesn't always exist.  That's because the ACPI data
in the BIOS is *wrong*.  It reports that it has been enabled when it
hasn't, and the disassembled byte code is at fault here.

The fix?  God knows.  As far as I'm concerned as a user, or even as an
OS developer, it's unfixable without getting the ACPI data structures
changed, and that's not something I can do.

Do you really want that on ARM?  Given the fiasco with the location of
the registers, are you sure you want to place more trust in that
direction?  Does it give you a warm fuzzy feeling to know that you
might have to work out some way to patch vendor supplied bytecode?
Sebastian Hesselbarth - May 24, 2013, 5:25 p.m.
On 05/24/2013 07:13 PM, Russell King - ARM Linux wrote:
> Do you really want that on ARM?  Given the fiasco with the location of
> the registers, are you sure you want to place more trust in that
> direction?  Does it give you a warm fuzzy feeling to know that you
> might have to work out some way to patch vendor supplied bytecode?

Don't get me wrong. I want mv643xx_eth DT or even platform_data to
evolve to a fully self configured driver not depending on proper u-boot
setup at all.

But I don't want to go that road now and I wonder if it might be safer
for us (and PPC guys) if we start mv643xx_eth over from scratch one day.

For this patch set, I want a basic DT binding that works. Patching the
driver to play with kirkwood loosing the MAC and other important
registers is not my main concern here. If clearing that one bit doesn't
help for all kirkwood boards, I'd rather leave the non-gating
workaround.

mv643xx_eth not knowing DT for ARM is stalling last important bits for
Orion SoCs. I want this to go in first as with David another maintainer
is involved.

Sebastian
Jason Gunthorpe - May 24, 2013, 5:33 p.m.
On Fri, May 24, 2013 at 12:46:36PM -0400, Jason Cooper wrote:

> > Why are you not keen on this? It seems like normal device driver
> > practice, that is what the data field of of_device_id is typically
> > used for..
> 
> I'm not keen on it because we don't have a document saying "All kirkwood
> SoCs need PSC1 set to X after reset."  We know it, but have we tested
> the 6282?

I disagree. The manul is very clear how PSC1 must be set for proper
operation. Clk125BypassEn bit is used only for loopback testing, it
should never set for driver operation. Similarly PortReset must be
cleared for driver operation.

It is always safe for the driver to clear these bits, if it knows they
are available.  In fact, I would argue the driver should always clear
these bits so that the bootloader isn't relied on to do it. It doesn't
matter if the SOC errantly sets the bit or not, it can *always* be
safely cleared.

Further, I compared my 88F6282/88F6283 manual against the public
88F6180/88F619x/88F6281 spec and confirmed that the PSC1 layout is the
same.

So all of these SOC's can share a driver compatible string.

AFAICT the only reason the driver doesn't touch PSC1 today is because
the PSC1 was introduced in a later IP revision and its presence isn't
auto-detectable.

The last bit of the puzzle to get bootloader independence on kirkwood
is to encode the phy interface type (GMII/RGMII/BASE-X) in the DT so
the entire PSC1 can be set by the driver..

Jason
Jason - May 28, 2013, 6:02 p.m.
Jason,

Sorry, I meant to get back to this earlier and it slipped off of my
plate. :(

On Fri, May 24, 2013 at 11:33:06AM -0600, Jason Gunthorpe wrote:
> On Fri, May 24, 2013 at 12:46:36PM -0400, Jason Cooper wrote:
> 
> > > Why are you not keen on this? It seems like normal device driver
> > > practice, that is what the data field of of_device_id is typically
> > > used for..
> > 
> > I'm not keen on it because we don't have a document saying "All kirkwood
> > SoCs need PSC1 set to X after reset."  We know it, but have we tested
> > the 6282?
> 
> I disagree. The manul is very clear how PSC1 must be set for proper
> operation. Clk125BypassEn bit is used only for loopback testing, it
> should never set for driver operation. Similarly PortReset must be
> cleared for driver operation.
> 
> It is always safe for the driver to clear these bits, if it knows they
> are available.  In fact, I would argue the driver should always clear
> these bits so that the bootloader isn't relied on to do it. It doesn't
> matter if the SOC errantly sets the bit or not, it can *always* be
> safely cleared.

Great!

> Further, I compared my 88F6282/88F6283 manual against the public
> 88F6180/88F619x/88F6281 spec and confirmed that the PSC1 layout is the
> same.

Even better.

> So all of these SOC's can share a driver compatible string.

Ok, "marvell,kirkwood-eth" works for me then.  I think Sebastian already
has patches for that.

> AFAICT the only reason the driver doesn't touch PSC1 today is because
> the PSC1 was introduced in a later IP revision and its presence isn't
> auto-detectable.

Makes sense.

> The last bit of the puzzle to get bootloader independence on kirkwood
> is to encode the phy interface type (GMII/RGMII/BASE-X) in the DT so
> the entire PSC1 can be set by the driver..

Hmm, let's work on that separately, and later.  I've snowballed this
attempt enough ;-)

Thanks for digging into this for us,

Jason.

Patch

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index f2c229c..d9ad8c7 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -119,6 +119,8 @@  static char mv643xx_eth_driver_version[] = "1.4";
 #define  LINK_UP			0x00000002
 #define TXQ_COMMAND			0x0048
 #define TXQ_FIX_PRIO_CONF		0x004c
+#define PORT_SERIAL_CONTROL1		0x004c
+#define  CLK125_BYPASS_EN		0x00000010
 #define TX_BW_RATE			0x0050
 #define TX_BW_MTU			0x0058
 #define TX_BW_BURST			0x005c
@@ -2843,6 +2845,14 @@  static int mv643xx_eth_probe(struct platform_device *pdev)
 
 	mp->dev = dev;
 
+	/* Kirkwood resets some registers on gated clocks. Especially
+	 * CLK125_BYPASS_EN must be cleared but is not available on
+	 * all other SoCs/System Controllers using this driver.
+	 */
+	if (of_machine_is_compatible("marvell,kirkwood"))
+		wrlp(mp, PORT_SERIAL_CONTROL1,
+		     rdlp(mp, PORT_SERIAL_CONTROL1) & ~CLK125_BYPASS_EN);
+
 	/*
 	 * Start with a default rate, and if there is a clock, allow
 	 * it to override the default.