From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752861AbaHUQtv (ORCPT ); Thu, 21 Aug 2014 12:49:51 -0400 Received: from mail-wg0-f50.google.com ([74.125.82.50]:43560 "EHLO mail-wg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751997AbaHUQtt (ORCPT ); Thu, 21 Aug 2014 12:49:49 -0400 Date: Thu, 21 Aug 2014 18:49:45 +0200 From: Thierry Reding To: Tomasz Figa Cc: Doug Anderson , Heiko =?utf-8?Q?St=C3=BCbner?= , Caesar Wang , Sonny Rao , Olof Johansson , Eddie Cai , Russell King , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/4] ARM: rockchip: rk3288: Switch to use the proper PWM IP Message-ID: <20140821164944.GB1172@ulmo> References: <1408381749-14156-1-git-send-email-dianders@chromium.org> <20140820153801.GB3427@ulmo> <2927513.T62GI7nq9T@phil> <20140821063637.GB4486@ulmo> <53F61502.2010204@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="i0/AhcQY5QxfSsSZ" Content-Disposition: inline In-Reply-To: <53F61502.2010204@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --i0/AhcQY5QxfSsSZ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 21, 2014 at 05:49:22PM +0200, Tomasz Figa wrote: > On 21.08.2014 17:38, Doug Anderson wrote: > > Thierry, > >=20 > > On Wed, Aug 20, 2014 at 11:36 PM, Thierry Reding > > wrote: > >> On Wed, Aug 20, 2014 at 06:20:31PM +0200, Heiko St=C3=BCbner wrote: > >>> Am Mittwoch, 20. August 2014, 08:55:09 schrieb Doug Anderson: > >>>> Thierry, > >>>> > >>>> On Wed, Aug 20, 2014 at 8:38 AM, Thierry Reding > >>>> > >>>> 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 > >>>>>> > >>>>>> 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 > >>>>>>>> > >>>>>>>> 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 blo= ck 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 addre= sses) > >>>>>>> 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 g= ive > >>>>>> 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" bloc= k is > >>>>>> not documented in my TRM. > >>>>>> > >>>>>> 1a) It's entirely possible it's located at some memory address tha= t 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 bl= ock > >>>>>> are supposed to be "compatible" but that the old block is broken a= nd > >>>>>> thus isn't behaving properly. > >>>>>> > >>>>>> 1c) It's entirely possible that the old IP block and the new IP bl= ock > >>>>>> 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 contro= ls > >>>>>> all of the PWMs. My guess is that there's actually a single IP bl= ock > >>>>>> that implements all 4 PWMs. > >>>>> > >>>>> Looking at the register offsets in the device tree that seems likel= y. 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 stra= nge: > >>>>> pwm0: pwm@20030000 { > >>>>> > >>>>> ... > >>>>> reg =3D <0x20030000 0x10>; > >>>>> ... > >>>>> clocks =3D <&cru PCLK_PWM01>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> pwm1: pwm@20030010 { > >>>>> > >>>>> ... > >>>>> reg =3D <0x20030010 0x10>; > >>>>> ... > >>>>> clocks =3D <&cru PCLK_PWM01>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> ... > >>>>> > >>>>> pwm2: pwm@20050020 { > >>>>> > >>>>> ... > >>>>> reg =3D <0x20050020 0x10>; > >>>>> ... > >>>>> clocks =3D <&cru PCLK_PWM23>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> pwm3: pwm@20050030 { > >>>>> > >>>>> ... > >>>>> reg =3D <0x20050030 0x10>; > >>>>> ... > >>>>> clocks =3D <&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 chan= ged > >>> 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 sin= gle > >>>>> block or two/four, but I can't seem to find a reference to it. Mayb= e I'm > >>>>> confusing it with another driver. > >>>> > >>>> At this point it seems like the choice has already been made to hand= le > >>>> 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 u= pdate > >>>>>>>>>> in > >>>>>>>>>> > >>>>>>>>>> the field. If we later find a bug in the new IP block and w= ant > >>>>>>>>>> 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 yo= ur > >>>>>>> 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 b= e 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 t= he I2C > >>> the rk2928 and before contained the old IP, the rk3xxx SoCs did conta= in both > >>> old and new i2c IP and now the rk3288 only contains the new one, as t= he new IP > >>> seems to have proven stable. > >>> > >>> So there really is no incentive to use the old one if no drastic issu= e 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 100= 644 > >>>>>>>>>> --- a/arch/arm/mach-rockchip/rockchip.c > >>>>>>>>>> +++ b/arch/arm/mach-rockchip/rockchip.c > >>>>>>>>>> @@ -24,6 +24,24 @@ > >>>>>>>>>> > >>>>>>>>>> #include > >>>>>>>>>> #include "core.h" > >>>>>>>>>> > >>>>>>>>>> +static void __init rk3288_init_machine(void) > >>>>>>>>>> +{ > >>>>>>>>>> + void *grf =3D 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 access= ed > >>>>>>>>> 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 a= ddress > >>>>>>>> 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 =3D <&grf>; > >>>>>>>> > >>>>>>>> Then the drivers do: > >>>>>>>> grf =3D 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 wa= y 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 sys= con > >>>>> driver. But yes, subclassing it sounds like a reasonable thing to d= o. > >>>> > >>>> 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 gu= ess 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 a= nd keep > >>> this somewhat close to the driver and not scatter pwm settings into o= ther > >>> kernel parts. > >>> > >>> Hacking up the syscon driver feels bad to me, especially as it is mea= nt to be > >>> generic and export such shared registers to other drivers for just th= ese 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. > >=20 > > 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... > >=20 > > 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 =3D > > <>". 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. > >=20 > >=20 > > 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. >=20 > 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. >=20 > 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 --i0/AhcQY5QxfSsSZ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT9iMoAAoJEN0jrNd/PrOh+mQP/09xODe2lKTzT3G/soMiRBjz 5XrjAyqd87clmZJ0Ht5/4Wqjsw5cIbzQs+xSdCViUrQcPRDIpm0BIHOWMseLgOC9 IOkKuLHY+Sqou/8gWuZK8dEXHzqeAkcivUvoJye/YKSL5dtYd+SrZQQ5ycv0h/WO XUym/ofvV4z0IED9P5TdqEMIuXUbpq2+UCmwHQVdYcpAQJaY2WWH/7+PCLOGwZ21 dw8cQFLrhu9f/CQAtZE4kTpyYscDS2UyljSwMOtgFpxITNmQxZ9HCqwynVC+6vWd 0Jnp2tRSnrsI+LPEjUVA1rJTVZrW0oIKQtlN1+oZx3LAVAxJXIy546udwo1sGkGE MbQ19lqdO+1rHkrqsBk2BGEJde6B5kgeHhjuxq784bzB2hbLpG7+W7sos7+AMkJD Rpz5Lc798qw5tvMMHsGAHx8Sue+crLbJSMSebbUIxsGcl+6mcDgqaCJ9QQgnyQH6 UzAF6tm6I1sOs0oJUkLxPj10h7limeCXqHWVFAGTIk53ZLNOL6oQpSTNJc7IN1qQ NeTfcwJ2iRS2N5JDto/+RydspaMRo/3qdb6ns7cyG+QO2M3NWMGY17cE8uMaWljQ bYKWQ7bG5PdrtJKvrDuGfPfkyhEd7kJC4Zn45zXSGqw1fk274i0bgzVc2xQON2Xh dWp6k+hQYn6vwv3SioIp =70BO -----END PGP SIGNATURE----- --i0/AhcQY5QxfSsSZ-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: thierry.reding@gmail.com (Thierry Reding) Date: Thu, 21 Aug 2014 18:49:45 +0200 Subject: [PATCH 1/4] ARM: rockchip: rk3288: Switch to use the proper PWM IP In-Reply-To: <53F61502.2010204@gmail.com> References: <1408381749-14156-1-git-send-email-dianders@chromium.org> <20140820153801.GB3427@ulmo> <2927513.T62GI7nq9T@phil> <20140821063637.GB4486@ulmo> <53F61502.2010204@gmail.com> Message-ID: <20140821164944.GB1172@ulmo> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > 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 > >>>> > >>>> 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 > >>>>>> > >>>>>> 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 > >>>>>>>> > >>>>>>>> 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 at 20030000 { > >>>>> > >>>>> ... > >>>>> reg = <0x20030000 0x10>; > >>>>> ... > >>>>> clocks = <&cru PCLK_PWM01>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> pwm1: pwm at 20030010 { > >>>>> > >>>>> ... > >>>>> reg = <0x20030010 0x10>; > >>>>> ... > >>>>> clocks = <&cru PCLK_PWM01>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> ... > >>>>> > >>>>> pwm2: pwm at 20050020 { > >>>>> > >>>>> ... > >>>>> reg = <0x20050020 0x10>; > >>>>> ... > >>>>> clocks = <&cru PCLK_PWM23>; > >>>>> ... > >>>>> > >>>>> }; > >>>>> > >>>>> pwm3: pwm at 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). > >>> > >>> 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 > >>>>>>>>>> #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 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: