[PATCH 0/7] mtd: rawnand: sunxi: Bug fixes and cleanup

[PATCH 0/7] mtd: rawnand: sunxi: Bug fixes and cleanup

[Samuel Holland]

I have an A33 tablet with MLC NAND, and I wanted to use mainline Linux
to dump the NAND. To do that, I updated this driver's ECC ops to fully
utilize the hardware for ECC and scrambling. This made the driver
compatible with the existing (scrambled) on-flash bad block map, and I
was able to read the full NAND contents.

This series contains some initial bug fixes and cleanup from that
effort. The changes to the ECC ops will come as a separate series.


Samuel Holland (7):
  mtd: rawnand: sunxi: Clean up chips after failed init
  mtd: rawnand: sunxi: Remove an unnecessary check
  mtd: rawnand: sunxi: Remove an unnecessary check
  mtd: rawnand: sunxi: Fix ECC strength maximization
  mtd: rawnand: sunxi: Fix the size of the last OOB region
  mtd: rawnand: sunxi: Update OOB layout to match hardware
  mtd: rawnand: sunxi: Precompute the ECC_CTL register value

 drivers/mtd/nand/raw/sunxi_nand.c | 132 ++++++++++--------------------
 1 file changed, 42 insertions(+), 90 deletions(-)

-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/7] mtd: rawnand: sunxi: Clean up chips after failed init

[Samuel Holland]

If a chip fails to initialize, we need to clean up any chips that were
already initialized/registered.

Fixes: 1fef62c1423b ("mtd: nand: add sunxi NAND flash controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 39 ++++++++++++++++---------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index ea953e31933e..2ee86f7b0905 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -1950,6 +1950,25 @@ static const struct nand_controller_ops sunxi_nand_controller_ops = {
 	.exec_op = sunxi_nfc_exec_op,
 };
 
+static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
+{
+	struct sunxi_nand_chip *sunxi_nand;
+	struct nand_chip *chip;
+	int ret;
+
+	while (!list_empty(&nfc->chips)) {
+		sunxi_nand = list_first_entry(&nfc->chips,
+					      struct sunxi_nand_chip,
+					      node);
+		chip = &sunxi_nand->nand;
+		ret = mtd_device_unregister(nand_to_mtd(chip));
+		WARN_ON(ret);
+		nand_cleanup(chip);
+		sunxi_nand_ecc_cleanup(sunxi_nand);
+		list_del(&sunxi_nand->node);
+	}
+}
+
 static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 				struct device_node *np)
 {
@@ -2053,6 +2072,7 @@ static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
 		ret = sunxi_nand_chip_init(dev, nfc, nand_np);
 		if (ret) {
 			of_node_put(nand_np);
+			sunxi_nand_chips_cleanup(nfc);
 			return ret;
 		}
 	}
@@ -2060,25 +2080,6 @@ static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
 	return 0;
 }
 
-static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
-{
-	struct sunxi_nand_chip *sunxi_nand;
-	struct nand_chip *chip;
-	int ret;
-
-	while (!list_empty(&nfc->chips)) {
-		sunxi_nand = list_first_entry(&nfc->chips,
-					      struct sunxi_nand_chip,
-					      node);
-		chip = &sunxi_nand->nand;
-		ret = mtd_device_unregister(nand_to_mtd(chip));
-		WARN_ON(ret);
-		nand_cleanup(chip);
-		sunxi_nand_ecc_cleanup(sunxi_nand);
-		list_del(&sunxi_nand->node);
-	}
-}
-
 static int sunxi_nfc_dma_init(struct sunxi_nfc *nfc, struct resource *r)
 {
 	int ret;
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] mtd: rawnand: sunxi: Clean up chips after failed init

[Miquel Raynal]

On Thu, 2022-12-29 at 18:15:20 UTC, Samuel Holland wrote:
> If a chip fails to initialize, we need to clean up any chips that were
> already initialized/registered.
> 
> Fixes: 1fef62c1423b ("mtd: nand: add sunxi NAND flash controller support")
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/7] mtd: rawnand: sunxi: Remove an unnecessary check

[Samuel Holland]

sunxi_nand->nsels cannot be zero, so the second check implies the first.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index 2ee86f7b0905..8b221f9f10a7 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -421,7 +421,7 @@ static void sunxi_nfc_select_chip(struct nand_chip *nand, unsigned int cs)
 	struct sunxi_nand_chip_sel *sel;
 	u32 ctl;
 
-	if (cs > 0 && cs >= sunxi_nand->nsels)
+	if (cs >= sunxi_nand->nsels)
 		return;
 
 	ctl = readl(nfc->regs + NFC_REG_CTL) &
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] mtd: rawnand: sunxi: Remove an unnecessary check

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:21 -0600:

> sunxi_nand->nsels cannot be zero, so the second check implies the first.

Actually this check comes from a time where we did check against -1 :)
But, yeah, now it's a duplicate.

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 2ee86f7b0905..8b221f9f10a7 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -421,7 +421,7 @@ static void sunxi_nfc_select_chip(struct nand_chip *nand, unsigned int cs)
>  	struct sunxi_nand_chip_sel *sel;
>  	u32 ctl;
>  
> -	if (cs > 0 && cs >= sunxi_nand->nsels)
> +	if (cs >= sunxi_nand->nsels)
>  		return;
>  
>  	ctl = readl(nfc->regs + NFC_REG_CTL) &


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] mtd: rawnand: sunxi: Remove an unnecessary check

[Miquel Raynal]

