ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Greetings.

I was occasionally running into problems at boot time on an
MPC8315-based board (derived from the MPC831xRDB, apparently), using
SATA to talk to an SSD.  My vendor suggested that I enable
CONFIG_MPC8315_DS.

That symbol is only found once in the entire kernel codebase:

  $ git checkout v3.4-rc7
  HEAD is now at 36be505... Linux 3.4-rc7

  $ git grep -nH CONFIG_MPC8315_DS
  drivers/ata/sata_fsl.c:729:#ifdef CONFIG_MPC8315_DS

There is no kconfig support for it at all.

It was added in 2007; further, this is the only commit in the entire
git history that contains this string:

   commit e7eac96e8f0e57a6e9f94943557bc2b23be31471
   Author: ashish kalra <ashish.kalra@freescale.com>
   Date:   Wed Oct 31 19:28:02 2007 +0800

       ata/sata_fsl: Move MPC8315DS link speed limit workaround to specific ifdef
       
       Signed-off-by: ashish kalra <ashish.kalra@freescale.com>
       Signed-off-by: Li Yang <leoli@freescale.com>
       Signed-off-by: Jeff Garzik <jeff@garzik.org>

   diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
   index 5892472..e076e1f 100644
   --- a/drivers/ata/sata_fsl.c
   +++ b/drivers/ata/sata_fsl.c
   @@ -652,6 +652,7 @@ static int sata_fsl_port_start(struct ata_port *ap)
           VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
           VPRINTK("CHBA  = 0x%x\n", ioread32(hcr_base + CHBA));
    
   +#ifdef CONFIG_MPC8315_DS
           /*
            * Workaround for 8315DS board 3gbps link-up issue,
            * currently limit SATA port to GEN1 speed
   @@ -664,6 +665,7 @@ static int sata_fsl_port_start(struct ata_port *ap)
           sata_fsl_scr_read(ap, SCR_CONTROL, &temp);
           dev_printk(KERN_WARNING, dev, "scr_control, speed limited to %x\n",
                           temp);
   +#endif
    
           return 0;
    }

This otherwise-unsupported variable was noted by Robert Day in 2008;
Adrian Bunk suggested a patch, but the Freescale folks said that it
was for a not-yet-mainlined board, so the patch was dropped:

   http://marc.info/?l=linux-ide&m=121783965216004&w=2

As Robert notied again in 2010, it still wasn't mainlined:

   http://marc.info/?l=linux-ide&m=121783965216004&w=2

And, obviously, it still isn't today.

Can the Freescale people tell us exactly what we should be testing to
determine when to enforce this restriction?  A config variable that
points to a non-existent board doesn't seem much help.

Thanks,
Tony

RE: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Li Yang-R58472]

> -----Original Message-----
> From: Anthony Foiani [mailto:tkil@scrye.com]
> Sent: Friday, May 18, 2012 1:08 AM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: ashish kalra; Li Yang-R58472; Jeff Garzik; Robert P.J.Day; Adrian
> Bunk
> Subject: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS
>=20
>=20
> Greetings.
>=20
> I was occasionally running into problems at boot time on an MPC8315-based
> board (derived from the MPC831xRDB, apparently), using SATA to talk to an
> SSD.  My vendor suggested that I enable CONFIG_MPC8315_DS.
>=20
> That symbol is only found once in the entire kernel codebase:
>=20
>   $ git checkout v3.4-rc7
>   HEAD is now at 36be505... Linux 3.4-rc7
>=20
>   $ git grep -nH CONFIG_MPC8315_DS
>   drivers/ata/sata_fsl.c:729:#ifdef CONFIG_MPC8315_DS
>=20
> There is no kconfig support for it at all.
>=20
> It was added in 2007; further, this is the only commit in the entire git
> history that contains this string:
>=20
>    commit e7eac96e8f0e57a6e9f94943557bc2b23be31471
>    Author: ashish kalra <ashish.kalra@freescale.com>
>    Date:   Wed Oct 31 19:28:02 2007 +0800
>=20
>        ata/sata_fsl: Move MPC8315DS link speed limit workaround to
> specific ifdef
>=20
>        Signed-off-by: ashish kalra <ashish.kalra@freescale.com>
>        Signed-off-by: Li Yang <leoli@freescale.com>
>        Signed-off-by: Jeff Garzik <jeff@garzik.org>
>=20
>    diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>    index 5892472..e076e1f 100644
>    --- a/drivers/ata/sata_fsl.c
>    +++ b/drivers/ata/sata_fsl.c
>    @@ -652,6 +652,7 @@ static int sata_fsl_port_start(struct ata_port *ap=
)
>            VPRINTK("HControl =3D 0x%x\n", ioread32(hcr_base + HCONTROL));
>            VPRINTK("CHBA  =3D 0x%x\n", ioread32(hcr_base + CHBA));
>=20
>    +#ifdef CONFIG_MPC8315_DS
>            /*
>             * Workaround for 8315DS board 3gbps link-up issue,
>             * currently limit SATA port to GEN1 speed
>    @@ -664,6 +665,7 @@ static int sata_fsl_port_start(struct ata_port *ap=
)
>            sata_fsl_scr_read(ap, SCR_CONTROL, &temp);
>            dev_printk(KERN_WARNING, dev, "scr_control, speed limited
> to %x\n",
>                            temp);
>    +#endif
>=20
>            return 0;
>     }
>=20
> This otherwise-unsupported variable was noted by Robert Day in 2008;
> Adrian Bunk suggested a patch, but the Freescale folks said that it was
> for a not-yet-mainlined board, so the patch was dropped:
>=20
>    http://marc.info/?l=3Dlinux-ide&m=3D121783965216004&w=3D2
>=20
> As Robert notied again in 2010, it still wasn't mainlined:
>=20
>    http://marc.info/?l=3Dlinux-ide&m=3D121783965216004&w=3D2
>=20
> And, obviously, it still isn't today.
>=20
> Can the Freescale people tell us exactly what we should be testing to
> determine when to enforce this restriction?  A config variable that
> points to a non-existent board doesn't seem much help.

Thanks for bringing it up again.  Looks like we do have a problem here.

Btw, did it help with your problem by enabling it?

Leo

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Li Yang-R58472 <r58472@freescale.com> writes:

> Thanks for bringing [CONFIG_MPC8315_DS] up again.  Looks like we do
> have a problem here.

My impression is that the simplest fix is Adrian's patch, which simply
keys off CONFIG_MPC831x_RDB.  It's not very satisfying, but I'll take
"working" vs. "rare lockups at boot".

If there is some other defining characteristic of boards that require
this patch, then a simple KConfig snippit with a description would be
even better.  Without any KConfig support for this variable, I lost it
even after using an oldconfig from my vendor.

(Or, if it was preserved, I might have removed it when trying to
optimize the kernel for support for our hardware, and I had no way of
knowing that the MPC8315_DS had any impact on my system at all...)

If it's actually a CPU/SOC-level problem, then an ideal situation
would conditionalize the fix by examining CPU version or stepping.
That would allow us to get rid of the config variable entirely.

> Btw, did it help with your problem by enabling it?

Possibly.  :)

I only saw the problem (failure to SATA handshake at 3Gbps?) very
rarely, maybe one in 100 warm boot cycles.

I've added the patch to my current project, and have not seen the
problem since then, but until I'm problem free for another few weeks,
I can't sign off on it.

It certainly does look like a reasonable band-aid fix.  In our case,
we don't need anywhere near the higher bandwidth, so it's acceptable
from that point of view.

A clear statement or reference to a CPU / SOC errata would be
preferred, though.  It's a 4-year-old design, so even a brown paper
bag bug isn't all that embarrassing anymore.  :)

Thanks,
Tony

p.s. This board also seems to suffer from occasionaly USB lockups on
     boot; if you end up digging through errata on 8315-based boards,
     please keep an eye out for that as well.  Thanks!  Link:

        http://patchwork.ozlabs.org/patch/152755/

     I'm currently using that patch as well as a 10ms delay to try to
     avoid the hang.  Successfully, so far, but a "blessed" solution
     from FSL would be awesome.

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 05/26/2012 01:53 AM, Anthony Foiani wrote:
> Li Yang-R58472 <r58472@freescale.com> writes:
> 
>> Thanks for bringing [CONFIG_MPC8315_DS] up again.  Looks like we do
>> have a problem here.
> 
> My impression is that the simplest fix is Adrian's patch, which simply
> keys off CONFIG_MPC831x_RDB.  It's not very satisfying, but I'll take
> "working" vs. "rare lockups at boot".

CONFIG_MPC831x_RDB doesn't mean that you're running on such a board,
only that the kernel supports those boards.  It should be a runtime test.

-Scott

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott Wood <scottwood@freescale.com> writes:

> CONFIG_MPC831x_RDB doesn't mean that you're running on such a board,
> only that the kernel supports those boards.  It should be a runtime
> test.

Point taken.

If that SATA check is CPU/SOC-based, then it should be easy enough to
test.  The cpuinfo for my board is:

  # cat /proc/cpuinfo
  processor       : 0
  cpu             : e300c3
  clock           : 266.666664MHz
  revision        : 2.0 (pvr 8085 0020)
  bogomips        : 66.66
  timebase        : 33333333

On the other hand, if the problem is actually caused by board trace
routing (or other hardware that's outside the control of the CPU/SOC),
then I don't know how possible a runtime check will be.

Do you know if there is a specific errata that the MPC8315_DS ran
across that required this fix, or was it a band-aid in the first
place?

Either way, thanks for looking into this.

Thanks,
Tony

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 05/29/2012 05:07 PM, Anthony Foiani wrote:
> Scott Wood <scottwood@freescale.com> writes:
> 
>> CONFIG_MPC831x_RDB doesn't mean that you're running on such a board,
>> only that the kernel supports those boards.  It should be a runtime
>> test.
> 
> Point taken.
> 
> If that SATA check is CPU/SOC-based, then it should be easy enough to
> test.  The cpuinfo for my board is:
> 
>   # cat /proc/cpuinfo
>   processor       : 0
>   cpu             : e300c3
>   clock           : 266.666664MHz
>   revision        : 2.0 (pvr 8085 0020)
>   bogomips        : 66.66
>   timebase        : 33333333
> 
> On the other hand, if the problem is actually caused by board trace
> routing (or other hardware that's outside the control of the CPU/SOC),
> then I don't know how possible a runtime check will be.

Board information is available from the device tree, and from platform
code that was selected based on the device tree.

> Do you know if there is a specific errata that the MPC8315_DS ran
> across that required this fix, or was it a band-aid in the first
> place?

I don't know the history of this, sorry.  It looks like Yang Li added
this code -- Yang, can you answer this?

-Scott

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Li Yang]

On Wed, May 30, 2012 at 6:57 AM, Scott Wood <scottwood@freescale.com> wrote=
:
> On 05/29/2012 05:07 PM, Anthony Foiani wrote:
>> Scott Wood <scottwood@freescale.com> writes:
>>
>>> CONFIG_MPC831x_RDB doesn't mean that you're running on such a board,
>>> only that the kernel supports those boards. =C2=A0It should be a runtim=
e
>>> test.
>>
>> Point taken.
>>
>> If that SATA check is CPU/SOC-based, then it should be easy enough to
>> test. =C2=A0The cpuinfo for my board is:
>>
>> =C2=A0 # cat /proc/cpuinfo
>> =C2=A0 processor =C2=A0 =C2=A0 =C2=A0 : 0
>> =C2=A0 cpu =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 : e300c3
>> =C2=A0 clock =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 : 266.666664MHz
>> =C2=A0 revision =C2=A0 =C2=A0 =C2=A0 =C2=A0: 2.0 (pvr 8085 0020)
>> =C2=A0 bogomips =C2=A0 =C2=A0 =C2=A0 =C2=A0: 66.66
>> =C2=A0 timebase =C2=A0 =C2=A0 =C2=A0 =C2=A0: 33333333
>>
>> On the other hand, if the problem is actually caused by board trace
>> routing (or other hardware that's outside the control of the CPU/SOC),
>> then I don't know how possible a runtime check will be.
>
> Board information is available from the device tree, and from platform
> code that was selected based on the device tree.
>
>> Do you know if there is a specific errata that the MPC8315_DS ran
>> across that required this fix, or was it a band-aid in the first
>> place?
>
> I don't know the history of this, sorry. =C2=A0It looks like Yang Li adde=
d
> this code -- Yang, can you answer this?

The original code was there before I touched the driver.  So
unfortunately I also don't know the history of the problem.  Judging
from the comment in code and current test result I guess it is a board
related issue.  I agree with Anthony that the best action for now is
to remove the workaround completely.

Leo

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Li Yang <leoli@freescale.com> writes:

> The original code was there before I touched the driver.  So
> unfortunately I also don't know the history of the problem.

Alas.

> Judging from the comment in code and current test result I guess it
> is a board related issue.

I wonder if anyone on the 8315_DS project knows where the limitation
came from, since that's the origin of the workaround...

Regardless, it's recommended by at least one vendor who based their
design on the 8315 RDB.  If it's board-related, then that seems a
reasonable conditional.

> I agree with Anthony that the best action for now is to remove the
> workaround completely.

Eeek.

I'm pretty sure that it needs to stay.  (I can't guarantee that it has
fixed my problem, but it's been a week or two without the hang, so I'm
becoming more confident).

I think the question is how to best conditionalize it.  The options
seem to be:

  1. at compile time, via kconfig bits;
  2. at runtime via probing / discovery; or
  3. at runtime via device tree.

Given that this is a relatively old platform, and only 2-3 of us have
run into this issue in 5 years, I'm inclined to just go with option 1.

That's exactly what Adrian's patch (from 2008!) does:

  http://old.nabble.com/-2.6-patch--sata_fsl.c%3A-fix-8315DS-workaround-td18807647.html

Using CONFIG_831x_RDB seems like a reasonable choice.

Anyway.  To be clear, my project is currently in good shape (by
adopting Adrian's patch) so I don't have any actual urgency for fixing
this.

I was hoping that someone might know the "correct" answer offhand, but
I honestly think that this isn't worth spending too much time on.
(But I do think that Adrian's patch is an improvement over the current
state of affairs.)

Thanks again to everyone that's chimed in.

Best regards,
Anthony Foiani

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott Wood <scottwood@freescale.com> writes:

> Board information is available from the device tree, and from
> platform code that was selected based on the device tree.

You're right, of course; I was focusing on discovery/probing, and
completely forgot about "provided information".

However, as I just mentioned in my reply to Yang, I'm pretty happy
with the kconfig solution (Adrian's patch, basically).

If we find that this is a more widespread problem, we can revisit this
discussion; but if only a handful of us have encountered this in a
5-year-old design, then I don't think it's worth the extra effort of
making it dynamic.

Maybe someone who knows devtree really well could crank that out in a
few minutes... but I'm not that person.  :)

Regardless, thanks very much for helping out on this.

I do advocate that Adrian's patch get put into place, so that we don't
have undocumented / unconnected kconfig symbols in the tree.  If we
ever do find out more details about the workaround, we can at least
add some comments at the code site.

Thanks again!

Best regards,
Anthony Foiani

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 05/30/2012 03:14 PM, Anthony Foiani wrote:
> Scott Wood <scottwood@freescale.com> writes:
> 
>> Board information is available from the device tree, and from
>> platform code that was selected based on the device tree.
> 
> You're right, of course; I was focusing on discovery/probing, and
> completely forgot about "provided information".
> 
> However, as I just mentioned in my reply to Yang, I'm pretty happy
> with the kconfig solution (Adrian's patch, basically).
> 
> If we find that this is a more widespread problem, we can revisit this
> discussion; but if only a handful of us have encountered this in a
> 5-year-old design, then I don't think it's worth the extra effort of
> making it dynamic.

We currently support building one kernel that supports a bunch of
different boards.  The hardcoding of this workaround was harmless so far
because it was conditional on a symbol that was never defined, but now
you'll be enabling this workaround on any kernel that simply has support
for mpc8315erdb.  That is not acceptable unless you show it's harmless
on all those other boards.

-Scott

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott Wood <scottwood@freescale.com> writes:

> We currently support building one kernel that supports a bunch of
> different boards.  The hardcoding of this workaround was harmless so
> far because it was conditional on a symbol that was never defined,
> but now you'll be enabling this workaround on any kernel that simply
> has support for mpc8315erdb.  That is not acceptable unless you show
> it's harmless on all those other boards.

Ok, I see your point now.  Sorry for being dense.

At the moment, I'm building a kernel that is only going to run on this
particular board, so the kconfig solution works *for me*.

Unfortunately, I'm not sure I can help develop a more generic
solution.  I can't reliably reproduce the problem, so I can't even
offer to help test for it.

Even more unfortunately, I don't currently have the bandwidth to do
any more investigation or experimenting with the devtree option (as
much as I would like to!).  At this point in my project, I probably
can't even justify trying to switch to a more current kernel, so I
couldn't try out a new release regardless.

Sorry I can't be more help.

Thanks again,
Tony

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

Apologies for resurrecting a very old thread, but...

On 05/30/2012 02:14 PM, Anthony Foiani wrote:
>
> Maybe someone who knows devtree really well could crank that out in a
> few minutes... but I'm not that person.  :)
Well, I wasn't last year, but this year I decided that I didn't care.  
Took me about an hour, not a minute, but...

Having been bitten by this config symbol disappearing one more time, 
please find attached my attempt at using information out of the device 
tree to enable this hack.

Patch is against 3.4.36 or so; hopefully upstream hasn't diverged very much.

Tested by me, so feel free to add that tag if required.

Thanks,
Anthony Foiani

[-- Attachment #2: 0001-sata-fsl-allow-device-tree-to-limit-sata-speed.patch --]
[-- Type: text/x-patch, Size: 4114 bytes --]

>From c0a85758a669b430c0a6af825e71d18a54ef88d0 Mon Sep 17 00:00:00 2001
From: Anthony Foiani <anthony.foiani@gmail.com>
Date: Mon, 29 Apr 2013 23:44:14 -0600
Subject: [PATCH] sata: fsl: allow device tree to limit sata speed.

There used to be an "orphan" config symbol (CONFIG_MPC8315_DS) that
would artificially limit SATA speed to generation 1 (1.5Gbps).

Since that config symbol got lost whenever any sort of configuration
was done, we instead extract the limitation from the device tree.

Signed-off-by: Anthony Foiani <anthony.foiani@gmail.com>
---
 .../devicetree/bindings/powerpc/fsl/board.txt      | 23 +++++++++++
 drivers/ata/sata_fsl.c                             | 44 ++++++++++++++++++----
 2 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/board.txt b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
index 380914e..6a30398 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
@@ -67,3 +67,26 @@ Example:
 			gpio-controller;
 		};
 	};
+
+* Maximum SATA Generation workaround
+
+Some boards advertise SATA speeds that they cannot actually achieve.
+Previously, this was dealt with via the orphaned config symbol
+CONFIG_MPC8315_DS.  We now have a device tree property
+"fsl,sata-max-gen" to control this.  It should live within the "sata"
+block.
+
+Example:
+
+		sata@18000 {
+			compatible = "fsl,mpc8315-sata", "fsl,pq-sata";
+			reg = <0x18000 0x1000>;
+			cell-index = <1>;
+			interrupts = <44 0x8>;
+			interrupt-parent = <&ipic>;
+			fsl,sata-max-gen = <1>;
+		};
+
+By default, there is no limitation; if a value is given, it indicates
+the maximum "generation" that should be negotiated.  Gen 1 is 1.5Gbps,
+Gen 2 is 3.0Gbps.
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index d6577b9..6d3ec47 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -274,6 +274,17 @@ struct sata_fsl_port_priv {
 };
 
 /*
+ * speed negotiation.
+ */
+
+enum {
+	SCR_SPEED_NEG_MASK	= 0xf0,
+	SCR_SPEED_NEG_UNLIMITED	= 0x00,
+	SCR_SPEED_NEG_GEN_1	= 0x10, /* 1.5Gbps max */
+	SCR_SPEED_NEG_GEN_2	= 0x20  /* 3.0Gbps max */
+};
+
+/*
  * ata_port->host_set private data
  */
 struct sata_fsl_host_priv {
@@ -282,6 +293,7 @@ struct sata_fsl_host_priv {
 	void __iomem *csr_base;
 	int irq;
 	int data_snoop;
+	u32 speed_neg;
 	struct device_attribute intr_coalescing;
 };
 
@@ -726,19 +738,23 @@ static int sata_fsl_port_start(struct ata_port *ap)
 	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
 	VPRINTK("CHBA  = 0x%x\n", ioread32(hcr_base + CHBA));
 
-#ifdef CONFIG_MPC8315_DS
 	/*
 	 * Workaround for 8315DS board 3gbps link-up issue,
 	 * currently limit SATA port to GEN1 speed
 	 */
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	temp &= ~(0xF << 4);
-	temp |= (0x1 << 4);
-	sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
+	if ( host_priv->speed_neg != SCR_SPEED_NEG_UNLIMITED )
+	{
 
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	dev_warn(dev, "scr_control, speed limited to %x\n", temp);
-#endif
+		u32 orig;
+		sata_fsl_scr_read(&ap->link, SCR_CONTROL, &orig);
+		temp = ( ( orig                 & ~SCR_SPEED_NEG_MASK ) |
+			 ( host_priv->speed_neg &  SCR_SPEED_NEG_MASK ) );
+		sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
+
+		sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
+		dev_warn(dev, "speed limited, scr_control 0x%x -> 0x%x\n",
+			 orig, temp);
+	}
 
 	return 0;
 }
@@ -1437,6 +1453,18 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 	else
 		host_priv->data_snoop = DATA_SNOOP_ENABLE_V1;
 
+	if (!of_property_read_u32(ofdev->dev.of_node, "fsl,sata-max-gen",
+				  &temp))
+	{
+		switch (temp)
+		{
+		case 1: host_priv->speed_neg = SCR_SPEED_NEG_GEN_1; break;
+		case 2: host_priv->speed_neg = SCR_SPEED_NEG_GEN_2; break;
+		}
+		dev_warn(&ofdev->dev, "speed limit set to gen %u (0x%x)\n",
+			 temp, host_priv->speed_neg);
+	}
+
 	/* allocate host structure */
 	host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS);
 	if (!host) {
-- 
1.8.1.4

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 04/30/2013 01:41:49 AM, Anthony Foiani wrote:
> Apologies for resurrecting a very old thread, but...
>=20
> On 05/30/2012 02:14 PM, Anthony Foiani wrote:
>>=20
>> Maybe someone who knows devtree really well could crank that out in a
>> few minutes... but I'm not that person.  :)
> Well, I wasn't last year, but this year I decided that I didn't =20
> care.  Took me about an hour, not a minute, but...
>=20
> Having been bitten by this config symbol disappearing one more time, =20
> please find attached my attempt at using information out of the =20
> device tree to enable this hack.
>=20
> Patch is against 3.4.36 or so; hopefully upstream hasn't diverged =20
> very much.
>=20
> Tested by me, so feel free to add that tag if required.
>=20
> Thanks,
> Anthony Foiani

------quoted attachment =20
"0001-sata-fsl-allow-device-tree-to-limit-sata-speed.patch"------
> >From c0a85758a669b430c0a6af825e71d18a54ef88d0 Mon Sep 17 00:00:00 =20
> 2001
> From: Anthony Foiani <anthony.foiani@gmail.com>
> Date: Mon, 29 Apr 2013 23:44:14 -0600
> Subject: [PATCH] sata: fsl: allow device tree to limit sata speed.
>=20
> There used to be an "orphan" config symbol (CONFIG_MPC8315_DS) that
> would artificially limit SATA speed to generation 1 (1.5Gbps).
>=20
> Since that config symbol got lost whenever any sort of configuration
> was done, we instead extract the limitation from the device tree.
>=20
> Signed-off-by: Anthony Foiani <anthony.foiani@gmail.com>
> ---
>  .../devicetree/bindings/powerpc/fsl/board.txt      | 23 +++++++++++
>  drivers/ata/sata_fsl.c                             | 44 =20
> ++++++++++++++++++----
>  2 files changed, 59 insertions(+), 8 deletions(-)
>=20
> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/board.txt =20
> b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> index 380914e..6a30398 100644
> --- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> +++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> @@ -67,3 +67,26 @@ Example:
>  			gpio-controller;
>  		};
>  	};
> +
> +* Maximum SATA Generation workaround
> +
> +Some boards advertise SATA speeds that they cannot actually achieve.
> +Previously, this was dealt with via the orphaned config symbol
> +CONFIG_MPC8315_DS.  We now have a device tree property
> +"fsl,sata-max-gen" to control this.  It should live within the "sata"
> +block.
> +
> +Example:
> +
> +		sata@18000 {
> +			compatible =3D "fsl,mpc8315-sata", "fsl,pq-sata";
> +			reg =3D <0x18000 0x1000>;
> +			cell-index =3D <1>;
> +			interrupts =3D <44 0x8>;
> +			interrupt-parent =3D <&ipic>;
> +			fsl,sata-max-gen =3D <1>;
> +		};
> +

