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

2013-08-27 Thread 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. 
I also tested it on p5040, the SATA worked as expected.

Best Regards, 
Shaohui Xie

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-08-24 Thread Anthony Foiani 
Scott Wood scottw...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-08-23 Thread Scott Wood 
Bcc: 
Subject: Re: ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS
Reply-To: 
In-Reply-To: g7gj9smnc@dworkin.scrye.com

On Wed, May 08, 2013 at 06:04:39AM -0600, Anthony Foiani wrote:
 Anthony Foiani t...@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/e000.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.foi...@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.foi...@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

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

2013-08-23 Thread Anthony Foiani 
Scott Wood scottw...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-08-23 Thread Scott Wood 
On Fri, 2013-08-23 at 17:41 -0600, Anthony Foiani wrote:
 Scott Wood scottw...@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



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-05-08 Thread Anthony Foiani 

Anthony Foiani t...@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/e000.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.foi...@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.foi...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-05-02 Thread 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/e000.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/e000.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

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

2013-05-01 Thread Scott Wood 

On 04/30/2013 09:06:56 PM, Anthony Foiani wrote:

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;


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



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


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.


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.


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?


-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-05-01 Thread Scott Wood 

On 05/01/2013 06:35:38 PM, Anthony Foiani wrote:

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?)


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


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@3 {
			compatible = fsl,sec3.3, fsl,sec3.1,  
fsl,sec3.0,
 fsl,sec2.4, fsl,sec2.2,  
fsl,sec2.1,

 fsl,sec2.0;
reg = 0x3 0x1;|

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


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


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.


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


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


The criteria for claiming compatibility should be based in the hardware  
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  
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.


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


-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-04-30 Thread Anthony Foiani 

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
From c0a85758a669b430c0a6af825e71d18a54ef88d0 Mon Sep 17 00:00:00 2001
From: Anthony Foiani anthony.foi...@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.foi...@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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-04-30 Thread Scott Wood 

On 04/30/2013 01:41:49 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.

Thanks,
Anthony Foiani


--quoted attachment  
0001-sata-fsl-allow-device-tree-to-limit-sata-speed.patch--
From c0a85758a669b430c0a6af825e71d18a54ef88d0 Mon Sep 17 00:00:00  
2001

From: Anthony Foiani anthony.foi...@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.foi...@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;
+   };
+


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



+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

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

2013-04-30 Thread 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




___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-04-30 Thread 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-04-30 Thread Scott Wood 

On 04/30/2013 07:34:39 PM, Anthony Foiani wrote:

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?)


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).  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?  Is this a custom board you're seeing it on?


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.


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

-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2013-04-30 Thread 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-30 Thread Li Yang 
On Wed, May 30, 2012 at 6:57 AM, Scott Wood scottw...@freescale.com wrote:
 On 05/29/2012 05:07 PM, Anthony Foiani wrote:
 Scott Wood scottw...@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.64MHz
   revision        : 2.0 (pvr 8085 0020)
   bogomips        : 66.66
   timebase        : 

 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?

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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-30 Thread Anthony Foiani 
Li Yang le...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-30 Thread Anthony Foiani 
Scott Wood scottw...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-30 Thread Scott Wood 
On 05/30/2012 03:14 PM, Anthony Foiani wrote:
 Scott Wood scottw...@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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-30 Thread Anthony Foiani 
Scott Wood scottw...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-29 Thread Scott Wood 
On 05/26/2012 01:53 AM, Anthony Foiani wrote:
 Li Yang-R58472 r58...@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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-29 Thread Anthony Foiani 
Scott Wood scottw...@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.64MHz
  revision: 2.0 (pvr 8085 0020)
  bogomips: 66.66
  timebase: 

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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-29 Thread Scott Wood 
On 05/29/2012 05:07 PM, Anthony Foiani wrote:
 Scott Wood scottw...@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.64MHz
   revision: 2.0 (pvr 8085 0020)
   bogomips: 66.66
   timebase: 
 
 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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-26 Thread Anthony Foiani 
Li Yang-R58472 r58...@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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2012-05-21 Thread Li Yang-R58472 


 -Original Message-
 From: Anthony Foiani [mailto:t...@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
 
 
 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.ka...@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.ka...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
Signed-off-by: Jeff Garzik j...@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-idem=121783965216004w=2
 
 As Robert notied again in 2010, it still wasn't mainlined:
 
http://marc.info/?l=linux-idem=121783965216004w=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 for bringing it up again.  Looks like we do have a problem here.

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

Leo

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev