[PATCH] of: Add a reg-names property to name reg entries

[PATCH] of: Add a reg-names property to name reg entries

[Benoit Cousson]

In a HW system, resources in general have name to identify them.
The is the case as well for DT "reg" entries.
The current DT mechanism is relying on the "reg" order to identify
the proper resource.
Add a reg-names property to allow the possiblity to provide a name
to any reg entries.
If the name is available, use it to name the resource, otherwise
keep the legacy device name.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---

 Documentation/devicetree/bindings/reg-names.txt |   48 +++++++++++++++++++++++
 drivers/of/address.c                            |   22 ++++++++--
 2 files changed, 65 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reg-names.txt

diff --git a/Documentation/devicetree/bindings/reg-names.txt b/Documentation/devicetree/bindings/reg-names.txt
new file mode 100644
index 0000000..5554065
--- /dev/null
+++ b/Documentation/devicetree/bindings/reg-names.txt
@@ -0,0 +1,48 @@
+reg-names property
+
+In a HW system, resources in general have name to identify them.
+The is the case as well for register entries.
+The current DT mechanism is relying on the "reg" order to identify
+the proper resource. The reg-names is adding the possiblity to
+provide a name to reg entries.
+
+Usage:
+
+This attribute must be used along with a regular reg entry. If not
+it will be simply ignored.
+The number of entry must match otherwise the default device name will
+be used to as the resource name.
+
+
+Example:
+
+
+l4-abe {
+	compatible = "simple-bus";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
+		 <1 0 0x49000000 0x00001000>; /* L3 path */
+	mcasp {
+		compatible = "ti,mcasp";
+		reg = <0 0x10 0x10>, <0 0x20 0x10>,
+		      <1 0x10 0x10>, <1 0x20 0x10>;
+		reg-names = "mpu", "dat",
+			    "dma", "dma_dat";
+	};
+
+	timer {
+		compatible = "ti,timer";
+		reg = <0 0x40 0x10>, <1 0x40 0x10>;
+		reg-names = "mpu", "dma";
+	};
+};
+
+
+usb {
+	compatible = "ti,usb-host";
+	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
+	      <0x4a064c00 0x200>;
+	reg-names = "config", "ohci", "ehci";
+};
+
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 72c33fb..1f9f8cb 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -14,7 +14,7 @@
 static struct of_bus *of_match_bus(struct device_node *np);
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r);
+		const char *name, struct resource *r);
 
 /* Debug utility */
 #ifdef DEBUG
@@ -215,7 +215,7 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
 	addrp = of_get_pci_address(dev, bar, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
 }
 EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
 #endif /* CONFIG_PCI */
@@ -529,7 +529,7 @@ EXPORT_SYMBOL(of_get_address);
 
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r)
+		const char *name, struct resource *r)
 {
 	u64 taddr;
 
@@ -551,7 +551,11 @@ static int __of_address_to_resource(struct device_node *dev,
 		r->end = taddr + size - 1;
 	}
 	r->flags = flags;
-	r->name = dev->full_name;
+	if (name)
+		r->name = name;
+	else
+		r->name = dev->full_name;
+
 	return 0;
 }
 
