From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Fri, 7 Feb 2014 14:48:31 +0100 Subject: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions In-Reply-To: <52F0BD94.1060601@redhat.com> References: <1390426587-16287-1-git-send-email-hdegoede@redhat.com> <1390426587-16287-3-git-send-email-hdegoede@redhat.com> <20140127144349.GJ3867@lukather> <52E67316.5020906@redhat.com> <20140128094427.GZ3867@lukather> <52E77FCD.5050701@redhat.com> <20140204094051.GL25625@lukather> <52F0BD94.1060601@redhat.com> Message-ID: <20140207134831.GJ3192@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Feb 04, 2014 at 11:14:44AM +0100, Hans de Goede wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi, > > On 02/04/2014 10:40 AM, Maxime Ripard wrote: > > Hi Hans, > > > > On Tue, Jan 28, 2014 at 11:00:45AM +0100, Hans de Goede wrote: > >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > >> > >> Hi, > >> > >> On 01/28/2014 10:44 AM, Maxime Ripard wrote: > >>> On Mon, Jan 27, 2014 at 03:54:14PM +0100, Hans de Goede wrote: > >>>>>> "allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13 > >>>>> > >>>>> Maybe we can just remove the gates from there? Even though they are gates, they are also (a bit) more than that. > >>>> > >>>> To be clear you mean s/usb-gates-clk/usb-clk/ right ? > >>> > >>> Yep, exactly > >>> > >>>>> I guess that means that we will have the OHCI0 gate declared with <&...-gates-clk 6>, while it's actually the first gate for this clock? > >>>> > >>>> Correct. > >>>> > >>>>> Maybe introducing an offset field in the gates_data would be a good idea, so that we always start from indexing the gates from 0 in the DT? > >>>> > >>>> Well for the other "gates" type clks we also have holes in the range, and we always refer to the clk with the bit number in the reg as the clock-cell value. > >>> > >>> Yes, we have holes, but I see two majors differences here: - the other gates are just gates, while the usb clocks are a bit more than that. > >> > >> The usb-clk registers contain more then that, but the bits we are talking about now are gates. > >> > >>> - the other gates' gating bits thus all start at bit 0, while - here, since it's kind of a "mixed" clock, the gating bits start - at bit 6 (on the A20 at least) > >> > >> Right, still I believe that the consistent thing to do is keeping the bit-number for the bit in the register controlling the gate as the specifier. When adding new dts entries / reviewing existing ones I'm used to matching the specifier to the bit-nr in the data-sheet, I think making things different just for this one register is counter productive. > > > > And if you turn it the other way around, it would be inconsistent that all gates indices start at 0, and we would start at 6 here :) > > I think the problem here is that you see the specifier part of the clk > phandle as an index, which it is not. All devicetree docs / code talks > about specifiers or arguments not indexes. Once you stop seeing this as > an index, you will hopefully also stop insisting this needs to > start at 0 :) > > Also note that it already is not an index for existing sunxi clks which have > cells != 0, as there are holes in the bits used in the gates registers and > calling the specifier an index suggest we're dealing with an array, and > arrays never have holes. > > The clk specifier as currently used in sunxi clks is a 1:1 mapping of the > gate register bit numbers, as is clearly documented in ie: > Documentation/devicetree/bindings/clock/sunxi/sun4i-a10-gates.txt > Where the datasheet is referenced as the source for (most) of the values > to put in the specifier. > > My biggest objection is that this would loose 1:1 mapping we currently > have between the specifier and bit-nr in the register, which really is > convenient when writing new dts bindings. > > When we add an offset users will need to first lookup which clk they need in > the datasheet and then look at both the dts bindings doc to find how this is > mapped to the specifier. In my experience such an extra level of indirection > in documentation is a PITA, and all that just so that some random number > (it is not an index!) can start at 0 ? > > To me adding an offset here and making the clk gates different form all > our other clock gates just feels wrong. Emilio pretty much share your feeling. I won't get in the way then :) I only had the compatible name comment left then. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions Date: Fri, 7 Feb 2014 14:48:31 +0100 Message-ID: <20140207134831.GJ3192@lukather> References: <1390426587-16287-1-git-send-email-hdegoede@redhat.com> <1390426587-16287-3-git-send-email-hdegoede@redhat.com> <20140127144349.GJ3867@lukather> <52E67316.5020906@redhat.com> <20140128094427.GZ3867@lukather> <52E77FCD.5050701@redhat.com> <20140204094051.GL25625@lukather> <52F0BD94.1060601@redhat.com> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="P6YfpwaDcfcOCJkJ" Return-path: Content-Disposition: inline In-Reply-To: <52F0BD94.1060601-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Subscribe: , List-Unsubscribe: , To: Hans de Goede Cc: Emilio Lopez , Mike Turquette , Philipp Zabel , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Roman Byshko List-Id: devicetree@vger.kernel.org --P6YfpwaDcfcOCJkJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 04, 2014 at 11:14:44AM +0100, Hans de Goede wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 >=20 > Hi, >=20 > On 02/04/2014 10:40 AM, Maxime Ripard wrote: > > Hi Hans, > >=20 > > On Tue, Jan 28, 2014 at 11:00:45AM +0100, Hans de Goede wrote: > >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > >>=20 > >> Hi, > >>=20 > >> On 01/28/2014 10:44 AM, Maxime Ripard wrote: > >>> On Mon, Jan 27, 2014 at 03:54:14PM +0100, Hans de Goede wrote: > >>>>>> "allwinner,sun5i-a13-usb-gates-clk" - for usb gates + resets on A13 > >>>>>=20 > >>>>> Maybe we can just remove the gates from there? Even though they are= gates, they are also (a bit) more than that. > >>>>=20 > >>>> To be clear you mean s/usb-gates-clk/usb-clk/ right ? > >>>=20 > >>> Yep, exactly > >>>=20 > >>>>> I guess that means that we will have the OHCI0 gate declared with <= &...-gates-clk 6>, while it's actually the first gate for this clock? > >>>>=20 > >>>> Correct. > >>>>=20 > >>>>> Maybe introducing an offset field in the gates_data would be a good= idea, so that we always start from indexing the gates from 0 in the DT? > >>>>=20 > >>>> Well for the other "gates" type clks we also have holes in the range= , and we always refer to the clk with the bit number in the reg as the cloc= k-cell value. > >>>=20 > >>> Yes, we have holes, but I see two majors differences here: - the othe= r gates are just gates, while the usb clocks are a bit more than that. > >>=20 > >> The usb-clk registers contain more then that, but the bits we are talk= ing about now are gates. > >>=20 > >>> - the other gates' gating bits thus all start at bit 0, while - here,= since it's kind of a "mixed" clock, the gating bits start - at bit 6 (on t= he A20 at least) > >>=20 > >> Right, still I believe that the consistent thing to do is keeping the = bit-number for the bit in the register controlling the gate as the specifie= r. When adding new dts entries / reviewing existing ones I'm used to match= ing the specifier to the bit-nr in the data-sheet, I think making things di= fferent just for this one register is counter productive. > >=20 > > And if you turn it the other way around, it would be inconsistent that = all gates indices start at 0, and we would start at 6 here :) >=20 > I think the problem here is that you see the specifier part of the clk > phandle as an index, which it is not. All devicetree docs / code talks > about specifiers or arguments not indexes. Once you stop seeing this as > an index, you will hopefully also stop insisting this needs to > start at 0 :) >=20 > Also note that it already is not an index for existing sunxi clks which h= ave > cells !=3D 0, as there are holes in the bits used in the gates registers = and > calling the specifier an index suggest we're dealing with an array, and > arrays never have holes. >=20 > The clk specifier as currently used in sunxi clks is a 1:1 mapping of the > gate register bit numbers, as is clearly documented in ie: > Documentation/devicetree/bindings/clock/sunxi/sun4i-a10-gates.txt > Where the datasheet is referenced as the source for (most) of the values > to put in the specifier. >=20 > My biggest objection is that this would loose 1:1 mapping we currently > have between the specifier and bit-nr in the register, which really is > convenient when writing new dts bindings. >=20 > When we add an offset users will need to first lookup which clk they need= in > the datasheet and then look at both the dts bindings doc to find how this= is > mapped to the specifier. In my experience such an extra level of indirect= ion > in documentation is a PITA, and all that just so that some random number > (it is not an index!) can start at 0 ? >=20 > To me adding an offset here and making the clk gates different form all > our other clock gates just feels wrong. Emilio pretty much share your feeling. I won't get in the way then :) I only had the compatible name comment left then. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --P6YfpwaDcfcOCJkJ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQIcBAEBAgAGBQJS9OQvAAoJEBx+YmzsjxAgnYwQALaxgDyQXp5sa336pRn1b+Og 2lf2yblABHAsop24ZO7tS455XEsQN6gyxuUHAswqvSG6b6dTR2F2dvwz34QxwhGe FfWumc1mOQb2lBJEyA6bFqTzRbeF1gqmZh6N72Vhncud7xlVXMdv/mH7aFyh3+vL luc6Ch6xrxIbtSclPcApIPZ7K8GT9LYcTPSisH2W2rueGXoefq5kjnZRQ9LZ4WKa BImD13njGwnWCRW61xfNLmMn7RZrv6O4zLtUGqxV+AZLq7h9N1KwgfIUuYnbWQtt 4lWD3VKscrZemAjL7fmGFV9yjS6tUIDAbCOWRGaFPA5qDxivXtnAx6oRLRWtIruF 8QWLOAKOQqfURgHgqBsCPET/QTlw1CbrQi6eAnSz7tGIP9P9e/8ZeB2eW7Dx0K+Q wvQ7TcRnMuhnSn28MBEiKVsTR5LP82k+jegbn/jggknfqT2VlxU0+Aar8/GmlmET 5X3EX26lT+aRyP09aBETXp96fJX858qiZCRiJDx3F1Fbq9JPAUFRzM6QddcnMmwJ 2pxjY5rmtoZ9C4kgoqIcfdIGn4/BRkrR0gDJMK7hjl6GX3kgYa3a3tbRexPE0Rhg d3pLpHblFL+9WanYRPAklOV9dmxUK9YTyi2037SoMJMGIjhylc2fGyZhzfUPMf68 ecbs6OoylfnGrNbpWp2i =Tyga -----END PGP SIGNATURE----- --P6YfpwaDcfcOCJkJ--