Message-ID: <20140820060759.GD13793@ulmo>
Date:	Wed, 20 Aug 2014 08:08:01 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Heiko Stuebner <heiko@...ech.de>,
	Caesar Wang <caesar.wang@...k-chips.com>,
	Sonny Rao <sonnyrao@...omium.org>,
	Olof Johansson <olof@...om.net>,
	Eddie Cai <eddie.cai@...k-chips.com>,
	Russell King <linux@....linux.org.uk>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] ARM: rockchip: rk3288: Switch to use the proper PWM
 IP

On Tue, Aug 19, 2014 at 08:18:54AM -0700, Doug Anderson wrote:
> Thierry,
> 
> On Tue, Aug 19, 2014 at 12:10 AM, Thierry Reding
> <thierry.reding@...il.com> wrote:
> > On Mon, Aug 18, 2014 at 10:09:06AM -0700, Doug Anderson wrote:
> >> The rk3288 SoC has an option to switch all of the PWMs in the system
> >> between the old IP block and the new IP block.  The new IP block is
> >> working and tested and the suggested PWM to use, so setup the SoC to
> >> use it and then we can pretend that the other IP block doesn't exist.

A few more questions as to how this actually works. Does it mean there
are two physically separate blocks (with different physical addresses)
to control the same PWM? And this register simply causes some of the
pins to be routed to one or another? As far as I recall there are a
number of instances of the PWM block, so the above would need to count
for all of them. Or are there separate bits for each of them?

> >> This code could go lots of other places, but we've put it here.  Why?
> >> - Pushing it to the bootloader just makes the code harder to update in
> >>   the field.  If we later find a bug in the new IP block and want to
> >>   change our mind about what to use we want it to be easy to update.

Depending on how this muxing works you won't be able to change your mind
anyway. If the IP blocks are different then the device tree will
effectively make the decision for you. So if you really want to be safe
you'd need to have code in the kernel that parses the device tree and
checks that all PWM instances are of the new type, then set this
register accordingly.

Or you don't set the bit from the driver and need to verify that device
tree and syscon match to avoid using the wrong controller.

> >> - Putting this code in the driver for IP block is a lot of extra work,
> >>   device tree bindings, etc.  Now that the new IP block is validated
> >>   it's likely no future SoCs will need this code.  Why pollute the PWM
> >>   driver with this?  This is an rk3288 thing so it should be in rk3288
> >>   code.
> >> - There's a single bit that switches over PWMs, which makes it extra
> >>   hard to put this under the PWM device tree nodes.
> >>
> >> Signed-off-by: Doug Anderson <dianders@...omium.org>
> >> ---
> >>  arch/arm/mach-rockchip/rockchip.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-rockchip/rockchip.c b/arch/arm/mach-rockchip/rockchip.c
> >> index 8ab9e0e..99133b9 100644
> >> --- a/arch/arm/mach-rockchip/rockchip.c
> >> +++ b/arch/arm/mach-rockchip/rockchip.c
> >> @@ -24,6 +24,24 @@
> >>  #include <asm/hardware/cache-l2x0.h>
> >>  #include "core.h"
> >>
> >> +static void __init rk3288_init_machine(void)
> >> +{
> >> +     void *grf = ioremap(0xff770000, 0x10000);
> >
> > This region of memory is part of the "grf" "syscon" device (according to
> > arch/arm/boot/dts/rk3288.dtsi) so the register should be accessed from
> > that driver. It looks as if no such driver currently exists, but given
> > the existence of the device tree node it's fair to assume that one will
> > eventually be merged.
> 
> The "grf" syscon device is the "general register file".  It's a
> collection of totally random registers stuffed together in one address
> space.  Sometimes a single 32-bit register has things you need to
> tweak for completely different subsystems.
> 
> Most drivers referene the syscon using this in dts:
>   rockchip,grf = <&grf>;
> 
> Then the drivers do:
>   grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> 
> 
> See the Rockchip i2c, pinctrl, or clock drivers for examples.

That's one way to do it. But if it's really just a one-time thing, then
you could easily perform the register write from the syscon driver where
the memory is already parsed from device tree and mapped. That way you
don't have to hardcode the physical address in some other random piece
of code and map the memory again.

> I could follow the lead of those subsystem and do the same thing, but
> I haven't because of the reasons talked about in the patch
> description.  To summarize: I thought it was cleaner and would have
> less baggage to carry to put this code in an rk3288-specific function.
> 
> There was no clean place to put rk3288-specific code such that it used
> the "syscon" interface like i2c/clk/pinctrl.  ...and adding a lot of
> infrastructure for something like that seems like a bit too much to
> me.  As it's written the code will never need to change (the physical
> address of GRF and this bit will always be right on rk3288) and
> hopefully nobody will need to think about it again.  ;)

I understand that it looks cleaner this way. But it's completely the
wrong way around. We're trying to move code out of arch/arm and into
proper drivers.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists