Hi Kever,
On 11 July 2016 at 00:58, Kever Yang <kever.y...@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.y...@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.y...@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.
>
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
