2019-06-03 12:45:12

by Shivamurthy Shastri (sshivamurthy)

Subject: [PATCH v3 04/12] mtd: rawnand: introduce struct onfi_helper

Create onfi_helper object. This is base to turn ONFI code to generic.

Signed-off-by: Shivamurthy Shastri <[email protected]>
---
include/linux/mtd/nand.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3cdf06cae8b6..645dde4c5797 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -11,6 +11,7 @@
#define __LINUX_MTD_NAND_H

#include <linux/mtd/mtd.h>
+#include <linux/mtd/onfi.h>

/**
* struct nand_memory_organization - Memory organization structure
@@ -157,6 +158,24 @@ struct nand_ops {
bool (*isbad)(struct nand_device *nand, const struct nand_pos *pos);
};

+/**
+ * struct onfi_helper - ONFI helper functions that should be implemented by
+ * specialized layers (raw NAND, SPI NAND, etc.)
+ * @page: Page number for ONFI parameter table
+ * @check_revision: Check ONFI revision number
+ * @parameter_page_read: Function to read parameter pages
+ * @init_intf_data: Initialize interface specific data or fixups
+ */
+struct onfi_helper {
+ u8 page;
+ int (*check_revision)(struct nand_device *base,
+ struct nand_onfi_params *p, int *onfi_version);
+ int (*parameter_page_read)(struct nand_device *base, u8 page,
+ void *buf, unsigned int len);
+ int (*init_intf_data)(struct nand_device *base,
+ struct nand_onfi_params *p);
+};
+
/**
* struct nand_device - NAND device
* @mtd: MTD instance attached to the NAND device
@@ -165,6 +184,7 @@ struct nand_ops {
* @rowconv: position to row address converter
* @bbt: bad block table info
* @ops: NAND operations attached to the NAND device
+ * @helper: Helper functions to detect and initialize ONFI NAND
*
* Generic NAND object. Specialized NAND layers (raw NAND, SPI NAND, OneNAND)
* should declare their own NAND object embedding a nand_device struct (that's
@@ -183,6 +203,7 @@ struct nand_device {
struct nand_row_converter rowconv;
struct nand_bbt bbt;
const struct nand_ops *ops;
+ struct onfi_helper helper;
};

/**
--
2.17.1

2019-06-03 13:09:22

by Boris Brezillon

Subject: Re: [PATCH v3 04/12] mtd: rawnand: introduce struct onfi_helper

Hi Shivamurthy,

On Mon, 3 Jun 2019 12:43:28 +0000
"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote:

> Create onfi_helper object. This is base to turn ONFI code to generic.
>
> Signed-off-by: Shivamurthy Shastri <[email protected]>
> ---
> include/linux/mtd/nand.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 3cdf06cae8b6..645dde4c5797 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -11,6 +11,7 @@
> #define __LINUX_MTD_NAND_H
>
> #include <linux/mtd/mtd.h>
> +#include <linux/mtd/onfi.h>
>
> /**
> * struct nand_memory_organization - Memory organization structure
> @@ -157,6 +158,24 @@ struct nand_ops {
> bool (*isbad)(struct nand_device *nand, const struct nand_pos *pos);
> };
>
> +/**
> + * struct onfi_helper - ONFI helper functions that should be implemented by
> + * specialized layers (raw NAND, SPI NAND, etc.)
> + * @page: Page number for ONFI parameter table
> + * @check_revision: Check ONFI revision number
> + * @parameter_page_read: Function to read parameter pages
> + * @init_intf_data: Initialize interface specific data or fixups
> + */
> +struct onfi_helper {
> + u8 page;
> + int (*check_revision)(struct nand_device *base,
> + struct nand_onfi_params *p, int *onfi_version);
> + int (*parameter_page_read)(struct nand_device *base, u8 page,
> + void *buf, unsigned int len);
> + int (*init_intf_data)(struct nand_device *base,
> + struct nand_onfi_params *p);
> +};
> +
> /**
> * struct nand_device - NAND device
> * @mtd: MTD instance attached to the NAND device
> @@ -165,6 +184,7 @@ struct nand_ops {
> * @rowconv: position to row address converter
> * @bbt: bad block table info
> * @ops: NAND operations attached to the NAND device
> + * @helper: Helper functions to detect and initialize ONFI NAND
> *
> * Generic NAND object. Specialized NAND layers (raw NAND, SPI NAND, OneNAND)
> * should declare their own NAND object embedding a nand_device struct (that's
> @@ -183,6 +203,7 @@ struct nand_device {
> struct nand_row_converter rowconv;
> struct nand_bbt bbt;
> const struct nand_ops *ops;
> + struct onfi_helper helper;

Sorry, but I don't think that's the right solution. When I said we
should have ONFI code shared I was thinking about the code that parses
the ONFI struct/data to extract nand_memory_organization bits or other
generic info, not something that would abstract how to retrieve the
ONFI param page. Clearly, the generic NAND layer is not supposed to
handle such protocol/low-level details.

Regards,

Boris

2019-06-04 12:06:41

by Shivamurthy Shastri (sshivamurthy)

Subject: RE: [EXT] Re: [PATCH v3 04/12] mtd: rawnand: introduce struct onfi_helper

Hi Boris,

> > Create onfi_helper object. This is base to turn ONFI code to generic.
> >
> > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > ---
> > include/linux/mtd/nand.h | 21 +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 3cdf06cae8b6..645dde4c5797 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -11,6 +11,7 @@
> > #define __LINUX_MTD_NAND_H
> >
> > #include <linux/mtd/mtd.h>
> > +#include <linux/mtd/onfi.h>
> >
> > /**
> > * struct nand_memory_organization - Memory organization structure
> > @@ -157,6 +158,24 @@ struct nand_ops {
> > bool (*isbad)(struct nand_device *nand, const struct nand_pos
> *pos);
> > };
> >
> > +/**
> > + * struct onfi_helper - ONFI helper functions that should be implemented
> by
> > + * specialized layers (raw NAND, SPI NAND, etc.)
> > + * @page: Page number for ONFI parameter table
> > + * @check_revision: Check ONFI revision number
> > + * @parameter_page_read: Function to read parameter pages
> > + * @init_intf_data: Initialize interface specific data or fixups
> > + */
> > +struct onfi_helper {
> > + u8 page;
> > + int (*check_revision)(struct nand_device *base,
> > + struct nand_onfi_params *p, int *onfi_version);
> > + int (*parameter_page_read)(struct nand_device *base, u8 page,
> > + void *buf, unsigned int len);
> > + int (*init_intf_data)(struct nand_device *base,
> > + struct nand_onfi_params *p);
> > +};
> > +
> > /**
> > * struct nand_device - NAND device
> > * @mtd: MTD instance attached to the NAND device
> > @@ -165,6 +184,7 @@ struct nand_ops {
> > * @rowconv: position to row address converter
> > * @bbt: bad block table info
> > * @ops: NAND operations attached to the NAND device
> > + * @helper: Helper functions to detect and initialize ONFI NAND
> > *
> > * Generic NAND object. Specialized NAND layers (raw NAND, SPI NAND,
> OneNAND)
> > * should declare their own NAND object embedding a nand_device struct
> (that's
> > @@ -183,6 +203,7 @@ struct nand_device {
> > struct nand_row_converter rowconv;
> > struct nand_bbt bbt;
> > const struct nand_ops *ops;
> > + struct onfi_helper helper;
>
> Sorry, but I don't think that's the right solution. When I said we
> should have ONFI code shared I was thinking about the code that parses
> the ONFI struct/data to extract nand_memory_organization bits or other
> generic info, not something that would abstract how to retrieve the
> ONFI param page. Clearly, the generic NAND layer is not supposed to
> handle such protocol/low-level details.
>

In that case, I am thinking to design as follows, which splits into generic independent code.
Let me know, if you have any concerns or inputs.

I will parsing code from nand_onfi_detect function and move it to mtd/nand/onfi.c.
Also, I will move functions like sanitize_string, nand_bit_wise_majority, onfi_crc16, and
any other generic info to mtd/nand/onfi.c.

Thanks,
Shiva

2019-06-04 13:02:36

by Boris Brezillon

Subject: Re: [EXT] Re: [PATCH v3 04/12] mtd: rawnand: introduce struct onfi_helper

On Tue, 4 Jun 2019 12:02:28 +0000
"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote:

> Hi Boris,
>
> > > Create onfi_helper object. This is base to turn ONFI code to generic.
> > >
> > > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > > ---
> > > include/linux/mtd/nand.h | 21 +++++++++++++++++++++
> > > 1 file changed, 21 insertions(+)
> > >
> > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > > index 3cdf06cae8b6..645dde4c5797 100644
> > > --- a/include/linux/mtd/nand.h
> > > +++ b/include/linux/mtd/nand.h
> > > @@ -11,6 +11,7 @@
> > > #define __LINUX_MTD_NAND_H
> > >
> > > #include <linux/mtd/mtd.h>
> > > +#include <linux/mtd/onfi.h>
> > >
> > > /**
> > > * struct nand_memory_organization - Memory organization structure
> > > @@ -157,6 +158,24 @@ struct nand_ops {
> > > bool (*isbad)(struct nand_device *nand, const struct nand_pos
> > *pos);
> > > };
> > >
> > > +/**
> > > + * struct onfi_helper - ONFI helper functions that should be implemented
> > by
> > > + * specialized layers (raw NAND, SPI NAND, etc.)
> > > + * @page: Page number for ONFI parameter table
> > > + * @check_revision: Check ONFI revision number
> > > + * @parameter_page_read: Function to read parameter pages
> > > + * @init_intf_data: Initialize interface specific data or fixups
> > > + */
> > > +struct onfi_helper {
> > > + u8 page;
> > > + int (*check_revision)(struct nand_device *base,
> > > + struct nand_onfi_params *p, int *onfi_version);
> > > + int (*parameter_page_read)(struct nand_device *base, u8 page,
> > > + void *buf, unsigned int len);
> > > + int (*init_intf_data)(struct nand_device *base,
> > > + struct nand_onfi_params *p);
> > > +};
> > > +
> > > /**
> > > * struct nand_device - NAND device
> > > * @mtd: MTD instance attached to the NAND device
> > > @@ -165,6 +184,7 @@ struct nand_ops {
> > > * @rowconv: position to row address converter
> > > * @bbt: bad block table info
> > > * @ops: NAND operations attached to the NAND device
> > > + * @helper: Helper functions to detect and initialize ONFI NAND
> > > *
> > > * Generic NAND object. Specialized NAND layers (raw NAND, SPI NAND,
> > OneNAND)
> > > * should declare their own NAND object embedding a nand_device struct
> > (that's
> > > @@ -183,6 +203,7 @@ struct nand_device {
> > > struct nand_row_converter rowconv;
> > > struct nand_bbt bbt;
> > > const struct nand_ops *ops;
> > > + struct onfi_helper helper;
> >
> > Sorry, but I don't think that's the right solution. When I said we
> > should have ONFI code shared I was thinking about the code that parses
> > the ONFI struct/data to extract nand_memory_organization bits or other
> > generic info, not something that would abstract how to retrieve the
> > ONFI param page. Clearly, the generic NAND layer is not supposed to
> > handle such protocol/low-level details.
> >
>
> In that case, I am thinking to design as follows, which splits into generic independent code.
> Let me know, if you have any concerns or inputs.
>
> I will parsing code from nand_onfi_detect function and move it to mtd/nand/onfi.c.
> Also, I will move functions like sanitize_string, nand_bit_wise_majority, onfi_crc16, and
> any other generic info to mtd/nand/onfi.c.

Sounds good.