On Thu, 2022-12-29 at 18:15:21 UTC, Samuel Holland wrote:
> sunxi_nand->nsels cannot be zero, so the second check implies the first.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/7] mtd: rawnand: sunxi: Remove an unnecessary check

[Samuel Holland]

Each chip is required to have a unique CS number ("reg" property) in the
range 0-7, so there is no need to separately count the number of chips.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index 8b221f9f10a7..1bddeb1be66f 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -2060,14 +2060,8 @@ static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
 {
 	struct device_node *np = dev->of_node;
 	struct device_node *nand_np;
-	int nchips = of_get_child_count(np);
 	int ret;
 
-	if (nchips > 8) {
-		dev_err(dev, "too many NAND chips: %d (max = 8)\n", nchips);
-		return -EINVAL;
-	}
-
 	for_each_child_of_node(np, nand_np) {
 		ret = sunxi_nand_chip_init(dev, nfc, nand_np);
 		if (ret) {
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] mtd: rawnand: sunxi: Remove an unnecessary check

[Miquel Raynal]

On Thu, 2022-12-29 at 18:15:22 UTC, Samuel Holland wrote:
> Each chip is required to have a unique CS number ("reg" property) in the
> range 0-7, so there is no need to separately count the number of chips.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Samuel Holland]

This is already accounted for in the subtraction for OOB, since the BBM
overlaps the first OOB dword. With this change, the driver picks the
same ECC strength as the vendor driver.

Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index 1bddeb1be66f..1ecf2cee343b 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 		ecc->size = 1024;
 		nsectors = mtd->writesize / ecc->size;
 
-		/* Reserve 2 bytes for the BBM */
-		bytes = (mtd->oobsize - 2) / nsectors;
+		bytes = mtd->oobsize / nsectors;
 
 		/* 4 non-ECC bytes are added before each ECC bytes section */
 		bytes -= 4;
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:

> This is already accounted for in the subtraction for OOB, since the BBM
> overlaps the first OOB dword. With this change, the driver picks the
> same ECC strength as the vendor driver.
> 
> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 1bddeb1be66f..1ecf2cee343b 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  		ecc->size = 1024;
>  		nsectors = mtd->writesize / ecc->size;
>  
> -		/* Reserve 2 bytes for the BBM */
> -		bytes = (mtd->oobsize - 2) / nsectors;
> +		bytes = mtd->oobsize / nsectors;

I'm sorry but I don't think we can make this work. This change would
break all existing users...

>  
>  		/* 4 non-ECC bytes are added before each ECC bytes section */
>  		bytes -= 4;


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Samuel Holland]

Hi Miquèl,

On 1/2/23 03:11, Miquel Raynal wrote:
> Hi Samuel,
> 
> samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:
> 
>> This is already accounted for in the subtraction for OOB, since the BBM
>> overlaps the first OOB dword. With this change, the driver picks the
>> same ECC strength as the vendor driver.
>>
>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>> index 1bddeb1be66f..1ecf2cee343b 100644
>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  		ecc->size = 1024;
>>  		nsectors = mtd->writesize / ecc->size;
>>  
>> -		/* Reserve 2 bytes for the BBM */
>> -		bytes = (mtd->oobsize - 2) / nsectors;
>> +		bytes = mtd->oobsize / nsectors;
> 
> I'm sorry but I don't think we can make this work. This change would
> break all existing users...

OK, it is not too much of an issue because I can manually specify the
ECC parameters in the devicetree. Do you think it makes sense to fix
this when adding new hardware variants/compatible strings?

Regards,
Samuel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Mon, 2 Jan 2023 09:59:29 -0600:

> Hi Miquèl,
> 
> On 1/2/23 03:11, Miquel Raynal wrote:
> > Hi Samuel,
> > 
> > samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:23 -0600:
> >   
> >> This is already accounted for in the subtraction for OOB, since the BBM
> >> overlaps the first OOB dword. With this change, the driver picks the
> >> same ECC strength as the vendor driver.
> >>
> >> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >> index 1bddeb1be66f..1ecf2cee343b 100644
> >> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> >>  		ecc->size = 1024;
> >>  		nsectors = mtd->writesize / ecc->size;
> >>  
> >> -		/* Reserve 2 bytes for the BBM */
> >> -		bytes = (mtd->oobsize - 2) / nsectors;
> >> +		bytes = mtd->oobsize / nsectors;  
> > 
> > I'm sorry but I don't think we can make this work. This change would
> > break all existing users...  
> 
> OK, it is not too much of an issue because I can manually specify the
> ECC parameters in the devicetree. Do you think it makes sense to fix
> this when adding new hardware variants/compatible strings?

Actually, looking at the code again, I don't get how the above diff
could be valid. The "maximize strength" logic (in which this diff is)
looks for the biggest region to store ECC bytes. These bytes cannot
be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
cannot get rid of this.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Samuel Holland]

Hi Miquèl,

On 1/2/23 10:45, Miquel Raynal wrote:
>>>> This is already accounted for in the subtraction for OOB, since the BBM
>>>> overlaps the first OOB dword. With this change, the driver picks the
>>>> same ECC strength as the vendor driver.
>>>>
>>>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>
>>>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>>>> index 1bddeb1be66f..1ecf2cee343b 100644
>>>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>>>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>>>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>>>  		ecc->size = 1024;
>>>>  		nsectors = mtd->writesize / ecc->size;
>>>>  
>>>> -		/* Reserve 2 bytes for the BBM */
>>>> -		bytes = (mtd->oobsize - 2) / nsectors;
>>>> +		bytes = mtd->oobsize / nsectors;  
>>>
>>> I'm sorry but I don't think we can make this work. This change would
>>> break all existing users...  
>>
>> OK, it is not too much of an issue because I can manually specify the
>> ECC parameters in the devicetree. Do you think it makes sense to fix
>> this when adding new hardware variants/compatible strings?
> 
> Actually, looking at the code again, I don't get how the above diff
> could be valid. The "maximize strength" logic (in which this diff is)
> looks for the biggest region to store ECC bytes. These bytes cannot
> be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
> cannot get rid of this.

