diff mbox

[U-Boot,28/55] dm: pmic: max77686: Support all BUCK regulators

Message ID 1435882592-487-29-git-send-email-sjg@chromium.org
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass July 3, 2015, 12:16 a.m. UTC 
Add support for all BUCK regulators, now that the correct register is
accessed for each.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/power/regulator/max77686.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Przemyslaw Marczak July 10, 2015, 11:53 a.m. UTC | #1 
Hello Simon,

On 07/03/2015 02:16 AM, Simon Glass wrote:
> Add support for all BUCK regulators, now that the correct register is
> accessed for each.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   drivers/power/regulator/max77686.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/power/regulator/max77686.c b/drivers/power/regulator/max77686.c
> index 21173fc..427b717 100644
> --- a/drivers/power/regulator/max77686.c
> +++ b/drivers/power/regulator/max77686.c
> @@ -81,11 +81,7 @@ static int max77686_buck_volt2hex(int buck, int uV)
>   		/* hex = (uV - 600000) / 12500; */
>   		hex = (uV - MAX77686_BUCK_UV_LMIN) / MAX77686_BUCK_UV_LSTEP;
>   		hex_max = MAX77686_BUCK234_VOLT_MAX_HEX;
> -		/**
> -		 * Those use voltage scaller - temporary not implemented
> -		 * so return just 0
> -		 */
> -		return -ENOSYS;
> +		break;
>   	default:
>   		/* hex = (uV - 750000) / 50000; */
>   		hex = (uV - MAX77686_BUCK_UV_HMIN) / MAX77686_BUCK_UV_HSTEP;
> @@ -379,11 +375,11 @@ static int max77686_buck_val(struct udevice *dev, int op, int *uV)
>   	case 2:
>   	case 3:
>   	case 4:
> -		/* Those use voltage scallers - will support in the future */
>   		mask = MAX77686_BUCK234_VOLT_MASK;
> -		return -ENOSYS;
> +		break;
>   	default:
>   		mask = MAX77686_BUCK_VOLT_MASK;
> +		break;
>   	}
>
>   	ret = pmic_read(dev->parent, adr, &val, 1);
>

The bucks 2,3,4 can work in DVS mode, which allows select one of eight 
DVS regulators for each output. The default selection at power-on is 
DVS1 for each output, and it corresponds to the currently defined output 
register addresses.

The selection can be done by six PMIC's GPIOs:
- DVS1/2/3 - output selection: 0x0=DVS1...0x7=DVS8
- SELB2/3/4 - mode switch: 'DVS' or 'no DVS'

Reading or writing the default registers is proper only in case:
- for the default PMIC's power-up setting - may conflict with bl1/bl2
- when DVS1/2/3 GPIOs are set to LOW - DVS1 selected
- SELB2/3/4 - are set to LOW - no DVS mode

The documentation is poor, but if I good understand, the SELB is used as 
"latch" for the DVS selection.

So the driver, could be unreliable for these outputs if it doesn't check 
the PMIC's GPIOs.

It's quite confusing, since the PMIC, doesn't provide registers to check 
those GPIOs. It should be checked by the driver and can be delivered by 
device-tree.

This is also confusing, since it depends on board design, because the 
PMIC's GPIOs can be connected to the SoCs GPIOs and also just pulled to 
POWER/GND signals.

The documentation says, that those GPIOs should be set accordingly, and 
for example Odroid U3 has connected the SELB to VDD_IO(LDO3) power line, 
so actually this state can not be changed or can be changed by accident 
when changing the VDD_IO - which is HIGH at PMIC's power-up.

The switching is impossible, since the VDD_IO line is shared with few 
other peripherals.

In this case, maybe you should add config to allow use of the BUCK234 
only for case in which DVS mode is disabled by board design (SELB2/3/4 
set to LOW). This may also work, when the GPIOs of Exynos stays in the 
reset state.

Then, using the default 'buck_out' registers: 0x14, 0x1e, 0x28 is 
reasonable.

Did you tried measure the output voltage, after its change?

Regards,
Simon Glass July 30, 2015, 2:05 a.m. UTC | #2 
Hi Przemyslaw,

