| Message ID | 20230705065434.297040-2-AVKrasnov@sberdevices.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support 512B ECC step size for Meson NAND | expand |
Hi Arseniy, AVKrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 09:54:33 +0300: > Meson NAND supports both 512B and 1024B ECC step size, so replace > 'const' for only 1024B step size with enum for both sizes. > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > index 3bec8af91bbb..81ca8828731a 100644 > --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > @@ -49,7 +49,8 @@ patternProperties: > const: hw > > nand-ecc-step-size: > - const: 1024 > + enum: [512, 1024] > + default: 1024 I was actually wrong in my previous review, there is no strong default here as the existing binding (and code) try to use the closest parameters required by the NAND chip: we pick the "optimal" configuration. So if you don't provide any value here, we expect the strength and step size advertized by the chip to be used. This is a common default in the raw NAND subsystem. Please drop the default line, re-integrate the missing R-by tag from Rob and in a separate patch please mark nand-ecc-step-size and nand-ecc-strength mandatory if the other is provide. IOW, we expect either both, or none of them, but not a single one. > > nand-ecc-strength: > enum: [8, 16, 24, 30, 40, 50, 60] > @@ -93,6 +94,7 @@ examples: > nand@0 { > reg = <0>; > nand-rb = <0>; > + nand-ecc-step-size = <1024>; So in the end this line is wrong and once you get the description right as I mentioned it above, this will fail to pass `make DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/ dt_binidng_check` Please drop it from the example, don't add the second property here, it's best to show a clean example where people stop tampering for no reason with the optimal values. > }; > }; > Thanks, Miquèl
On 05.07.2023 10:37, Miquel Raynal wrote: > Hi Arseniy, > > AVKrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 09:54:33 +0300: > >> Meson NAND supports both 512B and 1024B ECC step size, so replace >> 'const' for only 1024B step size with enum for both sizes. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> index 3bec8af91bbb..81ca8828731a 100644 >> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> @@ -49,7 +49,8 @@ patternProperties: >> const: hw >> >> nand-ecc-step-size: >> - const: 1024 >> + enum: [512, 1024] >> + default: 1024 > > I was actually wrong in my previous review, there is no strong default > here as the existing binding (and code) try to use the closest > parameters required by the NAND chip: we pick the "optimal" > configuration. So if you don't provide any value here, we expect > the strength and step size advertized by the chip to be used. This is a > common default in the raw NAND subsystem. > > Please drop the default line, re-integrate the missing R-by tag from > Rob and in a separate patch please mark nand-ecc-step-size and > nand-ecc-strength mandatory if the other is provide. IOW, we expect > either both, or none of them, but not a single one. I see, no problem! "mandatory" means update description of both fields like: description: Mandatory if nand-ecc-step-size is set. etc. ? > >> >> nand-ecc-strength: >> enum: [8, 16, 24, 30, 40, 50, 60] >> @@ -93,6 +94,7 @@ examples: >> nand@0 { >> reg = <0>; >> nand-rb = <0>; >> + nand-ecc-step-size = <1024>; > > So in the end this line is wrong and once you get the description right > as I mentioned it above, this will fail to pass > `make DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/ dt_binidng_check` > Please drop it from the example, don't add the second property here, > it's best to show a clean example where people stop tampering for no > reason with the optimal values. Ok! Thanks, Arseniy > >> }; >> }; >> > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 11:03:30 +0300: > On 05.07.2023 10:37, Miquel Raynal wrote: > > Hi Arseniy, > > > > AVKrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 09:54:33 +0300: > > > >> Meson NAND supports both 512B and 1024B ECC step size, so replace > >> 'const' for only 1024B step size with enum for both sizes. > >> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >> index 3bec8af91bbb..81ca8828731a 100644 > >> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >> @@ -49,7 +49,8 @@ patternProperties: > >> const: hw > >> > >> nand-ecc-step-size: > >> - const: 1024 > >> + enum: [512, 1024] > >> + default: 1024 > > > > I was actually wrong in my previous review, there is no strong default > > here as the existing binding (and code) try to use the closest > > parameters required by the NAND chip: we pick the "optimal" > > configuration. So if you don't provide any value here, we expect > > the strength and step size advertized by the chip to be used. This is a > > common default in the raw NAND subsystem. > > > > Please drop the default line, re-integrate the missing R-by tag from > > Rob and in a separate patch please mark nand-ecc-step-size and > > nand-ecc-strength mandatory if the other is provide. IOW, we expect > > either both, or none of them, but not a single one. > > I see, no problem! "mandatory" means update description of both fields like: > > description: > Mandatory if nand-ecc-step-size is set. Nope :-) Something along: allOf: - if: <nand-chip>: properties: contains: - nand-ecc-step-size then: required: <nand-chip>: properties: - nand-ecc-strength And same with the opposite logic. > > etc. > > ? > > > > >> > >> nand-ecc-strength: > >> enum: [8, 16, 24, 30, 40, 50, 60] > >> @@ -93,6 +94,7 @@ examples: > >> nand@0 { > >> reg = <0>; > >> nand-rb = <0>; > >> + nand-ecc-step-size = <1024>; > > > > So in the end this line is wrong and once you get the description right > > as I mentioned it above, this will fail to pass > > `make DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/ dt_binidng_check` > > Please drop it from the example, don't add the second property here, > > it's best to show a clean example where people stop tampering for no > > reason with the optimal values. > > Ok! > > Thanks, Arseniy > > > > >> }; > >> }; > >> > > > > > > Thanks, > > Miquèl Thanks, Miquèl
On Wed, 05 Jul 2023 09:54:33 +0300, Arseniy Krasnov wrote: > Meson NAND supports both 512B and 1024B ECC step size, so replace > 'const' for only 1024B step size with enum for both sizes. > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed. Missing tags: Acked-by: Rob Herring <robh@kernel.org>
On 05.07.2023 11:22, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 11:03:30 +0300: > >> On 05.07.2023 10:37, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> AVKrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 09:54:33 +0300: >>> >>>> Meson NAND supports both 512B and 1024B ECC step size, so replace >>>> 'const' for only 1024B step size with enum for both sizes. >>>> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >>>> index 3bec8af91bbb..81ca8828731a 100644 >>>> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >>>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >>>> @@ -49,7 +49,8 @@ patternProperties: >>>> const: hw >>>> >>>> nand-ecc-step-size: >>>> - const: 1024 >>>> + enum: [512, 1024] >>>> + default: 1024 >>> >>> I was actually wrong in my previous review, there is no strong default >>> here as the existing binding (and code) try to use the closest >>> parameters required by the NAND chip: we pick the "optimal" >>> configuration. So if you don't provide any value here, we expect >>> the strength and step size advertized by the chip to be used. This is a >>> common default in the raw NAND subsystem. >>> >>> Please drop the default line, re-integrate the missing R-by tag from >>> Rob and in a separate patch please mark nand-ecc-step-size and >>> nand-ecc-strength mandatory if the other is provide. IOW, we expect >>> either both, or none of them, but not a single one. >> >> I see, no problem! "mandatory" means update description of both fields like: >> >> description: >> Mandatory if nand-ecc-step-size is set. > > Nope :-) > > Something along: > > allOf: > - if: > <nand-chip>: > properties: > contains: > - nand-ecc-step-size > then: > required: > <nand-chip>: > properties: > - nand-ecc-strength > > And same with the opposite logic. I see, thanks! And this should be for all nand chips, not only Amlogic? I mean in nand-chip.yaml? I'll include it as third patch in this patchset. Thanks, Arseniy > >> >> etc. >> >> ? >> >>> >>>> >>>> nand-ecc-strength: >>>> enum: [8, 16, 24, 30, 40, 50, 60] >>>> @@ -93,6 +94,7 @@ examples: >>>> nand@0 { >>>> reg = <0>; >>>> nand-rb = <0>; >>>> + nand-ecc-step-size = <1024>; >>> >>> So in the end this line is wrong and once you get the description right >>> as I mentioned it above, this will fail to pass >>> `make DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/ dt_binidng_check` >>> Please drop it from the example, don't add the second property here, >>> it's best to show a clean example where people stop tampering for no >>> reason with the optimal values. >> >> Ok! >> >> Thanks, Arseniy >> >>> >>>> }; >>>> }; >>>> >>> >>> >>> Thanks, >>> Miquèl > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Thu, 6 Jul 2023 08:57:00 +0300: > On 05.07.2023 11:22, Miquel Raynal wrote: > > Hi Arseniy, > > > > avkrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 11:03:30 +0300: > > > >> On 05.07.2023 10:37, Miquel Raynal wrote: > >>> Hi Arseniy, > >>> > >>> AVKrasnov@sberdevices.ru wrote on Wed, 5 Jul 2023 09:54:33 +0300: > >>> > >>>> Meson NAND supports both 512B and 1024B ECC step size, so replace > >>>> 'const' for only 1024B step size with enum for both sizes. > >>>> > >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >>>> --- > >>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >>>> index 3bec8af91bbb..81ca8828731a 100644 > >>>> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >>>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > >>>> @@ -49,7 +49,8 @@ patternProperties: > >>>> const: hw > >>>> > >>>> nand-ecc-step-size: > >>>> - const: 1024 > >>>> + enum: [512, 1024] > >>>> + default: 1024 > >>> > >>> I was actually wrong in my previous review, there is no strong default > >>> here as the existing binding (and code) try to use the closest > >>> parameters required by the NAND chip: we pick the "optimal" > >>> configuration. So if you don't provide any value here, we expect > >>> the strength and step size advertized by the chip to be used. This is a > >>> common default in the raw NAND subsystem. > >>> > >>> Please drop the default line, re-integrate the missing R-by tag from > >>> Rob and in a separate patch please mark nand-ecc-step-size and > >>> nand-ecc-strength mandatory if the other is provide. IOW, we expect > >>> either both, or none of them, but not a single one. > >> > >> I see, no problem! "mandatory" means update description of both fields like: > >> > >> description: > >> Mandatory if nand-ecc-step-size is set. > > > > Nope :-) > > > > Something along: > > > > allOf: > > - if: > > <nand-chip>: > > properties: > > contains: > > - nand-ecc-step-size > > then: > > required: > > <nand-chip>: > > properties: > > - nand-ecc-strength > > > > And same with the opposite logic. > > I see, thanks! And this should be for all nand chips, not only Amlogic? I mean in > nand-chip.yaml? Some drivers can directly manage the user requests in terms of either step size *or* strength, so I would keep this into the Amlogic file for now. > I'll include it as third patch in this patchset. > > Thanks, Arseniy Thanks, Miquèl
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml index 3bec8af91bbb..81ca8828731a 100644 --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml @@ -49,7 +49,8 @@ patternProperties: const: hw nand-ecc-step-size: - const: 1024 + enum: [512, 1024] + default: 1024 nand-ecc-strength: enum: [8, 16, 24, 30, 40, 50, 60] @@ -93,6 +94,7 @@ examples: nand@0 { reg = <0>; nand-rb = <0>; + nand-ecc-step-size = <1024>; }; };
Meson NAND supports both 512B and 1024B ECC step size, so replace 'const' for only 1024B step size with enum for both sizes. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)