@@ -569,11 +573,19 @@ int of_address_to_resource(struct device_node *dev, int index,
 	const __be32	*addrp;
 	u64		size;
 	unsigned int	flags;
+	const char	*name = NULL;
+	int		name_cnt;
 
 	addrp = of_get_address(dev, index, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+
+	/* Get "reg-names" property to add a name to a resource */
+	name_cnt = of_property_count_strings(dev, "reg-names");
+	if (name_cnt > 0)
+		of_property_read_string_index(dev, "reg-names",	index, &name);
+
+	return __of_address_to_resource(dev, addrp, size, flags, name, r);
 }
 EXPORT_SYMBOL_GPL(of_address_to_resource);
 
-- 
1.7.5.4

Re: [PATCH] of: Add a reg-names property to name reg entries

[Grant Likely]

On Mon, Oct 24, 2011 at 05:54:57PM +0200, Benoit Cousson wrote:
> In a HW system, resources in general have name to identify them.
> The is the case as well for DT "reg" entries.
> The current DT mechanism is relying on the "reg" order to identify
> the proper resource.
> Add a reg-names property to allow the possiblity to provide a name
> to any reg entries.
> If the name is available, use it to name the resource, otherwise
> keep the legacy device name.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> ---
> 
>  Documentation/devicetree/bindings/reg-names.txt |   48 +++++++++++++++++++++++
>  drivers/of/address.c                            |   22 ++++++++--
>  2 files changed, 65 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/reg-names.txt
> 
> diff --git a/Documentation/devicetree/bindings/reg-names.txt b/Documentation/devicetree/bindings/reg-names.txt
> new file mode 100644
> index 0000000..5554065
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reg-names.txt
> @@ -0,0 +1,48 @@
> +reg-names property
> +
> +In a HW system, resources in general have name to identify them.
> +The is the case as well for register entries.
> +The current DT mechanism is relying on the "reg" order to identify
> +the proper resource. The reg-names is adding the possiblity to
> +provide a name to reg entries.
> +
> +Usage:
> +
> +This attribute must be used along with a regular reg entry. If not
> +it will be simply ignored.
> +The number of entry must match otherwise the default device name will
> +be used to as the resource name.
> +
> +
> +Example:
> +
> +
> +l4-abe {
> +	compatible = "simple-bus";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
> +		 <1 0 0x49000000 0x00001000>; /* L3 path */
> +	mcasp {
> +		compatible = "ti,mcasp";
> +		reg = <0 0x10 0x10>, <0 0x20 0x10>,
> +		      <1 0x10 0x10>, <1 0x20 0x10>;
> +		reg-names = "mpu", "dat",
> +			    "dma", "dma_dat";
> +	};
> +
> +	timer {
> +		compatible = "ti,timer";
> +		reg = <0 0x40 0x10>, <1 0x40 0x10>;
> +		reg-names = "mpu", "dma";
> +	};
> +};
> +
> +
> +usb {
> +	compatible = "ti,usb-host";
> +	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
> +	      <0x4a064c00 0x200>;
> +	reg-names = "config", "ohci", "ehci";
> +};
> +
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 72c33fb..1f9f8cb 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -14,7 +14,7 @@
>  static struct of_bus *of_match_bus(struct device_node *np);
>  static int __of_address_to_resource(struct device_node *dev,
>  		const __be32 *addrp, u64 size, unsigned int flags,
> -				    struct resource *r);
> +		const char *name, struct resource *r);
>  
>  /* Debug utility */
>  #ifdef DEBUG
> @@ -215,7 +215,7 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
>  	addrp = of_get_pci_address(dev, bar, &size, &flags);
>  	if (addrp == NULL)
>  		return -EINVAL;
> -	return __of_address_to_resource(dev, addrp, size, flags, r);
> +	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
>  }
>  EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
>  #endif /* CONFIG_PCI */
> @@ -529,7 +529,7 @@ EXPORT_SYMBOL(of_get_address);
>  
>  static int __of_address_to_resource(struct device_node *dev,
>  		const __be32 *addrp, u64 size, unsigned int flags,
> -				    struct resource *r)
> +		const char *name, struct resource *r)
>  {
>  	u64 taddr;
>  
> @@ -551,7 +551,11 @@ static int __of_address_to_resource(struct device_node *dev,
>  		r->end = taddr + size - 1;
>  	}
>  	r->flags = flags;
> -	r->name = dev->full_name;
> +	if (name)
> +		r->name = name;
> +	else
> +		r->name = dev->full_name;

This form please:
	r->name = name ? name : dev->full_name;

In general, I'm inclined to accept this patch as we talked about at
plumbers.  However, this particular hunk gives me pause as there is
still the objection that Russell raised about the (ab)use of r->name
for insert the resource name.

So, no I won't reject this patch, but I first what to have some idea
of what the plan is to migrate away from using r->name.  Perhaps we
can talk about this tomorrow.

>  	return 0;
>  }
>  
> @@ -569,11 +573,19 @@ int of_address_to_resource(struct device_node *dev, int index,
>  	const __be32	*addrp;
>  	u64		size;
>  	unsigned int	flags;
> +	const char	*name = NULL;
> +	int		name_cnt;
>  
>  	addrp = of_get_address(dev, index, &size, &flags);
>  	if (addrp == NULL)
>  		return -EINVAL;
> -	return __of_address_to_resource(dev, addrp, size, flags, r);
> +
> +	/* Get "reg-names" property to add a name to a resource */
> +	name_cnt = of_property_count_strings(dev, "reg-names");
> +	if (name_cnt > 0)
> +		of_property_read_string_index(dev, "reg-names",	index, &name);

Why not simply:
	of_property_read_string_index(dev, "reg-names", index, &name);

If it fails, then name remains NULL and everything works.


> +
> +	return __of_address_to_resource(dev, addrp, size, flags, name, r);
>  }
>  EXPORT_SYMBOL_GPL(of_address_to_resource);
>  
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] of: Add a reg-names property to name reg entries

[Cousson, Benoit]

