On 03/02/2014 17:50, Jason Cooper wrote:
>
> + Kevin Hilman (context kept for Kevin)
>
> Tejun, a request below.
>
> On Mon, Feb 03, 2014 at 04:32:46PM +0100, Gregory CLEMENT wrote:
>> Hi Andrew, Ezequiel,
>>
>> On 31/01/2014 11:54, Andrew Lunn wrote:
>>> On Thu, Jan 30, 2014 at 07:12:28PM -0300, Ezequiel Garcia wrote:
>>>> On Thu, Jan 30, 2014 at 09:50:35PM +0100, Andrew Lunn wrote:
>>>>> Armada 370 and XP do not have a SATA phy driver. The generic phy
>>>>> layer does not cleanly support optional phys. It is not possible to
>>>>> determine from the error code if there is expected to be a phy
>>>>> according to DT, but it cannot be found, or no phy is listed in
>>>>> DT. All that can be determined is that a phy is expected, but the
>>>>> driver has not been loaded yet, in which case -EPROBE_DEFER is
>>>>> returned. Thus for 370 and XP the driver failed to probe. Play safe,
>>>>> consider all errors except -EPROBE_DEFER to be none fatal and keep
>>>>> going, and in the case of -EPROBE_DEFER exit the probe function with
>>>>> that error code.
>>>>>
>>>>> Tested on Kirkwood with a sata phy driver and on 370 without a sata
>>>>> phy driver.
>>
>> As expected kernel fails booting on Armada 370 and Armada XP when SATA
>> is selected (so by default with mvebu_defconfig and multi_v7_defconfig)
>> on 3.14-rc1. I would realy like to see this issue fixed for 3.14-rc2.
>>
>>>>>
>>>>> Reported-by: Jean Pihet <jean.pihet@xxxxxxxxxx>
>>>>> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
>>>>> Tested-by: Gregory Clement <gregory.clement@xxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/ata/sata_mv.c | 5 ++---
>>>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>>>> index eaa21eddbe70..148ff5a82c8b 100644
>>>>> --- a/drivers/ata/sata_mv.c
>>>>> +++ b/drivers/ata/sata_mv.c
>>>>> @@ -4115,9 +4115,8 @@ static int mv_platform_probe(struct platform_device *pdev)
>>>>> if (IS_ERR(hpriv->port_phys[port])) {
>>>>> rc = PTR_ERR(hpriv->port_phys[port]);
>>>>> hpriv->port_phys[port] = NULL;
>>>>> - if ((rc != -EPROBE_DEFER) && (rc != -ENODEV))
>>>>> - dev_warn(&pdev->dev, "error getting phy");
>>>>> - goto err;
>>>>> + if (rc == -EPROBE_DEFER)
>>>>> + goto err;
>>>>
>>>> It feels a bit fishy to check for a specific errno.
>>
>> EPROBE_DEFER is a very special errno so from my point of view it is
>> not so surprising to have a specific treatment for this case.
>>
>>>>
>>>> How about not considering the lack of phy an error in all cases? In
>>>> other words, remove the check completely.
>>>
>>> Bad things would happen. EPROBE_DEFER means there is a phy driver, but
>>> because of the non-deterministic ordering of loading drivers, it has
>>> not been loaded yet. The sata_mv driver needs to fail its probe with
>>> EPROBE_DEFER, giving the phy driver chance to load, and then when
>>> sata_mv loads for a second time it will find the phy driver. If we
>>> ignored the EPROBE_DEFER and sata_mv loaded, it would be out of sync
>>> with the phy driver, resulting in the phy being turned off, and the
>>> discs would never be found.
>>>
>>> So
>>>
>>> EPROBE_DEFER: We need to fail the probe, but it is not fatal.
>>> ENOSYS: No generic PHY framework, sata_mv can load.
>>> ENODEV: No phy, probably because it is optional and not there, sata_mv can load.
>>> ENOMEM, EINVAL, etc are real errors and should probably be fatal and
>>> returned by the probe function.
>>>
>>> So i could reverse the comparison, look for ENOSYS and ENODEV and
>>> allow the probe to succeed and return the error in all other cases.
>>
>> This looks more unusual for me, but I understand the logic. Indeed this
>> solution seems better.
>>
>> Andrew, could you post a new version?
>> if you add the explanation you gave inside a comment just before the check,
>> I am sure it will be perfectly acceptable.
>
> Tejun,
>
> This patch is needed for our arm-soc bootfarms to continue testing. It
> would be helpful if, once you're ok with the patch, we took it through
> arm-soc. Would you mind Ack'ing it once you're happy with it?
>
Hi Jason,
Actually there were a new version of this series. I didn't notice it because
until today I didn't subscribed to the linux-ide mailing list.
Tejun already told he agreed to give his acked-by:
http://thread.gmane.org/gmane.linux.ide/56706/focus=56718
I also tested this series and after few try it finally worked, but Andrew
planned to send a v2 soon.
Sorry for the mess!
Gregory
> thx,
>
> Jason.
>
>>>> Isn't the phy used only for power saving purposes? Or do we want this
>>>> for another purpose?
>>>
>>> Yes. On Dove it can save around 10% of the idle power. I don't have
>>> kirkwood numbers at the moment, but it is probably similar.
>>>
>>>> Or as a different solution, can't we check for the compatible-string
>>>> and only try to get a phy for orion-sata?
>>>
>>> Orion5x cannot control its phy. Nor can PCI cards using the same IP
>>> core in discreet chips. I also hope that at some point 370 and XP gain
>>> phy support. I would really like this to work just like clocks do,
>>> where the clocks are optional and if they are in the DT node they are
>>> used, otherwise they are not.
>>>
>>> Andrew
>>>
>>
>>
>> --
>> Gregory Clement, Free Electrons
>> Kernel, drivers, real-time and embedded Linux
>> development, consulting, training and support.
>> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
