of: Add a reg-names property to name reg entries

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 <[email protected]>

Cc Grant Likely <[email protected]>

Cc Rob Herring <[email protected]>

---

 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

Documentation/devicetree/bindings/reg-names.txt new file +48 -0

new file mode 100644
index 0000000..5554065
@@ -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";
+};
+

drivers/of/address.c +3 -3

index 72c33fb..1f9f8cb 100644
@@ -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;
 

drivers/of/address.c +2 -0

 	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;

drivers/of/address.c +2 -0

+
+	return __of_address_to_resource(dev, addrp, size, flags, name, r);
 }
 EXPORT_SYMBOL_GPL(of_address_to_resource);
 

Signature · 7 lines

1.7.5.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Oct 24, 2011 at 05:54:57PM +0200, Benoit Cousson wrote:

drivers/of/address.c static int __of_address_to_resource(struct device_node *dev, · line 557

Quoted 9 lines

drivers/of/address.c +4 -1

@@ -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.

Quoted 9 lines

drivers/of/address.c +2 -0

  	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;

drivers/of/address.c int of_address_to_resource(struct device_node *dev, int index, · line 586

Quoted 10 lines

drivers/of/address.c +5 -1

 
  	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.

Quoted 8 lines · Grant Likely

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.

Documentation/devicetree/bindings/reg-names.txt line 31

Quoted 5 lines

Documentation/devicetree/bindings/reg-names.txt new file +5 -0

+		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.

Quoted 8 lines

Documentation/devicetree/bindings/reg-names.txt line 31

Quoted 5 lines

Documentation/devicetree/bindings/reg-names.txt new file +5 -0

+		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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line

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).

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 1 line

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.