| Message ID | 20170503185625.10297-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > to 288 MHz because the initial frequency "seems too high to be ssp clock > source directly". However this isn't enough to ensure that the frequency > isn't increased later again. In fact this happens on my machine as the > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > ssp2 which is resolved to > > ref_io1.rate = 320 MHz > ssp2_sel.parent = ref_io1 > ssp2_div.divider = 2 > > . The observed effect is that reading MISO reliably fails: Instead of > the least significant bit the second least significant bit is reported > twice. This is probably related to the reports > > https://community.nxp.com/thread/290209 > https://community.nxp.com/thread/310434 Adding Stefan on Cc in he could help testing this patch, as he was the one that created this thread. > https://community.nxp.com/thread/385546 > > So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure > that their frequency is never set to something bigger later on. This is > done by adding a parameter min_frac to clk-ref and use that instead of > the hard coded 18 to limit the valid values for FRAC. For all ref clocks > but ref_io0 and ref_io1 18 is used and so there is no functional change > for those. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > I'd expect this to go in as a fix as it repairs reading from an EEPROM which > should be a fairly common task. > > It would be nice to get confirmation from NXP that this is a right fix and > that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz. > > Best regards > Uwe > > drivers/clk/mxs/clk-imx23.c | 8 ++++---- > drivers/clk/mxs/clk-imx28.c | 14 +++++++------- > drivers/clk/mxs/clk-ref.c | 18 ++++++++++++------ > drivers/clk/mxs/clk.h | 2 +- > 4 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c > index f01876af6bb8..b7d3c44bbcbb 100644 > --- a/drivers/clk/mxs/clk-imx23.c > +++ b/drivers/clk/mxs/clk-imx23.c > @@ -117,10 +117,10 @@ static void __init mx23_clocks_init(struct device_node *np) > > clks[ref_xtal] = mxs_clk_fixed("ref_xtal", 24000000); > clks[pll] = mxs_clk_pll("pll", "ref_xtal", PLLCTRL0, 16, 480000000); > - clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0); > - clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1); > - clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2); > - clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3); > + clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0, 18); > + clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1, 18); > + clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2, 18); > + clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3, 18); > clks[saif_sel] = mxs_clk_mux("saif_sel", CLKSEQ, 0, 1, sel_pll, ARRAY_SIZE(sel_pll)); > clks[lcdif_sel] = mxs_clk_mux("lcdif_sel", CLKSEQ, 1, 1, sel_pix, ARRAY_SIZE(sel_pix)); > clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 4, 1, sel_io, ARRAY_SIZE(sel_io)); > diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c > index 6b572b759f9a..09e324cbd32f 100644 > --- a/drivers/clk/mxs/clk-imx28.c > +++ b/drivers/clk/mxs/clk-imx28.c > @@ -174,13 +174,13 @@ static void __init mx28_clocks_init(struct device_node *np) > clks[pll0] = mxs_clk_pll("pll0", "ref_xtal", PLL0CTRL0, 17, 480000000); > clks[pll1] = mxs_clk_pll("pll1", "ref_xtal", PLL1CTRL0, 17, 480000000); > clks[pll2] = mxs_clk_pll("pll2", "ref_xtal", PLL2CTRL0, 23, 50000000); > - clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0); > - clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1); > - clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2); > - clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3); > - clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0); > - clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1); > - clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2); > + clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0, 18); > + clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1, 18); > + clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2, 30); > + clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3, 30); > + clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0, 18); > + clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1, 18); > + clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2, 18); > clks[saif0_sel] = mxs_clk_mux("saif0_sel", CLKSEQ, 0, 1, sel_pll0, ARRAY_SIZE(sel_pll0)); > clks[saif1_sel] = mxs_clk_mux("saif1_sel", CLKSEQ, 1, 1, sel_pll0, ARRAY_SIZE(sel_pll0)); > clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 2, 1, sel_gpmi, ARRAY_SIZE(sel_gpmi)); > diff --git a/drivers/clk/mxs/clk-ref.c b/drivers/clk/mxs/clk-ref.c > index 495f99b7965e..9385563b3ea8 100644 > --- a/drivers/clk/mxs/clk-ref.c > +++ b/drivers/clk/mxs/clk-ref.c > @@ -23,13 +23,17 @@ > * > * The mxs reference clock sources from pll. Every 4 reference clocks share > * one register space, and @idx is used to identify them. Each reference > - * clock has a gate control and a fractional * divider. The rate is calculated > + * clock has a gate control and a fractional divider. The rate is calculated > * as pll rate * (18 / FRAC), where FRAC = 18 ~ 35. > + * > + * As some clocks are not reliable at high speed (imx28 io0 and io1), the clock > + * can be limited to not use FRAC < min_frac. > */ > struct clk_ref { > struct clk_hw hw; > void __iomem *reg; > u8 idx; > + u8 min_frac; > }; > > #define to_clk_ref(_hw) container_of(_hw, struct clk_ref, hw) > @@ -66,6 +70,7 @@ static unsigned long clk_ref_recalc_rate(struct clk_hw *hw, > static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate, > unsigned long *prate) > { > + struct clk_ref *ref = to_clk_ref(hw); > unsigned long parent_rate = *prate; > u64 tmp = parent_rate; > u8 frac; > @@ -74,8 +79,8 @@ static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate, > do_div(tmp, rate); > frac = tmp; > > - if (frac < 18) > - frac = 18; > + if (frac < ref->min_frac) > + frac = ref->min_frac; > else if (frac > 35) > frac = 35; > > @@ -99,8 +104,8 @@ static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate, > do_div(tmp, rate); > frac = tmp; > > - if (frac < 18) > - frac = 18; > + if (frac < ref->min_frac) > + frac = ref->min_frac; > else if (frac > 35) > frac = 35; > > @@ -125,7 +130,7 @@ static const struct clk_ops clk_ref_ops = { > }; > > struct clk *mxs_clk_ref(const char *name, const char *parent_name, > - void __iomem *reg, u8 idx) > + void __iomem *reg, u8 idx, u8 min_frac) > { > struct clk_ref *ref; > struct clk *clk; > @@ -143,6 +148,7 @@ struct clk *mxs_clk_ref(const char *name, const char *parent_name, > > ref->reg = reg; > ref->idx = idx; > + ref->min_frac = min_frac; > ref->hw.init = &init; > > clk = clk_register(NULL, &ref->hw); > diff --git a/drivers/clk/mxs/clk.h b/drivers/clk/mxs/clk.h > index 5a264a486ad9..fb63462a78cd 100644 > --- a/drivers/clk/mxs/clk.h > +++ b/drivers/clk/mxs/clk.h > @@ -28,7 +28,7 @@ struct clk *mxs_clk_pll(const char *name, const char *parent_name, > void __iomem *base, u8 power, unsigned long rate); > > struct clk *mxs_clk_ref(const char *name, const char *parent_name, > - void __iomem *reg, u8 idx); > + void __iomem *reg, u8 idx, u8 min_frac); > > struct clk *mxs_clk_div(const char *name, const char *parent_name, > void __iomem *reg, u8 shift, u8 width, u8 busy); > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Am 03.05.2017 um 21:53 schrieb Fabio Estevam: > On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and >> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for >> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized >> to 288 MHz because the initial frequency "seems too high to be ssp clock >> source directly". However this isn't enough to ensure that the frequency >> isn't increased later again. In fact this happens on my machine as the >> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being >> ssp2 which is resolved to >> >> ref_io1.rate = 320 MHz >> ssp2_sel.parent = ref_io1 >> ssp2_div.divider = 2 >> >> . The observed effect is that reading MISO reliably fails: Instead of >> the least significant bit the second least significant bit is reported >> twice. This is probably related to the reports >> >> https://community.nxp.com/thread/290209 >> https://community.nxp.com/thread/310434 > Adding Stefan on Cc in he could help testing this patch, as he was the > one that created this thread. thanks for forwarding. I tested the patch with Linux 4.11 it on our Duckbill (i.MX28), but it didn't fix the SPI bit errors with the QCA7000. Currently the only way to get the QCA7000 working is to switch the clock parent of ssp2 to ref_xtal (24 MHz). Is there someone at NXP/Freescale who could explain this behavior? Are there any relevant register values which could be helpful? I guess it's a special ratio between ref_io and ssp_div which causes the issue. Here are the clk_summary for Linux 4.11 on Duckbill with QCA7000 on ssp 2 for all 3 cases: 1) unpatched (= always bit errors on ssp2) clock enable_cnt prepare_cnt rate accuracy phase ---------------------------------------------------------------------------------------- ref_xtal 5 5 24000000 0 0 can1 0 0 24000000 0 0 can0 0 0 24000000 0 0 uart 2 3 24000000 0 0 pwm 0 0 24000000 0 0 rtc 0 0 31250 0 0 clk32k_div 1 1 32000 0 0 clk32k 1 1 32000 0 0 lradc 1 1 2000 0 0 emi_xtal 0 0 24000000 0 0 xbus 3 3 24000000 0 0 cpu_xtal 0 0 24000000 0 0 ptp_sel 0 0 24000000 0 0 ptp 0 0 24000000 0 0 lcdif_sel 0 0 24000000 0 0 lcdif_div 0 0 24000000 0 0 lcdif 0 0 24000000 0 0 etm_sel 0 0 24000000 0 0 etm_div 0 0 24000000 0 0 etm 0 0 24000000 0 0 gpmi_sel 0 0 24000000 0 0 gpmi_div 0 0 24000000 0 0 gpmi 0 0 24000000 0 0 pll2 1 1 50000000 0 0 enet_out 1 1 50000000 0 0 pll1 0 0 480000000 0 0 usb1_phy 0 0 480000000 0 0 usb1 0 0 480000000 0 0 pll0 5 5 480000000 0 0 usb0_phy 2 2 480000000 0 0 usb0 1 1 480000000 0 0 spdif_div 0 0 120000000 0 0 spdif 0 0 120000000 0 0 saif1_sel 0 0 480000000 0 0 saif1_div 0 0 7324 0 0 saif1 0 0 7324 0 0 saif0_sel 0 0 480000000 0 0 saif0_div 0 0 7324 0 0 saif0 0 0 7324 0 0 ref_gpmi 0 0 480000000 0 0 ref_hsadc 0 0 480000000 0 0 ref_pix 0 0 480000000 0 0 ref_io0 1 1 288000000 0 0 ssp1_sel 0 0 288000000 0 0 ssp1_div 0 0 288000000 0 0 ssp1 0 0 288000000 0 0 ssp0_sel 1 1 288000000 0 0 ssp0_div 1 1 57600000 0 0 ssp0 1 1 57600000 0 0 ref_io1 1 1 320000000 0 0 ssp3_sel 0 0 320000000 0 0 ssp3_div 0 0 320000000 0 0 ssp3 0 0 320000000 0 0 ssp2_sel 1 1 320000000 0 0 ssp2_div 1 1 160000000 0 0 ssp2 1 1 160000000 0 0 ref_emi 1 1 411428571 0 0 emi_pll 1 1 205714286 0 0 emi_sel 1 1 205714286 0 0 emi 1 1 205714286 0 0 ref_cpu 1 1 454736842 0 0 cpu_pll 1 1 454736842 0 0 cpu 2 2 454736842 0 0 hbus 4 4 151578948 0 0 fec 2 2 151578948 0 0 2) ref_xtal as clock parent of ssp2 (= no bit errors on ssp2) clock enable_cnt prepare_cnt rate accuracy phase ---------------------------------------------------------------------------------------- ref_xtal 6 6 24000000 0 0 can1 0 0 24000000 0 0 can0 0 0 24000000 0 0 uart 2 3 24000000 0 0 pwm 0 0 24000000 0 0 rtc 0 0 31250 0 0 clk32k_div 1 1 32000 0 0 clk32k 1 1 32000 0 0 lradc 1 1 2000 0 0 emi_xtal 0 0 24000000 0 0 xbus 3 3 24000000 0 0 cpu_xtal 0 0 24000000 0 0 ptp_sel 0 0 24000000 0 0 ptp 0 0 24000000 0 0 lcdif_sel 0 0 24000000 0 0 lcdif_div 0 0 24000000 0 0 lcdif 0 0 24000000 0 0 etm_sel 0 0 24000000 0 0 etm_div 0 0 24000000 0 0 etm 0 0 24000000 0 0 ssp2_sel 1 1 24000000 0 0 ssp2_div 1 1 24000000 0 0 ssp2 1 1 24000000 0 0 gpmi_sel 0 0 24000000 0 0 gpmi_div 0 0 24000000 0 0 gpmi 0 0 24000000 0 0 pll2 1 1 50000000 0 0 enet_out 1 1 50000000 0 0 pll1 0 0 480000000 0 0 usb1_phy 0 0 480000000 0 0 usb1 0 0 480000000 0 0 pll0 4 4 480000000 0 0 usb0_phy 2 2 480000000 0 0 usb0 1 1 480000000 0 0 spdif_div 0 0 120000000 0 0 spdif 0 0 120000000 0 0 saif1_sel 0 0 480000000 0 0 saif1_div 0 0 7324 0 0 saif1 0 0 7324 0 0 saif0_sel 0 0 480000000 0 0 saif0_div 0 0 7324 0 0 saif0 0 0 7324 0 0 ref_gpmi 0 0 480000000 0 0 ref_hsadc 0 0 480000000 0 0 ref_pix 0 0 480000000 0 0 ref_io0 1 1 288000000 0 0 ssp1_sel 0 0 288000000 0 0 ssp1_div 0 0 288000000 0 0 ssp1 0 0 288000000 0 0 ssp0_sel 1 1 288000000 0 0 ssp0_div 1 1 57600000 0 0 ssp0 1 1 57600000 0 0 ref_io1 0 0 288000000 0 0 ssp3_sel 0 0 288000000 0 0 ssp3_div 0 0 288000000 0 0 ssp3 0 0 288000000 0 0 ref_emi 1 1 411428571 0 0 emi_pll 1 1 205714286 0 0 emi_sel 1 1 205714286 0 0 emi 1 1 205714286 0 0 ref_cpu 1 1 454736842 0 0 cpu_pll 1 1 454736842 0 0 cpu 2 2 454736842 0 0 hbus 4 4 151578948 0 0 fec 2 2 151578948 0 0 3) limit ref_io to 288 MHz (= always bit errors on ssp2) clock enable_cnt prepare_cnt rate accuracy phase ---------------------------------------------------------------------------------------- ref_xtal 5 5 24000000 0 0 can1 0 0 24000000 0 0 can0 0 0 24000000 0 0 uart 2 3 24000000 0 0 pwm 0 0 24000000 0 0 rtc 0 0 31250 0 0 clk32k_div 1 1 32000 0 0 clk32k 1 1 32000 0 0 lradc 1 1 2000 0 0 emi_xtal 0 0 24000000 0 0 xbus 3 3 24000000 0 0 cpu_xtal 0 0 24000000 0 0 ptp_sel 0 0 24000000 0 0 ptp 0 0 24000000 0 0 lcdif_sel 0 0 24000000 0 0 lcdif_div 0 0 24000000 0 0 lcdif 0 0 24000000 0 0 etm_sel 0 0 24000000 0 0 etm_div 0 0 24000000 0 0 etm 0 0 24000000 0 0 gpmi_sel 0 0 24000000 0 0 gpmi_div 0 0 24000000 0 0 gpmi 0 0 24000000 0 0 pll2 1 1 50000000 0 0 enet_out 1 1 50000000 0 0 pll1 0 0 480000000 0 0 usb1_phy 0 0 480000000 0 0 usb1 0 0 480000000 0 0 pll0 5 5 480000000 0 0 usb0_phy 2 2 480000000 0 0 usb0 1 1 480000000 0 0 spdif_div 0 0 120000000 0 0 spdif 0 0 120000000 0 0 saif1_sel 0 0 480000000 0 0 saif1_div 0 0 7324 0 0 saif1 0 0 7324 0 0 saif0_sel 0 0 480000000 0 0 saif0_div 0 0 7324 0 0 saif0 0 0 7324 0 0 ref_gpmi 0 0 480000000 0 0 ref_hsadc 0 0 480000000 0 0 ref_pix 0 0 480000000 0 0 ref_io0 1 1 288000000 0 0 ssp1_sel 0 0 288000000 0 0 ssp1_div 0 0 288000000 0 0 ssp1 0 0 288000000 0 0 ssp0_sel 1 1 288000000 0 0 ssp0_div 1 1 57600000 0 0 ssp0 1 1 57600000 0 0 ref_io1 1 1 288000000 0 0 ssp3_sel 0 0 288000000 0 0 ssp3_div 0 0 288000000 0 0 ssp3 0 0 288000000 0 0 ssp2_sel 1 1 288000000 0 0 ssp2_div 1 1 144000000 0 0 ssp2 1 1 144000000 0 0 ref_emi 1 1 411428571 0 0 emi_pll 1 1 205714286 0 0 emi_sel 1 1 205714286 0 0 emi 1 1 205714286 0 0 ref_cpu 1 1 454736842 0 0 cpu_pll 1 1 454736842 0 0 cpu 2 2 454736842 0 0 hbus 4 4 151578948 0 0 fec 2 2 151578948 0 0 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stefan, On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote: > ref_io1 1 1 > 320000000 0 0 That means your ref_io1 is running at 320 MHz which my patch should prevent. Are you sure it is applied? What is the register FRAC0 (0x800401b0) set to? Given that ref_io1 is reported as running at 320000000 I'd expect that IO1FRAC is set to 27 which should be impossible with my patch given that clk_misc_init() in drivers/clk/mxs/clk-imx28.c initializes IO1FRAC0 to 30 and my patch should prevent it being set to something smaller than 30. Something I just noticed now is that the description of FRAC0 includes: NOTE: This register can only be addressed by byte instructions. Addressing word or half-word are not allowed. which is not respected by the clock driver. Best regards Uwe
Hello Uwe, Am 05.05.2017 um 09:25 schrieb Uwe Kleine-König: > Hello Stefan, > > On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote: >> ref_io1 1 1 >> 320000000 0 0 > That means your ref_io1 is running at 320 MHz which my patch should > prevent. Are you sure it is applied? sorry for the mess in the dumps. Your patch works fine regarding to ref_io1. I made 3 dumps: 1. without any patch ( ref_io1 320000000 ) 2. with my ref_xtal patch ( ref_io1 288000000 ) 3. with your patch ( ref_io1 288000000 ) I assume you looked at the wrong dump. > What is the register FRAC0 > (0x800401b0) set to? Given that ref_io1 is reported as running at > 320000000 I'd expect that IO1FRAC is set to 27 which should be > impossible with my patch given that clk_misc_init() in > drivers/clk/mxs/clk-imx28.c initializes IO1FRAC0 to 30 and my patch > should prevent it being set to something smaller than 30. > > Something I just noticed now is that the description of FRAC0 includes: > > NOTE: This register can only be addressed by byte instructions. > Addressing word or half-word are not allowed. > > which is not respected by the clock driver. I already tried it with no luck. Please follow the discussions [1] and [2]. Maybe the reason why [1] didn't work, was the missing memory barriers? Please don't get me wrong, i'm very interested in fixing this issue. Regards Stefan [1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318644.html [2] - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323720.html > > Best regards > Uwe > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 05.05.2017 um 09:49 schrieb Stefan Wahren: > Hello Uwe, > > Am 05.05.2017 um 09:25 schrieb Uwe Kleine-König: >> Hello Stefan, >> >> On Thu, May 04, 2017 at 02:25:07PM +0200, Stefan Wahren wrote: >>> ref_io1 1 1 >>> 320000000 0 0 >> That means your ref_io1 is running at 320 MHz which my patch should >> prevent. Are you sure it is applied? > sorry for the mess in the dumps. > > Your patch works fine regarding to ref_io1. I made 3 dumps: > > 1. without any patch ( ref_io1 320000000 ) > 2. with my ref_xtal patch ( ref_io1 288000000 ) > 3. with your patch ( ref_io1 288000000 ) > > I assume you looked at the wrong dump. > Uwe and i made some investigations regards to this issue. The busy bit of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the case. I have the suspicion that there is an issue during clock init or in the mxs clock driver. Duckbill (Linux 4.11 Mainline) HW_SSP2_TIMING 0x80014070 00000209 HW_CLKCTRL_HBUS 0x80040060 00000003 HW_CLKCTRL_SSP0 0x80040090 00000005 HW_CLKCTRL_SSP1 0x800400A0 80000001 HW_CLKCTRL_SSP2 0x800400B0 00000002 HW_CLKCTRL_SSP3 0x800400C0 A0000001 HW_CLKCTRL_EMI 0x800400F0 00000102 HW_CLKCTRL_FRAC0 0x800401B0 5E5B5513 HW_CLKCTRL_CLKSEQ 0x800401D0 00004104 @Uwe: Additionaly applying "spi: mxs: implement runtime pm" to this patch also doesn't fix the bit error. Regards Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, (adding Michael Krummsdorf who authored a different patch for the same problem back in 2014 which Stefan found in TQ's patch stack.) On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote: > Uwe and i made some investigations regards to this issue. The busy bit > of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the > case. I have the suspicion that there is an issue during clock init or > in the mxs clock driver. > > Duckbill (Linux 4.11 Mainline) > > HW_SSP2_TIMING 0x80014070 00000209 > HW_CLKCTRL_HBUS 0x80040060 00000003 > HW_CLKCTRL_SSP0 0x80040090 00000005 > HW_CLKCTRL_SSP1 0x800400A0 80000001 > HW_CLKCTRL_SSP2 0x800400B0 00000002 > HW_CLKCTRL_SSP3 0x800400C0 A0000001 > HW_CLKCTRL_EMI 0x800400F0 00000102 > HW_CLKCTRL_FRAC0 0x800401B0 5E5B5513 > HW_CLKCTRL_CLKSEQ 0x800401D0 00004104 Using barebox as bootloader I have this situation there: / md 0x800400c0+4; md 0x800400c0+4 800400c0: a0000005 .... 800400c0: a0000005 .... so I suspect the BUSY bit is set when the bootloader is started as (TTBOMK) barebox didn't touch ssp up to this point. The description of the busy bit is: This read-only bit field returns a one when the clock divider is busy transfering a new divider value across clock domains. That sounds to me as if this bit shouldn't be set for longer than a few microseconds?! For me my patch makes the following difference (just checked the above register set) when Linux is booted: name | address | value before patch | value with patch ------------------+------------+--------------------+----------------- HW_SSP2_TIMING | 0x80014070 | 0x00000209 | 0x00000208 HW_CLKCTRL_HBUS | 0x80040060 | 0x00000003 | 0x00000003 HW_CLKCTRL_SSP0 | 0x80040090 | 0x00000005 | 0x00000005 HW_CLKCTRL_SSP1 | 0x800400a0 | 0x00000005 | 0x00000005 HW_CLKCTRL_SSP2 | 0x800400b0 | 0x80000002 | 0x80000002 HW_CLKCTRL_SSP3 | 0x800400c0 | 0x80000005 | 0xa0000005 HW_CLKCTRL_EMI | 0x800400f0 | 0x00000102 | 0x00000102 HW_CLKCTRL_FRAC0 | 0x800401b0 | 0x1e9b5513 | 0x5ede5513 HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104 | 0x00000104 So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to 480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?! Something changed for CLKCTRL_SSP3 (BUSY) even though ssp3 isn't used on my machine. I also tried to reproduce BUSY disappearing by doing the same changes to io1 in barebox that Linux does, but without success. The change to HW_SSP2_TIMING compensates the decreased io1 speed. (Changing the clock rate from 320 MHz / (2 * 10) = 16 MHz (which is the target rate setup by the spi-mxs driver) to 288 MHz / (2 * 9) = 16 MHz . So it's not actually changed.) > @Uwe: Additionaly applying "spi: mxs: implement runtime pm" to this > patch also doesn't fix the bit error. ok. Would have been nice if that was the relevant difference between your machine that still has the problem with my patch and mine where the bug is fixed (or papered over) by my patch. I wonder if this part of drivers/clk/mxs/clk-imx28.c: /* * 480 MHz seems too high to be ssp clock source directly, * so set frac0 to get a 288 MHz ref_io0 and ref_io1. */ val = readl_relaxed(FRAC0); val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC)); val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC); writel_relaxed(val, FRAC0); is related to this "lowest bit is wrong"-problem and is the result from a wrong and/or incomplete analysis. It would be nice if someone from NXP could comment as this code was introduced (for ref_io0 only) in 7d81397cd93d ("clk: mxs: add clock support for imx28") by Shawn when he was still employee at Freescale. Best regards Uwe
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> hat am 5. Mai 2017 um 22:10 geschrieben: > > > Hello, > > (adding Michael Krummsdorf who authored a different patch for the same > problem back in 2014 which Stefan found in TQ's patch stack.) > > On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote: > > Uwe and i made some investigations regards to this issue. The busy bit > > of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the > > case. I have the suspicion that there is an issue during clock init or > > in the mxs clock driver. > > > > Duckbill (Linux 4.11 Mainline) > > > > HW_SSP2_TIMING 0x80014070 00000209 > > HW_CLKCTRL_HBUS 0x80040060 00000003 > > HW_CLKCTRL_SSP0 0x80040090 00000005 > > HW_CLKCTRL_SSP1 0x800400A0 80000001 > > HW_CLKCTRL_SSP2 0x800400B0 00000002 > > HW_CLKCTRL_SSP3 0x800400C0 A0000001 > > HW_CLKCTRL_EMI 0x800400F0 00000102 > > HW_CLKCTRL_FRAC0 0x800401B0 5E5B5513 > > HW_CLKCTRL_CLKSEQ 0x800401D0 00004104 > > Using barebox as bootloader I have this situation there: > > / md 0x800400c0+4; md 0x800400c0+4 > 800400c0: a0000005 .... > 800400c0: a0000005 .... > > so I suspect the BUSY bit is set when the bootloader is started as > (TTBOMK) barebox didn't touch ssp up to this point. The description of > the busy bit is: > > This read-only bit field returns a one when the clock divider is > busy transfering a new divider value across clock domains. > > That sounds to me as if this bit shouldn't be set for longer than a few > microseconds?! This is expected behavior according to the hardware. As long the clock is gated the busy bit will never be cleared. Unfortunately the mxs clk-div doesn't handle this case properly. In case the gate is set, clk-div changes the values and waits that the busy bit become cleared. Since this will never happen, the set_rate operation runs into an unnecessary timeout. I added a trace_printk into mxs_clk_wait and saw a timeout for changes on ssp3_div. This should be triggered by the clock change of spi-mxs on ssp2_div, because both have the same parent: ref_io1. In order to handle this properly the gate shouldn't be a child of clk-div. > > For me my patch makes the following difference (just checked the above > register set) when Linux is booted: > > name | address | value before patch | value with patch > ------------------+------------+--------------------+----------------- > HW_SSP2_TIMING | 0x80014070 | 0x00000209 | 0x00000208 > HW_CLKCTRL_HBUS | 0x80040060 | 0x00000003 | 0x00000003 > HW_CLKCTRL_SSP0 | 0x80040090 | 0x00000005 | 0x00000005 > HW_CLKCTRL_SSP1 | 0x800400a0 | 0x00000005 | 0x00000005 > HW_CLKCTRL_SSP2 | 0x800400b0 | 0x80000002 | 0x80000002 > HW_CLKCTRL_SSP3 | 0x800400c0 | 0x80000005 | 0xa0000005 > HW_CLKCTRL_EMI | 0x800400f0 | 0x00000102 | 0x00000102 > HW_CLKCTRL_FRAC0 | 0x800401b0 | 0x1e9b5513 | 0x5ede5513 > HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104 | 0x00000104 > > So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to > 480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other > changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?! > I have no idea because those flags only toggle on success. > > I wonder if this part of drivers/clk/mxs/clk-imx28.c: > > /* > * 480 MHz seems too high to be ssp clock source directly, > * so set frac0 to get a 288 MHz ref_io0 and ref_io1. > */ > val = readl_relaxed(FRAC0); > val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC)); > val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC); > writel_relaxed(val, FRAC0); > > is related to this "lowest bit is wrong"-problem and is the result from > a wrong and/or incomplete analysis. I think there are 2 possible issues with this init code. First we try to set IO0FRAC and IO1FRAC at the same time. The other one is the order. The parent of all 4 SSP clocks are switched to ref_io, before the FRAC0 register is set up to 288 MHz. This shouldn't be a real problem as long as the bootloader init FRAC0 before. Nevertheless i prepare a proof of concept (hacky) patch to address all 3 possible issues [1]: - swap FRAC0 setup and CLKSEQ bypass changes for all SSPs - split IO0FRAC and IO1FRAC changes into 2 separate R-M-W sequences incl. memory barrier - avoid clock changes to sspX_div if they are gated or busy - additionaly trace_printks I tested it with Duckbill, but without a QCA7000. So i don't know if it fixes the bit errors with the QCA7000. Uwe, could you please test it with your EEPROM but without your 2 clock patches? Does it have any influence on the bit errors? [1] - https://gist.github.com/lategoodbye/4db0ad08d1c23dfac51115d206ecbd7a -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stefan, On Sun, May 07, 2017 at 01:51:54PM +0200, Stefan Wahren wrote: > > On Fri, May 05, 2017 at 05:49:09PM +0200, Stefan Wahren wrote: > > > Uwe and i made some investigations regards to this issue. The busy bit > > > of the HW_CLKCTRL_SSP3 was high after boot up, but this shouldn't be the > > > case. I have the suspicion that there is an issue during clock init or > > > in the mxs clock driver. > > > > > > Duckbill (Linux 4.11 Mainline) > > > > > > HW_SSP2_TIMING 0x80014070 00000209 > > > HW_CLKCTRL_HBUS 0x80040060 00000003 > > > HW_CLKCTRL_SSP0 0x80040090 00000005 > > > HW_CLKCTRL_SSP1 0x800400A0 80000001 > > > HW_CLKCTRL_SSP2 0x800400B0 00000002 > > > HW_CLKCTRL_SSP3 0x800400C0 A0000001 > > > HW_CLKCTRL_EMI 0x800400F0 00000102 > > > HW_CLKCTRL_FRAC0 0x800401B0 5E5B5513 > > > HW_CLKCTRL_CLKSEQ 0x800401D0 00004104 > > > > Using barebox as bootloader I have this situation there: > > > > / md 0x800400c0+4; md 0x800400c0+4 > > 800400c0: a0000005 .... > > 800400c0: a0000005 .... > > > > so I suspect the BUSY bit is set when the bootloader is started as > > (TTBOMK) barebox didn't touch ssp up to this point. The description of > > the busy bit is: > > > > This read-only bit field returns a one when the clock divider is > > busy transfering a new divider value across clock domains. > > > > That sounds to me as if this bit shouldn't be set for longer than a few > > microseconds?! > > This is expected behavior according to the hardware. As long the clock is gated the busy bit will never be cleared. Unfortunately the mxs clk-div doesn't handle this case properly. In case the gate is set, clk-div changes the values and waits that the busy bit become cleared. Since this will never happen, the set_rate operation runs into an unnecessary timeout. > > I added a trace_printk into mxs_clk_wait and saw a timeout for changes on ssp3_div. This should be triggered by the clock change of spi-mxs on ssp2_div, because both have the same parent: ref_io1. In order to handle this properly the gate shouldn't be a child of clk-div. > > > > > For me my patch makes the following difference (just checked the above > > register set) when Linux is booted: > > > > name | address | value before patch | value with patch > > ------------------+------------+--------------------+----------------- > > HW_SSP2_TIMING | 0x80014070 | 0x00000209 | 0x00000208 > > HW_CLKCTRL_HBUS | 0x80040060 | 0x00000003 | 0x00000003 > > HW_CLKCTRL_SSP0 | 0x80040090 | 0x00000005 | 0x00000005 > > HW_CLKCTRL_SSP1 | 0x800400a0 | 0x00000005 | 0x00000005 > > HW_CLKCTRL_SSP2 | 0x800400b0 | 0x80000002 | 0x80000002 > > HW_CLKCTRL_SSP3 | 0x800400c0 | 0x80000005 | 0xa0000005 > > HW_CLKCTRL_EMI | 0x800400f0 | 0x00000102 | 0x00000102 > > HW_CLKCTRL_FRAC0 | 0x800401b0 | 0x1e9b5513 | 0x5ede5513 > > HW_CLKCTRL_CLKSEQ | 0x800401d0 | 0x00000104 | 0x00000104 > > > > So ref_io1 changed from 480 * (18 / 27) MHz = 320 MHz to > > 480 * (18 / 30) MHz = 288 MHz (as intended) (FRAC0[16:21]). The other > > changes to FRAC0 (IO1_STABLE, IO0_STABLE) don't look relevant?! > > > > I have no idea because those flags only toggle on success. > > > > > I wonder if this part of drivers/clk/mxs/clk-imx28.c: > > > > /* > > * 480 MHz seems too high to be ssp clock source directly, > > * so set frac0 to get a 288 MHz ref_io0 and ref_io1. > > */ > > val = readl_relaxed(FRAC0); > > val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC)); > > val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC); > > writel_relaxed(val, FRAC0); > > > > is related to this "lowest bit is wrong"-problem and is the result from > > a wrong and/or incomplete analysis. > > I think there are 2 possible issues with this init code. First we try to set IO0FRAC and IO1FRAC at the same time. > > The other one is the order. The parent of all 4 SSP clocks are switched to ref_io, before the FRAC0 register is set up to 288 MHz. This shouldn't be a real problem as long as the bootloader init FRAC0 before. > > Nevertheless i prepare a proof of concept (hacky) patch to address all 3 possible issues [1]: > - swap FRAC0 setup and CLKSEQ bypass changes for all SSPs > - split IO0FRAC and IO1FRAC changes into 2 separate R-M-W sequences incl. memory barrier > - avoid clock changes to sspX_div if they are gated or busy > - additionaly trace_printks > > I tested it with Duckbill, but without a QCA7000. So i don't know if it fixes the bit errors with the QCA7000. > > Uwe, could you please test it with your EEPROM but without your 2 clock patches? > Does it have any influence on the bit errors? The patch doesn't make the bit errors go away. The only message being generated is: swapper-1 [000] ....... 1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated When I compare your clk_summary (with my patch) to my working one the only differences are some enable/prepare counts, you have the following clocks on while I have not: lradc, enet_out, lcdif_sel (also reparented), fec while I have these clocks on and you have not: ssp1 There are no differences in clk freqs for clocks that we both have on which should rule out a problem of competing clock domains. I'm out of ideas :-( Best regards Uwe
Hello Uwe, > (adding Michael Krummsdorf who authored a different patch for the same > problem back in 2014 which Stefan found in TQ's patch stack.) thanks for adding me. Back then I narrowed the issue down to two conditions: * ref_io had to be clock source and * ssp clock div had to be 1 Limiting ssp clock div to be at least 2 eliminated the error. So in the driver I capped clk_freq to 480 MHz / 2. I did not consider changing ref_io frequency. Best regards, i.A. Michael Krummsdorf Entwicklung Standort Leipzig Tel. +49 341 308598-27, Fax +49 341 308598-11, Friedrich-List-Platz 2, 04103 Leipzig mailto:michael.krummsdorf@tq-group.com TQ-Systems GmbH Mühlstraße 2, Gut Delling, 82229 Seefeld Amtsgericht München, HRB 105018 Sitz der Gesellschaft: Seefeld Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider www.tq-group.com Besuchen Sie uns: Intersolar Europe, München, 31. Mai - 2. Juni 2017, Halle B2, Stand B2.234 MT-CONNECT, Nürnberg, 21. - 22. Juni 2017, Halle 10.1, Stand 10.1-116 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, May 08, 2017 at 09:53:28AM +0000, Krummsdorf Michael wrote: > > (adding Michael Krummsdorf who authored a different patch for the same > > problem back in 2014 which Stefan found in TQ's patch stack.) > > thanks for adding me. > Back then I narrowed the issue down to two conditions: > * ref_io had to be clock source and > * ssp clock div had to be 1 This is not the complete problem description because my patch lowers the speed on ref_io and has no influence on HW_CLKCTRL_SSP2 (which has already DIV=2 for me before the patch where it's broken). Also Stefan has values >1 and the problem is still there on a duckbill. Best regards Uwe
Am 08.05.2017 um 10:24 schrieb Uwe Kleine-König: > Hello Stefan, > > The patch doesn't make the bit errors go away. The only message being > generated is: > > swapper-1 [000] ....... 1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated > > When I compare your clk_summary (with my patch) to my working one the > only differences are some enable/prepare counts, you have the following > clocks on while I have not: > > lradc, enet_out, lcdif_sel (also reparented), fec > > while I have these clocks on and you have not: > > ssp1 Sorry, i don't have the time for any further investigations. Out of curiosity, what is connected to the ssp1 on your board? Stefan > > There are no differences in clk freqs for clocks that we both have on > which should rule out a problem of competing clock domains. > > I'm out of ideas :-( > > Best regards > Uwe > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stefan, On Wed, May 10, 2017 at 03:39:14PM +0200, Stefan Wahren wrote: > Am 08.05.2017 um 10:24 schrieb Uwe Kleine-König: > > The patch doesn't make the bit errors go away. The only message being > > generated is: > > > > swapper-1 [000] ....... 1.134985: clk_div_set_rate: Cannot to set ssp3_div: gated > > > > When I compare your clk_summary (with my patch) to my working one the > > only differences are some enable/prepare counts, you have the following > > clocks on while I have not: > > > > lradc, enet_out, lcdif_sel (also reparented), fec > > > > while I have these clocks on and you have not: > > > > ssp1 ssp0: SD-card ssp1: eMMC (which /) ssp2: eeprom Uwe
Hello, I found something strange while playing around with the HW_CLKCTRL_FRAC0 register: # memtool md 0x800401b0+4 800401b0: 0e1d5513 .U.. # memtool mw 0x800401b0 0x0e1d5513 # memtool md 0x800401b0+4 800401b0: 4e5d5513 .U]N Huh, why did IO0_STABLE and IO1_STABLE toggle? OK, after all we should access this register using 8-bit accesses: # memtool md -b 0x800401b0+4 800401b0: 13 55 5d 4e .U]N # memtool mw -b 0x800401b2 0x5d # memtool md -b 0x800401b0+4 800401b0: 13 55 5d 4e .U]N hmm, now IO1_STABLE didn't invert. # memtool mw -b 0x800401b2 0x5c # memtool md -b 0x800401b0+4 800401b0: 13 55 5c 4e .U\N not even now even though IO1FRAC changed! # memtool md 0x800401b0+4 800401b0: 4e5c5513 .U\N # memtool mw -b 0x800401b2 0x5b # memtool md 0x800401b0+4 800401b0: 4e5b5513 .U[N again IO1_STABLE doesn't toggle (this time with 32-bit access) # memtool mw 0x800401b0 0x4e5b5513 # memtool md 0x800401b0+4 800401b0: 0e1b5513 .U.. Just rewriting the reported value makes both IO0_STABLE and IO1_STABLE toggle. Definitely strange. Best regards Uwe
Hello, # cat dumpclk #!/bin/sh for r in 0x80014070 0x80040060 0x80040090 0x800400A0 0x800400B0 0x800400C0 0x800400F0 0x800401B0 0x800401D0; do memtool md $r+4 done # cat dumpeeprom #!/bin/sh dd if=/sys/bus/spi/devices/spi1.0/eeprom bs=16 count=1 2>/dev/null | hexdump -C Now when booting with an unpatched 4.9 kernel I have after startup: # ./dumpclk 80014070: 00000204 .... 80040060: 00000003 .... 80040090: 00000005 .... 800400a0: 00000005 .... 800400b0: 00000002 .... 800400c0: 80000005 .... 800400f0: 00000102 .... 800401b0: 5e5b5513 .U[^ 800401d0: 00000104 .... # ./dumpeeprom 00000000 08 ab 00 00 5f 70 00 00 00 80 00 03 00 00 cb 00 |...._p..........| 00000010 (which is broken, because the first byte in the eeprom is a 0x09). Now I make ref_io slower again: # memtool mw 0x800401b0 0x5e5e5513 # ./dumpclk 80014070: 00000204 .... 80040060: 00000003 .... 80040090: 00000005 .... 800400a0: 00000005 .... 800400b0: 00000002 .... 800400c0: 80000005 .... 800400f0: 00000102 .... 800401b0: 1e1e5513 .U^. 800401d0: 00000104 .... # ./dumpeeprom 00000000 09 aa 00 00 5f 70 00 00 00 80 00 02 00 00 ca 00 |...._p..........| 00000010 So on my machine it really seems to only depend on ref-io1 clk speed. With all available slower speeds resulting from IOFRAC1 in [0x1f .. 0x23] accessing the eeprom works fine. With IOFRAC = 0x1d reading works fine already. With all lower values [0x12 .. 0x1c] reading shows these bit errors. Also more or less randomly jumping between different settings for IO1FRAC doesn't suggest that there might be something else influencing the bug. It was always [0x12 .. 0x1c] shows the bug, [0x1d .. 0x23] doesn't. I also did a test using different values for SSP2_TIMING.CLOCK_DIVIDE and SSP2_TIMING.CLOCK_RATE. Going through all 32504 settings that don't yield a bus freq > 16 MHz with ssp2-clk running at 144 MHz or 160 MHz doesn't show any variation. It's always fine on 144 MHz (IOFRAC = 0x1e) and always fails on 160 MHz (IOFRAC = 0x1b). Best regards Uwe
Hello, On Wed, May 03, 2017 at 04:53:09PM -0300, Fabio Estevam wrote: > On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > > to 288 MHz because the initial frequency "seems too high to be ssp clock > > source directly". However this isn't enough to ensure that the frequency > > isn't increased later again. In fact this happens on my machine as the > > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > > ssp2 which is resolved to > > > > ref_io1.rate = 320 MHz > > ssp2_sel.parent = ref_io1 > > ssp2_div.divider = 2 > > > > . The observed effect is that reading MISO reliably fails: Instead of > > the least significant bit the second least significant bit is reported > > twice. This is probably related to the reports > > > > https://community.nxp.com/thread/290209 > > https://community.nxp.com/thread/310434 > > Adding Stefan on Cc in he could help testing this patch, as he was the > one that created this thread. Even though my patch doesn't help Stefan, I think it should be applied nevertheless as it makes my machine work, just completes the workaround/fix that is already in mainline and nobody who still has the issue has the time to debug this further. Best regards Uwe
Hi Uwe, > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> hat am 26. Mai 2017 um 14:06 geschrieben: > > > Hello, > > On Wed, May 03, 2017 at 04:53:09PM -0300, Fabio Estevam wrote: > > On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > > > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > > > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > > > to 288 MHz because the initial frequency "seems too high to be ssp clock > > > source directly". However this isn't enough to ensure that the frequency > > > isn't increased later again. In fact this happens on my machine as the > > > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > > > ssp2 which is resolved to > > > > > > ref_io1.rate = 320 MHz > > > ssp2_sel.parent = ref_io1 > > > ssp2_div.divider = 2 > > > > > > . The observed effect is that reading MISO reliably fails: Instead of > > > the least significant bit the second least significant bit is reported > > > twice. This is probably related to the reports > > > > > > https://community.nxp.com/thread/290209 > > > https://community.nxp.com/thread/310434 > > > > Adding Stefan on Cc in he could help testing this patch, as he was the > > one that created this thread. > > Even though my patch doesn't help Stefan, I think it should be applied > nevertheless as it makes my machine work, just completes the > workaround/fix that is already in mainline and nobody who still has the > issue has the time to debug this further. i like to see at least a comment within the code that the change is a workaround. Thanks Stefan > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > to 288 MHz because the initial frequency "seems too high to be ssp clock > source directly". However this isn't enough to ensure that the frequency > isn't increased later again. In fact this happens on my machine as the > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > ssp2 which is resolved to > > ref_io1.rate = 320 MHz > ssp2_sel.parent = ref_io1 > ssp2_div.divider = 2 > > . The observed effect is that reading MISO reliably fails: Instead of > the least significant bit the second least significant bit is reported > twice. This is probably related to the reports > > https://community.nxp.com/thread/290209 > https://community.nxp.com/thread/310434 As Stefan mentioned that your patch does not solve his issue, please remove this link when you submit a v2. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 29, 2017 at 06:21:49PM -0300, Fabio Estevam wrote: > Hi Uwe, > > On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > > to 288 MHz because the initial frequency "seems too high to be ssp clock > > source directly". However this isn't enough to ensure that the frequency > > isn't increased later again. In fact this happens on my machine as the > > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > > ssp2 which is resolved to > > > > ref_io1.rate = 320 MHz > > ssp2_sel.parent = ref_io1 > > ssp2_div.divider = 2 > > > > . The observed effect is that reading MISO reliably fails: Instead of > > the least significant bit the second least significant bit is reported > > twice. This is probably related to the reports > > > > https://community.nxp.com/thread/290209 > > https://community.nxp.com/thread/310434 > > As Stefan mentioned that your patch does not solve his issue, please > remove this link when you submit a v2. I still think it is related to these reports, don't you? Best regards Uwe
Am 30.05.2017 um 08:54 schrieb Uwe Kleine-König: > On Mon, May 29, 2017 at 06:21:49PM -0300, Fabio Estevam wrote: >> Hi Uwe, >> >> On Wed, May 3, 2017 at 3:56 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >>> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and >>> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for >>> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized >>> to 288 MHz because the initial frequency "seems too high to be ssp clock >>> source directly". However this isn't enough to ensure that the frequency >>> isn't increased later again. In fact this happens on my machine as the >>> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being >>> ssp2 which is resolved to >>> >>> ref_io1.rate = 320 MHz >>> ssp2_sel.parent = ref_io1 >>> ssp2_div.divider = 2 >>> >>> . The observed effect is that reading MISO reliably fails: Instead of >>> the least significant bit the second least significant bit is reported >>> twice. This is probably related to the reports >>> >>> https://community.nxp.com/thread/290209 >>> https://community.nxp.com/thread/310434 >> As Stefan mentioned that your patch does not solve his issue, please >> remove this link when you submit a v2. > I still think it is related to these reports, don't you? I think this is related somehow, but this version doesn't mention that it doesn't fix the linked issue. So removing the link would be the best in order to avoid confusion. Btw did you tried to disable DMA transfer during your tests? Regards Stefan > > Best regards > Uwe > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 30, 2017 at 3:54 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> I still think it is related to these reports, don't you?
Your commit log gives the impression that your patch fixes Stefan's
reported problem, which can be confusing.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote: > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > to 288 MHz because the initial frequency "seems too high to be ssp clock > source directly". However this isn't enough to ensure that the frequency > isn't increased later again. In fact this happens on my machine as the > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > ssp2 which is resolved to > > ref_io1.rate = 320 MHz > ssp2_sel.parent = ref_io1 > ssp2_div.divider = 2 > > . The observed effect is that reading MISO reliably fails: Instead of > the least significant bit the second least significant bit is reported > twice. This is probably related to the reports > > https://community.nxp.com/thread/290209 > https://community.nxp.com/thread/310434 > https://community.nxp.com/thread/385546 > > So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure > that their frequency is never set to something bigger later on. This is > done by adding a parameter min_frac to clk-ref and use that instead of > the hard coded 18 to limit the valid values for FRAC. For all ref clocks > but ref_io0 and ref_io1 18 is used and so there is no functional change > for those. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Back in 2017 this patch made the issue disappear on a customer's machine (using TQ's TQMa28). Now the problem is back on a variant of the machine (using TQMa28L). Back then Stefan had the problem with my patch on a Duckbill, too. Here you can see the problem in action: I hava a file that contains just the bytes from 0 to 256: root@em-switch:~ hexdump -C lala 00000000 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f |................| 00000010 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f |................| 00000020 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f | !"#$%&'()*+,-./| 00000030 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f |0123456789:;<=>?| 00000040 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f |@ABCDEFGHIJKLMNO| 00000050 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f |PQRSTUVWXYZ[\]^_| 00000060 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f |`abcdefghijklmno| 00000070 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f |pqrstuvwxyz{|}~.| 00000080 80 81 82 83 84 85 86 87 88 89 8a 8b 8c 8d 8e 8f |................| 00000090 90 91 92 93 94 95 96 97 98 99 9a 9b 9c 9d 9e 9f |................| 000000a0 a0 a1 a2 a3 a4 a5 a6 a7 a8 a9 aa ab ac ad ae af |................| 000000b0 b0 b1 b2 b3 b4 b5 b6 b7 b8 b9 ba bb bc bd be bf |................| 000000c0 c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 ca cb cc cd ce cf |................| 000000d0 d0 d1 d2 d3 d4 d5 d6 d7 d8 d9 da db dc dd de df |................| 000000e0 e0 e1 e2 e3 e4 e5 e6 e7 e8 e9 ea eb ec ed ee ef |................| 000000f0 f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd fe |...............| 000000ff this was written to an spi eeprom. Reading it looks ok on the oscilloscope, but in the driver I get the described read errors: root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom 3 2 3 15 16 17 18 21 20 23 26 27 34 41 40 35 42 43 38 45 44 46 55 54 54 65 64 87 126 127 143 216 217 199 306 307 203 312 313 211 322 323 231 346 347 239 356 357 243 362 363 251 372 373 cmp: EOF on lala root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom 10 11 10 11 12 13 18 21 20 23 26 27 26 31 30 38 45 44 39 46 47 47 56 57 59 72 73 114 161 160 151 226 227 179 262 263 203 312 313 239 356 357 247 366 367 251 372 373 255 376 377 cmp: EOF on lala root@em-switch:~ cmp -l lala /sys/bus/spi/devices/spi1.0/eeprom 15 16 17 19 22 23 27 32 33 35 42 43 51 62 63 55 66 67 63 76 77 71 106 107 79 116 117 83 122 123 87 126 127 91 132 133 95 136 137 99 142 143 123 172 173 138 211 210 139 212 213 151 226 227 155 232 233 163 242 243 166 245 244 187 272 273 231 346 347 cmp: EOF on lala (The output of cmp -l is for each differing byte: The decimal byte number (starting at 1), and then in octal the two values.) As you can see the problem is on changing offsets and the value in the third column always only differs in the least-significant bit and the wrong values always end in 0, 3, 4 or 7, so the two least-significant bits are always identical. So it seems the last bit (which is the last on the bus, too) is sampled too early by the CPU. Is the problem better understood in the meantime by someone? It would be great to find and fix the underlying issue. I guess only someone with a view into the SoC can give definitive results, though. Fabio: Doesn't that look bad enough to let the hardware guys at NXP look into that? Best regards Uwe
Hi Uwe, no new insights from my side. Am 26.07.2018 um 16:32 schrieb Uwe Kleine-König: > On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote: >> Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and >> 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for >> SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized >> to 288 MHz because the initial frequency "seems too high to be ssp clock >> source directly". However this isn't enough to ensure that the frequency >> isn't increased later again. In fact this happens on my machine as the >> mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being >> ssp2 which is resolved to >> >> ref_io1.rate = 320 MHz >> ssp2_sel.parent = ref_io1 >> ssp2_div.divider = 2 >> >> . The observed effect is that reading MISO reliably fails: Instead of >> the least significant bit the second least significant bit is reported >> twice. This is probably related to the reports >> >> https://community.nxp.com/thread/290209 >> https://community.nxp.com/thread/310434 >> https://community.nxp.com/thread/385546 >> >> So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure >> that their frequency is never set to something bigger later on. This is >> done by adding a parameter min_frac to clk-ref and use that instead of >> the hard coded 18 to limit the valid values for FRAC. For all ref clocks >> but ref_io0 and ref_io1 18 is used and so there is no functional change >> for those. >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Back in 2017 this patch made the issue disappear on a customer's machine > (using TQ's TQMa28). Now the problem is back on a variant of the machine > (using TQMa28L). Back then Stefan had the problem with my patch on a > Duckbill, too. > I only want to note that we are using this ugly hack for Duckbill to avoid this issue: https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 Best regards Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > I only want to note that we are using this ugly hack for Duckbill to avoid > this issue: > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 Do you see the problem on TQMa28L with Stefan's hack applied? -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, On Thu, Jul 26, 2018 at 11:32 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Fabio: Doesn't that look bad enough to let the hardware guys at NXP > look into that? Yes, I think we should ask for their help on this problem. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Uwe, On Thu, Jul 26, 2018 at 12:04 PM, Fabio Estevam <festevam@gmail.com> wrote: > Hi Uwe, > > On Thu, Jul 26, 2018 at 11:32 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > >> Fabio: Doesn't that look bad enough to let the hardware guys at NXP >> look into that? > > Yes, I think we should ask for their help on this problem. We are changing the ref_io while it is already feeding SSP, which is not a good practice. It would be better if we could change the ref_io frequency first, and then feed the SSP clock afterwards: --- a/drivers/clk/mxs/clk-imx28.c +++ b/drivers/clk/mxs/clk-imx28.c @@ -110,12 +110,6 @@ static void __init clk_misc_init(void) writel_relaxed(val, ENET); /* - * Source ssp clock from ref_io than ref_xtal, - * as ref_xtal only provides 24 MHz as maximum. - */ - writel_relaxed(0xf << BP_CLKSEQ_BYPASS_SSP0, CLKSEQ + CLR); - - /* * 480 MHz seems too high to be ssp clock source directly, * so set frac0 to get a 288 MHz ref_io0 and ref_io1. */ @@ -123,6 +117,12 @@ static void __init clk_misc_init(void) val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC)); val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC); writel_relaxed(val, FRAC0); + + /* + * Source ssp clock from ref_io than ref_xtal, + * as ref_xtal only provides 24 MHz as maximum. + */ + writel_relaxed(0xf << BP_CLKSEQ_BYPASS_SSP0, CLKSEQ + CLR); } -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Fabio, On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote: > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > I only want to note that we are using this ugly hack for Duckbill to avoid > > this issue: > > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 > > Do you see the problem on TQMa28L with Stefan's hack applied? With this change by Stefan the bug is not reproducible. Your change to set the bypass bits only later doesn't help though. Best regards Uwe
Hello, On Wed, Aug 01, 2018 at 11:31:13AM +0200, Uwe Kleine-König wrote: > On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote: > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > > I only want to note that we are using this ugly hack for Duckbill to avoid > > > this issue: > > > > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 > > > > Do you see the problem on TQMa28L with Stefan's hack applied? > > With this change by Stefan the bug is not reproducible. Your change to > set the bypass bits only later doesn't help though. I wanted to try without DMA (by just removing dem dma properties in the mmc node) but this makes the driver fail to probe :-| Best regards Uwe
On Thu, Aug 02, 2018 at 10:33:18AM +0200, Uwe Kleine-König wrote: > Hello, > > On Wed, Aug 01, 2018 at 11:31:13AM +0200, Uwe Kleine-König wrote: > > On Thu, Jul 26, 2018 at 12:02:42PM -0300, Fabio Estevam wrote: > > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > > > > I only want to note that we are using this ugly hack for Duckbill to avoid > > > > this issue: > > > > > > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 > > > > > > Do you see the problem on TQMa28L with Stefan's hack applied? > > > > With this change by Stefan the bug is not reproducible. Your change to > > set the bypass bits only later doesn't help though. > > I wanted to try without DMA (by just removing dem dma properties in the > mmc node) but this makes the driver fail to probe :-| I tested with diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index 3d216b950b41..36374e71d80a 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -401,7 +401,7 @@ static int mxs_spi_transfer_one(struct spi_master *master, * DMA only: 2.164808 seconds, 473.0KB/s * Combined: 1.676276 seconds, 610.9KB/s */ - if (t->len < 32) { + if (t->len < 32 || 1) { writel(BM_SSP_CTRL1_DMA_ENABLE, ssp->base + HW_SSP_CTRL1(ssp) + STMP_OFFSET_REG_CLR); which effectively disables DMA, the problem is still there. So DMA can be ruled out. Best regards Uwe
Hi, > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > > I only want to note that we are using this ugly hack for Duckbill to avoid > > > this issue: > > > > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 > > > > Do you see the problem on TQMa28L with Stefan's hack applied? > > With this change by Stefan the bug is not reproducible. Your change to > set the bypass bits only later doesn't help though. i can confirm that Fabio's patch doesn't fix the clock issue on Duckbill. But the idea seems to be right. If i remove all write access to CLKSEQ and FRAC0 in clk_misc_init() instead of swap them, i don't need the ref_xtal hack anymore. I think Fabio's patch doesn't work because the bootloader (U-Boot in my case) is already feeding SSP from ref_io. Regards Stefan > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 08, 2018 at 10:23:37AM +0200, Stefan Wahren wrote: > Hi, > > > > On Thu, Jul 26, 2018 at 11:50 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > > > > I only want to note that we are using this ugly hack for Duckbill to avoid > > > > this issue: > > > > > > > > https://github.com/I2SE/linux/commit/e457d5e0309c07e30e8dd8b17c32f7b3cbdd9547 > > > > > > Do you see the problem on TQMa28L with Stefan's hack applied? > > > > With this change by Stefan the bug is not reproducible. Your change to > > set the bypass bits only later doesn't help though. > > i can confirm that Fabio's patch doesn't fix the clock issue on > Duckbill. But the idea seems to be right. If i remove all write access > to CLKSEQ and FRAC0 in clk_misc_init() instead of swap them, i don't > need the ref_xtal hack anymore. I think Fabio's patch doesn't work > because the bootloader (U-Boot in my case) is already feeding SSP from > ref_io. I spend some time with this problem again and found out that it also depends on hclk. If that runs quicker than 2x sspX I couldn't reproduce the issue. When slowing down hclk to 1x sspX the issue reappeared with all stuff related to sspX keeping the same. Probably a problem when the value crosses the clock domain from sspX to hclk?! Best regards Uwe
On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote: > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > to 288 MHz because the initial frequency "seems too high to be ssp clock > source directly". However this isn't enough to ensure that the frequency > isn't increased later again. In fact this happens on my machine as the > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > ssp2 which is resolved to > > ref_io1.rate = 320 MHz > ssp2_sel.parent = ref_io1 > ssp2_div.divider = 2 > > . The observed effect is that reading MISO reliably fails: Instead of > the least significant bit the second least significant bit is reported > twice. This is probably related to the reports > > https://community.nxp.com/thread/290209 > https://community.nxp.com/thread/310434 > https://community.nxp.com/thread/385546 > > So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure > that their frequency is never set to something bigger later on. This is > done by adding a parameter min_frac to clk-ref and use that instead of > the hard coded 18 to limit the valid values for FRAC. For all ref clocks > but ref_io0 and ref_io1 18 is used and so there is no functional change > for those. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > I'd expect this to go in as a fix as it repairs reading from an EEPROM which > should be a fairly common task. > > It would be nice to get confirmation from NXP that this is a right fix and > that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz. I spend some time on this problem once more. I examined a machine that showed no issues with CLKCTRL_FRAC0.IO1FRAC >= 19, but reliably reported more than 32000 read errors with CLKCTRL_FRAC0.IO1FRAC == 18 when dumping an EEPROM that contains 32768 bytes that can show the error. The boring to find insights are, that the error is independent of the other dividers in the clock tree that affect the spi clk frequency and hbus. A more interesting insight is that applying coolant spray nearly halves the read rate (as reported by dd) but also the error rate drops to 0. After some time the machine reheats again and the error rate raises to nearly 100% as before. I just shared that information with NXP's tech support and will keep you updated on their reactions. Best regards Uwe
Hello, On Fri, Mar 22, 2019 at 10:51:32PM +0100, Uwe Kleine-König wrote: > On Wed, May 03, 2017 at 08:56:25PM +0200, Uwe Kleine-König wrote: > > Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and > > 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for > > SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized > > to 288 MHz because the initial frequency "seems too high to be ssp clock > > source directly". However this isn't enough to ensure that the frequency > > isn't increased later again. In fact this happens on my machine as the > > mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being > > ssp2 which is resolved to > > > > ref_io1.rate = 320 MHz > > ssp2_sel.parent = ref_io1 > > ssp2_div.divider = 2 > > > > . The observed effect is that reading MISO reliably fails: Instead of > > the least significant bit the second least significant bit is reported > > twice. This is probably related to the reports > > > > https://community.nxp.com/thread/290209 > > https://community.nxp.com/thread/310434 > > https://community.nxp.com/thread/385546 > > > > So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure > > that their frequency is never set to something bigger later on. This is > > done by adding a parameter min_frac to clk-ref and use that instead of > > the hard coded 18 to limit the valid values for FRAC. For all ref clocks > > but ref_io0 and ref_io1 18 is used and so there is no functional change > > for those. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello, > > > > I'd expect this to go in as a fix as it repairs reading from an EEPROM which > > should be a fairly common task. > > > > It would be nice to get confirmation from NXP that this is a right fix and > > that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz. > > I spend some time on this problem once more. I examined a machine that > showed no issues with CLKCTRL_FRAC0.IO1FRAC >= 19, but reliably reported > more than 32000 read errors with CLKCTRL_FRAC0.IO1FRAC == 18 when > dumping an EEPROM that contains 32768 bytes that can show the error. > > The boring to find insights are, that the error is independent of the > other dividers in the clock tree that affect the spi clk frequency and > hbus. > > A more interesting insight is that applying coolant spray nearly > halves the read rate (as reported by dd) but also the error rate drops > to 0. After some time the machine reheats again and the error rate > raises to nearly 100% as before. > > I just shared that information with NXP's tech support and will keep you > updated on their reactions. The gist of their response is that this CPU is too old and so they don't have much resources to spend time debugging things there any more. The supporter suggested to upgrade to i.MX6 or i.MX7 instead ... So we're living with the work around suggested in this thread and pray it will continue to work for us even tough it doesn't seem to be robust enough to fix the same problem for Stefan. Best regards Uwe
diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c index f01876af6bb8..b7d3c44bbcbb 100644 --- a/drivers/clk/mxs/clk-imx23.c +++ b/drivers/clk/mxs/clk-imx23.c @@ -117,10 +117,10 @@ static void __init mx23_clocks_init(struct device_node *np) clks[ref_xtal] = mxs_clk_fixed("ref_xtal", 24000000); clks[pll] = mxs_clk_pll("pll", "ref_xtal", PLLCTRL0, 16, 480000000); - clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0); - clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1); - clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2); - clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3); + clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll", FRAC, 0, 18); + clks[ref_emi] = mxs_clk_ref("ref_emi", "pll", FRAC, 1, 18); + clks[ref_pix] = mxs_clk_ref("ref_pix", "pll", FRAC, 2, 18); + clks[ref_io] = mxs_clk_ref("ref_io", "pll", FRAC, 3, 18); clks[saif_sel] = mxs_clk_mux("saif_sel", CLKSEQ, 0, 1, sel_pll, ARRAY_SIZE(sel_pll)); clks[lcdif_sel] = mxs_clk_mux("lcdif_sel", CLKSEQ, 1, 1, sel_pix, ARRAY_SIZE(sel_pix)); clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 4, 1, sel_io, ARRAY_SIZE(sel_io)); diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c index 6b572b759f9a..09e324cbd32f 100644 --- a/drivers/clk/mxs/clk-imx28.c +++ b/drivers/clk/mxs/clk-imx28.c @@ -174,13 +174,13 @@ static void __init mx28_clocks_init(struct device_node *np) clks[pll0] = mxs_clk_pll("pll0", "ref_xtal", PLL0CTRL0, 17, 480000000); clks[pll1] = mxs_clk_pll("pll1", "ref_xtal", PLL1CTRL0, 17, 480000000); clks[pll2] = mxs_clk_pll("pll2", "ref_xtal", PLL2CTRL0, 23, 50000000); - clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0); - clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1); - clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2); - clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3); - clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0); - clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1); - clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2); + clks[ref_cpu] = mxs_clk_ref("ref_cpu", "pll0", FRAC0, 0, 18); + clks[ref_emi] = mxs_clk_ref("ref_emi", "pll0", FRAC0, 1, 18); + clks[ref_io1] = mxs_clk_ref("ref_io1", "pll0", FRAC0, 2, 30); + clks[ref_io0] = mxs_clk_ref("ref_io0", "pll0", FRAC0, 3, 30); + clks[ref_pix] = mxs_clk_ref("ref_pix", "pll0", FRAC1, 0, 18); + clks[ref_hsadc] = mxs_clk_ref("ref_hsadc", "pll0", FRAC1, 1, 18); + clks[ref_gpmi] = mxs_clk_ref("ref_gpmi", "pll0", FRAC1, 2, 18); clks[saif0_sel] = mxs_clk_mux("saif0_sel", CLKSEQ, 0, 1, sel_pll0, ARRAY_SIZE(sel_pll0)); clks[saif1_sel] = mxs_clk_mux("saif1_sel", CLKSEQ, 1, 1, sel_pll0, ARRAY_SIZE(sel_pll0)); clks[gpmi_sel] = mxs_clk_mux("gpmi_sel", CLKSEQ, 2, 1, sel_gpmi, ARRAY_SIZE(sel_gpmi)); diff --git a/drivers/clk/mxs/clk-ref.c b/drivers/clk/mxs/clk-ref.c index 495f99b7965e..9385563b3ea8 100644 --- a/drivers/clk/mxs/clk-ref.c +++ b/drivers/clk/mxs/clk-ref.c @@ -23,13 +23,17 @@ * * The mxs reference clock sources from pll. Every 4 reference clocks share * one register space, and @idx is used to identify them. Each reference - * clock has a gate control and a fractional * divider. The rate is calculated + * clock has a gate control and a fractional divider. The rate is calculated * as pll rate * (18 / FRAC), where FRAC = 18 ~ 35. + * + * As some clocks are not reliable at high speed (imx28 io0 and io1), the clock + * can be limited to not use FRAC < min_frac. */ struct clk_ref { struct clk_hw hw; void __iomem *reg; u8 idx; + u8 min_frac; }; #define to_clk_ref(_hw) container_of(_hw, struct clk_ref, hw) @@ -66,6 +70,7 @@ static unsigned long clk_ref_recalc_rate(struct clk_hw *hw, static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *prate) { + struct clk_ref *ref = to_clk_ref(hw); unsigned long parent_rate = *prate; u64 tmp = parent_rate; u8 frac; @@ -74,8 +79,8 @@ static long clk_ref_round_rate(struct clk_hw *hw, unsigned long rate, do_div(tmp, rate); frac = tmp; - if (frac < 18) - frac = 18; + if (frac < ref->min_frac) + frac = ref->min_frac; else if (frac > 35) frac = 35; @@ -99,8 +104,8 @@ static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate, do_div(tmp, rate); frac = tmp; - if (frac < 18) - frac = 18; + if (frac < ref->min_frac) + frac = ref->min_frac; else if (frac > 35) frac = 35; @@ -125,7 +130,7 @@ static const struct clk_ops clk_ref_ops = { }; struct clk *mxs_clk_ref(const char *name, const char *parent_name, - void __iomem *reg, u8 idx) + void __iomem *reg, u8 idx, u8 min_frac) { struct clk_ref *ref; struct clk *clk; @@ -143,6 +148,7 @@ struct clk *mxs_clk_ref(const char *name, const char *parent_name, ref->reg = reg; ref->idx = idx; + ref->min_frac = min_frac; ref->hw.init = &init; clk = clk_register(NULL, &ref->hw); diff --git a/drivers/clk/mxs/clk.h b/drivers/clk/mxs/clk.h index 5a264a486ad9..fb63462a78cd 100644 --- a/drivers/clk/mxs/clk.h +++ b/drivers/clk/mxs/clk.h @@ -28,7 +28,7 @@ struct clk *mxs_clk_pll(const char *name, const char *parent_name, void __iomem *base, u8 power, unsigned long rate); struct clk *mxs_clk_ref(const char *name, const char *parent_name, - void __iomem *reg, u8 idx); + void __iomem *reg, u8 idx, u8 min_frac); struct clk *mxs_clk_div(const char *name, const char *parent_name, void __iomem *reg, u8 shift, u8 width, u8 busy);
Since commits 7d81397cd93d ("clk: mxs: add clock support for imx28") and 64e2bc413047 ("clk: mxs: imx28: decrease the frequency of ref_io1 for SSP2 and SSP3") the frequencies for ref_io0 and ref_io1 are initialized to 288 MHz because the initial frequency "seems too high to be ssp clock source directly". However this isn't enough to ensure that the frequency isn't increased later again. In fact this happens on my machine as the mxs-spi driver calls clk_set_rate(ssp->clk, 160000000) with ssp being ssp2 which is resolved to ref_io1.rate = 320 MHz ssp2_sel.parent = ref_io1 ssp2_div.divider = 2 . The observed effect is that reading MISO reliably fails: Instead of the least significant bit the second least significant bit is reported twice. This is probably related to the reports https://community.nxp.com/thread/290209 https://community.nxp.com/thread/310434 https://community.nxp.com/thread/385546 So additionally to initializing ref_io0 and ref_io1 to 288 MHz ensure that their frequency is never set to something bigger later on. This is done by adding a parameter min_frac to clk-ref and use that instead of the hard coded 18 to limit the valid values for FRAC. For all ref clocks but ref_io0 and ref_io1 18 is used and so there is no functional change for those. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, I'd expect this to go in as a fix as it repairs reading from an EEPROM which should be a fairly common task. It would be nice to get confirmation from NXP that this is a right fix and that spi transfers are supposed to be reliable with ref_ioX <= 288 MHz. Best regards Uwe drivers/clk/mxs/clk-imx23.c | 8 ++++---- drivers/clk/mxs/clk-imx28.c | 14 +++++++------- drivers/clk/mxs/clk-ref.c | 18 ++++++++++++------ drivers/clk/mxs/clk.h | 2 +- 4 files changed, 24 insertions(+), 18 deletions(-)