[U-Boot] [PATCH] pwm: add MACRO to limit some code which only for rk3288

[Kever Yang]

Hi Simon,

On 07/12/2016 09:12 PM, Simon Glass wrote:
> Hi Kever,
>
> On 11 July 2016 at 20:45, Kever Yang <kever.yang at rock-chips.com> wrote:
>> Hi Simon,
>>
>> CC Doug for this topic.
>>
>>
>> On 07/12/2016 07:54 AM, Simon Glass wrote:
>>> Hi Kever,
>>>
>>> On 11 July 2016 at 00:58, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 07/09/2016 10:39 PM, Simon Glass wrote:
>>>>> Hi Kever,
>>>>>
>>>>> On 7 July 2016 at 20:45, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>>>> The grf setting for rkpwm is only need in rk3288, other SoCs like
>>>>>> RK3399 which also use rkpwm do not need set the grf, let's add a
>>>>>> MACRO to make the code only for RK3288.
>>>>>>
>>>>>> Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
>>>>> patman will automatically remove these for you.
>>>> Will use patman for my new patches later, thanks.
>>>>
>>>>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>>>>> ---
>>>>>>     drivers/pwm/rk_pwm.c | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c
>>>>>> index 2d289a4..e34adf0 100644
>>>>>> --- a/drivers/pwm/rk_pwm.c
>>>>>> +++ b/drivers/pwm/rk_pwm.c
>>>>>> @@ -13,8 +13,10 @@
>>>>>>     #include <syscon.h>
>>>>>>     #include <asm/io.h>
>>>>>>     #include <asm/arch/clock.h>
>>>>>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>>>>>     #include <asm/arch/cru_rk3288.h>
>>>>>>     #include <asm/arch/grf_rk3288.h>
>>>>>> +#endif
>>>>>>     #include <asm/arch/pwm.h>
>>>>>>     #include <power/regulator.h>
>>>>>>
>>>>>> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>>
>>>>>>     struct rk_pwm_priv {
>>>>>>            struct rk3288_pwm *regs;
>>>>>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>>>>>            struct rk3288_grf *grf;
>>>>>> +#endif
>>>>>>     };
>>>>>>
>>>>>>     static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
>>>>>> period_ns,
>>>>>> @@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice
>>>>>> *dev)
>>>>>>            struct regmap *map;
>>>>>>
>>>>>>            priv->regs = (struct rk3288_pwm *)dev_get_addr(dev);
>>>>>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>>>>>            map = syscon_get_regmap_by_driver_data(ROCKCHIP_SYSCON_GRF);
>>>>>>            if (IS_ERR(map))
>>>>>>                    return PTR_ERR(map);
>>>>>>            priv->grf = regmap_get_range(map, 0);
>>>>>> +#endif
>>>>> I'd like to find a better way. Do all chips have a grf? If so then
>>>>> perhaps we can have a function like grf_enable_pwm() in the core SoC
>>>>> code and call it here. The #ifdef can be there.
>>>>>
>>>>> Or perhaps we should have a general soc.c, with its own struct
>>>>> containing pointers to grf, sgrf, etc. It can be set up at the start,
>>>>> and then provide a soc_enable_pwm() function to enable the pwm.
>>>>>
>>>>> What do you think?
>>>> Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like
>>>> grf. The content in grf are very different for different SoC, it gathers
>>>> all kinds of system/module control registers .
>>>> Back to the grf setting for pwm, this control operation is only need in
>>>> RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is
>>>> better to stay in driver file like rk_pwm.c.
>>>>
>>>> For these system control registers, I'm sure the dedicate general soc.c
>>>> is
>>>> needed, because they are not so common for different module and different
>>>> Soc. We are not able to have a common framework for them, a general soc.c
>>>> won't help much except all the system control are gather in one file .

Which entry for soc init in general soc.c is reasonable? I prefer as 
early as possible,
because I want to do the clock init at the same place, how about 
board_early_init_f()
or arch_cpu_init()?

>>>>
>>>> I think the GRF setting is part of the module and driver, so we can leave
>>>> it
>>>> as it is,
>>>> and a simple syscon driver is enough for grf like what is kernel do.
>>> I looked quickly at the Linux pwm driver but I don't see any grf
>>> access there. How does the grf register get set in Linux? I really
>>> want to avoid SoC-specific #ifdefs in drivers.
>> Doug(in cc list) send the patch for this pwm setting, but it seems does not
>> accept by upstream,
>> I think people also don't like this grf access in driver and prefer this
>> kind of one time init operation
>> to be done in bootloader.
>> patchset 1 implement in arch/arm/mach-rockchip/rockchip.c
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.html
>> patchset 4 implement in drivers/pwm_rockchip.c
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.html
>>
>> Hi Doug,
>>      Do you have any suggestion here?
> I think the device_is_compatible() idea would work OK for U-Boot if
> you want to do that. It's the CONFIG I want to avoid in the driver.

The compatible in rk3399.dtsi for pwm is:
compatible = "rockchip,rk3399-pwm", "rockchip,rk3288-pwm";

So I would like to consider this setting as a SoC general setting in a 
SoC file,
instead of setting in pwm driver, because there might be other setting 
for other
module need us to fix in soc.c.

Thanks,
- Kever
>
> Regards,
> Simon
>
>
>