* [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
@ 2019-12-07 0:48 Marek Vasut
2019-12-09 5:38 ` Masahiro Yamada
2019-12-20 8:50 ` Tudor.Ambarus
0 siblings, 2 replies; 10+ messages in thread
From: Marek Vasut @ 2019-12-07 0:48 UTC (permalink / raw)
To: linux-mtd
Cc: Marek Vasut, Vignesh Raghavendra, Richard Weinberger,
Ley Foon Tan, Masahiro Yamada, Miquel Raynal
The SPARE_AREA_SKIP_BYTES register is reset when the controller reset
signal is toggled. Yet, this register must be configured to match the
content of the NAND OOB area. The current default value is always set
to 8 and is programmed into the hardware in case the hardware was not
programmed before (e.g. in a bootloader) with a different value. This
however does not work when the block is reset properly by Linux.
On Altera SoCFPGA CycloneV, ArriaV and Arria10, which are the SoCFPGA
platforms which support booting from NAND, the SPARE_AREA_SKIP_BYTES
value must be set to 2. On Socionext Uniphier, the value is 8. This
patch adds support for preconfiguring the default value and handles
the special SoCFPGA case by setting the default to 2 on all SoCFPGA
platforms, while retaining the original behavior and default value of
8 on all the other platforms.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Ley Foon Tan <ley.foon.tan@intel.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
To: linux-mtd@lists.infradead.org
---
drivers/mtd/nand/raw/denali.c | 13 ++++++++++---
drivers/mtd/nand/raw/denali_dt.c | 6 ++++++
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index 3102ddbd8abdb..b6c463d021677 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
/*
* Set how many bytes should be skipped before writing data in OOB.
+ * If a non-zero value has already been configured, update it in HW.
* If a non-zero value has already been set (by firmware or something),
* just use it. Otherwise, set the driver's default.
*/
- denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
- if (!denali->oob_skip_bytes) {
- denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
+ if (denali->oob_skip_bytes) {
iowrite32(denali->oob_skip_bytes,
denali->reg + SPARE_AREA_SKIP_BYTES);
+ } else {
+ denali->oob_skip_bytes =
+ ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
+ if (!denali->oob_skip_bytes) {
+ denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
+ iowrite32(denali->oob_skip_bytes,
+ denali->reg + SPARE_AREA_SKIP_BYTES);
+ }
}
iowrite32(0, denali->reg + TRANSFER_SPARE_REG);
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index 5e14836f6bd5a..34c7c553f3412 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -27,6 +27,7 @@ struct denali_dt {
struct denali_dt_data {
unsigned int revision;
unsigned int caps;
+ unsigned int oob_skip_bytes;
const struct nand_ecc_caps *ecc_caps;
};
@@ -34,6 +35,7 @@ NAND_ECC_CAPS_SINGLE(denali_socfpga_ecc_caps, denali_calc_ecc_bytes,
512, 8, 15);
static const struct denali_dt_data denali_socfpga_data = {
.caps = DENALI_CAP_HW_ECC_FIXUP,
+ .oob_skip_bytes = 2,
.ecc_caps = &denali_socfpga_ecc_caps,
};
@@ -42,6 +44,7 @@ NAND_ECC_CAPS_SINGLE(denali_uniphier_v5a_ecc_caps, denali_calc_ecc_bytes,
static const struct denali_dt_data denali_uniphier_v5a_data = {
.caps = DENALI_CAP_HW_ECC_FIXUP |
DENALI_CAP_DMA_64BIT,
+ .oob_skip_bytes = 8,
.ecc_caps = &denali_uniphier_v5a_ecc_caps,
};
@@ -51,6 +54,7 @@ static const struct denali_dt_data denali_uniphier_v5b_data = {
.revision = 0x0501,
.caps = DENALI_CAP_HW_ECC_FIXUP |
DENALI_CAP_DMA_64BIT,
+ .oob_skip_bytes = 8,
.ecc_caps = &denali_uniphier_v5b_ecc_caps,
};
@@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
denali->clk_rate = clk_get_rate(dt->clk);
denali->clk_x_rate = clk_get_rate(dt->clk_x);
+ denali->oob_skip_bytes = data->oob_skip_bytes;
+
ret = denali_init(denali);
if (ret)
goto out_disable_clk_ecc;
--
2.24.0
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-07 0:48 [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES Marek Vasut
@ 2019-12-09 5:38 ` Masahiro Yamada
2019-12-09 12:53 ` Marek Vasut
2019-12-20 8:50 ` Tudor.Ambarus
1 sibling, 1 reply; 10+ messages in thread
From: Masahiro Yamada @ 2019-12-09 5:38 UTC (permalink / raw)
To: Marek Vasut
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On Sat, Dec 7, 2019 at 9:49 AM Marek Vasut <marex@denx.de> wrote:
>
> The SPARE_AREA_SKIP_BYTES register is reset when the controller reset
> signal is toggled. Yet, this register must be configured to match the
> content of the NAND OOB area. The current default value is always set
> to 8 and is programmed into the hardware in case the hardware was not
> programmed before (e.g. in a bootloader) with a different value. This
> however does not work when the block is reset properly by Linux.
>
> On Altera SoCFPGA CycloneV, ArriaV and Arria10, which are the SoCFPGA
> platforms which support booting from NAND, the SPARE_AREA_SKIP_BYTES
> value must be set to 2. On Socionext Uniphier, the value is 8. This
> patch adds support for preconfiguring the default value and handles
> the special SoCFPGA case by setting the default to 2 on all SoCFPGA
> platforms, while retaining the original behavior and default value of
> 8 on all the other platforms.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Ley Foon Tan <ley.foon.tan@intel.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> To: linux-mtd@lists.infradead.org
> ---
> drivers/mtd/nand/raw/denali.c | 13 ++++++++++---
> drivers/mtd/nand/raw/denali_dt.c | 6 ++++++
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index 3102ddbd8abdb..b6c463d021677 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>
> /*
> * Set how many bytes should be skipped before writing data in OOB.
> + * If a non-zero value has already been configured, update it in HW.
> * If a non-zero value has already been set (by firmware or something),
> * just use it. Otherwise, set the driver's default.
> */
> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> - if (!denali->oob_skip_bytes) {
> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> + if (denali->oob_skip_bytes) {
> iowrite32(denali->oob_skip_bytes,
> denali->reg + SPARE_AREA_SKIP_BYTES);
> + } else {
> + denali->oob_skip_bytes =
> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> + if (!denali->oob_skip_bytes) {
> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> + iowrite32(denali->oob_skip_bytes,
> + denali->reg + SPARE_AREA_SKIP_BYTES);
This fallback is ugly, and should be removed, I think.
It is only reachable by PCI platform (Intel MRST), where
DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
> denali->clk_rate = clk_get_rate(dt->clk);
> denali->clk_x_rate = clk_get_rate(dt->clk_x);
>
> + denali->oob_skip_bytes = data->oob_skip_bytes;
> +
Please move this to the relevant hunk.
Preferably, based on this:
http://patchwork.ozlabs.org/patch/1205912/
> ret = denali_init(denali);
> if (ret)
> goto out_disable_clk_ecc;
> --
> 2.24.0
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
Best Regards
Masahiro Yamada
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-09 5:38 ` Masahiro Yamada
@ 2019-12-09 12:53 ` Marek Vasut
2019-12-10 3:15 ` Masahiro Yamada
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2019-12-09 12:53 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On 12/9/19 6:38 AM, Masahiro Yamada wrote:
[...]
>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>> index 3102ddbd8abdb..b6c463d021677 100644
>> --- a/drivers/mtd/nand/raw/denali.c
>> +++ b/drivers/mtd/nand/raw/denali.c
>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>>
>> /*
>> * Set how many bytes should be skipped before writing data in OOB.
>> + * If a non-zero value has already been configured, update it in HW.
>> * If a non-zero value has already been set (by firmware or something),
>> * just use it. Otherwise, set the driver's default.
>> */
>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>> - if (!denali->oob_skip_bytes) {
>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>> + if (denali->oob_skip_bytes) {
>> iowrite32(denali->oob_skip_bytes,
>> denali->reg + SPARE_AREA_SKIP_BYTES);
>> + } else {
>> + denali->oob_skip_bytes =
>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>> + if (!denali->oob_skip_bytes) {
>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>> + iowrite32(denali->oob_skip_bytes,
>> + denali->reg + SPARE_AREA_SKIP_BYTES);
>
> This fallback is ugly, and should be removed, I think.
> It is only reachable by PCI platform (Intel MRST), where
> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
This fallback retains the original behavior on all platforms. It might
not be to your liking, but it does not break other platforms while
fixing SoCFPGA. We don't know what other platforms might be depending on
this behavior, do we ?
>> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
>> denali->clk_rate = clk_get_rate(dt->clk);
>> denali->clk_x_rate = clk_get_rate(dt->clk_x);
>>
>> + denali->oob_skip_bytes = data->oob_skip_bytes;
>> +
>
> Please move this to the relevant hunk.
> Preferably, based on this:
> http://patchwork.ozlabs.org/patch/1205912/
I don't understand what you want me to change ? Shall I rebase this on
top of your patch from today and repost ?
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-09 12:53 ` Marek Vasut
@ 2019-12-10 3:15 ` Masahiro Yamada
2019-12-10 3:32 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Masahiro Yamada @ 2019-12-10 3:15 UTC (permalink / raw)
To: Marek Vasut
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@denx.de> wrote:
>
> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
> [...]
>
> >> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >> index 3102ddbd8abdb..b6c463d021677 100644
> >> --- a/drivers/mtd/nand/raw/denali.c
> >> +++ b/drivers/mtd/nand/raw/denali.c
> >> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
> >>
> >> /*
> >> * Set how many bytes should be skipped before writing data in OOB.
> >> + * If a non-zero value has already been configured, update it in HW.
> >> * If a non-zero value has already been set (by firmware or something),
> >> * just use it. Otherwise, set the driver's default.
> >> */
> >> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >> - if (!denali->oob_skip_bytes) {
> >> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >> + if (denali->oob_skip_bytes) {
> >> iowrite32(denali->oob_skip_bytes,
> >> denali->reg + SPARE_AREA_SKIP_BYTES);
> >> + } else {
> >> + denali->oob_skip_bytes =
> >> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >> + if (!denali->oob_skip_bytes) {
> >> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >> + iowrite32(denali->oob_skip_bytes,
> >> + denali->reg + SPARE_AREA_SKIP_BYTES);
> >
> > This fallback is ugly, and should be removed, I think.
> > It is only reachable by PCI platform (Intel MRST), where
> > DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
>
> This fallback retains the original behavior on all platforms. It might
> not be to your liking, but it does not break other platforms while
> fixing SoCFPGA. We don't know what other platforms might be depending on
> this behavior, do we ?
if (denali->oob_skip_bytes) {
iowrite32(denali->oob_skip_bytes,
denali->reg + SPARE_AREA_SKIP_BYTES);
else
denali->oob_skip_bytes =
ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
... retains the original behavior.
For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
to the well-defined value.
denali_pci.c is the only platform that can read back the
register value.
See, how Intel originally wrote the code.
https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
Please notice the part "if this value is 0, just let it be."
The Intel MRST platform happily accepts
SPARE_AREA_SKIP_BYTES being set to 0.
I am not sure how many people are using this platform,
but anyway it is how it has worked for a long time.
> >> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
> >> denali->clk_rate = clk_get_rate(dt->clk);
> >> denali->clk_x_rate = clk_get_rate(dt->clk_x);
> >>
> >> + denali->oob_skip_bytes = data->oob_skip_bytes;
> >> +
> >
> > Please move this to the relevant hunk.
> > Preferably, based on this:
> > http://patchwork.ozlabs.org/patch/1205912/
>
> I don't understand what you want me to change ? Shall I rebase this on
> top of your patch from today and repost ?
Yes.
I do not want to scatter the relevant code
(coping struct fields) to random places.
I want the code to look like this:
denali->revision = data->revision;
denali->caps = data->caps;
+ denali->oob_skip_bytes = data->oob_skip_bytes;
denali->ecc_caps = data->ecc_caps;
--
Best Regards
Masahiro Yamada
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-10 3:15 ` Masahiro Yamada
@ 2019-12-10 3:32 ` Marek Vasut
2019-12-10 6:30 ` Masahiro Yamada
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2019-12-10 3:32 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On 12/10/19 4:15 AM, Masahiro Yamada wrote:
> On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
>> [...]
>>
>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>> index 3102ddbd8abdb..b6c463d021677 100644
>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>>>>
>>>> /*
>>>> * Set how many bytes should be skipped before writing data in OOB.
>>>> + * If a non-zero value has already been configured, update it in HW.
>>>> * If a non-zero value has already been set (by firmware or something),
>>>> * just use it. Otherwise, set the driver's default.
>>>> */
>>>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>> - if (!denali->oob_skip_bytes) {
>>>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>> + if (denali->oob_skip_bytes) {
>>>> iowrite32(denali->oob_skip_bytes,
>>>> denali->reg + SPARE_AREA_SKIP_BYTES);
>>>> + } else {
>>>> + denali->oob_skip_bytes =
>>>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>> + if (!denali->oob_skip_bytes) {
>>>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>> + iowrite32(denali->oob_skip_bytes,
>>>> + denali->reg + SPARE_AREA_SKIP_BYTES);
>>>
>>> This fallback is ugly, and should be removed, I think.
>>> It is only reachable by PCI platform (Intel MRST), where
>>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
>>
>> This fallback retains the original behavior on all platforms. It might
>> not be to your liking, but it does not break other platforms while
>> fixing SoCFPGA. We don't know what other platforms might be depending on
>> this behavior, do we ?
>
> if (denali->oob_skip_bytes) {
> iowrite32(denali->oob_skip_bytes,
> denali->reg + SPARE_AREA_SKIP_BYTES);
> else
> denali->oob_skip_bytes =
> ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>
>
> ... retains the original behavior.
It does not, because if the readback in the else branch sets
oob_skip_bytes to 0, the controller is not updated with the default value.
> For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
> to the well-defined value.
>
> denali_pci.c is the only platform that can read back the
> register value.
>
> See, how Intel originally wrote the code.
>
> https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
>
>
> Please notice the part "if this value is 0, just let it be."
> The Intel MRST platform happily accepts
> SPARE_AREA_SKIP_BYTES being set to 0.
>
> I am not sure how many people are using this platform,
> but anyway it is how it has worked for a long time.
The intel platform might accept 0 happily, but that's not how the
controller was configured for a long time. So if I were to change the
code as you suggest, it would likely break some setups.
>>>> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
>>>> denali->clk_rate = clk_get_rate(dt->clk);
>>>> denali->clk_x_rate = clk_get_rate(dt->clk_x);
>>>>
>>>> + denali->oob_skip_bytes = data->oob_skip_bytes;
>>>> +
>>>
>>> Please move this to the relevant hunk.
>>> Preferably, based on this:
>>> http://patchwork.ozlabs.org/patch/1205912/
>>
>> I don't understand what you want me to change ? Shall I rebase this on
>> top of your patch from today and repost ?
>
> Yes.
>
> I do not want to scatter the relevant code
> (coping struct fields) to random places.
>
> I want the code to look like this:
>
> denali->revision = data->revision;
> denali->caps = data->caps;
> + denali->oob_skip_bytes = data->oob_skip_bytes;
> denali->ecc_caps = data->ecc_caps;
Maybe you can rebase your patch on top of this one then ? It was posted
later after all.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-10 3:32 ` Marek Vasut
@ 2019-12-10 6:30 ` Masahiro Yamada
2019-12-12 0:48 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Masahiro Yamada @ 2019-12-10 6:30 UTC (permalink / raw)
To: Marek Vasut
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On Tue, Dec 10, 2019 at 12:35 PM Marek Vasut <marex@denx.de> wrote:
>
> On 12/10/19 4:15 AM, Masahiro Yamada wrote:
> > On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
> >> [...]
> >>
> >>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>> index 3102ddbd8abdb..b6c463d021677 100644
> >>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
> >>>>
> >>>> /*
> >>>> * Set how many bytes should be skipped before writing data in OOB.
> >>>> + * If a non-zero value has already been configured, update it in HW.
> >>>> * If a non-zero value has already been set (by firmware or something),
> >>>> * just use it. Otherwise, set the driver's default.
> >>>> */
> >>>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> - if (!denali->oob_skip_bytes) {
> >>>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>> + if (denali->oob_skip_bytes) {
> >>>> iowrite32(denali->oob_skip_bytes,
> >>>> denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> + } else {
> >>>> + denali->oob_skip_bytes =
> >>>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> + if (!denali->oob_skip_bytes) {
> >>>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>> + iowrite32(denali->oob_skip_bytes,
> >>>> + denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>
> >>> This fallback is ugly, and should be removed, I think.
> >>> It is only reachable by PCI platform (Intel MRST), where
> >>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
> >>
> >> This fallback retains the original behavior on all platforms. It might
> >> not be to your liking, but it does not break other platforms while
> >> fixing SoCFPGA. We don't know what other platforms might be depending on
> >> this behavior, do we ?
> >
> > if (denali->oob_skip_bytes) {
> > iowrite32(denali->oob_skip_bytes,
> > denali->reg + SPARE_AREA_SKIP_BYTES);
> > else
> > denali->oob_skip_bytes =
> > ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >
> >
> > ... retains the original behavior.
>
> It does not, because if the readback in the else branch sets
> oob_skip_bytes to 0, the controller is not updated with the default value.
>
> > For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
> > to the well-defined value.
> >
> > denali_pci.c is the only platform that can read back the
> > register value.
> >
> > See, how Intel originally wrote the code.
> >
> > https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
> >
> >
> > Please notice the part "if this value is 0, just let it be."
> > The Intel MRST platform happily accepts
> > SPARE_AREA_SKIP_BYTES being set to 0.
> >
> > I am not sure how many people are using this platform,
> > but anyway it is how it has worked for a long time.
>
> The intel platform might accept 0 happily, but that's not how the
> controller was configured for a long time.
It is.
It worked like that for 9 years. (i.e. v2.6.35 - v4.19)
> So if I were to change the
> code as you suggest, it would likely break some setups.
There is no perfect solution here
when SPARE_AREA_SKIP_BYTES was set to 0
before booting the kernel.
[A] Keep SPARE_AREA_SKIP_BYTES as it is.
This might affect the factory-recorded BBM,
but it should at least work if the firmware or the boot-loader
has set up the controller this way.
[B] Override SPARE_AREA_SKIP_BYTES with
a different value (8).
This can keep the factory-based BBM, but
this is very unlikely to work across software stages
if the NAND device was formatted by the firmware or
the boot-loader.
We need to give up something.
[A] was the original, 9 years' default, and cleaner.
> >>>> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
> >>>> denali->clk_rate = clk_get_rate(dt->clk);
> >>>> denali->clk_x_rate = clk_get_rate(dt->clk_x);
> >>>>
> >>>> + denali->oob_skip_bytes = data->oob_skip_bytes;
> >>>> +
> >>>
> >>> Please move this to the relevant hunk.
> >>> Preferably, based on this:
> >>> http://patchwork.ozlabs.org/patch/1205912/
> >>
> >> I don't understand what you want me to change ? Shall I rebase this on
> >> top of your patch from today and repost ?
> >
> > Yes.
> >
> > I do not want to scatter the relevant code
> > (coping struct fields) to random places.
> >
> > I want the code to look like this:
> >
> > denali->revision = data->revision;
> > denali->caps = data->caps;
> > + denali->oob_skip_bytes = data->oob_skip_bytes;
> > denali->ecc_caps = data->ecc_caps;
>
> Maybe you can rebase your patch on top of this one then ? It was posted
> later after all.
Neither is applied yet.
If you have a chance for v2,
it would not be so annoying to move the code
to the relevant place.
If you really do not like rebase,
the second best would be:
if (data) {
denali->revision = data->revision;
denali->caps = data->caps;
+ denali->oob_skip_bytes = data->oob_skip_bytes;
denali->ecc_caps = data->ecc_caps;
}
--
Best Regards
Masahiro Yamada
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-10 6:30 ` Masahiro Yamada
@ 2019-12-12 0:48 ` Marek Vasut
2019-12-12 4:12 ` Masahiro Yamada
0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2019-12-12 0:48 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On 12/10/19 7:30 AM, Masahiro Yamada wrote:
> On Tue, Dec 10, 2019 at 12:35 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 12/10/19 4:15 AM, Masahiro Yamada wrote:
>>> On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
>>>> [...]
>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>>>> index 3102ddbd8abdb..b6c463d021677 100644
>>>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>>>>>>
>>>>>> /*
>>>>>> * Set how many bytes should be skipped before writing data in OOB.
>>>>>> + * If a non-zero value has already been configured, update it in HW.
>>>>>> * If a non-zero value has already been set (by firmware or something),
>>>>>> * just use it. Otherwise, set the driver's default.
>>>>>> */
>>>>>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>> - if (!denali->oob_skip_bytes) {
>>>>>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>>>> + if (denali->oob_skip_bytes) {
>>>>>> iowrite32(denali->oob_skip_bytes,
>>>>>> denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>> + } else {
>>>>>> + denali->oob_skip_bytes =
>>>>>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>> + if (!denali->oob_skip_bytes) {
>>>>>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>>>> + iowrite32(denali->oob_skip_bytes,
>>>>>> + denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>
>>>>> This fallback is ugly, and should be removed, I think.
>>>>> It is only reachable by PCI platform (Intel MRST), where
>>>>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
>>>>
>>>> This fallback retains the original behavior on all platforms. It might
>>>> not be to your liking, but it does not break other platforms while
>>>> fixing SoCFPGA. We don't know what other platforms might be depending on
>>>> this behavior, do we ?
>>>
>>> if (denali->oob_skip_bytes) {
>>> iowrite32(denali->oob_skip_bytes,
>>> denali->reg + SPARE_AREA_SKIP_BYTES);
>>> else
>>> denali->oob_skip_bytes =
>>> ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>
>>>
>>> ... retains the original behavior.
>>
>> It does not, because if the readback in the else branch sets
>> oob_skip_bytes to 0, the controller is not updated with the default value.
>>
>>> For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
>>> to the well-defined value.
>>>
>>> denali_pci.c is the only platform that can read back the
>>> register value.
>>>
>>> See, how Intel originally wrote the code.
>>>
>>> https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
>>>
>>>
>>> Please notice the part "if this value is 0, just let it be."
>>> The Intel MRST platform happily accepts
>>> SPARE_AREA_SKIP_BYTES being set to 0.
>>>
>>> I am not sure how many people are using this platform,
>>> but anyway it is how it has worked for a long time.
>>
>> The intel platform might accept 0 happily, but that's not how the
>> controller was configured for a long time.
>
> It is.
> It worked like that for 9 years. (i.e. v2.6.35 - v4.19)
So it is broken now ?
If so, then that fix is for another patch.
>> So if I were to change the
>> code as you suggest, it would likely break some setups.
>
> There is no perfect solution here
> when SPARE_AREA_SKIP_BYTES was set to 0
> before booting the kernel.
>
> [A] Keep SPARE_AREA_SKIP_BYTES as it is.
> This might affect the factory-recorded BBM,
> but it should at least work if the firmware or the boot-loader
> has set up the controller this way.
>
> [B] Override SPARE_AREA_SKIP_BYTES with
> a different value (8).
> This can keep the factory-based BBM, but
> this is very unlikely to work across software stages
> if the NAND device was formatted by the firmware or
> the boot-loader.
>
>
> We need to give up something.
> [A] was the original, 9 years' default, and cleaner.
Or maybe
commit 0d55c668b218a1db68b5044bce4de74e1bd0f0c8
mtd: rawnand: denali: set SPARE_AREA_SKIP_BYTES register to 8 if unset
should be reverted, since it changed the behavior ?
[...]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-12 0:48 ` Marek Vasut
@ 2019-12-12 4:12 ` Masahiro Yamada
2019-12-12 8:28 ` Marek Vasut
0 siblings, 1 reply; 10+ messages in thread
From: Masahiro Yamada @ 2019-12-12 4:12 UTC (permalink / raw)
To: Marek Vasut
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On Thu, Dec 12, 2019 at 10:06 AM Marek Vasut <marex@denx.de> wrote:
>
> On 12/10/19 7:30 AM, Masahiro Yamada wrote:
> > On Tue, Dec 10, 2019 at 12:35 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 12/10/19 4:15 AM, Masahiro Yamada wrote:
> >>> On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
> >>>> [...]
> >>>>
> >>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>>>> index 3102ddbd8abdb..b6c463d021677 100644
> >>>>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
> >>>>>>
> >>>>>> /*
> >>>>>> * Set how many bytes should be skipped before writing data in OOB.
> >>>>>> + * If a non-zero value has already been configured, update it in HW.
> >>>>>> * If a non-zero value has already been set (by firmware or something),
> >>>>>> * just use it. Otherwise, set the driver's default.
> >>>>>> */
> >>>>>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>>>> - if (!denali->oob_skip_bytes) {
> >>>>>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>>>> + if (denali->oob_skip_bytes) {
> >>>>>> iowrite32(denali->oob_skip_bytes,
> >>>>>> denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>>>> + } else {
> >>>>>> + denali->oob_skip_bytes =
> >>>>>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>>>> + if (!denali->oob_skip_bytes) {
> >>>>>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>>>> + iowrite32(denali->oob_skip_bytes,
> >>>>>> + denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>>>
> >>>>> This fallback is ugly, and should be removed, I think.
> >>>>> It is only reachable by PCI platform (Intel MRST), where
> >>>>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
> >>>>
> >>>> This fallback retains the original behavior on all platforms. It might
> >>>> not be to your liking, but it does not break other platforms while
> >>>> fixing SoCFPGA. We don't know what other platforms might be depending on
> >>>> this behavior, do we ?
> >>>
> >>> if (denali->oob_skip_bytes) {
> >>> iowrite32(denali->oob_skip_bytes,
> >>> denali->reg + SPARE_AREA_SKIP_BYTES);
> >>> else
> >>> denali->oob_skip_bytes =
> >>> ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>
> >>>
> >>> ... retains the original behavior.
> >>
> >> It does not, because if the readback in the else branch sets
> >> oob_skip_bytes to 0, the controller is not updated with the default value.
> >>
> >>> For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
> >>> to the well-defined value.
> >>>
> >>> denali_pci.c is the only platform that can read back the
> >>> register value.
> >>>
> >>> See, how Intel originally wrote the code.
> >>>
> >>> https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
> >>>
> >>>
> >>> Please notice the part "if this value is 0, just let it be."
> >>> The Intel MRST platform happily accepts
> >>> SPARE_AREA_SKIP_BYTES being set to 0.
> >>>
> >>> I am not sure how many people are using this platform,
> >>> but anyway it is how it has worked for a long time.
> >>
> >> The intel platform might accept 0 happily, but that's not how the
> >> controller was configured for a long time.
> >
> > It is.
> > It worked like that for 9 years. (i.e. v2.6.35 - v4.19)
>
> So it is broken now ?
I do not know.
As I already said, there is no perfect solution about what
to do when SPARE_AREA_SKIP_BYTES is zero.
I received various feedback from SOCFPGA board users
about this driver, but nothing from denali_pci platfrom users.
Absolutely zero question/complaint.
I suspect there is no user of that platform, but who knows.
> If so, then that fix is for another patch.
So, do you want me to get back the original behavior,
then you want to send a new patch based on that?
I think it is a waste of time, but
it would be less worse than continuing this thread.
> >> So if I were to change the
> >> code as you suggest, it would likely break some setups.
> >
> > There is no perfect solution here
> > when SPARE_AREA_SKIP_BYTES was set to 0
> > before booting the kernel.
> >
> > [A] Keep SPARE_AREA_SKIP_BYTES as it is.
> > This might affect the factory-recorded BBM,
> > but it should at least work if the firmware or the boot-loader
> > has set up the controller this way.
> >
> > [B] Override SPARE_AREA_SKIP_BYTES with
> > a different value (8).
> > This can keep the factory-based BBM, but
> > this is very unlikely to work across software stages
> > if the NAND device was formatted by the firmware or
> > the boot-loader.
> >
> >
> > We need to give up something.
> > [A] was the original, 9 years' default, and cleaner.
>
> Or maybe
>
> commit 0d55c668b218a1db68b5044bce4de74e1bd0f0c8
> mtd: rawnand: denali: set SPARE_AREA_SKIP_BYTES register to 8 if unset
>
> should be reverted, since it changed the behavior ?
>
> [...]
--
Best Regards
Masahiro Yamada
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-12 4:12 ` Masahiro Yamada
@ 2019-12-12 8:28 ` Marek Vasut
0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2019-12-12 8:28 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Ley Foon Tan,
Miquel Raynal
On 12/12/19 5:12 AM, Masahiro Yamada wrote:
[...]
>>>>>>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * Set how many bytes should be skipped before writing data in OOB.
>>>>>>>> + * If a non-zero value has already been configured, update it in HW.
>>>>>>>> * If a non-zero value has already been set (by firmware or something),
>>>>>>>> * just use it. Otherwise, set the driver's default.
>>>>>>>> */
>>>>>>>> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>>>> - if (!denali->oob_skip_bytes) {
>>>>>>>> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>>>>>> + if (denali->oob_skip_bytes) {
>>>>>>>> iowrite32(denali->oob_skip_bytes,
>>>>>>>> denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>>>> + } else {
>>>>>>>> + denali->oob_skip_bytes =
>>>>>>>> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>>>> + if (!denali->oob_skip_bytes) {
>>>>>>>> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
>>>>>>>> + iowrite32(denali->oob_skip_bytes,
>>>>>>>> + denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>>>
>>>>>>> This fallback is ugly, and should be removed, I think.
>>>>>>> It is only reachable by PCI platform (Intel MRST), where
>>>>>>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
>>>>>>
>>>>>> This fallback retains the original behavior on all platforms. It might
>>>>>> not be to your liking, but it does not break other platforms while
>>>>>> fixing SoCFPGA. We don't know what other platforms might be depending on
>>>>>> this behavior, do we ?
>>>>>
>>>>> if (denali->oob_skip_bytes) {
>>>>> iowrite32(denali->oob_skip_bytes,
>>>>> denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>> else
>>>>> denali->oob_skip_bytes =
>>>>> ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
>>>>>
>>>>>
>>>>> ... retains the original behavior.
>>>>
>>>> It does not, because if the readback in the else branch sets
>>>> oob_skip_bytes to 0, the controller is not updated with the default value.
>>>>
>>>>> For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
>>>>> to the well-defined value.
>>>>>
>>>>> denali_pci.c is the only platform that can read back the
>>>>> register value.
>>>>>
>>>>> See, how Intel originally wrote the code.
>>>>>
>>>>> https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
>>>>>
>>>>>
>>>>> Please notice the part "if this value is 0, just let it be."
>>>>> The Intel MRST platform happily accepts
>>>>> SPARE_AREA_SKIP_BYTES being set to 0.
>>>>>
>>>>> I am not sure how many people are using this platform,
>>>>> but anyway it is how it has worked for a long time.
>>>>
>>>> The intel platform might accept 0 happily, but that's not how the
>>>> controller was configured for a long time.
>>>
>>> It is.
>>> It worked like that for 9 years. (i.e. v2.6.35 - v4.19)
>>
>> So it is broken now ?
>
> I do not know.
> As I already said, there is no perfect solution about what
> to do when SPARE_AREA_SKIP_BYTES is zero.
>
> I received various feedback from SOCFPGA board users
> about this driver, but nothing from denali_pci platfrom users.
> Absolutely zero question/complaint.
>
> I suspect there is no user of that platform, but who knows.
>
>
>> If so, then that fix is for another patch.
>
> So, do you want me to get back the original behavior,
> then you want to send a new patch based on that?
I think it's definitely two separate issues.
> I think it is a waste of time, but
> it would be less worse than continuing this thread.
This patch at least retains the current behavior, so if someone wants to
restore the original behavior before 4.19, it can be even done on top of
this patch.
[...]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES
2019-12-07 0:48 [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES Marek Vasut
2019-12-09 5:38 ` Masahiro Yamada
@ 2019-12-20 8:50 ` Tudor.Ambarus
1 sibling, 0 replies; 10+ messages in thread
From: Tudor.Ambarus @ 2019-12-20 8:50 UTC (permalink / raw)
To: marex, linux-mtd
Cc: richard, miquel.raynal, vigneshr, ley.foon.tan, yamada.masahiro
Hi, Marek, Masahiro,
On 12/7/19 2:48 AM, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The SPARE_AREA_SKIP_BYTES register is reset when the controller reset
> signal is toggled. Yet, this register must be configured to match the
> content of the NAND OOB area. The current default value is always set
> to 8 and is programmed into the hardware in case the hardware was not
> programmed before (e.g. in a bootloader) with a different value. This
> however does not work when the block is reset properly by Linux.
>
> On Altera SoCFPGA CycloneV, ArriaV and Arria10, which are the SoCFPGA
> platforms which support booting from NAND, the SPARE_AREA_SKIP_BYTES
> value must be set to 2. On Socionext Uniphier, the value is 8. This
> patch adds support for preconfiguring the default value and handles
> the special SoCFPGA case by setting the default to 2 on all SoCFPGA
> platforms, while retaining the original behavior and default value of
> 8 on all the other platforms.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Ley Foon Tan <ley.foon.tan@intel.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> To: linux-mtd@lists.infradead.org
> ---
> drivers/mtd/nand/raw/denali.c | 13 ++++++++++---
> drivers/mtd/nand/raw/denali_dt.c | 6 ++++++
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index 3102ddbd8abdb..b6c463d021677 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
>
> /*
> * Set how many bytes should be skipped before writing data in OOB.
> + * If a non-zero value has already been configured, update it in HW.
> * If a non-zero value has already been set (by firmware or something),
> * just use it. Otherwise, set the driver's default.
> */
> - denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> - if (!denali->oob_skip_bytes) {
> - denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> + if (denali->oob_skip_bytes) {
> iowrite32(denali->oob_skip_bytes,
> denali->reg + SPARE_AREA_SKIP_BYTES);
> + } else {
> + denali->oob_skip_bytes =
> + ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> + if (!denali->oob_skip_bytes) {
> + denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> + iowrite32(denali->oob_skip_bytes,
> + denali->reg + SPARE_AREA_SKIP_BYTES);
> + }
> }
>
> iowrite32(0, denali->reg + TRANSFER_SPARE_REG);
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index 5e14836f6bd5a..34c7c553f3412 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -27,6 +27,7 @@ struct denali_dt {
> struct denali_dt_data {
> unsigned int revision;
> unsigned int caps;
> + unsigned int oob_skip_bytes;
> const struct nand_ecc_caps *ecc_caps;
> };
>
> @@ -34,6 +35,7 @@ NAND_ECC_CAPS_SINGLE(denali_socfpga_ecc_caps, denali_calc_ecc_bytes,
> 512, 8, 15);
> static const struct denali_dt_data denali_socfpga_data = {
> .caps = DENALI_CAP_HW_ECC_FIXUP,
> + .oob_skip_bytes = 2,
> .ecc_caps = &denali_socfpga_ecc_caps,
> };
>
> @@ -42,6 +44,7 @@ NAND_ECC_CAPS_SINGLE(denali_uniphier_v5a_ecc_caps, denali_calc_ecc_bytes,
> static const struct denali_dt_data denali_uniphier_v5a_data = {
> .caps = DENALI_CAP_HW_ECC_FIXUP |
> DENALI_CAP_DMA_64BIT,
> + .oob_skip_bytes = 8,
> .ecc_caps = &denali_uniphier_v5a_ecc_caps,
> };
>
> @@ -51,6 +54,7 @@ static const struct denali_dt_data denali_uniphier_v5b_data = {
> .revision = 0x0501,
> .caps = DENALI_CAP_HW_ECC_FIXUP |
> DENALI_CAP_DMA_64BIT,
> + .oob_skip_bytes = 8,
> .ecc_caps = &denali_uniphier_v5b_ecc_caps,
> };
>
> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
> denali->clk_rate = clk_get_rate(dt->clk);
> denali->clk_x_rate = clk_get_rate(dt->clk_x);
>
> + denali->oob_skip_bytes = data->oob_skip_bytes;
> +
This indeed should be moved immediately after getting of_device_get_match_data.
I share Marek's opinion that what Masahiro have identified worths a separate
patch. Ideally we should fix one thing per patch. With this moved immediately
after of_device_get_match_data() one can add:
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-12-20 8:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-07 0:48 [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES Marek Vasut
2019-12-09 5:38 ` Masahiro Yamada
2019-12-09 12:53 ` Marek Vasut
2019-12-10 3:15 ` Masahiro Yamada
2019-12-10 3:32 ` Marek Vasut
2019-12-10 6:30 ` Masahiro Yamada
2019-12-12 0:48 ` Marek Vasut
2019-12-12 4:12 ` Masahiro Yamada
2019-12-12 8:28 ` Marek Vasut
2019-12-20 8:50 ` Tudor.Ambarus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.