Right, we cannot overlap the BBM, but the BBM is accounted for in the
line below:

  /* 4 non-ECC bytes are added before each ECC bytes section */
  bytes -= 4;

Normally those 4 bytes are all free OOB, but for the first ECC step,
those are split into 2 free bytes and 2 BBM bytes:

  /*
   * The first 2 bytes are used for BB markers, hence we
   * only have 2 bytes available in the first user data
   * section.
   */
  if (!section && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
          oobregion->offset = 2;
          oobregion->length = 2;

          return 0;
  }

So if we subtract 4 bytes for the each free OOB area, including the
first one, and also subtract 2 bytes for the BBM, we are double-counting
the BBM. I should have made my commit message clearer. But I am going to
drop this patch anyway.

Regards,
Samuel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] mtd: rawnand: sunxi: Fix ECC strength maximization

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Mon, 2 Jan 2023 11:06:20 -0600:

> Hi Miquèl,
> 
> On 1/2/23 10:45, Miquel Raynal wrote:
> >>>> This is already accounted for in the subtraction for OOB, since the BBM
> >>>> overlaps the first OOB dword. With this change, the driver picks the
> >>>> same ECC strength as the vendor driver.
> >>>>
> >>>> Fixes: 4796d8655915 ("mtd: nand: sunxi: Support ECC maximization")
> >>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >>>> ---
> >>>>
> >>>>  drivers/mtd/nand/raw/sunxi_nand.c | 3 +--
> >>>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> index 1bddeb1be66f..1ecf2cee343b 100644
> >>>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >>>> @@ -1643,8 +1643,7 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> >>>>  		ecc->size = 1024;
> >>>>  		nsectors = mtd->writesize / ecc->size;
> >>>>  
> >>>> -		/* Reserve 2 bytes for the BBM */
> >>>> -		bytes = (mtd->oobsize - 2) / nsectors;
> >>>> +		bytes = mtd->oobsize / nsectors;    
> >>>
> >>> I'm sorry but I don't think we can make this work. This change would
> >>> break all existing users...    
> >>
> >> OK, it is not too much of an issue because I can manually specify the
> >> ECC parameters in the devicetree. Do you think it makes sense to fix
> >> this when adding new hardware variants/compatible strings?  
> > 
> > Actually, looking at the code again, I don't get how the above diff
> > could be valid. The "maximize strength" logic (in which this diff is)
> > looks for the biggest region to store ECC bytes. These bytes cannot
> > be stored on the BBM, which "mtd->oobsize - 2" tries to avoid, so we
> > cannot get rid of this.  
> 
> Right, we cannot overlap the BBM, but the BBM is accounted for in the
> line below:
> 
>   /* 4 non-ECC bytes are added before each ECC bytes section */
>   bytes -= 4;
> 
> Normally those 4 bytes are all free OOB, but for the first ECC step,
> those are split into 2 free bytes and 2 BBM bytes:
> 
>   /*
>    * The first 2 bytes are used for BB markers, hence we
>    * only have 2 bytes available in the first user data
>    * section.
>    */
>   if (!section && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
>           oobregion->offset = 2;
>           oobregion->length = 2;
> 
>           return 0;
>   }
> 
> So if we subtract 4 bytes for the each free OOB area, including the
> first one, and also subtract 2 bytes for the BBM, we are double-counting
> the BBM. I should have made my commit message clearer. But I am going to
> drop this patch anyway.

Ah, yes, you are absolutely right, then.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/7] mtd: rawnand: sunxi: Fix the size of the last OOB region

[Samuel Holland]

The previous code assigned to the wrong structure member.

Fixes: c66811e6d350 ("mtd: nand: sunxi: switch to mtd_ooblayout_ops")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index 1ecf2cee343b..8e873f4fec9a 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -1609,7 +1609,7 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
 	if (section < ecc->steps)
 		oobregion->length = 4;
 	else
-		oobregion->offset = mtd->oobsize - oobregion->offset;
+		oobregion->length = mtd->oobsize - oobregion->offset;
 
 	return 0;
 }
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] mtd: rawnand: sunxi: Fix the size of the last OOB region

[Gole, Dhruva]

On 12/29/2022 11:45 PM, Samuel Holland wrote:
> The previous code assigned to the wrong structure member.
Good catch!
>
> Fixes: c66811e6d350 ("mtd: nand: sunxi: switch to mtd_ooblayout_ops")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
Acked-By: Dhruva Gole <d-gole@ti.com>
>
>   drivers/mtd/nand/raw/sunxi_nand.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 1ecf2cee343b..8e873f4fec9a 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -1609,7 +1609,7 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
>   	if (section < ecc->steps)
>   		oobregion->length = 4;
>   	else
> -		oobregion->offset = mtd->oobsize - oobregion->offset;
> +		oobregion->length = mtd->oobsize - oobregion->offset;
>   
>   	return 0;
>   }

-- 
Regards,
Dhruva Gole <d-gole@ti.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] mtd: rawnand: sunxi: Fix the size of the last OOB region

[Miquel Raynal]

On Thu, 2022-12-29 at 18:15:24 UTC, Samuel Holland wrote:
> The previous code assigned to the wrong structure member.
> 
> Fixes: c66811e6d350 ("mtd: nand: sunxi: switch to mtd_ooblayout_ops")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> Acked-By: Dhruva Gole <d-gole@ti.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/7] mtd: rawnand: sunxi: Update OOB layout to match hardware

[Samuel Holland]

When using the hardware ECC engine, the OOB data is made available in
the NFC_REG_USER_DATA registers, one 32-bit word per ECC step. Any
additional bytes are only accessible through raw reads and software
descrambling. For efficiency, and to match the vendor driver, ignore
these extra bytes.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index 8e873f4fec9a..a3bc9f7f9e5a 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -1604,6 +1604,13 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
 		return 0;
 	}
 
+	/*
+	 * The controller does not provide access to OOB bytes
+	 * past the end of the ECC data.
+	 */
+	if (section == ecc->steps && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
+		return -ERANGE;
+
 	oobregion->offset = section * (ecc->bytes + 4);
 
 	if (section < ecc->steps)
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/7] mtd: rawnand: sunxi: Update OOB layout to match hardware

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:25 -0600:

> When using the hardware ECC engine, the OOB data is made available in
> the NFC_REG_USER_DATA registers, one 32-bit word per ECC step. Any
> additional bytes are only accessible through raw reads and software
> descrambling. For efficiency, and to match the vendor driver, ignore
> these extra bytes.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 8e873f4fec9a..a3bc9f7f9e5a 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -1604,6 +1604,13 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
>  		return 0;
>  	}
>  
> +	/*
> +	 * The controller does not provide access to OOB bytes
> +	 * past the end of the ECC data.
> +	 */
> +	if (section == ecc->steps && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> +		return -ERANGE;

Again, I am sorry but I cannot take this change, it would typically
break jffs2 users (if any?) :(

>  	oobregion->offset = section * (ecc->bytes + 4);
>  
>  	if (section < ecc->steps)


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/7] mtd: rawnand: sunxi: Update OOB layout to match hardware

[Samuel Holland]

Hi Miquèl,

On 1/2/23 03:21, Miquel Raynal wrote:
> Hi Samuel,
> 
> samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:25 -0600:
> 
>> When using the hardware ECC engine, the OOB data is made available in
>> the NFC_REG_USER_DATA registers, one 32-bit word per ECC step. Any
>> additional bytes are only accessible through raw reads and software
>> descrambling. For efficiency, and to match the vendor driver, ignore
>> these extra bytes.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/mtd/nand/raw/sunxi_nand.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>> index 8e873f4fec9a..a3bc9f7f9e5a 100644
>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>> @@ -1604,6 +1604,13 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
>>  		return 0;
>>  	}
>>  
>> +	/*
>> +	 * The controller does not provide access to OOB bytes
>> +	 * past the end of the ECC data.
>> +	 */
>> +	if (section == ecc->steps && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
>> +		return -ERANGE;
> 
> Again, I am sorry but I cannot take this change, it would typically
> break jffs2 users (if any?) :(

Considering the bug I fixed in the previous patch, and the fact that
mtd_ooblayout_free() zeroes out the structure before calling the .free
callback, that region was being reported with a length of zero already.
So I don't think anyone could have been using those bytes anyway.

I am looking for a solution here because the ECC/scrambling engine
really provides no way to access these bytes. Reading them requires
turning off the ECC engine, performing another read command, and then
descrambling in software. So we are sort of lying when we claim those
bytes are available with hardware ECC enabled.

If this change cannot be made as-is, is there any way the user could opt
in to the new layout, to get the improved performance?

Regards,
Samuel

>>  	oobregion->offset = section * (ecc->bytes + 4);
>>  
>>  	if (section < ecc->steps)
> 
> 
> Thanks,
> Miquèl


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/7] mtd: rawnand: sunxi: Update OOB layout to match hardware

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Mon, 2 Jan 2023 10:26:48 -0600:

> Hi Miquèl,
> 
> On 1/2/23 03:21, Miquel Raynal wrote:
> > Hi Samuel,
> > 
> > samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:25 -0600:
> >   
> >> When using the hardware ECC engine, the OOB data is made available in
> >> the NFC_REG_USER_DATA registers, one 32-bit word per ECC step. Any
> >> additional bytes are only accessible through raw reads and software
> >> descrambling. For efficiency, and to match the vendor driver, ignore
> >> these extra bytes.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >>  drivers/mtd/nand/raw/sunxi_nand.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >> index 8e873f4fec9a..a3bc9f7f9e5a 100644
> >> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >> @@ -1604,6 +1604,13 @@ static int sunxi_nand_ooblayout_free(struct mtd_info *mtd, int section,
> >>  		return 0;
> >>  	}
> >>  
> >> +	/*
> >> +	 * The controller does not provide access to OOB bytes
> >> +	 * past the end of the ECC data.
> >> +	 */
> >> +	if (section == ecc->steps && ecc->engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> >> +		return -ERANGE;  
> > 
> > Again, I am sorry but I cannot take this change, it would typically
> > break jffs2 users (if any?) :(  
> 
> Considering the bug I fixed in the previous patch, and the fact that
> mtd_ooblayout_free() zeroes out the structure before calling the .free
> callback, that region was being reported with a length of zero already.
> So I don't think anyone could have been using those bytes anyway.
> 
> I am looking for a solution here because the ECC/scrambling engine
> really provides no way to access these bytes. Reading them requires
> turning off the ECC engine, performing another read command, and then
> descrambling in software. So we are sort of lying when we claim those
> bytes are available with hardware ECC enabled.
> 
> If this change cannot be made as-is, is there any way the user could opt
> in to the new layout, to get the improved performance?

Actually that's true, you fixed the reporting of the free area which
was set to 0 until then, which means there cannot be any upstream user.
So knowing that, preventing the accesses to the end of the area seems
acceptable when using HW ECC. Please mention it in the commit log.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/7] mtd: rawnand: sunxi: Precompute the ECC_CTL register value

[Samuel Holland]

This removes an unnecessary memory allocation, and avoids recomputing
the same register value every time ECC is enabled.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/mtd/nand/raw/sunxi_nand.c | 75 ++++++-------------------------
 1 file changed, 13 insertions(+), 62 deletions(-)

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index a3bc9f7f9e5a..5c5a567d8870 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -169,22 +169,13 @@ struct sunxi_nand_chip_sel {
 	s8 rb;
 };
 
-/**
- * struct sunxi_nand_hw_ecc - stores information related to HW ECC support
- *
- * @mode: the sunxi ECC mode field deduced from ECC requirements
- */
-struct sunxi_nand_hw_ecc {
-	int mode;
-};
-
 /**
  * struct sunxi_nand_chip - stores NAND chip device related information
  *
  * @node: used to store NAND chips into a list
  * @nand: base NAND chip structure
- * @ecc: ECC controller structure
  * @clk_rate: clk_rate required for this NAND chip
+ * @ecc_ctl: ECC_CTL register value for this NAND chip
  * @timing_cfg: TIMING_CFG register value for this NAND chip
  * @timing_ctl: TIMING_CTL register value for this NAND chip
  * @nsels: number of CS lines required by the NAND chip
@@ -193,8 +184,8 @@ struct sunxi_nand_hw_ecc {
 struct sunxi_nand_chip {
 	struct list_head node;
 	struct nand_chip nand;
-	struct sunxi_nand_hw_ecc *ecc;
 	unsigned long clk_rate;
+	u32 ecc_ctl;
 	u32 timing_cfg;
 	u32 timing_ctl;
 	int nsels;
@@ -689,26 +680,15 @@ static void sunxi_nfc_hw_ecc_enable(struct nand_chip *nand)
 {
 	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
 	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
-	u32 ecc_ctl;
-
-	ecc_ctl = readl(nfc->regs + NFC_REG_ECC_CTL);
-	ecc_ctl &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE |
-		     NFC_ECC_BLOCK_SIZE_MSK);
-	ecc_ctl |= NFC_ECC_EN | NFC_ECC_MODE(sunxi_nand->ecc->mode) |
-		   NFC_ECC_EXCEPTION | NFC_ECC_PIPELINE;
-
-	if (nand->ecc.size == 512)
-		ecc_ctl |= NFC_ECC_BLOCK_512;
 
-	writel(ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
+	writel(sunxi_nand->ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
 }
 
 static void sunxi_nfc_hw_ecc_disable(struct nand_chip *nand)
 {
 	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
 
-	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
-	       nfc->regs + NFC_REG_ECC_CTL);
+	writel(0, nfc->regs + NFC_REG_ECC_CTL);
 }
 
 static inline void sunxi_nfc_user_data_to_buf(u32 user_data, u8 *buf)
@@ -1626,11 +1606,6 @@ static const struct mtd_ooblayout_ops sunxi_nand_ooblayout_ops = {
 	.free = sunxi_nand_ooblayout_free,
 };
 
-static void sunxi_nand_hw_ecc_ctrl_cleanup(struct sunxi_nand_chip *sunxi_nand)
-{
-	kfree(sunxi_nand->ecc);
-}
-
 static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 				       struct nand_ecc_ctrl *ecc,
 				       struct device_node *np)
@@ -1641,7 +1616,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 	struct mtd_info *mtd = nand_to_mtd(nand);
 	struct nand_device *nanddev = mtd_to_nanddev(mtd);
 	int nsectors;
-	int ret;
 	int i;
 
 	if (nanddev->ecc.user_conf.flags & NAND_ECC_MAXIMIZE_STRENGTH) {
@@ -1675,10 +1649,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 	if (ecc->size != 512 && ecc->size != 1024)
 		return -EINVAL;
 
-	sunxi_nand->ecc = kzalloc(sizeof(*sunxi_nand->ecc), GFP_KERNEL);
-	if (!sunxi_nand->ecc)
-		return -ENOMEM;
-
 	/* Prefer 1k ECC chunk over 512 ones */
 	if (ecc->size == 512 && mtd->writesize > 512) {
 		ecc->size = 1024;
@@ -1699,12 +1669,9 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 
 	if (i >= ARRAY_SIZE(strengths)) {
 		dev_err(nfc->dev, "unsupported strength\n");
-		ret = -ENOTSUPP;
-		goto err;
+		return -ENOTSUPP;
 	}
 
-	sunxi_nand->ecc->mode = i;
-
 	/* HW ECC always request ECC bytes for 1024 bytes blocks */
 	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
 
@@ -1713,10 +1680,14 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 
 	nsectors = mtd->writesize / ecc->size;
 
-	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
-		ret = -EINVAL;
-		goto err;
-	}
+	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors))
+		return -EINVAL;
+
+	sunxi_nand->ecc_ctl = NFC_ECC_MODE(i) | NFC_ECC_EXCEPTION |
+			      NFC_ECC_PIPELINE | NFC_ECC_EN;
+
+	if (ecc->size == 512)
+		sunxi_nand->ecc_ctl |= NFC_ECC_BLOCK_512;
 
 	ecc->read_oob = sunxi_nfc_hw_ecc_read_oob;
 	ecc->write_oob = sunxi_nfc_hw_ecc_write_oob;