On 10 July 2015 at 05:53, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>
> Hello Simon,
>
> On 07/03/2015 02:16 AM, Simon Glass wrote:
>>
>> Add support for all BUCK regulators, now that the correct register is
>> accessed for each.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> ---
>>
>>   drivers/power/regulator/max77686.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/power/regulator/max77686.c b/drivers/power/regulator/max77686.c
>> index 21173fc..427b717 100644
>> --- a/drivers/power/regulator/max77686.c
>> +++ b/drivers/power/regulator/max77686.c
>> @@ -81,11 +81,7 @@ static int max77686_buck_volt2hex(int buck, int uV)
>>                 /* hex = (uV - 600000) / 12500; */
>>                 hex = (uV - MAX77686_BUCK_UV_LMIN) / MAX77686_BUCK_UV_LSTEP;
>>                 hex_max = MAX77686_BUCK234_VOLT_MAX_HEX;
>> -               /**
>> -                * Those use voltage scaller - temporary not implemented
>> -                * so return just 0
>> -                */
>> -               return -ENOSYS;
>> +               break;
>>         default:
>>                 /* hex = (uV - 750000) / 50000; */
>>                 hex = (uV - MAX77686_BUCK_UV_HMIN) / MAX77686_BUCK_UV_HSTEP;
>> @@ -379,11 +375,11 @@ static int max77686_buck_val(struct udevice *dev, int op, int *uV)
>>         case 2:
>>         case 3:
>>         case 4:
>> -               /* Those use voltage scallers - will support in the future */
>>                 mask = MAX77686_BUCK234_VOLT_MASK;
>> -               return -ENOSYS;
>> +               break;
>>         default:
>>                 mask = MAX77686_BUCK_VOLT_MASK;
>> +               break;
>>         }
>>
>>         ret = pmic_read(dev->parent, adr, &val, 1);
>>
>
> The bucks 2,3,4 can work in DVS mode, which allows select one of eight DVS regulators for each output. The default selection at power-on is DVS1 for each output, and it corresponds to the currently defined output register addresses.
>
> The selection can be done by six PMIC's GPIOs:
> - DVS1/2/3 - output selection: 0x0=DVS1...0x7=DVS8
> - SELB2/3/4 - mode switch: 'DVS' or 'no DVS'
>
> Reading or writing the default registers is proper only in case:
> - for the default PMIC's power-up setting - may conflict with bl1/bl2
> - when DVS1/2/3 GPIOs are set to LOW - DVS1 selected
> - SELB2/3/4 - are set to LOW - no DVS mode
>
> The documentation is poor, but if I good understand, the SELB is used as "latch" for the DVS selection.
>
> So the driver, could be unreliable for these outputs if it doesn't check the PMIC's GPIOs.
>
> It's quite confusing, since the PMIC, doesn't provide registers to check those GPIOs. It should be checked by the driver and can be delivered by device-tree.
>
> This is also confusing, since it depends on board design, because the PMIC's GPIOs can be connected to the SoCs GPIOs and also just pulled to POWER/GND signals.
>
> The documentation says, that those GPIOs should be set accordingly, and for example Odroid U3 has connected the SELB to VDD_IO(LDO3) power line, so actually this state can not be changed or can be changed by accident when changing the VDD_IO - which is HIGH at PMIC's power-up.
>
> The switching is impossible, since the VDD_IO line is shared with few other peripherals.
>
> In this case, maybe you should add config to allow use of the BUCK234 only for case in which DVS mode is disabled by board design (SELB2/3/4 set to LOW). This may also work, when the GPIOs of Exynos stays in the reset state.
>
> Then, using the default 'buck_out' registers: 0x14, 0x1e, 0x28 is reasonable.

I don't see anything in the binding. I have added a comment in the
driver to explain this limitation. However I haven't actually seen
hardware that makes use of it. Are you saying that Odroid U3 does use
it, or just that it has the lines connected up incorrectly?

>
> Did you tried measure the output voltage, after its change?

No I have not measured it, only verified that the code looks correct.
If you are able to do that I think it would be a good test.

>
> Regards,
> --
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak@samsung.com

Regards,
Simon
Przemyslaw Marczak July 30, 2015, 8:22 a.m. UTC | #3 
Hello Simon,

