| Message ID | 20210118100813.30821-2-ryan_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix AST2600A2 APLL calculate formuula | expand |
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@aspeedtech.com> > --- > 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);
> -----Original Message----- > From: Joel Stanley <joel@linux.ibm.com> > Sent: Tuesday, January 19, 2021 10:20 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Michael Turquette > <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; > linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; > andrewrj@au1.ibm.com; BMC-SW <BMC-SW@aspeedtech.com> > Cc: joel@jms.id.au; Andrew Jeffery <andrew@aj.id.au> > Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2 > > 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? > A0/A1 is use older calculate formula After A2 is new calculate formula as the patch. > You should add a line to indicate this is a fix: > > Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC") > Got it. so should I send new patch? > 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@aspeedtech.com> > > --- > > 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); >
On Tue, 19 Jan 2021 at 03:04, Ryan Chen <ryan_chen@aspeedtech.com> wrote: > > > -----Original Message----- > > From: Joel Stanley <joel@linux.ibm.com> > > Sent: Tuesday, January 19, 2021 10:20 AM > > To: Ryan Chen <ryan_chen@aspeedtech.com>; Michael Turquette > > <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; > > linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; > > andrewrj@au1.ibm.com; BMC-SW <BMC-SW@aspeedtech.com> > > Cc: joel@jms.id.au; Andrew Jeffery <andrew@aj.id.au> > > Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2 > > > > 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? > > > A0/A1 is use older calculate formula > After A2 is new calculate formula as the patch. Thanks for clarifying. I suggest you change the commit log to say something like this: Starting from A2, the A-PLL calculation has changed. Use the existing formula for A0/A1 and the new formula for A2 onwards. > > > You should add a line to indicate this is a fix: > > > > Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC") > > > Got it. so should I send new patch? Yes, please consider adjusting the commit message as I suggested above, and add the fixes line. > > > + 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) { Will this test be true if there are future versions of the chip (A3, etc)? Cheers, Joel
> -----Original Message----- > From: Joel Stanley <joel@jms.id.au> > Sent: Tuesday, January 19, 2021 11:10 AM > To: Ryan Chen <ryan_chen@aspeedtech.com> > Cc: Joel Stanley <joel@linux.ibm.com>; Michael Turquette > <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; > linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; > andrewrj@au1.ibm.com; BMC-SW <BMC-SW@aspeedtech.com>; Andrew > Jeffery <andrew@aj.id.au> > Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for ast2600-A2 > > On Tue, 19 Jan 2021 at 03:04, Ryan Chen <ryan_chen@aspeedtech.com> > wrote: > > > > > -----Original Message----- > > > From: Joel Stanley <joel@linux.ibm.com> > > > Sent: Tuesday, January 19, 2021 10:20 AM > > > To: Ryan Chen <ryan_chen@aspeedtech.com>; Michael Turquette > > > <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; > > > linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; > > > andrewrj@au1.ibm.com; BMC-SW <BMC-SW@aspeedtech.com> > > > Cc: joel@jms.id.au; Andrew Jeffery <andrew@aj.id.au> > > > Subject: Re: [PATCH 1/1] clk: aspeed: Fix APLL calculate formula for > > > ast2600-A2 > > > > > > 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? > > > > > A0/A1 is use older calculate formula > > After A2 is new calculate formula as the patch. > > Thanks for clarifying. I suggest you change the commit log to say something > like this: > > Starting from A2, the A-PLL calculation has changed. Use the existing formula > for A0/A1 and the new formula for A2 onwards. > > > > > > You should add a line to indicate this is a fix: > > > > > > Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC") > > > > > Got it. so should I send new patch? > > Yes, please consider adjusting the commit message as I suggested above, and > add the fixes line. > > > > > + 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) { > > Will this test be true if there are future versions of the chip (A3, etc)? Yes, is also support for A3.
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);
AST2600A1/A2 have different pll calculate formula. Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> --- drivers/clk/clk-ast2600.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)