This might be OK for a new board, but adding it now means that people =20
using existing device trees won't get the workaround.  It might be =20
better to just put the knowledge in platform code.

> +By default, there is no limitation; if a value is given, it indicates
> +the maximum "generation" that should be negotiated.  Gen 1 is =20
> 1.5Gbps,
> +Gen 2 is 3.0Gbps.
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index d6577b9..6d3ec47 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -274,6 +274,17 @@ struct sata_fsl_port_priv {
>  };
>=20
>  /*
> + * speed negotiation.
> + */
> +
> +enum {
> +	SCR_SPEED_NEG_MASK	=3D 0xf0,
> +	SCR_SPEED_NEG_UNLIMITED	=3D 0x00,
> +	SCR_SPEED_NEG_GEN_1	=3D 0x10, /* 1.5Gbps max */
> +	SCR_SPEED_NEG_GEN_2	=3D 0x20  /* 3.0Gbps max */
> +};
> +
> +/*
>   * ata_port->host_set private data
>   */
>  struct sata_fsl_host_priv {
> @@ -282,6 +293,7 @@ struct sata_fsl_host_priv {
>  	void __iomem *csr_base;
>  	int irq;
>  	int data_snoop;
> +	u32 speed_neg;
>  	struct device_attribute intr_coalescing;
>  };
>=20
> @@ -726,19 +738,23 @@ static int sata_fsl_port_start(struct ata_port =20
> *ap)
>  	VPRINTK("HControl =3D 0x%x\n", ioread32(hcr_base + HCONTROL));
>  	VPRINTK("CHBA  =3D 0x%x\n", ioread32(hcr_base + CHBA));
>=20
> -#ifdef CONFIG_MPC8315_DS
>  	/*
>  	 * Workaround for 8315DS board 3gbps link-up issue,
>  	 * currently limit SATA port to GEN1 speed
>  	 */
> -	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
> -	temp &=3D ~(0xF << 4);
> -	temp |=3D (0x1 << 4);
> -	sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
> +	if ( host_priv->speed_neg !=3D SCR_SPEED_NEG_UNLIMITED )
> +	{
>=20
> -	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
> -	dev_warn(dev, "scr_control, speed limited to %x\n", temp);
> -#endif
> +		u32 orig;
> +		sata_fsl_scr_read(&ap->link, SCR_CONTROL, &orig);
> +		temp =3D ( ( orig                 & ~SCR_SPEED_NEG_MASK ) =20
> |
> +			 ( host_priv->speed_neg &  SCR_SPEED_NEG_MASK ) =20
> );
> +		sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
> +
> +		sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
> +		dev_warn(dev, "speed limited, scr_control 0x%x -> =20
> 0x%x\n",
> +			 orig, temp);
> +	}
>=20
>  	return 0;
>  }
> @@ -1437,6 +1453,18 @@ static int sata_fsl_probe(struct =20
> platform_device *ofdev)
>  	else
>  		host_priv->data_snoop =3D DATA_SNOOP_ENABLE_V1;
>=20
> +	if (!of_property_read_u32(ofdev->dev.of_node, =20
> "fsl,sata-max-gen",
> +				  &temp))
> +	{
> +		switch (temp)
> +		{
> +		case 1: host_priv->speed_neg =3D SCR_SPEED_NEG_GEN_1; =20
> break;
> +		case 2: host_priv->speed_neg =3D SCR_SPEED_NEG_GEN_2; =20
> break;
> +		}
> +		dev_warn(&ofdev->dev, "speed limit set to gen %u =20
> (0x%x)\n",
> +			 temp, host_priv->speed_neg);
> +	}
> +
>  	/* allocate host structure */
>  	host =3D ata_host_alloc_pinfo(&ofdev->dev, ppi, =20
> SATA_FSL_MAX_PORTS);
>  	if (!host) {

Please use standard Linux coding style, and submit the patch inline =20
rather than as an attachment (e.g. use git send-email).

-Scott=

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott --

Thanks for the quick reply / review!

On 04/30/2013 12:15 PM, Scott Wood wrote:
> [The devtree approach] might be OK for a new board, but adding it now 
> means that people using existing device trees won't get the 
> workaround.  It might be better to just put the knowledge in platform 
> code.

That would be fantastic, if I knew what to test.

If you look at the thread from last year, it seems that even there in 
Freescale, nobody knows exactly what triggers this.

There was the "orphan" config value, and I simply turned that into a 
devtree switch.

If you can find an answer inside Freescale, I can try to respin the 
patch to accommodate that knowledge.  But until then, this is the best I 
can do.

(And it's not a new board -- it's based on the 8315ERDB, and the 
orphaned symbol is apparently related to another 8315 board that never 
got released?)

> Please use standard Linux coding style, 

Huh.  I thought I had.  I'll double-check.

> and submit the patch inline rather than as an attachment (e.g. use git 
> send-email).

Will see if I can do so.  I don't actually have any sort of MTA set up 
on this machine, hence these Thunderbirded messages.

Thanks again,
Anthony Foiani

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 04/30/2013 07:34:39 PM, Anthony Foiani wrote:
> Scott --
>=20
> Thanks for the quick reply / review!
>=20
> On 04/30/2013 12:15 PM, Scott Wood wrote:
>> [The devtree approach] might be OK for a new board, but adding it =20
>> now means that people using existing device trees won't get the =20
>> workaround.  It might be better to just put the knowledge in =20
>> platform code.
>=20
> That would be fantastic, if I knew what to test.
>=20
> If you look at the thread from last year, it seems that even there in =20
> Freescale, nobody knows exactly what triggers this.
>=20
> There was the "orphan" config value, and I simply turned that into a =20
> devtree switch.
>=20
> If you can find an answer inside Freescale, I can try to respin the =20
> patch to accommodate that knowledge.  But until then, this is the =20
> best I can do.
>=20
> (And it's not a new board -- it's based on the 8315ERDB, and the =20
> orphaned symbol is apparently related to another 8315 board that =20
> never got released?)

I just meant that, for whatever boards you would have put this in the =20
device tree, put it in platform code instead (if the platform file =20
supports more than one board type, then check the compatible at the top =20
of the device tree).  Or do you mean that you would not set this on any =20
board's device tree by default, and instead have users set it if they =20
encounter problems?  Is this a custom board you're seeing it on?

>> and submit the patch inline rather than as an attachment (e.g. use =20
>> git send-email).
>=20
> Will see if I can do so.  I don't actually have any sort of MTA set =20
> up on this machine, hence these Thunderbirded messages.

git send-email can connect directly to an SMTP server.

-Scott=

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott --

On 04/30/2013 06:42 PM, Scott Wood wrote:
> I just meant that, for whatever boards you would have put this in the 
> device tree, put it in platform code instead (if the platform file 
> supports more than one board type, then check the compatible at the 
> top of the device tree). 

I think I understand what you're suggesting.

Instead of a new property name, I would instead check for my specific 
board type (let's call it a foo-8315) in the top-level compatible list?  
So I'd change my devtree to have this top-level compatible:

/ {
     compatible = "example,foo-8315", "fsl,mpc8315erdb";

If I saw that, I would then twiddle the bits as needed?

MIght work, although having it in the sata block of the device tree has 
the advantage of providing me exactly the OF node that I need (in 
ofdev->dev.of_node).  I'd have to figure out how to traverse to the dev 
tree root and then back down one to the root compat entry.  Probably not 
impossible, but I was aiming for a fairly minimal patch.

It would also be nice if we could unravel exactly why that 
CONFIG_8315_DS ever showed up in the first place.  (The other "minimal" 
aspect of my patch was to try to make changes only around that one area, 
so that others could see that it was a simple change.)

> Or do you mean that you would not set this on any board's device tree 
> by default, and instead have users set it if they encounter problems? 

No, I would expect to set it on all the boards, so using the 
compatibility hack above would work.

> Is this a custom board you're seeing it on?

Not ours, but our vendor isn't very active on upstreaming things. (And 
yes, had I know that 5 years ago, I could possibly have changed vendors, 
or made upstreaming a part of the contract.  But at this point, we're 
stuck with this vendor, and they're not going to fix it; so I'm trying 
to fix it, and I'm trying to do the best job of upstreaming those fixes 
that I can.)

> git send-email can connect directly to an SMTP server.

Ok, I'll play around with that.

Thanks again,
Anthony

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 04/30/2013 09:06:56 PM, Anthony Foiani wrote:
> Scott --
>=20
> On 04/30/2013 06:42 PM, Scott Wood wrote:
>> I just meant that, for whatever boards you would have put this in =20
>> the device tree, put it in platform code instead (if the platform =20
>> file supports more than one board type, then check the compatible at =20
>> the top of the device tree).
>=20
> I think I understand what you're suggesting.
>=20
> Instead of a new property name, I would instead check for my specific =20
> board type (let's call it a foo-8315) in the top-level compatible =20
> list?  So I'd change my devtree to have this top-level compatible:
>=20
> / {
>     compatible =3D "example,foo-8315", "fsl,mpc8315erdb";

It should really only have compatible =3D "example,foo-8315", since it's =20
not 100% compatible with fsl,mpc8315erdb (at least due to this bug, but =20
probably there are other differences as well).

> If I saw that, I would then twiddle the bits as needed?

Yes.

> MIght work, although having it in the sata block of the device tree =20
> has the advantage of providing me exactly the OF node that I need (in =20
> ofdev->dev.of_node).  I'd have to figure out how to traverse to the =20
> dev tree root and then back down one to the root compat entry.  =20
> Probably not impossible, but I was aiming for a fairly minimal patch.

Well, if this is only seen on your board so far (or rather, your =20
vendor's board which isn't upstream), and you're OK with updating the =20
device tree, then I have no objection.

> It would also be nice if we could unravel exactly why that =20
> CONFIG_8315_DS ever showed up in the first place.

It would be nice, but I doubt that particular information is ever going =20
to surface...  IIRC I asked internally back when this first came up, =20
and didn't get an answer.

>> Or do you mean that you would not set this on any board's device =20
>> tree by default, and instead have users set it if they encounter =20
>> problems?
>=20
> No, I would expect to set it on all the boards, so using the =20
> compatibility hack above would work.

You mean all the boards that have the bug, which doesn't include any =20
upstream device tree, right?

-Scott=

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott --

Thanks again for the quick reply.

On 05/01/2013 12:05 PM, Scott Wood wrote:
> On 04/30/2013 09:06:56 PM, Anthony Foiani wrote:
>> Instead of a new property name, I would instead check for my specific 
>> board type (let's call it a foo-8315) in the top-level compatible 
>> list?  So I'd change my devtree to have this top-level compatible:
>>
>> / {
>>     compatible = "example,foo-8315", "fsl,mpc8315erdb";
>
> It should really only have compatible = "example,foo-8315", since it's 
> not 100% compatible with fsl,mpc8315erdb (at least due to this bug, 
> but probably there are other differences as well).

Then I guess I don't understand the proper use of "compatible" (or is 
the root node special?)

E.g., the DTS for the "parent" board (MPC8315ERDB) has multiple entries 
for the crypto "compatible" value:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/boot/dts/mpc8315erdb.dts?id=refs/tags/v3.9#n286
(or: *http://preview.tinyurl.com/btlqxgo* )

|		crypto@30000 {
			compatible = "fsl,sec3.3", "fsl,sec3.1", "fsl,sec3.0",
				     "fsl,sec2.4", "fsl,sec2.2", "fsl,sec2.1",
				     "fsl,sec2.0";
			reg = <0x30000 0x10000>;|

I read this as meaning: "if you have to ask if a certain feature is 
compatible with some 'foo', then this board provides that 
compatibility".  Not as "if a value is in the compatibility flag, then 
it is 100% compatible with that value".  (Although maybe that's true in 
the case of the SEC, so perhaps that a bad example.)

For what it's worth, the upstream vendor did have a separate root-node 
"compatible" value -- which called a board-specific function in a 
board-specific C file, both of which were direct cut & paste copies from 
the MPC8315ERDB function / file.  My gut instinct is that this degree of 
duplication is unhealthy and incorrect, but if my solution is considered 
abuse of the device tree, then I can try to do it a different way next 
time.

To clarify what I mean by "cut and paste", here are the differences 
between the 2.6.36 MPC8315ERDB dts, and the one the vendor sent us 
(based on that kernel rev):

$ diff -u arch/powerpc/boot/dts/mpc8315erdb.dts ~/foo.dts
[tony@hum linux-stable]$ --- arch/powerpc/boot/dts/mpc8315erdb.dts    
2013-05-01 17:23:36.803167646 -0600
+++ /home/tony/foo.dts    2013-05-01 17:18:55.009632505 -0600
@@ -1,4 +1,10 @@
  /*
+ * Foo 8315 Device Tree Source
+ *
+ * Copyright 2010 FooCorp
+ *
+ * Based on:
+ *
   * MPC8315E RDB Device Tree Source
   *
   * Copyright 2007 Freescale Semiconductor Inc.
@@ -12,7 +18,7 @@
  /dts-v1/;

  / {
-    compatible = "fsl,mpc8315erdb";
+    compatible = "fsl,foo-8315";
      #address-cells = <1>;
      #size-cells = <1>;

That's it.  And for the copy from mpc831x_rdb.c to foo8315.c?

--- mpc831x_rdb.c    2013-05-01 17:23:36.860167147 -0600
+++ foo8315.c    2013-05-01 17:27:04.310352475 -0600
@@ -1,5 +1,5 @@
  /*
- * arch/powerpc/platforms/83xx/mpc831x_rdb.c
+ * arch/powerpc/platforms/83xx/foo8315.c
   *
   * Description: MPC831x RDB board specific routines.
   * This file is based on mpc834x_sys.c
@@ -26,14 +26,14 @@
  /*
   * Setup the architecture
   */
-static void __init mpc831x_rdb_setup_arch(void)
+static void __init foo8315_setup_arch(void)
  {
  #ifdef CONFIG_PCI
      struct device_node *np;
  #endif

      if (ppc_md.progress)
-        ppc_md.progress("mpc831x_rdb_setup_arch()", 0);
+        ppc_md.progress("foo8315_setup_arch()", 0);

  #ifdef CONFIG_PCI
      for_each_compatible_node(np, "pci", "fsl,mpc8349-pci")
@@ -44,7 +44,7 @@
      mpc831x_usb_cfg();
  }

-static void __init mpc831x_rdb_init_IRQ(void)
+static void __init foo8315_init_IRQ(void)
  {
      struct device_node *np;

@@ -63,12 +63,12 @@
  /*
   * Called very early, MMU is off, device-tree isn't unflattened
   */
-static int __init mpc831x_rdb_probe(void)
+static int __init foo8315_probe(void)
  {
      unsigned long root = of_get_flat_dt_root();

      return of_flat_dt_is_compatible(root, "MPC8313ERDB") ||
-           of_flat_dt_is_compatible(root, "fsl,mpc8315erdb");
+           of_flat_dt_is_compatible(root, "fsl,foo8315");
  }

  static struct of_device_id __initdata of_bus_ids[] = {
@@ -83,13 +83,13 @@
      of_platform_bus_probe(NULL, of_bus_ids, NULL);
      return 0;
  }
-machine_device_initcall(mpc831x_rdb, declare_of_platform_devices);
+machine_device_initcall(foo8315, declare_of_platform_devices);

-define_machine(mpc831x_rdb) {
-    .name            = "MPC831x RDB",
-    .probe            = mpc831x_rdb_probe,
-    .setup_arch        = mpc831x_rdb_setup_arch,
-    .init_IRQ        = mpc831x_rdb_init_IRQ,
+define_machine(foo8315) {
+    .name            = "foo-8315",
+    .probe            = foo8315_probe,
+    .setup_arch        = foo8315_setup_arch,
+    .init_IRQ        = foo8315_init_IRQ,
      .get_irq        = ipic_get_irq,
      .restart        = mpc83xx_restart,
      .time_init        = mpc83xx_time_init,

Given those diffs, it didn't seem much of a stretch to use compatible = 
"fsl,mpc8315erdb"

> Well, if this is only seen on your board so far (or rather, your 
> vendor's board which isn't upstream), and you're OK with updating the 
> device tree, then I have no objection.
Well, so far as I know, this project is the only consumer of these 
boards.  I've fed all my fixes/changes back to our vendor, but they're 
not interested in upstreaming it.

I have been trying to at least get the patches into the public arena, 
but I don't have the bandwidth to move to tip-of-tree and do testing 
there, as well as moving project along the actual release path (which is 
based on long-term releases, hence my patch being against 3.4.y).

For my use, this is a good compromise: I've documented the errata (even 
if we don't know the root cause), and it is now set up so that I can't 
lose the setting just by doing a config update (which was the problem 
with the orphan config setting).

> > It would also be nice if we could unravel exactly why that 
> CONFIG_8315_DS ever showed up in the first place.
>
> It would be nice, but I doubt that particular information is ever 
> going to surface...  IIRC I asked internally back when this first came 
> up, and didn't get an answer.
Right, that's my recollection as well.

I end up feeling a bit put-upon, though, when I'm asked for a 
comprehensive patch to cover something that the original authors can't 
explain.

>
>>> Or do you mean that you would not set this on any board's device 
>>> tree by default, and instead have users set it if they encounter 
>>> problems?
>>
>> No, I would expect to set it on all the boards, so using the 
>> compatibility hack above would work.
>
> You mean all the boards that have the bug, which doesn't include any 
> upstream device tree, right?
As mentioned above, my primary concern is the use of these cards in the 
project/product I'm working on.  My answer has been to apply this fix 
(and the matching change to the device tree I supply as a part of the 
boot image).  I feel that I'm trying to do the right thing by getting 
some of these changes publicly visible, but I fear that I'll also have 
to go down the route of "not enough time or money to properly upstream it".

"doesn't include upstream device tree" ... no, the device tree was 
supplied with the original set of patches from the vendor.

And it seems like it's a bit of a corner case -- even on other MPC8315 
boards, it seems that only 2-3 of us have ever run into this.  Maybe not 
many people use the SATA features, or they use them with older/slower 
drives that never try to negotiate above 1.5Gbps (given that it's a 6yo 
design or so).

Anyway.  Thanks for all the feedback, but at this point, I'm going to go 
with the patch I already supplied.  Hopefully it will make someone 
else's life easier at some later date.

Thanks again!

Take care,
Anthony

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 05/01/2013 06:35:38 PM, Anthony Foiani wrote:
> Scott --
>=20
> Thanks again for the quick reply.
>=20
> On 05/01/2013 12:05 PM, Scott Wood wrote:
>> On 04/30/2013 09:06:56 PM, Anthony Foiani wrote:
>>> Instead of a new property name, I would instead check for my =20
>>> specific board type (let's call it a foo-8315) in the top-level =20
>>> compatible list?  So I'd change my devtree to have this top-level =20
>>> compatible:
>>>=20
>>> / {
>>>     compatible =3D "example,foo-8315", "fsl,mpc8315erdb";
>>=20
>> It should really only have compatible =3D "example,foo-8315", since =20
>> it's not 100% compatible with fsl,mpc8315erdb (at least due to this =20
>> bug, but probably there are other differences as well).
>=20
> Then I guess I don't understand the proper use of "compatible" (or is =20
> the root node special?)

It's only special in that 100% compatibility is much less likely to be =20
true of an entire system than of a particular component.

> E.g., the DTS for the "parent" board (MPC8315ERDB) has multiple =20
> entries for the crypto "compatible" value:
>=20
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch=
/powerpc/boot/dts/mpc8315erdb.dts?id=3Drefs/tags/v3.9#n286
> (or: *http://preview.tinyurl.com/btlqxgo* )
>=20
> |		crypto@30000 {
> 			compatible =3D "fsl,sec3.3", "fsl,sec3.1", =20
> "fsl,sec3.0",
> 				     "fsl,sec2.4", "fsl,sec2.2", =20
> "fsl,sec2.1",
> 				     "fsl,sec2.0";
> 			reg =3D <0x30000 0x10000>;|
>=20
> I read this as meaning: "if you have to ask if a certain feature is =20
> compatible with some 'foo', then this board provides that =20
> compatibility".  Not as "if a value is in the compatibility flag, =20
> then it is 100% compatible with that value".  (Although maybe that's =20
> true in the case of the SEC, so perhaps that a bad example.)

AFAIK there is backwards compatibility with these SEC versions.  If =20
not, they shouldn't be listed.

> For what it's worth, the upstream vendor did have a separate =20
> root-node "compatible" value -- which called a board-specific =20
> function in a board-specific C file, both of which were direct cut & =20
> paste copies from the MPC8315ERDB function / file.  My gut instinct =20
> is that this degree of duplication is unhealthy and incorrect, but if =20
> my solution is considered abuse of the device tree, then I can try to =20
> do it a different way next time.

It's quite possible to use the same C file for multiple similar boards =20
with different compatibles.  This is done often, including =20
mpc831x_erdb.c.

> Given those diffs, it didn't seem much of a stretch to use compatible =20
> =3D "fsl,mpc8315erdb"

The criteria for claiming compatibility should be based in the hardware =20
itself, not whether a particular file in Linux needs any changes.

>>>> Or do you mean that you would not set this on any board's device =20
>>>> tree by default, and instead have users set it if they encounter =20
>>>> problems?
>>>=20
>>> No, I would expect to set it on all the boards, so using the =20
>>> compatibility hack above would work.
>>=20
>> You mean all the boards that have the bug, which doesn't include any =20
>> upstream device tree, right?
> As mentioned above, my primary concern is the use of these cards in =20
> the project/product I'm working on.  My answer has been to apply this =20
> fix (and the matching change to the device tree I supply as a part of =20
> the boot image).  I feel that I'm trying to do the right thing by =20
> getting some of these changes publicly visible, but I fear that I'll =20
> also have to go down the route of "not enough time or money to =20
> properly upstream it".
>=20
> "doesn't include upstream device tree" ... no, the device tree was =20
> supplied with the original set of patches from the vendor.

I'm not saying that the device tree not being upstream is a problem -- =20
actually the opposite, that it means we don't have compatibility to =20
maintain with an already-accepted device tree.

-Scott=

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Jeff Garzik]

On 04/30/2013 02:41 AM, Anthony Foiani wrote:
> Apologies for resurrecting a very old thread, but...
>
> On 05/30/2012 02:14 PM, Anthony Foiani wrote:
>>
>> Maybe someone who knows devtree really well could crank that out in a
>> few minutes... but I'm not that person.  :)
> Well, I wasn't last year, but this year I decided that I didn't care.
> Took me about an hour, not a minute, but...
>
> Having been bitten by this config symbol disappearing one more time,
> please find attached my attempt at using information out of the device
> tree to enable this hack.
>
> Patch is against 3.4.36 or so; hopefully upstream hasn't diverged very
> much.
>
> Tested by me, so feel free to add that tag if required.

Regarding this patch:  Search for "sata_spd_limit" and xxx_spd* functions

	Jeff

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Jeff --

Thanks for the quick reply -- sorry that it took me a few days to get
back to you.

[Also, apologies if anyone gets a dupe -- I'm working on a new mail
configuration, and while I did test it, something went sideways the
first time I tried to send this.]

Jeff Garzik writes:
> Regarding this patch:  Search for "sata_spd_limit" and xxx_spd* functions

Ok, I see them, but it's not entirely clear how I'm supposed to use
them.

I think that maybe I want to set hw_sata_spd_limit in each port's link
in the ata_host structure, after ata_host_alloc_pinfo, but before
ata_host_activate.

Something like the patch at the end of this message?  It seems to have
correctly set the spd_ values, but the negotiated link speed is still
too high:

  # cd /sys/devices/e0000000.immr/e0019000.sata

  # find * -name '*_spd*' -print | xargs grep .
  ata2/link2/ata_link/link2/sata_spd:3.0 Gbps
  ata2/link2/ata_link/link2/hw_sata_spd_limit:1.5 Gbps
  ata2/link2/ata_link/link2/sata_spd_limit:1.5 Gbps

Or am I misinterpreting those values?

Maybe I need to call ata_set_sata_spd as well.  Can I do that before
discovery, or should it be a part of the port_start callback?  And if
the latter, shouldn't it be handled within the ata core, instead of
expecting each host driver to do that call?

With my original patch, I see the correct (throttled-down) value for
"sata_spd":

  # cd /sys/devices/e0000000.immr/e0019000.sata

  # find . -name '*_spd*' -print | xargs grep .
  ./ata2/link2/ata_link/link2/sata_spd:1.5 Gbps
  ./ata2/link2/ata_link/link2/hw_sata_spd_limit:1.5 Gbps
  ./ata2/link2/ata_link/link2/sata_spd_limit:1.5 Gbps

Thanks for any pointers.  Patch below.

Thanks again,
Anthony Foiani

(Pardon the extra context, but it seemed the easiest way to show how
this call slotted in between the alloc and the init.)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index d6577b9..bd24445 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -714,44 +714,30 @@ static int sata_fsl_port_start(struct ata_port *ap)
 	/*
 	 * Now, we can bring the controller on-line & also initiate
 	 * the COMINIT sequence, we simply return here and the boot-probing
 	 * & device discovery process is re-initiated by libATA using a
 	 * Softreset EH (dummy) session. Hence, boot probing and device
 	 * discovey will be part of sata_fsl_softreset() callback.
 	 */
 
 	temp = ioread32(hcr_base + HCONTROL);
 	iowrite32((temp | HCONTROL_ONLINE_PHY_RST), hcr_base + HCONTROL);
 
 	VPRINTK("HStatus = 0x%x\n", ioread32(hcr_base + HSTATUS));
 	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
 	VPRINTK("CHBA  = 0x%x\n", ioread32(hcr_base + CHBA));
 
-#ifdef CONFIG_MPC8315_DS
-	/*
-	 * Workaround for 8315DS board 3gbps link-up issue,
-	 * currently limit SATA port to GEN1 speed
-	 */
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	temp &= ~(0xF << 4);
-	temp |= (0x1 << 4);
-	sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
-
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	dev_warn(dev, "scr_control, speed limited to %x\n", temp);
-#endif
-
 	return 0;
 }
 
 static void sata_fsl_port_stop(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
 	struct sata_fsl_port_priv *pp = ap->private_data;
 	struct sata_fsl_host_priv *host_priv = ap->host->private_data;
 	void __iomem *hcr_base = host_priv->hcr_base;
 	u32 temp;
 
 	/*
 	 * Force host controller to go off-line, aborting current operations
 	 */
 	temp = ioread32(hcr_base + HCONTROL);
@@ -1432,30 +1418,39 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 	}
 	host_priv->irq = irq;
 
 	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,pq-sata-v2"))
 		host_priv->data_snoop = DATA_SNOOP_ENABLE_V2;
 	else
 		host_priv->data_snoop = DATA_SNOOP_ENABLE_V1;
 
 	/* allocate host structure */
 	host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS);
 	if (!host) {
 		retval = -ENOMEM;
 		goto error_exit_with_cleanup;
 	}
 
+	/* set speed limit if requested by device tree */
+	if (!of_property_read_u32(ofdev->dev.of_node, "sata-spd-limit",
+				  &temp)) {
+		int i;
+		for (i = 0; i < SATA_FSL_MAX_PORTS; ++i)
+			host->ports[i]->link.hw_sata_spd_limit = temp;
+		dev_warn(&ofdev->dev, "speed limit set to gen %u\n", temp);
+	}
+
 	/* host->iomap is not used currently */
 	host->private_data = host_priv;
 
 	/* initialize host controller */
 	sata_fsl_init_controller(host);
 
 	/*
 	 * Now, register with libATA core, this will also initiate the
 	 * device discovery process, invoking our port_start() handler &
 	 * error_handler() to execute a dummy Softreset EH session
 	 */
 	ata_host_activate(host, irq, sata_fsl_interrupt, SATA_FSL_IRQ_FLAG,
 			  &sata_fsl_sht);

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Anthony Foiani <tkil@scrye.com> writes:
> Maybe I need to call ata_set_sata_spd as well.  Can I do that before
> discovery, or should it be a part of the port_start callback?  And
> if the latter, shouldn't it be handled within the ata core, instead
> of expecting each host driver to do that call?

My final version calls sata_set_spd from within the hard reset
callback for the fsl sata driver.

If there's a better place to put it, please let me know.

With this patch (and an appropriate entry in the device tree), the
machine comes up and reports:

  # cd /sys/devices/e0000000.immr/e0019000.sata

  # find * -name '*_spd*' -print | xargs grep .
  ata2/link2/ata_link/link2/sata_spd:1.5 Gbps
  ata2/link2/ata_link/link2/hw_sata_spd_limit:1.5 Gbps
  ata2/link2/ata_link/link2/sata_spd_limit:1.5 Gbps

Which is what I needed to see.

Thanks for the hints!

Best regards,
Anthony Foiani
--
>From 357c96b4f31b457eca0b96147c749c21d0f4f086 Mon Sep 17 00:00:00 2001
From: Anthony Foiani <anthony.foiani@gmail.com>
Date: Wed, 8 May 2013 05:24:20 -0600
Subject: [PATCH] sata: fsl: allow device tree to limit sata speed.

There used to be an "orphan" config symbol (CONFIG_MPC8315_DS) that
would artificially limit SATA speed to generation 1 (1.5Gbps).

Since that config symbol got lost whenever any sort of configuration
was done, we instead extract the limitation from the device tree,
using a new name "sata-spd-limit".

Signed-off-by: Anthony Foiani <anthony.foiani@gmail.com>
---
 .../devicetree/bindings/powerpc/fsl/board.txt      | 23 ++++++++++++++++++
 drivers/ata/sata_fsl.c                             | 28 +++++++++++-----------
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/board.txt b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
index 380914e..9c9fed4 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
@@ -67,3 +67,26 @@ Example:
 			gpio-controller;
 		};
 	};
+
+* Maximum SATA Generation workaround
+
+Some boards advertise SATA speeds that they cannot actually achieve.
+Previously, this was dealt with via the orphaned config symbol
+CONFIG_MPC8315_DS.  We now have a device tree property
+"sata-spd-limit" to control this.  It should live within the "sata"
+block.
+
+Example:
+
+		sata@18000 {
+			compatible = "fsl,mpc8315-sata", "fsl,pq-sata";
+			reg = <0x18000 0x1000>;
+			cell-index = <1>;
+			interrupts = <44 0x8>;
+			interrupt-parent = <&ipic>;
+			sata-spd-limit = <1>;
+		};
+
+By default, there is no limitation; if a value is given, it indicates
+the maximum "generation" that should be negotiated.  Gen 1 is 1.5Gbps,
+Gen 2 is 3.0Gbps.
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index d6577b9..9e3f3ec 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -726,20 +726,6 @@ static int sata_fsl_port_start(struct ata_port *ap)
 	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
 	VPRINTK("CHBA  = 0x%x\n", ioread32(hcr_base + CHBA));
 
-#ifdef CONFIG_MPC8315_DS
-	/*
-	 * Workaround for 8315DS board 3gbps link-up issue,
-	 * currently limit SATA port to GEN1 speed
-	 */
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	temp &= ~(0xF << 4);
-	temp |= (0x1 << 4);
-	sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
-
-	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
-	dev_warn(dev, "scr_control, speed limited to %x\n", temp);
-#endif
-
 	return 0;
 }
 
@@ -836,6 +822,11 @@ try_offline_again:
 	 */
 	ata_msleep(ap, 1);
 
+	/* if the device tree forces a speed limit, set it here. */
+	ata_link_info(link, "setting speed (in hard reset)\n");
+	DPRINTK("setting spd_limit\n");
+	sata_set_spd(link);
+
 	/*
 	 * Now, bring the host controller online again, this can take time
 	 * as PHY reset and communication establishment, 1st D2H FIS and
@@ -1444,6 +1435,15 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 		goto error_exit_with_cleanup;
 	}
 
+	/* record speed limit if requested by device tree */
+	if (!of_property_read_u32(ofdev->dev.of_node, "sata-spd-limit",
+				  &temp)) {
+		int i;
+		for (i = 0; i < SATA_FSL_MAX_PORTS; ++i)
+			host->ports[i]->link.hw_sata_spd_limit = temp;
+		dev_warn(&ofdev->dev, "speed limit set to gen %u\n", temp);
+	}
+
 	/* host->iomap is not used currently */
 	host->private_data = host_priv;
 
-- 
1.8.1.4

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

Bcc: 
Subject: Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS
Reply-To: 
In-Reply-To: <g7gj9smnc.fsf@dworkin.scrye.com>

On Wed, May 08, 2013 at 06:04:39AM -0600, Anthony Foiani wrote:
> Anthony Foiani <tkil@scrye.com> writes:
> > Maybe I need to call ata_set_sata_spd as well.  Can I do that before
> > discovery, or should it be a part of the port_start callback?  And
> > if the latter, shouldn't it be handled within the ata core, instead
> > of expecting each host driver to do that call?
> 
> My final version calls sata_set_spd from within the hard reset
> callback for the fsl sata driver.
> 
> If there's a better place to put it, please let me know.
> 
> With this patch (and an appropriate entry in the device tree), the
> machine comes up and reports:
> 
>   # cd /sys/devices/e0000000.immr/e0019000.sata
> 
>   # find * -name '*_spd*' -print | xargs grep .
>   ata2/link2/ata_link/link2/sata_spd:1.5 Gbps
>   ata2/link2/ata_link/link2/hw_sata_spd_limit:1.5 Gbps
>   ata2/link2/ata_link/link2/sata_spd_limit:1.5 Gbps
> 
> Which is what I needed to see.
> 
> Thanks for the hints!
> 
> Best regards,
> Anthony Foiani
> 
> ---
> >From 357c96b4f31b457eca0b96147c749c21d0f4f086 Mon Sep 17 00:00:00 2001
> From: Anthony Foiani <anthony.foiani@gmail.com>
> Date: Wed, 8 May 2013 05:24:20 -0600
> Subject: [PATCH] sata: fsl: allow device tree to limit sata speed.
> 
> There used to be an "orphan" config symbol (CONFIG_MPC8315_DS) that
> would artificially limit SATA speed to generation 1 (1.5Gbps).
> 
> Since that config symbol got lost whenever any sort of configuration
> was done, we instead extract the limitation from the device tree,
> using a new name "sata-spd-limit".
> 
> Signed-off-by: Anthony Foiani <anthony.foiani@gmail.com>
> ---
>  .../devicetree/bindings/powerpc/fsl/board.txt      | 23 ++++++++++++++++++
>  drivers/ata/sata_fsl.c                             | 28 +++++++++++-----------
>  2 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/board.txt b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> index 380914e..9c9fed4 100644
> --- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> +++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> @@ -67,3 +67,26 @@ Example:
>  			gpio-controller;
>  		};
>  	};
> +
> +* Maximum SATA Generation workaround
> +
> +Some boards advertise SATA speeds that they cannot actually achieve.
> +Previously, this was dealt with via the orphaned config symbol
> +CONFIG_MPC8315_DS.  We now have a device tree property
> +"sata-spd-limit" to control this.  It should live within the "sata"
> +block.
> +
> +Example:
> +
> +		sata@18000 {
> +			compatible = "fsl,mpc8315-sata", "fsl,pq-sata";
> +			reg = <0x18000 0x1000>;
> +			cell-index = <1>;
> +			interrupts = <44 0x8>;
> +			interrupt-parent = <&ipic>;
> +			sata-spd-limit = <1>;
> +		};
>
> +By default, there is no limitation; if a value is given, it indicates
> +the maximum "generation" that should be negotiated.  Gen 1 is 1.5Gbps,
> +Gen 2 is 3.0Gbps.

This should go in Documentation/devicetree/bindings/ata/fsl-sata.txt.

As for the property name, I'd prefer "fsl,sata-speed-limit" or
"fsl,sata-max-generation".  Shaohui, do the driver bits look OK?

This patch should go via the linux-scsi list (note that Tejun Heo is now
the SATA maintainer).

-Scott

> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index d6577b9..9e3f3ec 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -726,20 +726,6 @@ static int sata_fsl_port_start(struct ata_port *ap)
>  	VPRINTK("HControl = 0x%x\n", ioread32(hcr_base + HCONTROL));
>  	VPRINTK("CHBA  = 0x%x\n", ioread32(hcr_base + CHBA));
>  
> -#ifdef CONFIG_MPC8315_DS
> -	/*
> -	 * Workaround for 8315DS board 3gbps link-up issue,
> -	 * currently limit SATA port to GEN1 speed
> -	 */
> -	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
> -	temp &= ~(0xF << 4);
> -	temp |= (0x1 << 4);
> -	sata_fsl_scr_write(&ap->link, SCR_CONTROL, temp);
> -
> -	sata_fsl_scr_read(&ap->link, SCR_CONTROL, &temp);
> -	dev_warn(dev, "scr_control, speed limited to %x\n", temp);
> -#endif
> -
>  	return 0;
>  }
>  
> @@ -836,6 +822,11 @@ try_offline_again:
>  	 */
>  	ata_msleep(ap, 1);
>  
> +	/* if the device tree forces a speed limit, set it here. */
> +	ata_link_info(link, "setting speed (in hard reset)\n");
> +	DPRINTK("setting spd_limit\n");
> +	sata_set_spd(link);
> +
>  	/*
>  	 * Now, bring the host controller online again, this can take time
>  	 * as PHY reset and communication establishment, 1st D2H FIS and
> @@ -1444,6 +1435,15 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>  		goto error_exit_with_cleanup;
>  	}
>  
> +	/* record speed limit if requested by device tree */
> +	if (!of_property_read_u32(ofdev->dev.of_node, "sata-spd-limit",
> +				  &temp)) {
> +		int i;
> +		for (i = 0; i < SATA_FSL_MAX_PORTS; ++i)
> +			host->ports[i]->link.hw_sata_spd_limit = temp;
> +		dev_warn(&ofdev->dev, "speed limit set to gen %u\n", temp);
> +	}
> +
>  	/* host->iomap is not used currently */
>  	host->private_data = host_priv;
>  

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott Wood <scottwood@freescale.com> writes:

>> --- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
>
> This should go in Documentation/devicetree/bindings/ata/fsl-sata.txt.

Ok, will change.

> As for the property name, I'd prefer "fsl,sata-speed-limit" or
> "fsl,sata-max-generation".  

In my original patch:

  http://article.gmane.org/gmane.linux.ports.ppc.embedded/58710

I used "fsl,sata-max-gen".  I thought Jeff disliked it, so I changed
it be more generic -- but maybe I misread his complaint.  (And while
his opinions are still respected, new maintainers might have different
tastes.)

I think my logic was that there exist "sata_spd_limit" and related
functions in the ata core, so I should mirror that in the dev tree.
No guarantees, though -- it's been a while since I wrote that code.

> Shaohui, do the driver bits look OK?

> This patch should go via the linux-scsi list (note that Tejun Heo is
> now the SATA maintainer).

linux-scsi, or linux-ide?  My other recent change to sata_fsl went
through the latter.

Thanks for the review / comments.  Let me know how you'd like to
proceed on the above points, and I can resubmit (as a proper patch for
easier tracking).

Best regards,
Anthony Foiani

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On Fri, 2013-08-23 at 17:41 -0600, Anthony Foiani wrote:
> Scott Wood <scottwood@freescale.com> writes:
> 
> >> --- a/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> >> +++ b/Documentation/devicetree/bindings/powerpc/fsl/board.txt
> >
> > This should go in Documentation/devicetree/bindings/ata/fsl-sata.txt.
> 
> Ok, will change.
> 
> > As for the property name, I'd prefer "fsl,sata-speed-limit" or
> > "fsl,sata-max-generation".  
> 
> In my original patch:
> 
>   http://article.gmane.org/gmane.linux.ports.ppc.embedded/58710
> 
> I used "fsl,sata-max-gen".  I thought Jeff disliked it, so I changed
> it be more generic -- but maybe I misread his complaint.  (And while
> his opinions are still respected, new maintainers might have different
> tastes.)

I didn't see anything to that effect from Jeff in that thread -- maybe
it was elsewhere.

> I think my logic was that there exist "sata_spd_limit" and related
> functions in the ata core, so I should mirror that in the dev tree.
> No guarantees, though -- it's been a while since I wrote that code.

The device tree describes the hardware, not the driver -- and thus
should be free to use clearer wording. :-)

