Hi, Am Donnerstag, 28. Juli 2016, 21:46:04 schrieb Doug Anderson: > On Mon, Jul 11, 2016 at 7:45 PM, Kever Yang <kever.y...@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.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. > > > > 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.h > > tml patchset 4 implement in drivers/pwm_rockchip.c > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.h > > tml > > > > Hi Doug, > > > > Do you have any suggestion here? > > Heiko will skin me for suggesting it, but one option would be to > consider this as a pinmux thing in the linux kernel. The GRF can > choose between two IP blocks internally: the old PWM IP block or the > new PWM IP block. ;)
somehow this mail slipped through the cracks in my inbox :-( . But anyway, compared to the uart-discussion we're having elsewhere, I can somehow see how this might be viewed as pinctrl, simply because it has the funnel-structure, pin-muxing implies: dw_pwm - \ --- pwm-pinmux - \ rk_pwm - / -- pin other pinfunc - / So no skinning here ;-) > I believe the other option is to handle atop Heiko's patches: > > 9131959 New [1/3] dt-bindings: add used but undocumented > rockchip grf compatible values > 9131963 New [2/3] soc: rockchip: add driver handling grf setup > 9131957 New [3/3] ARM: rockchip: drop rk3288 jtag/mmc switch > handling > > I believe Heiko's patches implement the "grf subclass" talked about in > <https://patchwork.kernel.org/patch/4738311/>. considering that the two pwm, i2c etc implementations are mainly to have a fallback if the new block proves faulty (aka use rk_pwm by default and only fall back to the old one if it proves to have some unfixable error) I'd really consider this a init operation that should just be done one time. Heiko _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot