[U-Boot] [PATCH] dwc_ahsata.c: Add weak disable_sata_clock

[U-Boot] [PATCH] dwc_ahsata.c: Add weak disable_sata_clock

[Tom Rini]

Signed-off-by: Tom Rini <trini@ti.com>
---
 drivers/block/dwc_ahsata.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
index 9a2b547..e9d4283 100644
--- a/drivers/block/dwc_ahsata.c
+++ b/drivers/block/dwc_ahsata.c
@@ -16,6 +16,7 @@
 #include <asm/errno.h>
 #include <asm/io.h>
 #include <linux/bitops.h>
+#include <linux/compiler.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/sys_proto.h>
 #include "dwc_ahsata.h"
@@ -592,6 +593,10 @@ int init_sata(int dev)
 	return 0;
 }
 
+__weak void  disable_sata_clock(void)
+{
+}
+
 int reset_sata(int dev)
 {
 	struct ahci_probe_ent *probe_ent =
-- 
1.7.9.5

[U-Boot] [PATCH] dwc_ahsata.c: Add weak disable_sata_clock

[Soeren Moch]

On 11/25/14 23:34, Soeren Moch wrote:
> > Signed-off-by: Tom Rini <trini@ti.com  <http://lists.denx.de/mailman/listinfo/u-boot>>
> > ---
> > drivers/block/dwc_ahsata.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
> > index 9a2b547..e9d4283 100644
> > --- a/drivers/block/dwc_ahsata.c
> > +++ b/drivers/block/dwc_ahsata.c
> > @@ -16,6 +16,7 @@
> >  #include <asm/errno.h>
> >  #include <asm/io.h>
> >  #include <linux/bitops.h>
> > +#include <linux/compiler.h>
> >  #include <asm/arch/clock.h>
> >  #include <asm/arch/sys_proto.h>
> >  #include "dwc_ahsata.h"
> > @@ -592,6 +593,10 @@ int init_sata(int dev)
> >  	return 0;
> >  }
> >
> > +__weak void  disable_sata_clock(void)
> > +{
> > +}
> > +
> >  int reset_sata(int dev)
> >  {
> >  	struct ahci_probe_ent *probe_ent =
>
> Tom, Nikita,
>
> instead of adding a weak function for architectures without 'disable_sata_clock',
> should we remove this call from reset_sata entirely?
>
> 'reset_sata' is called repeatedly for several devices, but 'disable_sata_clock'
> has no such device parameter. Which clock should be disabled here? Makes not much
> sense for me.
>
> BTW, there is an additional problem with 'reset_sata'. If sata support is configured
> into u-boot, but nobody has called 'sata init' before booting the kernel, I see a
> data abort exception on bootm. Tested on TBS2910 board (i.MX6Q-based).
>
> Regards,
> Soeren
Forgot to add the u-boot list, sorry. Please ignore the previous email with wrong sender address.

[U-Boot] [PATCH] dwc_ahsata.c: Add weak disable_sata_clock

[Tom Rini]

On Tue, Nov 25, 2014 at 11:50:41PM +0100, Soeren Moch wrote:
> On 11/25/14 23:34, Soeren Moch wrote:
> >> Signed-off-by: Tom Rini <trini@ti.com  <http://lists.denx.de/mailman/listinfo/u-boot>>
> >> ---
> >> drivers/block/dwc_ahsata.c |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
> >> index 9a2b547..e9d4283 100644
> >> --- a/drivers/block/dwc_ahsata.c
> >> +++ b/drivers/block/dwc_ahsata.c
> >> @@ -16,6 +16,7 @@
> >>  #include <asm/errno.h>
> >>  #include <asm/io.h>
> >>  #include <linux/bitops.h>
> >> +#include <linux/compiler.h>
> >>  #include <asm/arch/clock.h>
> >>  #include <asm/arch/sys_proto.h>
> >>  #include "dwc_ahsata.h"
> >> @@ -592,6 +593,10 @@ int init_sata(int dev)
> >>  	return 0;
> >>  }
> >>
> >> +__weak void  disable_sata_clock(void)
> >> +{
> >> +}
> >> +
> >>  int reset_sata(int dev)
> >>  {
> >>  	struct ahci_probe_ent *probe_ent =
> >
> >Tom, Nikita,
> >
> >instead of adding a weak function for architectures without 'disable_sata_clock',
> >should we remove this call from reset_sata entirely?
> >
> >'reset_sata' is called repeatedly for several devices, but 'disable_sata_clock'
> >has no such device parameter. Which clock should be disabled here? Makes not much
> >sense for me.
> >
> >BTW, there is an additional problem with 'reset_sata'. If sata support is configured
> >into u-boot, but nobody has called 'sata init' before booting the kernel, I see a
> >data abort exception on bootm. Tested on TBS2910 board (i.MX6Q-based).

I'm fine with reverting the original patch too, which sounds like the
best case here.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141125/c935952e/attachment.pgp>

[U-Boot] [PATCH] dwc_ahsata.c: Add weak disable_sata_clock

[Soeren Moch]

On 26.11.2014 01:26, Tom Rini wrote:
> On Tue, Nov 25, 2014 at 11:50:41PM +0100, Soeren Moch wrote:
>> On 11/25/14 23:34, Soeren Moch wrote:
>>>> Signed-off-by: Tom Rini <trini@ti.com
>>>> <http://lists.denx.de/mailman/listinfo/u-boot>> --- 
>>>> drivers/block/dwc_ahsata.c |    5 +++++ 1 file changed, 5
>>>> insertions(+)
>>>> 
>>>> diff --git a/drivers/block/dwc_ahsata.c
>>>> b/drivers/block/dwc_ahsata.c index 9a2b547..e9d4283 100644 
>>>> --- a/drivers/block/dwc_ahsata.c +++
>>>> b/drivers/block/dwc_ahsata.c @@ -16,6 +16,7 @@ #include
>>>> <asm/errno.h> #include <asm/io.h> #include <linux/bitops.h> 
>>>> +#include <linux/compiler.h> #include <asm/arch/clock.h> 
>>>> #include <asm/arch/sys_proto.h> #include "dwc_ahsata.h" @@
>>>> -592,6 +593,10 @@ int init_sata(int dev) return 0; }
>>>> 
>>>> +__weak void  disable_sata_clock(void) +{ +} + int
>>>> reset_sata(int dev) { struct ahci_probe_ent *probe_ent =
>>> 
>>> Tom, Nikita,
>>> 
>>> instead of adding a weak function for architectures without
>>> 'disable_sata_clock', should we remove this call from
>>> reset_sata entirely?
>>> 
>>> 'reset_sata' is called repeatedly for several devices, but
>>> 'disable_sata_clock' has no such device parameter. Which clock
>>> should be disabled here? Makes not much sense for me.
>>> 
>>> BTW, there is an additional problem with 'reset_sata'. If sata
>>> support is configured into u-boot, but nobody has called 'sata
>>> init' before booting the kernel, I see a data abort exception
>>> on bootm. Tested on TBS2910 board (i.MX6Q-based).
> 
> I'm fine with reverting the original patch too, which sounds like
> the best case here.
> 

The original patch is part of a series which as a whole makes sense, I
think. Tomorrow I can provide a patch to fix this behavior - it is not
so easy to test patches currently, as u-boot-2015.01-rc2 is broken on
armhf.

  Soeren

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Soeren Moch]

