| Message ID | 1521933899-362-2-git-send-email-albeu@free.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: Add support for reading MTD devices via the nvmem API | expand |
On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote: > Having the cells as subnodes of the provider device without any > compatible property might clash with other bindings. To avoid this > problem update the binding to have all the cells in a 'nvmem-cells' > subnode with a 'nvmem-cells' compatible string. This new binding > guarantee that we can turn any kind of device in a nvmem provider. > > While discouraged for new uses the old scheme is still supported for > backward compatibility. > > Signed-off-by: Alban Bedel <albeu@free.fr> > --- > Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++------- > drivers/nvmem/core.c | 10 +++++ > 2 files changed, 48 insertions(+), 17 deletions(-) > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt > index fd06c09..6b723e7 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device. > This document is here to document this. > > = Data providers = > -Contains bindings specific to provider drivers and data cells as children > -of this node. > +A data provider should have a subnode named 'nvmem-cells' that contains > +a subnodes for each data cells. > + > +For backward compatibility the nvmem data cells can be direct children > +of the data provider. This use is discouraged as it can conflict with > +other bindings. I don't think we need to go this far. Whether this is necessary depends on the provider. > > Optional properties: > read-only: Mark the provider as read only. > > += Data cells list = > +The data cells list node should be named 'nvmem-cells' and have a > +child node for each data cell. > + > +Required properties: > + compatible: Must be "nvmem-cells" > + #address-cells: <1> if the provider use 32 bit addressing, > + <2> for 64 bits addressing > + #size-cells: <1> if the provider use 32 bit sizes, > + <2> for 64 bits sizes > + > = Data cells = > -These are the child nodes of the provider which contain data cell > +These are the child nodes of the nvmem-cells node which contain data cell > information like offset and size in nvmem provider.
On Mon, 16 Apr 2018 16:04:29 -0500 Rob Herring <robh@kernel.org> wrote: > On Sun, Mar 25, 2018 at 12:24:57AM +0100, Alban Bedel wrote: > > Having the cells as subnodes of the provider device without any > > compatible property might clash with other bindings. To avoid this > > problem update the binding to have all the cells in a 'nvmem-cells' > > subnode with a 'nvmem-cells' compatible string. This new binding > > guarantee that we can turn any kind of device in a nvmem provider. > > > > While discouraged for new uses the old scheme is still supported for > > backward compatibility. > > > > Signed-off-by: Alban Bedel <albeu@free.fr> > > --- > > Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++------- > > drivers/nvmem/core.c | 10 +++++ > > 2 files changed, 48 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > index fd06c09..6b723e7 100644 > > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device. > > This document is here to document this. > > > > = Data providers = > > -Contains bindings specific to provider drivers and data cells as children > > -of this node. > > +A data provider should have a subnode named 'nvmem-cells' that contains > > +a subnodes for each data cells. > > + > > +For backward compatibility the nvmem data cells can be direct children > > +of the data provider. This use is discouraged as it can conflict with > > +other bindings. > > I don't think we need to go this far. Whether this is necessary depends > on the provider. It depend more on the drivers implementation. Sure as long as the driver only support the nvmem API it doesn't matter, both binding are fine. But if it ever need to support another API the bindings might clash and the whole device binding will need to be updated. So all in all I see very few value in still allowing the old binding for new devices, or do you seen any problem with the new binding? However if the consensus is to keep both styles I will rewrite this paragraph as needed. Alban
On 24/03/18 23:24, Alban Bedel wrote: > Having the cells as subnodes of the provider device without any > compatible property might clash with other bindings. To avoid this > problem update the binding to have all the cells in a 'nvmem-cells' > subnode with a 'nvmem-cells' compatible string. This new binding > guarantee that we can turn any kind of device in a nvmem provider. > > While discouraged for new uses the old scheme is still supported for > backward compatibility. Am not sure if this a really good idea to change nvmem bindings based on provider requirements. This can be a beginning of other problems!! Did you know that we can pass nvmem cells info via nvmem config ? Why can't mtd-nvmem provider populate the nvmem_config->cells from its dt "nvmem-cells" subnode before it registers the provider? Doing this way will make the binding very much specific to the provider rather than changing nvmem core bindings. thanks, srini
On Tue, 17 Apr 2018 13:54:07 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 24/03/18 23:24, Alban Bedel wrote: > > Having the cells as subnodes of the provider device without any > > compatible property might clash with other bindings. To avoid this > > problem update the binding to have all the cells in a 'nvmem-cells' > > subnode with a 'nvmem-cells' compatible string. This new binding > > guarantee that we can turn any kind of device in a nvmem provider. > > > > While discouraged for new uses the old scheme is still supported for > > backward compatibility. > > Am not sure if this a really good idea to change nvmem bindings based on > provider requirements. This can be a beginning of other problems!! I think you misunderstood something here, this proposed new binding would be for all new nvmem bindings, not just mtd backed nvmem. > Did you know that we can pass nvmem cells info via nvmem config ? > > Why can't mtd-nvmem provider populate the nvmem_config->cells from > its dt "nvmem-cells" subnode before it registers the provider? The DT based lookup of nvmem-cells doesn't use nvmem_config->cells, so that's not an option. In fact here the problem come from the MTD side because it also had a similar binding using subnodes without compatible string. Just to make things clear, here is an example of the clash using the current nvmem binding on an unpartitioned MTD device: flash@0 { #address-cells = <1>; #size-cells = <1>; compatible = "s25sl064a"; reg = <0>; calibration: calib@404 { reg = <0x404 0x10>; }; }; This will not only allow reading the calibration data from nvmem, but will also create a partition on the MTD device, which is not acceptable. With my proposed binding this would become: flash@0 { #address-cells = <1>; #size-cells = <1>; compatible = "s25sl064a"; reg = <0>; nvmem-cells { compatible = "nvmem-cells"; #address-cells = <1>; #address-cells = <1>; calibration: calib@404 { reg = <0x404 0x10>; }; }; }; Which would work fine as the MTD code will ignore the nvmem-cells subnode thanks to its compatible string. IMHO subnodes without any compatible properties should never be used in such generic bindings, as it is very likely that it will at some point clash with another generic binding or with a device specific binding. Alban
Thanks for explaining, On 17/04/18 15:54, Alban wrote: > This will not only allow reading the calibration data from nvmem, but > will also create a partition on the MTD device, which is not acceptable. > With my proposed binding this would become: > > flash@0 { > #address-cells = <1>; > #size-cells = <1>; > compatible = "s25sl064a"; > reg = <0>; > > nvmem-cells { > compatible = "nvmem-cells"; > #address-cells = <1>; > #address-cells = <1>; > > calibration: calib@404 { > reg = <0x404 0x10>; > }; > }; Why can't we make nvmem-cells node a nvmem provider in this case? Which should work! --srini > }; > > Which would work fine as the MTD code will ignore the nvmem-cells > subnode thanks to its compatible string.
On Tue, 17 Apr 2018 16:44:01 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > Thanks for explaining, > > On 17/04/18 15:54, Alban wrote: > > This will not only allow reading the calibration data from nvmem, but > > will also create a partition on the MTD device, which is not acceptable. > > With my proposed binding this would become: > > > > flash@0 { > > #address-cells = <1>; > > #size-cells = <1>; > > compatible = "s25sl064a"; > > reg = <0>; > > > > nvmem-cells { > > compatible = "nvmem-cells"; > > #address-cells = <1>; > > #address-cells = <1>; > > > > calibration: calib@404 { > > reg = <0x404 0x10>; > > }; > > }; > > Why can't we make nvmem-cells node a nvmem provider in this case? > Which should work! TBH I just copied what have been done to fix the same problem with the MTD partitions. But yes we could also just extend the current binding to require a compatible string on each nvmem-cell, which would not require any code change to support. Alban
On Tue, 17 Apr 2018 18:00:40 +0200 Alban <albeu@free.fr> wrote: > On Tue, 17 Apr 2018 16:44:01 +0100 > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > Thanks for explaining, > > > > On 17/04/18 15:54, Alban wrote: > > > This will not only allow reading the calibration data from nvmem, but > > > will also create a partition on the MTD device, which is not acceptable. > > > With my proposed binding this would become: > > > > > > flash@0 { > > > #address-cells = <1>; > > > #size-cells = <1>; > > > compatible = "s25sl064a"; > > > reg = <0>; > > > > > > nvmem-cells { > > > compatible = "nvmem-cells"; > > > #address-cells = <1>; > > > #address-cells = <1>; > > > > > > calibration: calib@404 { > > > reg = <0x404 0x10>; > > > }; > > > }; > > > > Why can't we make nvmem-cells node a nvmem provider in this case? > > Which should work! > > TBH I just copied what have been done to fix the same problem with the > MTD partitions. But yes we could also just extend the current binding > to require a compatible string on each nvmem-cell, which would not > require any code change to support. However this scheme will not work if the device node binding already have subnodes with addresses. The addressing, as specified by #address-cells and #size-cells, might be incompatible or might overlap. Using the nvmem-cells subnode solve this problem. Alban
On 18/04/18 12:41, Alban wrote: > On Tue, 17 Apr 2018 18:00:40 +0200 > Alban <albeu@free.fr> wrote: > >> On Tue, 17 Apr 2018 16:44:01 +0100 >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: >> >>> Thanks for explaining, >>> >>> On 17/04/18 15:54, Alban wrote: >>>> This will not only allow reading the calibration data from nvmem, but >>>> will also create a partition on the MTD device, which is not acceptable. >>>> With my proposed binding this would become: >>>> >>>> flash@0 { >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> compatible = "s25sl064a"; >>>> reg = <0>; >>>> >>>> nvmem-cells { >>>> compatible = "nvmem-cells"; >>>> #address-cells = <1>; >>>> #address-cells = <1>; >>>> >>>> calibration: calib@404 { >>>> reg = <0x404 0x10>; >>>> }; >>>> }; >>> >>> Why can't we make nvmem-cells node a nvmem provider in this case? >>> Which should work! >> >> TBH I just copied what have been done to fix the same problem with the >> MTD partitions. But yes we could also just extend the current binding >> to require a compatible string on each nvmem-cell, which would not >> require any code change to support. > > However this scheme will not work if the device node binding already > have subnodes with addresses. The addressing, as specified by > #address-cells and #size-cells, might be incompatible or might overlap. > Using the nvmem-cells subnode solve this problem. > I was also suggesting you to use nvmem-cell subnode, but make it a proper nvmem provider device, rather than reusing its parent device. You would end up some thing like this in dt. flash@0 { #address-cells = <1>; #size-cells = <1>; compatible = "s25sl064a"; reg = <0>; nvmem-cells { compatible = "mtd-nvmem"; #address-cells = <1>; #size-cells = <1>; calibration: calib@404 { reg = <0x404 0x10>; }; }; }; --srini > Alban >
On Wed, 18 Apr 2018 13:12:48 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 18/04/18 12:41, Alban wrote: > > On Tue, 17 Apr 2018 18:00:40 +0200 > > Alban <albeu@free.fr> wrote: > > > >> On Tue, 17 Apr 2018 16:44:01 +0100 > >> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > >> > >>> Thanks for explaining, > >>> > >>> On 17/04/18 15:54, Alban wrote: > >>>> This will not only allow reading the calibration data from nvmem, but > >>>> will also create a partition on the MTD device, which is not acceptable. > >>>> With my proposed binding this would become: > >>>> > >>>> flash@0 { > >>>> #address-cells = <1>; > >>>> #size-cells = <1>; > >>>> compatible = "s25sl064a"; > >>>> reg = <0>; > >>>> > >>>> nvmem-cells { > >>>> compatible = "nvmem-cells"; > >>>> #address-cells = <1>; > >>>> #address-cells = <1>; > >>>> > >>>> calibration: calib@404 { > >>>> reg = <0x404 0x10>; > >>>> }; > >>>> }; > >>> > >>> Why can't we make nvmem-cells node a nvmem provider in this case? > >>> Which should work! > >> > >> TBH I just copied what have been done to fix the same problem with the > >> MTD partitions. But yes we could also just extend the current binding > >> to require a compatible string on each nvmem-cell, which would not > >> require any code change to support. > > > > However this scheme will not work if the device node binding already > > have subnodes with addresses. The addressing, as specified by > > #address-cells and #size-cells, might be incompatible or might overlap. > > Using the nvmem-cells subnode solve this problem. > > > > I was also suggesting you to use nvmem-cell subnode, but make it a > proper nvmem provider device, rather than reusing its parent device. > > You would end up some thing like this in dt. > > flash@0 { > #address-cells = <1>; > #size-cells = <1>; > compatible = "s25sl064a"; > reg = <0>; > > nvmem-cells { > compatible = "mtd-nvmem"; > #address-cells = <1>; > #size-cells = <1>; > > calibration: calib@404 { > reg = <0x404 0x10>; > }; > }; > }; But the root cause is in the nvmem binding, this conflict could exists with any device type, not just MTD. I don't understand why we would want such workarounds instead of just fixing the problem once and for all. Alban
On 18/04/18 13:32, Alban wrote: >> I was also suggesting you to use nvmem-cell subnode, but make it a >> proper nvmem provider device, rather than reusing its parent device. >> >> You would end up some thing like this in dt. >> >> flash@0 { >> #address-cells = <1>; >> #size-cells = <1>; >> compatible = "s25sl064a"; >> reg = <0>; >> >> nvmem-cells { >> compatible = "mtd-nvmem"; >> #address-cells = <1>; >> #size-cells = <1>; >> >> calibration: calib@404 { >> reg = <0x404 0x10>; >> }; >> }; >> }; > But the root cause is in the nvmem binding, this conflict could exists No, the root cause is because of passing wrong device instance to nvmem core. And trying to workaround is the actual issue. > with any device type, not just MTD. I don't understand why we would want > such workarounds instead of just fixing the problem once and for all. AFAIU, This is not a workaround, this is how nvmem provider bindings are and all providers should try to follow it. I still do not understand what is the issue in making nvmem-cells node a proper nvmem provider device? --srini
On Wed, 18 Apr 2018 13:53:56 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 18/04/18 13:32, Alban wrote: > >> I was also suggesting you to use nvmem-cell subnode, but make it a > >> proper nvmem provider device, rather than reusing its parent device. > >> > >> You would end up some thing like this in dt. > >> > >> flash@0 { > >> #address-cells = <1>; > >> #size-cells = <1>; > >> compatible = "s25sl064a"; > >> reg = <0>; > >> > >> nvmem-cells { > >> compatible = "mtd-nvmem"; > >> #address-cells = <1>; > >> #size-cells = <1>; > >> > >> calibration: calib@404 { > >> reg = <0x404 0x10>; > >> }; > >> }; > >> }; > > But the root cause is in the nvmem binding, this conflict could exists > No, the root cause is because of passing wrong device instance to nvmem > core. And trying to workaround is the actual issue. The data is stored on the MTD, so the nvmem provider is the MTD device. I don't think it is a good idea to have a virtual device in the DT to accommodate the nvmem API. > > with any device type, not just MTD. I don't understand why we would want > > such workarounds instead of just fixing the problem once and for all. > AFAIU, This is not a workaround, this is how nvmem provider bindings are > and all providers should try to follow it. > > I still do not understand what is the issue in making nvmem-cells node a > proper nvmem provider device? It is doable, but beside making the code more complex, AFAIU that would goes against DT best practice as no such device exists in the hardware. The DT should only represent that this device contains data that can be requested by a driver, and ideally in the same way for all kind of storages. Alban
On 18/04/18 14:34, Alban wrote: > On Wed, 18 Apr 2018 13:53:56 +0100 > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > >> On 18/04/18 13:32, Alban wrote: >>>> I was also suggesting you to use nvmem-cell subnode, but make it a >>>> proper nvmem provider device, rather than reusing its parent device. >>>> >>>> You would end up some thing like this in dt. >>>> >>>> flash@0 { >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> compatible = "s25sl064a"; >>>> reg = <0>; >>>> >>>> nvmem-cells { >>>> compatible = "mtd-nvmem"; >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> >>>> calibration: calib@404 { >>>> reg = <0x404 0x10>; >>>> }; >>>> }; >>>> }; >>> But the root cause is in the nvmem binding, this conflict could exists >> No, the root cause is because of passing wrong device instance to nvmem >> core. And trying to workaround is the actual issue. > > The data is stored on the MTD, so the nvmem provider is the MTD device. > I don't think it is a good idea to have a virtual device in the DT to > accommodate the nvmem API. > Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem provider too. However, I would like to see this moving forward. I can think of one possible solution here, which is, adding "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell. The problem you mentioned regarding #address-cells and #size-cells with provider need to be addressed in nvmem core. Currently nvmem core only support offsets of 32 bits, if you are expecting a 64 bit offsets then we should add that as a feature to nvmem core. nvmem core as it is today should work fine with 32 bit offsets for mtd cases. what do you think? thanks, srini
On Tue, 1 May 2018 17:49:03 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 18/04/18 14:34, Alban wrote: > > On Wed, 18 Apr 2018 13:53:56 +0100 > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > >> On 18/04/18 13:32, Alban wrote: > >>>> I was also suggesting you to use nvmem-cell subnode, but make it a > >>>> proper nvmem provider device, rather than reusing its parent device. > >>>> > >>>> You would end up some thing like this in dt. > >>>> > >>>> flash@0 { > >>>> #address-cells = <1>; > >>>> #size-cells = <1>; > >>>> compatible = "s25sl064a"; > >>>> reg = <0>; > >>>> > >>>> nvmem-cells { > >>>> compatible = "mtd-nvmem"; > >>>> #address-cells = <1>; > >>>> #size-cells = <1>; > >>>> > >>>> calibration: calib@404 { > >>>> reg = <0x404 0x10>; > >>>> }; > >>>> }; > >>>> }; > >>> But the root cause is in the nvmem binding, this conflict could exists > >> No, the root cause is because of passing wrong device instance to nvmem > >> core. And trying to workaround is the actual issue. > > > > The data is stored on the MTD, so the nvmem provider is the MTD device. > > I don't think it is a good idea to have a virtual device in the DT to > > accommodate the nvmem API. > > > Yep, I agree! this is same issue if we make nvmem-cells a child of nvmem > provider too. > > However, I would like to see this moving forward. > > I can think of one possible solution here, which is, adding > "nvmem-mtd-cell" or "nvmem-cell" compatible string to each cell. I would definitely use "nvmem-cell", from the binding point of view it doesn't matter what the underlaying storage is. > The problem you mentioned regarding #address-cells and #size-cells with > provider need to be addressed in nvmem core. > > Currently nvmem core only support offsets of 32 bits, if you are > expecting a 64 bit offsets then we should add that as a feature to nvmem > core. > > nvmem core as it is today should work fine with 32 bit offsets for mtd > cases. That's not what I meant, 32 bit should be more that enough for now. What I meant is that if a binding already has children nodes using unit-address, then we would end up with two different uses of the same "address space". > what do you think? AFAIU the only thing that we disagree on now is if the nodes representing the cells should be direct children of the provider or in a dedicated subnode. For the MTD case both solution would solve the binding clash. I would really appreciate if the DT people could chip in so that we can settle this and get the MTD support merged. Alban
On 07/06/18 17:41, Alban wrote: > AFAIU the only thing that we disagree on now is if the nodes > representing the cells should be direct children of the provider > or in a dedicated subnode. For the MTD case both solution would solve > the binding clash. I would really appreciate if the DT people could Am reluctant in changing the nvmem generic bindings for a special case. Can you try this with your original subnode proposal: just pass the subnode node pointer in np of nvmem_config: ------------------------->cut<------------------------------------ diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index b05aa8e81303..c9621632bbfb 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) nvmem->priv = config->priv; nvmem->reg_read = config->reg_read; nvmem->reg_write = config->reg_write; - nvmem->dev.of_node = config->dev->of_node; + + if (config->np) + nvmem->dev.of_node = config->np; + else + nvmem->dev.of_node = config->dev->of_node; if (config->id == -1 && config->name) { dev_set_name(&nvmem->dev, "%s", config->name); diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8 100644 --- a/include/linux/nvmem-provider.h +++ b/include/linux/nvmem-provider.h @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, */ struct nvmem_config { struct device *dev; + struct device_node *np; const char *name; int id; struct module *owner; ------------------------->cut<------------------------------------ thanks, srini > chip in so that we can settle this and get the MTD support merged.
On Thu, 7 Jun 2018 18:03:16 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 07/06/18 17:41, Alban wrote: > > AFAIU the only thing that we disagree on now is if the nodes > > representing the cells should be direct children of the provider > > or in a dedicated subnode. For the MTD case both solution would solve > > the binding clash. I would really appreciate if the DT people could > Am reluctant in changing the nvmem generic bindings for a special case. Where I think the generic binding is fundamentally flawed, as this problem will most probably appear again. But do note that my proposal doesn't require updating the dts using the original binding, both are still supported. I proposed deprecating the current binding because I think it is flawed, but we could also "officially" support both style. > Can you try this with your original subnode proposal: > just pass the subnode node pointer in np of nvmem_config: > > ------------------------->cut<------------------------------------ > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index b05aa8e81303..c9621632bbfb 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct > nvmem_config *config) > nvmem->priv = config->priv; > nvmem->reg_read = config->reg_read; > nvmem->reg_write = config->reg_write; > - nvmem->dev.of_node = config->dev->of_node; > + > + if (config->np) > + nvmem->dev.of_node = config->np; > + else > + nvmem->dev.of_node = config->dev->of_node; > > if (config->id == -1 && config->name) { > dev_set_name(&nvmem->dev, "%s", config->name); > diff --git a/include/linux/nvmem-provider.h > b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8 > 100644 --- a/include/linux/nvmem-provider.h > +++ b/include/linux/nvmem-provider.h > @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, > unsigned int offset,nvmem_device_get( > */ > struct nvmem_config { > struct device *dev; > + struct device_node *np; > const char *name; > int id; > struct module *owner; > > ------------------------->cut<------------------------------------ That should work just fine to allow next to any kind of binding. I'll do a new patch using this approach for the code side and leaving the generic binding as it is. Alban
On 08/06/18 11:59, Alban wrote: > On Thu, 7 Jun 2018 18:03:16 +0100 > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > >> On 07/06/18 17:41, Alban wrote: >>> AFAIU the only thing that we disagree on now is if the nodes >>> representing the cells should be direct children of the provider >>> or in a dedicated subnode. For the MTD case both solution would solve >>> the binding clash. I would really appreciate if the DT people could >> Am reluctant in changing the nvmem generic bindings for a special case. > > Where I think the generic binding is fundamentally flawed, as this > problem will most probably appear again. But do note that my proposal It depends from which side you are looking at this!! There are more than 16 nvmem providers in kernel which use this bindings and they are fine!! I obviously take some of your concerns w.r.t bindings and there is a scope of making the bindings much robust, most importantly the #address and #size-cells. But having a subnode still sounds very fragile to me, and this could be much specific case of MTD provider. We might have instances where this could be sub-sub node of the the original provider for other providers. Also I do not want to bring in Provider specifics into this bindings. If single level of subnode works for your case and the below patch works for you then I would recommend lets move on and try to get this going. > doesn't require updating the dts using the original binding, both are > still supported. I proposed deprecating the current binding because I > think it is flawed, but we could also "officially" support both style. > >> Can you try this with your original subnode proposal: >> just pass the subnode node pointer in np of nvmem_config: >> >> ------------------------->cut<------------------------------------ >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >> index b05aa8e81303..c9621632bbfb 100644 >> --- a/drivers/nvmem/core.c >> +++ b/drivers/nvmem/core.c >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct >> nvmem_config *config) >> nvmem->priv = config->priv; >> nvmem->reg_read = config->reg_read; >> nvmem->reg_write = config->reg_write; >> - nvmem->dev.of_node = config->dev->of_node; >> + >> + if (config->np) >> + nvmem->dev.of_node = config->np; >> + else >> + nvmem->dev.of_node = config->dev->of_node; >> >> if (config->id == -1 && config->name) { >> dev_set_name(&nvmem->dev, "%s", config->name); >> diff --git a/include/linux/nvmem-provider.h >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8 >> 100644 --- a/include/linux/nvmem-provider.h >> +++ b/include/linux/nvmem-provider.h >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, >> unsigned int offset,nvmem_device_get( >> */ >> struct nvmem_config { >> struct device *dev; >> + struct device_node *np; >> const char *name; >> int id; >> struct module *owner; >> >> ------------------------->cut<------------------------------------ > > That should work just fine to allow next to any kind of binding. > I'll do a new patch using this approach for the code side and leaving > the generic binding as it is. Sure!! This will give more flexibility to other provider drivers! thanks, srini > > Alban >
On Fri, 8 Jun 2018 12:34:12 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > >> Can you try this with your original subnode proposal: > >> just pass the subnode node pointer in np of nvmem_config: > >> > >> ------------------------->cut<------------------------------------ > >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > >> index b05aa8e81303..c9621632bbfb 100644 > >> --- a/drivers/nvmem/core.c > >> +++ b/drivers/nvmem/core.c > >> @@ -472,7 +472,11 @@ struct nvmem_device *nvmem_register(const struct > >> nvmem_config *config) > >> nvmem->priv = config->priv; > >> nvmem->reg_read = config->reg_read; > >> nvmem->reg_write = config->reg_write; > >> - nvmem->dev.of_node = config->dev->of_node; > >> + > >> + if (config->np) > >> + nvmem->dev.of_node = config->np; > >> + else > >> + nvmem->dev.of_node = config->dev->of_node; > >> > >> if (config->id == -1 && config->name) { > >> dev_set_name(&nvmem->dev, "%s", config->name); > >> diff --git a/include/linux/nvmem-provider.h > >> b/include/linux/nvmem-provider.h index f89598bc4e1c..743345ffe2c8 > >> 100644 --- a/include/linux/nvmem-provider.h > >> +++ b/include/linux/nvmem-provider.h > >> @@ -49,6 +49,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, > >> unsigned int offset,nvmem_device_get( > >> */ > >> struct nvmem_config { > >> struct device *dev; > >> + struct device_node *np; > >> const char *name; > >> int id; > >> struct module *owner; > >> > >> ------------------------->cut<------------------------------------ > > > > That should work just fine to allow next to any kind of binding. > > I'll do a new patch using this approach for the code side and leaving > > the generic binding as it is. > Sure!! > This will give more flexibility to other provider drivers! I looked into this. It would work fine for the cells but not so nicely for the nvmem device API. The phandle for the nvmem device would have to reference the node passed here and not the real device. We would end up with a DT like this: flash@0 { compatible = "mtd"; ... nvmem_dev: nvmem-cells { compatible = "nvmem-cells"; ... }; }; other-device@10 { ... nvmem = <&nvmem_dev>; }; Now if there is no cell defined we have this empty child node that make very little sense, it is just there to accommodate the nvmem API. What I would suggest now is to just change the wording. We don't deprecate the current binding, but we extend it to allow grouping the cells in a child node if required. The code to support this is trivial, (4 lines including error handling) so even if we expect very few bindings to make use of it it is not going to be maintenance drag. That would look like this: diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt index fd06c09..085d042 100644 --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt @@ -19,7 +19,10 @@ Optional properties: = Data cells = These are the child nodes of the provider which contain data cell -information like offset and size in nvmem provider. +information like offset and size in nvmem provider. Alternatively the data +cells can be grouped in a node that has a compatible property set to +"nvmem-cells". + Required properties: reg: specifies the offset in byte within the storage device. diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 4e94a78..3e1369c 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, if (!nvmem_np) return ERR_PTR(-EINVAL); + /* bindings that already have anonymous child nodes can instead put + * their cells in a child node with an nvmem-cells compatible. */ + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) { + nvmem_np = of_get_next_parent(nvmem_np); + if (!nvmem_np) + return ERR_PTR(-EINVAL); + } + nvmem = __nvmem_device_get(nvmem_np, NULL, NULL); of_node_put(nvmem_np); if (IS_ERR(nvmem)) What about it? Alban
On 08/06/18 18:07, Alban wrote: > On Fri, 8 Jun 2018 12:34:12 +0100 > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > ... > > I looked into this. It would work fine for the cells but not so nicely > for the nvmem device API. The phandle for the nvmem device would have > to reference the node passed here and not the real device. We would end > up with a DT like this: > > flash@0 { > compatible = "mtd"; > ... > nvmem_dev: nvmem-cells { > compatible = "nvmem-cells"; > ... > }; > }; > > other-device@10 { > ... > nvmem = <&nvmem_dev>; > }; > > Now if there is no cell defined we have this empty child node that make > very little sense, it is just there to accommodate the nvmem API. > NO. This just looks fine! nvmem-cells is the nvmem provider node without which you would not have any provider instance. All this looks as expected! Am not sure what is the problem here! > What I would suggest now is to just change the wording. We don't > deprecate the current binding, but we extend it to allow grouping the > cells in a child node if required. The code to support this is trivial, > (4 lines including error handling) so even if we expect very few > bindings to make use of it it is not going to be maintenance drag. > That would look like this: > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt > index fd06c09..085d042 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > @@ -19,7 +19,10 @@ Optional properties: > > = Data cells = > These are the child nodes of the provider which contain data cell > -information like offset and size in nvmem provider. > +information like offset and size in nvmem provider. Alternatively the data > +cells can be grouped in a node that has a compatible property set to > +"nvmem-cells". > + > > Required properties: > reg: specifies the offset in byte within the storage device. > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index 4e94a78..3e1369c 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, > if (!nvmem_np) > return ERR_PTR(-EINVAL); > > + /* bindings that already have anonymous child nodes can instead put > + * their cells in a child node with an nvmem-cells compatible. */ > + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) { > + nvmem_np = of_get_next_parent(nvmem_np); > + if (!nvmem_np) > + return ERR_PTR(-EINVAL); > + } > + > nvmem = __nvmem_device_get(nvmem_np, NULL, NULL); > of_node_put(nvmem_np); > if (IS_ERR(nvmem)) > > What about it? Let me repeat what I have said in my previous emails: Having a subnode still sounds very fragile to me, and this could be much specific case of MTD provider. We might have instances where this could be sub-sub node of the the original provider for other providers. Also I do not want to bring in Provider specifics layout into nvmem bindings. I can not make myself any clearer than this, Its going to be a NAK from my side for the above reasons! Also, patch I shared should give enough flexibility to various range of providers which have different child node layouts without touching the nvmem bindings. If it works please use it. thanks, srini > > Alban >
On Sun, 10 Jun 2018 11:32:36 +0100 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > On 08/06/18 18:07, Alban wrote: > > On Fri, 8 Jun 2018 12:34:12 +0100 > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > ... > > > > I looked into this. It would work fine for the cells but not so nicely > > for the nvmem device API. The phandle for the nvmem device would have > > to reference the node passed here and not the real device. We would end > > up with a DT like this: > > > > flash@0 { > > compatible = "mtd"; > > ... > > nvmem_dev: nvmem-cells { > > compatible = "nvmem-cells"; > > ... > > }; > > }; > > > > other-device@10 { > > ... > > nvmem = <&nvmem_dev>; > > }; > > > > Now if there is no cell defined we have this empty child node that make > > very little sense, it is just there to accommodate the nvmem API. > > > NO. This just looks fine! > nvmem-cells is the nvmem provider node without which you would not have > any provider instance. > All this looks as expected! > Am not sure what is the problem here! The problem is that DT should represent the hardware, not the OS API. What should be represented is that other drivers can access data stored on this device. It is my understanding that this wouldn't be an acceptable binding as the nvmem provider node would only exists because of how the NVMEM API currently works, a correct binding would just directly reference the storage device without this extra node. > > What I would suggest now is to just change the wording. We don't > > deprecate the current binding, but we extend it to allow grouping the > > cells in a child node if required. The code to support this is trivial, > > (4 lines including error handling) so even if we expect very few > > bindings to make use of it it is not going to be maintenance drag. > > That would look like this: > > > > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > index fd06c09..085d042 100644 > > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > > @@ -19,7 +19,10 @@ Optional properties: > > > > = Data cells = > > These are the child nodes of the provider which contain data cell > > -information like offset and size in nvmem provider. > > +information like offset and size in nvmem provider. Alternatively the data > > +cells can be grouped in a node that has a compatible property set to > > +"nvmem-cells". > > + > > > > Required properties: > > reg: specifies the offset in byte within the storage device. > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > > index 4e94a78..3e1369c 100644 > > --- a/drivers/nvmem/core.c > > +++ b/drivers/nvmem/core.c > > @@ -859,6 +859,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, > > if (!nvmem_np) > > return ERR_PTR(-EINVAL); > > > > + /* bindings that already have anonymous child nodes can instead put > > + * their cells in a child node with an nvmem-cells compatible. */ > > + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) { > > + nvmem_np = of_get_next_parent(nvmem_np); > > + if (!nvmem_np) > > + return ERR_PTR(-EINVAL); > > + } > > + > > nvmem = __nvmem_device_get(nvmem_np, NULL, NULL); > > of_node_put(nvmem_np); > > if (IS_ERR(nvmem)) > > > > What about it? > Let me repeat what I have said in my previous emails: > > Having a subnode still sounds very fragile to me, > and this could be much specific case of MTD provider. We might have > instances where this could be sub-sub node of the the original provider > for other providers. Also I do not want to bring in Provider specifics > layout into nvmem bindings. > > I can not make myself any clearer than this, Its going to be a NAK from > my side for the above reasons! I fully understand you concern but I think they are overblown. First I highly doubt that more layouts will ever be needed, using a compatible string pretty much guarantee that we won't clash with another binding. Furthermore even if you consider this extension "MTD specific" the amount of code is very small, non intrusive and only run once at registration time. I would understand if we were talking about pages of code nesting in various place, but not really when it is a single small if block with an obvious condition. And finally I don't see that as MTD specific as any other device could use this feature without any code change. > Also, patch I shared should give enough flexibility to various range of > providers which have different child node layouts without touching the > nvmem bindings. If it works please use it. It works from a code POV but it break the basic guidelines of DT bindings. As I want to have this done, I'm going to do a patch as you want, but I see a high chance that the binding is going to be rejected by the DT maintainers and we'll be back here again. Alban
On 10/06/18 12:36, Alban wrote: > On Sun, 10 Jun 2018 11:32:36 +0100 > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > >> On 08/06/18 18:07, Alban wrote: >>> On Fri, 8 Jun 2018 12:34:12 +0100 >>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: >>> >> ... >>> >>> I looked into this. It would work fine for the cells but not so nicely >>> for the nvmem device API. The phandle for the nvmem device would have >>> to reference the node passed here and not the real device. We would end >>> up with a DT like this: >>> >>> flash@0 { >>> compatible = "mtd"; >>> ... >>> nvmem_dev: nvmem-cells { >>> compatible = "nvmem-cells"; >>> ... >>> }; >>> }; >>> >>> other-device@10 { >>> ... >>> nvmem = <&nvmem_dev>; >>> }; >>> >>> Now if there is no cell defined we have this empty child node that make >>> very little sense, it is just there to accommodate the nvmem API. >>> >> NO. This just looks fine! >> nvmem-cells is the nvmem provider node without which you would not have >> any provider instance. >> All this looks as expected! >> Am not sure what is the problem here! > > The problem is that DT should represent the hardware, not the OS API. Exactly!! flash/mtd has nvmem provider which should be represented in the DT. There is no change in DT side from your original patch vs the new approach. You still are going to have the same subnode. Isn't it? AFAIU, the new approach will make it explicit that there is a nvmem provider in the DT. ... > What should be represented is that other drivers can access data stored > on this device. It is my understanding that this wouldn't be an > acceptable binding as the nvmem provider node would only exists because > of how the NVMEM API currently works, a correct binding would just > directly reference the storage device without this extra node. > ... >> Having a subnode still sounds very fragile to me, >> and this could be much specific case of MTD provider. We might have >> instances where this could be sub-sub node of the the original provider >> for other providers. Also I do not want to bring in Provider specifics >> layout into nvmem bindings. >> >> I can not make myself any clearer than this, Its going to be a NAK from >> my side for the above reasons! > > I fully understand you concern but I think they are overblown. First I > highly doubt that more layouts will ever be needed, using a compatible > string pretty much guarantee that we won't clash with another binding. > Furthermore even if you consider this extension "MTD specific" the > amount of code is very small, non intrusive and only run once at > registration time. I would understand if we were talking about pages of > code nesting in various place, but not really when it is a single small > if block with an obvious condition. And finally I don't see that as MTD > specific as any other device could use this feature without any code > change. > >> Also, patch I shared should give enough flexibility to various range of >> providers which have different child node layouts without touching the >> nvmem bindings. If it works please use it. > > It works from a code POV but it break the basic guidelines of DT > bindings. As I want to have this done, I'm going to do a patch as you > want, but I see a high chance that the binding is going to be rejected > by the DT maintainers and we'll be back here again. If you think the sub node is going to be a problem from MTD point of view then that is worth discussing. Adding subnode in nvmem bindings is not going to help or make the situation any better. Lets see how this goes! thanks, srini > > Alban >
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt index fd06c09..6b723e7 100644 --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt @@ -11,14 +11,29 @@ these data from, and where they are stored on the storage device. This document is here to document this. = Data providers = -Contains bindings specific to provider drivers and data cells as children -of this node. +A data provider should have a subnode named 'nvmem-cells' that contains +a subnodes for each data cells. + +For backward compatibility the nvmem data cells can be direct children +of the data provider. This use is discouraged as it can conflict with +other bindings. Optional properties: read-only: Mark the provider as read only. += Data cells list = +The data cells list node should be named 'nvmem-cells' and have a +child node for each data cell. + +Required properties: + compatible: Must be "nvmem-cells" + #address-cells: <1> if the provider use 32 bit addressing, + <2> for 64 bits addressing + #size-cells: <1> if the provider use 32 bit sizes, + <2> for 64 bits sizes + = Data cells = -These are the child nodes of the provider which contain data cell +These are the child nodes of the nvmem-cells node which contain data cell information like offset and size in nvmem provider. Required properties: @@ -37,24 +52,30 @@ For example: ... /* Data cells */ - tsens_calibration: calib@404 { - reg = <0x404 0x10>; - }; + nvmem-cells { + compatible = "nvmem-cells"; + #address-cells = <1>; + #size-cells = <1>; - tsens_calibration_bckp: calib_bckp@504 { - reg = <0x504 0x11>; - bits = <6 128> - }; + tsens_calibration: calib@404 { + reg = <0x404 0x10>; + }; - pvs_version: pvs-version@6 { - reg = <0x6 0x2> - bits = <7 2> - }; + tsens_calibration_bckp: calib_bckp@504 { + reg = <0x504 0x11>; + bits = <6 128> + }; - speed_bin: speed-bin@c{ - reg = <0xc 0x1>; - bits = <2 3>; + pvs_version: pvs-version@6 { + reg = <0x6 0x2> + bits = <7 2> + }; + speed_bin: speed-bin@c{ + reg = <0xc 0x1>; + bits = <2 3>; + + }; }; ... }; diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 78051f0..a59195c 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -783,6 +783,16 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, if (!nvmem_np) return ERR_PTR(-EINVAL); + /* Devices using the new binding have all the cells in + * a subnode with compatible = "nvmem-cells". In this + * case the device will be the parent of this node. + */ + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) { + nvmem_np = of_get_next_parent(nvmem_np); + if (!nvmem_np) + return ERR_PTR(-EINVAL); + } + nvmem = __nvmem_device_get(nvmem_np, NULL, NULL); of_node_put(nvmem_np); if (IS_ERR(nvmem))
Having the cells as subnodes of the provider device without any compatible property might clash with other bindings. To avoid this problem update the binding to have all the cells in a 'nvmem-cells' subnode with a 'nvmem-cells' compatible string. This new binding guarantee that we can turn any kind of device in a nvmem provider. While discouraged for new uses the old scheme is still supported for backward compatibility. Signed-off-by: Alban Bedel <albeu@free.fr> --- Documentation/devicetree/bindings/nvmem/nvmem.txt | 55 ++++++++++++++++------- drivers/nvmem/core.c | 10 +++++ 2 files changed, 48 insertions(+), 17 deletions(-)