| Message ID | 1343661359-10150-5-git-send-email-ian.molton@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 30, 2012 at 04:15:57PM +0100, Ian Molton wrote: > This patch modifies the ethernet setup helper functions so that they can be > used /purely/ to set up the clocks. Hi Ian Set up is a bit ambiguous. I would actually say, start the clock ticking, if it is not already. I'm also not sure this is the best way to do it. I'd like to throw in a counter proposal, and then we can discuss.... Problems start then the ethernet driver is a module, not built in. In this situation, the clock gets turned off, in a lateinitcall, and then later turned back on again when the module loads. Unfortunately, by then its forgotten its own MAC address, as programmed by u-boot. So this clk_prepare_enable() is here to ensure that the clock does not get turned off, when we know the module is likely to be loaded sometime later. I've not looked at the clk DT bindings yet. Does it provide a mechanism to prepare & enable a named clock? Maybe it does, but this seems a bit of an edge case. What i would instead do is add code to board-dt.c which looks into the DT and see if it finds nodes egige0/egige1 and if so, calls clk_prepare_enable(). We then don't need any per board code. Andrew > > This is part of ongoing work to enable device tree support in the mv643xx.c > ethernet driver, where as yet the kirkwood platform does not have proper clk > support. > > This should allow a gradual migration to device tree and later to proper clk > support, wherupon the helper functions can be removed entirely. > > Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> > --- > arch/arm/mach-kirkwood/common.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c > index c4b64ad..c1776ea 100644 > --- a/arch/arm/mach-kirkwood/common.c > +++ b/arch/arm/mach-kirkwood/common.c > @@ -299,7 +299,8 @@ void __init kirkwood_ehci_init(void) > ****************************************************************************/ > void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) > { > - orion_ge00_init(eth_data, > + if (eth_data) > + orion_ge00_init(eth_data, > GE00_PHYS_BASE, IRQ_KIRKWOOD_GE00_SUM, > IRQ_KIRKWOOD_GE00_ERR); > /* The interface forgets the MAC address assigned by u-boot if > @@ -313,7 +314,8 @@ void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) > ****************************************************************************/ > void __init kirkwood_ge01_init(struct mv643xx_eth_platform_data *eth_data) > { > - orion_ge01_init(eth_data, > + if (eth_data) > + orion_ge01_init(eth_data, > GE01_PHYS_BASE, IRQ_KIRKWOOD_GE01_SUM, > IRQ_KIRKWOOD_GE01_ERR); > clk_prepare_enable(ge1); > -- > 1.7.9.5 >
If I'm reading this correctly, if kirkwood_ge00_init is called without any data, all that happens is the clock is enabled. If the right data is passed in, the ethernet interface is initialized, and then the clock is enabled. Patch 6 in your series sets all the existing DT board files to call kirkwood_ge00_init with no data. Would it make more sense for readability to separate the clock prep and other interface initialization so DT board files don't appear to be calling for a full turn up despite now handling that in the DTS tree? IE instead of converting all the board-XXX.c to call kirkwood_ge00_init(NULL) have them call a new function, kirkwood_ge00_clk_prep(NULL) instead, assuming the clock can't be handled directly via DTS. Josh C On Mon, Jul 30, 2012 at 11:15 AM, Ian Molton <ian.molton@codethink.co.uk> wrote: > This patch modifies the ethernet setup helper functions so that they can be > used /purely/ to set up the clocks. > > This is part of ongoing work to enable device tree support in the mv643xx.c > ethernet driver, where as yet the kirkwood platform does not have proper clk > support. > > This should allow a gradual migration to device tree and later to proper clk > support, wherupon the helper functions can be removed entirely. > > Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> > --- > arch/arm/mach-kirkwood/common.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c > index c4b64ad..c1776ea 100644 > --- a/arch/arm/mach-kirkwood/common.c > +++ b/arch/arm/mach-kirkwood/common.c > @@ -299,7 +299,8 @@ void __init kirkwood_ehci_init(void) > ****************************************************************************/ > void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) > { > - orion_ge00_init(eth_data, > + if (eth_data) > + orion_ge00_init(eth_data, > GE00_PHYS_BASE, IRQ_KIRKWOOD_GE00_SUM, > IRQ_KIRKWOOD_GE00_ERR); > /* The interface forgets the MAC address assigned by u-boot if > @@ -313,7 +314,8 @@ void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) > ****************************************************************************/ > void __init kirkwood_ge01_init(struct mv643xx_eth_platform_data *eth_data) > { > - orion_ge01_init(eth_data, > + if (eth_data) > + orion_ge01_init(eth_data, > GE01_PHYS_BASE, IRQ_KIRKWOOD_GE01_SUM, > IRQ_KIRKWOOD_GE01_ERR); > clk_prepare_enable(ge1); > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 30/07/12 17:12, Andrew Lunn wrote: > On Mon, Jul 30, 2012 at 04:15:57PM +0100, Ian Molton wrote: >> This patch modifies the ethernet setup helper functions so that >> they can be used /purely/ to set up the clocks. > > Hi Ian > > Set up is a bit ambiguous. I would actually say, start the clock > ticking, if it is not already. Fair. > I'm also not sure this is the best way to do it. I'd like to throw > in a counter proposal, and then we can discuss.... > > Problems start then the ethernet driver is a module, not built in. I find that even if built in, it fails, not because it forgets its MAC, but later, it hangs because as yet it does not know how to handle the clk when it is set up from DT bindings. (Kirkwood doesnt really have this support yet AFAICT). > I've not looked at the clk DT bindings yet. Does it provide a > mechanism to prepare & enable a named clock? Maybe it does, but this > seems a bit of an edge case. If it does, I cant see it. > What i would instead do is add code to board-dt.c which looks into > the DT and see if it finds nodes egige0/egige1 and if so, calls > clk_prepare_enable(). We then don't need any per board code. I agree, this seems like a good compromise for now. I will implement this for v2. -Ian -Ian
> I find that even if built in, it fails, not because it forgets its MAC, but > later, it hangs because as yet it does not know how to handle the clk > when it is set up from DT bindings. (Kirkwood doesnt really have this > support yet AFAICT). Ah, of course. You are missing auxdata in board-dt.c! Andrew
On 30/07/12 17:46, Josh Coombs wrote: > If I'm reading this correctly, if kirkwood_ge00_init is called without > any data, all that happens is the clock is enabled. If the right data > is passed in, the ethernet interface is initialized, and then the > clock is enabled. > > Patch 6 in your series sets all the existing DT board files to call > kirkwood_ge00_init with no data. > > Would it make more sense for readability to separate the clock prep > and other interface initialization so DT board files don't appear to > be calling for a full turn up despite now handling that in the DTS > tree? IE instead of converting all the board-XXX.c to call > kirkwood_ge00_init(NULL) have them call a new function, > kirkwood_ge00_clk_prep(NULL) instead, assuming the clock can't be > handled directly via DTS. This is certainly possible, but I was trying to avoid yet another function being added to the sources with a (necessarily) short lifespan - it will only be needed until such time as the clk stuff is sorted out, which is on my TODO list. My preference would be to add a comment explaining the choice in the code. If its preferred, however, I have no real objection to splitting out the functionality. -Ian
On 30/07/12 17:46, Josh Coombs wrote: > If I'm reading this correctly, if kirkwood_ge00_init is called without > any data, all that happens is the clock is enabled. If the right data > is passed in, the ethernet interface is initialized, and then the > clock is enabled. > > Patch 6 in your series sets all the existing DT board files to call > kirkwood_ge00_init with no data. Actually Im thinking of dropping this change altogether - it would seem that since adding aliases so that the driver can managed the clocks itself (thanks Andrew), the DT code apparently doesnt disable the clocks, so the MAC is preserved. I see no reason to keep the calls now that the driver manages to hang onto its clocks. -Ian
> I see no reason to keep the calls now that the driver manages to > hang onto its clocks. Hi Ian Did you test this with it built as a module? That is the real test.... Andrew
On 31/07/12 12:08, Andrew Lunn wrote: >> I see no reason to keep the calls now that the driver manages to >> hang onto its clocks. > Hi Ian > > Did you test this with it built as a module? That is the real test.... Rats. Yes, however it managed to survive. The minute I read this was the minute it died. sods law :( Guess it'll still be needed for now. So, the question is about how to implement it - I like the 'probe the OF device name' idea - I can implement that quickly enough. Once thats done, I can push out a v2 patch, all the other issues are now addressed, AFAICS. -Ian
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c index c4b64ad..c1776ea 100644 --- a/arch/arm/mach-kirkwood/common.c +++ b/arch/arm/mach-kirkwood/common.c @@ -299,7 +299,8 @@ void __init kirkwood_ehci_init(void) ****************************************************************************/ void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) { - orion_ge00_init(eth_data, + if (eth_data) + orion_ge00_init(eth_data, GE00_PHYS_BASE, IRQ_KIRKWOOD_GE00_SUM, IRQ_KIRKWOOD_GE00_ERR); /* The interface forgets the MAC address assigned by u-boot if @@ -313,7 +314,8 @@ void __init kirkwood_ge00_init(struct mv643xx_eth_platform_data *eth_data) ****************************************************************************/ void __init kirkwood_ge01_init(struct mv643xx_eth_platform_data *eth_data) { - orion_ge01_init(eth_data, + if (eth_data) + orion_ge01_init(eth_data, GE01_PHYS_BASE, IRQ_KIRKWOOD_GE01_SUM, IRQ_KIRKWOOD_GE01_ERR); clk_prepare_enable(ge1);
This patch modifies the ethernet setup helper functions so that they can be used /purely/ to set up the clocks. This is part of ongoing work to enable device tree support in the mv643xx.c ethernet driver, where as yet the kirkwood platform does not have proper clk support. This should allow a gradual migration to device tree and later to proper clk support, wherupon the helper functions can be removed entirely. Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> --- arch/arm/mach-kirkwood/common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)