Re: [RFC v5 03/15] MIPS: ath79: use clk-ath79.c driver for AR933X

[Antony Pavlov <antonynpavlov@gmail.com>]

On Tue, 9 Feb 2016 23:07:21 +0100
Alban <albeu@free.fr> wrote:

> On Tue,  9 Feb 2016 11:13:49 +0300
> Antony Pavlov <antonynpavlov@gmail.com> wrote:
> 
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Cc: Gabor Juhos <juhosg@openwrt.org>
> > Cc: Alban Bedel <albeu@free.fr>
> > Cc: linux-mips@linux-mips.org
> > ---
> >  arch/mips/ath79/clock.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/mips/ath79/clock.c b/arch/mips/ath79/clock.c
> > index eb5117c..3c98eba 100644
> > --- a/arch/mips/ath79/clock.c
> > +++ b/arch/mips/ath79/clock.c
> > @@ -24,6 +24,7 @@
> >  #include <asm/mach-ath79/ath79.h>
> >  #include <asm/mach-ath79/ar71xx_regs.h>
> >  #include "common.h"
> > +#include "machtypes.h"
> >  
> >  #define AR71XX_BASE_FREQ	40000000
> >  #define AR724X_BASE_FREQ	5000000
> > @@ -441,7 +442,9 @@ static void __init qca955x_clocks_init(void)
> >  
> >  void __init ath79_clocks_init(void)
> >  {
> > -	if (soc_is_ar71xx())
> > +	if (IS_ENABLED(CONFIG_OF) && mips_machtype == ATH79_MACH_GENERIC_OF) {
> > +		/* pass */
> > +	} else if (soc_is_ar71xx())
> 
> This will break all non AR9330 SoC as their clock won't be properly
> initialized, so this is not acceptable.

Actually this can't broke non-OF board.
This breaks only OF AR9132-based board WR1043DN just because I have not
included AR9132-related patches into RFC v5 patcheseries.

But after adding these patches the problem will disappear.

> I would also really prefer if we can avoid having two completely
> different implementation for legacy and OF platforms. Ideally both
> legacy and OF should use the same core code, just with 2 different
> wrappers. The OF wrapper would just need to get the parent clock from
> DT and call the core setup. The legacy code path would need to create
> the fixed rate parent clock with the current hard coded value, call the
> core setup, then register the clkdev and alias mapping.
> 
> I find it important to avoid code duplication because that mean double
> the amount of testing work. But in practice that generally mean that
> only one half get tested as no one wants to do double the work.

I'm completely agreed your plan. The RFC v5 patchseries was intended
to proof of-clk driver implementation. Please not that it's a RFC series.

> > @@ -484,7 +487,6 @@ static void __init ath79_clocks_init_dt(struct device_node *np)
> >  CLK_OF_DECLARE(ar7100, "qca,ar7100-pll", ath79_clocks_init_dt);
> >  CLK_OF_DECLARE(ar7240, "qca,ar7240-pll", ath79_clocks_init_dt);
> >  CLK_OF_DECLARE(ar9130, "qca,ar9130-pll", ath79_clocks_init_dt);
> > -CLK_OF_DECLARE(ar9330, "qca,ar9330-pll", ath79_clocks_init_dt);
> >  CLK_OF_DECLARE(ar9340, "qca,ar9340-pll", ath79_clocks_init_dt);
> >  CLK_OF_DECLARE(ar9550, "qca,qca9550-pll", ath79_clocks_init_dt);
> >  #endif
> 
> This should have been part of the new driver, otherwise this will break
> bisecting as there would be a version with 2 CLK_OF_DECLARE for ar9330.

No, it can't break a bisection because there is no defconfig that enables both simultaneously.
Of cause you always prepare special configuration by hand :) if you wish intentionally
break your bisection.
If I have missed something then please explain exactly the situation then a bisection
can be broken.
 
-- 
Best regards,
  Antony Pavlov