[U-Boot] [PATCH v2 07/12] exynos5: support tps65090 pmic

[Simon Glass]

Hi Minkyu,

On 28 May 2014 07:02, Minkyu Kang <mk7.kang at samsung.com> wrote:
> Dear Simon Glass,
>
> On 27/05/14 09:01, Simon Glass wrote:
>> Hi Minkyu,
>>
>> On 25 May 2014 21:15, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>> Dear Simon,
>>>
>>> On 23/05/14 02:27, Simon Glass wrote:
>>>> Hi Minkyu,
>>>>
>>>> On 21 May 2014 15:20, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>>>> On 22/05/14 03:58, Simon Glass wrote:
>>>>>> Hi Minkyu,
>>>>>>
>>>>>> On 21 May 2014 00:05, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>>>>>> On 20/05/14 20:47, Simon Glass wrote:
>>>>>>>> Hi Minkyu,
>>>>>>>>
>>>>>>>> On 15 May 2014 00:51, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>>>>>>>> On 03/04/14 08:24, Simon Glass wrote:
>>>>>>>>>> From: Aaron Durbin <adurbin at chromium.org>
>>>>>>>>>>
>>>>>>>>>> The TSP65090 is a PMIC on some exynos5 boards. The init function is
>>>>>>>>>> called for the TPS65090 pmic. If that device is not a part of the device
>>>>>>>>>> tree (returns -ENODEV) then continue. Otherwise return a failure.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Aaron Durbin <adurbin at chromium.org>
>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - Move code to exynos5-dt.c
>>>>>>>>>> - Fix comment style
>>>>>>>>>> - Add #ifdef around tps65090 code
>>>>>>>>>>
>>>>>>>>>>  board/samsung/smdk5250/exynos5-dt.c | 13 +++++++++++++
>>>>>>>>>>  1 file changed, 13 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/board/samsung/smdk5250/exynos5-dt.c b/board/samsung/smdk5250/exynos5-dt.c
>>>>>>>>>> index 1a64b9b..2c1cf8a 100644
>>>>>>>>>> --- a/board/samsung/smdk5250/exynos5-dt.c
>>>>>>>>>> +++ b/board/samsung/smdk5250/exynos5-dt.c
>>>>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>>>>  #include <asm/arch/sromc.h>
>>>>>>>>>>  #include <power/pmic.h>
>>>>>>>>>>  #include <power/max77686_pmic.h>
>>>>>>>>>> +#include <power/tps65090_pmic.h>
>>>>>>>>>>  #include <tmu.h>
>>>>>>>>>>
>>>>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>>> @@ -164,7 +165,19 @@ int exynos_power_init(void)
>>>>>>>>>>
>>>>>>>>>>  #ifdef CONFIG_POWER_MAX77686
>>>>>>>>>>       ret = max77686_init();
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             return ret;
>>>>>>>>>>  #endif
>>>>>>>>>> +#ifdef CONFIG_POWER_TPS65090
>>>>>>>>>> +     /*
>>>>>>>>>> +      * The TPS65090 may not be in the device tree. If so, it is not
>>>>>>>>>> +      * an error.
>>>>>>>>>
>>>>>>>>> Then, how we can initialise the tps65090?
>>>>>>>>
>>>>>>>> It is initialised if a suitable node is found in the device tree. If
>>>>>>>> the device tree does not have it, then the hardware is assumed to not
>>>>>>>> have this chip.
>>>>>>>
>>>>>>> then I think, it's an error.
>>>>>>> Why you said, it is not an error?
>>>>>>
>>>>>> I may be misunderstanding your question, but I'll try to answer what I
>>>>>> think you are asking.
>>>>>>
>>>>>> The device tree contains nodes for hardware in the machine that you
>>>>>> want to initialise, and information about each one. Devices can be
>>>>>> enabled or disabled by including or removing this information from the
>>>>>> device tree (or marking a device disabled with a status = "disabled"
>>>>>> property in the node).
>>>>>>
>>>>>> The tps65090 chip is not used in all exynos5-dt boards, but is used in
>>>>>> some. For example smdk5250 does not have it, but snow does. So we use
>>>>>> the device tree to specify the difference, including (on snow) things
>>>>>> like the tps65090, the display bridge chip, etc.
>>>>>>
>>>>>
>>>>> Hm, it doesn't make sense.
>>>>> Then it(#define CONFIG_POWER_TPS65090) should go into each config files.
>>>>> Not in exynos5-dt.h.
>>>>> Please modify it and patch 6/12 also.
>>>>
>>>> The way it works at present is that there is a single exynos5-dt.h
>>>> file which is used by all exynos5 boards. To the extent possible we
>>>> have avoided putting particular features in things like snow.h and
>>>> smdk5250.h - they just include exynos5-dt.h without changes.
>>>>
>>>> The idea is that we can have one U-Boot binary that runs on any
>>>> exynos5 device, rather than lots of different options. This makes it
>>>> much easier to test changes sine we only need to build it once. If
>>>> every exynos5 board has different #defines then there are more
>>>> combinations to build and test. This is similar to what the kernel
>>>> does with arch/arm/mach-exynos/mach-exynos5-dt.c.
>>>>
>>>> Using this approach the only differences between smdk5250, daisy,
>>>> snow, spring, etc. are in the device tree. I'd really like to avoid
>>>> changing this now. It is easy enough for people to add their own
>>>> private variants of these locally if they want to, but for mainline I
>>>> believe it is better to be more generic.
>>>
>>> I totally understood what you assert.
>>> But I can't agree with you.
>>> Do you think if we collect all features at exynos5-dt.h then
>>> it can be generic? even if some boards doesn't have the device?
>>> I think no. It just wrong definition.
>>> We should separate exynos5 generic features and board specific features.
>>
>> Yes my intent is that we have an exynos5 build which (given the right
>> device tree) can support any board. That has been the intent of the
>> device tree effort for a while, and similar to the kernel approach.
>>
>> What is the objection of having the driver compiled in for a board
>> that doesn't have the device? Is it the extra 1KB of code space for
>> smdk5250 etc.?
>
> Yes, it's a tiny part of binary.
> But if we allow such things.. then it can be grew more and more.
>
>>
>> Anyway, it doesn't necessarily have to be exynos5-dt.h. If you really
>> don't like that, we could create a new generic exynos5 board and I can
>> target that instead. But I really want to avoid having to build
>> multiple U-Boots for each board if possible.
>
> I want to define CONFIG_POWER_TPS65090 at snow.h and other each board's config file that have a device.
> But, I know what you want.
> So I will merge this patchset.
> But at future, I'll not allow any exception.
> It was not right way I think.
> I just want to keep clean our codes.
> That's all.

Thanks for being flexible. It is good to get this applied.

I'll take a look at how to improve this to separate out the 'generic'
board from the others.

Regards,
Simon