| Message ID | 1483448495-31607-8-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On 01/03/2017 02:01 PM, Boris Brezillon wrote: > Move Samsung specific initialization and detection logic into > nand_samsung.c. This is part of the "separate vendor specific code from > core" cleanup process. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> [...] > diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c > index b3a332f37e14..05e9366696c9 100644 > --- a/drivers/mtd/nand/nand_ids.c > +++ b/drivers/mtd/nand/nand_ids.c > @@ -10,7 +10,7 @@ > #include <linux/mtd/nand.h> > #include <linux/sizes.h> > > -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS > +#define LP_OPTIONS 0 > #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) > > #define SP_OPTIONS NAND_NEED_READRDY > @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { > }; > > /* Manufacturer IDs */ > +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; Is the extern needed ? > struct nand_manufacturers nand_manuf_ids[] = { > {NAND_MFR_TOSHIBA, "Toshiba"}, > {NAND_MFR_ESMT, "ESMT"}, > - {NAND_MFR_SAMSUNG, "Samsung"}, > + {NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops}, > {NAND_MFR_FUJITSU, "Fujitsu"}, > {NAND_MFR_NATIONAL, "National"}, > {NAND_MFR_RENESAS, "Renesas"}, [...]
On Wed, 4 Jan 2017 16:14:07 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 01/03/2017 02:01 PM, Boris Brezillon wrote: > > Move Samsung specific initialization and detection logic into > > nand_samsung.c. This is part of the "separate vendor specific code from > > core" cleanup process. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > [...] > > > diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c > > index b3a332f37e14..05e9366696c9 100644 > > --- a/drivers/mtd/nand/nand_ids.c > > +++ b/drivers/mtd/nand/nand_ids.c > > @@ -10,7 +10,7 @@ > > #include <linux/mtd/nand.h> > > #include <linux/sizes.h> > > > > -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS > > +#define LP_OPTIONS 0 > > #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) > > > > #define SP_OPTIONS NAND_NEED_READRDY > > @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { > > }; > > > > /* Manufacturer IDs */ > > +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; > > Is the extern needed ? Yes, unless you have another solution. If you remove the extern keyword you just redeclare samsung_nand_manuf_ops here, which is not what we want. > > > struct nand_manufacturers nand_manuf_ids[] = { > > {NAND_MFR_TOSHIBA, "Toshiba"}, > > {NAND_MFR_ESMT, "ESMT"}, > > - {NAND_MFR_SAMSUNG, "Samsung"}, > > + {NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops}, > > {NAND_MFR_FUJITSU, "Fujitsu"}, > > {NAND_MFR_NATIONAL, "National"}, > > {NAND_MFR_RENESAS, "Renesas"}, > > [...] > >
On 01/04/2017 06:08 PM, Boris Brezillon wrote: > On Wed, 4 Jan 2017 16:14:07 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 01/03/2017 02:01 PM, Boris Brezillon wrote: >>> Move Samsung specific initialization and detection logic into >>> nand_samsung.c. This is part of the "separate vendor specific code from >>> core" cleanup process. >>> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> >> [...] >> >>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c >>> index b3a332f37e14..05e9366696c9 100644 >>> --- a/drivers/mtd/nand/nand_ids.c >>> +++ b/drivers/mtd/nand/nand_ids.c >>> @@ -10,7 +10,7 @@ >>> #include <linux/mtd/nand.h> >>> #include <linux/sizes.h> >>> >>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS >>> +#define LP_OPTIONS 0 >>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) >>> >>> #define SP_OPTIONS NAND_NEED_READRDY >>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { >>> }; >>> >>> /* Manufacturer IDs */ >>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; >> >> Is the extern needed ? > > Yes, unless you have another solution. If you remove the extern keyword > you just redeclare samsung_nand_manuf_ops here, which is not what we > want. Maybe some accessor function can help ?
On Sat, 7 Jan 2017 00:53:24 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 01/04/2017 06:08 PM, Boris Brezillon wrote: > > On Wed, 4 Jan 2017 16:14:07 +0100 > > Marek Vasut <marek.vasut@gmail.com> wrote: > > > >> On 01/03/2017 02:01 PM, Boris Brezillon wrote: > >>> Move Samsung specific initialization and detection logic into > >>> nand_samsung.c. This is part of the "separate vendor specific code from > >>> core" cleanup process. > >>> > >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > >> > >> [...] > >> > >>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c > >>> index b3a332f37e14..05e9366696c9 100644 > >>> --- a/drivers/mtd/nand/nand_ids.c > >>> +++ b/drivers/mtd/nand/nand_ids.c > >>> @@ -10,7 +10,7 @@ > >>> #include <linux/mtd/nand.h> > >>> #include <linux/sizes.h> > >>> > >>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS > >>> +#define LP_OPTIONS 0 > >>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) > >>> > >>> #define SP_OPTIONS NAND_NEED_READRDY > >>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { > >>> }; > >>> > >>> /* Manufacturer IDs */ > >>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; > >> > >> Is the extern needed ? > > > > Yes, unless you have another solution. If you remove the extern keyword > > you just redeclare samsung_nand_manuf_ops here, which is not what we > > want. > > Maybe some accessor function can help ? > You mean, in nand_ids.c const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops(); struct nand_manufacturers nand_manuf_ids[] = { ... {NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops}, ... }; and then, in nand_samsung.c const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops() { return &samsung_nand_mafuf_ops; } What's the point of this extra indirection? I mean, in both cases you use a symbol that is not part of the same source file, so you'll have to define this symbol (using a function prototype or an extern object definition). Is this all about fixing checkpatch warnings, or do you have a problem with objects shared between different source files? Now, I agree that the current approach is not ideal. A real improvement would be to let the NAND manufacturer drivers (nand_<vendor>.c) register themselves to the core. Something similar to CLK_OF_DECLARE() or IRQCHIP_DECLARE() for example. But that means creating a dedicated section for the nand_manufs_id table, and it's a lot more invasive than the current approach.
On 01/07/2017 08:49 AM, Boris Brezillon wrote: > On Sat, 7 Jan 2017 00:53:24 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 01/04/2017 06:08 PM, Boris Brezillon wrote: >>> On Wed, 4 Jan 2017 16:14:07 +0100 >>> Marek Vasut <marek.vasut@gmail.com> wrote: >>> >>>> On 01/03/2017 02:01 PM, Boris Brezillon wrote: >>>>> Move Samsung specific initialization and detection logic into >>>>> nand_samsung.c. This is part of the "separate vendor specific code from >>>>> core" cleanup process. >>>>> >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>>> >>>> [...] >>>> >>>>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c >>>>> index b3a332f37e14..05e9366696c9 100644 >>>>> --- a/drivers/mtd/nand/nand_ids.c >>>>> +++ b/drivers/mtd/nand/nand_ids.c >>>>> @@ -10,7 +10,7 @@ >>>>> #include <linux/mtd/nand.h> >>>>> #include <linux/sizes.h> >>>>> >>>>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS >>>>> +#define LP_OPTIONS 0 >>>>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) >>>>> >>>>> #define SP_OPTIONS NAND_NEED_READRDY >>>>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { >>>>> }; >>>>> >>>>> /* Manufacturer IDs */ >>>>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; >>>> >>>> Is the extern needed ? >>> >>> Yes, unless you have another solution. If you remove the extern keyword >>> you just redeclare samsung_nand_manuf_ops here, which is not what we >>> want. >> >> Maybe some accessor function can help ? >> > > You mean, in nand_ids.c > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops(); > > struct nand_manufacturers nand_manuf_ids[] = { > ... > {NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops}, > ... > }; > > and then, in nand_samsung.c > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops() > { > return &samsung_nand_mafuf_ops; > } Yeah, something like that. > What's the point of this extra indirection? I mean, in both cases you > use a symbol that is not part of the same source file, so you'll have > to define this symbol (using a function prototype or an extern object > definition). > Is this all about fixing checkpatch warnings, or do you have a problem > with objects shared between different source files? The later, separating this with an accessor function feels a bit cleaner to me than using extern foo. > Now, I agree that the current approach is not ideal. A real improvement > would be to let the NAND manufacturer drivers (nand_<vendor>.c) register > themselves to the core. Something similar to CLK_OF_DECLARE() or > IRQCHIP_DECLARE() for example. But that means creating a dedicated > section for the nand_manufs_id table, and it's a lot more invasive than > the current approach. Well this would be awesome, but this can also be done later. I presume you'll get to it eventually anyway, as soon as you'll be annoyed enough with the current ugly-ish implementation.
On Tue, 10 Jan 2017 20:00:28 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 01/07/2017 08:49 AM, Boris Brezillon wrote: > > On Sat, 7 Jan 2017 00:53:24 +0100 > > Marek Vasut <marek.vasut@gmail.com> wrote: > > > >> On 01/04/2017 06:08 PM, Boris Brezillon wrote: > >>> On Wed, 4 Jan 2017 16:14:07 +0100 > >>> Marek Vasut <marek.vasut@gmail.com> wrote: > >>> > >>>> On 01/03/2017 02:01 PM, Boris Brezillon wrote: > >>>>> Move Samsung specific initialization and detection logic into > >>>>> nand_samsung.c. This is part of the "separate vendor specific code from > >>>>> core" cleanup process. > >>>>> > >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > >>>> > >>>> [...] > >>>> > >>>>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c > >>>>> index b3a332f37e14..05e9366696c9 100644 > >>>>> --- a/drivers/mtd/nand/nand_ids.c > >>>>> +++ b/drivers/mtd/nand/nand_ids.c > >>>>> @@ -10,7 +10,7 @@ > >>>>> #include <linux/mtd/nand.h> > >>>>> #include <linux/sizes.h> > >>>>> > >>>>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS > >>>>> +#define LP_OPTIONS 0 > >>>>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) > >>>>> > >>>>> #define SP_OPTIONS NAND_NEED_READRDY > >>>>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { > >>>>> }; > >>>>> > >>>>> /* Manufacturer IDs */ > >>>>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; > >>>> > >>>> Is the extern needed ? > >>> > >>> Yes, unless you have another solution. If you remove the extern keyword > >>> you just redeclare samsung_nand_manuf_ops here, which is not what we > >>> want. > >> > >> Maybe some accessor function can help ? > >> > > > > You mean, in nand_ids.c > > > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops(); > > > > struct nand_manufacturers nand_manuf_ids[] = { > > ... > > {NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops}, > > ... > > }; > > > > and then, in nand_samsung.c > > > > const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops() > > { > > return &samsung_nand_mafuf_ops; > > } > > Yeah, something like that. > > > What's the point of this extra indirection? I mean, in both cases you > > use a symbol that is not part of the same source file, so you'll have > > to define this symbol (using a function prototype or an extern object > > definition). > > Is this all about fixing checkpatch warnings, or do you have a problem > > with objects shared between different source files? > > The later, separating this with an accessor function feels a bit cleaner > to me than using extern foo. > > > Now, I agree that the current approach is not ideal. A real improvement > > would be to let the NAND manufacturer drivers (nand_<vendor>.c) register > > themselves to the core. Something similar to CLK_OF_DECLARE() or > > IRQCHIP_DECLARE() for example. But that means creating a dedicated > > section for the nand_manufs_id table, and it's a lot more invasive than > > the current approach. > > Well this would be awesome, but this can also be done later. I presume > you'll get to it eventually anyway, as soon as you'll be annoyed enough > with the current ugly-ish implementation. > If we plan to rework it this way, I'd like to keep the existing approach (with the extern) to avoid changing the prototype of nand_manufacturer once again when we rework the nand_manufacturer registration logic. Also note that in v6 I'm keeping a pointer to the nand_manfucturer object in nand_chip, so that if we ever need to print the manufacturer name we don't have to search again in the NAND manufacturer table. After this rework, I no longer store the manufacturer_ops directly in nand_chip, and have to access them by doing chip->manufacturer.desc->ops->xxx. Which means, with your solution, I'll have to do ops = nand_get_manufacturer_ops(chip->manufacturer.desc); ops->xxx(); instead of chip->manufacturer.desc->ops->xxx();
On 01/11/2017 08:57 AM, Boris Brezillon wrote: > On Tue, 10 Jan 2017 20:00:28 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 01/07/2017 08:49 AM, Boris Brezillon wrote: >>> On Sat, 7 Jan 2017 00:53:24 +0100 >>> Marek Vasut <marek.vasut@gmail.com> wrote: >>> >>>> On 01/04/2017 06:08 PM, Boris Brezillon wrote: >>>>> On Wed, 4 Jan 2017 16:14:07 +0100 >>>>> Marek Vasut <marek.vasut@gmail.com> wrote: >>>>> >>>>>> On 01/03/2017 02:01 PM, Boris Brezillon wrote: >>>>>>> Move Samsung specific initialization and detection logic into >>>>>>> nand_samsung.c. This is part of the "separate vendor specific code from >>>>>>> core" cleanup process. >>>>>>> >>>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>>>>> >>>>>> [...] >>>>>> >>>>>>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c >>>>>>> index b3a332f37e14..05e9366696c9 100644 >>>>>>> --- a/drivers/mtd/nand/nand_ids.c >>>>>>> +++ b/drivers/mtd/nand/nand_ids.c >>>>>>> @@ -10,7 +10,7 @@ >>>>>>> #include <linux/mtd/nand.h> >>>>>>> #include <linux/sizes.h> >>>>>>> >>>>>>> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS >>>>>>> +#define LP_OPTIONS 0 >>>>>>> #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) >>>>>>> >>>>>>> #define SP_OPTIONS NAND_NEED_READRDY >>>>>>> @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { >>>>>>> }; >>>>>>> >>>>>>> /* Manufacturer IDs */ >>>>>>> +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; >>>>>> >>>>>> Is the extern needed ? >>>>> >>>>> Yes, unless you have another solution. If you remove the extern keyword >>>>> you just redeclare samsung_nand_manuf_ops here, which is not what we >>>>> want. >>>> >>>> Maybe some accessor function can help ? >>>> >>> >>> You mean, in nand_ids.c >>> >>> const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops(); >>> >>> struct nand_manufacturers nand_manuf_ids[] = { >>> ... >>> {NAND_MFR_SAMSUNG, "Samsung", get_samsung_nand_mafuf_ops}, >>> ... >>> }; >>> >>> and then, in nand_samsung.c >>> >>> const struct nand_manufacturer_ops *get_samsung_nand_mafuf_ops() >>> { >>> return &samsung_nand_mafuf_ops; >>> } >> >> Yeah, something like that. >> >>> What's the point of this extra indirection? I mean, in both cases you >>> use a symbol that is not part of the same source file, so you'll have >>> to define this symbol (using a function prototype or an extern object >>> definition). >>> Is this all about fixing checkpatch warnings, or do you have a problem >>> with objects shared between different source files? >> >> The later, separating this with an accessor function feels a bit cleaner >> to me than using extern foo. >> >>> Now, I agree that the current approach is not ideal. A real improvement >>> would be to let the NAND manufacturer drivers (nand_<vendor>.c) register >>> themselves to the core. Something similar to CLK_OF_DECLARE() or >>> IRQCHIP_DECLARE() for example. But that means creating a dedicated >>> section for the nand_manufs_id table, and it's a lot more invasive than >>> the current approach. >> >> Well this would be awesome, but this can also be done later. I presume >> you'll get to it eventually anyway, as soon as you'll be annoyed enough >> with the current ugly-ish implementation. >> > > If we plan to rework it this way, I'd like to keep the existing > approach (with the extern) to avoid changing the prototype of > nand_manufacturer once again when we rework the nand_manufacturer > registration logic. > > Also note that in v6 I'm keeping a pointer to the nand_manfucturer > object in nand_chip, so that if we ever need to print the manufacturer > name we don't have to search again in the NAND manufacturer table. > After this rework, I no longer store the manufacturer_ops directly in > nand_chip, and have to access them by doing > chip->manufacturer.desc->ops->xxx. > > Which means, with your solution, I'll have to do > > ops = nand_get_manufacturer_ops(chip->manufacturer.desc); > ops->xxx(); > > instead of > > chip->manufacturer.desc->ops->xxx(); > All right, I think we can live with this either way.
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index bfd5d12b9ade..d4b90b0f879e 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -61,3 +61,4 @@ obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o +nand-objs += nand_samsung.o diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 8f80faa57984..0f0fa13e9448 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3792,48 +3792,13 @@ void nand_decode_ext_id(struct nand_chip *chip) /* * Field definitions are in the following datasheets: * Old style (4,5 byte ID): Samsung K9GAG08U0M (p.32) - * New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44) * Hynix MLC (6 byte ID): Hynix H27UBG8T2B (p.22) * * Check for ID length, non-zero 6th byte, cell type, and Hynix/Samsung * ID to decide what to do. */ - if (id_len == 6 && id_data[0] == NAND_MFR_SAMSUNG && - !nand_is_slc(chip) && id_data[5] != 0x00) { - /* Calc pagesize */ - mtd->writesize = 2048 << (extid & 0x03); - extid >>= 2; - /* Calc oobsize */ - switch (((extid >> 2) & 0x04) | (extid & 0x03)) { - case 1: - mtd->oobsize = 128; - break; - case 2: - mtd->oobsize = 218; - break; - case 3: - mtd->oobsize = 400; - break; - case 4: - mtd->oobsize = 436; - break; - case 5: - mtd->oobsize = 512; - break; - case 6: - mtd->oobsize = 640; - break; - case 7: - default: /* Other cases are "reserved" (unknown) */ - mtd->oobsize = 1024; - break; - } - extid >>= 2; - /* Calc blocksize */ - mtd->erasesize = (128 * 1024) << - (((extid >> 1) & 0x04) | (extid & 0x03)); - } else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX && - !nand_is_slc(chip)) { + if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX && + !nand_is_slc(chip)) { unsigned int tmp; /* Calc pagesize */ @@ -3961,13 +3926,10 @@ static void nand_decode_bbm_options(struct nand_chip *chip) * Micron devices with 2KiB pages and on SLC Samsung, Hynix, Toshiba, * AMD/Spansion, and Macronix. All others scan only the first page. */ - if (!nand_is_slc(chip) && - (maf_id == NAND_MFR_SAMSUNG || - maf_id == NAND_MFR_HYNIX)) + if (!nand_is_slc(chip) && maf_id == NAND_MFR_HYNIX) chip->bbt_options |= NAND_BBT_SCANLASTPAGE; else if ((nand_is_slc(chip) && - (maf_id == NAND_MFR_SAMSUNG || - maf_id == NAND_MFR_HYNIX || + (maf_id == NAND_MFR_HYNIX || maf_id == NAND_MFR_TOSHIBA || maf_id == NAND_MFR_AMD || maf_id == NAND_MFR_MACRONIX)) || @@ -4149,12 +4111,6 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) /* Get chip options */ chip->options |= type->options; - /* - * Check if chip is not a Samsung device. Do not clear the - * options for chips which do not have an extended id. - */ - if (maf_id != NAND_MFR_SAMSUNG && !type->pagesize) - chip->options &= ~NAND_SAMSUNG_LP_OPTIONS; ident_done: if (chip->options & NAND_BUSWIDTH_AUTO) { diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c index b3a332f37e14..05e9366696c9 100644 --- a/drivers/mtd/nand/nand_ids.c +++ b/drivers/mtd/nand/nand_ids.c @@ -10,7 +10,7 @@ #include <linux/mtd/nand.h> #include <linux/sizes.h> -#define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS +#define LP_OPTIONS 0 #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16) #define SP_OPTIONS NAND_NEED_READRDY @@ -169,10 +169,12 @@ struct nand_flash_dev nand_flash_ids[] = { }; /* Manufacturer IDs */ +extern const struct nand_manufacturer_ops samsung_nand_manuf_ops; + struct nand_manufacturers nand_manuf_ids[] = { {NAND_MFR_TOSHIBA, "Toshiba"}, {NAND_MFR_ESMT, "ESMT"}, - {NAND_MFR_SAMSUNG, "Samsung"}, + {NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops}, {NAND_MFR_FUJITSU, "Fujitsu"}, {NAND_MFR_NATIONAL, "National"}, {NAND_MFR_RENESAS, "Renesas"}, diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c new file mode 100644 index 000000000000..863be01a2232 --- /dev/null +++ b/drivers/mtd/nand/nand_samsung.c @@ -0,0 +1,89 @@ +/* + * Copyright (C) 2013 Boris Brezillon <boris.brezillon@free-electrons.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/mtd/nand.h> + +static void samsung_nand_decode_id(struct nand_chip *chip) +{ + struct mtd_info *mtd = nand_to_mtd(chip); + + /* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44) */ + if (chip->id.len == 6 && !nand_is_slc(chip) && + chip->id.data[5] != 0x00) { + u8 extid = chip->id.data[3]; + + /* Get pagesize */ + mtd->writesize = 2048 << (extid & 0x03); + + extid >>= 2; + + /* Get oobsize */ + switch (((extid >> 2) & 0x4) | (extid & 0x3)) { + case 1: + mtd->oobsize = 128; + break; + case 2: + mtd->oobsize = 218; + break; + case 3: + mtd->oobsize = 400; + break; + case 4: + mtd->oobsize = 436; + break; + case 5: + mtd->oobsize = 512; + break; + case 6: + mtd->oobsize = 640; + break; + default: + /* + * We should never reach this case, but if that + * happens, this probably means Samsung decided to use + * a different extended ID format, and we should find + * a way to support it. + */ + WARN(1, "Invalid OOB size value"); + break; + } + + /* Get blocksize */ + extid >>= 2; + mtd->erasesize = (128 * 1024) << + (((extid >> 1) & 0x04) | (extid & 0x03)); + } else { + nand_decode_ext_id(chip); + } +} + +static int samsung_nand_init(struct nand_chip *chip) +{ + struct mtd_info *mtd = nand_to_mtd(chip); + + if (mtd->writesize > 512) + chip->options |= NAND_SAMSUNG_LP_OPTIONS; + + if (!nand_is_slc(chip)) + chip->bbt_options |= NAND_BBT_SCANLASTPAGE; + else + chip->bbt_options |= NAND_BBT_SCAN2NDPAGE; + + return 0; +} + +const struct nand_manufacturer_ops samsung_nand_manuf_ops = { + .detect = samsung_nand_decode_id, + .init = samsung_nand_init, +};
Move Samsung specific initialization and detection logic into nand_samsung.c. This is part of the "separate vendor specific code from core" cleanup process. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/nand_base.c | 52 ++---------------------- drivers/mtd/nand/nand_ids.c | 6 ++- drivers/mtd/nand/nand_samsung.c | 89 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 50 deletions(-) create mode 100644 drivers/mtd/nand/nand_samsung.c