| Message ID | 1380575010-8573-4-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > These clocks should never be gated, since the ethernet interfaces > forget the assigned MAC address assigned if their clock source is > turned off. Hrm, this hack is only needed to support the hack of passing the MAC address from the bootloader to the kernel via the ethernet registers. If local-mac-address is present in the DT (or fixed up via ATAGS) then none of this is needed. Boards that have well formed DT's don't need this. The major downside to this patch is we never get power savings by disabling the 2nd GE. On my boards it is not connected so we always want to see it powered down. Our bootloader gates the power and turns off the clock, having Linux turn the clock back on without knowing how to control the power seems like a possible problem. I don't have code, but when this was first brought up it was suggested to add local-mac-address to the DT nodes if it isn't present in the common code and don't mess with the clocks. Psudeo code: void marvell_dt_ethernet_fixup(void) { for_each_compatible_node(np, "marvell,kirkwood-eth-port") { if (of_get_mac_address(np) != NULL) continue; regs = ioremap(of_regs(np->parent,0)); address = (readl(regs + MAC_ADDR_HIGH) << 32) | readl(regs + MAC_ADDR_LOW)) iounmap(regs); of_add_property(np, {"local-mac-address" = address}); } } Jason
Hi Jason > Our bootloader gates the power and turns off the clock, having Linux > turn the clock back on without knowing how to control the power seems > like a possible problem. This patch won't do that. This patch will stop it getting turn off. It will never turn it on. The only thing that will turn it on is the Ethernet driver. So if the bootloader turned it off, it should stay off. > I don't have code, but when this was first brought up it was suggested > to add local-mac-address to the DT nodes if it isn't present in the > common code and don't mess with the clocks. > > Psudeo code: > > void marvell_dt_ethernet_fixup(void) > { > for_each_compatible_node(np, "marvell,kirkwood-eth-port") > { > if (of_get_mac_address(np) != NULL) > continue; > regs = ioremap(of_regs(np->parent,0)); > address = (readl(regs + MAC_ADDR_HIGH) << 32) | > readl(regs + MAC_ADDR_LOW)) > iounmap(regs); > of_add_property(np, {"local-mac-address" = address}); > } > } The pseudo code looks nice, but i think in reality, the of_add_property(np, {"local-mac-address" = address}); is not so simple. Sebastian took a look at this. Andrew
On Mon, Sep 30, 2013 at 11:34:37PM +0200, Andrew Lunn wrote: > > Our bootloader gates the power and turns off the clock, having Linux > > turn the clock back on without knowing how to control the power seems > > like a possible problem. > > This patch won't do that. This patch will stop it getting turn off. It > will never turn it on. The only thing that will turn it on is the > Ethernet driver. So if the bootloader turned it off, it should stay > off. Ok, that sounds good. > The pseudo code looks nice, but i think in reality, the > of_add_property(np, {"local-mac-address" = address}); is not so > simple. Sebastian took a look at this. Oh? I though it was just this: struct Tmp { struct property prop; u8 mac[6]; }; struct Tmp *prop = kzalloc(sizeof(struct Tmp)); prop->prop.name = "local-mac-address"; prop->prop.length = sizeof(prop->mac); prop->prop.value = prop->mac; prop->mac = (...); of_add_property(np, &prop->prop); That is basically what other stuff in the kernel does, at least. Jason
On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > These clocks should never be gated, since the ethernet interfaces forget > the assigned MAC address assigned if their clock source is turned off. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > Cc: Mike Turquette <mturquette@linaro.org> > > drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) I'm inclined to leave this as a hack in board-dt.c... At some point we are going to move kirkwood over to mach-mvebu/, and there will be eyes on the code. There's less chance of 're-discovering' this for cleanup if it's in the clock driver, looking all proper and such. Even with the clear comment. thx, Jason.
On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > > These clocks should never be gated, since the ethernet interfaces forget > > the assigned MAC address assigned if their clock source is turned off. > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > Cc: Mike Turquette <mturquette@linaro.org> > > > > drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > I'm inclined to leave this as a hack in board-dt.c... At some point we > are going to move kirkwood over to mach-mvebu/, and there will be eyes > on the code. There's less chance of 're-discovering' this for cleanup > if it's in the clock driver, looking all proper and such. Even with the > clear comment. > Hm.. Really? I honestly think this change is a far cleaner way of dealing with the problem. The motivation for these 4 patches, came from an attempt of booting a multiplatform kernel Kirkwood, which _almost_ booted fine :P So, I see this as preparation work for the move to mach-mvebu. Does anybody have any insights on how a proper fix would be done in the net driver?
On 10/01/2013 03:42 PM, Ezequiel Garcia wrote: > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: >> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: >>> These clocks should never be gated, since the ethernet interfaces forget >>> the assigned MAC address assigned if their clock source is turned off. >>> >>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >>> --- >>> Cc: Mike Turquette <mturquette@linaro.org> >>> >>> drivers/clk/mvebu/kirkwood.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> I'm inclined to leave this as a hack in board-dt.c... At some point we >> are going to move kirkwood over to mach-mvebu/, and there will be eyes >> on the code. There's less chance of 're-discovering' this for cleanup >> if it's in the clock driver, looking all proper and such. Even with the >> clear comment. >> > > Hm.. Really? I honestly think this change is a far cleaner way of > dealing with the problem. Actually I prefer to leave the workaround there, too. > The motivation for these 4 patches, came from an attempt of booting a > multiplatform kernel Kirkwood, which _almost_ booted fine :P But the workaround doesn't affect "_almost_", does it? > So, I see this as preparation work for the move to mach-mvebu. > > Does anybody have any insights on how a proper fix would be done > in the net driver? The proper fix would be anything that is related with providing proper 'local-mac-address' property to the corresponding nodes *and* properly setup some registers in eth ip, e.g. (R)GMII, basically all that is usually done by u-boot. Currently, there is no support for PHY configuration at all but if your board uses reset defaults, IIRC it should work except for the missing MAC address. There is one fix for kirkwood already that enables proper clock. Again, IIRC, I tried to push a fix similar to Jason's but used of_set_property. Jason's looks way more sane and we should give it a try. Sebastian
On Tue, Oct 01, 2013 at 10:42:14AM -0300, Ezequiel Garcia wrote: > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: > > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > > > These clocks should never be gated, since the ethernet interfaces forget > > > the assigned MAC address assigned if their clock source is turned off. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > > --- > > > Cc: Mike Turquette <mturquette@linaro.org> > > > > > > drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > I'm inclined to leave this as a hack in board-dt.c... At some point we > > are going to move kirkwood over to mach-mvebu/, and there will be eyes > > on the code. There's less chance of 're-discovering' this for cleanup > > if it's in the clock driver, looking all proper and such. Even with the > > clear comment. > > > > Hm.. Really? I honestly think this change is a far cleaner way of > dealing with the problem. That depends on your definition of 'problem' ;-) It sounds like right now you consider the problem to be kirkwood going multiplatform. In that case, this is a solution. However, I think the problem is that the IP block loses it's mac address when gated. That hasn't been solved, all you're doing is moving the hack/workaround from one place to another less visible place. > The motivation for these 4 patches, came from an attempt of booting a > multiplatform kernel Kirkwood, which _almost_ booted fine :P > > So, I see this as preparation work for the move to mach-mvebu. Fair enough. > Does anybody have any insights on how a proper fix would be done > in the net driver? A reliable way to make sure the mac address is passed to the kernel from existing bootloaders via DT. I _really_ need to add fdt parsing/modification to the impedance matcher. Then we can safely say "wrap the kernel and dtb with the impedance matcher". /or/, we could save it in some fashion in mv643xx_eth's suspend/resume code. Unfortunately, it has to survive module unload/load, and there's still the issue of loading the module after kernel boot. Clocks would be gated, so the mac address has to be retrieved from somewhere. Since modifying the dt in the kernel is frowned upon, we're left with inserting what the bootloader tells us and handing it off to the kernel at boot time. thx, Jason.
On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote: > On Tue, Oct 01, 2013 at 10:42:14AM -0300, Ezequiel Garcia wrote: > > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: > > > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > > > > These clocks should never be gated, since the ethernet interfaces forget > > > > the assigned MAC address assigned if their clock source is turned off. > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > > > --- > > > > Cc: Mike Turquette <mturquette@linaro.org> > > > > > > > > drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > I'm inclined to leave this as a hack in board-dt.c... At some point we > > > are going to move kirkwood over to mach-mvebu/, and there will be eyes > > > on the code. There's less chance of 're-discovering' this for cleanup > > > if it's in the clock driver, looking all proper and such. Even with the > > > clear comment. > > > > > > > Hm.. Really? I honestly think this change is a far cleaner way of > > dealing with the problem. > > That depends on your definition of 'problem' ;-) It sounds like right > now you consider the problem to be kirkwood going multiplatform. In > that case, this is a solution. > Our 'problem' definition is certainly the same. > However, I think the problem is that the IP block loses it's mac address > when gated. That hasn't been solved, all you're doing is moving the > hack/workaround from one place to another less visible place. > Of course, my opinion doesn't count this time, but I still think the fix is cleaner. There's an already existing and clock-framework-specific way of preventing a clock from being ever gated -which seems to match exactly this case- so I have a hard time seeing why we wouldn't use it. Also: I'm wondering why you think this change would 'hide' the clock gating. It seems whenever a (future?) developer finds these clocks are not gated, one of the first places he will look for is precisely on those flags. Just my two cents, the hack doesn't really worth a fight.
On Tue, Oct 01, 2013 at 03:49:43PM +0200, Sebastian Hesselbarth wrote: > On 10/01/2013 03:42 PM, Ezequiel Garcia wrote: > > On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: > >> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > >>> These clocks should never be gated, since the ethernet interfaces forget > >>> the assigned MAC address assigned if their clock source is turned off. > >>> > >>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > >>> --- > >>> Cc: Mike Turquette <mturquette@linaro.org> > >>> > >>> drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > >>> 1 file changed, 7 insertions(+), 2 deletions(-) [..] > > The motivation for these 4 patches, came from an attempt of booting a > > multiplatform kernel Kirkwood, which _almost_ booted fine :P > > But the workaround doesn't affect "_almost_", does it? > No. I have no idea why the multiplatfrom didn't work: it booted just fine and then just before entering userland, it freezed :P Although I had to do some weird stuff to get it working, so it doesn't really matter right now. It was a fun experiment though :) > > So, I see this as preparation work for the move to mach-mvebu. > > > > Does anybody have any insights on how a proper fix would be done > > in the net driver? > > The proper fix would be anything that is related with providing > proper 'local-mac-address' property to the corresponding nodes *and* > properly setup some registers in eth ip, e.g. (R)GMII, basically > all that is usually done by u-boot. > The kirwood.dtsi file already contains 'local-mac-address' property, but the driver is not doing anything with it. I guess the only meaningful case we want to support is a nicely defined MAC passed in that property, and not retaining any other MAC set by some other way, uh? But I haven't really looked at any of this until just now, so I might be completely off.
On Mon, Sep 30, 2013 at 03:31:31PM -0600, Jason Gunthorpe wrote: > On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > > > These clocks should never be gated, since the ethernet interfaces > > forget the assigned MAC address assigned if their clock source is > > turned off. > > Hrm, this hack is only needed to support the hack of passing the MAC > address from the bootloader to the kernel via the ethernet registers. > > If local-mac-address is present in the DT (or fixed up via ATAGS) then > none of this is needed. > > Boards that have well formed DT's don't need this. > > The major downside to this patch is we never get power savings by > disabling the 2nd GE. On my boards it is not connected so we always > want to see it powered down. > > Our bootloader gates the power and turns off the clock, having Linux > turn the clock back on without knowing how to control the power seems > like a possible problem. > > I don't have code, but when this was first brought up it was suggested > to add local-mac-address to the DT nodes if it isn't present in the > common code and don't mess with the clocks. > > Psudeo code: > > void marvell_dt_ethernet_fixup(void) > { > for_each_compatible_node(np, "marvell,kirkwood-eth-port") > { > if (of_get_mac_address(np) != NULL) > continue; > regs = ioremap(of_regs(np->parent,0)); > address = (readl(regs + MAC_ADDR_HIGH) << 32) | > readl(regs + MAC_ADDR_LOW)) > iounmap(regs); > of_add_property(np, {"local-mac-address" = address}); > } > } Or, could we test for the mac address in the driver's suspend routine and set the clock's IGNORE_UNUSED accordingly? That wouldn't cover the case of the modules being loaded after boot, but it would get us power savings when we can. It also has the benefit of being easily discoverable by future users wondering why the clock isn't gated when attempting to conserve power. To that end, we could set the bit as Ezequiel has done, and then change it in the eth driver's probe if there's a mac address in the DT. thx, Jason.
On 10/01/2013 04:22 PM, Ezequiel Garcia wrote: > On Tue, Oct 01, 2013 at 03:49:43PM +0200, Sebastian Hesselbarth wrote: >> On 10/01/2013 03:42 PM, Ezequiel Garcia wrote: >>> On Mon, Sep 30, 2013 at 08:40:09PM -0400, Jason Cooper wrote: >>>> On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: >>>>> These clocks should never be gated, since the ethernet interfaces forget >>>>> the assigned MAC address assigned if their clock source is turned off. >>>>> >>>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >>>>> --- >>>>> Cc: Mike Turquette <mturquette@linaro.org> >>>>> >>>>> drivers/clk/mvebu/kirkwood.c | 9 +++++++-- >>>>> 1 file changed, 7 insertions(+), 2 deletions(-) > [..] >>> The motivation for these 4 patches, came from an attempt of booting a >>> multiplatform kernel Kirkwood, which _almost_ booted fine :P >> >> But the workaround doesn't affect "_almost_", does it? >> > > No. I have no idea why the multiplatfrom didn't work: it booted > just fine and then just before entering userland, it freezed :P > > Although I had to do some weird stuff to get it working, so it > doesn't really matter right now. It was a fun experiment though :) > >>> So, I see this as preparation work for the move to mach-mvebu. >>> >>> Does anybody have any insights on how a proper fix would be done >>> in the net driver? >> >> The proper fix would be anything that is related with providing >> proper 'local-mac-address' property to the corresponding nodes *and* >> properly setup some registers in eth ip, e.g. (R)GMII, basically >> all that is usually done by u-boot. >> > > The kirwood.dtsi file already contains 'local-mac-address' property, > but the driver is not doing anything with it. The local-mac-address property is a placeholder for the real mac address. Also, mv643xx_eth calls of_get_mac_address which fails for the invalid local-mac-address value of [00 00 00 00 00 00]. If you manage to setup local-mac-address in your bootloader, everything should be fine. If we have a DT fixup that takes care of the above for non-DT bootloaders, everything should be fine. > I guess the only meaningful case we want to support is a nicely > defined MAC passed in that property, and not retaining any other > MAC set by some other way, uh? For DT, you rely on valid addresses returned by of_get_mac_address() or fall back to non-DT register readback, if none was passed. As most of our kirkwood boards don't have DT support in u-boot at all, that is what we do right now. Drawback of the register readback is that contents get lost as soon as you gate the clock. So the idea is to backup the contents early in the DT to allow the clocks to be gated. CLK_IGNORE_UNUSED is _not_ the right fix, because actually we *want* those clocks to be gated. Either for those who not use ETH at all or especially those boards providing only one ETH jack. > But I haven't really looked at any of this until just now, > so I might be completely off. I still prefer Jason's approach and would give it a try on LAKML and devtree ML: Backup register contents if of_get_mac_address does not contain a valid address, and gate clocks. Sebastian
On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote: > On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote: ... > > However, I think the problem is that the IP block loses it's mac address > > when gated. That hasn't been solved, all you're doing is moving the > > hack/workaround from one place to another less visible place. > > > > There's an already existing and clock-framework-specific way of preventing > a clock from being ever gated -which seems to match exactly this case- > so I have a hard time seeing why we wouldn't use it. > > Also: I'm wondering why you think this change would 'hide' the clock gating. > It seems whenever a (future?) developer finds these clocks are not gated, > one of the first places he will look for is precisely on those flags. True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power draw didn't change. Let's go look at the driver." But you are correct. I'd really like to see if we can extend your patch to having the driver's probe function detect the mac address in the DT, and change the IGNORE_UNUSED flag on the clock. MikeT might have some thoughts on that since I don't see anyone doing that atm. thx, Jason.
Dear Jason Cooper, On Tue, 1 Oct 2013 11:01:30 -0400, Jason Cooper wrote: > On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote: > > On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote: > ... > > > However, I think the problem is that the IP block loses it's mac address > > > when gated. That hasn't been solved, all you're doing is moving the > > > hack/workaround from one place to another less visible place. > > > > > > > There's an already existing and clock-framework-specific way of preventing > > a clock from being ever gated -which seems to match exactly this case- > > so I have a hard time seeing why we wouldn't use it. > > > > Also: I'm wondering why you think this change would 'hide' the clock gating. > > It seems whenever a (future?) developer finds these clocks are not gated, > > one of the first places he will look for is precisely on those flags. > > True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power > draw didn't change. Let's go look at the driver." But you are correct. > > I'd really like to see if we can extend your patch to having the > driver's probe function detect the mac address in the DT, and change the > IGNORE_UNUSED flag on the clock. MikeT might have some thoughts on that > since I don't see anyone doing that atm. Sorry for jumping into the discussion, but how does that solve the case when the driver is loaded as a module, and therefore unused clocks are disabled before the driver ->probe() function gets called? Thomas
On Tue, Oct 01, 2013 at 05:09:32PM +0200, Thomas Petazzoni wrote: > Dear Jason Cooper, > > On Tue, 1 Oct 2013 11:01:30 -0400, Jason Cooper wrote: > > On Tue, Oct 01, 2013 at 11:17:18AM -0300, Ezequiel Garcia wrote: > > > On Tue, Oct 01, 2013 at 09:58:00AM -0400, Jason Cooper wrote: > > ... > > > > However, I think the problem is that the IP block loses it's mac address > > > > when gated. That hasn't been solved, all you're doing is moving the > > > > hack/workaround from one place to another less visible place. > > > > > > > > > > There's an already existing and clock-framework-specific way of preventing > > > a clock from being ever gated -which seems to match exactly this case- > > > so I have a hard time seeing why we wouldn't use it. > > > > > > Also: I'm wondering why you think this change would 'hide' the clock gating. > > > It seems whenever a (future?) developer finds these clocks are not gated, > > > one of the first places he will look for is precisely on those flags. > > > > True, I'm approaching it from "I unloaded mv643xx_eth.ko, and my power > > draw didn't change. Let's go look at the driver." But you are correct. > > > > I'd really like to see if we can extend your patch to having the > > driver's probe function detect the mac address in the DT, and change the > > IGNORE_UNUSED flag on the clock. MikeT might have some thoughts on that > > since I don't see anyone doing that atm. > > Sorry for jumping into the discussion, but how does that solve the case > when the driver is loaded as a module, and therefore unused clocks are > disabled before the driver ->probe() function gets called? Sorry, I wrote the reply to JasonG just moments before and assumed a little knowledge of that in this reply. We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the clock during boot, then, after boot, mv643xx_eth's probe would look for the mac address in the DT. If it's there, it would unset the flag knowing that it can recover the mac address from the DT. Then we could get the power savings from suspend/unload. thx, Jason.
Dear Jason Cooper, On Tue, 1 Oct 2013 11:15:28 -0400, Jason Cooper wrote: > > Sorry for jumping into the discussion, but how does that solve the case > > when the driver is loaded as a module, and therefore unused clocks are > > disabled before the driver ->probe() function gets called? > > Sorry, I wrote the reply to JasonG just moments before and assumed a > little knowledge of that in this reply. > > We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the > clock during boot, then, after boot, mv643xx_eth's probe would look for > the mac address in the DT. If it's there, it would unset the flag > knowing that it can recover the mac address from the DT. Then we could > get the power savings from suspend/unload. Ah, ok. What was wrong with the idea of getting the MAC address from the hardware early at boot time, and adjust the in-memory Device Tree with this information, so that the mv643xx_eth driver can find them whenever it gets loaded, and regardless of whether clocks have been disabled or not? Thomas
On Tue, Oct 01, 2013 at 05:33:58PM +0200, Thomas Petazzoni wrote: > Dear Jason Cooper, > > On Tue, 1 Oct 2013 11:15:28 -0400, Jason Cooper wrote: > > > > Sorry for jumping into the discussion, but how does that solve the case > > > when the driver is loaded as a module, and therefore unused clocks are > > > disabled before the driver ->probe() function gets called? > > > > Sorry, I wrote the reply to JasonG just moments before and assumed a > > little knowledge of that in this reply. > > > > We would use Ezequiel's patch to ensure CLK_IGNORE_UNUSED is set for the > > clock during boot, then, after boot, mv643xx_eth's probe would look for > > the mac address in the DT. If it's there, it would unset the flag > > knowing that it can recover the mac address from the DT. Then we could > > get the power savings from suspend/unload. > > Ah, ok. What was wrong with the idea of getting the MAC address from > the hardware early at boot time, and adjust the in-memory Device Tree > with this information, so that the mv643xx_eth driver can find them > whenever it gets loaded, and regardless of whether clocks have been > disabled or not? I'm personally not opposed to that at all. iirc, there was some opposition (from Grant?) to modifying the DT from within the kernel. Sorry, I can't put my finger on the objection atm. thx, Jason.
Dear Jason Cooper, On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote: > > Ah, ok. What was wrong with the idea of getting the MAC address from > > the hardware early at boot time, and adjust the in-memory Device Tree > > with this information, so that the mv643xx_eth driver can find them > > whenever it gets loaded, and regardless of whether clocks have been > > disabled or not? > > I'm personally not opposed to that at all. iirc, there was some > opposition (from Grant?) to modifying the DT from within the kernel. > Sorry, I can't put my finger on the objection atm. Strange, because when we discussed the MBus DT binding, we were told exactly the opposite: the in-memory DT should be updated to reflect the MBus windows that are created dynamically (if I remember correctly). So it seems like updating the DT from the kernel was not really frowned upon (but it's true the of API does not really encourage that). Best regards, Thomas
On Tue, Oct 01, 2013 at 05:42:51PM +0200, Thomas Petazzoni wrote: > Dear Jason Cooper, > > On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote: > > > > Ah, ok. What was wrong with the idea of getting the MAC address from > > > the hardware early at boot time, and adjust the in-memory Device Tree > > > with this information, so that the mv643xx_eth driver can find them > > > whenever it gets loaded, and regardless of whether clocks have been > > > disabled or not? > > > > I'm personally not opposed to that at all. iirc, there was some > > opposition (from Grant?) to modifying the DT from within the kernel. > > Sorry, I can't put my finger on the objection atm. > > Strange, because when we discussed the MBus DT binding, we were told > exactly the opposite: the in-memory DT should be updated to reflect the > MBus windows that are created dynamically (if I remember correctly). > So it seems like updating the DT from the kernel was not really > frowned upon (but it's true the of API does not really encourage that). Well, MBus is by it's nature a dynamic beast, the MAC address shouldn't be. ;-) So I think the change was viewed as using the DT as a form of global data storage. At any rate, we could keep guessing, or have Ezequiel float a patch doing just that (store the mac address in the DT if one isn't set) and see where things stand. I suspect the proposed patch would need to be contained within mv643xx_eth.c as opposed to mach-kirkwood/. Which means doing the CLK_IGNORE_UNUSED workaround until the driver loads. thx, Jason.
On 10/01/2013 05:53 PM, Jason Cooper wrote: > On Tue, Oct 01, 2013 at 05:42:51PM +0200, Thomas Petazzoni wrote: >> Dear Jason Cooper, >> >> On Tue, 1 Oct 2013 11:36:31 -0400, Jason Cooper wrote: >> >>>> Ah, ok. What was wrong with the idea of getting the MAC address from >>>> the hardware early at boot time, and adjust the in-memory Device Tree >>>> with this information, so that the mv643xx_eth driver can find them >>>> whenever it gets loaded, and regardless of whether clocks have been >>>> disabled or not? >>> >>> I'm personally not opposed to that at all. iirc, there was some >>> opposition (from Grant?) to modifying the DT from within the kernel. >>> Sorry, I can't put my finger on the objection atm. >> >> Strange, because when we discussed the MBus DT binding, we were told >> exactly the opposite: the in-memory DT should be updated to reflect the >> MBus windows that are created dynamically (if I remember correctly). >> So it seems like updating the DT from the kernel was not really >> frowned upon (but it's true the of API does not really encourage that). > > Well, MBus is by it's nature a dynamic beast, the MAC address shouldn't > be. ;-) So I think the change was viewed as using the DT as a form of > global data storage. > > At any rate, we could keep guessing, or have Ezequiel float a patch > doing just that (store the mac address in the DT if one isn't set) and > see where things stand. Have a look at Jason G's proposal, arch/xtensa/platforms/xtfpga/setup.c, arch/arm/mach-mxs/mach-mxs.c They all use of_update_property (JasonG uses of_add_property, which is finally called by of_update_property if non-existant). IMHO updating DT that is missing a _valid_ MAC address is the only option we have here. BTW, introducing of_update_property to mach-mxs.c got its Acked-by from Benjamin Herrenschmidt, so I guess it is accepted practice to update DT here. > I suspect the proposed patch would need to be contained within > mv643xx_eth.c as opposed to mach-kirkwood/. Which means doing the > CLK_IGNORE_UNUSED workaround until the driver loads.
On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote: > The local-mac-address property is a placeholder for the real mac > address. Also, mv643xx_eth calls of_get_mac_address which fails for > the invalid local-mac-address value of [00 00 00 00 00 00]. > > If you manage to setup local-mac-address in your bootloader, everything > should be fine. FWIW, my environment is like this, and it works fine (this is recent, 3.10 didn't work). The driver now properly sets the MAC from the DT. The driver will also need an update to call of_get_phy_mode and set the interface properly, and the board files will need update to specify the phy-mode property. I'm guessing things are working for most people because the POR value happens to match the board? > If we have a DT fixup that takes care of the above for non-DT > bootloaders, everything should be fine. Exactly, the problem is a broken DT on input to the kernel, the proper action, IMHO, is to correct the DT in the kernel, so the hacking is contained. Similar to how pci-fixups.c centralizes fixing of broken firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it for ATAG conversion. BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot loaders on these systems not pass the mac at all?? > I still prefer Jason's approach and would give it a try on LAKML and > devtree ML: Backup register contents if of_get_mac_address does not > contain a valid address, and gate clocks. FWIW, I would not use the language of 'backup' - this is a fixup correcting an unsupported input DT. IMHO, when this code triggers it should also: printk(KERN_ERROR FW_BUG "local-mac-address is not set"); Jason
On Tue, Oct 01, 2013 at 10:45:56AM -0600, Jason Gunthorpe wrote: ... > BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot > loaders on these systems not pass the mac at all?? Because the ATAGs used were never approved. The vendors just randomly picked their own. thx, Jason.
On 10/01/2013 06:45 PM, Jason Gunthorpe wrote: > On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote: > >> The local-mac-address property is a placeholder for the real mac >> address. Also, mv643xx_eth calls of_get_mac_address which fails for >> the invalid local-mac-address value of [00 00 00 00 00 00]. >> >> If you manage to setup local-mac-address in your bootloader, everything >> should be fine. > > FWIW, my environment is like this, and it works fine (this is recent, > 3.10 didn't work). The driver now properly sets the MAC from the DT. > > The driver will also need an update to call of_get_phy_mode and set > the interface properly, and the board files will need update to > specify the phy-mode property. I'm guessing things are working for > most people because the POR value happens to match the board? > >> If we have a DT fixup that takes care of the above for non-DT >> bootloaders, everything should be fine. > > Exactly, the problem is a broken DT on input to the kernel, the proper > action, IMHO, is to correct the DT in the kernel, so the hacking is > contained. Similar to how pci-fixups.c centralizes fixing of broken > firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it > for ATAG conversion. > > BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot > loaders on these systems not pass the mac at all?? > >> I still prefer Jason's approach and would give it a try on LAKML and >> devtree ML: Backup register contents if of_get_mac_address does not >> contain a valid address, and gate clocks. > > FWIW, I would not use the language of 'backup' - this is a fixup > correcting an unsupported input DT. > > IMHO, when this code triggers it should also: > > printk(KERN_ERROR FW_BUG "local-mac-address is not set"); Ezequiel, before you jump to prepare a patch for the above, I made one based on my previous work and JasonG's suggestions above. Testing it right now, looks promising. @JasonC: To avoid rebase mess again, where do you want it based against? Sebastian
On 10/01/2013 09:10 PM, Sebastian Hesselbarth wrote: > On 10/01/2013 06:45 PM, Jason Gunthorpe wrote: >> On Tue, Oct 01, 2013 at 04:43:54PM +0200, Sebastian Hesselbarth wrote: >> >>> The local-mac-address property is a placeholder for the real mac >>> address. Also, mv643xx_eth calls of_get_mac_address which fails for >>> the invalid local-mac-address value of [00 00 00 00 00 00]. >>> >>> If you manage to setup local-mac-address in your bootloader, everything >>> should be fine. >> >> FWIW, my environment is like this, and it works fine (this is recent, >> 3.10 didn't work). The driver now properly sets the MAC from the DT. >> >> The driver will also need an update to call of_get_phy_mode and set >> the interface properly, and the board files will need update to >> specify the phy-mode property. I'm guessing things are working for >> most people because the POR value happens to match the board? >> >>> If we have a DT fixup that takes care of the above for non-DT >>> bootloaders, everything should be fine. >> >> Exactly, the problem is a broken DT on input to the kernel, the proper >> action, IMHO, is to correct the DT in the kernel, so the hacking is >> contained. Similar to how pci-fixups.c centralizes fixing of broken >> firmware for PCI, and how CONFIG_ARM_ATAG_DTB_COMPAT centralizes it >> for ATAG conversion. >> >> BTW, why doesn't CONFIG_ARM_ATAG_DTB_COMPAT fix this? Do the boot >> loaders on these systems not pass the mac at all?? >> >>> I still prefer Jason's approach and would give it a try on LAKML and >>> devtree ML: Backup register contents if of_get_mac_address does not >>> contain a valid address, and gate clocks. >> >> FWIW, I would not use the language of 'backup' - this is a fixup >> correcting an unsupported input DT. >> >> IMHO, when this code triggers it should also: >> >> printk(KERN_ERROR FW_BUG "local-mac-address is not set"); > > Ezequiel, > > before you jump to prepare a patch for the above, I made one based on > my previous work and JasonG's suggestions above. Testing it right now, > looks promising. And works: Uncompressing Linux... done, booting the kernel. [ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 3.12.0-rc3-dirty (hesselba@edge) (gcc version 4.4.6 (Debian 4.4.6-14) ) #9 PREEMPT Tue Oct 1 21:01:34 CEST 2013 ... [ 0.100128] [Firmware Bug]: /ocp@f1000000/ethernet-controller@72000/ethernet0-port@0: local-mac-address is not set ... # cat /sys/kernel/debug/clk/clk_summary clock enable_cnt prepare_cnt rate --------------------------------------------------------------------- ... tclk 6 6 200000000 ... ge1 0 0 200000000 ... ge0 0 0 200000000 # insmod mvmdio.ko [ 172.888802] libphy: orion_mdio_bus: probed # insmod mv643xx_eth.ko [ 175.790366] mv643xx_eth: MV-643xx 10/100/1000 ethernet driver version 1.4 [ 176.811888] mv643xx_eth_port mv643xx_eth_port.0 eth0: port 0 with MAC address 02:50:43:19:fd:e2 > @JasonC: To avoid rebase mess again, where do you want it based against?
On Tue, Oct 01, 2013 at 09:10:10PM +0200, Sebastian Hesselbarth wrote: ... > before you jump to prepare a patch for the above, I made one based on > my previous work and JasonG's suggestions above. Testing it right now, > looks promising. > > @JasonC: To avoid rebase mess again, where do you want it based against? Just base against v3.12-rc1 and we'll see there's going to be any issues with it. Hopefully, the mv643xx_eth changes can be independent of the other changes... thx, Jason.
On 10/02/2013 02:02 PM, Jason Cooper wrote: > On Tue, Oct 01, 2013 at 09:10:10PM +0200, Sebastian Hesselbarth wrote: > ... >> before you jump to prepare a patch for the above, I made one based on >> my previous work and JasonG's suggestions above. Testing it right now, >> looks promising. >> >> @JasonC: To avoid rebase mess again, where do you want it based against? > > Just base against v3.12-rc1 and we'll see there's going to be any issues > with it. Hopefully, the mv643xx_eth changes can be independent of the > other changes... Ok, the mv643xx_eth fixes have been sent as fixes for v3.12 while the corresponding kirkwood workaround should drop in v3.13. I hopefully find some time tonight to prepare workaround patches. Sebastian
On Mon, Sep 30, 2013 at 06:03:29PM -0300, Ezequiel Garcia wrote: > These clocks should never be gated, since the ethernet interfaces forget > the assigned MAC address assigned if their clock source is turned off. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > Cc: Mike Turquette <mturquette@linaro.org> > > drivers/clk/mvebu/kirkwood.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) fyi - I've dropped this and #4 favor of Sebastian's series covering the same issue. thx, Jason.
diff --git a/drivers/clk/mvebu/kirkwood.c b/drivers/clk/mvebu/kirkwood.c index 2636a55..69d9603 100644 --- a/drivers/clk/mvebu/kirkwood.c +++ b/drivers/clk/mvebu/kirkwood.c @@ -220,7 +220,12 @@ CLK_OF_DECLARE(mv88f6180_core_clk, "marvell,mv88f6180-core-clock", */ static const struct clk_gating_soc_desc kirkwood_gating_desc[] __initconst = { - { "ge0", NULL, 0, 0 }, + /* + * The ethernet interfaces forget their assigned MAC address + * if the clocks are turned off. Until proper DT support + * is available we prevent ge0 and ge1 gating for now. + */ + { "ge0", NULL, 0, CLK_IGNORE_UNUSED }, { "pex0", NULL, 2, 0 }, { "usb0", NULL, 3, 0 }, { "sdio", NULL, 4, 0 }, @@ -234,7 +239,7 @@ static const struct clk_gating_soc_desc kirkwood_gating_desc[] __initconst = { { "xor1", NULL, 16, 0 }, { "crypto", NULL, 17, 0 }, { "pex1", NULL, 18, 0 }, - { "ge1", NULL, 19, 0 }, + { "ge1", NULL, 19, CLK_IGNORE_UNUSED }, { "tdm", NULL, 20, 0 }, { } };
These clocks should never be gated, since the ethernet interfaces forget the assigned MAC address assigned if their clock source is turned off. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- Cc: Mike Turquette <mturquette@linaro.org> drivers/clk/mvebu/kirkwood.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)