@@ -1739,25 +1710,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
 	ecc->write_oob_raw = nand_write_oob_std;
 
 	return 0;
-
-err:
-	kfree(sunxi_nand->ecc);
-
-	return ret;
-}
-
-static void sunxi_nand_ecc_cleanup(struct sunxi_nand_chip *sunxi_nand)
-{
-	struct nand_ecc_ctrl *ecc = &sunxi_nand->nand.ecc;
-
-	switch (ecc->engine_type) {
-	case NAND_ECC_ENGINE_TYPE_ON_HOST:
-		sunxi_nand_hw_ecc_ctrl_cleanup(sunxi_nand);
-		break;
-	case NAND_ECC_ENGINE_TYPE_NONE:
-	default:
-		break;
-	}
 }
 
 static int sunxi_nand_attach_chip(struct nand_chip *nand)
@@ -1970,7 +1922,6 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
 		ret = mtd_device_unregister(nand_to_mtd(chip));
 		WARN_ON(ret);
 		nand_cleanup(chip);
-		sunxi_nand_ecc_cleanup(sunxi_nand);
 		list_del(&sunxi_nand->node);
 	}
 }
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] mtd: rawnand: sunxi: Precompute the ECC_CTL register value

[Miquel Raynal]

Hi Samuel,

samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:26 -0600:

