Message ID | 20240306180435.1033052-1-catalin.popescu@leica-geosystems.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [next] clk: rs9: fix wrong default value for clock amplitude | expand |
Ping :) On 06/03/2024 19:04, Catalin Popescu wrote: > According to 9FGV0241 & 9FGV0441 datasheets, the default value > for the clock amplitude is 0.8V, while the driver was assuming > 0.7V. > > Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> > --- > drivers/clk/clk-renesas-pcie.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c > index 53e21ac302e6..4c3a5e4eb77a 100644 > --- a/drivers/clk/clk-renesas-pcie.c > +++ b/drivers/clk/clk-renesas-pcie.c > @@ -25,10 +25,12 @@ > #define RS9_REG_SS_AMP_0V7 0x1 > #define RS9_REG_SS_AMP_0V8 0x2 > #define RS9_REG_SS_AMP_0V9 0x3 > +#define RS9_REG_SS_AMP_DEFAULT RS9_REG_SS_AMP_0V8 > #define RS9_REG_SS_AMP_MASK 0x3 > #define RS9_REG_SS_SSC_100 0 > #define RS9_REG_SS_SSC_M025 (1 << 3) > #define RS9_REG_SS_SSC_M050 (3 << 3) > +#define RS9_REG_SS_SSC_DEFAULT RS9_REG_SS_SSC_100 > #define RS9_REG_SS_SSC_MASK (3 << 3) > #define RS9_REG_SS_SSC_LOCK BIT(5) > #define RS9_REG_SR 0x2 > @@ -205,8 +207,8 @@ static int rs9_get_common_config(struct rs9_driver_data *rs9) > int ret; > > /* Set defaults */ > - rs9->pll_amplitude = RS9_REG_SS_AMP_0V7; > - rs9->pll_ssc = RS9_REG_SS_SSC_100; > + rs9->pll_amplitude = RS9_REG_SS_AMP_DEFAULT; > + rs9->pll_ssc = RS9_REG_SS_SSC_DEFAULT; > > /* Output clock amplitude */ > ret = of_property_read_u32(np, "renesas,out-amplitude-microvolt", > @@ -247,13 +249,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9) > int i; > > /* If amplitude is non-default, update it. */ > - if (rs9->pll_amplitude != RS9_REG_SS_AMP_0V7) { > + if (rs9->pll_amplitude != RS9_REG_SS_AMP_DEFAULT) { > regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_AMP_MASK, > rs9->pll_amplitude); > } > > /* If SSC is non-default, update it. */ > - if (rs9->pll_ssc != RS9_REG_SS_SSC_100) { > + if (rs9->pll_ssc != RS9_REG_SS_SSC_DEFAULT) { > regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_SSC_MASK, > rs9->pll_ssc); > } > > base-commit: 61996c073c9b070922ad3a36c981ca6ddbea19a5 > prerequisite-patch-id: 0000000000000000000000000000000000000000
Quoting Catalin Popescu (2024-03-06 10:04:35) > According to 9FGV0241 & 9FGV0441 datasheets, the default value > for the clock amplitude is 0.8V, while the driver was assuming > 0.7V. > > Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> > --- Adding folks who seem to know about this than me. -Stephen > drivers/clk/clk-renesas-pcie.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c > index 53e21ac302e6..4c3a5e4eb77a 100644 > --- a/drivers/clk/clk-renesas-pcie.c > +++ b/drivers/clk/clk-renesas-pcie.c > @@ -25,10 +25,12 @@ > #define RS9_REG_SS_AMP_0V7 0x1 > #define RS9_REG_SS_AMP_0V8 0x2 > #define RS9_REG_SS_AMP_0V9 0x3 > +#define RS9_REG_SS_AMP_DEFAULT RS9_REG_SS_AMP_0V8 > #define RS9_REG_SS_AMP_MASK 0x3 > #define RS9_REG_SS_SSC_100 0 > #define RS9_REG_SS_SSC_M025 (1 << 3) > #define RS9_REG_SS_SSC_M050 (3 << 3) > +#define RS9_REG_SS_SSC_DEFAULT RS9_REG_SS_SSC_100 > #define RS9_REG_SS_SSC_MASK (3 << 3) > #define RS9_REG_SS_SSC_LOCK BIT(5) > #define RS9_REG_SR 0x2 > @@ -205,8 +207,8 @@ static int rs9_get_common_config(struct rs9_driver_data *rs9) > int ret; > > /* Set defaults */ > - rs9->pll_amplitude = RS9_REG_SS_AMP_0V7; > - rs9->pll_ssc = RS9_REG_SS_SSC_100; > + rs9->pll_amplitude = RS9_REG_SS_AMP_DEFAULT; > + rs9->pll_ssc = RS9_REG_SS_SSC_DEFAULT; > > /* Output clock amplitude */ > ret = of_property_read_u32(np, "renesas,out-amplitude-microvolt", > @@ -247,13 +249,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9) > int i; > > /* If amplitude is non-default, update it. */ > - if (rs9->pll_amplitude != RS9_REG_SS_AMP_0V7) { > + if (rs9->pll_amplitude != RS9_REG_SS_AMP_DEFAULT) { > regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_AMP_MASK, > rs9->pll_amplitude); > } > > /* If SSC is non-default, update it. */ > - if (rs9->pll_ssc != RS9_REG_SS_SSC_100) { > + if (rs9->pll_ssc != RS9_REG_SS_SSC_DEFAULT) { > regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_SSC_MASK, > rs9->pll_ssc); > } > > base-commit: 61996c073c9b070922ad3a36c981ca6ddbea19a5 > prerequisite-patch-id: 0000000000000000000000000000000000000000 > -- > 2.34.1 >
On 4/9/24 10:19 AM, Stephen Boyd wrote: > Quoting Catalin Popescu (2024-03-06 10:04:35) >> According to 9FGV0241 & 9FGV0441 datasheets 9FGV0841 too. >, the default value >> for the clock amplitude is 0.8V, while the driver was assuming >> 0.7V. Can you also document the SCC spread spectrum change in the commit message ? >> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> This also needs Fixes: 892e0ddea1aa ("clk: rs9: Add Renesas 9-series PCIe clock generator driver") Thanks ! Sorry for the delayed reply.
On 12/04/2024 20:35, Marek Vasut wrote: > This email is not from Hexagon’s Office 365 instance. Please be > careful while clicking links, opening attachments, or replying to this > email. > > > On 4/9/24 10:19 AM, Stephen Boyd wrote: >> Quoting Catalin Popescu (2024-03-06 10:04:35) >>> According to 9FGV0241 & 9FGV0441 datasheets > > 9FGV0841 too. > >> , the default value >>> for the clock amplitude is 0.8V, while the driver was assuming >>> 0.7V. > > Can you also document the SCC spread spectrum change in the commit > message ? > >>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> > > This also needs > Fixes: 892e0ddea1aa ("clk: rs9: Add Renesas 9-series PCIe clock > generator driver") > > Thanks ! Sorry for the delayed reply. No problem, thanks for the review.
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c index 53e21ac302e6..4c3a5e4eb77a 100644 --- a/drivers/clk/clk-renesas-pcie.c +++ b/drivers/clk/clk-renesas-pcie.c @@ -25,10 +25,12 @@ #define RS9_REG_SS_AMP_0V7 0x1 #define RS9_REG_SS_AMP_0V8 0x2 #define RS9_REG_SS_AMP_0V9 0x3 +#define RS9_REG_SS_AMP_DEFAULT RS9_REG_SS_AMP_0V8 #define RS9_REG_SS_AMP_MASK 0x3 #define RS9_REG_SS_SSC_100 0 #define RS9_REG_SS_SSC_M025 (1 << 3) #define RS9_REG_SS_SSC_M050 (3 << 3) +#define RS9_REG_SS_SSC_DEFAULT RS9_REG_SS_SSC_100 #define RS9_REG_SS_SSC_MASK (3 << 3) #define RS9_REG_SS_SSC_LOCK BIT(5) #define RS9_REG_SR 0x2 @@ -205,8 +207,8 @@ static int rs9_get_common_config(struct rs9_driver_data *rs9) int ret; /* Set defaults */ - rs9->pll_amplitude = RS9_REG_SS_AMP_0V7; - rs9->pll_ssc = RS9_REG_SS_SSC_100; + rs9->pll_amplitude = RS9_REG_SS_AMP_DEFAULT; + rs9->pll_ssc = RS9_REG_SS_SSC_DEFAULT; /* Output clock amplitude */ ret = of_property_read_u32(np, "renesas,out-amplitude-microvolt", @@ -247,13 +249,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9) int i; /* If amplitude is non-default, update it. */ - if (rs9->pll_amplitude != RS9_REG_SS_AMP_0V7) { + if (rs9->pll_amplitude != RS9_REG_SS_AMP_DEFAULT) { regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_AMP_MASK, rs9->pll_amplitude); } /* If SSC is non-default, update it. */ - if (rs9->pll_ssc != RS9_REG_SS_SSC_100) { + if (rs9->pll_ssc != RS9_REG_SS_SSC_DEFAULT) { regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_SSC_MASK, rs9->pll_ssc); }
According to 9FGV0241 & 9FGV0441 datasheets, the default value for the clock amplitude is 0.8V, while the driver was assuming 0.7V. Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> --- drivers/clk/clk-renesas-pcie.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) base-commit: 61996c073c9b070922ad3a36c981ca6ddbea19a5 prerequisite-patch-id: 0000000000000000000000000000000000000000