[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-02-07 Thread Hans de Goede

Hi,

On 02/07/2014 02:48 PM, Maxime Ripard wrote:

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.


Yes I've already fixed that in my latest sunxi-devel tree:
https://github.com/linux-sunxi/linux-sunxi/commits/sunxi-devel

Which is also rebased to 3.14-rc1 for those interested, I'll resend
the usb-clk patches with this + Emilio's reverted remark fixed soon-ish
then.

Regards,

Hans

--
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-02-07 Thread Maxime Ripard
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


signature.asc
Description: Digital signature


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-02-04 Thread Hans de Goede
-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.

Regards,

Hans
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLwvZMACgkQF3VEtJrzE/sz0gCfQNwhM/RpimtbhumvKKQ4a4V+
Vo4AoLTNKRZXlPC84hi1JInPGYvZIxuR
=dA68
-END PGP SIGNATURE-

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-02-04 Thread Maxime Ripard
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 :)

Plus, this clock is already a special case, since it's the only gate
that is more than just a gate so far.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-01-28 Thread Hans de Goede
-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.

Regards,

Hans

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLnf80ACgkQF3VEtJrzE/udugCdEDpN65hazG7H+FD45iOVnTY9
548An3dXeF6f8wp5REck5H3gqQPQkIoX
=6yba
-END PGP SIGNATURE-

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-01-28 Thread Maxime Ripard
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 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)


-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-01-27 Thread Hans de Goede
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Hi,

On 01/27/2014 03:43 PM, Maxime Ripard wrote:
> Hi Hans,
> 
> Mostly looking good, but I have a few comments below.
> 
> On Wed, Jan 22, 2014 at 10:36:24PM +0100, Hans de Goede wrote:
>> From: Roman Byshko 
>> 
>> Add register definitions for the usb-clk register found on sun4i, sun5i and 
>> sun7i SoCs.
>> 
>> Signed-off-by: Roman Byshko  Signed-off-by: Hans de Goede 
>>  --- Documentation/devicetree/bindings/clock/sunxi.txt 
>> |  5 + drivers/clk/sunxi/clk-sunxi.c | 12 
>>  2 files changed, 17 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt 
>> b/Documentation/devicetree/bindings/clock/sunxi.txt index 79c7197..8bccb6a 
>> 100644 --- a/Documentation/devicetree/bindings/clock/sunxi.txt +++ 
>> b/Documentation/devicetree/bindings/clock/sunxi.txt @@ -37,6 +37,8 @@ 
>> Required properties: "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 
>> gates on A31 "allwinner,sun4i-mod0-clk" - for the module 0 family of clocks 
>> "allwinner,sun7i-a20-out-clk" - for the external output clocks + 
>> "allwinner,sun4i-usb-gates-clk" - for usb gates + resets on A10 / A20 +
>> "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 ?

That sounds reasonable :)

> 
>> Required properties for all clocks: - reg : shall be the control register 
>> address for the clock. @@ -49,6 +51,9 @@ Required properties for all clocks: 
>> Additionally, "allwinner,*-gates-clk" clocks require: - clock-output-names : 
>> the corresponding gate names that the clock controls
>> 
>> +And "allwinner,*-usb-gates-clk" clocks also require: +- reset-cells : shall 
>> be set to 1 +
> 
> You should also document what value we should put in the cells, and where to 
> refer to to find the right one.

Ok.

> 
>> Clock consumers should specify the desired clocks they use with a "clocks" 
>> phandle cell. Consumers that are using a gated clock should provide an 
>> additional ID in their clock property. This ID is the diff --git 
>> a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index 
>> f1a147c..18cbc3c 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ 
>> b/drivers/clk/sunxi/clk-sunxi.c @@ -813,6 +813,16 @@ static const struct 
>> gates_data sun4i_ahb_gates_data __initconst = { .mask = {0x7F77FFF, 
>> 0x14FB3F}, };
>> 
>> +static const struct gates_data sun4i_usb_gates_data __initconst = { +   
>> .mask = {0x1C0}, +  .reset_mask = 0x07, +}; + +static const struct 
>> gates_data sun5i_a13_usb_gates_data __initconst = { +.mask = {0x140}, +  
>> .reset_mask = 0x03, +}; +
> 
> 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.

Here the hole just happens to be at the start, but it seems best to me
to be consistent and keep using the bit nr inside the reg as clock-cell
value, without an offset.