As for fsl-specific versus generic, generic is fine but then it needs to
be documented in a generic place.

> > Shaohui, do the driver bits look OK?
> 
> > This patch should go via the linux-scsi list (note that Tejun Heo is
> > now the SATA maintainer).
> 
> linux-scsi, or linux-ide?  My other recent change to sata_fsl went
> through the latter.

Sorry, linux-ide.

-Scott

Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Anthony Foiani]

Scott Wood <scottwood@freescale.com> writes:

> On Fri, 2013-08-23 at 17:41 -0600, Anthony Foiani wrote:
> > In my original patch [...]  I used "fsl,sata-max-gen".  I thought
> > Jeff disliked it, so I changed it be more generic -- but maybe I
> > misread his complaint.  (And while his opinions are still
> > respected, new maintainers might have different tastes.)
>
> I didn't see anything to that effect from Jeff in that thread -- maybe
> it was elsewhere.

I think I'm referring to this message:

  http://article.gmane.org/gmane.linux.ports.ppc.embedded/58720

As he was referring me to generic methods, I inferred that I should be
providing generic knobs...

> The device tree describes the hardware, not the driver -- and thus
> should be free to use clearer wording. :-)

*nod*

> As for fsl-specific versus generic, generic is fine but then it
> needs to be documented in a generic place.

Agreed.  I actually prefer the "generation" nomenclature, as it has a
more direct/straightforward interpretation.  ("speed=1" vs
"generation=1"; the latter is a much bigger clue, IMHO.)

> Sorry, linux-ide.

Ok, thanks.

I'll wait a few days to see if there are any other comments or
concerns, then I'll spin a final version

As always, thanks for the review and insight!

Best regards,
Anthony Foiani

RE: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Xie Shaohui-B21989]

> As for the property name, I'd prefer "fsl,sata-speed-limit" or "fsl,sata-
> max-generation".  Shaohui, do the driver bits look OK?
[S.H] The driver part is OK.=20
I also tested it on p5040, the SATA worked as expected.

Best Regards,=20
Shaohui Xie