Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2

[Joel Stanley]

On Mon, 2021-01-18 at 18:08 +0800, Ryan Chen wrote:
> AST2600A1/A2 have different pll calculate formula.

To clarify, only the A0 has the old calculation, and all subsequent
revisions use the new calculation?

If this is the case, do we need to support A0 in mainline linux, or
should we drop support for A0 and only support A1, A2 and onwards?

You should add a line to indicate this is a fix:

Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC")

Also, when sending single patches you do not need to include the cover
letter. You should include all of the relevant information in the
patch's commit message.

> 
> Signed-off-by: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/clk-ast2600.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index bbacaccad554..8933bd1506b3 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -17,7 +17,8 @@
>  
>  #define ASPEED_G6_NUM_CLKS             71
>  
> -#define ASPEED_G6_SILICON_REV          0x004
> +#define ASPEED_G6_SILICON_REV          0x014
> +#define CHIP_REVISION_ID                       GENMASK(23, 16)
>  
>  #define ASPEED_G6_RESET_CTRL           0x040
>  #define ASPEED_G6_RESET_CTRL2          0x050
> @@ -190,18 +191,34 @@ static struct clk_hw *ast2600_calc_pll(const
> char *name, u32 val)
>  static struct clk_hw *ast2600_calc_apll(const char *name, u32 val)
>  {
>         unsigned int mult, div;
> +       u32 chip_id = readl(scu_g6_base + ASPEED_G6_SILICON_REV);
>  
> -       if (val & BIT(20)) {
> -               /* Pass through mode */
> -               mult = div = 1;
> +       if (((chip_id & CHIP_REVISION_ID) >> 16) >= 2) {
> +               if (val & BIT(24)) {
> +                       /* Pass through mode */
> +                       mult = div = 1;
> +               } else {
> +                       /* F = 25Mhz * [(m + 1) / (n + 1)] / (p + 1)
> */
> +                       u32 m = val & 0x1fff;
> +                       u32 n = (val >> 13) & 0x3f;
> +                       u32 p = (val >> 19) & 0xf;
> +
> +                       mult = (m + 1);
> +                       div = (n + 1) * (p + 1);
> +               }
>         } else {
> -               /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)] */
> -               u32 m = (val >> 5) & 0x3f;
> -               u32 od = (val >> 4) & 0x1;
> -               u32 n = val & 0xf;
> +               if (val & BIT(20)) {
> +                       /* Pass through mode */
> +                       mult = div = 1;
> +               } else {
> +                       /* F = 25Mhz * (2-od) * [(m + 2) / (n + 1)]
> */
> +                       u32 m = (val >> 5) & 0x3f;
> +                       u32 od = (val >> 4) & 0x1;
> +                       u32 n = val & 0xf;
>  
> -               mult = (2 - od) * (m + 2);
> -               div = n + 1;
> +                       mult = (2 - od) * (m + 2);
> +                       div = n + 1;
> +               }
>         }
>         return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
>                         mult, div);