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

[Simon Glass]

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 .
>>>
>>> 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.

Regards,
Simon