| Message ID | CAOh2x==LNuSFftB1r8cyJ6TOqOVJfExMTUVxKZnS16wL4B8eBA@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Ping!! On 7 November 2012 09:52, viresh kumar <viresh.kumar@linaro.org> wrote: > On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring <robherring2@gmail.com> wrote: >>> +#define of_property_read_array(_np, _pname, _out, _sz) \ > >>> + while (_sz--) \ >>> + *_out++ = (typeof(*_out))be32_to_cpup(_val++); \ > >> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but >> _val is always incremented by 4 bytes. >> >> According to the dtc commit adding this feature, the values are packed: >> >> With this patch the following property assignment: >> >> property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>; >> >> is equivalent to: >> >> property = <0x12345678 0x0000ffff>; > > I thought of it a bit more and wasn't actually aligned with your explanation :( > > If that is the case, how will current implementation of u32 array will work > if we pass something like: 0x88 0x84000000 0x5890 from DT? > > So, i did a dummy test of my current implementation, with following changes > in one of my drivers: > > dts changes: > > cluster0: cluster@0 { > + data1 = <0x50 0x60 0x70>; > + data2 = <0x5000 0x6000 0x7000>; > + data3 = <0x50000000 0x60000000 0x70000000>; > } > > driver changes: > > +void test(struct device_node *cluster) > +{ > + u8 data1[3]; > + u16 data2[3]; > + u32 data3[3], i; > + > + of_property_read_u8_array(cluster, "data1", data1, 3); > + of_property_read_u16_array(cluster, "data2", data2, 3); > + of_property_read_u32_array(cluster, "data3", data3, 3); > + > + for (i = 0; i < 3; i++) { > + printk(KERN_INFO "u8 %d: %x\n", i, data1[i]); > + printk(KERN_INFO "u16 %d: %x\n", i, data2[i]); > + printk(KERN_INFO "u32 %d: %x\n", i, data3[i]); > + } > +} > > And following is the output > > [ 4.087205] u8 0: 50 > [ 4.093746] u16 0: 5000 > [ 4.101067] u32 0: 50000000 > [ 4.109512] u8 1: 60 > [ 4.116036] u16 1: 6000 > [ 4.123357] u32 1: 60000000 > [ 4.131718] u8 2: 70 > [ 4.138241] u16 2: 7000 > [ 4.145573] u32 2: 70000000 > > which looks to be what we were looking for, isn't it? > > Following is fixup for the doc comment missing: > > commit 00803aed0781de451048df0d15a3e8c814a343c8 > Author: Viresh Kumar <viresh.kumar@linaro.org> > Date: Wed Nov 7 09:48:46 2012 +0530 > > fixup! dt: add helper function to read u8 & u16 variables & arrays > --- > drivers/of/base.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index fbb634b..4a6632e 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 8-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, > @@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 16-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, > @@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 32-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, > > _______________________________________________ > linaro-dev mailing list > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev
On 11/06/2012 10:22 PM, viresh kumar wrote: > On Tue, Nov 6, 2012 at 7:48 PM, Rob Herring <robherring2@gmail.com> wrote: >>> +#define of_property_read_array(_np, _pname, _out, _sz) \ > >>> + while (_sz--) \ >>> + *_out++ = (typeof(*_out))be32_to_cpup(_val++); \ > >> This will not work. You are incrementing _out by 1, 2, or 4 bytes, but >> _val is always incremented by 4 bytes. >> >> According to the dtc commit adding this feature, the values are packed: >> >> With this patch the following property assignment: >> >> property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>; >> >> is equivalent to: >> >> property = <0x12345678 0x0000ffff>; > > I thought of it a bit more and wasn't actually aligned with your explanation :( > > If that is the case, how will current implementation of u32 array will work > if we pass something like: 0x88 0x84000000 0x5890 from DT? > > So, i did a dummy test of my current implementation, with following changes > in one of my drivers: > > dts changes: > > cluster0: cluster@0 { > + data1 = <0x50 0x60 0x70>; > + data2 = <0x5000 0x6000 0x7000>; > + data3 = <0x50000000 0x60000000 0x70000000>; So there is a mismatch in our assumptions. You are just truncating 32-bit values. I assumed you were using the 8 and 16 bit sizes that are now supported in dts. I don't think we should just truncate values blindly. We have support for specifying 8 and 16 values now so you should use that and define that as part of a binding. Rob > } > > driver changes: > > +void test(struct device_node *cluster) > +{ > + u8 data1[3]; > + u16 data2[3]; > + u32 data3[3], i; > + > + of_property_read_u8_array(cluster, "data1", data1, 3); > + of_property_read_u16_array(cluster, "data2", data2, 3); > + of_property_read_u32_array(cluster, "data3", data3, 3); > + > + for (i = 0; i < 3; i++) { > + printk(KERN_INFO "u8 %d: %x\n", i, data1[i]); > + printk(KERN_INFO "u16 %d: %x\n", i, data2[i]); > + printk(KERN_INFO "u32 %d: %x\n", i, data3[i]); > + } > +} > > And following is the output > > [ 4.087205] u8 0: 50 > [ 4.093746] u16 0: 5000 > [ 4.101067] u32 0: 50000000 > [ 4.109512] u8 1: 60 > [ 4.116036] u16 1: 6000 > [ 4.123357] u32 1: 60000000 > [ 4.131718] u8 2: 70 > [ 4.138241] u16 2: 7000 > [ 4.145573] u32 2: 70000000 > > which looks to be what we were looking for, isn't it? > > Following is fixup for the doc comment missing: > > commit 00803aed0781de451048df0d15a3e8c814a343c8 > Author: Viresh Kumar <viresh.kumar@linaro.org> > Date: Wed Nov 7 09:48:46 2012 +0530 > > fixup! dt: add helper function to read u8 & u16 variables & arrays > --- > drivers/of/base.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index fbb634b..4a6632e 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 8-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, > @@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 16-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, > @@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array); > * @np: device node from which the property value is to be read. > * @propname: name of the property to be searched. > * @out_value: pointer to return value, modified only if return value is 0. > + * @sz: number of array elements to read > * > * Search for a property in a device node and read 32-bit value(s) from > * it. Returns 0 on success, -EINVAL if the property does not exist, >
On 11 November 2012 19:42, Rob Herring <robherring2@gmail.com> wrote: > On 11/06/2012 10:22 PM, viresh kumar wrote: >> cluster0: cluster@0 { >> + data1 = <0x50 0x60 0x70>; >> + data2 = <0x5000 0x6000 0x7000>; >> + data3 = <0x50000000 0x60000000 0x70000000>; > > So there is a mismatch in our assumptions. You are just truncating > 32-bit values. I assumed you were using the 8 and 16 bit sizes that are > now supported in dts. I don't think we should just truncate values > blindly. We have support for specifying 8 and 16 values now so you > should use that and define that as part of a binding. Sorry couldn't get your point at all :( What did you mean by "truncating 32 bit values" and how should we tell via DT, that the value passed is 8 bit, 16 bit or 32 bit? -- viresh
On 11/11/2012 11:27 AM, Viresh Kumar wrote: > On 11 November 2012 19:42, Rob Herring <robherring2@gmail.com> wrote: >> On 11/06/2012 10:22 PM, viresh kumar wrote: > >>> cluster0: cluster@0 { >>> + data1 = <0x50 0x60 0x70>; >>> + data2 = <0x5000 0x6000 0x7000>; >>> + data3 = <0x50000000 0x60000000 0x70000000>; >> >> So there is a mismatch in our assumptions. You are just truncating >> 32-bit values. I assumed you were using the 8 and 16 bit sizes that are >> now supported in dts. I don't think we should just truncate values >> blindly. We have support for specifying 8 and 16 values now so you >> should use that and define that as part of a binding. > > Sorry couldn't get your point at all :( > What did you mean by "truncating 32 bit values" and how should we > tell via DT, that the value passed is 8 bit, 16 bit or 32 bit? > You are trying to retrieve an array of 8 or 16-bit values which are stored as 32-bit values in dtb. Why not define them in the binding as 8 or 16 bit to begin with. Then there is never any ambiguity about their size. I don't think the size is stored in the dtb. It is only in the dts. You need to define the size in the binding definitions and use '/bits/' annotation. With this the data is packed. Then the array function used should match what the binding defines. Rob
On 12 November 2012 01:12, Rob Herring <robherring2@gmail.com> wrote: > On 11/11/2012 11:27 AM, Viresh Kumar wrote: >> On 11 November 2012 19:42, Rob Herring <robherring2@gmail.com> wrote: >>> On 11/06/2012 10:22 PM, viresh kumar wrote: >> >>>> cluster0: cluster@0 { >>>> + data1 = <0x50 0x60 0x70>; >>>> + data2 = <0x5000 0x6000 0x7000>; >>>> + data3 = <0x50000000 0x60000000 0x70000000>; >>> >>> So there is a mismatch in our assumptions. You are just truncating >>> 32-bit values. I assumed you were using the 8 and 16 bit sizes that are >>> now supported in dts. I don't think we should just truncate values >>> blindly. We have support for specifying 8 and 16 values now so you >>> should use that and define that as part of a binding. >> >> Sorry couldn't get your point at all :( >> What did you mean by "truncating 32 bit values" and how should we >> tell via DT, that the value passed is 8 bit, 16 bit or 32 bit? >> > > You are trying to retrieve an array of 8 or 16-bit values which are > stored as 32-bit values in dtb. Why not define them in the binding as 8 > or 16 bit to begin with. Then there is never any ambiguity about their size. > > I don't think the size is stored in the dtb. It is only in the dts. You > need to define the size in the binding definitions and use '/bits/' > annotation. With this the data is packed. Then the array function used > should match what the binding defines. Aha, and in that case incrementing address by 4 in my patch will fail. Right? Will fix it. Thanks for increasing my knowledge on this :) -- viresh
On 12 November 2012 09:03, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 November 2012 01:12, Rob Herring <robherring2@gmail.com> wrote: >> I don't think the size is stored in the dtb. It is only in the dts. You >> need to define the size in the binding definitions and use '/bits/' >> annotation. With this the data is packed. Then the array function used >> should match what the binding defines. Hi Rob, Can you please help me in getting the correct format to do this from dts. I tried: data1 = /bits/ 8 <0x50 0x60 0x70>; as written in your earlier mail... but it didn't worked. Tried to search too, but couldn't find it.
On 19 November 2012 09:24, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 November 2012 09:03, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 12 November 2012 01:12, Rob Herring <robherring2@gmail.com> wrote: >>> I don't think the size is stored in the dtb. It is only in the dts. You >>> need to define the size in the binding definitions and use '/bits/' >>> annotation. With this the data is packed. Then the array function used >>> should match what the binding defines. > > Hi Rob, > > Can you please help me in getting the correct format to do this from dts. > I tried: > > data1 = /bits/ 8 <0x50 0x60 0x70>; as per spec, format for data byte defines will be: data1 = [ 0x50 0x60 0x70 ]; however, i see a parse error from device tree compiler when i tried. > > as written in your earlier mail... but it didn't worked. Tried to > search too, but > couldn't find it. > > _______________________________________________ > linaro-dev mailing list > linaro-dev@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev
On 19 November 2012 11:54, Rajanikanth HV <rajanikanth.hv@linaro.org> wrote: >> data1 = /bits/ 8 <0x50 0x60 0x70>; > as per spec, format for data byte defines will be: > data1 = [ 0x50 0x60 0x70 ]; > however, i see a parse error from device tree compiler when i tried. Firstly you tried square braces [ ], I am not sure if that is allowed. Can you point me to the specification? And simply passing 0x50, 0x60 etc.. will make them u32 instead of u8. i.e. they will be stored as 0x00000050, etc. -- viresh
On 19 November 2012 12:00, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Firstly you tried square braces [ ], I am not sure if that is allowed. > Can you point me to the specification? http://www.devicetree.org/Device_Tree_Usage " a-byte-data-property = [0x01 0x23 0x34 0x56]; " > > And simply passing 0x50, 0x60 etc.. will make them u32 instead of > u8. i.e. they will be stored as 0x00000050, etc. > > -- > viresh
On 19 November 2012 12:05, Rajanikanth HV <rajanikanth.hv@linaro.org> wrote: > On 19 November 2012 12:00, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> Firstly you tried square braces [ ], I am not sure if that is allowed. >> Can you point me to the specification? > http://www.devicetree.org/Device_Tree_Usage > " > a-byte-data-property = [0x01 0x23 0x34 0x56]; > " Ok, but what about 16 bit then {} :)
On 11/18/2012 11:41 PM, Viresh Kumar wrote: > On 19 November 2012 12:05, Rajanikanth HV <rajanikanth.hv@linaro.org> wrote: >> On 19 November 2012 12:00, Viresh Kumar <viresh.kumar@linaro.org> wrote: >>> Firstly you tried square braces [ ], I am not sure if that is allowed. >>> Can you point me to the specification? >> http://www.devicetree.org/Device_Tree_Usage >> " >> a-byte-data-property = [0x01 0x23 0x34 0x56]; >> " > > Ok, but what about 16 bit then {} :) Support for byte- and word- properties is relatively recent I believe (or at least, the /bits/ syntax is). Which dtc version are you using?
On 19 November 2012 21:58, Stephen Warren <swarren@wwwdotorg.org> wrote: > Support for byte- and word- properties is relatively recent I believe > (or at least, the /bits/ syntax is). Which dtc version are you using? Ok, i was on a older version. I just saw this patch now: commit cd296721a9645f9f28800a072490fa15458d1fb7 Author: Stephen Warren <swarren@nvidia.com> Date: Fri Sep 28 21:25:59 2012 +0000 dtc: import latest upstream dtc This updates scripts/dtc to commit 317a5d9 "dtc: zero out new label objects" from git://git.jdl.com/software/dtc.git. This adds features such as: * /bits/ syntax for cell data. * Math expressions within cell data. * The ability to delete properties or nodes. * Support for #line directives in the input file, which allows the use of cpp on *.dts. * -i command-line option (/include/ path) * -W/-E command-line options for error/warning control. * Removal of spew to STDOUT containing the filename being compiled. * Many additions to the libfdt API. Signed-off-by: Stephen Warren <swarren@nvidia.com> Acked-by: Jon Loeliger <jdl@jdl.com> Signed-off-by: Rob Herring <rob.herring@calxeda.com> Will try it tomorrow -- viresh
diff --git a/drivers/of/base.c b/drivers/of/base.c index fbb634b..4a6632e 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -669,6 +669,7 @@ EXPORT_SYMBOL(of_find_node_by_phandle); * @np: device node from which the property value is to be read. * @propname: name of the property to be searched. * @out_value: pointer to return value, modified only if return value is 0. + * @sz: number of array elements to read * * Search for a property in a device node and read 8-bit value(s) from * it. Returns 0 on success, -EINVAL if the property does not exist, @@ -690,6 +691,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array); * @np: device node from which the property value is to be read. * @propname: name of the property to be searched. * @out_value: pointer to return value, modified only if return value is 0. + * @sz: number of array elements to read * * Search for a property in a device node and read 16-bit value(s) from * it. Returns 0 on success, -EINVAL if the property does not exist, @@ -712,6 +714,7 @@ EXPORT_SYMBOL_GPL(of_property_read_u16_array); * @np: device node from which the property value is to be read. * @propname: name of the property to be searched. * @out_value: pointer to return value, modified only if return value is 0. + * @sz: number of array elements to read * * Search for a property in a device node and read 32-bit value(s) from * it. Returns 0 on success, -EINVAL if the property does not exist,