| Message ID | 1413984884-20273-3-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Ezequiel Garcia, On Wed, 22 Oct 2014 10:34:42 -0300, Ezequiel Garcia wrote: > The 25 MHz reference clock has better stability so its use is preferred over the > core clock. Changes the Armada 375 clock initialization to use this reference > clock. To ensure the driver is compatible with an old devicetree, also provide > a fallback path which will silently return to the previous behavior. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) The Armada 375 case should be similar to the Armada XP case, so can't we use the existing armadaxp_wdt_clock_init() function ? Thanks, Thomas
On Wed, Oct 22, 2014 at 10:34:42AM -0300, Ezequiel Garcia wrote: > The 25 MHz reference clock has better stability so its use is preferred over the > core clock. Changes the Armada 375 clock initialization to use this reference > clock. To ensure the driver is compatible with an old devicetree, also provide > a fallback path which will silently return to the previous behavior. Hi Ezequiel There is now quite a lot of code in orion_wdt.c which is not relevant to Orion5x and Kirkwood. Would it be possible to put some of it inside a #ifdef MACH_MVEBU_V7? Andrew > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c > index 00d0741..6452fa2 100644 > --- a/drivers/watchdog/orion_wdt.c > +++ b/drivers/watchdog/orion_wdt.c > @@ -114,6 +114,44 @@ static int armada370_wdt_clock_init(struct platform_device *pdev, > return 0; > } > > +static int armada375_wdt_clock_init(struct platform_device *pdev, > + struct orion_watchdog *dev) > +{ > + int ret; > + > + dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed"); > + if (!IS_ERR(dev->clk)) { > + > + ret = clk_prepare_enable(dev->clk); > + if (ret) { > + clk_put(dev->clk); > + return ret; > + } > + > + atomic_io_modify(dev->reg + TIMER_CTRL, > + WDT_AXP_FIXED_ENABLE_BIT, > + WDT_AXP_FIXED_ENABLE_BIT); > + dev->clk_rate = clk_get_rate(dev->clk); > + return 0; > + } > + > + /* Mandatory fallback for proper devicetree backward compatibility */ > + dev->clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(dev->clk)) > + return PTR_ERR(dev->clk); > + ret = clk_prepare_enable(dev->clk); > + if (ret) { > + clk_put(dev->clk); > + return ret; > + } > + > + atomic_io_modify(dev->reg + TIMER_CTRL, > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT), > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT)); > + dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO; > + return 0; > +} > + > static int armadaxp_wdt_clock_init(struct platform_device *pdev, > struct orion_watchdog *dev) > { > @@ -394,7 +432,7 @@ static const struct orion_watchdog_data armada375_data = { > .rstout_mask_bit = BIT(10), > .wdt_enable_bit = BIT(8), > .wdt_counter_offset = 0x34, > - .clock_init = armada370_wdt_clock_init, > + .clock_init = armada375_wdt_clock_init, > .enabled = armada375_enabled, > .start = armada375_start, > .stop = armada375_stop, > -- > 2.1.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 10/22/2014 11:02 AM, Andrew Lunn wrote: > On Wed, Oct 22, 2014 at 10:34:42AM -0300, Ezequiel Garcia wrote: >> The 25 MHz reference clock has better stability so its use is preferred over the >> core clock. Changes the Armada 375 clock initialization to use this reference >> clock. To ensure the driver is compatible with an old devicetree, also provide >> a fallback path which will silently return to the previous behavior. > > Hi Ezequiel > > There is now quite a lot of code in orion_wdt.c which is not relevant > to Orion5x and Kirkwood. Would it be possible to put some of it inside > a #ifdef MACH_MVEBU_V7? > Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. Just did a quick hack enclosing all the armada-xxx stuff around #if 0 and here's the result: $ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68) function old new delta orion_wdt_probe 732 744 +12 orion_wdt_get_timeleft 44 40 -4 orion_enabled 68 60 -8 orion_start 120 88 -32 orion_wdt_ping 80 44 -36 To be honest, I don't think it's worth the ugliness.
On Wed, Oct 22, 2014 at 07:41:27PM -0300, Ezequiel Garcia wrote: > On 10/22/2014 11:02 AM, Andrew Lunn wrote: > > On Wed, Oct 22, 2014 at 10:34:42AM -0300, Ezequiel Garcia wrote: > >> The 25 MHz reference clock has better stability so its use is preferred over the > >> core clock. Changes the Armada 375 clock initialization to use this reference > >> clock. To ensure the driver is compatible with an old devicetree, also provide > >> a fallback path which will silently return to the previous behavior. > > > > Hi Ezequiel > > > > There is now quite a lot of code in orion_wdt.c which is not relevant > > to Orion5x and Kirkwood. Would it be possible to put some of it inside > > a #ifdef MACH_MVEBU_V7? > > > > Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. > Just did a quick hack enclosing all the armada-xxx stuff around #if 0 > and here's the result: > > $ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o > add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68) > function old new delta > orion_wdt_probe 732 744 +12 > orion_wdt_get_timeleft 44 40 -4 > orion_enabled 68 60 -8 > orion_start 120 88 -32 > orion_wdt_ping 80 44 -36 > > To be honest, I don't think it's worth the ugliness. I agree, especially since each #ifdef means that some code may not be compiled and errors are easier to creep into the code. Guenter
On Wed, Oct 22, 2014 at 07:41:27PM -0300, Ezequiel Garcia wrote: > On 10/22/2014 11:02 AM, Andrew Lunn wrote: > > On Wed, Oct 22, 2014 at 10:34:42AM -0300, Ezequiel Garcia wrote: > >> The 25 MHz reference clock has better stability so its use is preferred over the > >> core clock. Changes the Armada 375 clock initialization to use this reference > >> clock. To ensure the driver is compatible with an old devicetree, also provide > >> a fallback path which will silently return to the previous behavior. > > > > Hi Ezequiel > > > > There is now quite a lot of code in orion_wdt.c which is not relevant > > to Orion5x and Kirkwood. Would it be possible to put some of it inside > > a #ifdef MACH_MVEBU_V7? > > > > Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. > Just did a quick hack enclosing all the armada-xxx stuff around #if 0 > and here's the result: > > $ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o > add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68) > function old new delta > orion_wdt_probe 732 744 +12 > orion_wdt_get_timeleft 44 40 -4 > orion_enabled 68 60 -8 > orion_start 120 88 -32 > orion_wdt_ping 80 44 -36 Hi Ezequiel I did a similar test: size drivers/watchdog/orion_wdt.o-* text data bss dec hex filename 4428 100 1 4529 11b1 drivers/watchdog/orion_wdt.o-full-fat 2324 100 1 2425 979 drivers/watchdog/orion_wdt.o-skimmed So for v5 kirkwood/orion5x, there is about 90% overhead from the v7 code, in the text section. This is for -rc1 code, before adding any more v7 code in this patchset. I don't think the #ifdef's look that scary. Andrew
On 10/22/2014 08:59 PM, Andrew Lunn wrote: > On Wed, Oct 22, 2014 at 07:41:27PM -0300, Ezequiel Garcia wrote: >> On 10/22/2014 11:02 AM, Andrew Lunn wrote: >>> On Wed, Oct 22, 2014 at 10:34:42AM -0300, Ezequiel Garcia wrote: >>>> The 25 MHz reference clock has better stability so its use is preferred over the >>>> core clock. Changes the Armada 375 clock initialization to use this reference >>>> clock. To ensure the driver is compatible with an old devicetree, also provide >>>> a fallback path which will silently return to the previous behavior. >>> >>> Hi Ezequiel >>> >>> There is now quite a lot of code in orion_wdt.c which is not relevant >>> to Orion5x and Kirkwood. Would it be possible to put some of it inside >>> a #ifdef MACH_MVEBU_V7? >>> >> >> Hum.. I found ifdefs scary, so I tend to avoid them if at all possible. >> Just did a quick hack enclosing all the armada-xxx stuff around #if 0 >> and here's the result: >> >> $ ./scripts/bloat-o-meter ~/linux/.builds/mvebu_v7/drivers/watchdog/orion_wdt.o ~/linux/.builds/orion5x/drivers/watchdog/orion_wdt.o >> add/remove: 0/0 grow/shrink: 1/4 up/down: 12/-80 (-68) >> function old new delta >> orion_wdt_probe 732 744 +12 >> orion_wdt_get_timeleft 44 40 -4 >> orion_enabled 68 60 -8 >> orion_start 120 88 -32 >> orion_wdt_ping 80 44 -36 > > Hi Ezequiel > > I did a similar test: > > size drivers/watchdog/orion_wdt.o-* > text data bss dec hex filename > 4428 100 1 4529 11b1 drivers/watchdog/orion_wdt.o-full-fat > 2324 100 1 2425 979 drivers/watchdog/orion_wdt.o-skimmed > > So for v5 kirkwood/orion5x, there is about 90% overhead from the v7 > code, in the text section. This is for -rc1 code, before adding any > more v7 code in this patchset. > > I don't think the #ifdef's look that scary. > I understand it's a lot of overhead when you think of the relative driver's size, but still it's just a 2 KiB saving. Don't get me wrong, I'm all for reducing bloat, but I don't want to author a patch that ifdefs all around the place to save a couple kilobytes.
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c index 00d0741..6452fa2 100644 --- a/drivers/watchdog/orion_wdt.c +++ b/drivers/watchdog/orion_wdt.c @@ -114,6 +114,44 @@ static int armada370_wdt_clock_init(struct platform_device *pdev, return 0; } +static int armada375_wdt_clock_init(struct platform_device *pdev, + struct orion_watchdog *dev) +{ + int ret; + + dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed"); + if (!IS_ERR(dev->clk)) { + + ret = clk_prepare_enable(dev->clk); + if (ret) { + clk_put(dev->clk); + return ret; + } + + atomic_io_modify(dev->reg + TIMER_CTRL, + WDT_AXP_FIXED_ENABLE_BIT, + WDT_AXP_FIXED_ENABLE_BIT); + dev->clk_rate = clk_get_rate(dev->clk); + return 0; + } + + /* Mandatory fallback for proper devicetree backward compatibility */ + dev->clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(dev->clk)) + return PTR_ERR(dev->clk); + ret = clk_prepare_enable(dev->clk); + if (ret) { + clk_put(dev->clk); + return ret; + } + + atomic_io_modify(dev->reg + TIMER_CTRL, + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT), + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT)); + dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO; + return 0; +} + static int armadaxp_wdt_clock_init(struct platform_device *pdev, struct orion_watchdog *dev) { @@ -394,7 +432,7 @@ static const struct orion_watchdog_data armada375_data = { .rstout_mask_bit = BIT(10), .wdt_enable_bit = BIT(8), .wdt_counter_offset = 0x34, - .clock_init = armada370_wdt_clock_init, + .clock_init = armada375_wdt_clock_init, .enabled = armada375_enabled, .start = armada375_start, .stop = armada375_stop,
The 25 MHz reference clock has better stability so its use is preferred over the core clock. Changes the Armada 375 clock initialization to use this reference clock. To ensure the driver is compatible with an old devicetree, also provide a fallback path which will silently return to the previous behavior. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)