| Message ID | 1410033389-32357-3-git-send-email-ezequiel@vanguardiasur.com.ar |
|---|---|
| State | Superseded |
| Headers | show |
+ linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c Hi Ezequiel, On Sunday 07 September 2014 01:26 AM, Ezequiel Garcia wrote: > The current code abuses ifdefs to determine if the selected ECC scheme > is supported by the running kernel. As a result the code is hard to read, > and it also fails to load as a module. > > This commit removes all the ifdefs and instead introduces a function > omap2_nand_ecc_check() to check if the ECC is supported by using > IS_ENABLED(CONFIG_xxx). > Yup, true. Thanks for cleaning up OMAP2 NAND driver, but few feedbacks. Just for record, current version of omap2.c NAND driver has so many #ifdef because: (1) Many ECC schemes like HAM1 and BCH4_xx, BCH8_SW are supported only for compatibility with old platforms like OMAP3. And these legacy schemes have significant amount of code and data foot-print (as compared to whole driver), so it does not make sense to keep driver bulky for all new platforms which do not use legacy ECC schemes. (2) Specifically, BCH4_SW and BCH8_SW ECC schemes used software library /lib/bch.c "directly". Due to which if BCH4_SW or BCH8_SW was include in code, /lib/bch.c was included implicitly. Later from following following commit nand_bch.c wrapper was used which took care to exclude /lib/bch.c when CONFIG_MTD_NAND_ECC_BCH was not defined, but #ifdef continued :-( (My bad, I accept ..) commit 32d42a855a6f1445373f3d680e9125344aca294c mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c > Since IS_ENABLED() is true when a config is =y or =m, this change fixes the > module so it can be loaded with no issues. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > drivers/mtd/nand/omap2.c | 134 +++++++++++++++++++++----------------- > include/linux/platform_data/elm.h | 14 ++++ > 2 files changed, 87 insertions(+), 61 deletions(-) [...] > @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > /** > * is_elm_present - checks for presence of ELM module by scanning DT nodes > * @omap_nand_info: NAND device structure containing platform data > - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16 > */ > -static int is_elm_present(struct omap_nand_info *info, > - struct device_node *elm_node, enum bch_ecc bch_type) > +static bool is_elm_present(struct omap_nand_info *info, > + struct device_node *elm_node) > { > struct platform_device *pdev; > - struct nand_ecc_ctrl *ecc = &info->nand.ecc; > - int err; > + > /* check whether elm-id is passed via DT */ > if (!elm_node) { > - pr_err("nand: error: ELM DT node not found\n"); > - return -ENODEV; > + dev_err(&info->pdev->dev, "ELM devicetree node not found\n"); > + return false; > } > pdev = of_find_device_by_node(elm_node); > /* check whether ELM device is registered */ > if (!pdev) { > - pr_err("nand: error: ELM device not found\n"); > - return -ENODEV; > + dev_err(&info->pdev->dev, "ELM device not found\n"); > + return false; > } Thanks, for this clean-up of replacing pr_err() to dev_err(). Just that if you can resend this as separate patch then I'll surely Ack it. [...] > +static bool omap2_nand_ecc_check(struct omap_nand_info *info, > + struct omap_nand_platform_data *pdata) > +{ > + bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm; > + > + switch (info->ecc_opt) { > + case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: > + case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > + ecc_needs_omap_bch = false; > + ecc_needs_bch = true; > + ecc_needs_elm = false; > + break; > + case OMAP_ECC_BCH4_CODE_HW: > + case OMAP_ECC_BCH8_CODE_HW: > + case OMAP_ECC_BCH16_CODE_HW: > + ecc_needs_omap_bch = true; > + ecc_needs_bch = false; > + ecc_needs_elm = true; > + break; > + default: > + ecc_needs_omap_bch = false; > + ecc_needs_bch = false; > + ecc_needs_elm = false; > + break; > + } > + > + if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) { > + dev_err(&info->pdev->dev, > + "CONFIG_MTD_NAND_ECC_BCH not enabled\n"); > + return false; > + } > + if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) { > + dev_err(&info->pdev->dev, > + "CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > + return false; > + } > + if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { > + dev_err(&info->pdev->dev, "ELM not available\n"); > + return false; > + } > + > + return true; > } Actually this new function is not required. We already have this check in GPMC driver when we select the ecc-scheme. Plz refer: File: /arch/arm/mach-omap2/gpmc.c @@ static int gpmc_probe_nand_child(...) ... /* select ecc-scheme for NAND */ is_elm_enabled() is a misnomer here. It actually does not check for ELM DTS node, because that check is already done in GPMC driver. It rather checks if ELM device is correctly binded and driver "initialized" before NAND driver is probed. Though ELM is not required during NAND probe because there is no error checking/correction required during NAND probe, but this again comes from the history of maintaining back-ward compatibility with board-file (non DTS) approach. Once we move to all DTS approach may be this can be removed (but on case-by case basis). [...] > diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h > index 780d1e9..25d1bca 100644 > --- a/include/linux/platform_data/elm.h > +++ b/include/linux/platform_data/elm.h > @@ -42,8 +42,22 @@ struct elm_errorvec { > int error_loc[16]; > }; > > +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) > void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > struct elm_errorvec *err_vec); > int elm_config(struct device *dev, enum bch_ecc bch_type, > int ecc_steps, int ecc_step_size, int ecc_syndrome_size); > +#else > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > + struct elm_errorvec *err_vec) > +{ > +} > + > +int elm_config(struct device *dev, enum bch_ecc bch_type, > + int ecc_steps, int ecc_step_size, int ecc_syndrome_size) > +{ > + return -ENOSYS; > +} > +#endif /* CONFIG_MTD_NAND_ECC_BCH */ > + > #endif /* __ELM_H */ > If I'm not wrong, this is all you need in this patch empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already handled in /include/linux/mtd/nand_bch.h So, after this change, you can most of #ifdefs and IS_ENABLED() and this patch should be simplified. with regards, pekon ------------------------ Powered by BigRock.com
On 07 Sep 02:40 AM, pekon wrote: > + linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c > > Hi Ezequiel, > > On Sunday 07 September 2014 01:26 AM, Ezequiel Garcia wrote: > >The current code abuses ifdefs to determine if the selected ECC scheme > >is supported by the running kernel. As a result the code is hard to read, > >and it also fails to load as a module. > > > >This commit removes all the ifdefs and instead introduces a function > >omap2_nand_ecc_check() to check if the ECC is supported by using > >IS_ENABLED(CONFIG_xxx). > > > Yup, true. > Thanks for cleaning up OMAP2 NAND driver, but few feedbacks. > First of all, this is no cleaning. The driver fails to load as a module over here. Unless I'm missed something, ifdef seemed false when CONFIG_xxx=m. > Just for record, current version of omap2.c NAND driver has so > many #ifdef because: > [..] I can't think of any valid excuses for cluttering the code with ifdefs like this. There are several ways of doing things cleaner, by grouping code differently. [..] > >+ } > >+ if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { > >+ dev_err(&info->pdev->dev, "ELM not available\n"); > >+ return false; > >+ } > >+ > >+ return true; > > } > > Actually this new function is not required. [snip] > If I'm not wrong, this is all you need in this patch > empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already > handled in /include/linux/mtd/nand_bch.h > > So, after this change, you can most of #ifdefs and IS_ENABLED() > and this patch should be simplified. > OK, I'll take a look. Thanks for the suggestion,
On 07 Sep 02:40 AM, pekon wrote: [..] > >+static bool omap2_nand_ecc_check(struct omap_nand_info *info, > >+ struct omap_nand_platform_data *pdata) > >+{ > >+ bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm; > >+ > >+ switch (info->ecc_opt) { > >+ case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: > >+ case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > >+ ecc_needs_omap_bch = false; > >+ ecc_needs_bch = true; > >+ ecc_needs_elm = false; > >+ break; > >+ case OMAP_ECC_BCH4_CODE_HW: > >+ case OMAP_ECC_BCH8_CODE_HW: > >+ case OMAP_ECC_BCH16_CODE_HW: > >+ ecc_needs_omap_bch = true; > >+ ecc_needs_bch = false; > >+ ecc_needs_elm = true; > >+ break; > >+ default: > >+ ecc_needs_omap_bch = false; > >+ ecc_needs_bch = false; > >+ ecc_needs_elm = false; > >+ break; > >+ } > >+ > >+ if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) { > >+ dev_err(&info->pdev->dev, > >+ "CONFIG_MTD_NAND_ECC_BCH not enabled\n"); > >+ return false; > >+ } > >+ if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) { > >+ dev_err(&info->pdev->dev, > >+ "CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > >+ return false; > >+ } > >+ if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { > >+ dev_err(&info->pdev->dev, "ELM not available\n"); > >+ return false; > >+ } > >+ > >+ return true; > > } > > Actually this new function is not required. > We already have this check in GPMC driver when we select the ecc-scheme. > Plz refer: File: /arch/arm/mach-omap2/gpmc.c > @@ static int gpmc_probe_nand_child(...) > ... > /* select ecc-scheme for NAND */ > > is_elm_enabled() is a misnomer here. It actually does not check > for ELM DTS node, because that check is already done in GPMC driver. > It rather checks if ELM device is correctly binded and driver > "initialized" before NAND driver is probed. > > Though ELM is not required during NAND probe because there is no error > checking/correction required during NAND probe, but this again comes > from the history of maintaining back-ward compatibility with board-file > (non DTS) approach. Once we move to all DTS approach may be this can be > removed (but on case-by case basis). > > [...] > > >diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h > >index 780d1e9..25d1bca 100644 > >--- a/include/linux/platform_data/elm.h > >+++ b/include/linux/platform_data/elm.h > >@@ -42,8 +42,22 @@ struct elm_errorvec { > > int error_loc[16]; > > }; > > > >+#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) > > void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > > struct elm_errorvec *err_vec); > > int elm_config(struct device *dev, enum bch_ecc bch_type, > > int ecc_steps, int ecc_step_size, int ecc_syndrome_size); > >+#else > >+void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > >+ struct elm_errorvec *err_vec) > >+{ > >+} > >+ > >+int elm_config(struct device *dev, enum bch_ecc bch_type, > >+ int ecc_steps, int ecc_step_size, int ecc_syndrome_size) > >+{ > >+ return -ENOSYS; > >+} > >+#endif /* CONFIG_MTD_NAND_ECC_BCH */ > >+ > > #endif /* __ELM_H */ > > > If I'm not wrong, this is all you need in this patch > empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already > handled in /include/linux/mtd/nand_bch.h > > So, after this change, you can most of #ifdefs and IS_ENABLED() > and this patch should be simplified. > If I understand your proposal correctly you are suggesting to drop the checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM devicetree node. However, I'd say that change is even more invasive than this one. This commit merely replaces the current #ifdefs check with IS_ENABLED and tries to do so in a cleaner way. This is done on purpose, to keep the current behavior as much as possible. In addition, if we don't check for the configs explicitly at probe time, we would only defer the error until some later point, for instance in the call to nand_chip->ecc.correct(). I don't think that's user-friendly.
Hi Ezequiel, On Sunday 07 September 2014 04:47 AM, Ezequiel Garcia wrote: > On 07 Sep 02:40 AM, pekon wrote: >> [...] >> >>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h >>> index 780d1e9..25d1bca 100644 >>> --- a/include/linux/platform_data/elm.h >>> +++ b/include/linux/platform_data/elm.h >>> @@ -42,8 +42,22 @@ struct elm_errorvec { >>> int error_loc[16]; >>> }; >>> >>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) >>> void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, >>> struct elm_errorvec *err_vec); >>> int elm_config(struct device *dev, enum bch_ecc bch_type, >>> int ecc_steps, int ecc_step_size, int ecc_syndrome_size); >>> +#else >>> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, >>> + struct elm_errorvec *err_vec) >>> +{ >>> +} >>> + >>> +int elm_config(struct device *dev, enum bch_ecc bch_type, >>> + int ecc_steps, int ecc_step_size, int ecc_syndrome_size) >>> +{ >>> + return -ENOSYS; >>> +} >>> +#endif /* CONFIG_MTD_NAND_ECC_BCH */ >>> + >>> #endif /* __ELM_H */ >>> >> If I'm not wrong, this is all you need in this patch >> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already >> handled in /include/linux/mtd/nand_bch.h >> >> So, after this change, you can most of #ifdefs and IS_ENABLED() >> and this patch should be simplified. >> > > If I understand your proposal correctly you are suggesting to drop the > checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM > devicetree node. > Absolutely .. (1) Just the change marked above handles whether CONFIG_MTD_NAND_OMAP_BCH is defined or not. (2) Same way nand_bch.c takes care whether CONFIG_MTD_NAND_ECC_BCH is defined or not. (3) And gpmc.c @@gpmc_probe_nand_child(...) already checks for ELM DTS node. > However, I'd say that change is even more invasive than this one. This commit > merely replaces the current #ifdefs check with IS_ENABLED and tries to do so > in a cleaner way. > I would say you can get rid of most of #ifdefs and IS_ENABLED() in this patch itself. And testing it that should be easy, you just need to compile with CONFIG_MTD_NAND_xxx_BCH enabled one at a time. > This is done on purpose, to keep the current behavior as much as possible. > > In addition, if we don't check for the configs explicitly at probe time, > we would only defer the error until some later point, for instance in the call to > nand_chip->ecc.correct(). I don't think that's user-friendly. > As ECC-scheme is selected in GPMC driver based on DTS settings, so any mis-match is easily handled there. Moreover, error will occur when we change ECC-scheme on-the-fly, which is still not supported by framework, and require many other updates if we plan to support that in near future. So considering ECC-scheme as static configuration is a safe assumption. But surely you can drop new check in omap2_nand_ecc_check(), which is already covered in @@gpmc_probe_nand_child(...) However, I leave it to you and rogerq@ti.com (as he currently maintains OMAP NAND driver from TI side) to decide how to go about it. with regards, pekon ------------------------ Powered by BigRock.com
On 07 Sep 03:05 PM, pekon wrote: [..] > As ECC-scheme is selected in GPMC driver based on DTS settings, so > any mis-match is easily handled there. > Moreover, error will occur when we change ECC-scheme on-the-fly, > which is still not supported by framework, and require many other > updates if we plan to support that in near future. > So considering ECC-scheme as static configuration is a safe assumption. > > But surely you can drop new check in omap2_nand_ecc_check(), which > is already covered in @@gpmc_probe_nand_child(...) > > However, I leave it to you and rogerq@ti.com (as he currently > maintains OMAP NAND driver from TI side) to decide how to go about it. > As far as I can see, your proposal only cover the devicetree-probed case and it won't work for legacy boards. Given it seems we still support legacy, I'd say your proposal would break things. Not to mention that I still prefer to fail at probe time if the driver cannot deal with the selected ECC, and that's exactly what omap2_nand_ecc_check() does.
On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: > The current code abuses ifdefs to determine if the selected ECC scheme > is supported by the running kernel. As a result the code is hard to read, > and it also fails to load as a module. > > This commit removes all the ifdefs and instead introduces a function > omap2_nand_ecc_check() to check if the ECC is supported by using > IS_ENABLED(CONFIG_xxx). > > Since IS_ENABLED() is true when a config is =y or =m, this change fixes the > module so it can be loaded with no issues. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Didn't apply cleanly on 3.17-rc4. Needs a rebase? > --- > drivers/mtd/nand/omap2.c | 134 +++++++++++++++++++++----------------- > include/linux/platform_data/elm.h | 14 ++++ > 2 files changed, 87 insertions(+), 61 deletions(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index 1170389..987dd94 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -136,7 +136,6 @@ > > #define BADBLOCK_MARKER_LENGTH 2 > > -#ifdef CONFIG_MTD_NAND_OMAP_BCH > static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, > 0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78, > 0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93, > @@ -144,7 +143,6 @@ static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, > static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc, > 0xac, 0x6b, 0xff, 0x99, 0x7b}; > static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10}; > -#endif > > /* oob info generated runtime depending on ecc algorithm and layout selected */ > static struct nand_ecclayout omap_oobinfo; > @@ -1292,7 +1290,6 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, > return 0; > } > > -#ifdef CONFIG_MTD_NAND_OMAP_BCH > /** > * erased_sector_bitflips - count bit flips > * @data: data sector buffer > @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, > /** > * is_elm_present - checks for presence of ELM module by scanning DT nodes > * @omap_nand_info: NAND device structure containing platform data > - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16 > */ > -static int is_elm_present(struct omap_nand_info *info, > - struct device_node *elm_node, enum bch_ecc bch_type) > +static bool is_elm_present(struct omap_nand_info *info, > + struct device_node *elm_node) > { > struct platform_device *pdev; > - struct nand_ecc_ctrl *ecc = &info->nand.ecc; > - int err; > + > /* check whether elm-id is passed via DT */ > if (!elm_node) { > - pr_err("nand: error: ELM DT node not found\n"); > - return -ENODEV; > + dev_err(&info->pdev->dev, "ELM devicetree node not found\n"); > + return false; > } > pdev = of_find_device_by_node(elm_node); > /* check whether ELM device is registered */ > if (!pdev) { > - pr_err("nand: error: ELM device not found\n"); > - return -ENODEV; > + dev_err(&info->pdev->dev, "ELM device not found\n"); > + return false; > } > /* ELM module available, now configure it */ > info->elm_dev = &pdev->dev; > - err = elm_config(info->elm_dev, bch_type, > - (info->mtd.writesize / ecc->size), ecc->size, ecc->bytes); > + return true; > +} > > - return err; > +static bool omap2_nand_ecc_check(struct omap_nand_info *info, > + struct omap_nand_platform_data *pdata) > +{ I like the idea of this function and it being called in probe. It would be also be nice (maybe later) to get rid of all the ECC decision making done in gpmc_probe_nand_child() based on presence/absence of the elm_of_node. I guess we need to add new DT entries for "bch4sw" and "bch8sw" so that DT/platform data can explicitly specify what it needs. I don't see how ELM works with the non-DT boot way as there is no elm_of_node there. Also, the elm_of_node should not be set via platform data or in gpmc.c but parsed here directly in the NAND driver. But changes should be done in a separate patch. Just brought them out here for discussion. > + bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm; > + > + switch (info->ecc_opt) { > + case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: > + case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > + ecc_needs_omap_bch = false; > + ecc_needs_bch = true; > + ecc_needs_elm = false; > + break; > + case OMAP_ECC_BCH4_CODE_HW: > + case OMAP_ECC_BCH8_CODE_HW: > + case OMAP_ECC_BCH16_CODE_HW: > + ecc_needs_omap_bch = true; > + ecc_needs_bch = false; > + ecc_needs_elm = true; > + break; > + default: > + ecc_needs_omap_bch = false; > + ecc_needs_bch = false; > + ecc_needs_elm = false; > + break; > + } > + > + if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) { > + dev_err(&info->pdev->dev, > + "CONFIG_MTD_NAND_ECC_BCH not enabled\n"); > + return false; > + } > + if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) { > + dev_err(&info->pdev->dev, > + "CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > + return false; > + } > + if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { > + dev_err(&info->pdev->dev, "ELM not available\n"); > + return false; > + } > + > + return true; > } > -#endif /* CONFIG_MTD_NAND_ECC_BCH */ > > static int omap_nand_probe(struct platform_device *pdev) > { > @@ -1797,6 +1832,11 @@ static int omap_nand_probe(struct platform_device *pdev) > goto return_error; > } > > + if (!omap2_nand_ecc_check(info, pdata)) { > + err = -EINVAL; > + goto return_error; > + } > + > /* populate MTD interface based on ECC scheme */ > nand_chip->ecc.layout = &omap_oobinfo; > ecclayout = &omap_oobinfo; > @@ -1826,7 +1866,6 @@ static int omap_nand_probe(struct platform_device *pdev) > break; > > case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: > -#ifdef CONFIG_MTD_NAND_ECC_BCH > pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n"); > nand_chip->ecc.mode = NAND_ECC_HW; > nand_chip->ecc.size = 512; > @@ -1858,14 +1897,8 @@ static int omap_nand_probe(struct platform_device *pdev) > err = -EINVAL; > } > break; > -#else > - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); > - err = -EINVAL; > - goto return_error; > -#endif > > case OMAP_ECC_BCH4_CODE_HW: > -#ifdef CONFIG_MTD_NAND_OMAP_BCH > pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n"); > nand_chip->ecc.mode = NAND_ECC_HW; > nand_chip->ecc.size = 512; > @@ -1887,21 +1920,15 @@ static int omap_nand_probe(struct platform_device *pdev) > /* reserved marker already included in ecclayout->eccbytes */ > ecclayout->oobfree->offset = > ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; > - /* This ECC scheme requires ELM H/W block */ > - if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) { > - pr_err("nand: error: could not initialize ELM\n"); > - err = -ENODEV; > + > + err = elm_config(info->elm_dev, BCH4_ECC, > + info->mtd.writesize / nand_chip->ecc.size, > + nand_chip->ecc.size, nand_chip->ecc.bytes); > + if (err < 0) > goto return_error; > - } > break; > -#else > - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > - err = -EINVAL; > - goto return_error; > -#endif > > case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: > -#ifdef CONFIG_MTD_NAND_ECC_BCH > pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n"); > nand_chip->ecc.mode = NAND_ECC_HW; > nand_chip->ecc.size = 512; > @@ -1934,14 +1961,8 @@ static int omap_nand_probe(struct platform_device *pdev) > goto return_error; > } > break; > -#else > - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); > - err = -EINVAL; > - goto return_error; > -#endif > > case OMAP_ECC_BCH8_CODE_HW: > -#ifdef CONFIG_MTD_NAND_OMAP_BCH > pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n"); > nand_chip->ecc.mode = NAND_ECC_HW; > nand_chip->ecc.size = 512; > @@ -1953,12 +1974,13 @@ static int omap_nand_probe(struct platform_device *pdev) > nand_chip->ecc.calculate = omap_calculate_ecc_bch; > nand_chip->ecc.read_page = omap_read_page_bch; > nand_chip->ecc.write_page = omap_write_page_bch; > - /* This ECC scheme requires ELM H/W block */ > - err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC); > - if (err < 0) { > - pr_err("nand: error: could not initialize ELM\n"); > + > + err = elm_config(info->elm_dev, BCH8_ECC, > + info->mtd.writesize / nand_chip->ecc.size, > + nand_chip->ecc.size, nand_chip->ecc.bytes); > + if (err < 0) > goto return_error; > - } > + > /* define ECC layout */ > ecclayout->eccbytes = nand_chip->ecc.bytes * > (mtd->writesize / > @@ -1970,14 +1992,8 @@ static int omap_nand_probe(struct platform_device *pdev) > ecclayout->oobfree->offset = > ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; > break; > -#else > - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > - err = -EINVAL; > - goto return_error; > -#endif > > case OMAP_ECC_BCH16_CODE_HW: > -#ifdef CONFIG_MTD_NAND_OMAP_BCH > pr_info("using OMAP_ECC_BCH16_CODE_HW ECC scheme\n"); > nand_chip->ecc.mode = NAND_ECC_HW; > nand_chip->ecc.size = 512; > @@ -1988,12 +2004,13 @@ static int omap_nand_probe(struct platform_device *pdev) > nand_chip->ecc.calculate = omap_calculate_ecc_bch; > nand_chip->ecc.read_page = omap_read_page_bch; > nand_chip->ecc.write_page = omap_write_page_bch; > - /* This ECC scheme requires ELM H/W block */ > - err = is_elm_present(info, pdata->elm_of_node, BCH16_ECC); > - if (err < 0) { > - pr_err("ELM is required for this ECC scheme\n"); > + > + err = elm_config(info->elm_dev, BCH16_ECC, > + info->mtd.writesize / nand_chip->ecc.size, > + nand_chip->ecc.size, nand_chip->ecc.bytes); > + if (err < 0) > goto return_error; > - } > + > /* define ECC layout */ > ecclayout->eccbytes = nand_chip->ecc.bytes * > (mtd->writesize / > @@ -2005,11 +2022,6 @@ static int omap_nand_probe(struct platform_device *pdev) > ecclayout->oobfree->offset = > ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; > break; > -#else > - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > - err = -EINVAL; > - goto return_error; > -#endif > default: > pr_err("nand: error: invalid or unsupported ECC scheme\n"); > err = -EINVAL; > diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h > index 780d1e9..25d1bca 100644 > --- a/include/linux/platform_data/elm.h > +++ b/include/linux/platform_data/elm.h > @@ -42,8 +42,22 @@ struct elm_errorvec { > int error_loc[16]; > }; > > +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) > void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > struct elm_errorvec *err_vec); > int elm_config(struct device *dev, enum bch_ecc bch_type, > int ecc_steps, int ecc_step_size, int ecc_syndrome_size); > +#else > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > + struct elm_errorvec *err_vec) > +{ > +} > + > +int elm_config(struct device *dev, enum bch_ecc bch_type, > + int ecc_steps, int ecc_step_size, int ecc_syndrome_size) > +{ > + return -ENOSYS; > +} > +#endif /* CONFIG_MTD_NAND_ECC_BCH */ > + > #endif /* __ELM_H */ > Patch looks good to me. I'll give it a spin on all platforms that I have and keep you posted. cheers, -roger
On 09/08/2014 11:45 AM, Roger Quadros wrote: > On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: >> The current code abuses ifdefs to determine if the selected ECC scheme >> is supported by the running kernel. As a result the code is hard to read, >> and it also fails to load as a module. >> >> This commit removes all the ifdefs and instead introduces a function >> omap2_nand_ecc_check() to check if the ECC is supported by using >> IS_ENABLED(CONFIG_xxx). >> >> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the >> module so it can be loaded with no issues. >> >> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > Didn't apply cleanly on 3.17-rc4. Needs a rebase? > >> --- >> drivers/mtd/nand/omap2.c | 134 +++++++++++++++++++++----------------- >> include/linux/platform_data/elm.h | 14 ++++ >> 2 files changed, 87 insertions(+), 61 deletions(-) >> >> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c >> index 1170389..987dd94 100644 >> --- a/drivers/mtd/nand/omap2.c >> +++ b/drivers/mtd/nand/omap2.c >> @@ -136,7 +136,6 @@ >> >> #define BADBLOCK_MARKER_LENGTH 2 >> >> -#ifdef CONFIG_MTD_NAND_OMAP_BCH >> static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, >> 0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78, >> 0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93, >> @@ -144,7 +143,6 @@ static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, >> static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc, >> 0xac, 0x6b, 0xff, 0x99, 0x7b}; >> static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10}; >> -#endif >> >> /* oob info generated runtime depending on ecc algorithm and layout selected */ >> static struct nand_ecclayout omap_oobinfo; >> @@ -1292,7 +1290,6 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, >> return 0; >> } >> >> -#ifdef CONFIG_MTD_NAND_OMAP_BCH >> /** >> * erased_sector_bitflips - count bit flips >> * @data: data sector buffer >> @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, >> /** >> * is_elm_present - checks for presence of ELM module by scanning DT nodes >> * @omap_nand_info: NAND device structure containing platform data >> - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16 >> */ >> -static int is_elm_present(struct omap_nand_info *info, >> - struct device_node *elm_node, enum bch_ecc bch_type) >> +static bool is_elm_present(struct omap_nand_info *info, >> + struct device_node *elm_node) >> { >> struct platform_device *pdev; >> - struct nand_ecc_ctrl *ecc = &info->nand.ecc; >> - int err; >> + >> /* check whether elm-id is passed via DT */ >> if (!elm_node) { >> - pr_err("nand: error: ELM DT node not found\n"); >> - return -ENODEV; >> + dev_err(&info->pdev->dev, "ELM devicetree node not found\n"); >> + return false; >> } >> pdev = of_find_device_by_node(elm_node); >> /* check whether ELM device is registered */ >> if (!pdev) { >> - pr_err("nand: error: ELM device not found\n"); >> - return -ENODEV; >> + dev_err(&info->pdev->dev, "ELM device not found\n"); >> + return false; >> } >> /* ELM module available, now configure it */ >> info->elm_dev = &pdev->dev; >> - err = elm_config(info->elm_dev, bch_type, >> - (info->mtd.writesize / ecc->size), ecc->size, ecc->bytes); >> + return true; >> +} >> >> - return err; >> +static bool omap2_nand_ecc_check(struct omap_nand_info *info, >> + struct omap_nand_platform_data *pdata) >> +{ > > I like the idea of this function and it being called in probe. > > It would be also be nice (maybe later) to get rid of all the ECC decision making > done in gpmc_probe_nand_child() based on presence/absence of the elm_of_node. > I guess we need to add new DT entries for "bch4sw" and "bch8sw" so that DT/platform data > can explicitly specify what it needs. I don't see how ELM works with the non-DT boot > way as there is no elm_of_node there. > > Also, the elm_of_node should not be set via platform data or in gpmc.c but parsed here > directly in the NAND driver. > > But changes should be done in a separate patch. Just brought them out > here for discussion. > >> + bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm; >> + >> + switch (info->ecc_opt) { >> + case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: >> + case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: >> + ecc_needs_omap_bch = false; >> + ecc_needs_bch = true; >> + ecc_needs_elm = false; >> + break; >> + case OMAP_ECC_BCH4_CODE_HW: >> + case OMAP_ECC_BCH8_CODE_HW: >> + case OMAP_ECC_BCH16_CODE_HW: >> + ecc_needs_omap_bch = true; >> + ecc_needs_bch = false; >> + ecc_needs_elm = true; >> + break; >> + default: >> + ecc_needs_omap_bch = false; >> + ecc_needs_bch = false; >> + ecc_needs_elm = false; >> + break; >> + } >> + >> + if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) { >> + dev_err(&info->pdev->dev, >> + "CONFIG_MTD_NAND_ECC_BCH not enabled\n"); >> + return false; >> + } >> + if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) { >> + dev_err(&info->pdev->dev, >> + "CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); >> + return false; >> + } >> + if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { >> + dev_err(&info->pdev->dev, "ELM not available\n"); >> + return false; >> + } >> + >> + return true; >> } >> -#endif /* CONFIG_MTD_NAND_ECC_BCH */ >> >> static int omap_nand_probe(struct platform_device *pdev) >> { >> @@ -1797,6 +1832,11 @@ static int omap_nand_probe(struct platform_device *pdev) >> goto return_error; >> } >> >> + if (!omap2_nand_ecc_check(info, pdata)) { >> + err = -EINVAL; >> + goto return_error; >> + } >> + >> /* populate MTD interface based on ECC scheme */ >> nand_chip->ecc.layout = &omap_oobinfo; >> ecclayout = &omap_oobinfo; >> @@ -1826,7 +1866,6 @@ static int omap_nand_probe(struct platform_device *pdev) >> break; >> >> case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: >> -#ifdef CONFIG_MTD_NAND_ECC_BCH >> pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n"); >> nand_chip->ecc.mode = NAND_ECC_HW; >> nand_chip->ecc.size = 512; >> @@ -1858,14 +1897,8 @@ static int omap_nand_probe(struct platform_device *pdev) >> err = -EINVAL; >> } >> break; >> -#else >> - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); >> - err = -EINVAL; >> - goto return_error; >> -#endif >> >> case OMAP_ECC_BCH4_CODE_HW: >> -#ifdef CONFIG_MTD_NAND_OMAP_BCH >> pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n"); >> nand_chip->ecc.mode = NAND_ECC_HW; >> nand_chip->ecc.size = 512; >> @@ -1887,21 +1920,15 @@ static int omap_nand_probe(struct platform_device *pdev) >> /* reserved marker already included in ecclayout->eccbytes */ >> ecclayout->oobfree->offset = >> ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; >> - /* This ECC scheme requires ELM H/W block */ >> - if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) { >> - pr_err("nand: error: could not initialize ELM\n"); >> - err = -ENODEV; >> + >> + err = elm_config(info->elm_dev, BCH4_ECC, >> + info->mtd.writesize / nand_chip->ecc.size, >> + nand_chip->ecc.size, nand_chip->ecc.bytes); >> + if (err < 0) >> goto return_error; >> - } >> break; >> -#else >> - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); >> - err = -EINVAL; >> - goto return_error; >> -#endif >> >> case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: >> -#ifdef CONFIG_MTD_NAND_ECC_BCH >> pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n"); >> nand_chip->ecc.mode = NAND_ECC_HW; >> nand_chip->ecc.size = 512; >> @@ -1934,14 +1961,8 @@ static int omap_nand_probe(struct platform_device *pdev) >> goto return_error; >> } >> break; >> -#else >> - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); >> - err = -EINVAL; >> - goto return_error; >> -#endif >> >> case OMAP_ECC_BCH8_CODE_HW: >> -#ifdef CONFIG_MTD_NAND_OMAP_BCH >> pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n"); >> nand_chip->ecc.mode = NAND_ECC_HW; >> nand_chip->ecc.size = 512; >> @@ -1953,12 +1974,13 @@ static int omap_nand_probe(struct platform_device *pdev) >> nand_chip->ecc.calculate = omap_calculate_ecc_bch; >> nand_chip->ecc.read_page = omap_read_page_bch; >> nand_chip->ecc.write_page = omap_write_page_bch; >> - /* This ECC scheme requires ELM H/W block */ >> - err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC); >> - if (err < 0) { >> - pr_err("nand: error: could not initialize ELM\n"); >> + >> + err = elm_config(info->elm_dev, BCH8_ECC, >> + info->mtd.writesize / nand_chip->ecc.size, >> + nand_chip->ecc.size, nand_chip->ecc.bytes); >> + if (err < 0) >> goto return_error; >> - } >> + >> /* define ECC layout */ >> ecclayout->eccbytes = nand_chip->ecc.bytes * >> (mtd->writesize / >> @@ -1970,14 +1992,8 @@ static int omap_nand_probe(struct platform_device *pdev) >> ecclayout->oobfree->offset = >> ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; >> break; >> -#else >> - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); >> - err = -EINVAL; >> - goto return_error; >> -#endif >> >> case OMAP_ECC_BCH16_CODE_HW: >> -#ifdef CONFIG_MTD_NAND_OMAP_BCH >> pr_info("using OMAP_ECC_BCH16_CODE_HW ECC scheme\n"); >> nand_chip->ecc.mode = NAND_ECC_HW; >> nand_chip->ecc.size = 512; >> @@ -1988,12 +2004,13 @@ static int omap_nand_probe(struct platform_device *pdev) >> nand_chip->ecc.calculate = omap_calculate_ecc_bch; >> nand_chip->ecc.read_page = omap_read_page_bch; >> nand_chip->ecc.write_page = omap_write_page_bch; >> - /* This ECC scheme requires ELM H/W block */ >> - err = is_elm_present(info, pdata->elm_of_node, BCH16_ECC); >> - if (err < 0) { >> - pr_err("ELM is required for this ECC scheme\n"); >> + >> + err = elm_config(info->elm_dev, BCH16_ECC, >> + info->mtd.writesize / nand_chip->ecc.size, >> + nand_chip->ecc.size, nand_chip->ecc.bytes); >> + if (err < 0) >> goto return_error; >> - } >> + >> /* define ECC layout */ >> ecclayout->eccbytes = nand_chip->ecc.bytes * >> (mtd->writesize / >> @@ -2005,11 +2022,6 @@ static int omap_nand_probe(struct platform_device *pdev) >> ecclayout->oobfree->offset = >> ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; >> break; >> -#else >> - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); >> - err = -EINVAL; >> - goto return_error; >> -#endif >> default: >> pr_err("nand: error: invalid or unsupported ECC scheme\n"); >> err = -EINVAL; >> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h >> index 780d1e9..25d1bca 100644 >> --- a/include/linux/platform_data/elm.h >> +++ b/include/linux/platform_data/elm.h >> @@ -42,8 +42,22 @@ struct elm_errorvec { >> int error_loc[16]; >> }; >> >> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and CONFIG_MTD_NAND_OMAP_BCH to m. CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to be built-in if CONFIG_MTD_NAND_OMAP2 is built-in. Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2. IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c drivers/built-in.o: In function `omap_nand_probe': /work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config' /work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config' /work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config' drivers/built-in.o: In function `omap_elm_correct_data': /work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page' make: *** [vmlinux] Error 1 >> void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, >> struct elm_errorvec *err_vec); >> int elm_config(struct device *dev, enum bch_ecc bch_type, >> int ecc_steps, int ecc_step_size, int ecc_syndrome_size); >> +#else >> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, >> + struct elm_errorvec *err_vec) >> +{ >> +} >> + >> +int elm_config(struct device *dev, enum bch_ecc bch_type, >> + int ecc_steps, int ecc_step_size, int ecc_syndrome_size) >> +{ >> + return -ENOSYS; >> +} >> +#endif /* CONFIG_MTD_NAND_ECC_BCH */ >> + >> #endif /* __ELM_H */ >> > cheers, -roger
On 08 Sep 11:45 AM, Roger Quadros wrote: > On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: > > The current code abuses ifdefs to determine if the selected ECC scheme > > is supported by the running kernel. As a result the code is hard to read, > > and it also fails to load as a module. > > > > This commit removes all the ifdefs and instead introduces a function > > omap2_nand_ecc_check() to check if the ECC is supported by using > > IS_ENABLED(CONFIG_xxx). > > > > Since IS_ENABLED() is true when a config is =y or =m, this change fixes the > > module so it can be loaded with no issues. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > Didn't apply cleanly on 3.17-rc4. Needs a rebase? > Just sent a new version rebased on v3.17-rc4. Thanks a lot for the test!
On 08 Sep 01:53 PM, Roger Quadros wrote: > On 09/08/2014 11:45 AM, Roger Quadros wrote: > > On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: [..] > >> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h > >> index 780d1e9..25d1bca 100644 > >> --- a/include/linux/platform_data/elm.h > >> +++ b/include/linux/platform_data/elm.h > >> @@ -42,8 +42,22 @@ struct elm_errorvec { > >> int error_loc[16]; > >> }; > >> > >> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) > > I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and > CONFIG_MTD_NAND_OMAP_BCH to m. > > CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to > be built-in if CONFIG_MTD_NAND_OMAP2 is built-in. > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :(
On 09/10/2014 03:48 PM, Ezequiel Garcia wrote: > On 08 Sep 01:53 PM, Roger Quadros wrote: >> On 09/08/2014 11:45 AM, Roger Quadros wrote: >>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: > [..] >>>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h >>>> index 780d1e9..25d1bca 100644 >>>> --- a/include/linux/platform_data/elm.h >>>> +++ b/include/linux/platform_data/elm.h >>>> @@ -42,8 +42,22 @@ struct elm_errorvec { >>>> int error_loc[16]; >>>> }; >>>> >>>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) >> >> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and >> CONFIG_MTD_NAND_OMAP_BCH to m. >> >> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to >> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in. >> > > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :( > > Mine neither ;). I'm unaware of any other method than making CONFIG_MTD_NAND_OMAP_BCH to bool. cheers, -roger
On Wednesday 10 September 2014 06:18 PM, Ezequiel Garcia wrote: > On 08 Sep 01:53 PM, Roger Quadros wrote: >> On 09/08/2014 11:45 AM, Roger Quadros wrote: >>> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote: > [..] >>>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h >>>> index 780d1e9..25d1bca 100644 >>>> --- a/include/linux/platform_data/elm.h >>>> +++ b/include/linux/platform_data/elm.h >>>> @@ -42,8 +42,22 @@ struct elm_errorvec { >>>> int error_loc[16]; >>>> }; >>>> >>>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) >> >> I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and >> CONFIG_MTD_NAND_OMAP_BCH to m. >> >> CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to >> be built-in if CONFIG_MTD_NAND_OMAP2 is built-in. >> > > Hm, yup. Any ideas on how to accomplish that? My Kconfig-foo is not strong enough :( > > Does following work ? config MTD_NAND_OMAP_BCH depends on MTD_NAND_OMAP2 tristate "<message>" if (MTD_NAND_OMAP2 && m) bool "<message>" if (MTD_NAND_OMAP2 && y) default n ... Sorry, I don't have tools to check myself. If not following should help: $KERNEL/Documentation/kbuild/kconfig-language.txt with regards, pekon ------------------------ Powered by BigRock.com
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 1170389..987dd94 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -136,7 +136,6 @@ #define BADBLOCK_MARKER_LENGTH 2 -#ifdef CONFIG_MTD_NAND_OMAP_BCH static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, 0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78, 0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93, @@ -144,7 +143,6 @@ static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55, static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc, 0xac, 0x6b, 0xff, 0x99, 0x7b}; static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10}; -#endif /* oob info generated runtime depending on ecc algorithm and layout selected */ static struct nand_ecclayout omap_oobinfo; @@ -1292,7 +1290,6 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd, return 0; } -#ifdef CONFIG_MTD_NAND_OMAP_BCH /** * erased_sector_bitflips - count bit flips * @data: data sector buffer @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip, /** * is_elm_present - checks for presence of ELM module by scanning DT nodes * @omap_nand_info: NAND device structure containing platform data - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16 */ -static int is_elm_present(struct omap_nand_info *info, - struct device_node *elm_node, enum bch_ecc bch_type) +static bool is_elm_present(struct omap_nand_info *info, + struct device_node *elm_node) { struct platform_device *pdev; - struct nand_ecc_ctrl *ecc = &info->nand.ecc; - int err; + /* check whether elm-id is passed via DT */ if (!elm_node) { - pr_err("nand: error: ELM DT node not found\n"); - return -ENODEV; + dev_err(&info->pdev->dev, "ELM devicetree node not found\n"); + return false; } pdev = of_find_device_by_node(elm_node); /* check whether ELM device is registered */ if (!pdev) { - pr_err("nand: error: ELM device not found\n"); - return -ENODEV; + dev_err(&info->pdev->dev, "ELM device not found\n"); + return false; } /* ELM module available, now configure it */ info->elm_dev = &pdev->dev; - err = elm_config(info->elm_dev, bch_type, - (info->mtd.writesize / ecc->size), ecc->size, ecc->bytes); + return true; +} - return err; +static bool omap2_nand_ecc_check(struct omap_nand_info *info, + struct omap_nand_platform_data *pdata) +{ + bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm; + + switch (info->ecc_opt) { + case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: + case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: + ecc_needs_omap_bch = false; + ecc_needs_bch = true; + ecc_needs_elm = false; + break; + case OMAP_ECC_BCH4_CODE_HW: + case OMAP_ECC_BCH8_CODE_HW: + case OMAP_ECC_BCH16_CODE_HW: + ecc_needs_omap_bch = true; + ecc_needs_bch = false; + ecc_needs_elm = true; + break; + default: + ecc_needs_omap_bch = false; + ecc_needs_bch = false; + ecc_needs_elm = false; + break; + } + + if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) { + dev_err(&info->pdev->dev, + "CONFIG_MTD_NAND_ECC_BCH not enabled\n"); + return false; + } + if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) { + dev_err(&info->pdev->dev, + "CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); + return false; + } + if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) { + dev_err(&info->pdev->dev, "ELM not available\n"); + return false; + } + + return true; } -#endif /* CONFIG_MTD_NAND_ECC_BCH */ static int omap_nand_probe(struct platform_device *pdev) { @@ -1797,6 +1832,11 @@ static int omap_nand_probe(struct platform_device *pdev) goto return_error; } + if (!omap2_nand_ecc_check(info, pdata)) { + err = -EINVAL; + goto return_error; + } + /* populate MTD interface based on ECC scheme */ nand_chip->ecc.layout = &omap_oobinfo; ecclayout = &omap_oobinfo; @@ -1826,7 +1866,6 @@ static int omap_nand_probe(struct platform_device *pdev) break; case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: -#ifdef CONFIG_MTD_NAND_ECC_BCH pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; @@ -1858,14 +1897,8 @@ static int omap_nand_probe(struct platform_device *pdev) err = -EINVAL; } break; -#else - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); - err = -EINVAL; - goto return_error; -#endif case OMAP_ECC_BCH4_CODE_HW: -#ifdef CONFIG_MTD_NAND_OMAP_BCH pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; @@ -1887,21 +1920,15 @@ static int omap_nand_probe(struct platform_device *pdev) /* reserved marker already included in ecclayout->eccbytes */ ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; - /* This ECC scheme requires ELM H/W block */ - if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) { - pr_err("nand: error: could not initialize ELM\n"); - err = -ENODEV; + + err = elm_config(info->elm_dev, BCH4_ECC, + info->mtd.writesize / nand_chip->ecc.size, + nand_chip->ecc.size, nand_chip->ecc.bytes); + if (err < 0) goto return_error; - } break; -#else - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); - err = -EINVAL; - goto return_error; -#endif case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: -#ifdef CONFIG_MTD_NAND_ECC_BCH pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; @@ -1934,14 +1961,8 @@ static int omap_nand_probe(struct platform_device *pdev) goto return_error; } break; -#else - pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n"); - err = -EINVAL; - goto return_error; -#endif case OMAP_ECC_BCH8_CODE_HW: -#ifdef CONFIG_MTD_NAND_OMAP_BCH pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; @@ -1953,12 +1974,13 @@ static int omap_nand_probe(struct platform_device *pdev) nand_chip->ecc.calculate = omap_calculate_ecc_bch; nand_chip->ecc.read_page = omap_read_page_bch; nand_chip->ecc.write_page = omap_write_page_bch; - /* This ECC scheme requires ELM H/W block */ - err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC); - if (err < 0) { - pr_err("nand: error: could not initialize ELM\n"); + + err = elm_config(info->elm_dev, BCH8_ECC, + info->mtd.writesize / nand_chip->ecc.size, + nand_chip->ecc.size, nand_chip->ecc.bytes); + if (err < 0) goto return_error; - } + /* define ECC layout */ ecclayout->eccbytes = nand_chip->ecc.bytes * (mtd->writesize / @@ -1970,14 +1992,8 @@ static int omap_nand_probe(struct platform_device *pdev) ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; break; -#else - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); - err = -EINVAL; - goto return_error; -#endif case OMAP_ECC_BCH16_CODE_HW: -#ifdef CONFIG_MTD_NAND_OMAP_BCH pr_info("using OMAP_ECC_BCH16_CODE_HW ECC scheme\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; @@ -1988,12 +2004,13 @@ static int omap_nand_probe(struct platform_device *pdev) nand_chip->ecc.calculate = omap_calculate_ecc_bch; nand_chip->ecc.read_page = omap_read_page_bch; nand_chip->ecc.write_page = omap_write_page_bch; - /* This ECC scheme requires ELM H/W block */ - err = is_elm_present(info, pdata->elm_of_node, BCH16_ECC); - if (err < 0) { - pr_err("ELM is required for this ECC scheme\n"); + + err = elm_config(info->elm_dev, BCH16_ECC, + info->mtd.writesize / nand_chip->ecc.size, + nand_chip->ecc.size, nand_chip->ecc.bytes); + if (err < 0) goto return_error; - } + /* define ECC layout */ ecclayout->eccbytes = nand_chip->ecc.bytes * (mtd->writesize / @@ -2005,11 +2022,6 @@ static int omap_nand_probe(struct platform_device *pdev) ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; break; -#else - pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); - err = -EINVAL; - goto return_error; -#endif default: pr_err("nand: error: invalid or unsupported ECC scheme\n"); err = -EINVAL; diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h index 780d1e9..25d1bca 100644 --- a/include/linux/platform_data/elm.h +++ b/include/linux/platform_data/elm.h @@ -42,8 +42,22 @@ struct elm_errorvec { int error_loc[16]; }; +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH) void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, struct elm_errorvec *err_vec); int elm_config(struct device *dev, enum bch_ecc bch_type, int ecc_steps, int ecc_step_size, int ecc_syndrome_size); +#else +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, + struct elm_errorvec *err_vec) +{ +} + +int elm_config(struct device *dev, enum bch_ecc bch_type, + int ecc_steps, int ecc_step_size, int ecc_syndrome_size) +{ + return -ENOSYS; +} +#endif /* CONFIG_MTD_NAND_ECC_BCH */ + #endif /* __ELM_H */
The current code abuses ifdefs to determine if the selected ECC scheme is supported by the running kernel. As a result the code is hard to read, and it also fails to load as a module. This commit removes all the ifdefs and instead introduces a function omap2_nand_ecc_check() to check if the ECC is supported by using IS_ENABLED(CONFIG_xxx). Since IS_ENABLED() is true when a config is =y or =m, this change fixes the module so it can be loaded with no issues. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- drivers/mtd/nand/omap2.c | 134 +++++++++++++++++++++----------------- include/linux/platform_data/elm.h | 14 ++++ 2 files changed, 87 insertions(+), 61 deletions(-)