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

[Kever Yang]

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?

Thanks,
- Kever
>
>> Thanks,
>> - Kever
>>
>>>>           return 0;
>>>>    }
>>>>
>>>>    static int rk_pwm_probe(struct udevice *dev)
>>>>    {
>>>> +#ifdef CONFIG_ROCKCHIP_RK3288
>>>>           struct rk_pwm_priv *priv = dev_get_priv(dev);
>>>>
>>>>           rk_setreg(&priv->grf->soc_con2, 1 << 0);
>>>> +#endif
>>>>
>>>>           return 0;
>>>>    }
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>> Regards,
>>> Simon
>>>
>>>
>>>
>>
> Regards,
> Simon
>
>
>