> This removes an unnecessary memory allocation, and avoids recomputing
> the same register value every time ECC is enabled.

I am fine with the "let's not recompute the register value each time"
idea, but I like having a dedicated object reserved for the ECC
engine, that is separated from the main controller structure (both
are two different things, even though they are usually well
integrated).

If it's actually useless you can still get rid of the allocation and in
the structure you can save the ecc_ctrl reg value instead of mode.

The other patches in the series look good to me.

Thanks,
Miquèl

> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 75 ++++++-------------------------
>  1 file changed, 13 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index a3bc9f7f9e5a..5c5a567d8870 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -169,22 +169,13 @@ struct sunxi_nand_chip_sel {
>  	s8 rb;
>  };
>  
> -/**
> - * struct sunxi_nand_hw_ecc - stores information related to HW ECC support
> - *
> - * @mode: the sunxi ECC mode field deduced from ECC requirements
> - */
> -struct sunxi_nand_hw_ecc {
> -	int mode;
> -};
> -
>  /**
>   * struct sunxi_nand_chip - stores NAND chip device related information
>   *
>   * @node: used to store NAND chips into a list
>   * @nand: base NAND chip structure
> - * @ecc: ECC controller structure
>   * @clk_rate: clk_rate required for this NAND chip
> + * @ecc_ctl: ECC_CTL register value for this NAND chip
>   * @timing_cfg: TIMING_CFG register value for this NAND chip
>   * @timing_ctl: TIMING_CTL register value for this NAND chip
>   * @nsels: number of CS lines required by the NAND chip
> @@ -193,8 +184,8 @@ struct sunxi_nand_hw_ecc {
>  struct sunxi_nand_chip {
>  	struct list_head node;
>  	struct nand_chip nand;
> -	struct sunxi_nand_hw_ecc *ecc;
>  	unsigned long clk_rate;
> +	u32 ecc_ctl;
>  	u32 timing_cfg;
>  	u32 timing_ctl;
>  	int nsels;
> @@ -689,26 +680,15 @@ static void sunxi_nfc_hw_ecc_enable(struct nand_chip *nand)
>  {
>  	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
> -	u32 ecc_ctl;
> -
> -	ecc_ctl = readl(nfc->regs + NFC_REG_ECC_CTL);
> -	ecc_ctl &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE |
> -		     NFC_ECC_BLOCK_SIZE_MSK);
> -	ecc_ctl |= NFC_ECC_EN | NFC_ECC_MODE(sunxi_nand->ecc->mode) |
> -		   NFC_ECC_EXCEPTION | NFC_ECC_PIPELINE;
> -
> -	if (nand->ecc.size == 512)
> -		ecc_ctl |= NFC_ECC_BLOCK_512;
>  
> -	writel(ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
> +	writel(sunxi_nand->ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
>  }
>  
>  static void sunxi_nfc_hw_ecc_disable(struct nand_chip *nand)
>  {
>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
>  
> -	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
> -	       nfc->regs + NFC_REG_ECC_CTL);
> +	writel(0, nfc->regs + NFC_REG_ECC_CTL);
>  }
>  
>  static inline void sunxi_nfc_user_data_to_buf(u32 user_data, u8 *buf)
> @@ -1626,11 +1606,6 @@ static const struct mtd_ooblayout_ops sunxi_nand_ooblayout_ops = {
>  	.free = sunxi_nand_ooblayout_free,
>  };
>  
> -static void sunxi_nand_hw_ecc_ctrl_cleanup(struct sunxi_nand_chip *sunxi_nand)
> -{
> -	kfree(sunxi_nand->ecc);
> -}
> -
>  static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  				       struct nand_ecc_ctrl *ecc,
>  				       struct device_node *np)
> @@ -1641,7 +1616,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	struct mtd_info *mtd = nand_to_mtd(nand);
>  	struct nand_device *nanddev = mtd_to_nanddev(mtd);
>  	int nsectors;
> -	int ret;
>  	int i;
>  
>  	if (nanddev->ecc.user_conf.flags & NAND_ECC_MAXIMIZE_STRENGTH) {
> @@ -1675,10 +1649,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	if (ecc->size != 512 && ecc->size != 1024)
>  		return -EINVAL;
>  
> -	sunxi_nand->ecc = kzalloc(sizeof(*sunxi_nand->ecc), GFP_KERNEL);
> -	if (!sunxi_nand->ecc)
> -		return -ENOMEM;
> -
>  	/* Prefer 1k ECC chunk over 512 ones */
>  	if (ecc->size == 512 && mtd->writesize > 512) {
>  		ecc->size = 1024;
> @@ -1699,12 +1669,9 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  
>  	if (i >= ARRAY_SIZE(strengths)) {
>  		dev_err(nfc->dev, "unsupported strength\n");
> -		ret = -ENOTSUPP;
> -		goto err;
> +		return -ENOTSUPP;
>  	}
>  
> -	sunxi_nand->ecc->mode = i;
> -
>  	/* HW ECC always request ECC bytes for 1024 bytes blocks */
>  	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
>  
> @@ -1713,10 +1680,14 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  
>  	nsectors = mtd->writesize / ecc->size;
>  
> -	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
> -		ret = -EINVAL;
> -		goto err;
> -	}
> +	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors))
> +		return -EINVAL;
> +
> +	sunxi_nand->ecc_ctl = NFC_ECC_MODE(i) | NFC_ECC_EXCEPTION |
> +			      NFC_ECC_PIPELINE | NFC_ECC_EN;
> +
> +	if (ecc->size == 512)
> +		sunxi_nand->ecc_ctl |= NFC_ECC_BLOCK_512;
>  
>  	ecc->read_oob = sunxi_nfc_hw_ecc_read_oob;
>  	ecc->write_oob = sunxi_nfc_hw_ecc_write_oob;
> @@ -1739,25 +1710,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	ecc->write_oob_raw = nand_write_oob_std;
>  
>  	return 0;
> -
> -err:
> -	kfree(sunxi_nand->ecc);
> -
> -	return ret;
> -}
> -
> -static void sunxi_nand_ecc_cleanup(struct sunxi_nand_chip *sunxi_nand)
> -{
> -	struct nand_ecc_ctrl *ecc = &sunxi_nand->nand.ecc;
> -
> -	switch (ecc->engine_type) {
> -	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> -		sunxi_nand_hw_ecc_ctrl_cleanup(sunxi_nand);
> -		break;
> -	case NAND_ECC_ENGINE_TYPE_NONE:
> -	default:
> -		break;
> -	}
>  }
>  
>  static int sunxi_nand_attach_chip(struct nand_chip *nand)
> @@ -1970,7 +1922,6 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
>  		ret = mtd_device_unregister(nand_to_mtd(chip));
>  		WARN_ON(ret);
>  		nand_cleanup(chip);
> -		sunxi_nand_ecc_cleanup(sunxi_nand);
>  		list_del(&sunxi_nand->node);
>  	}
>  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] mtd: rawnand: sunxi: Precompute the ECC_CTL register value