On 07/30/2015 04:05 AM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 10 July 2015 at 05:53, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>
>> Hello Simon,
>>
>> On 07/03/2015 02:16 AM, Simon Glass wrote:
>>>
>>> Add support for all BUCK regulators, now that the correct register is
>>> accessed for each.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>> ---
>>>
>>>    drivers/power/regulator/max77686.c | 10 +++-------
>>>    1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/power/regulator/max77686.c b/drivers/power/regulator/max77686.c
>>> index 21173fc..427b717 100644
>>> --- a/drivers/power/regulator/max77686.c
>>> +++ b/drivers/power/regulator/max77686.c
>>> @@ -81,11 +81,7 @@ static int max77686_buck_volt2hex(int buck, int uV)
>>>                  /* hex = (uV - 600000) / 12500; */
>>>                  hex = (uV - MAX77686_BUCK_UV_LMIN) / MAX77686_BUCK_UV_LSTEP;
>>>                  hex_max = MAX77686_BUCK234_VOLT_MAX_HEX;
>>> -               /**
>>> -                * Those use voltage scaller - temporary not implemented
>>> -                * so return just 0
>>> -                */
>>> -               return -ENOSYS;
>>> +               break;
>>>          default:
>>>                  /* hex = (uV - 750000) / 50000; */
>>>                  hex = (uV - MAX77686_BUCK_UV_HMIN) / MAX77686_BUCK_UV_HSTEP;
>>> @@ -379,11 +375,11 @@ static int max77686_buck_val(struct udevice *dev, int op, int *uV)
>>>          case 2:
>>>          case 3:
>>>          case 4:
>>> -               /* Those use voltage scallers - will support in the future */
>>>                  mask = MAX77686_BUCK234_VOLT_MASK;
>>> -               return -ENOSYS;
>>> +               break;
>>>          default:
>>>                  mask = MAX77686_BUCK_VOLT_MASK;
>>> +               break;
>>>          }
>>>
>>>          ret = pmic_read(dev->parent, adr, &val, 1);
>>>
>>
>> The bucks 2,3,4 can work in DVS mode, which allows select one of eight DVS regulators for each output. The default selection at power-on is DVS1 for each output, and it corresponds to the currently defined output register addresses.
>>
>> The selection can be done by six PMIC's GPIOs:
>> - DVS1/2/3 - output selection: 0x0=DVS1...0x7=DVS8
>> - SELB2/3/4 - mode switch: 'DVS' or 'no DVS'
>>
>> Reading or writing the default registers is proper only in case:
>> - for the default PMIC's power-up setting - may conflict with bl1/bl2
>> - when DVS1/2/3 GPIOs are set to LOW - DVS1 selected
>> - SELB2/3/4 - are set to LOW - no DVS mode
>>
>> The documentation is poor, but if I good understand, the SELB is used as "latch" for the DVS selection.
>>
>> So the driver, could be unreliable for these outputs if it doesn't check the PMIC's GPIOs.
>>
>> It's quite confusing, since the PMIC, doesn't provide registers to check those GPIOs. It should be checked by the driver and can be delivered by device-tree.
>>
>> This is also confusing, since it depends on board design, because the PMIC's GPIOs can be connected to the SoCs GPIOs and also just pulled to POWER/GND signals.
>>
>> The documentation says, that those GPIOs should be set accordingly, and for example Odroid U3 has connected the SELB to VDD_IO(LDO3) power line, so actually this state can not be changed or can be changed by accident when changing the VDD_IO - which is HIGH at PMIC's power-up.
>>
>> The switching is impossible, since the VDD_IO line is shared with few other peripherals.
>>
>> In this case, maybe you should add config to allow use of the BUCK234 only for case in which DVS mode is disabled by board design (SELB2/3/4 set to LOW). This may also work, when the GPIOs of Exynos stays in the reset state.
>>
>> Then, using the default 'buck_out' registers: 0x14, 0x1e, 0x28 is reasonable.
>
> I don't see anything in the binding. I have added a comment in the
> driver to explain this limitation. However I haven't actually seen
> hardware that makes use of it. Are you saying that Odroid U3 does use
> it, or just that it has the lines connected up incorrectly?
>

Right, the binding for those GPIOs don't exists, since change of those 
registers values was not supported by the driver before.

I looked into the bl2 code from the Hardkernel's U-Boot (we are using 
it's signed bl2 binary for U3). There is a code for smdk4212/smdk4412, 
which is reused for Odroid U3 and the bl2 MAX77686 driver configures the 
DVS/SELB gpios. Unfortunately the GPIO is invalid, probably it is valid 
for smdk.

Code:
https://github.com/hardkernel/u-boot/blob/odroid-v2010.12/board/samsung/smdk4212/pmic.c#L707