- fix crash when sata device is not initialized
- remove disable_sata_clock() since it is not clear which clock for which
  device should be disabled here
- call disable_sata_clock() for mx6 in preboot_os instead

Signed-off-by: Soeren Moch <smoch@web.de>
---
Cc: Tom Rini <trini@ti.com>
Cc: Nikita Kiryanov <nikita@compulab.co.il>
Cc: Simon Glass <sjg@chromium.org>
Cc: guillaume.gardet at free.fr
Cc: Stefano Babic <sbabic@denx.de>
---
 arch/arm/imx-common/cpu.c  |  3 +++
 drivers/block/dwc_ahsata.c | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
index b58df7d..28ccd29 100644
--- a/arch/arm/imx-common/cpu.c
+++ b/arch/arm/imx-common/cpu.c
@@ -206,6 +206,9 @@ void arch_preboot_os(void)
 {
 #if defined(CONFIG_CMD_SATA)
 	sata_stop();
+#if defined(CONFIG_MX6)
+	disable_sata_clock();
+#endif
 #endif
 #if defined(CONFIG_VIDEO_IPUV3)
 	/* disable video before launching O/S */
diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
index 9a2b547..9e925d1 100644
--- a/drivers/block/dwc_ahsata.c
+++ b/drivers/block/dwc_ahsata.c
@@ -594,22 +594,24 @@ int init_sata(int dev)
 
 int reset_sata(int dev)
 {
-	struct ahci_probe_ent *probe_ent =
-			(struct ahci_probe_ent *)sata_dev_desc[dev].priv;
-	struct sata_host_regs *host_mmio =
-			(struct sata_host_regs *)probe_ent->mmio_base;
+	struct ahci_probe_ent *probe_ent;
+	struct sata_host_regs *host_mmio;
 
 	if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
 		printf("The sata index %d is out of ranges\n\r", dev);
 		return -1;
 	}
 
+	probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
+	if (NULL == probe_ent)
+		/* not initialized, so nothing to reset */
+		return 0;
+
+	host_mmio = probe_ent->mmio_base;
 	setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
 	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
 		udelay(100);
 
-	disable_sata_clock();
-
 	return 0;
 }
 
