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

[Simon Glass]

Hi Przemyslaw,

On 30 July 2015 at 02:22, Przemyslaw Marczak <p.marczak at 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 at 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 at 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