Re: [PATCH 1/4] ARM: rockchip: rk3288: Switch to use the proper PWM IP

[Thierry Reding]

On Wed, Aug 20, 2014 at 08:55:09AM -0700, Doug Anderson wrote:
> On Wed, Aug 20, 2014 at 8:38 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
[...]
> > Looking at the register offsets in the device tree that seems likely. At
> > least PWMs 0 and 1 as well as 2 and 3 seem like they could be in the
> > same IP block. Their placement in the register map is somewhat strange:
> >
> >         pwm0: pwm@20030000 {
> >                 ...
> >                 reg = <0x20030000 0x10>;
> >                 ...
> >                 clocks = <&cru PCLK_PWM01>;
> >                 ...
> >         };
> >
> >         pwm1: pwm@20030010 {
> >                 ...
> >                 reg = <0x20030010 0x10>;
> >                 ...
> >                 clocks = <&cru PCLK_PWM01>;
> >                 ...
> >         };
> >
> >         ...
> >
> >         pwm2: pwm@20050020 {
> >                 ...
> >                 reg = <0x20050020 0x10>;
> >                 ...
> >                 clocks = <&cru PCLK_PWM23>;
> >                 ...
> >         };
> >
> >         pwm3: pwm@20050030 {
> >                 ...
> >                 reg = <0x20050030 0x10>;
> >                 ...
> >                 clocks = <&cru PCLK_PWM23>;
> >                 ...
> >         };
> 
> Ah, you're looking at "rk3xxx.dtsi".  That doesn't apply to rk3288
> (the downsides of trying to guess ahead of time what SoC vendors will
> name new models).
> 
> In rk3288 they have the same clocks.  See patch #3 in this series.
> 
> 
> > The clocks would also indicate that there are actually two blocks. I
> > seem to remember a discussion about whether to handle them as a single
> > block or two/four, but I can't seem to find a reference to it. Maybe I'm
> > confusing it with another driver.
> 
> At this point it seems like the choice has already been made to handle
> them as separate PWMs.  I can change this choice if you want...

Well, looking at patch 3/4 this really does seem to be one single block
providing four PWM channels, so the right thing to do would be to
represent it in one device tree node. But I'll leave it up to Heiko to
decide how he wants to handle this.

One downside of describing it as one device is that it would make the
pinmux handling slightly more difficult, since presumably you'd only
want to apply the pinmux settings when a channel is actually being used.
Currently the pinmux doesn't apply as long as the device remains
disabled in device tree (though enabling it doesn't necessarily mean
that it's being used).

Like I said, it's up to Heiko to decide whether it's worth making this
change (and it'd make sense to apply it to existing DTS files
retroactively) or better to keep what we have.

Thierry

Attachment: pgpUH2lXumEJa.pgp
Description: PGP signature