-- 
1.9.1

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Nikita Kiryanov]

Hi Soeren,

On 11/26/2014 01:40 PM, Soeren Moch wrote:
> - fix crash when sata device is not initialized
> - remove disable_sata_clock() since it is not clear which clock for which
>    device should be disabled here
> - call disable_sata_clock() for mx6 in preboot_os instead
>
> Signed-off-by: Soeren Moch <smoch@web.de>
> ---
> Cc: Tom Rini <trini@ti.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: guillaume.gardet at free.fr
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>   arch/arm/imx-common/cpu.c  |  3 +++
>   drivers/block/dwc_ahsata.c | 14 ++++++++------
>   2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index b58df7d..28ccd29 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -206,6 +206,9 @@ void arch_preboot_os(void)
>   {
>   #if defined(CONFIG_CMD_SATA)
>   	sata_stop();
> +#if defined(CONFIG_MX6)
> +	disable_sata_clock();
> +#endif

I think a more proper way to do this might be to mirror the
init sequence completely. The simplest init sequence calls
setup_sata() somewhere before sata_initialize() is invoked,
and the distribution of labor is:
- arch/arm/imx-common/sata.c:setup_sata() Turns on clocks
- common/cmd_sata.c: sata_initialize() Calls driver init

The inverse would be:
- common/cmd_sata.c: sata_stop() Calls driver reset
- arch/arm/imx-common/sata.c:disable_sata() Turns off clocks

Thus disable_sata() would need to be defined in
arch/arm/imx-common/sata.c; it will call disable_sata_clock(),
and be used in cm_fx6's version of stop_sata().

It's a little convoluted, but consistent with the existing code
flow, and it also fixes the MX53 problems because
arch/arm/imx-common/sata.c is not compiled for this platform.

>   #endif
>   #if defined(CONFIG_VIDEO_IPUV3)
>   	/* disable video before launching O/S */
> diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
> index 9a2b547..9e925d1 100644
> --- a/drivers/block/dwc_ahsata.c
> +++ b/drivers/block/dwc_ahsata.c
> @@ -594,22 +594,24 @@ int init_sata(int dev)
>
>   int reset_sata(int dev)
>   {
> -	struct ahci_probe_ent *probe_ent =
> -			(struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> -	struct sata_host_regs *host_mmio =
> -			(struct sata_host_regs *)probe_ent->mmio_base;
> +	struct ahci_probe_ent *probe_ent;
> +	struct sata_host_regs *host_mmio;
>
>   	if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
>   		printf("The sata index %d is out of ranges\n\r", dev);
>   		return -1;
>   	}
>
> +	probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> +	if (NULL == probe_ent)
> +		/* not initialized, so nothing to reset */
> +		return 0;
> +
> +	host_mmio = probe_ent->mmio_base;

The lack of casting generates a warning during compilation:
drivers/block/dwc_ahsata.c:610:12: warning: assignment makes pointer 
from integer without a cast [enabled by default]
   host_mmio = probe_ent->mmio_base;
             ^

>   	setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
>   	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
>   		udelay(100);
>
> -	disable_sata_clock();
> -
>   	return 0;
>   }
>
>

-- 
Regards,
Nikita Kiryanov

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Soeren Moch]

On 26.11.2014 18:38, Nikita Kiryanov wrote:
> Hi Soeren,
> 
> On 11/26/2014 01:40 PM, Soeren Moch wrote:
>> - fix crash when sata device is not initialized
>> - remove disable_sata_clock() since it is not clear which clock for which
>>    device should be disabled here
>> - call disable_sata_clock() for mx6 in preboot_os instead
>>
>> Signed-off-by: Soeren Moch <smoch@web.de>
>> ---
>> Cc: Tom Rini <trini@ti.com>
>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: guillaume.gardet at free.fr
>> Cc: Stefano Babic <sbabic@denx.de>
>> ---
>>   arch/arm/imx-common/cpu.c  |  3 +++
>>   drivers/block/dwc_ahsata.c | 14 ++++++++------
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>> index b58df7d..28ccd29 100644
>> --- a/arch/arm/imx-common/cpu.c
>> +++ b/arch/arm/imx-common/cpu.c
>> @@ -206,6 +206,9 @@ void arch_preboot_os(void)
>>   {
>>   #if defined(CONFIG_CMD_SATA)
>>       sata_stop();
>> +#if defined(CONFIG_MX6)
>> +    disable_sata_clock();
>> +#endif
> 
> I think a more proper way to do this might be to mirror the
> init sequence completely. The simplest init sequence calls
> setup_sata() somewhere before sata_initialize() is invoked,
> and the distribution of labor is:
> - arch/arm/imx-common/sata.c:setup_sata() Turns on clocks
> - common/cmd_sata.c: sata_initialize() Calls driver init
> 
> The inverse would be:
> - common/cmd_sata.c: sata_stop() Calls driver reset
> - arch/arm/imx-common/sata.c:disable_sata() Turns off clocks
> 
> Thus disable_sata() would need to be defined in
> arch/arm/imx-common/sata.c; it will call disable_sata_clock(),
> and be used in cm_fx6's version of stop_sata().
> 
> It's a little convoluted, but consistent with the existing code
> flow, and it also fixes the MX53 problems because
> arch/arm/imx-common/sata.c is not compiled for this platform.

Nikita,

if I remember correctly, the whole purpose of this patch series was to
get a SSD running, which was not recognized by linux if sata is enabled
on boot. So for me disabling the sata link in u-boot makes perfectly
sense to overcome this issue.
But I cannot imagine that disabling the clock of the sata controller
makes any difference here. But I did not test this (I have no access to
such problematic SSD).
I would prefer to keep the code simple and easy to maintain, I don't see
any advantage in mirroring the init sequence on exit. I would not switch
off the sata controller clock. I'm even not sure that my patch would
work for i.MX6SX, if I further think about it. It is already not easy to
understand all implications.
But this is only my personal opinion, I'm not the maintainer of this
code. My only goal with this patch was to get sata running again, for
boards which enable sata (call setup_sata() in board_init() ), but do
not run 'sata init' in the bootcmd by default - and for other
architectures than mx6.

So how to proceed is a question for the maintainers now, I think.
I see following opportunities:
- implement the clock disabling framework as Nikita has described above
- try to get my patch working with minimal changes of the merged code
  (as we are in -rc2 phase of development),
  and possibly apply additional patches from Nikita on this later
- revert the whole patch series

> 
>>   #endif
>>   #if defined(CONFIG_VIDEO_IPUV3)
>>       /* disable video before launching O/S */
>> diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
>> index 9a2b547..9e925d1 100644
>> --- a/drivers/block/dwc_ahsata.c
>> +++ b/drivers/block/dwc_ahsata.c
>> @@ -594,22 +594,24 @@ int init_sata(int dev)
>>
>>   int reset_sata(int dev)
>>   {
>> -    struct ahci_probe_ent *probe_ent =
>> -            (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
>> -    struct sata_host_regs *host_mmio =
>> -            (struct sata_host_regs *)probe_ent->mmio_base;
>> +    struct ahci_probe_ent *probe_ent;
>> +    struct sata_host_regs *host_mmio;
>>
>>       if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
>>           printf("The sata index %d is out of ranges\n\r", dev);
>>           return -1;
>>       }
>>
>> +    probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
>> +    if (NULL == probe_ent)
>> +        /* not initialized, so nothing to reset */
>> +        return 0;
>> +
>> +    host_mmio = probe_ent->mmio_base;
> 
> The lack of casting generates a warning during compilation:
> drivers/block/dwc_ahsata.c:610:12: warning: assignment makes pointer
> from integer without a cast [enabled by default]
>   host_mmio = probe_ent->mmio_base;
>             ^
> 

Of course you are right here, I accidentally removed the type cast. If
we decide to go on with this patch, I will fix this in a new patch version.

  Soeren

>>       setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
>>       while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
>>           udelay(100);
>>
>> -    disable_sata_clock();
>> -
>>       return 0;
>>   }
>>
>>
> 

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Fabio Estevam]

