On Tue, 9 Feb 2016 23:07:21 +0100
Alban <albeu@xxxxxxx> wrote:
> On Tue, 9 Feb 2016 11:13:49 +0300
> Antony Pavlov <antonynpavlov@xxxxxxxxx> wrote:
>
> > Signed-off-by: Antony Pavlov <antonynpavlov@xxxxxxxxx>
> > Cc: Gabor Juhos <juhosg@xxxxxxxxxxx>
> > Cc: Alban Bedel <albeu@xxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > ---
> > 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
