Message-ID: <20140821164944.GB1172@ulmo>
Date:	Thu, 21 Aug 2014 18:49:45 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tomasz Figa <tomasz.figa@...il.com>
Cc:	Doug Anderson <dianders@...omium.org>,
	Heiko Stübner <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 Thu, Aug 21, 2014 at 05:49:22PM +0200, Tomasz Figa wrote:
> On 21.08.2014 17:38, Doug Anderson wrote:
> > Thierry,
> > 
> > On Wed, Aug 20, 2014 at 11:36 PM, Thierry Reding
> > <thierry.reding@...il.com> wrote:
> >> On Wed, Aug 20, 2014 at 06:20:31PM +0200, Heiko Stübner 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 :-) .
> >>>
> >>>>
> >>>> 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.
> >>>
> >>> Hacking up the syscon driver feels bad to me, especially as it is meant to be
> >>> generic and export such shared registers to other drivers for just these stuff.
> >>
> >> I think using syscon in the first place is bad. In my opinion it would
> >> be far better to export an explicit API from drivers that are currently
> >> "implemented" as syscon. The thing is, nothing about syscon is truly
> >> generic. All it provides is a memory-mapped I/O region and lets drivers
> >> do to that memory region whatever they wants. But ioremap() can be used
> >> for that purpose already. Yet we have infrastructure to prevent drivers
> >> from doing that (request_resource() and friends) because it's usually a
> >> bad idea. All syscon really gives us is a ratified way of doing things
> >> that are otherwise frowned upon.
> > 
> > Agreed that it's a bit awkward, but it's the generally accepted way of
> > doing things across multiple drivers as far as I can tell...
> > 
> > In exynos we were also doing this.  Another alternative (which I saw
> > used before syscon) was just to list a second address in the "reg =
> > <>".  The second address might only be 4 bytes big if only a single
> > 32-bit register was needed.  That started failing because sometimes
> > two drivers needed to access the same 32-bit register.  Added Tomasz
> > to this thread since I remember him being a fan of solving this with
> > syscon.
> > 
> > 
> > At the moment I'm not planning to spin this patch.  If folks come up
> > with a solution that they definitely like better I'm happy to spin it,
> > but for now this seems to work and doesn't seem (to me) to be terribly
> > worse than the alternatives proposed so far.
> 
> So, in fact, I'm really a fan of the kind of solutions proposed by
> Thierry. My idea of handling this kind of integration details is that we
> should rather have a PMU driver on Exynos and it should be exporting all
> the various functions to configure certain subtle bits without the IP
> driver really knowing about SoC specifics. The PMU driver would know
> which bits in which registers to set up depending on SoC compatible
> string or data in PMU's device tree node.
> 
> I've been recommending the use of syscon for this purpose mostly because
> few times before I received negative opinions about the idea of private
> APIs like this and I simply didn't have time to push for them.

syscon is in fact not different from a private API. Except that the API
takes the form of arbitrary register accesses.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists