Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
The 07/01/2019 12:04, Miquel Raynal wrote: EXTERNAL MAIL Hi Piotr, Piotr Sroka wrote on Mon, 1 Jul 2019 10:51:45 +0100: [...] >> >> > >> >> >This driver is way too massive, I am pretty sure it can shrink a >> >> >little bit more. >> >> >[...] >> >> > >> >> I will try to make it shorer but it will be difucult to achive. It is because - there are a lot of calculation needed for PHY - ECC are interleaved with data (like on marvell-nand or gpmi-nand). >> >>Therefore:+ RAW mode is complicated+ protecting BBM increases number of lines of source code >> >> - need to support two DMA engines internal and external (slave) We will see on next patch version what is the result. That page layout looks: >> > >> >Maybe you don't need to support both internal and external DMA? >> > >> >I am pretty sure there are rooms for size reduction. >> >> I describe how it works in general and maybe you help me chose better solution. >> >> HW controller can work in 3 modes. PIO - can work in master or slave DMA >> CDMA - needs Master DMA for accessing command descriptors. >> Generic mode - can use only Slave DMA. >> >> Generic mode is neccessery to implement functions other than page >> program, page read, block erase. So it is essential. I cannot avoid >> to use Slave DMA. > >This deserves a nice comment at the top. Ok I will add the modes description to cover letter. > Not only to the cover letter: People read the code. Interested people might also read the commit log which is quite easy to find. The cover letter however will just disappear in the history of the Internet. I would rather prefer you explain how the IP works at the top of the driver. So I will add the modes description to both cover letter and at the top of the driver. Thanks, Miquèl Thanks, Piotr
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Piotr, Piotr Sroka wrote on Mon, 1 Jul 2019 10:51:45 +0100: [...] > >> >> > > >> >> >This driver is way too massive, I am pretty sure it can shrink a > >> >> >little bit more. > >> >> >[...] > >> >> > > >> >> I will try to make it shorer but it will be difucult to achive. It is > >> >> because - there are a lot of calculation needed for PHY - ECC are > >> >> interleaved with data (like on marvell-nand or gpmi-nand). > >> >>Therefore:+ RAW mode is complicated+ protecting BBM > >> >> increases number of lines of source code > >> >> - need to support two DMA engines internal and external (slave) We will > >> >> see on next patch version what is the result. That page layout > >> >> looks: > >> > > >> >Maybe you don't need to support both internal and external DMA? > >> > > >> >I am pretty sure there are rooms for size reduction. > >> > >> I describe how it works in general and maybe you help me chose better > >> solution. > >> > >> HW controller can work in 3 modes. PIO - can work in master or slave DMA > >> CDMA - needs Master DMA for accessing command descriptors. > >> Generic mode - can use only Slave DMA. > >> > >> Generic mode is neccessery to implement functions other than page > >> program, page read, block erase. So it is essential. I cannot avoid > >> to use Slave DMA. > > > >This deserves a nice comment at the top. > Ok I will add the modes description to cover letter. > Not only to the cover letter: People read the code. Interested people might also read the commit log which is quite easy to find. The cover letter however will just disappear in the history of the Internet. I would rather prefer you explain how the IP works at the top of the driver. Thanks, Miquèl
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
The 06/27/2019 18:15, Miquel Raynal wrote:
EXTERNAL MAIL
Hi Piotr,
Piotr Sroka wrote on Thu, 6 Jun 2019 16:19:51
+0100:
Hi Miquel
The 05/12/2019 14:24, Miquel Raynal wrote:
>EXTERNAL MAIL
>
>
>EXTERNAL MAIL
>
>
>Hi Piotr,
>
>Sorry for de delay.
>
>Piotr Sroka wrote on Thu, 21 Mar 2019 09:33:58
>+:
>
>> The 03/05/2019 19:09, Miquel Raynal wrote:
>> >EXTERNAL MAIL
>> >
>> >
>> >Hi Piotr,
>> >
>> >Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
>> >+:
>> >
>> >> This patch adds driver for Cadence HPNFC NAND controller.
>> >>
>> >> Signed-off-by: Piotr Sroka
>> >> ---
>> >> Changes for v2:
>> >> - create one universal wait function for all events instead of one
>> >> function per event.
>> >> - split one big function executing nand operations to separate
>> >> functions one per each type of operation.
>> >> - add erase atomic operation to nand operation parser
>> >> - remove unnecessary includes.
>> >> - remove unused register defines
>> >> - add support for multiple nand chips
>> >> - remove all code using legacy functions
>> >> - remove chip dependents parameters from dts bindings, they were
>> >> attached to the SoC specific compatible at the driver level
>> >> - simplify interrupt handling
>> >> - simplify timing calculations
>> >> - fix calculation of maximum supported cs signals
>> >> - simplify ecc size calculation
>> >> - remove header file and put whole code to one c file
>> >> ---
>> >> drivers/mtd/nand/raw/Kconfig |8 +
>> >> drivers/mtd/nand/raw/Makefile |1 +
>> >> drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
>> >
>> >This driver is way too massive, I am pretty sure it can shrink a
>> >little bit more.
>> >[...]
>> >
>> I will try to make it shorer but it will be difucult to achive. It is
because - there are a lot of calculation needed for PHY - ECC are interleaved
with data (like on marvell-nand or gpmi-nand).
>>Therefore:+ RAW mode is complicated+ protecting BBM increases
number of lines of source code
>> - need to support two DMA engines internal and external (slave) We will see
on next patch version what is the result. That page layout looks:
>
>Maybe you don't need to support both internal and external DMA?
>
>I am pretty sure there are rooms for size reduction.
I describe how it works in general and maybe you help me chose better solution.
HW controller can work in 3 modes. PIO - can work in master or slave DMA
CDMA - needs Master DMA for accessing command descriptors.
Generic mode - can use only Slave DMA.
Generic mode is neccessery to implement functions other than page
program, page read, block erase. So it is essential. I cannot avoid
to use Slave DMA.
This deserves a nice comment at the top.
Ok I will add the modes description to cover letter.
I change CDMA mode to PIO mode. Then I can use only slave DMA. But CDMA has a
feature which is not present in PIO mode. The feature
gives possibility to point DMA engine two buffers to transfer. It is
used to point data buffer and oob bufer. In PIO mode I would need to
copy data buffer and oob buffer to third buffer. Next transfer data from
third buffer.
In that solution we need to copy all data by CPU and then use DMA. Controller
needs always transfer oob because of HW ECC restrictions. Such change will
decrease performce for all data transfers.
I think performance is more important in that case. What is your
opinion? [...]
Indeed
>> >
>> >What is this for?
>> Fucntions enables/disables hardware detection of erased data
>> pages. >
>
>Ok, the name is not very explicit , maybe you could tell this with a
>comment.
>
Ok.
>> >> +
>> >> +/* hardware initialization */
>> >> +static int cadence_nand_hw_init(struct cdns_nand_ctrl *cdns_ctrl)
>> >> +{
>> >> + int status = 0;
>> >> + u32 reg;
>> >> +
>> >> + status = cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
>> >> + 100,
>> >> + CTRL_STATUS_INIT_COMP, false);
>> >> + if (status)
>> >> + return status;
>> >> +
>> >> + reg = readl(cdns_ctrl->reg + CTRL_VERSION);
>> >> +
>> >> + dev_info(cdns_ctrl->dev,
>> >> + "%s: cadence nand controller version reg %x\n",
>> >> + __func__, reg);
>> >> +
>> >> + /* disable cache and multiplane */
>> >> + writel(0, cdns_ctrl->reg + MULTIPLANE_CFG);
>> >> + writel(0, cdns_ctrl->reg + CACHE_CFG);
>> >> +
>> >> + /* clear all interrupts */
>> >> + writel(0x, cdns_ctrl->reg + INTR_STATUS);
>> >> +
>> >> + cadence_nand_get_caps(cdns_ctrl);
>> >> + cadence_nand_read_bch_cfg(cdns_ctrl);
>> >
>> >No, you cannot rely on the bootloader's configuration. And I suppose
>> >this is what the first call to read_bch_cfg does?
>> I do not realy on boot loader. Just read NAND flash
>> controller configuration from read only capabilities registers.
>
>Ok, i
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Piotr,
Piotr Sroka wrote on Thu, 6 Jun 2019 16:19:51
+0100:
> Hi Miquel
>
>
> The 05/12/2019 14:24, Miquel Raynal wrote:
> >EXTERNAL MAIL
> >
> >
> >EXTERNAL MAIL
> >
> >
> >Hi Piotr,
> >
> >Sorry for de delay.
> >
> >Piotr Sroka wrote on Thu, 21 Mar 2019 09:33:58
> >+:
> >
> >> The 03/05/2019 19:09, Miquel Raynal wrote:
> >> >EXTERNAL MAIL
> >> >
> >> >
> >> >Hi Piotr,
> >> >
> >> >Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
> >> >+:
> >> >
> >> >> This patch adds driver for Cadence HPNFC NAND controller.
> >> >>
> >> >> Signed-off-by: Piotr Sroka
> >> >> ---
> >> >> Changes for v2:
> >> >> - create one universal wait function for all events instead of one
> >> >> function per event.
> >> >> - split one big function executing nand operations to separate
> >> >> functions one per each type of operation.
> >> >> - add erase atomic operation to nand operation parser
> >> >> - remove unnecessary includes.
> >> >> - remove unused register defines
> >> >> - add support for multiple nand chips
> >> >> - remove all code using legacy functions
> >> >> - remove chip dependents parameters from dts bindings, they were
> >> >> attached to the SoC specific compatible at the driver level
> >> >> - simplify interrupt handling
> >> >> - simplify timing calculations
> >> >> - fix calculation of maximum supported cs signals
> >> >> - simplify ecc size calculation
> >> >> - remove header file and put whole code to one c file
> >> >> ---
> >> >> drivers/mtd/nand/raw/Kconfig |8 +
> >> >> drivers/mtd/nand/raw/Makefile |1 +
> >> >> drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
> >> >>
> >> >
> >> >This driver is way too massive, I am pretty sure it can shrink a
> >> >little bit more.
> >> >[...]
> >> >
> >> I will try to make it shorer but it will be difucult to achive. It is
> >> because - there are a lot of calculation needed for PHY - ECC are
> >> interleaved with data (like on marvell-nand or gpmi-nand).
> >>Therefore:+ RAW mode is complicated+ protecting BBM increases
> >> number of lines of source code
> >> - need to support two DMA engines internal and external (slave) We will
> >> see on next patch version what is the result. That page layout looks:
> >>
> >
> >Maybe you don't need to support both internal and external DMA?
> >
> >I am pretty sure there are rooms for size reduction.
>
> I describe how it works in general and maybe you help me chose better
> solution.
>
> HW controller can work in 3 modes. PIO - can work in master or slave DMA
> CDMA - needs Master DMA for accessing command descriptors.
> Generic mode - can use only Slave DMA.
>
> Generic mode is neccessery to implement functions other than page
> program, page read, block erase. So it is essential. I cannot avoid
> to use Slave DMA.
This deserves a nice comment at the top.
>
> I change CDMA mode to PIO mode. Then I can use only slave DMA. But CDMA has a
> feature which is not present in PIO mode. The feature
> gives possibility to point DMA engine two buffers to transfer. It is
> used to point data buffer and oob bufer. In PIO mode I would need to
> copy data buffer and oob buffer to third buffer. Next transfer data from
> third buffer.
> In that solution we need to copy all data by CPU and then use DMA.
> Controller needs always transfer oob because of HW ECC restrictions. Such
> change will decrease performce for all data transfers.
> I think performance is more important in that case. What is your
> opinion? [...]
Indeed
> >> >
> >> >What is this for?
> >> Fucntions enables/disables hardware detection of erased data
> >> pages. >
> >
> >Ok, the name is not very explicit , maybe you could tell this with a
> >comment.
> >
> Ok.
>
> >> >> +
> >> >> +/* hardware initialization */
> >> >> +static int cadence_nand_hw_init(struct cdns_nand_ctrl *cdns_ctrl)
> >> >> +{
> >> >> + int status = 0;
> >> >> + u32 reg;
> >> >> +
> >> >> + status = cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
> >> >> +100,
> >> >> +CTRL_STATUS_INIT_COMP,
> >> >> false);
> >> >> + if (status)
> >> >> + return status;
> >> >> +
> >> >> + reg = readl(cdns_ctrl->reg + CTRL_VERSION);
> >> >> +
> >> >> + dev_info(cdns_ctrl->dev,
> >> >> +"%s: cadence nand controller version reg %x\n",
> >> >> +__func__, reg);
> >> >> +
> >> >> + /* disable cache and multiplane */
> >> >> + writel(0, cdns_ctrl->reg + MULTIPLANE_CFG);
> >> >> + writel(0, cdns_ctrl->reg + CACHE_CFG);
> >> >> +
> >> >> + /* clear all interrupts */
> >> >> + writel(0x, cdns_ctrl->reg + INTR_STATUS);
> >> >> +
> >> >> + cadence_nand_get_caps(cdns_ctrl);
> >> >> + cadence_nand_read_bch_cfg(cdns_ctrl);
> >> >
> >> >No, you cannot rely
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Miquel
The 05/12/2019 14:24, Miquel Raynal wrote:
EXTERNAL MAIL
EXTERNAL MAIL
Hi Piotr,
Sorry for de delay.
Piotr Sroka wrote on Thu, 21 Mar 2019 09:33:58
+:
The 03/05/2019 19:09, Miquel Raynal wrote:
>EXTERNAL MAIL
>
>
>Hi Piotr,
>
>Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
>+:
>
>> This patch adds driver for Cadence HPNFC NAND controller.
>>
>> Signed-off-by: Piotr Sroka
>> ---
>> Changes for v2:
>> - create one universal wait function for all events instead of one
>> function per event.
>> - split one big function executing nand operations to separate
>> functions one per each type of operation.
>> - add erase atomic operation to nand operation parser
>> - remove unnecessary includes.
>> - remove unused register defines
>> - add support for multiple nand chips
>> - remove all code using legacy functions
>> - remove chip dependents parameters from dts bindings, they were
>> attached to the SoC specific compatible at the driver level
>> - simplify interrupt handling
>> - simplify timing calculations
>> - fix calculation of maximum supported cs signals
>> - simplify ecc size calculation
>> - remove header file and put whole code to one c file
>> ---
>> drivers/mtd/nand/raw/Kconfig |8 +
>> drivers/mtd/nand/raw/Makefile |1 +
>> drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
>
>This driver is way too massive, I am pretty sure it can shrink a
>little bit more.
>[...]
>
I will try to make it shorer but it will be difucult to achive. It is because -
there are a lot of calculation needed for PHY - ECC are interleaved with
data (like on marvell-nand or gpmi-nand).
Therefore:+ RAW mode is complicated+ protecting BBM increases number
of lines of source code
- need to support two DMA engines internal and external (slave) We will see on
next patch version what is the result. That page layout looks:
Maybe you don't need to support both internal and external DMA?
I am pretty sure there are rooms for size reduction.
I describe how it works in general and maybe you help me chose better solution.
HW controller can work in 3 modes.
PIO - can work in master or slave DMA
CDMA - needs Master DMA for accessing command descriptors.
Generic mode - can use only Slave DMA.
Generic mode is neccessery to implement functions other than page
program, page read, block erase. So it is essential. I cannot avoid
to use Slave DMA.
I change CDMA mode to PIO mode. Then I can use only slave DMA.
But CDMA has a feature which is not present in PIO mode. The feature
gives possibility to point DMA engine two buffers to transfer. It is
used to point data buffer and oob bufer. In PIO mode I would need to
copy data buffer and oob buffer to third buffer. Next transfer data from
third buffer.
In that solution we need to copy all data by CPU and then use DMA.
Controller needs always transfer oob because of HW ECC restrictions.
Such change will decrease performce for all data transfers.
I think performance is more important in that case. What is your
opinion?
[...]
>
>What is this for?
Fucntions enables/disables hardware detection of erased data
pages. >
Ok, the name is not very explicit , maybe you could tell this with a
comment.
Ok.
>> +
>> +/* hardware initialization */
>> +static int cadence_nand_hw_init(struct cdns_nand_ctrl *cdns_ctrl)
>> +{
>> + int status = 0;
>> + u32 reg;
>> +
>> + status = cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
>> + 100,
>> + CTRL_STATUS_INIT_COMP, false);
>> + if (status)
>> + return status;
>> +
>> + reg = readl(cdns_ctrl->reg + CTRL_VERSION);
>> +
>> + dev_info(cdns_ctrl->dev,
>> + "%s: cadence nand controller version reg %x\n",
>> + __func__, reg);
>> +
>> + /* disable cache and multiplane */
>> + writel(0, cdns_ctrl->reg + MULTIPLANE_CFG);
>> + writel(0, cdns_ctrl->reg + CACHE_CFG);
>> +
>> + /* clear all interrupts */
>> + writel(0x, cdns_ctrl->reg + INTR_STATUS);
>> +
>> + cadence_nand_get_caps(cdns_ctrl);
>> + cadence_nand_read_bch_cfg(cdns_ctrl);
>
>No, you cannot rely on the bootloader's configuration. And I suppose
>this is what the first call to read_bch_cfg does?
I do not realy on boot loader. Just read NAND flash
controller configuration from read only capabilities registers.
Ok, if these are RO registers, it's fine. But maybe don't call the
function "read bch config" which suggest that this is something you can
change.
ok.
>> +
>> +#define TT_OOB_AREA 1
>> +#define TT_MAIN_OOB_AREAS 2
>> +#define TT_RAW_PAGE 3
>> +#define TT_BBM4
>> +#define TT_MAIN_OOB_AREA_EXT 5
>> +
>> +/* prepare size of data to transfer */
>> +static int
>> +cadence_nand_prepare_data_size(struct nand_chip *chi
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Piotr,
Sorry for de delay.
Piotr Sroka wrote on Thu, 21 Mar 2019 09:33:58
+:
> The 03/05/2019 19:09, Miquel Raynal wrote:
> >EXTERNAL MAIL
> >
> >
> >Hi Piotr,
> >
> >Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
> >+:
> >
> >> This patch adds driver for Cadence HPNFC NAND controller.
> >>
> >> Signed-off-by: Piotr Sroka
> >> ---
> >> Changes for v2:
> >> - create one universal wait function for all events instead of one
> >> function per event.
> >> - split one big function executing nand operations to separate
> >> functions one per each type of operation.
> >> - add erase atomic operation to nand operation parser
> >> - remove unnecessary includes.
> >> - remove unused register defines
> >> - add support for multiple nand chips
> >> - remove all code using legacy functions
> >> - remove chip dependents parameters from dts bindings, they were
> >> attached to the SoC specific compatible at the driver level
> >> - simplify interrupt handling
> >> - simplify timing calculations
> >> - fix calculation of maximum supported cs signals
> >> - simplify ecc size calculation
> >> - remove header file and put whole code to one c file
> >> ---
> >> drivers/mtd/nand/raw/Kconfig |8 +
> >> drivers/mtd/nand/raw/Makefile |1 +
> >> drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
> >>
> >
> >This driver is way too massive, I am pretty sure it can shrink a
> >little bit more.
> >[...]
> >
> I will try to make it shorer but it will be difucult to achive. It is because
> - there are a lot of calculation needed for PHY - ECC are interleaved
> with data (like on marvell-nand or gpmi-nand).
>Therefore:+ RAW mode is complicated+ protecting BBM increases
> number of lines of source code
> - need to support two DMA engines internal and external (slave) We will see
> on next patch version what is the result. That page layout looks:
Maybe you don't need to support both internal and external DMA?
I am pretty sure there are rooms for size reduction.
>
> +-
> | Data 1 | ECC 1 | ... | Data N | ECC N |
> +-
>
>--+
> Last Data | OOB bytes | Last ECC |
>--+
> /\ || OOB area started
> usualy vendor specified BBM
>
> Flash OOB area starts somewhere in last data sector. Flash OOB area contains
> part of last sector, oob data (accessible by driver), and last ECC code >> +
> >> +struct cdns_nand_chip {
> >> + struct cadence_nand_timings timings;
> >> + struct nand_chip chip;
> >> + u8 nsels;
> >> + struct list_head node;
> >> +
> >> + /*
> >> + * part of oob area of NANF flash memory page.
> >> + * This part is available for user to read or write.
> >> + */
> >> + u32 avail_oob_size;
> >> + /* oob area size of NANF flash memory page */
> >> + u32 oob_size;
> >> + /* main area size of NANF flash memory page */
> >> + u32 main_size;
> >
> >These fields are redundant and exist in mtd_info/nand_chip.
> >
> Ok I will use the parameters from mtd_info.
> >> +
> >> + /* sector size few sectors are located on main area of NF memory page */
> >> + u32 sector_size;
> >> + u32 sector_count;
> >> +
> >> + /* offset of BBM*/
> >> + u8 bbm_offs;
> >> + /* number of bytes reserved for BBM */
> >> + u8 bbm_len;
> >
> >Why do you bother at the controller driver level with bbm?
> >
> When ECC is enabled then BBM is somewhere in last data sector. So for write
> operation real BBM will be overwritten. For read operation
> it will be read from wrong offset. To protect BBM we use HW feature skip
> bytes. To be able to properly configure this feature we need to
> know what is the offset of BBM. >> +
> >> +static int cadence_nand_set_erase_detection(struct cdns_nand_ctrl
> >> *cdns_ctrl,
> >> + bool enable,
> >> + u8 bitflips_threshold)
> >
> >What is this for?
> Fucntions enables/disables hardware detection of erased data
> pages. >
Ok, the name is not very explicit , maybe you could tell this with a
comment.
> >> +
> >> +/* hardware initialization */
> >> +static int cadence_nand_hw_init(struct cdns_nand_ctrl *cdns_ctrl)
> >> +{
> >> + int status = 0;
> >> + u32 reg;
> >> +
> >> + status = cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
> >> + 100,
> >> + CTRL_STATUS_INIT_COMP, false);
> >> + if (status)
> >> + return status;
> >> +
> >> + reg = readl(cdns_ctrl->reg + CTRL_VERSION);
> >> +
> >> + dev_info(cdns_ctrl->dev,
> >> + "%s: cadence nand controller version reg %x\n",
> >> + __func__, reg);
> >> +
> >> + /* disable cache and multiplane */
> >> + writel(0, cdns_ctrl->reg + MULTIPLANE_CFG
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
The 03/05/2019 19:09, Miquel Raynal wrote:
EXTERNAL MAIL
Hi Piotr,
Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
+:
This patch adds driver for Cadence HPNFC NAND controller.
Signed-off-by: Piotr Sroka
---
Changes for v2:
- create one universal wait function for all events instead of one
function per event.
- split one big function executing nand operations to separate
functions one per each type of operation.
- add erase atomic operation to nand operation parser
- remove unnecessary includes.
- remove unused register defines
- add support for multiple nand chips
- remove all code using legacy functions
- remove chip dependents parameters from dts bindings, they were
attached to the SoC specific compatible at the driver level
- simplify interrupt handling
- simplify timing calculations
- fix calculation of maximum supported cs signals
- simplify ecc size calculation
- remove header file and put whole code to one c file
---
drivers/mtd/nand/raw/Kconfig |8 +
drivers/mtd/nand/raw/Makefile |1 +
drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
This driver is way too massive, I am pretty sure it can shrink a
little bit more.
[...]
I will try to make it shorer but it will be difucult to achive. It is because
- there are a lot of calculation needed for PHY
- ECC are interleaved with data (like on marvell-nand or gpmi-nand).
Therefore:
+ RAW mode is complicated
+ protecting BBM increases number of lines of source code
- need to support two DMA engines internal and external (slave)
We will see on next patch version what is the result.
That page layout looks:
+-
| Data 1 | ECC 1 | ... | Data N | ECC N |
+-
--+
Last Data | OOB bytes | Last ECC |
--+
/\
||
OOB area started
usualy vendor specified BBM
Flash OOB area starts somewhere in last data sector.
Flash OOB area contains part of last sector, oob data
(accessible by driver), and last ECC code
+
+struct cdns_nand_chip {
+ struct cadence_nand_timings timings;
+ struct nand_chip chip;
+ u8 nsels;
+ struct list_head node;
+
+ /*
+* part of oob area of NANF flash memory page.
+* This part is available for user to read or write.
+*/
+ u32 avail_oob_size;
+ /* oob area size of NANF flash memory page */
+ u32 oob_size;
+ /* main area size of NANF flash memory page */
+ u32 main_size;
These fields are redundant and exist in mtd_info/nand_chip.
Ok I will use the parameters from mtd_info.
+
+ /* sector size few sectors are located on main area of NF memory page */
+ u32 sector_size;
+ u32 sector_count;
+
+ /* offset of BBM*/
+ u8 bbm_offs;
+ /* number of bytes reserved for BBM */
+ u8 bbm_len;
Why do you bother at the controller driver level with bbm?
When ECC is enabled then BBM is somewhere in last data sector.
So for write operation real BBM will be overwritten. For read operation
it will be read from wrong offset. To protect BBM we use HW feature
skip bytes. To be able to properly configure this feature we need to
know what is the offset of BBM.
+
+static int cadence_nand_set_erase_detection(struct cdns_nand_ctrl *cdns_ctrl,
+ bool enable,
+ u8 bitflips_threshold)
What is this for?
Fucntions enables/disables hardware detection of erased data
pages.
+
+/* hardware initialization */
+static int cadence_nand_hw_init(struct cdns_nand_ctrl *cdns_ctrl)
+{
+ int status = 0;
+ u32 reg;
+
+ status = cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
+100,
+CTRL_STATUS_INIT_COMP, false);
+ if (status)
+ return status;
+
+ reg = readl(cdns_ctrl->reg + CTRL_VERSION);
+
+ dev_info(cdns_ctrl->dev,
+"%s: cadence nand controller version reg %x\n",
+__func__, reg);
+
+ /* disable cache and multiplane */
+ writel(0, cdns_ctrl->reg + MULTIPLANE_CFG);
+ writel(0, cdns_ctrl->reg + CACHE_CFG);
+
+ /* clear all interrupts */
+ writel(0x, cdns_ctrl->reg + INTR_STATUS);
+
+ cadence_nand_get_caps(cdns_ctrl);
+ cadence_nand_read_bch_cfg(cdns_ctrl);
No, you cannot rely on the bootloader's configuration. And I suppose
this is what the first call to read_bch_cfg does?
I do not realy on boot loader. Just read NAND flash
controller configuration from read only capabilities registers.
+
+#define TT_OOB_AREA1
+#define TT_MAIN_OOB_AREAS 2
+#define TT_RAW_PAGE3
+#define TT_BBM 4
+#define TT
Re: [PATCH v2 1/2] mtd: nand: Add Cadence NAND controller driver
Hi Piotr,
Piotr Sroka wrote on Tue, 19 Feb 2019 16:18:23
+:
> This patch adds driver for Cadence HPNFC NAND controller.
>
> Signed-off-by: Piotr Sroka
> ---
> Changes for v2:
> - create one universal wait function for all events instead of one
> function per event.
> - split one big function executing nand operations to separate
> functions one per each type of operation.
> - add erase atomic operation to nand operation parser
> - remove unnecessary includes.
> - remove unused register defines
> - add support for multiple nand chips
> - remove all code using legacy functions
> - remove chip dependents parameters from dts bindings, they were
> attached to the SoC specific compatible at the driver level
> - simplify interrupt handling
> - simplify timing calculations
> - fix calculation of maximum supported cs signals
> - simplify ecc size calculation
> - remove header file and put whole code to one c file
> ---
> drivers/mtd/nand/raw/Kconfig |8 +
> drivers/mtd/nand/raw/Makefile |1 +
> drivers/mtd/nand/raw/cadence-nand-controller.c | 3288
>
This driver is way too massive, I am pretty sure it can shrink a
little bit more.
[...]
> +
> +struct cdns_nand_chip {
> + struct cadence_nand_timings timings;
> + struct nand_chip chip;
> + u8 nsels;
> + struct list_head node;
> +
> + /*
> + * part of oob area of NANF flash memory page.
> + * This part is available for user to read or write.
> + */
> + u32 avail_oob_size;
> + /* oob area size of NANF flash memory page */
> + u32 oob_size;
> + /* main area size of NANF flash memory page */
> + u32 main_size;
These fields are redundant and exist in mtd_info/nand_chip.
> +
> + /* sector size few sectors are located on main area of NF memory page */
> + u32 sector_size;
> + u32 sector_count;
> +
> + /* offset of BBM*/
> + u8 bbm_offs;
> + /* number of bytes reserved for BBM */
> + u8 bbm_len;
Why do you bother at the controller driver level with bbm?
> + /* ECC strength index */
> + u8 corr_str_idx;
> +
> + u8 cs[];
> +};
> +
> +struct ecc_info {
> + int (*calc_ecc_bytes)(int step_size, int strength);
> + int max_step_size;
> +};
> +
[...]
> +
> +static int cadence_nand_set_erase_detection(struct cdns_nand_ctrl *cdns_ctrl,
> + bool enable,
> + u8 bitflips_threshold)
What is this for?
> +{
> + u32 reg;
> +
> + if (cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
> + 100,
> + CTRL_STATUS_CTRL_BUSY, true))
> + return -ETIMEDOUT;
> +
> + reg = readl(cdns_ctrl->reg + ECC_CONFIG_0);
> +
> + if (enable)
> + reg |= ECC_CONFIG_0_ERASE_DET_EN;
> + else
> + reg &= ~ECC_CONFIG_0_ERASE_DET_EN;
> +
> + writel(reg, cdns_ctrl->reg + ECC_CONFIG_0);
> + writel(bitflips_threshold, cdns_ctrl->reg + ECC_CONFIG_1);
> +
> + return 0;
> +}
> +
> +static int cadence_nand_set_access_width16(struct cdns_nand_ctrl *cdns_ctrl,
> +bool bit_bus16)
> +{
> + u32 reg;
> +
> + if (cadence_nand_wait_for_value(cdns_ctrl, CTRL_STATUS,
> + 100,
> + CTRL_STATUS_CTRL_BUSY, true))
> + return -ETIMEDOUT;
> +
> + reg = readl(cdns_ctrl->reg + COMMON_SET);
> +
> + if (!bit_bus16)
> + reg &= ~COMMON_SET_DEVICE_16BIT;
> + else
> + reg |= COMMON_SET_DEVICE_16BIT;
> + writel(reg, cdns_ctrl->reg + COMMON_SET);
> +
> + return 0;
> +}
> +
> +static void
> +cadence_nand_clear_interrupt(struct cdns_nand_ctrl *cdns_ctrl,
> + struct cadence_nand_irq_status *irq_status)
> +{
> + writel(irq_status->status, cdns_ctrl->reg + INTR_STATUS);
> + writel(irq_status->trd_status, cdns_ctrl->reg + TRD_COMP_INT_STATUS);
> + writel(irq_status->trd_error, cdns_ctrl->reg + TRD_ERR_INT_STATUS);
> +}
> +
> +static void
> +cadence_nand_read_int_status(struct cdns_nand_ctrl *cdns_ctrl,
> + struct cadence_nand_irq_status *irq_status)
> +{
> + irq_status->status = readl(cdns_ctrl->reg + INTR_STATUS);
> + irq_status->trd_status = readl(cdns_ctrl->reg
> + + TRD_COMP_INT_STATUS);
> + irq_status->trd_error = readl(cdns_ctrl->reg + TRD_ERR_INT_STATUS);
> +}
> +
> +static u32 irq_detected(struct cdns_nand_ctrl *cdns_ctrl,
> + struct cadence_nand_irq_status *irq_status)
> +{
> + cadence_nand_read_int_status(cdns_ctrl, irq_status);
> +
> + return irq_status->status || irq_status->trd_status ||
> + irq_status->trd_error;
> +}
> +
> +static void cadence_nand_reset_irq(struct cdns_nand_ctrl *cdns_ctrl)
> +{
> + spi
