[PATCH 1/2] clk: fixed-rate: use full DT node name

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Stephen Warren]

clk-fixed-rate currently names clocks according to a node's name without
the unit address. When faced with the legal and technically correct DT
structure below, this causes rgistration attempts for 3 clocks with the
same name, 2 of which fail.

	clocks {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <0>;

		clk_mmc: clock at 0 {
			compatible = "fixed-clock";
			reg = <0>;
...
		clk_i2c: clock at 1 {
			compatible = "fixed-clock";
			reg = <1>;
...
		clk_spi: clock at 2 {
			compatible = "fixed-clock";
			reg = <2>;
...

Solve this by naming the clocks after the full node name rather than the
short version (e.g. /clocks/clock at 0).

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
Note that if this is accepted, I intend to submit a patch for the RPi DTS
which uses the naming structure above, so it might be useful to place this
patch in its own branch. Or, I could submit the cleanup after 3.15-rc1.
---
 drivers/clk/clk-fixed-rate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 0fc56ab..3335b3c 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(clk_register_fixed_rate);
 void of_fixed_clk_setup(struct device_node *node)
 {
 	struct clk *clk;
-	const char *clk_name = node->name;
+	const char *clk_name = node->full_name;
 	u32 rate;
 	u32 accuracy = 0;
 
-- 
1.8.3.2

[PATCH 2/2] ARM: bcm2835: fix clock DT node names

[Stephen Warren]

DT nodes should be named according to the type of object that they
represent rather than the identity. DT nodes that contain a reg
property should include a unit address in their name. Fix these issues.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
This is the bcm2835 patch which depends on patch 1. I guess this could
also go through the clk tree if that makes it easier, although there's
always the small risk of conflicts if you do that.

 arch/arm/boot/dts/bcm2835.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index a2d4889..1cf1ae7 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -155,21 +155,21 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		clk_mmc: mmc {
+		clk_mmc: clock at 0 {
 			compatible = "fixed-clock";
 			reg = <0>;
 			#clock-cells = <0>;
 			clock-frequency = <100000000>;
 		};
 
-		clk_i2c: i2c {
+		clk_i2c: clock at 1 {
 			compatible = "fixed-clock";
 			reg = <1>;
 			#clock-cells = <0>;
 			clock-frequency = <250000000>;
 		};
 
-		clk_spi: spi {
+		clk_spi: clock at 2 {
 			compatible = "fixed-clock";
 			reg = <2>;
 			#clock-cells = <0>;
-- 
1.8.3.2

[PATCH 2/2] ARM: bcm2835: fix clock DT node names

[Mark Rutland]

On Fri, Feb 14, 2014 at 06:16:53AM +0000, Stephen Warren wrote:
> DT nodes should be named according to the type of object that they
> represent rather than the identity. DT nodes that contain a reg
> property should include a unit address in their name. Fix these issues.

As mentioned on patch one, I don't think this makes sense. clock at N is
simply not correct, and if simple-bus weren't being abused it would be
far clearer that that is the case.

Please use clock_N rather than clock at N here, and get rid of the
meaningless reg values.

The simple-bus should either be removed or fixed up to meet the
requirements of the simple-bus binding (i.e. add a ranges property). I
would prefer the former.

Thanks,
Mark.

> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> This is the bcm2835 patch which depends on patch 1. I guess this could
> also go through the clk tree if that makes it easier, although there's
> always the small risk of conflicts if you do that.
> 
>  arch/arm/boot/dts/bcm2835.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index a2d4889..1cf1ae7 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -155,21 +155,21 @@
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> -		clk_mmc: mmc {
> +		clk_mmc: clock at 0 {
>  			compatible = "fixed-clock";
>  			reg = <0>;
>  			#clock-cells = <0>;
>  			clock-frequency = <100000000>;
>  		};
>  
> -		clk_i2c: i2c {
> +		clk_i2c: clock at 1 {
>  			compatible = "fixed-clock";
>  			reg = <1>;
>  			#clock-cells = <0>;
>  			clock-frequency = <250000000>;
>  		};
>  
> -		clk_spi: spi {
> +		clk_spi: clock at 2 {
>  			compatible = "fixed-clock";
>  			reg = <2>;
>  			#clock-cells = <0>;
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Mark Rutland]

On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
> clk-fixed-rate currently names clocks according to a node's name without
> the unit address. When faced with the legal and technically correct DT
> structure below, this causes rgistration attempts for 3 clocks with the
> same name, 2 of which fail.
> 
> 	clocks {
> 		compatible = "simple-bus";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		clk_mmc: clock at 0 {
> 			compatible = "fixed-clock";
> 			reg = <0>;
> ...
> 		clk_i2c: clock at 1 {
> 			compatible = "fixed-clock";
> 			reg = <1>;
> ...
> 		clk_spi: clock at 2 {
> 			compatible = "fixed-clock";
> 			reg = <2>;
> ...

I'd argue that this case isn't valid.

The fixed-clock binding doesn't define a reg, yet simple bus binding
implies that the reg property of child nodes should be interpretted as
the same address space as their parent (MMIO in this case?). The
fixed-clock nodes reg proeprties clearly aren't MMIO addresses.

Additionally, the _requred_ ranges property is missing.

It's just nonsensical; rename them to clock_{0,1,..} instead and get rid
of the reg properties. Then they're named uniquely.

However, for cases where the reg value is meaningful the below patch
makes sense.

Thanks,
Mark.

> 
> Solve this by naming the clocks after the full node name rather than the
> short version (e.g. /clocks/clock at 0).
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> Note that if this is accepted, I intend to submit a patch for the RPi DTS
> which uses the naming structure above, so it might be useful to place this
> patch in its own branch. Or, I could submit the cleanup after 3.15-rc1.
> ---
>  drivers/clk/clk-fixed-rate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index 0fc56ab..3335b3c 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(clk_register_fixed_rate);
>  void of_fixed_clk_setup(struct device_node *node)
>  {
>  	struct clk *clk;
> -	const char *clk_name = node->name;
> +	const char *clk_name = node->full_name;
>  	u32 rate;
>  	u32 accuracy = 0;
>  
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Stephen Warren]

On 02/14/2014 03:35 AM, Mark Rutland wrote:
> On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
>> clk-fixed-rate currently names clocks according to a node's name without
>> the unit address. When faced with the legal and technically correct DT
>> structure below, this causes rgistration attempts for 3 clocks with the
>> same name, 2 of which fail.
>>
>> 	clocks {
>> 		compatible = "simple-bus";
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		clk_mmc: clock at 0 {
>> 			compatible = "fixed-clock";
>> 			reg = <0>;
>> ...
>> 		clk_i2c: clock at 1 {
>> 			compatible = "fixed-clock";
>> 			reg = <1>;
>> ...
>> 		clk_spi: clock at 2 {
>> 			compatible = "fixed-clock";
>> 			reg = <2>;
>> ...
> 
> I'd argue that this case isn't valid.

Well, it's very widely used, and was the result of numerous discussions
of how this kind of thing should be represented:-/

> The fixed-clock binding doesn't define a reg, yet simple bus binding
> implies that the reg property of child nodes should be interpretted as
> the same address space as their parent (MMIO in this case?). The
> fixed-clock nodes reg proeprties clearly aren't MMIO addresses.
> 
> Additionally, the _requred_ ranges property is missing.

Perhaps we need to invent a simple-container instead then?

> It's just nonsensical; rename them to clock_{0,1,..} instead and get rid
> of the reg properties. Then they're named uniquely.

That's not legal either. DT node names are supposed to represent the
type of device/object (i.e. just "clock"), not the identity of the
device/object (i.e. not include IDs etc.). Hence, the node name needs to
be "clock" for all of them, and the unit address must be used to
differentiate them.

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Mark Rutland]

On Fri, Feb 14, 2014 at 04:43:04PM +0000, Stephen Warren wrote:
> On 02/14/2014 03:35 AM, Mark Rutland wrote:
> > On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
> >> clk-fixed-rate currently names clocks according to a node's name without
> >> the unit address. When faced with the legal and technically correct DT
> >> structure below, this causes rgistration attempts for 3 clocks with the
> >> same name, 2 of which fail.
> >>
> >> 	clocks {
> >> 		compatible = "simple-bus";
> >> 		#address-cells = <1>;
> >> 		#size-cells = <0>;
> >>
> >> 		clk_mmc: clock at 0 {
> >> 			compatible = "fixed-clock";
> >> 			reg = <0>;
> >> ...
> >> 		clk_i2c: clock at 1 {
> >> 			compatible = "fixed-clock";
> >> 			reg = <1>;
> >> ...
> >> 		clk_spi: clock at 2 {
> >> 			compatible = "fixed-clock";
> >> 			reg = <2>;
> >> ...
> > 
> > I'd argue that this case isn't valid.
> 
> Well, it's very widely used, and was the result of numerous discussions
> of how this kind of thing should be represented:-/

Maybe we have to live with it then. :/

> 
> > The fixed-clock binding doesn't define a reg, yet simple bus binding
> > implies that the reg property of child nodes should be interpretted as
> > the same address space as their parent (MMIO in this case?). The
> > fixed-clock nodes reg proeprties clearly aren't MMIO addresses.
> > 
> > Additionally, the _requred_ ranges property is missing.
> 
> Perhaps we need to invent a simple-container instead then?

As you mention in your other reply, it makes some sense to omit ranges
if the address space is disjoint. My concern is that the address space
is meaningless and arbitrary. If we had a real disjoint address space
we'd have another kind of bus node as the container.

> 
> > It's just nonsensical; rename them to clock_{0,1,..} instead and get rid
> > of the reg properties. Then they're named uniquely.
> 
> That's not legal either. DT node names are supposed to represent the
> type of device/object (i.e. just "clock"), not the identity of the
> device/object (i.e. not include IDs etc.). Hence, the node name needs to
> be "clock" for all of them, and the unit address must be used to
> differentiate them.

As far as I can see from ePAPR, the only requriement is:

  The node-name shall start with a lower or uppercase character and
  should describe the general class of device.

IMO clock_1 describes the general class of device as well as clock at 1,
while also not filling a unexpected property with a meaningless value.

Thanks,
Mark.

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Stephen Warren]

On 02/18/2014 04:23 AM, Mark Rutland wrote:
> On Fri, Feb 14, 2014 at 04:43:04PM +0000, Stephen Warren wrote:
>> On 02/14/2014 03:35 AM, Mark Rutland wrote:
>>> On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
>>>> clk-fixed-rate currently names clocks according to a node's name without
>>>> the unit address. When faced with the legal and technically correct DT
>>>> structure below, this causes rgistration attempts for 3 clocks with the
>>>> same name, 2 of which fail.
>>>>
>>>> 	clocks {
>>>> 		compatible = "simple-bus";
>>>> 		#address-cells = <1>;
>>>> 		#size-cells = <0>;
>>>>
>>>> 		clk_mmc: clock at 0 {
>>>> 			compatible = "fixed-clock";
>>>> 			reg = <0>;
>>>> ...
>>>> 		clk_i2c: clock at 1 {
>>>> 			compatible = "fixed-clock";
>>>> 			reg = <1>;
>>>> ...
>>>> 		clk_spi: clock at 2 {
>>>> 			compatible = "fixed-clock";
>>>> 			reg = <2>;
>>>> ...
>>>
>>> I'd argue that this case isn't valid.
>>
>> Well, it's very widely used, and was the result of numerous discussions
>> of how this kind of thing should be represented:-/
> 
> Maybe we have to live with it then. :/

Great:-)

...
>>> It's just nonsensical; rename them to clock_{0,1,..} instead and get rid
>>> of the reg properties. Then they're named uniquely.
>>
>> That's not legal either. DT node names are supposed to represent the
>> type of device/object (i.e. just "clock"), not the identity of the
>> device/object (i.e. not include IDs etc.). Hence, the node name needs to
>> be "clock" for all of them, and the unit address must be used to
>> differentiate them.
> 
> As far as I can see from ePAPR, the only requriement is:
> 
>   The node-name shall start with a lower or uppercase character and
>   should describe the general class of device.
> 
> IMO clock_1 describes the general class of device as well as clock at 1,
> while also not filling a unexpected property with a meaningless value.

I believe section 2.2.2 "Generic Names Recommendation" is the source of
the rule that nodes should be named after the type of object rather than
the identity.

"""
The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model. If appropriate, the
name should be one of the following choices:

* atm
* cache-controller
* compact-flash
* can
* cpu
...
"""

(I note e.g. "cpu" not "cpu-1", "cpu_2", etc.)

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Stephen Warren]

On 02/14/2014 03:35 AM, Mark Rutland wrote:
> On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
>> clk-fixed-rate currently names clocks according to a node's name without
>> the unit address. When faced with the legal and technically correct DT
>> structure below, this causes rgistration attempts for 3 clocks with the
>> same name, 2 of which fail.
>>
>> 	clocks {
>> 		compatible = "simple-bus";
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		clk_mmc: clock at 0 {
>> 			compatible = "fixed-clock";
>> 			reg = <0>;
>> ...
>> 		clk_i2c: clock at 1 {
>> 			compatible = "fixed-clock";
>> 			reg = <1>;
>> ...
>> 		clk_spi: clock at 2 {
>> 			compatible = "fixed-clock";
>> 			reg = <2>;
>> ...
> 
> I'd argue that this case isn't valid.
> 
> The fixed-clock binding doesn't define a reg, yet simple bus binding
> implies that the reg property of child nodes should be interpretted as
> the same address space as their parent (MMIO in this case?). The
> fixed-clock nodes reg proeprties clearly aren't MMIO addresses.
> 
> Additionally, the _requred_ ranges property is missing.

Oh, IIRC that was deliberate to indicate that the child address space
was disjoint from the parent address space.

[PATCH 1/2] clk: fixed-rate: use full DT node name

[Emilio López]

Hello Stephen,

El 14/02/14 03:16, Stephen Warren escribi?:
> clk-fixed-rate currently names clocks according to a node's name without
> the unit address. When faced with the legal and technically correct DT
> structure below, this causes rgistration attempts for 3 clocks with the
> same name, 2 of which fail.
>
> 	clocks {
> 		compatible = "simple-bus";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
>
> 		clk_mmc: clock at 0 {
> 			compatible = "fixed-clock";
> 			reg = <0>;
> ...
> 		clk_i2c: clock at 1 {
> 			compatible = "fixed-clock";
> 			reg = <1>;
> ...
> 		clk_spi: clock at 2 {
> 			compatible = "fixed-clock";
> 			reg = <2>;
> ...
>
> Solve this by naming the clocks after the full node name rather than the
> short version (e.g. /clocks/clock at 0).

An alternative that doesn't require any change in the driver would be to 
use the optional but recommended clock-output-names property from the 
common clock binding.

Your DT would then look something like the following

 > ...
 > 		clk_mmc: clk at 0 {
 > 			compatible = "fixed-clock";
 > 			clock-output-names = "mmc";
 > ...
 > 		clk_i2c: clk at 1 {
 > 			compatible = "fixed-clock";
 > 			clock-output-names = "i2c";
 > ...
 > 		clk_spi: clk at 2 {
 > 			compatible = "fixed-clock";
 > 			clock-output-names = "spi";
 > ...

Cheers,

Emilio