imx6 eSATA

imx6 eSATA

[Russell King - ARM Linux]

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".

imx6 eSATA

[Russell King - ARM Linux]

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.

-- 
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".

imx6 eSATA

[Russell King - ARM Linux]

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.

-- 
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".

imx6 eSATA

[Shawn Guo]

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

imx6 eSATA

[Shawn Guo]

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".

imx6 eSATA

[Hong-Xing.Zhu at freescale.com]

Hi:

> -----Original Message-----
> From: Shawn Guo [mailto:shawn.guo at linaro.org]
> Sent: Sunday, January 19, 2014 12:16 PM
> To: Russell King - ARM Linux
> Cc: Sascha Hauer; Zhu Richard-R65037; linux-arm-kernel at 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

imx6 eSATA

[Jean-Michel Hautbois]

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

imx6 eSATA

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.