| Message ID | 1369253042-15082-2-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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.
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
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.
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
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
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 <&ersand> 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
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.
> > 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
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.
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 <&ersand> 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.
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.
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?
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
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, 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.
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.
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(+)