[Samuel Holland]

Hi Miquèl,

On 1/2/23 03:30, Miquel Raynal wrote:
> Hi Samuel,
> 
> samuel@sholland.org wrote on Thu, 29 Dec 2022 12:15:26 -0600:
> 
>> This removes an unnecessary memory allocation, and avoids recomputing
>> the same register value every time ECC is enabled.
> 
> I am fine with the "let's not recompute the register value each time"
> idea, but I like having a dedicated object reserved for the ECC
> engine, that is separated from the main controller structure (both
> are two different things, even though they are usually well
> integrated).
> 
> If it's actually useless you can still get rid of the allocation and in
> the structure you can save the ecc_ctrl reg value instead of mode.

OK, I will do this, and split it into two patches: one for replacing
mode with ecc_ctrl, and the second to keep the structure but get rid of
the allocation.

Regards,
Samuel

> The other patches in the series look good to me.
> 
> Thanks,
> Miquèl
> 
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/mtd/nand/raw/sunxi_nand.c | 75 ++++++-------------------------
>>  1 file changed, 13 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
>> index a3bc9f7f9e5a..5c5a567d8870 100644
>> --- a/drivers/mtd/nand/raw/sunxi_nand.c
>> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
>> @@ -169,22 +169,13 @@ struct sunxi_nand_chip_sel {
>>  	s8 rb;
>>  };
>>  
>> -/**
>> - * struct sunxi_nand_hw_ecc - stores information related to HW ECC support
>> - *
>> - * @mode: the sunxi ECC mode field deduced from ECC requirements
>> - */
>> -struct sunxi_nand_hw_ecc {
>> -	int mode;
>> -};
>> -
>>  /**
>>   * struct sunxi_nand_chip - stores NAND chip device related information
>>   *
>>   * @node: used to store NAND chips into a list
>>   * @nand: base NAND chip structure
>> - * @ecc: ECC controller structure
>>   * @clk_rate: clk_rate required for this NAND chip
>> + * @ecc_ctl: ECC_CTL register value for this NAND chip
>>   * @timing_cfg: TIMING_CFG register value for this NAND chip
>>   * @timing_ctl: TIMING_CTL register value for this NAND chip
>>   * @nsels: number of CS lines required by the NAND chip
>> @@ -193,8 +184,8 @@ struct sunxi_nand_hw_ecc {
>>  struct sunxi_nand_chip {
>>  	struct list_head node;
>>  	struct nand_chip nand;
>> -	struct sunxi_nand_hw_ecc *ecc;
>>  	unsigned long clk_rate;
>> +	u32 ecc_ctl;
>>  	u32 timing_cfg;
>>  	u32 timing_ctl;
>>  	int nsels;
>> @@ -689,26 +680,15 @@ static void sunxi_nfc_hw_ecc_enable(struct nand_chip *nand)
>>  {
>>  	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
>> -	u32 ecc_ctl;
>> -
>> -	ecc_ctl = readl(nfc->regs + NFC_REG_ECC_CTL);
>> -	ecc_ctl &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE |
>> -		     NFC_ECC_BLOCK_SIZE_MSK);
>> -	ecc_ctl |= NFC_ECC_EN | NFC_ECC_MODE(sunxi_nand->ecc->mode) |
>> -		   NFC_ECC_EXCEPTION | NFC_ECC_PIPELINE;
>> -
>> -	if (nand->ecc.size == 512)
>> -		ecc_ctl |= NFC_ECC_BLOCK_512;
>>  
>> -	writel(ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
>> +	writel(sunxi_nand->ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
>>  }
>>  
>>  static void sunxi_nfc_hw_ecc_disable(struct nand_chip *nand)
>>  {
>>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
>>  
>> -	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
>> -	       nfc->regs + NFC_REG_ECC_CTL);
>> +	writel(0, nfc->regs + NFC_REG_ECC_CTL);
>>  }
>>  
>>  static inline void sunxi_nfc_user_data_to_buf(u32 user_data, u8 *buf)
>> @@ -1626,11 +1606,6 @@ static const struct mtd_ooblayout_ops sunxi_nand_ooblayout_ops = {
>>  	.free = sunxi_nand_ooblayout_free,
>>  };
>>  
>> -static void sunxi_nand_hw_ecc_ctrl_cleanup(struct sunxi_nand_chip *sunxi_nand)
>> -{
>> -	kfree(sunxi_nand->ecc);
>> -}
>> -
>>  static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  				       struct nand_ecc_ctrl *ecc,
>>  				       struct device_node *np)
>> @@ -1641,7 +1616,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  	struct mtd_info *mtd = nand_to_mtd(nand);
>>  	struct nand_device *nanddev = mtd_to_nanddev(mtd);
>>  	int nsectors;
>> -	int ret;
>>  	int i;
>>  
>>  	if (nanddev->ecc.user_conf.flags & NAND_ECC_MAXIMIZE_STRENGTH) {
>> @@ -1675,10 +1649,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  	if (ecc->size != 512 && ecc->size != 1024)
>>  		return -EINVAL;
>>  
>> -	sunxi_nand->ecc = kzalloc(sizeof(*sunxi_nand->ecc), GFP_KERNEL);
>> -	if (!sunxi_nand->ecc)
>> -		return -ENOMEM;
>> -
>>  	/* Prefer 1k ECC chunk over 512 ones */
>>  	if (ecc->size == 512 && mtd->writesize > 512) {
>>  		ecc->size = 1024;
>> @@ -1699,12 +1669,9 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  
>>  	if (i >= ARRAY_SIZE(strengths)) {
>>  		dev_err(nfc->dev, "unsupported strength\n");
>> -		ret = -ENOTSUPP;
>> -		goto err;
>> +		return -ENOTSUPP;
>>  	}
>>  
>> -	sunxi_nand->ecc->mode = i;
>> -
>>  	/* HW ECC always request ECC bytes for 1024 bytes blocks */
>>  	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
>>  
>> @@ -1713,10 +1680,14 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  
>>  	nsectors = mtd->writesize / ecc->size;
>>  
>> -	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
>> -		ret = -EINVAL;
>> -		goto err;
>> -	}
>> +	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors))
>> +		return -EINVAL;
>> +
>> +	sunxi_nand->ecc_ctl = NFC_ECC_MODE(i) | NFC_ECC_EXCEPTION |
>> +			      NFC_ECC_PIPELINE | NFC_ECC_EN;
>> +
>> +	if (ecc->size == 512)
>> +		sunxi_nand->ecc_ctl |= NFC_ECC_BLOCK_512;
>>  
>>  	ecc->read_oob = sunxi_nfc_hw_ecc_read_oob;
>>  	ecc->write_oob = sunxi_nfc_hw_ecc_write_oob;
>> @@ -1739,25 +1710,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>>  	ecc->write_oob_raw = nand_write_oob_std;
>>  
>>  	return 0;
>> -
>> -err:
>> -	kfree(sunxi_nand->ecc);
>> -
>> -	return ret;
>> -}
>> -
>> -static void sunxi_nand_ecc_cleanup(struct sunxi_nand_chip *sunxi_nand)
>> -{
>> -	struct nand_ecc_ctrl *ecc = &sunxi_nand->nand.ecc;
>> -
>> -	switch (ecc->engine_type) {
>> -	case NAND_ECC_ENGINE_TYPE_ON_HOST:
>> -		sunxi_nand_hw_ecc_ctrl_cleanup(sunxi_nand);
>> -		break;
>> -	case NAND_ECC_ENGINE_TYPE_NONE:
>> -	default:
>> -		break;
>> -	}
>>  }
>>  
>>  static int sunxi_nand_attach_chip(struct nand_chip *nand)
>> @@ -1970,7 +1922,6 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
>>  		ret = mtd_device_unregister(nand_to_mtd(chip));
>>  		WARN_ON(ret);
>>  		nand_cleanup(chip);
>> -		sunxi_nand_ecc_cleanup(sunxi_nand);
>>  		list_del(&sunxi_nand->node);
>>  	}
>>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel