| Message ID | 20140118184427.GJ15937@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jan 18, 2014 at 06:44:27PM +0000, Russell King - ARM Linux wrote: > So, I see we have AHCI support for SATA on the iMX6. Great... but it > doesn't work on the cubox-i, because the phy settings are wrong. And another thing. This is wonderful... really wonderful. static int imx_ahci_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; ... ahci_pdev = platform_device_alloc("ahci", -1); ... ahci_dev = &ahci_pdev->dev; ... ahci_dev->of_node = dev->of_node; This is a hanging offence. Here's a lesson in how DT matching works: - A device gets declared into the device model. - The device is offered to each driver in turn via the bus specific code to see whether the driver should be bound to the device. - If there's an of_node present, the DT IDs for the driver are walked to compare the device with the driver's DT IDs. If a match is found, the device is passed to the driver probe function. - If there is no of_node present, and it's a platform device, the bare device name is compared the driver name(s) and if it matches the probe function is called. Now, what does this mean for the above monstrosity? If the driver model happens to find the ahci_imx driver _before_ the ahci driver while trying to bind the ahci_dev, it will find that the ahci_dev has an of_node with a compatible string which matches this driver. So, imx_ahci_probe() _can_ be called with the ahci_pdev that it just created. It doesn't take much to understand what the result will be of that. It will try to create another ahci device... hopefully this time erroring out. This is utterly disgusting. You must *never* *ever* assign an of_node from one device to another of the same bus type. If you need to pass the of_node to another device, then it _must_ be done outside of the child device's of_node pointer - in other words, it must be done using platform data. Alternatively, turn ahci into a library that both the original ahci and ahci_imx drivers can use without jumping through these kinds of games - or in this case, just get rid of that assignment - I can't see anything in ahci.c which needs the of_node.
On Sat, Jan 18, 2014 at 07:03:37PM +0000, Russell King - ARM Linux wrote: > On Sat, Jan 18, 2014 at 06:44:27PM +0000, Russell King - ARM Linux wrote: > > So, I see we have AHCI support for SATA on the iMX6. Great... but it > > doesn't work on the cubox-i, because the phy settings are wrong. > > And another thing. This is wonderful... really wonderful. > > static int imx_ahci_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > ... > ahci_pdev = platform_device_alloc("ahci", -1); > ... > ahci_dev = &ahci_pdev->dev; > ... > ahci_dev->of_node = dev->of_node; > > This is a hanging offence. > > Here's a lesson in how DT matching works: > > - A device gets declared into the device model. > - The device is offered to each driver in turn via the bus specific > code to see whether the driver should be bound to the device. > - If there's an of_node present, the DT IDs for the driver are walked > to compare the device with the driver's DT IDs. If a match is found, > the device is passed to the driver probe function. > - If there is no of_node present, and it's a platform device, the bare > device name is compared the driver name(s) and if it matches the > probe function is called. > > Now, what does this mean for the above monstrosity? If the driver > model happens to find the ahci_imx driver _before_ the ahci driver > while trying to bind the ahci_dev, it will find that the ahci_dev > has an of_node with a compatible string which matches this driver. > So, imx_ahci_probe() _can_ be called with the ahci_pdev that it > just created. > > It doesn't take much to understand what the result will be of that. > It will try to create another ahci device... hopefully this time > erroring out. > > This is utterly disgusting. You must *never* *ever* assign an of_node > from one device to another of the same bus type. If you need to pass > the of_node to another device, then it _must_ be done outside of the > child device's of_node pointer - in other words, it must be done using > platform data. > > Alternatively, turn ahci into a library that both the original ahci > and ahci_imx drivers can use without jumping through these kinds of > games - or in this case, just get rid of that assignment - I can't > see anything in ahci.c which needs the of_node. Sigh. You can't get rid of the of_node there because it's needed for clk_get() inside ahci_platform.c. So, I think ahci_platform needs to become yet another library, and thereby avoid creating a child platform device.
Copy Richard who might be better to clarify. Shawn On Sat, Jan 18, 2014 at 06:44:27PM +0000, Russell King - ARM Linux wrote: > So, I see we have AHCI support for SATA on the iMX6. Great... but it > doesn't work on the cubox-i, because the phy settings are wrong. > > The Cubox-i requires GPR13 set to 0x0593A044 - I haven't decoded what > this means yet, but it's different from the 0x0593E4A4 which is > currently hard-coded into the driver (and I've independently tested > that this is indeed required.) > > So, there's presently no DT properties for this - given that these > parameters would be board specific, it surprises me that this has not > been thought about, and properties already generated, because now it > means that we need to _add_ new properties to this driver. > > Also, this PDDQ mode thing, which can't be recovered except by reset. > This is another illustration why Linux is unfriendly - the thing can > silently go into this power down mode which is irrecoverable without > any messages being generated nor any hints how to avoid it - maybe > this should also be a DT property, not just a command line option. > > More importantly, maybe we should print a message when we discover > that there's nothing connected and we're going to enter this mode - > maybe something like this: > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c > index 3e23e9941dad..0a1ae7213992 100644 > --- a/drivers/ata/ahci_imx.c > +++ b/drivers/ata/ahci_imx.c > @@ -77,6 +77,10 @@ static void ahci_imx_error_handler(struct ata_port *ap) > !IMX6Q_GPR13_SATA_MPLL_CLK_EN); > clk_disable_unprepare(imxpriv->sata_ref_clk); > imxpriv->no_device = true; > + > + dev_info(ap->dev, "no device, link disabled until next reset.\n"); > + dev_info(ap->dev, "pass " MODULE_PARAM_PREFIX > + ".hotplug=1 to enable link hotplug support\n"); > } > > static struct ata_port_operations ahci_imx_ops = { > > > -- > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > Estimate before purchase was "up to 13.2Mbit".
On Sat, Jan 18, 2014 at 09:11:19PM +0000, Russell King - ARM Linux wrote: > Sigh. You can't get rid of the of_node there because it's needed > for clk_get() inside ahci_platform.c. So, I think ahci_platform needs > to become yet another library, and thereby avoid creating a child > platform device. Yes, you're right. This has been discussed for a while [1]. Shawn [1] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/4835/focus=4909
Hi: > -----Original Message----- > From: Shawn Guo [mailto:shawn.guo@linaro.org] > Sent: Sunday, January 19, 2014 12:16 PM > To: Russell King - ARM Linux > Cc: Sascha Hauer; Zhu Richard-R65037; linux-arm-kernel@lists.infradead.org > Subject: Re: imx6 eSATA > > Copy Richard who might be better to clarify. > > Shawn > > On Sat, Jan 18, 2014 at 06:44:27PM +0000, Russell King - ARM Linux wrote: > > So, I see we have AHCI support for SATA on the iMX6. Great... but it > > doesn't work on the cubox-i, because the phy settings are wrong. > > > > The Cubox-i requires GPR13 set to 0x0593A044 - I haven't decoded what > > this means yet, but it's different from the 0x0593E4A4 which is [Richard] About the configurations of GPR13, first of all, this parameters are used to configure imx6q ahci sata phy and to co-operated with board properly. The differences between, 0x0593A044 and 0x0593E4A4 are listed below: 0x0593E4A4 enable the SATA PHY - Spread Spectrum Enable[bit 14] 0x0593A044 doesn't enable the SATA PHY - Spread Spectrum Enable[bit 14] 0x0593E4A4: SATA PHY Tx -Transmit Boost Control[bit10-7] 3.33 dB 0x0593A044: SATA PHY Tx -Transmit Boost Control[bit10-7] 0 dB 0x0593E4A4: SATA PHY - Transmit level settings[bit6-2] 1.025V 0x0593A044: SATA PHY - Transmit level settings[bit6-2] 1.104V > > currently hard-coded into the driver (and I've independently tested > > that this is indeed required.) > > > > So, there's presently no DT properties for this - given that these > > parameters would be board specific, it surprises me that this has not > > been thought about, and properties already generated, because now it > > means that we need to _add_ new properties to this driver. > > > > Also, this PDDQ mode thing, which can't be recovered except by reset. > > This is another illustration why Linux is unfriendly - the thing can > > silently go into this power down mode which is irrecoverable without > > any messages being generated nor any hints how to avoid it - maybe > > this should also be a DT property, not just a command line option. > > > > More importantly, maybe we should print a message when we discover > > that there's nothing connected and we're going to enter this mode - > > maybe something like this: > > > > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index > > 3e23e9941dad..0a1ae7213992 100644 > > --- a/drivers/ata/ahci_imx.c > > +++ b/drivers/ata/ahci_imx.c > > @@ -77,6 +77,10 @@ static void ahci_imx_error_handler(struct ata_port *ap) > > !IMX6Q_GPR13_SATA_MPLL_CLK_EN); > > clk_disable_unprepare(imxpriv->sata_ref_clk); > > imxpriv->no_device = true; > > + > > + dev_info(ap->dev, "no device, link disabled until next reset.\n"); > > + dev_info(ap->dev, "pass " MODULE_PARAM_PREFIX > > + ".hotplug=1 to enable link hotplug support\n"); > > } > > > > static struct ata_port_operations ahci_imx_ops = { > > > > > > -- > > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > > Estimate before purchase was "up to 13.2Mbit". Best Regards Richard Zhu
Hi Russel, 2014-01-18 19:44 GMT+01:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > So, I see we have AHCI support for SATA on the iMX6. Great... but it > doesn't work on the cubox-i, because the phy settings are wrong. > > The Cubox-i requires GPR13 set to 0x0593A044 - I haven't decoded what > this means yet, but it's different from the 0x0593E4A4 which is > currently hard-coded into the driver (and I've independently tested > that this is indeed required.) Pardon me if this is a naive question, but how do you know which value is required for a specific board ? Thanks, JM
On Wed, Aug 27, 2014 at 11:11:15AM +0200, Jean-Michel Hautbois wrote: > Hi Russel, > > 2014-01-18 19:44 GMT+01:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > > So, I see we have AHCI support for SATA on the iMX6. Great... but it > > doesn't work on the cubox-i, because the phy settings are wrong. > > > > The Cubox-i requires GPR13 set to 0x0593A044 - I haven't decoded what > > this means yet, but it's different from the 0x0593E4A4 which is > > currently hard-coded into the driver (and I've independently tested > > that this is indeed required.) > > Pardon me if this is a naive question, but how do you know which value > is required for a specific board ? It's not something that one just knows. The register defines the electrical characteristics of the port, which is dependent on the PCB layout of the platform. The SATA spec includes requirements for the signal properties at the connector, and the values in this register need to be adjusted to meet the specification. The problem that this presents for open source software folk is that they generally do not have the expensive equipment to be able to perform this validation, so one must rely on someone who does (hopefully the platform manufacturer has done that.) The alternative is to look at the register definition, and adjust each property until you get something which works - though the likelyhood of that method working for a wide range of SATA devices is pretty small, especially as there will be some interdependence between the various properties.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 3e23e9941dad..0a1ae7213992 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -77,6 +77,10 @@ static void ahci_imx_error_handler(struct ata_port *ap) !IMX6Q_GPR13_SATA_MPLL_CLK_EN); clk_disable_unprepare(imxpriv->sata_ref_clk); imxpriv->no_device = true; + + dev_info(ap->dev, "no device, link disabled until next reset.\n"); + dev_info(ap->dev, "pass " MODULE_PARAM_PREFIX + ".hotplug=1 to enable link hotplug support\n"); } static struct ata_port_operations ahci_imx_ops = {