| Message ID | 1459126489-18311-1-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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). > 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@vger.kernel.org> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > 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. > + .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. Patch looks good to me though and as I said it fixes the issue so: Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> Best regards,
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@vger.kernel.org> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> >> --- >> >> 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@osg.samsung.com> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> Thanks! I'll update patch as you suggested and re-spin with tags. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof On 28 March 2016 at 09:02, Krzysztof Kozlowski <k.kozlowski@samsung.com> 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@vger.kernel.org> >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >>> >>> --- >>> >>> 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@osg.samsung.com> >> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28.03.2016 14:35, Anand Moon wrote: > Should their be a fix in the u-boot for HK for this issue ? It depends whether U-Boot S2MPS11 driver has this bug or has not. I did not observe any issues with U-Boot at this matter. > mmc card detection logic is pretty old in HK u-boot. This is not related to MMC card detection logic. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 28 Mar 2016, Krzysztof Kozlowski wrote: > The buck9 regulator of S2MPS11 PMIC lacked minimal selector for linear > mapping. The mapping starts from 0x40 (3 V). > > This buck9 provides power to other regulators, including LDO13 and LDO19 > which supply the MMC2 (SD card). > > 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@vger.kernel.org> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > 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/include/linux/mfd/samsung/s2mps11.h b/include/linux/mfd/samsung/s2mps11.h > index b288965e8101..3937a932bfe0 100644 > --- a/include/linux/mfd/samsung/s2mps11.h > +++ b/include/linux/mfd/samsung/s2mps11.h > @@ -173,10 +173,19 @@ enum s2mps11_regulators { > > #define S2MPS11_LDO_VSEL_MASK 0x3F > #define S2MPS11_BUCK_VSEL_MASK 0xFF > +#define S2MPS11_BUCK9_MIN_VSEL 0x40 > #define S2MPS11_ENABLE_MASK (0x03 << S2MPS11_ENABLE_SHIFT) > #define S2MPS11_ENABLE_SHIFT 0x06 > #define S2MPS11_LDO_N_VOLTAGES (S2MPS11_LDO_VSEL_MASK + 1) > #define S2MPS11_BUCK_N_VOLTAGES (S2MPS11_BUCK_VSEL_MASK + 1) > +/* > + * Buck9 supports only 32 voltages (values from 0x40 to 0x5F) but bootloader > + * initializes the register with value of 0xff so when probing this would > + * cause a failure (Odroid XU3): > + * vdd_2.8v_ldo: failed to get the current voltage(-22) > + * Instead pretend we support up to 0xff (5 V). > + */ > +#define S2MPS11_BUCK9_N_VOLTAGES 192 Err... NACK. Please go and fix the bootloader instead of hacking the kernel. > #define S2MPS11_RAMP_DELAY 25000 /* uV/us */ > > #define S2MPS11_CTRL1_PWRHOLD_MASK BIT(4)
On 29.03.2016 18:08, Lee Jones wrote: > On Mon, 28 Mar 2016, Krzysztof Kozlowski wrote: > >> The buck9 regulator of S2MPS11 PMIC lacked minimal selector for linear >> mapping. The mapping starts from 0x40 (3 V). >> >> This buck9 provides power to other regulators, including LDO13 and LDO19 >> which supply the MMC2 (SD card). >> >> 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@vger.kernel.org> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> >> --- >> >> 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/include/linux/mfd/samsung/s2mps11.h b/include/linux/mfd/samsung/s2mps11.h >> index b288965e8101..3937a932bfe0 100644 >> --- a/include/linux/mfd/samsung/s2mps11.h >> +++ b/include/linux/mfd/samsung/s2mps11.h >> @@ -173,10 +173,19 @@ enum s2mps11_regulators { >> >> #define S2MPS11_LDO_VSEL_MASK 0x3F >> #define S2MPS11_BUCK_VSEL_MASK 0xFF >> +#define S2MPS11_BUCK9_MIN_VSEL 0x40 >> #define S2MPS11_ENABLE_MASK (0x03 << S2MPS11_ENABLE_SHIFT) >> #define S2MPS11_ENABLE_SHIFT 0x06 >> #define S2MPS11_LDO_N_VOLTAGES (S2MPS11_LDO_VSEL_MASK + 1) >> #define S2MPS11_BUCK_N_VOLTAGES (S2MPS11_BUCK_VSEL_MASK + 1) >> +/* >> + * Buck9 supports only 32 voltages (values from 0x40 to 0x5F) but bootloader >> + * initializes the register with value of 0xff so when probing this would >> + * cause a failure (Odroid XU3): >> + * vdd_2.8v_ldo: failed to get the current voltage(-22) >> + * Instead pretend we support up to 0xff (5 V). >> + */ >> +#define S2MPS11_BUCK9_N_VOLTAGES 192 > > Err... NACK. > > Please go and fix the bootloader instead of hacking the kernel. The change is not needed and was an effect of my inaccurate understanding of device and driver behaviour. In v2 there is no such statement and changes. Instead driver properly uses the mask without touching other parts of register. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 29 Mar 2016, Krzysztof Kozlowski wrote: > On 29.03.2016 18:08, Lee Jones wrote: > > On Mon, 28 Mar 2016, Krzysztof Kozlowski wrote: > > > >> The buck9 regulator of S2MPS11 PMIC lacked minimal selector for linear > >> mapping. The mapping starts from 0x40 (3 V). > >> > >> This buck9 provides power to other regulators, including LDO13 and LDO19 > >> which supply the MMC2 (SD card). > >> > >> 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@vger.kernel.org> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> > >> --- > >> > >> 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/include/linux/mfd/samsung/s2mps11.h b/include/linux/mfd/samsung/s2mps11.h > >> index b288965e8101..3937a932bfe0 100644 > >> --- a/include/linux/mfd/samsung/s2mps11.h > >> +++ b/include/linux/mfd/samsung/s2mps11.h > >> @@ -173,10 +173,19 @@ enum s2mps11_regulators { > >> > >> #define S2MPS11_LDO_VSEL_MASK 0x3F > >> #define S2MPS11_BUCK_VSEL_MASK 0xFF > >> +#define S2MPS11_BUCK9_MIN_VSEL 0x40 > >> #define S2MPS11_ENABLE_MASK (0x03 << S2MPS11_ENABLE_SHIFT) > >> #define S2MPS11_ENABLE_SHIFT 0x06 > >> #define S2MPS11_LDO_N_VOLTAGES (S2MPS11_LDO_VSEL_MASK + 1) > >> #define S2MPS11_BUCK_N_VOLTAGES (S2MPS11_BUCK_VSEL_MASK + 1) > >> +/* > >> + * Buck9 supports only 32 voltages (values from 0x40 to 0x5F) but bootloader > >> + * initializes the register with value of 0xff so when probing this would > >> + * cause a failure (Odroid XU3): > >> + * vdd_2.8v_ldo: failed to get the current voltage(-22) > >> + * Instead pretend we support up to 0xff (5 V). > >> + */ > >> +#define S2MPS11_BUCK9_N_VOLTAGES 192 > > > > Err... NACK. > > > > Please go and fix the bootloader instead of hacking the kernel. > > The change is not needed and was an effect of my inaccurate > understanding of device and driver behaviour. In v2 there is no such > statement and changes. Instead driver properly uses the mask without > touching other parts of register. Very well. Thanks for letting me know.
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, \ + .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), + regulator_desc_s2mps11_buck9, regulator_desc_s2mps11_buck6_10(10, MIN_750_MV, STEP_12_5_MV), }; diff --git a/include/linux/mfd/samsung/s2mps11.h b/include/linux/mfd/samsung/s2mps11.h index b288965e8101..3937a932bfe0 100644 --- a/include/linux/mfd/samsung/s2mps11.h +++ b/include/linux/mfd/samsung/s2mps11.h @@ -173,10 +173,19 @@ enum s2mps11_regulators { #define S2MPS11_LDO_VSEL_MASK 0x3F #define S2MPS11_BUCK_VSEL_MASK 0xFF +#define S2MPS11_BUCK9_MIN_VSEL 0x40 #define S2MPS11_ENABLE_MASK (0x03 << S2MPS11_ENABLE_SHIFT) #define S2MPS11_ENABLE_SHIFT 0x06 #define S2MPS11_LDO_N_VOLTAGES (S2MPS11_LDO_VSEL_MASK + 1) #define S2MPS11_BUCK_N_VOLTAGES (S2MPS11_BUCK_VSEL_MASK + 1) +/* + * Buck9 supports only 32 voltages (values from 0x40 to 0x5F) but bootloader + * initializes the register with value of 0xff so when probing this would + * cause a failure (Odroid XU3): + * vdd_2.8v_ldo: failed to get the current voltage(-22) + * Instead pretend we support up to 0xff (5 V). + */ +#define S2MPS11_BUCK9_N_VOLTAGES 192 #define S2MPS11_RAMP_DELAY 25000 /* uV/us */ #define S2MPS11_CTRL1_PWRHOLD_MASK BIT(4)
The buck9 regulator of S2MPS11 PMIC lacked minimal selector for linear mapping. The mapping starts from 0x40 (3 V). This buck9 provides power to other regulators, including LDO13 and LDO19 which supply the MMC2 (SD card). 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@vger.kernel.org> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- 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(-)