> 
>> static const struct gates_data sun5i_a10s_ahb_gates_data __initconst = { 
>> .mask = {0x147667e7, 0x185915}, }; @@ -1159,6 +1169,8 @@ static const struct 
>> of_device_id clk_gates_match[] __initconst = { {.compatible = 
>> "allwinner,sun6i-a31-apb1-gates-clk", .data = &sun6i_a31_apb1_gates_data,}, 
>> {.compatible = "allwinner,sun7i-a20-apb1-gates-clk", .data = 
>> &sun7i_a20_apb1_gates_data,}, {.compatible = 
>> "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,}, 
>> +  {.compatible = "allwinner,sun4i-usb-gates-clk", .data = 
>> &sun4i_usb_gates_data,}, +  {.compatible =
>> "allwinner,sun5i-a13-usb-gates-clk", .data = &sun5i_a13_usb_gates_data,}, {} 
>> };
> 
> Thanks a lot! Maxime

Regards,

Hans

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLmcxYACgkQF3VEtJrzE/vjVgCfT8pMd/WAl2lr5HVURDcr6zz6
pDsAnjStUQa3j6WGOHPstjO2kV3WwkKO
=ozuq
-END PGP SIGNATURE-

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


[linux-sunxi] Re: [PATCH v2 2/5] clk: sunxi: Add USB clock register defintions

2014-01-27 Thread Maxime Ripard
Hi Hans,

Mostly looking good, but I have a few comments below.

On Wed, Jan 22, 2014 at 10:36:24PM +0100, Hans de Goede wrote:
> From: Roman Byshko 
> 
> Add register definitions for the usb-clk register found on sun4i, sun5i and
> sun7i SoCs.
> 
> Signed-off-by: Roman Byshko 
> Signed-off-by: Hans de Goede 
> ---
>  Documentation/devicetree/bindings/clock/sunxi.txt |  5 +
>  drivers/clk/sunxi/clk-sunxi.c | 12 
>  2 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt 
> b/Documentation/devicetree/bindings/clock/sunxi.txt
> index 79c7197..8bccb6a 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -37,6 +37,8 @@ Required properties:
>   "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
>   "allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
>   "allwinner,sun7i-a20-out-clk" - for the external output clocks
> + "allwinner,sun4i-usb-gates-clk" - for usb gates + resets on A10 / A20
> + "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.

>  Required properties for all clocks:
>  - reg : shall be the control register address for the clock.
> @@ -49,6 +51,9 @@ Required properties for all clocks:
>  Additionally, "allwinner,*-gates-clk" clocks require:
>  - clock-output-names : the corresponding gate names that the clock controls
>  
> +And "allwinner,*-usb-gates-clk" clocks also require:
> +- reset-cells : shall be set to 1
> +

You should also document what value we should put in the cells, and
where to refer to to find the right one.

>  Clock consumers should specify the desired clocks they use with a
>  "clocks" phandle cell. Consumers that are using a gated clock should
>  provide an additional ID in their clock property. This ID is the
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index f1a147c..18cbc3c 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -813,6 +813,16 @@ static const struct gates_data sun4i_ahb_gates_data 
> __initconst = {
>   .mask = {0x7F77FFF, 0x14FB3F},
>  };
>  
> +static const struct gates_data sun4i_usb_gates_data __initconst = {
> + .mask = {0x1C0},
> + .reset_mask = 0x07,
> +};
> +
> +static const struct gates_data sun5i_a13_usb_gates_data __initconst = {
> + .mask = {0x140},
> + .reset_mask = 0x03,
> +};
> +

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?

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?

>  static const struct gates_data sun5i_a10s_ahb_gates_data __initconst = {
>   .mask = {0x147667e7, 0x185915},
>  };
> @@ -1159,6 +1169,8 @@ static const struct of_device_id clk_gates_match[] 
> __initconst = {
>   {.compatible = "allwinner,sun6i-a31-apb1-gates-clk", .data = 
> &sun6i_a31_apb1_gates_data,},
>   {.compatible = "allwinner,sun7i-a20-apb1-gates-clk", .data = 
> &sun7i_a20_apb1_gates_data,},
>   {.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = 
> &sun6i_a31_apb2_gates_data,},
> + {.compatible = "allwinner,sun4i-usb-gates-clk", .data = 
> &sun4i_usb_gates_data,},
> + {.compatible = "allwinner,sun5i-a13-usb-gates-clk", .data = 
> &sun5i_a13_usb_gates_data,},
>   {}
>  };

Thanks a lot!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature