* [RFC PATCH v2 0/2] support 512B ECC step size for Meson NAND
@ 2023-07-05 6:54 Arseniy Krasnov
2023-07-05 6:54 ` [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size Arseniy Krasnov
2023-07-05 6:54 ` [RFC PATCH v2 2/2] mtd: rawnand: " Arseniy Krasnov
0 siblings, 2 replies; 9+ messages in thread
From: Arseniy Krasnov @ 2023-07-05 6:54 UTC (permalink / raw)
To: Liang Yang, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl
Cc: oxffffaa, kernel, Arseniy Krasnov, linux-mtd, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Hello,
this patchset adds support for 512B ECC step size for Meson NAND. Current
implementation only supports 1024B. There are two patches:
1) Update for device tree bindings to replace 'const' type of field
'nand-ecc-step-size' with 'enum' which contains 512 and 1024. Example
is also updated.
2) Update for Meson driver - new enum value for 512B ECC and reworked
ECC capabilities structure to support both 512B and 1024B ECC. By
default this driver uses 1024B ECC, 512B could be enabled in device
tree.
Changelog:
v1 -> v2:
* Add default value of 1024 to the bindings patch (0001).
* Remove "Acked-by: Rob Herring <robh@kernel.org>" from the bindings
patch (0001) due to added default value.
* Remove invalid calculation of OOB bytes, available for ECC engine
from patch 0002. This logic is incorrect from the origins, so I don't
touch it in this patchset. It will be fixed by another patch, as in
fact, it doesn't affect this patchset.
Links:
v1:
https://lore.kernel.org/linux-mtd/20230628092937.538683-1-AVKrasnov@sberdevices.ru/
Arseniy Krasnov (2):
dt-bindings: nand: meson: support for 512B ECC step size
mtd: rawnand: meson: support for 512B ECC step size
.../bindings/mtd/amlogic,meson-nand.yaml | 4 +-
drivers/mtd/nand/raw/meson_nand.c | 45 ++++++++++++++-----
2 files changed, 37 insertions(+), 12 deletions(-)
--
2.35.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 6:54 [RFC PATCH v2 0/2] support 512B ECC step size for Meson NAND Arseniy Krasnov
@ 2023-07-05 6:54 ` Arseniy Krasnov
2023-07-05 7:37 ` Miquel Raynal
2023-07-05 16:03 ` Rob Herring
2023-07-05 6:54 ` [RFC PATCH v2 2/2] mtd: rawnand: " Arseniy Krasnov
1 sibling, 2 replies; 9+ messages in thread
From: Arseniy Krasnov @ 2023-07-05 6:54 UTC (permalink / raw)
To: Liang Yang, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl
Cc: oxffffaa, kernel, Arseniy Krasnov, linux-mtd, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
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
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>;
};
};
--
2.35.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH v2 2/2] mtd: rawnand: meson: support for 512B ECC step size
2023-07-05 6:54 [RFC PATCH v2 0/2] support 512B ECC step size for Meson NAND Arseniy Krasnov
2023-07-05 6:54 ` [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size Arseniy Krasnov
@ 2023-07-05 6:54 ` Arseniy Krasnov
1 sibling, 0 replies; 9+ messages in thread
From: Arseniy Krasnov @ 2023-07-05 6:54 UTC (permalink / raw)
To: Liang Yang, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, Kevin Hilman, Jerome Brunet,
Martin Blumenstingl
Cc: oxffffaa, kernel, Arseniy Krasnov, linux-mtd, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Meson NAND supports both 512B and 1024B ECC step size.
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
drivers/mtd/nand/raw/meson_nand.c | 45 +++++++++++++++++++++++--------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 345212e8c691..369e81356240 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
struct meson_nand_ecc {
u32 bch;
u32 strength;
+ u32 size;
};
struct meson_nfc_data {
@@ -190,7 +191,8 @@ struct meson_nfc {
};
enum {
- NFC_ECC_BCH8_1K = 2,
+ NFC_ECC_BCH8_512 = 1,
+ NFC_ECC_BCH8_1K,
NFC_ECC_BCH24_1K,
NFC_ECC_BCH30_1K,
NFC_ECC_BCH40_1K,
@@ -198,15 +200,16 @@ enum {
NFC_ECC_BCH60_1K,
};
-#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)}
+#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) }
static struct meson_nand_ecc meson_ecc[] = {
- MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
- MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
- MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
- MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
- MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
- MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
+ MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512),
+ MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024),
+ MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
+ MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
+ MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
+ MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
+ MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
};
static int meson_nand_calc_ecc_bytes(int step_size, int strength)
@@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
-NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
- meson_nand_calc_ecc_bytes, 1024, 8);
+
+static const int axg_stepinfo_strengths[] = { 8 };
+static const struct nand_ecc_step_info axg_stepinfo_1024 = {
+ .stepsize = 1024,
+ .strengths = axg_stepinfo_strengths,
+ .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
+};
+
+static const struct nand_ecc_step_info axg_stepinfo_512 = {
+ .stepsize = 512,
+ .strengths = axg_stepinfo_strengths,
+ .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
+};
+
+static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
+
+static const struct nand_ecc_caps meson_axg_ecc_caps = {
+ .stepinfos = axg_stepinfo,
+ .nstepinfos = ARRAY_SIZE(axg_stepinfo),
+ .calc_ecc_bytes = meson_nand_calc_ecc_bytes,
+};
static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
{
@@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
return -EINVAL;
for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
- if (meson_ecc[i].strength == nand->ecc.strength) {
+ if (meson_ecc[i].strength == nand->ecc.strength &&
+ meson_ecc[i].size == nand->ecc.size) {
meson_chip->bch_mode = meson_ecc[i].bch;
return 0;
}
--
2.35.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 6:54 ` [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size Arseniy Krasnov
@ 2023-07-05 7:37 ` Miquel Raynal
2023-07-05 8:03 ` Arseniy Krasnov
2023-07-05 16:03 ` Rob Herring
1 sibling, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2023-07-05 7:37 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd,
devicetree, linux-arm-kernel, linux-amlogic, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 7:37 ` Miquel Raynal
@ 2023-07-05 8:03 ` Arseniy Krasnov
2023-07-05 8:22 ` Miquel Raynal
0 siblings, 1 reply; 9+ messages in thread
From: Arseniy Krasnov @ 2023-07-05 8:03 UTC (permalink / raw)
To: Miquel Raynal
Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd,
devicetree, linux-arm-kernel, linux-amlogic, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 8:03 ` Arseniy Krasnov
@ 2023-07-05 8:22 ` Miquel Raynal
2023-07-06 5:57 ` Arseniy Krasnov
0 siblings, 1 reply; 9+ messages in thread
From: Miquel Raynal @ 2023-07-05 8:22 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd,
devicetree, linux-arm-kernel, linux-amlogic, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 6:54 ` [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size Arseniy Krasnov
2023-07-05 7:37 ` Miquel Raynal
@ 2023-07-05 16:03 ` Rob Herring
1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring @ 2023-07-05 16:03 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Krzysztof Kozlowski, linux-mtd, devicetree, Neil Armstrong,
Jerome Brunet, Vignesh Raghavendra, kernel, Richard Weinberger,
Martin Blumenstingl, linux-arm-kernel, Liang Yang, Miquel Raynal,
Conor Dooley, Kevin Hilman, Rob Herring, oxffffaa, linux-amlogic,
linux-kernel
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-05 8:22 ` Miquel Raynal
@ 2023-07-06 5:57 ` Arseniy Krasnov
2023-07-06 6:05 ` Miquel Raynal
0 siblings, 1 reply; 9+ messages in thread
From: Arseniy Krasnov @ 2023-07-06 5:57 UTC (permalink / raw)
To: Miquel Raynal
Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd,
devicetree, linux-arm-kernel, linux-amlogic, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size
2023-07-06 5:57 ` Arseniy Krasnov
@ 2023-07-06 6:05 ` Miquel Raynal
0 siblings, 0 replies; 9+ messages in thread
From: Miquel Raynal @ 2023-07-06 6:05 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Liang Yang, Richard Weinberger, Vignesh Raghavendra, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Neil Armstrong, Kevin Hilman,
Jerome Brunet, Martin Blumenstingl, oxffffaa, kernel, linux-mtd,
devicetree, linux-arm-kernel, linux-amlogic, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-07-06 6:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-05 6:54 [RFC PATCH v2 0/2] support 512B ECC step size for Meson NAND Arseniy Krasnov
2023-07-05 6:54 ` [RFC PATCH v2 1/2] dt-bindings: nand: meson: support for 512B ECC step size Arseniy Krasnov
2023-07-05 7:37 ` Miquel Raynal
2023-07-05 8:03 ` Arseniy Krasnov
2023-07-05 8:22 ` Miquel Raynal
2023-07-06 5:57 ` Arseniy Krasnov
2023-07-06 6:05 ` Miquel Raynal
2023-07-05 16:03 ` Rob Herring
2023-07-05 6:54 ` [RFC PATCH v2 2/2] mtd: rawnand: " Arseniy Krasnov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).