On 10/25/2011 12:19 AM, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 05:54:57PM +0200, Benoit Cousson wrote:

[...]

>> @@ -551,7 +551,11 @@ static int __of_address_to_resource(struct device_node *dev,
>>   		r->end = taddr + size - 1;
>>   	}
>>   	r->flags = flags;
>> -	r->name = dev->full_name;
>> +	if (name)
>> +		r->name = name;
>> +	else
>> +		r->name = dev->full_name;
>
> This form please:
> 	r->name = name ? name : dev->full_name;

Much better, indeed.

> In general, I'm inclined to accept this patch as we talked about at
> plumbers.  However, this particular hunk gives me pause as there is
> still the objection that Russell raised about the (ab)use of r->name
> for insert the resource name.
>
> So, no I won't reject this patch, but I first what to have some idea
> of what the plan is to migrate away from using r->name.  Perhaps we
> can talk about this tomorrow.

OK, sure, let's discuss about that.
But I still think that Russell's concern is not related at all with this 
patch. That means that it should be fixed separately if needed. It can 
be done before or after, but this is somehow orthogonal to the DT 
reg-names support problem.

>>   	return 0;
>>   }
>>
>> @@ -569,11 +573,19 @@ int of_address_to_resource(struct device_node *dev, int index,
>>   	const __be32	*addrp;
>>   	u64		size;
>>   	unsigned int	flags;
>> +	const char	*name = NULL;
>> +	int		name_cnt;
>>
>>   	addrp = of_get_address(dev, index,&size,&flags);
>>   	if (addrp == NULL)
>>   		return -EINVAL;
>> -	return __of_address_to_resource(dev, addrp, size, flags, r);
>> +
>> +	/* Get "reg-names" property to add a name to a resource */
>> +	name_cnt = of_property_count_strings(dev, "reg-names");
>> +	if (name_cnt>  0)
>> +		of_property_read_string_index(dev, "reg-names",	index,&name);
>
> Why not simply:
> 	of_property_read_string_index(dev, "reg-names", index,&name);
>
> If it fails, then name remains NULL and everything works.

Good point, that API is already taking care of that.

Thanks,
Benoit

Re: [PATCH] of: Add a reg-names property to name reg entries

[Grant Likely]

On Tue, Oct 25, 2011 at 12:56:53AM +0200, Cousson, Benoit wrote:
> On 10/25/2011 12:19 AM, Grant Likely wrote:
> >In general, I'm inclined to accept this patch as we talked about at
> >plumbers.  However, this particular hunk gives me pause as there is
> >still the objection that Russell raised about the (ab)use of r->name
> >for insert the resource name.
> >
> >So, no I won't reject this patch, but I first what to have some idea
> >of what the plan is to migrate away from using r->name.  Perhaps we
> >can talk about this tomorrow.
> 
> OK, sure, let's discuss about that.
> But I still think that Russell's concern is not related at all with
> this patch. That means that it should be fixed separately if needed.
> It can be done before or after, but this is somehow orthogonal to
> the DT reg-names support problem.

Right, but this patch does add more infrastructure code manuipulating
that field, so I'd like to have a plan for fixing the problem before
committing this.

g.

Re: [PATCH] of: Add a reg-names property to name reg entries

[Tony Lindgren]

* Benoit Cousson <b-cousson@ti.com> [111024 17:20]:
> +		compatible = "ti,mcasp";
> +		reg = <0 0x10 0x10>, <0 0x20 0x10>,
> +		      <1 0x10 0x10>, <1 0x20 0x10>;
> +		reg-names = "mpu", "dat",
> +			    "dma", "dma_dat";

Hmm for some systems looks like this can also solve how to pass the
mux signal names cleanly from DT.

For some systems we may also need reg-bits in addition to reg-names
to describe the signal names. Or do we already have something for
that?

Tony

Re: [PATCH] of: Add a reg-names property to name reg entries

[Segher Boessenkool]

>> +		compatible = "ti,mcasp";
>> +		reg = <0 0x10 0x10>, <0 0x20 0x10>,
>> +		      <1 0x10 0x10>, <1 0x20 0x10>;
>> +		reg-names = "mpu", "dat",
>> +			    "dma", "dma_dat";
>
> Hmm for some systems looks like this can also solve how to pass the
> mux signal names cleanly from DT.

What problem does any of this solve?  The device binding for the
"mcasp" device will have to describe the possible "reg-names", and
what those mean; but the binding already has to describe its "reg"
property anyway.

The example looks like something that should be more properly done
with subdevices, not sure.


Segher

Re: [PATCH] of: Add a reg-names property to name reg entries

[Cousson, Benoit]

On 10/25/2011 12:29 PM, Segher Boessenkool wrote:
>>> +		compatible = "ti,mcasp";
>>> +		reg =<0 0x10 0x10>,<0 0x20 0x10>,
>>> +		<1 0x10 0x10>,<1 0x20 0x10>;
>>> +		reg-names = "mpu", "dat",
>>> +			    "dma", "dma_dat";
>>
>> Hmm for some systems looks like this can also solve how to pass the
>> mux signal names cleanly from DT.
>
> What problem does any of this solve?  The device binding for the
> "mcasp" device will have to describe the possible "reg-names", and
> what those mean; but the binding already has to describe its "reg"
> property anyway.

What this solve is the ability to use the platform_get_resource_byname 
directly to retrieve the proper register base address. The binding is 
just a text description that the driver will not be able to use 
directly. It will have to get the resource using an abstract index.
It thus removes a level of indirection that is error prone and useless 
most of the time.

Benoit

Re: [PATCH] of: Add a reg-names property to name reg entries

[Segher Boessenkool]

>> What problem does any of this solve?  The device binding for the
>> "mcasp" device will have to describe the possible "reg-names", and
>> what those mean; but the binding already has to describe its "reg"
>> property anyway.
>
> What this solve is the ability to use the  
> platform_get_resource_byname directly to retrieve the proper  
> register base address.

You do not have to put it in the device tree for that, the device
driver can implement this itself if it cares.

> The binding is just a text description that the driver will not be  
> able to use directly. It will have to get the resource using an  
> abstract index.

Your reg-names are abstract identifiers just as well.

> It thus removes a level of indirection that is error prone and  
> useless most of the time.

It *adds* a level of indirection.  I doubt it helps prevent errors
either, but who knows.


Segher

Re: [PATCH] of: Add a reg-names property to name reg entries

[Cousson, Benoit]

On 10/25/2011 4:17 PM, Segher Boessenkool wrote:
>>> What problem does any of this solve?  The device binding for the
>>> "mcasp" device will have to describe the possible "reg-names", and
>>> what those mean; but the binding already has to describe its "reg"
>>> property anyway.
>>
>> What this solve is the ability to use the
>> platform_get_resource_byname directly to retrieve the proper
>> register base address.
>
> You do not have to put it in the device tree for that, the device
> driver can implement this itself if it cares.

???

The driver is the user of that name, so it has to be populated before 
into the resource during device creation.

>> The binding is just a text description that the driver will not be
>> able to use directly. It will have to get the resource using an
>> abstract index.
>
> Your reg-names are abstract identifiers just as well.

This is the name used in the HW documentation.
Ordering them to get a number is purely arbitrary. That's why using the 
name will allow the driver to get the resource the way it is represented 
in the documentation and thus avoid some intermediate number.

>> It thus removes a level of indirection that is error prone and
>> useless most of the time.
>
> It *adds* a level of indirection.  I doubt it helps prevent errors
> either, but who knows.

Well, if that does not bring anything to you, you can just not use it.

Benoit

Re: [PATCH] of: Add a reg-names property to name reg entries

[Segher Boessenkool]

>>>> What problem does any of this solve?  The device binding for the
>>>> "mcasp" device will have to describe the possible "reg-names", and
>>>> what those mean; but the binding already has to describe its "reg"
>>>> property anyway.
>>>
>>> What this solve is the ability to use the
>>> platform_get_resource_byname directly to retrieve the proper
>>> register base address.
>>
>> You do not have to put it in the device tree for that, the device
>> driver can implement this itself if it cares.
>
> ???
>
> The driver is the user of that name, so it has to be populated  
> before into the resource during device creation.

It can be as simple as

#define FOO_REG_INDEX 0
#define BAR_REG_INDEX 1

but you can use C strings if you want to.

>>> The binding is just a text description that the driver will not be
>>> able to use directly. It will have to get the resource using an
>>> abstract index.
>>
>> Your reg-names are abstract identifiers just as well.
>
> This is the name used in the HW documentation.

So?  The device binding will still have to list them, for exact spelling
and register block size etc.

> Ordering them to get a number is purely arbitrary.

Ordering them is required, "reg" is an array.  It also matters which
entry is the first entry (for the unit address).

> That's why using the name will allow the driver to get the resource  
> the way it is represented in the documentation and thus avoid some  
> intermediate number.

You use an intermediate string instead, and add code and binding to
translate that to that same number.

>>> It thus removes a level of indirection that is error prone and
>>> useless most of the time.
>>
>> It *adds* a level of indirection.  I doubt it helps prevent errors
>> either, but who knows.
>
> Well, if that does not bring anything to you, you can just not use it.

I could, if you did this for your device binding only, but it seems
you are adding this as a generic thing.  I am very much against that.
The device tree should describe the hardware; it shouldn't describe
the description of the hardware, that's what bindings are for.


Segher

Re: [PATCH] of: Add a reg-names property to name reg entries

[Tony Lindgren]

* Segher Boessenkool <segher@kernel.crashing.org> [111026 05:22]:
> >>>>What problem does any of this solve?  The device binding for the
> >>>>"mcasp" device will have to describe the possible "reg-names", and
> >>>>what those mean; but the binding already has to describe its "reg"
> >>>>property anyway.
> >>>
> >>>What this solve is the ability to use the
> >>>platform_get_resource_byname directly to retrieve the proper
> >>>register base address.
> >>
> >>You do not have to put it in the device tree for that, the device
> >>driver can implement this itself if it cares.
> >
> >???
> >
> >The driver is the user of that name, so it has to be populated
> >before into the resource during device creation.
> 
> It can be as simple as
> 
> #define FOO_REG_INDEX 0
> #define BAR_REG_INDEX 1
> 
> but you can use C strings if you want to.

Fox pin muxing, we really want to use the actual signal names in
the drivers. The define example above would means mapping the defines
multiple times depending which packaging you're using.

There can be several packages for each SoC, where the pins wired up
whichever way. For example, for a serial driver to use UART RX pin,
the driver can just request a signal called "uart_tx", which is the
actual hardware name for the signal inside the SoC.

Using the signal names also makes it easier to make drivers to support
the same device IP blocks across multiple SoC as the signal name most
likely is the same as what the device IP block is using.

Regards,

Tony 

Re: [PATCH] of: Add a reg-names property to name reg entries

[Cousson, Benoit]

On 10/26/2011 5:57 AM, Segher Boessenkool wrote:
>>>>> What problem does any of this solve?  The device binding for the
>>>>> "mcasp" device will have to describe the possible "reg-names", and
>>>>> what those mean; but the binding already has to describe its "reg"
>>>>> property anyway.
>>>>
>>>> What this solve is the ability to use the
>>>> platform_get_resource_byname directly to retrieve the proper
>>>> register base address.
>>>
>>> You do not have to put it in the device tree for that, the device
>>> driver can implement this itself if it cares.
>>
>> ???
>>
>> The driver is the user of that name, so it has to be populated
>> before into the resource during device creation.
>
> It can be as simple as
>
> #define FOO_REG_INDEX 0
> #define BAR_REG_INDEX 1
>
> but you can use C strings if you want to.

But that will add an extra level of indirection...

The idea is to avoid that and use directly:

platform_get_resource_byname(pdev, IORESOURCE_MEM, "smem");
platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmem");
....

>>>> The binding is just a text description that the driver will not be
>>>> able to use directly. It will have to get the resource using an
>>>> abstract index.
>>>
>>> Your reg-names are abstract identifiers just as well.
>>
>> This is the name used in the HW documentation.
>
> So?  The device binding will still have to list them, for exact spelling
> and register block size etc.

The binding is *just* a text... so instead of having to read a text file 
and then getting an index, we can just use the text that represent the 
memory entry.

>> Ordering them to get a number is purely arbitrary.
>
> Ordering them is required, "reg" is an array.  It also matters which
> entry is the first entry (for the unit address).
>
>> That's why using the name will allow the driver to get the resource
>> the way it is represented in the documentation and thus avoid some
>> intermediate number.
>
> You use an intermediate string instead, and add code and binding to
> translate that to that same number.

Nope. We use a string to get the resource directly. There is no 
intermediate index.

>>>> It thus removes a level of indirection that is error prone and
>>>> useless most of the time.
>>>
>>> It *adds* a level of indirection.  I doubt it helps prevent errors
>>> either, but who knows.
>>
>> Well, if that does not bring anything to you, you can just not use it.
>
> I could, if you did this for your device binding only, but it seems
> you are adding this as a generic thing.  I am very much against that.
> The device tree should describe the hardware; it shouldn't describe
> the description of the hardware, that's what bindings are for.

Not really, it is still a representation of the HW using a more readable 
way. I kept the original "reg" for legacy only.

This idea is to extend that to any kind of resources attached to a 
device: irq, dma_req, clock, regulator...
FWIW, using a name is already the natural way to get a clock and a 
regulator.
The point here is to extend legacy binding and thus improve the 
consistency in the device resource representation.

Benoit