Hi Soeren,

On Wed, Nov 26, 2014 at 9:32 PM, Soeren Moch <smoch@web.de> wrote:

> off the sata controller clock. I'm even not sure that my patch would
> work for i.MX6SX, if I further think about it. It is already not easy to

No need to worry about it: there is no sata on solox ;-)

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Soeren Moch]

Hi Fabio,

On 27.11.2014 02:12, Fabio Estevam wrote:
> Hi Soeren,
> 
> On Wed, Nov 26, 2014 at 9:32 PM, Soeren Moch <smoch@web.de> wrote:
> 
>> off the sata controller clock. I'm even not sure that my patch would
>> work for i.MX6SX, if I further think about it. It is already not easy to
> 
> No need to worry about it: there is no sata on solox ;-)
> 

Of course, but does the code compile for it?
OK, probably yes, because nobody would define CMD_SATA for such board.

Even better if you think the patch will work for all mx6 variants. Maybe
I should send a second version of the patch with fixed typecast.

  Soeren

[U-Boot] [PATCH v2] sata: fix reset_sata for dwc_ahsata

[Soeren Moch]

- fix crash when sata device is not initialized
- remove disable_sata_clock() since it is not clear which clock for which
  device should be disabled here
- call disable_sata_clock() for mx6 in preboot_os instead

Signed-off-by: Soeren Moch <smoch@web.de>
---
Changes in v2:
- fix type cast warning

Cc: Tom Rini <trini@ti.com>
Cc: Nikita Kiryanov <nikita@compulab.co.il>
Cc: Simon Glass <sjg@chromium.org>
Cc: guillaume.gardet at free.fr
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 arch/arm/imx-common/cpu.c  |  3 +++
 drivers/block/dwc_ahsata.c | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
index b58df7d..28ccd29 100644
--- a/arch/arm/imx-common/cpu.c
+++ b/arch/arm/imx-common/cpu.c
@@ -206,6 +206,9 @@ void arch_preboot_os(void)
 {
 #if defined(CONFIG_CMD_SATA)
 	sata_stop();
+#if defined(CONFIG_MX6)
+	disable_sata_clock();
+#endif
 #endif
 #if defined(CONFIG_VIDEO_IPUV3)
 	/* disable video before launching O/S */
diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
index 9a2b547..01a4148 100644
--- a/drivers/block/dwc_ahsata.c
+++ b/drivers/block/dwc_ahsata.c
@@ -594,22 +594,24 @@ int init_sata(int dev)
 
 int reset_sata(int dev)
 {
-	struct ahci_probe_ent *probe_ent =
-			(struct ahci_probe_ent *)sata_dev_desc[dev].priv;
-	struct sata_host_regs *host_mmio =
-			(struct sata_host_regs *)probe_ent->mmio_base;
+	struct ahci_probe_ent *probe_ent;
+	struct sata_host_regs *host_mmio;
 
 	if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
 		printf("The sata index %d is out of ranges\n\r", dev);
 		return -1;
 	}
 
+	probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
+	if (NULL == probe_ent)
+		/* not initialized, so nothing to reset */
+		return 0;
+
+	host_mmio = (struct sata_host_regs *)probe_ent->mmio_base;
 	setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
 	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
 		udelay(100);
 
-	disable_sata_clock();
-
 	return 0;
 }
 
-- 
1.9.1

[U-Boot] [PATCH] sata: fix reset_sata for dwc_ahsata

[Nikita Kiryanov]

