| Message ID | 20190329153325.23869-2-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: meson: fix mpll jitter | expand |
Hi Jerome, On Fri, Mar 29, 2019 at 4:34 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > The bit 'SSEN' available on some MPLL DSS outputs is not related to the > fractional part of the divider but to the function called > 'Spread Spectrum'. > > This function might be used to solve EM issues by adding a jitter on > clock signal. This widens the signal spectrum and weakens the peaks in it. > > While spread spectrum might be useful for some application, it is > problematic for others, such as audio. > > This patch introduce a new flag to the MPLL driver to enable (or not) the > spread spectrum function. > > Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/clk/meson/clk-mpll.c | 9 ++++++--- > drivers/clk/meson/clk-mpll.h | 1 + > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > index 64d31c8ba3d0..2d39a8bc367c 100644 > --- a/drivers/clk/meson/clk-mpll.c > +++ b/drivers/clk/meson/clk-mpll.c > @@ -141,9 +141,12 @@ static void mpll_init(struct clk_hw *hw) > /* Enable the fractional part */ > meson_parm_write(clk->map, &mpll->sdm_en, 1); > > - /* Set additional fractional part enable if required */ > - if (MESON_PARM_APPLICABLE(&mpll->ssen)) > - meson_parm_write(clk->map, &mpll->ssen, 1); > + /* Set spread spectrum if possible */ > + if (MESON_PARM_APPLICABLE(&mpll->ssen)) { > + unsigned int ss = > + mpll->flags & CLK_MESON_MPLL_SPREAD_SPECTRUM ? 1 : 0; > + meson_parm_write(clk->map, &mpll->ssen, ss); > + } this changes the "ssen" flag on all supported clocks from 1 (before this patch) to 0 (after this patch). is this on purpose and how does it affect existing clocks? based on the original commit 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") it seems that CLK_MESON_MPLL_SPREAD_SPECTRUM should be set for mpll0 (at least on GXBB and Meson8b) Martin
On Fri, 2019-03-29 at 20:39 +0100, Martin Blumenstingl wrote: > Hi Jerome, > > On Fri, Mar 29, 2019 at 4:34 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > The bit 'SSEN' available on some MPLL DSS outputs is not related to the > > fractional part of the divider but to the function called > > 'Spread Spectrum'. > > > > This function might be used to solve EM issues by adding a jitter on > > clock signal. This widens the signal spectrum and weakens the peaks in it. > > > > While spread spectrum might be useful for some application, it is > > problematic for others, such as audio. > > > > This patch introduce a new flag to the MPLL driver to enable (or not) the > > spread spectrum function. > > > > Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > drivers/clk/meson/clk-mpll.c | 9 ++++++--- > > drivers/clk/meson/clk-mpll.h | 1 + > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > > index 64d31c8ba3d0..2d39a8bc367c 100644 > > --- a/drivers/clk/meson/clk-mpll.c > > +++ b/drivers/clk/meson/clk-mpll.c > > @@ -141,9 +141,12 @@ static void mpll_init(struct clk_hw *hw) > > /* Enable the fractional part */ > > meson_parm_write(clk->map, &mpll->sdm_en, 1); > > > > - /* Set additional fractional part enable if required */ > > - if (MESON_PARM_APPLICABLE(&mpll->ssen)) > > - meson_parm_write(clk->map, &mpll->ssen, 1); > > + /* Set spread spectrum if possible */ > > + if (MESON_PARM_APPLICABLE(&mpll->ssen)) { > > + unsigned int ss = > > + mpll->flags & CLK_MESON_MPLL_SPREAD_SPECTRUM ? 1 : 0; > > + meson_parm_write(clk->map, &mpll->ssen, ss); > > + } > this changes the "ssen" flag on all supported clocks from 1 (before > this patch) to 0 (after this patch). > is this on purpose and how does it affect existing clocks? Yes, none of our application require spread spectrum The fact is that only 2 MPLL had this bit, mpll0 on gx (without effect) and mpll0 on axg: actually spread spectrum impacts mpll2, making it unusable, as explained in the related patch > > based on the original commit 1f737ffa13ef ("clk: meson: mpll: fix > mpll0 fractional part ignored") it seems that > CLK_MESON_MPLL_SPREAD_SPECTRUM should be set for mpll0 (at least on > GXBB and Meson8b) > There a patch specifically targeting gxbb. I have checked on GXL and this bit had no effect (fractional part still on, no spread spectrum) So either we fixed something since then or I messed up when doing the patch initially. Feel free to cross check > > Martin
Hi Jerome, On Sat, Mar 30, 2019 at 12:07 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Fri, 2019-03-29 at 20:39 +0100, Martin Blumenstingl wrote: > > Hi Jerome, > > > > On Fri, Mar 29, 2019 at 4:34 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > The bit 'SSEN' available on some MPLL DSS outputs is not related to the > > > fractional part of the divider but to the function called > > > 'Spread Spectrum'. > > > > > > This function might be used to solve EM issues by adding a jitter on > > > clock signal. This widens the signal spectrum and weakens the peaks in it. > > > > > > While spread spectrum might be useful for some application, it is > > > problematic for others, such as audio. > > > > > > This patch introduce a new flag to the MPLL driver to enable (or not) the > > > spread spectrum function. > > > > > > Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > --- > > > drivers/clk/meson/clk-mpll.c | 9 ++++++--- > > > drivers/clk/meson/clk-mpll.h | 1 + > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c > > > index 64d31c8ba3d0..2d39a8bc367c 100644 > > > --- a/drivers/clk/meson/clk-mpll.c > > > +++ b/drivers/clk/meson/clk-mpll.c > > > @@ -141,9 +141,12 @@ static void mpll_init(struct clk_hw *hw) > > > /* Enable the fractional part */ > > > meson_parm_write(clk->map, &mpll->sdm_en, 1); > > > > > > - /* Set additional fractional part enable if required */ > > > - if (MESON_PARM_APPLICABLE(&mpll->ssen)) > > > - meson_parm_write(clk->map, &mpll->ssen, 1); > > > + /* Set spread spectrum if possible */ > > > + if (MESON_PARM_APPLICABLE(&mpll->ssen)) { > > > + unsigned int ss = > > > + mpll->flags & CLK_MESON_MPLL_SPREAD_SPECTRUM ? 1 : 0; > > > + meson_parm_write(clk->map, &mpll->ssen, ss); > > > + } > > this changes the "ssen" flag on all supported clocks from 1 (before > > this patch) to 0 (after this patch). > > is this on purpose and how does it affect existing clocks? > > Yes, none of our application require spread spectrum thank you for the explanation. can you please add it to the patch description? > The fact is that only 2 MPLL had this bit, mpll0 on gx (without effect) and > mpll0 on axg: actually spread spectrum impacts mpll2, making it unusable, as > explained in the related patch > > > > > based on the original commit 1f737ffa13ef ("clk: meson: mpll: fix > > mpll0 fractional part ignored") it seems that > > CLK_MESON_MPLL_SPREAD_SPECTRUM should be set for mpll0 (at least on > > GXBB and Meson8b) > > > > There a patch specifically targeting gxbb. I missed that, sorry for the noise > I have checked on GXL and this bit had no effect (fractional part still on, no > spread spectrum) > > So either we fixed something since then or I messed up when doing the patch > initially. > > Feel free to cross check I tried it on Meson8b and it seems that there's no difference. I've done a bit of software archaeology: $ grep -R mpll_cntl uboot-2015-01-15-23a3562521/ | grep "=" | head -n3 uboot-2015-01-15-23a3562521/board/amlogic/m8_k100_1G/firmware/timming.c: .mpll_cntl = 0x600009A9, //2.5G, fixed uboot-2015-01-15-23a3562521/board/amlogic/m8_k100_2G/firmware/timming.c: .mpll_cntl = 0x600009A9, //2.5G, fixed uboot-2015-01-15-23a3562521/board/amlogic/m8b_ft_v1/firmware/timming.c: .mpll_cntl = 0x600009A9, //2.5G, fixed Ethernet is still working on my Odroid-C1, so you can add my: Tested-by: Martin Blumenstingl<martin.blumenstingl@googlemail.com> Regards Martin
On Sat, 2019-03-30 at 16:56 +0100, Martin Blumenstingl wrote: > Hi Jerome, > > On Sat, Mar 30, 2019 at 12:07 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Fri, 2019-03-29 at 20:39 +0100, Martin Blumenstingl wrote: > > > Hi Jerome, > > > > > > On Fri, Mar 29, 2019 at 4:34 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > > The bit 'SSEN' available on some MPLL DSS outputs is not related to the > > > > fractional part of the divider but to the function called > > > > 'Spread Spectrum'. > > > > > > > > This function might be used to solve EM issues by adding a jitter on > > > > clock signal. This widens the signal spectrum and weakens the peaks in it. > > > > > > > > While spread spectrum might be useful for some application, it is > > > > problematic for others, such as audio. > > > > > > > > This patch introduce a new flag to the MPLL driver to enable (or not) the > > > > spread spectrum function. > > > > > > > > Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > > --- > > > > drivers/clk/meson/clk-mpll.c | 9 ++++++--- > > > > drivers/clk/meson/clk-mpll.h | 1 + > > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > [...] > > > > Yes, none of our application require spread spectrum > thank you for the explanation. can you please add it to the patch description? I think I described spread spectrum already in the commit description. > > > The fact is that only 2 MPLL had this bit, mpll0 on gx (without effect) and > > mpll0 on axg: actually spread spectrum impacts mpll2, making it unusable, as > > explained in the related patch > > > > > based on the original commit 1f737ffa13ef ("clk: meson: mpll: fix > > > mpll0 fractional part ignored") it seems that > > > CLK_MESON_MPLL_SPREAD_SPECTRUM should be set for mpll0 (at least on > > > GXBB and Meson8b) > > > > > > > There a patch specifically targeting gxbb. > I missed that, sorry for the noise > > > I have checked on GXL and this bit had no effect (fractional part still on, no > > spread spectrum) > > > > So either we fixed something since then or I messed up when doing the patch > > initially. > > > > Feel free to cross check > I tried it on Meson8b and it seems that there's no difference. I've > done a bit of software archaeology: > $ grep -R mpll_cntl uboot-2015-01-15-23a3562521/ | grep "=" | head -n3 > uboot-2015-01-15-23a3562521/board/amlogic/m8_k100_1G/firmware/timming.c: > .mpll_cntl = 0x600009A9, //2.5G, fixed > uboot-2015-01-15-23a3562521/board/amlogic/m8_k100_2G/firmware/timming.c: > .mpll_cntl = 0x600009A9, //2.5G, fixed > uboot-2015-01-15-23a3562521/board/amlogic/m8b_ft_v1/firmware/timming.c: > .mpll_cntl = 0x600009A9, //2.5G, fixed > > Ethernet is still working on my Odroid-C1, so you can add my: > Tested-by: Martin Blumenstingl<martin.blumenstingl@googlemail.com> > > > Regards > Martin
Hi Jerome, On Mon, Apr 1, 2019 at 10:40 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Sat, 2019-03-30 at 16:56 +0100, Martin Blumenstingl wrote: > > Hi Jerome, > > > > On Sat, Mar 30, 2019 at 12:07 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Fri, 2019-03-29 at 20:39 +0100, Martin Blumenstingl wrote: > > > > Hi Jerome, > > > > > > > > On Fri, Mar 29, 2019 at 4:34 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > > > The bit 'SSEN' available on some MPLL DSS outputs is not related to the > > > > > fractional part of the divider but to the function called > > > > > 'Spread Spectrum'. > > > > > > > > > > This function might be used to solve EM issues by adding a jitter on > > > > > clock signal. This widens the signal spectrum and weakens the peaks in it. > > > > > > > > > > While spread spectrum might be useful for some application, it is > > > > > problematic for others, such as audio. > > > > > > > > > > This patch introduce a new flag to the MPLL driver to enable (or not) the > > > > > spread spectrum function. > > > > > > > > > > Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") > > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > > > --- > > > > > drivers/clk/meson/clk-mpll.c | 9 ++++++--- > > > > > drivers/clk/meson/clk-mpll.h | 1 + > > > > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > [...] > > > > > > > Yes, none of our application require spread spectrum > > thank you for the explanation. can you please add it to the patch description? > > I think I described spread spectrum already in the commit description. I haven't been clear about this one - let me do it better this time: if you have to re-send the patches can you please add a note stating that "none of our application require spread spectrum" to the description so it's clear that you're disabling spread spectrum on purpose. your explanation about spread spectrum itself is perfectly fine Regards Martin
diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c index 64d31c8ba3d0..2d39a8bc367c 100644 --- a/drivers/clk/meson/clk-mpll.c +++ b/drivers/clk/meson/clk-mpll.c @@ -141,9 +141,12 @@ static void mpll_init(struct clk_hw *hw) /* Enable the fractional part */ meson_parm_write(clk->map, &mpll->sdm_en, 1); - /* Set additional fractional part enable if required */ - if (MESON_PARM_APPLICABLE(&mpll->ssen)) - meson_parm_write(clk->map, &mpll->ssen, 1); + /* Set spread spectrum if possible */ + if (MESON_PARM_APPLICABLE(&mpll->ssen)) { + unsigned int ss = + mpll->flags & CLK_MESON_MPLL_SPREAD_SPECTRUM ? 1 : 0; + meson_parm_write(clk->map, &mpll->ssen, ss); + } /* Set the magic misc bit if required */ if (MESON_PARM_APPLICABLE(&mpll->misc)) diff --git a/drivers/clk/meson/clk-mpll.h b/drivers/clk/meson/clk-mpll.h index 2925fb939fdd..a991d568c43a 100644 --- a/drivers/clk/meson/clk-mpll.h +++ b/drivers/clk/meson/clk-mpll.h @@ -25,6 +25,7 @@ struct meson_clk_mpll_data { }; #define CLK_MESON_MPLL_ROUND_CLOSEST BIT(0) +#define CLK_MESON_MPLL_SPREAD_SPECTRUM BIT(1) extern const struct clk_ops meson_clk_mpll_ro_ops; extern const struct clk_ops meson_clk_mpll_ops;
The bit 'SSEN' available on some MPLL DSS outputs is not related to the fractional part of the divider but to the function called 'Spread Spectrum'. This function might be used to solve EM issues by adding a jitter on clock signal. This widens the signal spectrum and weakens the peaks in it. While spread spectrum might be useful for some application, it is problematic for others, such as audio. This patch introduce a new flag to the MPLL driver to enable (or not) the spread spectrum function. Fixes: 1f737ffa13ef ("clk: meson: mpll: fix mpll0 fractional part ignored") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/meson/clk-mpll.c | 9 ++++++--- drivers/clk/meson/clk-mpll.h | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) -- 2.20.1