From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Date: Tue, 12 Jul 2016 10:45:39 +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> Message-ID: <578459D3.8060307@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, 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 . >> >> 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 > > >