[U-Boot] [PATCH] pwm: add MACRO to limit some code which only for rk3288

["Heiko Stübner" <heiko@sntech.de>]

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@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@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@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@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