In the hardkernel's code we can also see the MAX77686 driver for 
smdk5250, which also sets the DVS/SELB by GPD/GPX pins in a proper way.

Code:
https://github.com/hardkernel/u-boot/blob/odroid-v2010.12/board/samsung/smdk5250/pmic.c#L546


>>
>> Did you tried measure the output voltage, after its change?
>
> No I have not measured it, only verified that the code looks correct.
> If you are able to do that I think it would be a good test.
>

At present I can't measure it, but it should be verified in the future.
But if you can notice a stability change after setting those registers, 
then we can assume, that it works.

>>
>> Regards,
>> --
>> Przemyslaw Marczak
>> Samsung R&D Institute Poland
>> Samsung Electronics
>> p.marczak@samsung.com
>
> Regards,
> Simon
>

Best regards,
Simon Glass Aug. 3, 2015, 2:05 p.m. UTC | #4 
Hi Przemyslaw,

On 30 July 2015 at 02:22, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello Simon,
>
>
> On 07/30/2015 04:05 AM, Simon Glass wrote:
>>
>> Hi Przemyslaw,
>>
>> On 10 July 2015 at 05:53, Przemyslaw Marczak <p.marczak@samsung.com>
>> wrote:
>>>
>>>
>>> Hello Simon,
>>>
>>> On 07/03/2015 02:16 AM, Simon Glass wrote:
>>>>
>>>>
>>>> Add support for all BUCK regulators, now that the correct register is
>>>> accessed for each.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> ---
>>>>
>>>>    drivers/power/regulator/max77686.c | 10 +++-------
>>>>    1 file changed, 3 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/power/regulator/max77686.c
>>>> b/drivers/power/regulator/max77686.c
>>>> index 21173fc..427b717 100644
>>>> --- a/drivers/power/regulator/max77686.c
>>>> +++ b/drivers/power/regulator/max77686.c
>>>> @@ -81,11 +81,7 @@ static int max77686_buck_volt2hex(int buck, int uV)
>>>>                  /* hex = (uV - 600000) / 12500; */
>>>>                  hex = (uV - MAX77686_BUCK_UV_LMIN) /
>>>> MAX77686_BUCK_UV_LSTEP;
>>>>                  hex_max = MAX77686_BUCK234_VOLT_MAX_HEX;
>>>> -               /**
>>>> -                * Those use voltage scaller - temporary not implemented
>>>> -                * so return just 0
>>>> -                */
>>>> -               return -ENOSYS;
>>>> +               break;
>>>>          default:
>>>>                  /* hex = (uV - 750000) / 50000; */
>>>>                  hex = (uV - MAX77686_BUCK_UV_HMIN) /
>>>> MAX77686_BUCK_UV_HSTEP;
>>>> @@ -379,11 +375,11 @@ static int max77686_buck_val(struct udevice *dev,
>>>> int op, int *uV)
>>>>          case 2:
>>>>          case 3:
>>>>          case 4:
>>>> -               /* Those use voltage scallers - will support in the
>>>> future */
>>>>                  mask = MAX77686_BUCK234_VOLT_MASK;
>>>> -               return -ENOSYS;
>>>> +               break;
>>>>          default:
>>>>                  mask = MAX77686_BUCK_VOLT_MASK;
>>>> +               break;
>>>>          }
>>>>
>>>>          ret = pmic_read(dev->parent, adr, &val, 1);
>>>>
>>>
>>> The bucks 2,3,4 can work in DVS mode, which allows select one of eight
>>> DVS regulators for each output. The default selection at power-on is DVS1
>>> for each output, and it corresponds to the currently defined output register
>>> addresses.
>>>
>>> The selection can be done by six PMIC's GPIOs:
>>> - DVS1/2/3 - output selection: 0x0=DVS1...0x7=DVS8
>>> - SELB2/3/4 - mode switch: 'DVS' or 'no DVS'
>>>
>>> Reading or writing the default registers is proper only in case:
>>> - for the default PMIC's power-up setting - may conflict with bl1/bl2
>>> - when DVS1/2/3 GPIOs are set to LOW - DVS1 selected
>>> - SELB2/3/4 - are set to LOW - no DVS mode
>>>
>>> The documentation is poor, but if I good understand, the SELB is used as
>>> "latch" for the DVS selection.
>>>
>>> So the driver, could be unreliable for these outputs if it doesn't check
>>> the PMIC's GPIOs.
>>>
>>> It's quite confusing, since the PMIC, doesn't provide registers to check
>>> those GPIOs. It should be checked by the driver and can be delivered by
>>> device-tree.
>>>
>>> This is also confusing, since it depends on board design, because the
>>> PMIC's GPIOs can be connected to the SoCs GPIOs and also just pulled to
>>> POWER/GND signals.
>>>
>>> The documentation says, that those GPIOs should be set accordingly, and
>>> for example Odroid U3 has connected the SELB to VDD_IO(LDO3) power line, so
>>> actually this state can not be changed or can be changed by accident when
>>> changing the VDD_IO - which is HIGH at PMIC's power-up.
>>>
>>> The switching is impossible, since the VDD_IO line is shared with few
>>> other peripherals.
>>>
>>> In this case, maybe you should add config to allow use of the BUCK234
>>> only for case in which DVS mode is disabled by board design (SELB2/3/4 set
>>> to LOW). This may also work, when the GPIOs of Exynos stays in the reset
>>> state.
>>>
>>> Then, using the default 'buck_out' registers: 0x14, 0x1e, 0x28 is
>>> reasonable.
>>
>>
>> I don't see anything in the binding. I have added a comment in the
>> driver to explain this limitation. However I haven't actually seen
>> hardware that makes use of it. Are you saying that Odroid U3 does use
>> it, or just that it has the lines connected up incorrectly?
>>
>
> Right, the binding for those GPIOs don't exists, since change of those
> registers values was not supported by the driver before.
>
> I looked into the bl2 code from the Hardkernel's U-Boot (we are using it's
> signed bl2 binary for U3). There is a code for smdk4212/smdk4412, which is
> reused for Odroid U3 and the bl2 MAX77686 driver configures the DVS/SELB
> gpios. Unfortunately the GPIO is invalid, probably it is valid for smdk.
>
> Code:
> https://github.com/hardkernel/u-boot/blob/odroid-v2010.12/board/samsung/smdk4212/pmic.c#L707
>
>
> In the hardkernel's code we can also see the MAX77686 driver for smdk5250,
> which also sets the DVS/SELB by GPD/GPX pins in a proper way.
>
> Code:
> https://github.com/hardkernel/u-boot/blob/odroid-v2010.12/board/samsung/smdk5250/pmic.c#L546
>

That is a good collection of hackery!

The question is, should I do anything here? Things seem to work OK in
my testing with the patch as it is.

>
>>>
>>> Did you tried measure the output voltage, after its change?
>>
>>
>> No I have not measured it, only verified that the code looks correct.
>> If you are able to do that I think it would be a good test.
>>
>
> At present I can't measure it, but it should be verified in the future.
> But if you can notice a stability change after setting those registers, then
> we can assume, that it works.

OK, sounds good. I can read the values back at least. I definitely had
it die before I added the voltage stuff so it seems to be having an
effect.

Regards,
Simon
diff mbox

Patch

diff --git a/drivers/power/regulator/max77686.c b/drivers/power/regulator/max77686.c
index 21173fc..427b717 100644
--- a/drivers/power/regulator/max77686.c
+++ b/drivers/power/regulator/max77686.c
@@ -81,11 +81,7 @@  static int max77686_buck_volt2hex(int buck, int uV)
 		/* hex = (uV - 600000) / 12500; */
 		hex = (uV - MAX77686_BUCK_UV_LMIN) / MAX77686_BUCK_UV_LSTEP;
 		hex_max = MAX77686_BUCK234_VOLT_MAX_HEX;
-		/**
-		 * Those use voltage scaller - temporary not implemented
-		 * so return just 0
-		 */
-		return -ENOSYS;
+		break;
 	default:
 		/* hex = (uV - 750000) / 50000; */
 		hex = (uV - MAX77686_BUCK_UV_HMIN) / MAX77686_BUCK_UV_HSTEP;
@@ -379,11 +375,11 @@  static int max77686_buck_val(struct udevice *dev, int op, int *uV)
 	case 2:
 	case 3:
 	case 4:
-		/* Those use voltage scallers - will support in the future */
 		mask = MAX77686_BUCK234_VOLT_MASK;
-		return -ENOSYS;
+		break;
 	default:
 		mask = MAX77686_BUCK_VOLT_MASK;
+		break;
 	}
 
 	ret = pmic_read(dev->parent, adr, &val, 1);