| Message ID | 1390088935-7193-11-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote: > +enum { > + CLK_SATA, > + CLK_SATA_REF, > + CLK_AHB > +}; Err, so we now rely on the order that these clocks are specified in DT rather than their name properties to provide the correct clock... that sounds particularly fragile to me.
Hi, On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote: > On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote: >> +enum { >> + CLK_SATA, >> + CLK_SATA_REF, >> + CLK_AHB >> +}; > > Err, so we now rely on the order that these clocks are specified in DT > rather than their name properties to provide the correct clock... that > sounds particularly fragile to me. Both in the ohci- / ehci-platform case, where the idea of purely addressing clocks by index comes from, as well as in the ahci-platform case, people have been asking me to make things more generic, so as to avoid having a gazillion almost but not quite the same ehci-foo platform drivers. This has already happened with ohci-foo.c drivers, and the hope is that with the new generalized ohci-plaform.c many of the existing ohci-foo drivers can go away over time. The downside of this generalized approach is that we cannot use clock-names since those tend to be implementation specific. In the specific case of the ahci-imx driver this means that certain clocks must be at a specific index, since it needs to know which one is the AHB clock, as documented in the bindings documentation. I don't see how mandating certain indices is different and/or more fragile then mandating certain names in the bindings document. I agree that is is slightly less clear to someone reading the dts, but that is the price we have to pay for this desire for things to be generic. Regards, Hans
On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote: > Hi, > > On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote: >> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote: >>> +enum { >>> + CLK_SATA, >>> + CLK_SATA_REF, >>> + CLK_AHB >>> +}; >> >> Err, so we now rely on the order that these clocks are specified in DT >> rather than their name properties to provide the correct clock... that >> sounds particularly fragile to me. > > Both in the ohci- / ehci-platform case, where the idea of purely addressing > clocks by index comes from, as well as in the ahci-platform case, people > have been asking me to make things more generic, so as to avoid having > a gazillion almost but not quite the same ehci-foo platform drivers. > > This has already happened with ohci-foo.c drivers, and the hope is that > with the new generalized ohci-plaform.c many of the existing ohci-foo > drivers can go away over time. > > The downside of this generalized approach is that we cannot use clock-names > since those tend to be implementation specific. > > In the specific case of the ahci-imx driver this means that certain clocks > must be at a specific index, since it needs to know which one is the AHB > clock, as documented in the bindings documentation. I don't see how mandating > certain indices is different and/or more fragile then mandating certain names > in the bindings document. I agree that is is slightly less clear to someone > reading the dts, but that is the price we have to pay for this desire for > things to be generic. So what happens if we have the same IP block appearing, but the SATA_REF or SATA clock isn't present - how do we still provide an AHB clock (for argument sake)?
Hi, On 01/19/2014 08:32 PM, Russell King - ARM Linux wrote: > On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote: >> Hi, >> >> On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote: >>> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote: >>>> +enum { >>>> + CLK_SATA, >>>> + CLK_SATA_REF, >>>> + CLK_AHB >>>> +}; >>> >>> Err, so we now rely on the order that these clocks are specified in DT >>> rather than their name properties to provide the correct clock... that >>> sounds particularly fragile to me. >> >> Both in the ohci- / ehci-platform case, where the idea of purely addressing >> clocks by index comes from, as well as in the ahci-platform case, people >> have been asking me to make things more generic, so as to avoid having >> a gazillion almost but not quite the same ehci-foo platform drivers. >> >> This has already happened with ohci-foo.c drivers, and the hope is that >> with the new generalized ohci-plaform.c many of the existing ohci-foo >> drivers can go away over time. >> >> The downside of this generalized approach is that we cannot use clock-names >> since those tend to be implementation specific. >> >> In the specific case of the ahci-imx driver this means that certain clocks >> must be at a specific index, since it needs to know which one is the AHB >> clock, as documented in the bindings documentation. I don't see how mandating >> certain indices is different and/or more fragile then mandating certain names >> in the bindings document. I agree that is is slightly less clear to someone >> reading the dts, but that is the price we have to pay for this desire for >> things to be generic. > > So what happens if we have the same IP block appearing, but the SATA_REF > or SATA clock isn't present - how do we still provide an AHB clock (for > argument sake)? Then it will not use the same compatible string, since then clearly it is not compatible with "fsl,imx6q-ahci" And we can document different indices for the new compatible string (and adjust the code to match). Regards, Hans
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt index f036e786..ee3a127 100644 --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt @@ -5,8 +5,8 @@ Each SATA controller should have its own node. Required properties: - compatible : compatible list, one of "snps,spear-ahci", - "snps,exynos5440-ahci", "ibm,476gtr-ahci", or - "allwinner,sun4i-a10-ahci" + "snps,exynos5440-ahci", "ibm,476gtr-ahci", + "allwinner,sun4i-a10-ahci" or "fsl,imx6q-ahci" - interrupts : <interrupt mapping for SATA IRQ> - reg : <registers mapping> @@ -18,6 +18,10 @@ Optional properties: allwinner,sun4i-a10-ahci required properties: - clocks : index 0 must point to the sata_ref clk, 1 to the ahb clk +fsl,imx6q-ahci required properties: +- clocks : index 0 must point to the sataf clk, 1 to the sata_ref + clk and 2 to the ahb clk + Examples: sata@ffe08000 { compatible = "snps,spear-ahci"; diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 0051f29..8eb24a3 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -34,12 +34,15 @@ enum { HOST_TIMER1MS = 0xe0, /* Timer 1-ms */ }; +enum { + CLK_SATA, + CLK_SATA_REF, + CLK_AHB +}; + struct imx_ahci_priv { struct platform_device *ahci_pdev; - struct clk *sata_ref_clk; - struct clk *ahb_clk; struct regmap *gpr; - bool no_device; bool first_time; }; @@ -55,6 +58,7 @@ static void ahci_imx_error_handler(struct ata_port *ap) struct ahci_host_priv *hpriv = host->private_data; void __iomem *mmio = hpriv->mmio; struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent); + int i; ahci_error_handler(ap); @@ -75,8 +79,13 @@ static void ahci_imx_error_handler(struct ata_port *ap) 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); - imxpriv->no_device = true; + + for (i = CLK_AHB; i >= 0; i--) { + clk_disable_unprepare(hpriv->clks[i]); + /* Stop ahci_platform.c from trying to use the clks */ + clk_put(hpriv->clks[i]); + hpriv->clks[i] = NULL; + } } static struct ata_port_operations ahci_imx_ops = { @@ -94,7 +103,6 @@ static const struct ata_port_info ahci_imx_port_info = { static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv, void __iomem *mmio) { - int ret = 0; unsigned int reg_val; struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); @@ -105,12 +113,6 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv, 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 @@ -157,7 +159,11 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv, writel(reg_val, mmio + HOST_PORTS_IMPL); } - reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; + if (!hpriv->clks[CLK_AHB]) { + dev_err(dev, "no ahb clk, need sata, sata_ref and ahb clks\n"); + return -ENOENT; + } + reg_val = clk_get_rate(hpriv->clks[CLK_AHB]) / 1000; writel(reg_val, mmio + HOST_TIMER1MS); return 0; @@ -165,41 +171,36 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv, static void imx6q_sata_exit(struct device *dev) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN, + if (hpriv->clks[CLK_SATA]) + 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); } static void imx_ahci_suspend(struct device *dev) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - /* - * 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) { + /* Check the CLKs have not been gated off in the initialization. */ + if (hpriv->clks[CLK_SATA]) 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 int imx_ahci_resume(struct device *dev) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - 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); - return ret; - } + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + if (hpriv->clks[CLK_SATA]) { regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, IMX6Q_GPR13_SATA_MPLL_CLK_EN, IMX6Q_GPR13_SATA_MPLL_CLK_EN); @@ -247,22 +248,7 @@ static int imx_ahci_probe(struct platform_device *pdev) ahci_dev = &ahci_pdev->dev; ahci_dev->parent = dev; - imxpriv->no_device = false; imxpriv->first_time = true; - imxpriv->ahb_clk = devm_clk_get(dev, "ahb"); - if (IS_ERR(imxpriv->ahb_clk)) { - dev_err(dev, "can't get ahb clock.\n"); - ret = PTR_ERR(imxpriv->ahb_clk); - goto err_out; - } - - imxpriv->sata_ref_clk = devm_clk_get(dev, "sata_ref"); - if (IS_ERR(imxpriv->sata_ref_clk)) { - dev_err(dev, "can't get sata_ref clock.\n"); - ret = PTR_ERR(imxpriv->sata_ref_clk); - goto err_out; - } - imxpriv->ahci_pdev = ahci_pdev; platform_set_drvdata(pdev, imxpriv);
ahci_platform manages all 3 clocks now, so enabling / disabling them from ahci_imx just results in the sata_ref clock getting enabled / disabled twice. Note untested, I've ordered a wandboard to be able to test these changes. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../devicetree/bindings/ata/ahci-platform.txt | 8 ++- drivers/ata/ahci_imx.c | 82 +++++++++------------- 2 files changed, 40 insertions(+), 50 deletions(-)