From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Date: Fri, 29 Jul 2016 10:55:19 +0800 Subject: [U-Boot] [PATCH] pwm: add MACRO to limit some code which only for rk3288 In-Reply-To: References: <1467945923-8848-1-git-send-email-kever.yang@rock-chips.com> <578343B3.7030105@rock-chips.com> <578459D3.8060307@rock-chips.com> Message-ID: <579AC597.2060805@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 07/12/2016 09:12 PM, Simon Glass wrote: > Hi Kever, > > On 11 July 2016 at 20:45, Kever Yang 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 wrote: >>>> Hi Simon, >>>> >>>> On 07/09/2016 10:39 PM, Simon Glass wrote: >>>>> Hi Kever, >>>>> >>>>> On 7 July 2016 at 20:45, Kever Yang 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 >>>>>> --- >>>>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#ifdef CONFIG_ROCKCHIP_RK3288 >>>>>> #include >>>>>> #include >>>>>> +#endif >>>>>> #include >>>>>> #include >>>>>> >>>>>> @@ -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 > > >