Re: [PATCH] regulator: s2mps11: Fix invalid minimal selector for buck9 supplying SD card

[Anand Moon]

Hi Krzysztof

On 28 March 2016 at 09:02, Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> wrote:
> On 28.03.2016 10:59, Javier Martinez Canillas wrote:
>> Hello Krzysztof,
>>
>> On 03/27/2016 08:54 PM, Krzysztof Kozlowski wrote:
>>> The buck9 regulator of S2MPS11 PMIC lacked minimal selector for linear
>>> mapping. The mapping starts from 0x40 (3 V).
>>>
>>
>> This patch is a real fix since the the SD error goes away and the regulator
>> driver probes correctly now.
>>
>>> This buck9 provides power to other regulators, including LDO13 and LDO19
>>> which supply the MMC2 (SD card).
>>>
>>
>> I think it's worth mentioning that this is the case for the Exynos5422 Odroid
>> XU{3,4} boards. I mean, the regulators can be used for anything but are those
>> boards that use LDO19 as the SD card supply.
>
> Yes, indeed. I forgot to add that details.
>
>>
>> In fact, I wonder if the subject line shouldn't be changed to something like:
>>
>> "regulator: s2mps11: Fix invalid min selector and voltages num for buck9"
>>
>> since the real problem is that the .linear_min_sel and .n_voltages are wrong.
>> The fact that the SD was failing on Odroids is just a consequence of that
>> (although I agree that this information should be part of the commit message).
>
> Okay, seems more accurate.
>
>>
>>> Bootloader initializes the regulator with value of 0xff (5 V) which is
>>> outside of supported voltage range. When (during boot) constraints to
>>> buck9 were applied, the driver wrote value counting from 0x00, not 0x40.
>>> Effectively driver set lower voltage than required leading to SD card
>>> detection errors on Odroid XU3/XU4:
>>>      mmc1: card never left busy state
>>>      mmc1: error -110 whilst initialising SD card
>>>
>>> Fixes: cb74685ecb39 ("regulator: s2mps11: Add samsung s2mps11 regulator driver")
>>> Cc: <stable@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
>>>
>>> ---
>>>
>>> The issue can be reproduced on next-20160324 with
>>> bae4fdc88d7f7dda1 (regulator: core: Ensure we are at least in bounds
>>> for our constraints).
>>> ---
>>>  drivers/regulator/s2mps11.c         | 19 ++++++++++++++++++-
>>>  include/linux/mfd/samsung/s2mps11.h |  9 +++++++++
>>>  2 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
>>> index d24e2c783dc5..caeefc38ac47 100644
>>> --- a/drivers/regulator/s2mps11.c
>>> +++ b/drivers/regulator/s2mps11.c
>>> @@ -324,6 +324,23 @@ static struct regulator_ops s2mps11_buck_ops = {
>>>      .enable_mask    = S2MPS11_ENABLE_MASK                   \
>>>  }
>>>
>>> +#define regulator_desc_s2mps11_buck9 {                              \
>>> +    .name           = "BUCK9",                              \
>>> +    .id             = S2MPS11_BUCK9,                        \
>>> +    .ops            = &s2mps11_buck_ops,                    \
>>> +    .type           = REGULATOR_VOLTAGE,                    \
>>> +    .owner          = THIS_MODULE,                          \
>>> +    .min_uV         = MIN_3000_MV,                          \
>>> +    .uV_step        = STEP_25_MV,                           \
>>> +    .linear_min_sel = S2MPS11_BUCK9_MIN_VSEL,               \
>>
>> I don't have a datasheet for this PMIC but I wonder if buck9 is the only
>> buck regulator whose minimal register value is != 0. If that's not the
>> case, it would be good to fix the descriptions for all other regulators.
>
> Some of them are broken, some not. :) Buck 1-4 and 6 also should have
> minimal selector. Also number of selectors and masks seems to be
> invalid. I already have a plan to fix this up but it is not an urgent
> task because the driver works so far.
>
>>
>>> +    .n_voltages     = S2MPS11_BUCK9_N_VOLTAGES,             \
>>> +    .ramp_delay     = S2MPS11_RAMP_DELAY,                   \
>>> +    .vsel_reg       = S2MPS11_REG_B9CTRL2,                  \
>>> +    .vsel_mask      = S2MPS11_BUCK_VSEL_MASK,               \
>>> +    .enable_reg     = S2MPS11_REG_B9CTRL1,                  \
>>> +    .enable_mask    = S2MPS11_ENABLE_MASK                   \
>>> +}
>>> +
>>>  static const struct regulator_desc s2mps11_regulators[] = {
>>>      regulator_desc_s2mps11_ldo(1, STEP_25_MV),
>>>      regulator_desc_s2mps11_ldo(2, STEP_50_MV),
>>> @@ -371,7 +388,7 @@ static const struct regulator_desc s2mps11_regulators[] = {
>>>      regulator_desc_s2mps11_buck6_10(6, MIN_600_MV, STEP_6_25_MV),
>>>      regulator_desc_s2mps11_buck6_10(7, MIN_600_MV, STEP_6_25_MV),
>>>      regulator_desc_s2mps11_buck6_10(8, MIN_600_MV, STEP_6_25_MV),
>>> -    regulator_desc_s2mps11_buck6_10(9, MIN_3000_MV, STEP_25_MV),
>>
>> Maybe the regulator_desc_s2mps11_buck6_10() define should be renamed?
>> Since it's no longer true that can be used for buck6-10, so the name
>> is misleading now.
>
> Sure.
>
>>
>> Patch looks good to me though and as I said it fixes the issue so:
>>
>> Reviewed-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> Tested-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>
> Thanks!
>
> I'll update patch as you suggested and re-spin with tags.
>
> Best regards,
> Krzysztof
>

Should their be a fix in the u-boot for HK for this issue ?
mmc card detection logic is pretty old in HK u-boot.

Best Regards
-Anand Moon

> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html