On 11/27/2014 01:32 AM, Soeren Moch wrote:
> On 26.11.2014 18:38, Nikita Kiryanov wrote:
>> Hi Soeren,
>>
>> On 11/26/2014 01:40 PM, Soeren Moch wrote:
>>> - fix crash when sata device is not initialized
>>> - remove disable_sata_clock() since it is not clear which clock for which
>>>     device should be disabled here
>>> - call disable_sata_clock() for mx6 in preboot_os instead
>>>
>>> Signed-off-by: Soeren Moch <smoch@web.de>
>>> ---
>>> Cc: Tom Rini <trini@ti.com>
>>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: guillaume.gardet at free.fr
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> ---
>>>    arch/arm/imx-common/cpu.c  |  3 +++
>>>    drivers/block/dwc_ahsata.c | 14 ++++++++------
>>>    2 files changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>>> index b58df7d..28ccd29 100644
>>> --- a/arch/arm/imx-common/cpu.c
>>> +++ b/arch/arm/imx-common/cpu.c
>>> @@ -206,6 +206,9 @@ void arch_preboot_os(void)
>>>    {
>>>    #if defined(CONFIG_CMD_SATA)
>>>        sata_stop();
>>> +#if defined(CONFIG_MX6)
>>> +    disable_sata_clock();
>>> +#endif
>>
>> I think a more proper way to do this might be to mirror the
>> init sequence completely. The simplest init sequence calls
>> setup_sata() somewhere before sata_initialize() is invoked,
>> and the distribution of labor is:
>> - arch/arm/imx-common/sata.c:setup_sata() Turns on clocks
>> - common/cmd_sata.c: sata_initialize() Calls driver init
>>
>> The inverse would be:
>> - common/cmd_sata.c: sata_stop() Calls driver reset
>> - arch/arm/imx-common/sata.c:disable_sata() Turns off clocks
>>
>> Thus disable_sata() would need to be defined in
>> arch/arm/imx-common/sata.c; it will call disable_sata_clock(),
>> and be used in cm_fx6's version of stop_sata().
>>
>> It's a little convoluted, but consistent with the existing code
>> flow, and it also fixes the MX53 problems because
>> arch/arm/imx-common/sata.c is not compiled for this platform.
>
> Nikita,
>
> if I remember correctly, the whole purpose of this patch series was to
> get a SSD running, which was not recognized by linux if sata is enabled
> on boot. So for me disabling the sata link in u-boot makes perfectly
> sense to overcome this issue.
> But I cannot imagine that disabling the clock of the sata controller
> makes any difference here. But I did not test this (I have no access to
> such problematic SSD).
> I would prefer to keep the code simple and easy to maintain, I don't see
> any advantage in mirroring the init sequence on exit. I would not switch
> off the sata controller clock. I'm even not sure that my patch would
> work for i.MX6SX, if I further think about it. It is already not easy to
> understand all implications.
> But this is only my personal opinion, I'm not the maintainer of this
> code. My only goal with this patch was to get sata running again, for
> boards which enable sata (call setup_sata() in board_init() ), but do
> not run 'sata init' in the bootcmd by default - and for other
> architectures than mx6.
>
> So how to proceed is a question for the maintainers now, I think.
> I see following opportunities:
> - implement the clock disabling framework as Nikita has described above
> - try to get my patch working with minimal changes of the merged code
>    (as we are in -rc2 phase of development),
>    and possibly apply additional patches from Nikita on this later

Well, mostly I just wanted to put this idea up for discussion. I'm fine
with first applying your fix and later making these changes, or even not
making them at all if people think that being consistent with the init
sequence is not worth it.

-- 
Regards,
Nikita Kiryanov

[U-Boot] [PATCH v2] sata: fix reset_sata for dwc_ahsata

[Nikita Kiryanov]

Hi Soeren,

On 11/27/2014 11:11 AM, Soeren Moch wrote:
> - fix crash when sata device is not initialized
> - remove disable_sata_clock() since it is not clear which clock for which
>    device should be disabled here
> - call disable_sata_clock() for mx6 in preboot_os instead
>
> Signed-off-by: Soeren Moch <smoch@web.de>
> ---
> Changes in v2:
> - fix type cast warning
>
> Cc: Tom Rini <trini@ti.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: guillaume.gardet at free.fr
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>   arch/arm/imx-common/cpu.c  |  3 +++
>   drivers/block/dwc_ahsata.c | 14 ++++++++------
>   2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index b58df7d..28ccd29 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -206,6 +206,9 @@ void arch_preboot_os(void)
>   {
>   #if defined(CONFIG_CMD_SATA)
>   	sata_stop();
> +#if defined(CONFIG_MX6)
> +	disable_sata_clock();
> +#endif
>   #endif
>   #if defined(CONFIG_VIDEO_IPUV3)
>   	/* disable video before launching O/S */
> diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
> index 9a2b547..01a4148 100644
> --- a/drivers/block/dwc_ahsata.c
> +++ b/drivers/block/dwc_ahsata.c
> @@ -594,22 +594,24 @@ int init_sata(int dev)
>
>   int reset_sata(int dev)
>   {
> -	struct ahci_probe_ent *probe_ent =
> -			(struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> -	struct sata_host_regs *host_mmio =
> -			(struct sata_host_regs *)probe_ent->mmio_base;
> +	struct ahci_probe_ent *probe_ent;
> +	struct sata_host_regs *host_mmio;
>
>   	if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
>   		printf("The sata index %d is out of ranges\n\r", dev);
>   		return -1;
>   	}
>
> +	probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> +	if (NULL == probe_ent)
> +		/* not initialized, so nothing to reset */
> +		return 0;
> +
> +	host_mmio = (struct sata_host_regs *)probe_ent->mmio_base;
>   	setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
>   	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
>   		udelay(100);
>
> -	disable_sata_clock();
> -
>   	return 0;
>   }
>
>

Acked-by: Nikita Kiryanov <nikita@compulab.co.il>
Tested-by: Nikita Kiryanov <nikita@compulab.co.il>

-- 
Regards,
Nikita Kiryanov

[U-Boot] [PATCH v2] sata: fix reset_sata for dwc_ahsata

[Stefano Babic]

On 27/11/2014 10:11, Soeren Moch wrote:
> - fix crash when sata device is not initialized
> - remove disable_sata_clock() since it is not clear which clock for which
>   device should be disabled here
> - call disable_sata_clock() for mx6 in preboot_os instead
> 
> Signed-off-by: Soeren Moch <smoch@web.de>
> ---
> Changes in v2:
> - fix type cast warning
> 
> Cc: Tom Rini <trini@ti.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: guillaume.gardet at free.fr
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  arch/arm/imx-common/cpu.c  |  3 +++
>  drivers/block/dwc_ahsata.c | 14 ++++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index b58df7d..28ccd29 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -206,6 +206,9 @@ void arch_preboot_os(void)
>  {
>  #if defined(CONFIG_CMD_SATA)
>  	sata_stop();
> +#if defined(CONFIG_MX6)
> +	disable_sata_clock();
> +#endif
>  #endif
>  #if defined(CONFIG_VIDEO_IPUV3)
>  	/* disable video before launching O/S */
> diff --git a/drivers/block/dwc_ahsata.c b/drivers/block/dwc_ahsata.c
> index 9a2b547..01a4148 100644
> --- a/drivers/block/dwc_ahsata.c
> +++ b/drivers/block/dwc_ahsata.c
> @@ -594,22 +594,24 @@ int init_sata(int dev)
>  
>  int reset_sata(int dev)
>  {
> -	struct ahci_probe_ent *probe_ent =
> -			(struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> -	struct sata_host_regs *host_mmio =
> -			(struct sata_host_regs *)probe_ent->mmio_base;
> +	struct ahci_probe_ent *probe_ent;
> +	struct sata_host_regs *host_mmio;
>  
>  	if (dev < 0 || dev > (CONFIG_SYS_SATA_MAX_DEVICE - 1)) {
>  		printf("The sata index %d is out of ranges\n\r", dev);
>  		return -1;
>  	}
>  
> +	probe_ent = (struct ahci_probe_ent *)sata_dev_desc[dev].priv;
> +	if (NULL == probe_ent)
> +		/* not initialized, so nothing to reset */
> +		return 0;
> +
> +	host_mmio = (struct sata_host_regs *)probe_ent->mmio_base;
>  	setbits_le32(&host_mmio->ghc, SATA_HOST_GHC_HR);
>  	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
>  		udelay(100);
>  
> -	disable_sata_clock();
> -
>  	return 0;
>  }
>  
> 

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================