| Message ID | 1393084424-31099-9-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | 90870d79d4f28711610dd2e72d8fa616c922d110 |
| Headers | show |
On Sat, Feb 22, 2014 at 04:53:37PM +0100, Hans de Goede wrote: > + /* > + * set PHY Paremeters, two steps to configure the GPR13, > + * one write for rest of parameters, mask of first write > + * is 0x07ffffff, and the other one write for setting > + * the mpll_clk_en. > + */ > + 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_MPLL_CLK_EN | > + 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); While I see this, I'll stick an oar in. It is totally wrong for this to be hard-coded into the driver - many of these should be specified via DT, since they're board specific. These are already different from the reset default in this register. The side effect of this hard coding is that eSATA just doesn't work at all on some boards - the board I have requires TX_LVL_1_104V and TX_BOOST_0_00_DB. Hence, I'd ask that while you move stuff like this around, you bear in mind that we do need to add additional properties. I'm in two minds about this - whether to make the existing binding with its compatible string always use these settings, and invent a new compatible string for one which is fully configurable (as it _should_ have been from the very start) or whether to make this the default if the various properties aren't specified. Either way, this driver is electrically unusable as it stands here.
Hi Russell, On 02/28/2014 10:08 PM, Russell King - ARM Linux wrote: > On Sat, Feb 22, 2014 at 04:53:37PM +0100, Hans de Goede wrote: >> + /* >> + * set PHY Paremeters, two steps to configure the GPR13, >> + * one write for rest of parameters, mask of first write >> + * is 0x07ffffff, and the other one write for setting >> + * the mpll_clk_en. >> + */ >> + 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_MPLL_CLK_EN | >> + 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); > > While I see this, I'll stick an oar in. > > It is totally wrong for this to be hard-coded into the driver - many > of these should be specified via DT, since they're board specific. > These are already different from the reset default in this register. > > The side effect of this hard coding is that eSATA just doesn't work > at all on some boards - the board I have requires TX_LVL_1_104V and > TX_BOOST_0_00_DB. I completely agree that if this is board specific it should be settable (override-able since we've existing dt files without a setting for it). > Hence, I'd ask that while you move stuff like this around, you bear > in mind that we do need to add additional properties. I understand, but my interest in the imx code only goes as far as not making it regress. I already went above and beyond duty by buying an imx board with sata and cleaning up the horific platform device driver instantiating a platform device hack there was. My interest in platform-ahci was to clean it up so that it could be used as a proper basis to build other ARM ahci drivers on top of, such as a driver for the allwinner A10 and A20 ahci controller. While at this I ended up having to clean up the imx driver too, so as to not break at. As a bonus I fixed it to not loose the harddisk after a suspend / resume cycle, and that is really as far as my interest in the imx driver goes. I'm sure Tejun would welcome patches to add a dts property for setting the board-specific phy parameters, but I won't be writing it. > I'm in two minds about this - whether to make the existing binding > with its compatible string always use these settings, and invent a > new compatible string for one which is fully configurable (as it > _should_ have been from the very start) or whether to make this > the default if the various properties aren't specified. IMHO this does not warrant doing a new compatibole-string. Simply default to the current hardcoded phy settings if no settings are specified through devicetree. > Either way, this driver is electrically unusable as it stands here. That is only true on some boards. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I've moved the devicetree mailing list to the To: header in case anyone there wants to comment on this. On Sat, Mar 01, 2014 at 11:38:32AM +0100, Hans de Goede wrote: > I'm sure Tejun would welcome patches to add a dts property for > setting the board-specific phy parameters, but I won't be > writing it. Don't worry, I already have something :) >> I'm in two minds about this - whether to make the existing binding >> with its compatible string always use these settings, and invent a >> new compatible string for one which is fully configurable (as it >> _should_ have been from the very start) or whether to make this >> the default if the various properties aren't specified. > > IMHO this does not warrant doing a new compatibole-string. Simply > default to the current hardcoded phy settings if no settings are > specified through devicetree. The problem is that we need to keep existing setups working - which means when there's no properties specified, we need to default to the settings hard-coded into the driver. That means introducing properties like: fsl,no-spread-spectrum so that the hard-coded defaults can be turned off, and also deal with a whole load of defaults for the other properties. That's not particularly nice. Doing it this way, what I currently have is this: struct reg_value { u32 of_value; u32 reg_value; }; struct reg_property { const char *name; const struct reg_value *values; size_t num_values; u32 def_value; u32 set_value; }; static const struct reg_value gpr13_tx_level[] = { { 937, IMX6Q_GPR13_SATA_TX_LVL_0_937_V }, { 947, IMX6Q_GPR13_SATA_TX_LVL_0_947_V }, ... { 1230, IMX6Q_GPR13_SATA_TX_LVL_1_230_V }, { 1240, IMX6Q_GPR13_SATA_TX_LVL_1_240_V } }; static const struct reg_value gpr13_tx_boost[] = { { 0, IMX6Q_GPR13_SATA_TX_BOOST_0_00_DB }, { 370, IMX6Q_GPR13_SATA_TX_BOOST_0_37_DB }, ... { 528, IMX6Q_GPR13_SATA_TX_BOOST_5_28_DB }, { 575, IMX6Q_GPR13_SATA_TX_BOOST_5_75_DB } }; static const struct reg_value gpr13_tx_atten[] = { { 8, IMX6Q_GPR13_SATA_TX_ATTEN_8_16 }, { 9, IMX6Q_GPR13_SATA_TX_ATTEN_9_16 }, ... { 14, IMX6Q_GPR13_SATA_TX_ATTEN_14_16 }, { 16, IMX6Q_GPR13_SATA_TX_ATTEN_16_16 }, }; static const struct reg_value gpr13_rx_eq[] = { { 500, IMX6Q_GPR13_SATA_RX_EQ_VAL_0_5_DB }, { 1000, IMX6Q_GPR13_SATA_RX_EQ_VAL_1_0_DB }, ... { 3500, IMX6Q_GPR13_SATA_RX_EQ_VAL_3_5_DB }, { 4000, IMX6Q_GPR13_SATA_RX_EQ_VAL_4_0_DB }, }; static const struct reg_property gpr13_props[] = { { .name = "fsl,transmit-level-mV", .values = gpr13_tx_level, .num_values = ARRAY_SIZE(gpr13_tx_level), .def_value = IMX6Q_GPR13_SATA_TX_LVL_1_025_V, }, { .name = "fsl,transmit-boost-mdB", .values = gpr13_tx_boost, .num_values = ARRAY_SIZE(gpr13_tx_boost), .def_value = IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB, }, { .name = "fsl,transmit-atten-16ths", .values = gpr13_tx_atten, .num_values = ARRAY_SIZE(gpr13_tx_atten), .def_value = IMX6Q_GPR13_SATA_TX_ATTEN_9_16, }, { .name = "fsl,receive-eq-mdB", .values = gpr13_rx_eq, .num_values = ARRAY_SIZE(gpr13_rx_eq), .def_value = IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB, }, { .name = "fsl,no-spread-spectrum", .def_value = IMX6Q_GPR13_SATA_MPLL_SS_EN, .set_value = 0, }, }; and then a bunch of code to read through these tables and work out the GPR13 register value from it - it doesn't handle everything yet because I've not worked out a good way to do the last remaining three - I'm thinking that they want to be strings: regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK | ... IMX6Q_GPR13_SATA_TX_EDGE_RATE, IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M | IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F | IMX6Q_GPR13_SATA_SPD_MODE_3P0G | reg_value); RX_LOS_LVL: SATA1I / SATA1M / SATA1X / SATA2I / SATA2M / SATA2X RX_DPLL_MODE: 1P_1F / 2P_2F / 1P_4F / 2P_4F SPD_MODE: 3P0G / 1P5G (I do wonder whether this should be changed when the Linux wants to slow the link speed.) With a new compatible string, we can use the hard-coded version for fsl,imx6q-ahci, but require all properties (with values) to be specified for a different compatible string, thereby eliminating all the defaults, and making things like "no-spread-spectrum" be a positive property instead of negative, and this simplifies the parsing code. There's also the obvious question about which of these properties should be generic ones for AHCI/SATA interfaces... The only one I see with any kind of electrical properties specified is sata_highbank: - calxeda,tx-atten : a u32 array that contains TX attenuation override codes, one per port. The upper 3 bytes are always 0 and thus ignored. and that seems to be a register-coded value rather than an actual attenuation figure. A simpler solution to this would be to just provide an imx6-specific property which encodes the GPR13 value directly, and not bother with trying to provide individual properties.
Hi, On 03/01/2014 12:24 PM, Russell King - ARM Linux wrote: > I've moved the devicetree mailing list to the To: header in case anyone > there wants to comment on this. > > On Sat, Mar 01, 2014 at 11:38:32AM +0100, Hans de Goede wrote: >> I'm sure Tejun would welcome patches to add a dts property for >> setting the board-specific phy parameters, but I won't be >> writing it. > > Don't worry, I already have something :) Great. >>> I'm in two minds about this - whether to make the existing binding >>> with its compatible string always use these settings, and invent a >>> new compatible string for one which is fully configurable (as it >>> _should_ have been from the very start) or whether to make this >>> the default if the various properties aren't specified. >> >> IMHO this does not warrant doing a new compatibole-string. Simply >> default to the current hardcoded phy settings if no settings are >> specified through devicetree. > > The problem is that we need to keep existing setups working - which > means when there's no properties specified, we need to default to the > settings hard-coded into the driver. > > That means introducing properties like: > > fsl,no-spread-spectrum > > so that the hard-coded defaults can be turned off, and also deal with > a whole load of defaults for the other properties. That's not > particularly nice. Doing it this way, what I currently have is this: > > struct reg_value { > u32 of_value; > u32 reg_value; > }; > > struct reg_property { > const char *name; > const struct reg_value *values; > size_t num_values; > u32 def_value; > u32 set_value; > }; > > static const struct reg_value gpr13_tx_level[] = { > { 937, IMX6Q_GPR13_SATA_TX_LVL_0_937_V }, > { 947, IMX6Q_GPR13_SATA_TX_LVL_0_947_V }, > ... > { 1230, IMX6Q_GPR13_SATA_TX_LVL_1_230_V }, > { 1240, IMX6Q_GPR13_SATA_TX_LVL_1_240_V } > }; > > static const struct reg_value gpr13_tx_boost[] = { > { 0, IMX6Q_GPR13_SATA_TX_BOOST_0_00_DB }, > { 370, IMX6Q_GPR13_SATA_TX_BOOST_0_37_DB }, > ... > { 528, IMX6Q_GPR13_SATA_TX_BOOST_5_28_DB }, > { 575, IMX6Q_GPR13_SATA_TX_BOOST_5_75_DB } > }; > > static const struct reg_value gpr13_tx_atten[] = { > { 8, IMX6Q_GPR13_SATA_TX_ATTEN_8_16 }, > { 9, IMX6Q_GPR13_SATA_TX_ATTEN_9_16 }, > ... > { 14, IMX6Q_GPR13_SATA_TX_ATTEN_14_16 }, > { 16, IMX6Q_GPR13_SATA_TX_ATTEN_16_16 }, > }; > > static const struct reg_value gpr13_rx_eq[] = { > { 500, IMX6Q_GPR13_SATA_RX_EQ_VAL_0_5_DB }, > { 1000, IMX6Q_GPR13_SATA_RX_EQ_VAL_1_0_DB }, > ... > { 3500, IMX6Q_GPR13_SATA_RX_EQ_VAL_3_5_DB }, > { 4000, IMX6Q_GPR13_SATA_RX_EQ_VAL_4_0_DB }, > }; > > static const struct reg_property gpr13_props[] = { > { > .name = "fsl,transmit-level-mV", > .values = gpr13_tx_level, > .num_values = ARRAY_SIZE(gpr13_tx_level), > .def_value = IMX6Q_GPR13_SATA_TX_LVL_1_025_V, > }, { > .name = "fsl,transmit-boost-mdB", > .values = gpr13_tx_boost, > .num_values = ARRAY_SIZE(gpr13_tx_boost), > .def_value = IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB, > }, { > .name = "fsl,transmit-atten-16ths", > .values = gpr13_tx_atten, > .num_values = ARRAY_SIZE(gpr13_tx_atten), > .def_value = IMX6Q_GPR13_SATA_TX_ATTEN_9_16, > }, { > .name = "fsl,receive-eq-mdB", > .values = gpr13_rx_eq, > .num_values = ARRAY_SIZE(gpr13_rx_eq), > .def_value = IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB, > }, { > .name = "fsl,no-spread-spectrum", > .def_value = IMX6Q_GPR13_SATA_MPLL_SS_EN, > .set_value = 0, > }, > }; > > and then a bunch of code to read through these tables and work out the > GPR13 register value from it - it doesn't handle everything yet because > I've not worked out a good way to do the last remaining three - I'm > thinking that they want to be strings: > > regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, > IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK | > ... > IMX6Q_GPR13_SATA_TX_EDGE_RATE, > IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M | > IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F | > IMX6Q_GPR13_SATA_SPD_MODE_3P0G | > reg_value); > > RX_LOS_LVL: SATA1I / SATA1M / SATA1X / SATA2I / SATA2M / SATA2X > RX_DPLL_MODE: 1P_1F / 2P_2F / 1P_4F / 2P_4F > SPD_MODE: 3P0G / 1P5G (I do wonder whether this should be changed > when the Linux wants to slow the link speed.) > > With a new compatible string, we can use the hard-coded version for > fsl,imx6q-ahci, but require all properties (with values) to be specified > for a different compatible string, thereby eliminating all the defaults, > and making things like "no-spread-spectrum" be a positive property instead > of negative, and this simplifies the parsing code. I see, that does make sense. So consider my +1 for keeping the same compatible string changed to a 0 > There's also the obvious question about which of these properties should > be generic ones for AHCI/SATA interfaces... The only one I see with any > kind of electrical properties specified is sata_highbank: > > - calxeda,tx-atten : a u32 array that contains TX attenuation override > codes, one per port. The upper 3 bytes are always > 0 and thus ignored. > > and that seems to be a register-coded value rather than an actual > attenuation figure. > > A simpler solution to this would be to just provide an imx6-specific > property which encodes the GPR13 value directly, and not bother with > trying to provide individual properties. Yes, that is actually the direction I was thinking in. I think it is great we know what all the bits do in so much detail in the freescale case, but for many other phys we don't have such extensive documentation. Still I can see how in the freescale case you do want to use that documentation to do something better then coding a register value inside the devicetree. OTOH I do like the KISS value of jut specifying a register value. So in the end it is all up to you I'm afraid. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Saturday, February 22, 2014 04:53:37 PM Hans de Goede wrote: > This avoids the ugliness of creating a nested platform device from probe. > > While moving it around anyways, move the mk6q phy init code from probe > to imx_sata_enable, as the phy needs to be re-initialized on resume too, > otherwise the drive won't be recognized after resume. > > Tested on a wandboard i.mx6 quad. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../devicetree/bindings/ata/ahci-platform.txt | 9 +- > drivers/ata/ahci_imx.c | 331 ++++++++------------- > 2 files changed, 134 insertions(+), 206 deletions(-) > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt > index 499bfed..d86e854 100644 > --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt > @@ -5,8 +5,9 @@ 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", "fsl,imx53-ahci" or > + "fsl,imx6q-ahci" > - interrupts : <interrupt mapping for SATA IRQ> > - reg : <registers mapping> > > @@ -15,6 +16,10 @@ Optional properties: > - clocks : a list of phandle + clock specifier pairs > - target-supply : regulator for SATA target power > > +"fsl,imx53-ahci", "fsl,imx6q-ahci" required properties: > +- clocks : must contain the sata, sata_ref and ahb clocks > +- clock-names : must contain "ahb" for the ahb clock > + > Examples: > sata@ffe08000 { > compatible = "snps,spear-ahci"; > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > index dd4d6f7..3cb5d69 100644 > --- a/drivers/ata/ahci_imx.c > +++ b/drivers/ata/ahci_imx.c [...] > +static void ahci_imx_host_stop(struct ata_host *host) > +{ > + struct ahci_host_priv *hpriv = host->private_data; > > - ret = platform_device_add_resources(ahci_pdev, res, 2); > - if (ret) > - goto err_out; > + imx_sata_disable(hpriv); > +} > > - ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata)); > - if (ret) > - goto err_out; > +static int imx_ahci_suspend(struct device *dev) > +{ > + struct ata_host *host = dev_get_drvdata(dev); > + struct ahci_host_priv *hpriv = host->private_data; > + int ret; > > - ret = platform_device_add(ahci_pdev); > - if (ret) { > -err_out: > - platform_device_put(ahci_pdev); > + ret = ahci_platform_suspend_host(dev); > + if (ret) > return ret; > - } > + > + imx_sata_disable(hpriv); > > return 0; > } > > -static int imx_ahci_remove(struct platform_device *pdev) > +static int imx_ahci_resume(struct device *dev) > { > - struct imx_ahci_priv *imxpriv = platform_get_drvdata(pdev); > - struct platform_device *ahci_pdev = imxpriv->ahci_pdev; > + struct ata_host *host = dev_get_drvdata(dev); > + struct ahci_host_priv *hpriv = host->private_data; > + int ret; > > - platform_device_unregister(ahci_pdev); > - return 0; > + ret = imx_sata_enable(hpriv); > + if (ret) > + return ret; > + > + return ahci_platform_resume_host(dev); > } The code above introduces two new warnings for CONFIG_PM_SLEEP=n: drivers/ata/ahci_imx.c:284:12: warning: ‘imx_ahci_suspend’ defined but not used [-Wunused-function] drivers/ata/ahci_imx.c:299:12: warning: ‘imx_ahci_resume’ defined but not used [-Wunused-function] [ There needs to be CONFIG_PM_SLEEP ifdef around imh_ahci_suspend() and imx_ahci_resume(). ] All the rest looks good. > +static SIMPLE_DEV_PM_OPS(ahci_imx_pm_ops, imx_ahci_suspend, imx_ahci_resume); > + > static struct platform_driver imx_ahci_driver = { > .probe = imx_ahci_probe, > - .remove = imx_ahci_remove, > + .remove = ata_platform_remove_one, > .driver = { > .name = "ahci-imx", > .owner = THIS_MODULE, > .of_match_table = imx_ahci_of_match, > + .pm = &ahci_imx_pm_ops, > }, > }; > module_platform_driver(imx_ahci_driver); Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt index 499bfed..d86e854 100644 --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt @@ -5,8 +5,9 @@ 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", "fsl,imx53-ahci" or + "fsl,imx6q-ahci" - interrupts : <interrupt mapping for SATA IRQ> - reg : <registers mapping> @@ -15,6 +16,10 @@ Optional properties: - clocks : a list of phandle + clock specifier pairs - target-supply : regulator for SATA target power +"fsl,imx53-ahci", "fsl,imx6q-ahci" required properties: +- clocks : must contain the sata, sata_ref and ahb clocks +- clock-names : must contain "ahb" for the ahb clock + Examples: sata@ffe08000 { compatible = "snps,spear-ahci"; diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index dd4d6f7..3cb5d69 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -42,13 +42,7 @@ enum ahci_imx_type { struct imx_ahci_priv { struct platform_device *ahci_pdev; enum ahci_imx_type type; - - /* i.MX53 clock */ - struct clk *sata_gate_clk; - /* Common clock */ - struct clk *sata_ref_clk; struct clk *ahb_clk; - struct regmap *gpr; bool no_device; bool first_time; @@ -58,28 +52,52 @@ 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) +static void ahci_imx_host_stop(struct ata_host *host); + +static int imx_sata_enable(struct ahci_host_priv *hpriv) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + struct imx_ahci_priv *imxpriv = hpriv->plat_data; int ret; - if (imxpriv->type == AHCI_IMX53) { - ret = clk_prepare_enable(imxpriv->sata_gate_clk); - if (ret < 0) { - dev_err(dev, "prepare-enable sata_gate clock err:%d\n", - ret); + if (imxpriv->no_device) + return 0; + + if (hpriv->target_pwr) { + ret = regulator_enable(hpriv->target_pwr); + if (ret) return ret; - } } - ret = clk_prepare_enable(imxpriv->sata_ref_clk); - if (ret < 0) { - dev_err(dev, "prepare-enable sata_ref clock err:%d\n", - ret); - goto clk_err; - } + ret = ahci_platform_enable_clks(hpriv); + if (ret < 0) + goto disable_regulator; if (imxpriv->type == AHCI_IMX6Q) { + /* + * set PHY Paremeters, two steps to configure the GPR13, + * one write for rest of parameters, mask of first write + * is 0x07ffffff, and the other one write for setting + * the mpll_clk_en. + */ + 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_MPLL_CLK_EN | + 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); @@ -89,15 +107,19 @@ static int imx_sata_clock_enable(struct device *dev) return 0; -clk_err: - if (imxpriv->type == AHCI_IMX53) - clk_disable_unprepare(imxpriv->sata_gate_clk); +disable_regulator: + if (hpriv->target_pwr) + regulator_disable(hpriv->target_pwr); + return ret; } -static void imx_sata_clock_disable(struct device *dev) +static void imx_sata_disable(struct ahci_host_priv *hpriv) { - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); + struct imx_ahci_priv *imxpriv = hpriv->plat_data; + + if (imxpriv->no_device) + return; if (imxpriv->type == AHCI_IMX6Q) { regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13, @@ -105,10 +127,10 @@ static void imx_sata_clock_disable(struct device *dev) !IMX6Q_GPR13_SATA_MPLL_CLK_EN); } - clk_disable_unprepare(imxpriv->sata_ref_clk); + ahci_platform_disable_clks(hpriv); - if (imxpriv->type == AHCI_IMX53) - clk_disable_unprepare(imxpriv->sata_gate_clk); + if (hpriv->target_pwr) + regulator_disable(hpriv->target_pwr); } static void ahci_imx_error_handler(struct ata_port *ap) @@ -118,7 +140,7 @@ static void ahci_imx_error_handler(struct ata_port *ap) struct ata_host *host = dev_get_drvdata(ap->dev); struct ahci_host_priv *hpriv = host->private_data; void __iomem *mmio = hpriv->mmio; - struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent); + struct imx_ahci_priv *imxpriv = hpriv->plat_data; ahci_error_handler(ap); @@ -136,7 +158,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); - imx_sata_clock_disable(ap->dev); + imx_sata_disable(hpriv); imxpriv->no_device = true; } @@ -144,7 +166,9 @@ static int ahci_imx_softreset(struct ata_link *link, unsigned int *class, unsigned long deadline) { struct ata_port *ap = link->ap; - struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent); + struct ata_host *host = dev_get_drvdata(ap->dev); + struct ahci_host_priv *hpriv = host->private_data; + struct imx_ahci_priv *imxpriv = hpriv->plat_data; int ret = -EIO; if (imxpriv->type == AHCI_IMX53) @@ -156,7 +180,8 @@ static int ahci_imx_softreset(struct ata_link *link, unsigned int *class, } static struct ata_port_operations ahci_imx_ops = { - .inherits = &ahci_platform_ops, + .inherits = &ahci_ops, + .host_stop = ahci_imx_host_stop, .error_handler = ahci_imx_error_handler, .softreset = ahci_imx_softreset, }; @@ -168,79 +193,6 @@ static const struct ata_port_info ahci_imx_port_info = { .port_ops = &ahci_imx_ops, }; -static int imx_sata_init(struct device *dev, void __iomem *mmio) -{ - int ret = 0; - unsigned int reg_val; - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - - ret = imx_sata_clock_enable(dev); - if (ret < 0) - return ret; - - /* - * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL, - * and IP vendor specific register HOST_TIMER1MS. - * Configure CAP_SSS (support stagered spin up). - * Implement the port0. - * Get the ahb clock rate, and configure the TIMER1MS register. - */ - reg_val = readl(mmio + HOST_CAP); - if (!(reg_val & HOST_CAP_SSS)) { - reg_val |= HOST_CAP_SSS; - writel(reg_val, mmio + HOST_CAP); - } - reg_val = readl(mmio + HOST_PORTS_IMPL); - if (!(reg_val & 0x1)) { - reg_val |= 0x1; - writel(reg_val, mmio + HOST_PORTS_IMPL); - } - - reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; - writel(reg_val, mmio + HOST_TIMER1MS); - - return 0; -} - -static void imx_sata_exit(struct device *dev) -{ - imx_sata_clock_disable(dev); -} - -static int imx_ahci_suspend(struct device *dev) -{ - 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) - imx_sata_clock_disable(dev); - - return 0; -} - -static int imx_ahci_resume(struct device *dev) -{ - struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); - int ret = 0; - - if (!imxpriv->no_device) - ret = imx_sata_clock_enable(dev); - - return ret; -} - -static struct ahci_platform_data imx_sata_pdata = { - .init = imx_sata_init, - .exit = imx_sata_exit, - .ata_port_info = &ahci_imx_port_info, - .suspend = imx_ahci_suspend, - .resume = imx_ahci_resume, - -}; - static const struct of_device_id imx_ahci_of_match[] = { { .compatible = "fsl,imx53-ahci", .data = (void *)AHCI_IMX53 }, { .compatible = "fsl,imx6q-ahci", .data = (void *)AHCI_IMX6Q }, @@ -251,151 +203,122 @@ MODULE_DEVICE_TABLE(of, imx_ahci_of_match); static int imx_ahci_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct resource *mem, *irq, res[2]; const struct of_device_id *of_id; - enum ahci_imx_type type; - const struct ahci_platform_data *pdata = NULL; + struct ahci_host_priv *hpriv; struct imx_ahci_priv *imxpriv; - struct device *ahci_dev; - struct platform_device *ahci_pdev; + unsigned int reg_val; int ret; of_id = of_match_device(imx_ahci_of_match, dev); if (!of_id) return -EINVAL; - type = (enum ahci_imx_type)of_id->data; - pdata = &imx_sata_pdata; - imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL); - if (!imxpriv) { - dev_err(dev, "can't alloc ahci_host_priv\n"); + if (!imxpriv) return -ENOMEM; - } - - ahci_pdev = platform_device_alloc("ahci", -1); - if (!ahci_pdev) - return -ENODEV; - - ahci_dev = &ahci_pdev->dev; - ahci_dev->parent = dev; imxpriv->no_device = false; imxpriv->first_time = true; - imxpriv->type = type; - + imxpriv->type = (enum ahci_imx_type)of_id->data; 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; + return PTR_ERR(imxpriv->ahb_clk); } - if (type == AHCI_IMX53) { - imxpriv->sata_gate_clk = devm_clk_get(dev, "sata_gate"); - if (IS_ERR(imxpriv->sata_gate_clk)) { - dev_err(dev, "can't get sata_gate clock.\n"); - ret = PTR_ERR(imxpriv->sata_gate_clk); - goto err_out; + if (imxpriv->type == AHCI_IMX6Q) { + 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); } } - 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; - } + hpriv = ahci_platform_get_resources(pdev); + if (IS_ERR(hpriv)) + return PTR_ERR(hpriv); + + hpriv->plat_data = imxpriv; - imxpriv->ahci_pdev = ahci_pdev; - platform_set_drvdata(pdev, imxpriv); + ret = imx_sata_enable(hpriv); + if (ret) + return ret; - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); - if (!mem || !irq) { - dev_err(dev, "no mmio/irq resource\n"); - ret = -ENOMEM; - goto err_out; + /* + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL, + * and IP vendor specific register HOST_TIMER1MS. + * Configure CAP_SSS (support stagered spin up). + * Implement the port0. + * Get the ahb clock rate, and configure the TIMER1MS register. + */ + reg_val = readl(hpriv->mmio + HOST_CAP); + if (!(reg_val & HOST_CAP_SSS)) { + reg_val |= HOST_CAP_SSS; + writel(reg_val, hpriv->mmio + HOST_CAP); + } + reg_val = readl(hpriv->mmio + HOST_PORTS_IMPL); + if (!(reg_val & 0x1)) { + reg_val |= 0x1; + writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL); } - res[0] = *mem; - res[1] = *irq; + reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; + writel(reg_val, hpriv->mmio + HOST_TIMER1MS); - ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32); - ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask; - ahci_dev->of_node = dev->of_node; + ret = ahci_platform_init_host(pdev, hpriv, &ahci_imx_port_info, 0, 0); + if (ret) + imx_sata_disable(hpriv); - if (type == AHCI_IMX6Q) { - 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"); - ret = PTR_ERR(imxpriv->gpr); - goto err_out; - } + return ret; +} - /* - * Set PHY Paremeters, two steps to configure the GPR13, - * one write for rest of parameters, mask of first write - * is 0x07fffffe, and the other one write for setting - * the mpll_clk_en happens in imx_sata_clock_enable(). - */ - 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_MPLL_CLK_EN | - 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); - } +static void ahci_imx_host_stop(struct ata_host *host) +{ + struct ahci_host_priv *hpriv = host->private_data; - ret = platform_device_add_resources(ahci_pdev, res, 2); - if (ret) - goto err_out; + imx_sata_disable(hpriv); +} - ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata)); - if (ret) - goto err_out; +static int imx_ahci_suspend(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + int ret; - ret = platform_device_add(ahci_pdev); - if (ret) { -err_out: - platform_device_put(ahci_pdev); + ret = ahci_platform_suspend_host(dev); + if (ret) return ret; - } + + imx_sata_disable(hpriv); return 0; } -static int imx_ahci_remove(struct platform_device *pdev) +static int imx_ahci_resume(struct device *dev) { - struct imx_ahci_priv *imxpriv = platform_get_drvdata(pdev); - struct platform_device *ahci_pdev = imxpriv->ahci_pdev; + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + int ret; - platform_device_unregister(ahci_pdev); - return 0; + ret = imx_sata_enable(hpriv); + if (ret) + return ret; + + return ahci_platform_resume_host(dev); } +static SIMPLE_DEV_PM_OPS(ahci_imx_pm_ops, imx_ahci_suspend, imx_ahci_resume); + static struct platform_driver imx_ahci_driver = { .probe = imx_ahci_probe, - .remove = imx_ahci_remove, + .remove = ata_platform_remove_one, .driver = { .name = "ahci-imx", .owner = THIS_MODULE, .of_match_table = imx_ahci_of_match, + .pm = &ahci_imx_pm_ops, }, }; module_platform_driver(imx_ahci_driver);
This avoids the ugliness of creating a nested platform device from probe. While moving it around anyways, move the mk6q phy init code from probe to imx_sata_enable, as the phy needs to be re-initialized on resume too, otherwise the drive won't be recognized after resume. Tested on a wandboard i.mx6 quad. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../devicetree/bindings/ata/ahci-platform.txt | 9 +- drivers/ata/ahci_imx.c | 331 ++++++++------------- 2 files changed, 134 insertions(+), 206 deletions(-)