| Message ID | 1384651251-5548-1-git-send-email-marex@denx.de |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hi, Marek Vasut wrote: > The same code for enabling and disabling SATA clock was found in multiple > places in the driver. Implement functions that enable/disable the SATA clock > and use them in such places instead of duplicating the code. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Richard Zhu <r65037@freescale.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Linux-IDE <linux-ide@vger.kernel.org> > --- > drivers/ata/ahci_imx.c | 133 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 75 insertions(+), 58 deletions(-) > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > index ae2d73f..c7ee505 100644 > --- a/drivers/ata/ahci_imx.c > +++ b/drivers/ata/ahci_imx.c > @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; > module_param_named(hotplug, ahci_imx_hotplug, int, 0644); > MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, 1=support)"); > > +static int imx_sata_clock_enable(struct device *dev, bool config) > +{ > + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); > + int ret; > + > + imxpriv->gpr = > + syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr"); > + if (IS_ERR(imxpriv->gpr)) { > + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n"); > + return PTR_ERR(imxpriv->gpr); > + } > + > + ret = clk_prepare_enable(imxpriv->sata_ref_clk); > + if (ret < 0) { > + dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret); > + return ret; > + } > + > + /* > + * set PHY Paremeters, two steps to configure the GPR13, > + * one write for rest of parameters, mask of first write > + * is 0x07fffffd, and the other one write for setting > + * the mpll_clk_en. > + */ > + if (config) { > + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > + IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK > + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK > + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK > + | IMX6Q_GPR13_SATA_SPD_MODE_MASK > + | IMX6Q_GPR13_SATA_MPLL_SS_EN > + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK > + | IMX6Q_GPR13_SATA_TX_BOOST_MASK > + | IMX6Q_GPR13_SATA_TX_LVL_MASK > + | IMX6Q_GPR13_SATA_TX_EDGE_RATE > + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB > I would prefer the comma to be at the end of the line where one usually looks for it. Also '#define'ing constants for these bitmasks would make the code more readable. Lothar Waßmann
Hi Marek, On 11/16/2013 06:20 PM, Marek Vasut wrote: > The same code for enabling and disabling SATA clock was found in multiple > places in the driver. Implement functions that enable/disable the SATA clock > and use them in such places instead of duplicating the code. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Richard Zhu <r65037@freescale.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Linux-IDE <linux-ide@vger.kernel.org> > --- > drivers/ata/ahci_imx.c | 133 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 75 insertions(+), 58 deletions(-) > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > index ae2d73f..c7ee505 100644 > --- a/drivers/ata/ahci_imx.c > +++ b/drivers/ata/ahci_imx.c > @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; > module_param_named(hotplug, ahci_imx_hotplug, int, 0644); > MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, 1=support)"); > > <snip> > I haven't traced through all of this, but if you're copying from the Freescale 3.0.35 kernel, note that there's a bug in it, and the 0x7FFFFFFD really wanted to be an 0x7FFFFFFF. The way I read this comment, the writes need to happen in two steps: - write everything with the PHY disabled - enable the PHY We had reports of stalls waiting for SATA drives to be enumerated that were solved with this commit... https://github.com/boundarydevices/linux-imx6/commit/0186ea224ce6bd1cb4757a0f83b0090e26a021f4 > + /* > + * set PHY Paremeters, two steps to configure the GPR13, > + * one write for rest of parameters, mask of first write > + * is 0x07fffffd, and the other one write for setting > + * the mpll_clk_en. > + */ > + if (config) { > + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > + IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK > + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK > + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK > + | IMX6Q_GPR13_SATA_SPD_MODE_MASK > + | IMX6Q_GPR13_SATA_MPLL_SS_EN > + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK > + | IMX6Q_GPR13_SATA_TX_BOOST_MASK > + | IMX6Q_GPR13_SATA_TX_LVL_MASK > + | IMX6Q_GPR13_SATA_TX_EDGE_RATE > + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB > + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M > + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F > + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G > + | IMX6Q_GPR13_SATA_MPLL_SS_EN > + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16 > + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB > + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V); > + } > + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > + IMX6Q_GPR13_SATA_MPLL_CLK_EN, > + IMX6Q_GPR13_SATA_MPLL_CLK_EN); > + Regards, Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Eric, > Hi Marek, > > On 11/16/2013 06:20 PM, Marek Vasut wrote: > > The same code for enabling and disabling SATA clock was found in multiple > > places in the driver. Implement functions that enable/disable the SATA > > clock and use them in such places instead of duplicating the code. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Richard Zhu <r65037@freescale.com> > > Cc: Tejun Heo <tj@kernel.org> > > Cc: Linux-IDE <linux-ide@vger.kernel.org> > > --- > > > > drivers/ata/ahci_imx.c | 133 > > ++++++++++++++++++++++++++++--------------------- 1 file changed, 75 > > insertions(+), 58 deletions(-) > > > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > > index ae2d73f..c7ee505 100644 > > --- a/drivers/ata/ahci_imx.c > > +++ b/drivers/ata/ahci_imx.c > > @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; > > > > module_param_named(hotplug, ahci_imx_hotplug, int, 0644); > > MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, > > 1=support)"); > > > > <snip> > > I haven't traced through all of this, but if you're copying from > the Freescale 3.0.35 kernel, note that there's a bug in it, and > the 0x7FFFFFFD really wanted to be an 0x7FFFFFFF. I'm not taking this from FSL 3.0.35, no, but thanks for pointing this out! > The way I read this comment, the writes need to happen in two > steps: > - write everything with the PHY disabled > - enable the PHY > > We had reports of stalls waiting for SATA drives to be enumerated > that were solved with this commit... > > https://github.com/boundarydevices/linux- imx6/commit/0186ea224ce6bd1cb4757 > a0f83b0090e26a021f4 [...] > > + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > > + IMX6Q_GPR13_SATA_MPLL_CLK_EN, > > + IMX6Q_GPR13_SATA_MPLL_CLK_EN); Isn't this snippet doing exactly what your patch does ? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marek, On 11/18/2013 01:23 PM, Marek Vasut wrote: > Hi Eric, > >> Hi Marek, >> >> On 11/16/2013 06:20 PM, Marek Vasut wrote: >>> The same code for enabling and disabling SATA clock was found in multiple >>> places in the driver. Implement functions that enable/disable the SATA >>> clock and use them in such places instead of duplicating the code. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Shawn Guo <shawn.guo@linaro.org> >>> Cc: Richard Zhu <r65037@freescale.com> >>> Cc: Tejun Heo <tj@kernel.org> >>> Cc: Linux-IDE <linux-ide@vger.kernel.org> >>> --- >>> >>> drivers/ata/ahci_imx.c | 133 >>> ++++++++++++++++++++++++++++--------------------- 1 file changed, 75 >>> insertions(+), 58 deletions(-) >>> >>> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c >>> index ae2d73f..c7ee505 100644 >>> --- a/drivers/ata/ahci_imx.c >>> +++ b/drivers/ata/ahci_imx.c >>> @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; >>> >>> module_param_named(hotplug, ahci_imx_hotplug, int, 0644); >>> MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, >>> 1=support)"); >>> >>> <snip> >> >> I haven't traced through all of this, but if you're copying from >> the Freescale 3.0.35 kernel, note that there's a bug in it, and >> the 0x7FFFFFFD really wanted to be an 0x7FFFFFFF. > > I'm not taking this from FSL 3.0.35, no, but thanks for pointing this out! > >> The way I read this comment, the writes need to happen in two >> steps: >> - write everything with the PHY disabled >> - enable the PHY >> >> We had reports of stalls waiting for SATA drives to be enumerated >> that were solved with this commit... >> >> https://github.com/boundarydevices/linux- > imx6/commit/0186ea224ce6bd1cb4757 >> a0f83b0090e26a021f4 > > [...] > >>> + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, >>> + IMX6Q_GPR13_SATA_MPLL_CLK_EN, >>> + IMX6Q_GPR13_SATA_MPLL_CLK_EN); > > Isn't this snippet doing exactly what your patch does ? > This part is doing the "set the bit" part: https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.0.35_4.1.0/arch/arm/mach-mx6/board-mx6_nitrogen6x.c#L728 The previous block isn't clearing the enable bit though: + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, + IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK + | IMX6Q_GPR13_SATA_SPD_MODE_MASK + | IMX6Q_GPR13_SATA_MPLL_SS_EN + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK + | IMX6Q_GPR13_SATA_TX_BOOST_MASK + | IMX6Q_GPR13_SATA_TX_LVL_MASK + | IMX6Q_GPR13_SATA_TX_EDGE_RATE + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G + | IMX6Q_GPR13_SATA_MPLL_SS_EN + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16 + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V); The explicit clearing of bit 1 is what was necessary (and I think it's what the original author was after). This was a hard one to find, because it seems that it only shows up on some boards, and most of the time on those boards. Here are some references: http://boundarydevices.com/freescale-ubuntu-image-with-4-0-0-kernel/#comment-60464 http://boundarydevices.com/debian-installer-on-i-mx6-boards/#comment-76203 The symptom is (was) that U-Boot worked 100% of the time, but the kernel failed to find the drive. Using 'mm' in U-Boot to clear the bit before kernel launch also allowed things to function. Regards, Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGkgTmVsc29uOg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IGxpbnV4 LWlkZS1vd25lckB2Z2VyLmtlcm5lbC5vcmcgW21haWx0bzpsaW51eC1pZGUtb3duZXJAdmdlci5r ZXJuZWwub3JnXQ0KPiBPbiBCZWhhbGYgT2YgRXJpYyBOZWxzb24NCj4gU2VudDogVHVlc2RheSwg Tm92ZW1iZXIgMTksIDIwMTMgNjoxMiBBTQ0KPiBUbzogTWFyZWsgVmFzdXQNCj4gQ2M6IGxpbnV4 LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsgWmh1IFJpY2hhcmQtUjY1MDM3OyBUZWp1 biBIZW87IFNoYXduDQo+IEd1bzsgTGludXgtSURFDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMS81 XSBhaGNpOiBpbXg6IFB1bGwgb3V0IHRoZSBjbG9jayBlbmFibGUvZGlzYWJsZSBjYWxscw0KPiAN Cj4gSGkgTWFyZWssDQo+IA0KPiBPbiAxMS8xOC8yMDEzIDAxOjIzIFBNLCBNYXJlayBWYXN1dCB3 cm90ZToNCj4gPiBIaSBFcmljLA0KPiA+DQo+ID4+IEhpIE1hcmVrLA0KPiA+Pg0KPiA+PiBPbiAx MS8xNi8yMDEzIDA2OjIwIFBNLCBNYXJlayBWYXN1dCB3cm90ZToNCj4gPj4+IFRoZSBzYW1lIGNv ZGUgZm9yIGVuYWJsaW5nIGFuZCBkaXNhYmxpbmcgU0FUQSBjbG9jayB3YXMgZm91bmQgaW4NCj4g Pj4+IG11bHRpcGxlIHBsYWNlcyBpbiB0aGUgZHJpdmVyLiBJbXBsZW1lbnQgZnVuY3Rpb25zIHRo YXQNCj4gPj4+IGVuYWJsZS9kaXNhYmxlIHRoZSBTQVRBIGNsb2NrIGFuZCB1c2UgdGhlbSBpbiBz dWNoIHBsYWNlcyBpbnN0ZWFkIG9mDQo+IGR1cGxpY2F0aW5nIHRoZSBjb2RlLg0KPiA+Pj4NCj4g Pj4+IFNpZ25lZC1vZmYtYnk6IE1hcmVrIFZhc3V0IDxtYXJleEBkZW54LmRlPg0KPiA+Pj4gQ2M6 IFNoYXduIEd1byA8c2hhd24uZ3VvQGxpbmFyby5vcmc+DQo+ID4+PiBDYzogUmljaGFyZCBaaHUg PHI2NTAzN0BmcmVlc2NhbGUuY29tPg0KPiA+Pj4gQ2M6IFRlanVuIEhlbyA8dGpAa2VybmVsLm9y Zz4NCj4gPj4+IENjOiBMaW51eC1JREUgPGxpbnV4LWlkZUB2Z2VyLmtlcm5lbC5vcmc+DQo+ID4+ PiAtLS0NCj4gPj4+DQo+ID4+PiAgICBkcml2ZXJzL2F0YS9haGNpX2lteC5jIHwgMTMzDQo+ID4+ PiAgICArKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tIDEg ZmlsZSBjaGFuZ2VkLCA3NQ0KPiA+Pj4gICAgaW5zZXJ0aW9ucygrKSwgNTggZGVsZXRpb25zKC0p DQo+ID4+Pg0KPiA+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvYXRhL2FoY2lfaW14LmMgYi9kcml2 ZXJzL2F0YS9haGNpX2lteC5jIGluZGV4DQo+ID4+PiBhZTJkNzNmLi5jN2VlNTA1IDEwMDY0NA0K PiA+Pj4gLS0tIGEvZHJpdmVycy9hdGEvYWhjaV9pbXguYw0KPiA+Pj4gKysrIGIvZHJpdmVycy9h dGEvYWhjaV9pbXguYw0KPiA+Pj4gQEAgLTQ3LDYgKzQ3LDczIEBAIHN0YXRpYyBpbnQgYWhjaV9p bXhfaG90cGx1ZzsNCj4gPj4+DQo+ID4+PiAgICBtb2R1bGVfcGFyYW1fbmFtZWQoaG90cGx1Zywg YWhjaV9pbXhfaG90cGx1ZywgaW50LCAwNjQ0KTsNCj4gPj4+ICAgIE1PRFVMRV9QQVJNX0RFU0Mo aG90cGx1ZywgIkFIQ0kgSU1YIGhvdC1wbHVnIHN1cHBvcnQgKDA9RG9uJ3Qgc3VwcG9ydCwNCj4g Pj4+ICAgIDE9c3VwcG9ydCkiKTsNCj4gPj4+DQo+ID4+PiA8c25pcD4NCj4gPj4NCj4gPj4gSSBo YXZlbid0IHRyYWNlZCB0aHJvdWdoIGFsbCBvZiB0aGlzLCBidXQgaWYgeW91J3JlIGNvcHlpbmcg ZnJvbSB0aGUNCj4gPj4gRnJlZXNjYWxlIDMuMC4zNSBrZXJuZWwsIG5vdGUgdGhhdCB0aGVyZSdz IGEgYnVnIGluIGl0LCBhbmQgdGhlDQo+ID4+IDB4N0ZGRkZGRkQgcmVhbGx5IHdhbnRlZCB0byBi ZSBhbiAweDdGRkZGRkZGLg0KPiA+DQo+ID4gSSdtIG5vdCB0YWtpbmcgdGhpcyBmcm9tIEZTTCAz LjAuMzUsIG5vLCBidXQgdGhhbmtzIGZvciBwb2ludGluZyB0aGlzIG91dCENCj4gPg0KPiA+PiBU aGUgd2F5IEkgcmVhZCB0aGlzIGNvbW1lbnQsIHRoZSB3cml0ZXMgbmVlZCB0byBoYXBwZW4gaW4g dHdvDQo+ID4+IHN0ZXBzOg0KPiA+PiAJLSB3cml0ZSBldmVyeXRoaW5nIHdpdGggdGhlIFBIWSBk aXNhYmxlZA0KPiA+PiAJLSBlbmFibGUgdGhlIFBIWQ0KPiA+Pg0KPiA+PiBXZSBoYWQgcmVwb3J0 cyBvZiBzdGFsbHMgd2FpdGluZyBmb3IgU0FUQSBkcml2ZXMgdG8gYmUgZW51bWVyYXRlZA0KPiA+ PiB0aGF0IHdlcmUgc29sdmVkIHdpdGggdGhpcyBjb21taXQuLi4NCj4gPj4NCj4gPj4gCWh0dHBz Oi8vZ2l0aHViLmNvbS9ib3VuZGFyeWRldmljZXMvbGludXgtDQo+ID4gaW14Ni9jb21taXQvMDE4 NmVhMjI0Y2U2YmQxY2I0NzU3DQo+ID4+IGEwZjgzYjAwOTBlMjZhMDIxZjQNCj4gPg0KPiA+IFsu Li5dDQo+ID4NCj4gPj4+ICsJcmVnbWFwX3VwZGF0ZV9iaXRzKGlteHByaXYtPmdwciwgSU9NVVhD X0dQUjEzLA0KPiA+Pj4gKwkJCUlNWDZRX0dQUjEzX1NBVEFfTVBMTF9DTEtfRU4sDQo+ID4+PiAr CQkJSU1YNlFfR1BSMTNfU0FUQV9NUExMX0NMS19FTik7DQo+ID4NCj4gPiBJc24ndCB0aGlzIHNu aXBwZXQgZG9pbmcgZXhhY3RseSB3aGF0IHlvdXIgcGF0Y2ggZG9lcyA/DQo+ID4NCj4gVGhpcyBw YXJ0IGlzIGRvaW5nIHRoZSAic2V0IHRoZSBiaXQiIHBhcnQ6DQo+IAlodHRwczovL2dpdGh1Yi5j b20vYm91bmRhcnlkZXZpY2VzL2xpbnV4LWlteDYvYmxvYi9ib3VuZGFyeS0NCj4gaW14XzMuMC4z NV80LjEuMC9hcmNoL2FybS9tYWNoLW14Ni9ib2FyZC1teDZfbml0cm9nZW42eC5jI0w3MjgNCj4g DQo+IFRoZSBwcmV2aW91cyBibG9jayBpc24ndCBjbGVhcmluZyB0aGUgZW5hYmxlIGJpdCB0aG91 Z2g6DQo+IA0KPiArCQlyZWdtYXBfdXBkYXRlX2JpdHMoaW14cHJpdi0+Z3ByLCBJT01VWENfR1BS MTMsDQo+ICsJCQkJSU1YNlFfR1BSMTNfU0FUQV9SWF9FUV9WQUxfTUFTSw0KPiArCQkJCXwgSU1Y NlFfR1BSMTNfU0FUQV9SWF9MT1NfTFZMX01BU0sNCj4gKwkJCQl8IElNWDZRX0dQUjEzX1NBVEFf UlhfRFBMTF9NT0RFX01BU0sNCj4gKwkJCQl8IElNWDZRX0dQUjEzX1NBVEFfU1BEX01PREVfTUFT Sw0KPiArCQkJCXwgSU1YNlFfR1BSMTNfU0FUQV9NUExMX1NTX0VODQo+ICsJCQkJfCBJTVg2UV9H UFIxM19TQVRBX1RYX0FUVEVOX01BU0sNCj4gKwkJCQl8IElNWDZRX0dQUjEzX1NBVEFfVFhfQk9P U1RfTUFTSw0KPiArCQkJCXwgSU1YNlFfR1BSMTNfU0FUQV9UWF9MVkxfTUFTSw0KPiArCQkJCXwg SU1YNlFfR1BSMTNfU0FUQV9UWF9FREdFX1JBVEUNCj4gKwkJCQksIElNWDZRX0dQUjEzX1NBVEFf UlhfRVFfVkFMXzNfMF9EQg0KPiArCQkJCXwgSU1YNlFfR1BSMTNfU0FUQV9SWF9MT1NfTFZMX1NB VEEyTQ0KPiArCQkJCXwgSU1YNlFfR1BSMTNfU0FUQV9SWF9EUExMX01PREVfMlBfNEYNCj4gKwkJ CQl8IElNWDZRX0dQUjEzX1NBVEFfU1BEX01PREVfM1AwRw0KPiArCQkJCXwgSU1YNlFfR1BSMTNf U0FUQV9NUExMX1NTX0VODQo+ICsJCQkJfCBJTVg2UV9HUFIxM19TQVRBX1RYX0FUVEVOXzlfMTYN Cj4gKwkJCQl8IElNWDZRX0dQUjEzX1NBVEFfVFhfQk9PU1RfM18zM19EQg0KPiArCQkJCXwgSU1Y NlFfR1BSMTNfU0FUQV9UWF9MVkxfMV8wMjVfVik7DQo+IA0KPiBUaGUgZXhwbGljaXQgY2xlYXJp bmcgb2YgYml0IDEgaXMgd2hhdCB3YXMgbmVjZXNzYXJ5IChhbmQgSSB0aGluayBpdCdzIHdoYXQN Cj4gdGhlIG9yaWdpbmFsIGF1dGhvciB3YXMgYWZ0ZXIpLg0KPiANCj4gVGhpcyB3YXMgYSBoYXJk IG9uZSB0byBmaW5kLCBiZWNhdXNlIGl0IHNlZW1zIHRoYXQgaXQgb25seSBzaG93cyB1cCBvbiBz b21lDQo+IGJvYXJkcywgYW5kIG1vc3Qgb2YgdGhlIHRpbWUgb24gdGhvc2UgYm9hcmRzLg0KPiAN Cj4gSGVyZSBhcmUgc29tZSByZWZlcmVuY2VzOg0KPiAJaHR0cDovL2JvdW5kYXJ5ZGV2aWNlcy5j b20vZnJlZXNjYWxlLXVidW50dS1pbWFnZS13aXRoLTQtMC0wLQ0KPiBrZXJuZWwvI2NvbW1lbnQt NjA0NjQNCj4gCWh0dHA6Ly9ib3VuZGFyeWRldmljZXMuY29tL2RlYmlhbi1pbnN0YWxsZXItb24t aS1teDYtYm9hcmRzLyNjb21tZW50LQ0KPiA3NjIwMw0KPiANCj4gVGhlIHN5bXB0b20gaXMgKHdh cykgdGhhdCBVLUJvb3Qgd29ya2VkIDEwMCUgb2YgdGhlIHRpbWUsIGJ1dCB0aGUga2VybmVsDQo+ IGZhaWxlZCB0byBmaW5kIHRoZSBkcml2ZS4NCj4gDQo+IFVzaW5nICdtbScgaW4gVS1Cb290IHRv IGNsZWFyIHRoZSBiaXQgYmVmb3JlIGtlcm5lbCBsYXVuY2ggYWxzbyBhbGxvd2VkIHRoaW5ncw0K PiB0byBmdW5jdGlvbi4NCj4gDQpbUmljaGFyZF0gQWdyZWUgdG8gZXhwbGljaXQgY2xlYXJpbmcg b2YgYml0MSwgcmVmZXIgdG8gdGhlIGNoYXB0ZXIgIlBvd2VyLVVwIFNlcXVlbmNlcyIgb2YgDQoi U2VyaWFsIEFkdmFuY2VkIFRlY2hub2xvZ3kgQXR0YWNobWVudCBQSFkgKFNBVEEgUEhZKSIgaW4g aU1YNiBSTSBkb2MuDQoNCj4gUmVnYXJkcywNCj4gDQo+IA0KPiBFcmljDQo+IC0tDQo+IFRvIHVu c3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51 eC1pZGUiIGluIHRoZQ0KPiBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJu ZWwub3JnIE1vcmUgbWFqb3Jkb21vIGluZm8gYXQNCj4gaHR0cDovL3ZnZXIua2VybmVsLm9yZy9t YWpvcmRvbW8taW5mby5odG1sDQoNCkJlc3QgUmVnYXJkcw0KUmljaGFyZCBaaHUNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Eric Nelson, > Hi Marek, > > On 11/18/2013 01:23 PM, Marek Vasut wrote: > > Hi Eric, > > > >> Hi Marek, > >> > >> On 11/16/2013 06:20 PM, Marek Vasut wrote: > >>> The same code for enabling and disabling SATA clock was found in > >>> multiple places in the driver. Implement functions that enable/disable > >>> the SATA clock and use them in such places instead of duplicating the > >>> code. > >>> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Cc: Shawn Guo <shawn.guo@linaro.org> > >>> Cc: Richard Zhu <r65037@freescale.com> > >>> Cc: Tejun Heo <tj@kernel.org> > >>> Cc: Linux-IDE <linux-ide@vger.kernel.org> > >>> --- > >>> > >>> drivers/ata/ahci_imx.c | 133 > >>> ++++++++++++++++++++++++++++--------------------- 1 file changed, 75 > >>> insertions(+), 58 deletions(-) > >>> > >>> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > >>> index ae2d73f..c7ee505 100644 > >>> --- a/drivers/ata/ahci_imx.c > >>> +++ b/drivers/ata/ahci_imx.c > >>> @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; > >>> > >>> module_param_named(hotplug, ahci_imx_hotplug, int, 0644); > >>> MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't > >>> support, 1=support)"); > >>> > >>> <snip> > >> > >> I haven't traced through all of this, but if you're copying from > >> the Freescale 3.0.35 kernel, note that there's a bug in it, and > >> the 0x7FFFFFFD really wanted to be an 0x7FFFFFFF. > > > > I'm not taking this from FSL 3.0.35, no, but thanks for pointing this > > out! > > > >> The way I read this comment, the writes need to happen in two > >> > >> steps: > >> - write everything with the PHY disabled > >> - enable the PHY > >> > >> We had reports of stalls waiting for SATA drives to be enumerated > >> that were solved with this commit... > >> > >> https://github.com/boundarydevices/linux- > > > > imx6/commit/0186ea224ce6bd1cb4757 > > > >> a0f83b0090e26a021f4 > > > > [...] > > > >>> + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > >>> + IMX6Q_GPR13_SATA_MPLL_CLK_EN, > >>> + IMX6Q_GPR13_SATA_MPLL_CLK_EN); > > > > Isn't this snippet doing exactly what your patch does ? > > This part is doing the "set the bit" part: > https://github.com/boundarydevices/linux-imx6/blob/boundary- imx_3.0.35_4.1 > .0/arch/arm/mach-mx6/board-mx6_nitrogen6x.c#L728 > > The previous block isn't clearing the enable bit though: > > + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > + IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK > + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK > + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK > + | IMX6Q_GPR13_SATA_SPD_MODE_MASK > + | IMX6Q_GPR13_SATA_MPLL_SS_EN > + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK > + | IMX6Q_GPR13_SATA_TX_BOOST_MASK > + | IMX6Q_GPR13_SATA_TX_LVL_MASK > + | IMX6Q_GPR13_SATA_TX_EDGE_RATE > + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB > + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M > + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F > + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G > + | IMX6Q_GPR13_SATA_MPLL_SS_EN > + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16 > + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB > + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V); > > The explicit clearing of bit 1 is what was necessary (and I think > it's what the original author was after). > > This was a hard one to find, because it seems that it only shows > up on some boards, and most of the time on those boards. > > Here are some references: > http://boundarydevices.com/freescale-ubuntu-image-with-4-0-0- kernel/#comme > nt-60464 > http://boundarydevices.com/debian-installer-on-i-mx6-boards/#comment-76203 > > The symptom is (was) that U-Boot worked 100% of the time, but > the kernel failed to find the drive. > > Using 'mm' in U-Boot to clear the bit before kernel launch also > allowed things to function. Ah, now I see it. I will cook a patch, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index ae2d73f..c7ee505 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -47,6 +47,73 @@ static int ahci_imx_hotplug; module_param_named(hotplug, ahci_imx_hotplug, int, 0644); MODULE_PARM_DESC(hotplug, "AHCI IMX hot-plug support (0=Don't support, 1=support)"); +static int imx_sata_clock_enable(struct device *dev, bool config) +{ + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + int ret; + + imxpriv->gpr = + syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr"); + if (IS_ERR(imxpriv->gpr)) { + dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n"); + return PTR_ERR(imxpriv->gpr); + } + + ret = clk_prepare_enable(imxpriv->sata_ref_clk); + if (ret < 0) { + dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret); + return ret; + } + + /* + * set PHY Paremeters, two steps to configure the GPR13, + * one write for rest of parameters, mask of first write + * is 0x07fffffd, and the other one write for setting + * the mpll_clk_en. + */ + if (config) { + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, + IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK + | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK + | IMX6Q_GPR13_SATA_SPD_MODE_MASK + | IMX6Q_GPR13_SATA_MPLL_SS_EN + | IMX6Q_GPR13_SATA_TX_ATTEN_MASK + | IMX6Q_GPR13_SATA_TX_BOOST_MASK + | IMX6Q_GPR13_SATA_TX_LVL_MASK + | IMX6Q_GPR13_SATA_TX_EDGE_RATE + , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB + | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M + | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F + | IMX6Q_GPR13_SATA_SPD_MODE_3P0G + | IMX6Q_GPR13_SATA_MPLL_SS_EN + | IMX6Q_GPR13_SATA_TX_ATTEN_9_16 + | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB + | IMX6Q_GPR13_SATA_TX_LVL_1_025_V); + } + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, + IMX6Q_GPR13_SATA_MPLL_CLK_EN, + IMX6Q_GPR13_SATA_MPLL_CLK_EN); + + ret = clk_prepare_enable(imxpriv->sata_ref_clk); + if (ret < 0) { + dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret); + return ret; + } + + return 0; +} + +static void imx_sata_clock_disable(struct device *dev) +{ + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + + regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, + IMX6Q_GPR13_SATA_MPLL_CLK_EN, + !IMX6Q_GPR13_SATA_MPLL_CLK_EN); + clk_disable_unprepare(imxpriv->sata_ref_clk); +} + static void ahci_imx_error_handler(struct ata_port *ap) { u32 reg_val; @@ -72,10 +139,7 @@ static void ahci_imx_error_handler(struct ata_port *ap) */ reg_val = readl(mmio + PORT_PHY_CTL); writel(reg_val | PORT_PHY_CTL_PDDQ_LOC, mmio + PORT_PHY_CTL); - regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, - IMX6Q_GPR13_SATA_MPLL_CLK_EN, - !IMX6Q_GPR13_SATA_MPLL_CLK_EN); - clk_disable_unprepare(imxpriv->sata_ref_clk); + imx_sata_clock_disable(ap->dev); imxpriv->no_device = true; } @@ -97,44 +161,10 @@ static int imx6q_sata_init(struct device *dev, void __iomem *mmio) unsigned int reg_val; struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - imxpriv->gpr = - syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr"); - if (IS_ERR(imxpriv->gpr)) { - dev_err(dev, "failed to find fsl,imx6q-iomux-gpr regmap\n"); - return PTR_ERR(imxpriv->gpr); - } - - ret = clk_prepare_enable(imxpriv->sata_ref_clk); - if (ret < 0) { - dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret); + ret = imx_sata_clock_enable(dev, true); + if (ret < 0) return ret; - } - /* - * set PHY Paremeters, two steps to configure the GPR13, - * one write for rest of parameters, mask of first write - * is 0x07fffffd, and the other one write for setting - * the mpll_clk_en. - */ - regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK - | IMX6Q_GPR13_SATA_RX_LOS_LVL_MASK - | IMX6Q_GPR13_SATA_RX_DPLL_MODE_MASK - | IMX6Q_GPR13_SATA_SPD_MODE_MASK - | IMX6Q_GPR13_SATA_MPLL_SS_EN - | IMX6Q_GPR13_SATA_TX_ATTEN_MASK - | IMX6Q_GPR13_SATA_TX_BOOST_MASK - | IMX6Q_GPR13_SATA_TX_LVL_MASK - | IMX6Q_GPR13_SATA_TX_EDGE_RATE - , IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB - | IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M - | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F - | IMX6Q_GPR13_SATA_SPD_MODE_3P0G - | IMX6Q_GPR13_SATA_MPLL_SS_EN - | IMX6Q_GPR13_SATA_TX_ATTEN_9_16 - | IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB - | IMX6Q_GPR13_SATA_TX_LVL_1_025_V); - regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN, - IMX6Q_GPR13_SATA_MPLL_CLK_EN); usleep_range(100, 200); /* @@ -163,11 +193,7 @@ static int imx6q_sata_init(struct device *dev, void __iomem *mmio) static void imx6q_sata_exit(struct device *dev) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - - regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN, - !IMX6Q_GPR13_SATA_MPLL_CLK_EN); - clk_disable_unprepare(imxpriv->sata_ref_clk); + imx_sata_clock_disable(dev); } static int imx_ahci_suspend(struct device *dev) @@ -178,12 +204,8 @@ static int imx_ahci_suspend(struct device *dev) * If no_device is set, The CLKs had been gated off in the * initialization so don't do it again here. */ - if (!imxpriv->no_device) { - regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, - IMX6Q_GPR13_SATA_MPLL_CLK_EN, - !IMX6Q_GPR13_SATA_MPLL_CLK_EN); - clk_disable_unprepare(imxpriv->sata_ref_clk); - } + if (!imxpriv->no_device) + imx_sata_clock_disable(dev); return 0; } @@ -194,15 +216,10 @@ static int imx_ahci_resume(struct device *dev) int ret; if (!imxpriv->no_device) { - ret = clk_prepare_enable(imxpriv->sata_ref_clk); - if (ret < 0) { - dev_err(dev, "pre-enable sata_ref clock err:%d\n", ret); + ret = imx_sata_clock_enable(dev, false); + if (ret < 0) return ret; - } - regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, - IMX6Q_GPR13_SATA_MPLL_CLK_EN, - IMX6Q_GPR13_SATA_MPLL_CLK_EN); usleep_range(1000, 2000); }
The same code for enabling and disabling SATA clock was found in multiple places in the driver. Implement functions that enable/disable the SATA clock and use them in such places instead of duplicating the code. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: Richard Zhu <r65037@freescale.com> Cc: Tejun Heo <tj@kernel.org> Cc: Linux-IDE <linux-ide@vger.kernel.org> --- drivers/ata/ahci_imx.c | 133 ++++++++++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 58 deletions(-)