[U-Boot] [PATCH] pwm: add MACRO to limit some code which only for rk3288
Heiko Stübner
heiko at sntech.de
Thu Sep 22 00:01:15 CEST 2016
Hi,
Am Donnerstag, 28. Juli 2016, 21:46:04 schrieb Doug Anderson:
> On Mon, Jul 11, 2016 at 7:45 PM, Kever Yang <kever.yang at 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.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.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
More information about the U-Boot
mailing list