Messages in this thread Patch in this message | | | Date | Wed, 20 Aug 2014 17:38:05 +0200 | From | Thierry Reding <> | Subject | Re: [PATCH 1/4] ARM: rockchip: rk3288: Switch to use the proper PWM IP |
| |
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@gmail.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@gmail.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@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>; ... };
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.
> >> >> 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.
> >> >> 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 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. > > Yup, I understand that. I did ask for some advice before posting this > and I got the impression that folks thought that it would be fine to > put it here, though. I will let those folks clarify their thoughts > and/or correct my understanding.
Sure.
Thierry [unhandled content-type:application/pgp-signature] | |