Message-ID: <CAD=FV=U_TEi7zh=LcnfTz07yFCrOXo1SsGJ0h6-VFCVdy1GDEA@mail.gmail.com>
Date:	Wed, 20 Aug 2014 09:27:02 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Heiko Stübner <heiko@...ech.de>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	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

Heiko,

On Wed, Aug 20, 2014 at 9:20 AM, Heiko Stübner <heiko@...ech.de> wrote:
> Am Mittwoch, 20. August 2014, 08:55:09 schrieb Doug Anderson:
>> Thierry,
>>
>> On Wed, Aug 20, 2014 at 8:38 AM, Thierry Reding
>>
>> <thierry.reding@...il.com> wrote:
>> > On Wed, Aug 20, 2014 at 08:20:53AM -0700, Doug Anderson wrote:
>> >> Thierry,
>> >>
>> >> On Tue, Aug 19, 2014 at 11:08 PM, Thierry Reding
>> >>
>> >> <thierry.reding@...il.com> wrote:
>> >> > 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?
>> >>
>> >> All I have is the TRM (technical reference manual) which doesn't give
>> >> me much more info than I've provided you.  But I can answer some of
>> >> your questoins:
>> >>
>> >> 1. If there are two physically separate blocks then the "old" block is
>> >> not documented in my TRM.
>> >>
>> >> 1a) It's entirely possible it's located at some memory address that is
>> >> marked "Reserved" in the TRM, but I have no idea.
>> >>
>> >> 1b) It's entirely possible that the old IP block and the new IP block
>> >> are supposed to be "compatible" but that the old block is broken and
>> >> thus isn't behaving properly.
>> >>
>> >> 1c) It's entirely possible that the old IP block and the new IP block
>> >> are located at the same physical addresses but somehow work
>> >> differently.  If so, the old IP block isn't documented.
>> >>
>> >>
>> >> 2. As per the patch description, there is a single bit that controls
>> >> all of the PWMs.  My guess is that there's actually a single IP block
>> >> that implements all 4 PWMs.
>> >
>> > 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@...30000 {
>> >
>> >                 ...
>> >                 reg = <0x20030000 0x10>;
>> >                 ...
>> >                 clocks = <&cru PCLK_PWM01>;
>> >                 ...
>> >
>> >         };
>> >
>> >         pwm1: pwm@...30010 {
>> >
>> >                 ...
>> >                 reg = <0x20030010 0x10>;
>> >                 ...
>> >                 clocks = <&cru PCLK_PWM01>;
>> >                 ...
>> >
>> >         };
>> >
>> >         ...
>> >
>> >         pwm2: pwm@...50020 {
>> >
>> >                 ...
>> >                 reg = <0x20050020 0x10>;
>> >                 ...
>> >                 clocks = <&cru PCLK_PWM23>;
>> >                 ...
>> >
>> >         };
>> >
>> >         pwm3: pwm@...50030 {
>> >
>> >                 ...
>> >                 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).
>
> It did sound like a nice idea at the time to hold the common stuff of
> rk3066/rk3188 and all their derivatives and I assumed a SoC that changed
> dramatically (including the core) would be called 4xxx or so :-) .

Yes, I've fallen into the same trap.  Now I jump on the bandwagon and
name things arbitrarily by the first machine that had them.  It's
confusing, but sorta less confusing too.


>> 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...
>>
>> >> >> >> 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.
>> >>
>> >> Since there is no documentation about how you would instantiate the
>> >> "old" type in the TRM and no good reason I can think of why someone
>> >> would want to do this, it doesn't seem super fruitful.
>> >
>> > Okay, so if it's not at all documented and never used then yes, we'd
>> > better just ignore it.
>>
>> Heiko just pointed me at the base address for the other block.
>> There's nothing in the rk3288 TRM about it, but we can see the base
>> address.  We could probably guess that it behaves the same as the
>> older PWM if we need to.  I'm still not convinced there's a good
>> reason for someone to use it.
>
> From what I understood the old one was included as a fallback in case some
> drastic problem appeared with the newly developed IP. Similarly for the I2C
> the rk2928 and before contained the old IP, the rk3xxx SoCs did contain both
> old and new i2c IP and now the rk3288 only contains the new one, as the new IP
> seems to have proven stable.
>
> So there really is no incentive to use the old one if no drastic issue has
> appeared with the new one until now.
>
>
>> >> >> >> 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.
>> >>
>> >> Well, except that we're using the general "syscon" driver.  I could
>> >> create a whole new driver that "subclasses" this syscon driver I
>> >> suppose.
>> >
>> > Ah, I wasn't aware that there was even something like a generic syscon
>> > driver. But yes, subclassing it sounds like a reasonable thing to do.
>>
>> I will do that if need be, but it's not my favorite.  I will let
>> others chime in.
>
> I guess personally I like the idea best of just setting the relevant bit in
> _probe of the pwm driver, like the i2c driver does:
>
> if (of_device_is_compatible(np, "rockchip,rk3288-pwm") {
>         /* get regmap and set bit */
> }
>
> The downside would be that the bit would be written 4 times, but I guess this
> shouldn't matter to much. And I don't think anybody will get the idea of
> combining both ip variants in one dts anyway.
> And of course in the next SoC the old IP will mostly have gone away and keep
> this somewhat close to the driver and not scatter pwm settings into other
> kernel parts.

I will try to spin this up today.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists