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

[Nikita Kiryanov <nikita@compulab.co.il>]

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