Re: [PATCH v2 2/2] drivers: clk: zynqmp: Update fraction clock check from custom type flags

[Jolly Shah]

Hi Stephan,

> ------Original Message------
> From: Stephen Boyd <sboyd@xxxxxxxxxx>
> Sent:  Thursday, May 28, 2020 4:12PM
> To: Jolly Shah <jolly.shah@xxxxxxxxxx>, Arm <arm@xxxxxxxxxx>, 
Linux-clk <linux-clk@xxxxxxxxxxxxxxx>, Michal Simek 
<michal.simek@xxxxxxxxxx>, Mturquette <mturquette@xxxxxxxxxxxx>, Olof 
<olof@xxxxxxxxx>
> Cc: Rajan Vaja <rajanv@xxxxxxxxxx>, 
Linux-arm-kernel@xxxxxxxxxxxxxxxxxxx 
<linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, Linux-kernel@xxxxxxxxxxxxxxx 
<linux-kernel@xxxxxxxxxxxxxxx>, Tejas Patel <tejas.patel@xxxxxxxxxx>, 
Rajan Vaja <rajan.vaja@xxxxxxxxxx>
> Subject: Re: [PATCH v2 2/2] drivers: clk: zynqmp: Update fraction 
clock check from custom type flags
>
> Quoting Jolly Shah (2020-05-28 10:44:01)
> > Hi Stephan,
> >
> > Thanks for the review.
> >
> >   > ------Original Message------
> >   > From: Stephen Boyd <sboyd@xxxxxxxxxx>
> >   > Sent:  Tuesday, May 26, 2020 6:08PM
> >   > To: Jolly Shah <jolly.shah@xxxxxxxxxx>, Arm <arm@xxxxxxxxxx>,
> > Linux-clk <linux-clk@xxxxxxxxxxxxxxx>, Michal Simek
> > <michal.simek@xxxxxxxxxx>, Mturquette <mturquette@xxxxxxxxxxxx>, Olof
> > <olof@xxxxxxxxx>
> >   > Cc: Rajan Vaja <rajanv@xxxxxxxxxx>,
> > Linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, Linux-kernel@xxxxxxxxxxxxxxx
> > <linux-kernel@xxxxxxxxxxxxxxx>, Tejas Patel <tejas.patel@xxxxxxxxxx>,
> > Rajan Vaja <rajan.vaja@xxxxxxxxxx>, Jolly Shah <jolly.shah@xxxxxxxxxx>
> >   > Subject: Re: [PATCH v2 2/2] drivers: clk: zynqmp: Update fraction
> > clock check from custom type flags
> >   >
> > > Quoting Jolly Shah (2020-03-12 14:31:39)
> > > > From: Tejas Patel <tejas.patel@xxxxxxxxxx>
> > > >
> > > > Older firmware version sets BIT(13) in clkflag to mark a
> > > > divider as fractional divider. Updated firmware version sets BIT(4)
> > > > in type flags to mark a divider as fractional divider since
> > > > BIT(13) is defined as CLK_DUTY_CYCLE_PARENT in the common clk
> > > > framework flags.
> > > >
> > > > To support both old and new firmware version, consider BIT(13) from
> > > > clkflag and BIT(4) from type_flag to check if divider is fractional
> > > > or not.
> > > >
> > > > To maintain compatibility BIT(13) of clkflag in firmware will not be
> > > > used in future for any purpose and will be marked as unused.
> > >
> > > Why are we mixing the firmware flags with the ccf flags? They shouldn't
> > > be the same. The firmware should have its own 'flag numberspace' that is
> > > distinct from the common clk framework's 'flag numberspace'. Please fix
> > > the code.
> >
> > Yes firmware flags are using separate numberspace now. Firmware
> > maintains CCF and firmware specific flags separately but earlier
> > CLK_FRAC was mistakenly defined in ccf flagspace and hence handled here
> > for backward compatibility. Driver takes care of not registering same
> > with CCF. Let me know if I misunderstood.
>
> Why is the firmware maintaining CCF specific flags? The firmware
> shouldn't know about the CCF flag numbering at all. We can change the
> numbers that the CCF flags are assigned to randomly and that shouldn't
> mean that the firmware needs to change. Maybe I should apply this patch?

Got it. Will fix it.

Thanks,
Jolly Shah


> ---8<----
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index bd1ee9039558..c1f36bca85b0 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -16,22 +16,22 @@
>    *
>    * Please update clk_flags[] in drivers/clk/clk.c when making changes here!
>    */
> -#define CLK_SET_RATE_GATE	BIT(0) /* must be gated across rate change */
> -#define CLK_SET_PARENT_GATE	BIT(1) /* must be gated across re-parent */
> -#define CLK_SET_RATE_PARENT	BIT(2) /* propagate rate change up one level */
> -#define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
> +#define CLK_SET_RATE_GATE	BIT(13) /* must be gated across rate change */
> +#define CLK_SET_PARENT_GATE	BIT(2) /* must be gated across re-parent */
> +#define CLK_SET_RATE_PARENT	BIT(3) /* propagate rate change up one level */
> +#define CLK_IGNORE_UNUSED	BIT(4) /* do not gate even if unused */
>   				/* unused */
>   				/* unused */
> -#define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
> -#define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
> -#define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
> -#define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
> -#define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
> -#define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
> +#define CLK_GET_RATE_NOCACHE	BIT(5) /* do not use the cached clk rate */
> +#define CLK_SET_RATE_NO_REPARENT BIT(6) /* don't re-parent on rate change */
> +#define CLK_GET_ACCURACY_NOCACHE BIT(7) /* do not use the cached clk accuracy */
> +#define CLK_RECALC_NEW_RATES	BIT(8) /* recalc rates after notifications */
> +#define CLK_SET_RATE_UNGATE	BIT(9) /* clock needs to run to set rate */
> +#define CLK_IS_CRITICAL		BIT(10) /* do not gate, ever */
>   /* parents need enable during gate/ungate, set rate and re-parent */
> -#define CLK_OPS_PARENT_ENABLE	BIT(12)
> +#define CLK_OPS_PARENT_ENABLE	BIT(11)
>   /* duty cycle call may be forwarded to the parent clock */
> -#define CLK_DUTY_CYCLE_PARENT	BIT(13)
> +#define CLK_DUTY_CYCLE_PARENT	BIT(12)
>   
>   struct clk;
>   struct clk_hw;