From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752439AbdDIQdG (ORCPT ); Sun, 9 Apr 2017 12:33:06 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:44632 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752270AbdDIQdE (ORCPT ); Sun, 9 Apr 2017 12:33:04 -0400 Date: Sun, 9 Apr 2017 18:33:01 +0200 From: Boris Brezillon To: Masahiro Yamada Cc: linux-mtd@lists.infradead.org, Enrico Jorns , Artem Bityutskiy , Dinh Nguyen , Marek Vasut , Graham Moore , David Woodhouse , Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , devicetree@vger.kernel.org, Linux Kernel Mailing List , Brian Norris , Richard Weinberger , Cyrille Pitchen , Rob Herring , Mark Rutland Subject: Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property Message-ID: <20170409183301.037d3f95@bbrezillon> In-Reply-To: References: <1490856383-31560-1-git-send-email-yamada.masahiro@socionext.com> <1490856383-31560-15-git-send-email-yamada.masahiro@socionext.com> <20170330160238.59e5a2c1@bbrezillon> <20170331114659.4964be35@bbrezillon> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 3 Apr 2017 12:16:34 +0900 Masahiro Yamada wrote: > Hi Boris, > > > > 2017-03-31 18:46 GMT+09:00 Boris Brezillon : > > > You can try something like that when no explicit ecc.strength and > > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed. > > > > static int > > denali_get_closest_ecc_strength(struct denali_nand_info *denali, > > int strength) > > { > > /* > > * Whatever you need to select a strength that is greater than > > * or equal to strength. > > */ > > > > return X; > > } > > > Is here anything specific to Denali? Well, only the denali driver knows what the hardware supports, though having a generic function that takes a table of supported strengths would work. > > > > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali) > > { > > struct nand_chip *chip = &denali->nand; > > struct mtd_info *mtd = nand_to_mtd(chip); > > int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes; > > int ecc_steps, ecc_strength, ecc_bytes; > > int ecc_size = chip->ecc_step_ds; > > int ecc_strength = chip->ecc_strength_ds; > > > > /* > > * No information provided by the NAND chip, let the core > > * maximize the strength. > > */ > > if (!ecc_size || !ecc_strength) > > return -ENOTSUPP; > > > > if (ecc_size > 512) > > ecc_size = 1024; > > else > > ecc_size = 512; > > > > /* Adjust ECC step size based on hardware support. */ > > if (ecc_size == 1024 && > > !(denali->caps & DENALI_CAP_ECC_SIZE_1024)) > > ecc_size = 512; > > else if(ecc_size == 512 && > > !(denali->caps & DENALI_CAP_ECC_SIZE_512)) > > ecc_size = 1024; > > > > if (ecc_size < chip->ecc_size_ds) { > > /* > > * When the selected size if smaller than the expected > > * one we try to use the same strength but on 512 blocks > > * so that we can still fix the same number of errors > > * even if they are concentrated in the first 512bytes > > * of a 1024bytes portion. > > */ > > ecc_strength = chip->ecc_strength_ds; > > ecc_strength = denali_get_closest_ecc_strength(denali, > > ecc_strength); > > } else { > > /* Always prefer 1024bytes ECC blocks when possible. */ > > if (ecc_size != 1024 && > > (denali->caps & DENALI_CAP_ECC_SIZE_1024) && > > mtd->writesize > 1024) > > ecc_size = 1024; > > > > /* > > * Adjust the strength based on the selected ECC step > > * size. > > */ > > ecc_strength = DIV_ROUND_UP(ecc_size, > > chip->ecc_step_ds) * > > chip->ecc_strength_ds; > > } > > > > ecc_bytes = denali_calc_ecc_bytes(ecc_size, > > ecc_strength); > > ecc_bytes *= mtd->writesize / ecc_size; > > > > /* > > * If we don't have enough space, let the core maximize > > * the strength. > > */ > > if (ecc_bytes > max_ecc_bytes) > > return -ENOTSUPP; > > > > chip->ecc.strength = ecc_strength; > > chip->ecc.size = ecc_size; > > > > return 0; > > } > > > As a whole, this does not seem to driver-specific. It's almost controller-agnostic, except for the denali_calc_ecc_bytes() function, but I guess we could ask drivers to implement a hook that is passed the ECC step size and strength and returns the associated number of ECC bytes. > > > [1] A driver provides some pairs of (ecc_strength, ecc_size) > it can support. > > [2] The core framework knows the chip's requirement > (ecc_strength_ds, ecc_size_ds). > > > Then, the core framework provides a function > to return a most recommended (ecc_strength, ecc_size). > > > > struct nand_ecc_spec { > int ecc_strength; > int ecc_size; > }; > > /* > * This function choose the most recommented (ecc_str, ecc_size) > * "recommended" means: minimum ecc stregth that meets > * the chip's requirment. > * > * > * @chip - nand_chip > * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the > controller. (terminated by NULL as sentinel) > */ > struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip, > struct nand_ecc_spec > *controller_ecc_spec) > { > /* > * Return the pointer to the most recommended > * struct nand_ecc_spec. > * If nothing suitable found, return NULL. > */ > } > I like the idea, except I would do this slightly differently to avoid declaring all combinations of stepsize and strengths struct nand_ecc_stepsize_info { int stepsize; int nstrengths; int *strengths; }; struct nand_ecc_engine_caps { int nstepsizes; struct nand_ecc_stepsize_info *stepsizes; int (*calc_ecc_bytes)(int stepsize, int strength); }; int nand_try_to_match_ecc_req(struct nand_chip *chip, const struct nand_ecc_engine_caps *caps, struct nand_ecc_spec *spec) { /* * Find the most appropriate setting based on the ECC engine * caps and fill the spec object accordingly. * Returns 0 in case of success and a negative error code * otherwise. */ } Note that nand_try_to_match_ecc_req() has to be more generic than denali_try_to_match_ecc_req() WRT step sizes, which will probably complexify the logic. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property Date: Sun, 9 Apr 2017 18:33:01 +0200 Message-ID: <20170409183301.037d3f95@bbrezillon> References: <1490856383-31560-1-git-send-email-yamada.masahiro@socionext.com> <1490856383-31560-15-git-send-email-yamada.masahiro@socionext.com> <20170330160238.59e5a2c1@bbrezillon> <20170331114659.4964be35@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Masahiro Yamada Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Enrico Jorns , Artem Bityutskiy , Dinh Nguyen , Marek Vasut , Graham Moore , David Woodhouse , Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Kernel Mailing List , Brian Norris , Richard Weinberger , Cyrille Pitchen , Rob Herring , Mark Rutland List-Id: devicetree@vger.kernel.org On Mon, 3 Apr 2017 12:16:34 +0900 Masahiro Yamada wrote: > Hi Boris, > > > > 2017-03-31 18:46 GMT+09:00 Boris Brezillon : > > > You can try something like that when no explicit ecc.strength and > > ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed. > > > > static int > > denali_get_closest_ecc_strength(struct denali_nand_info *denali, > > int strength) > > { > > /* > > * Whatever you need to select a strength that is greater than > > * or equal to strength. > > */ > > > > return X; > > } > > > Is here anything specific to Denali? Well, only the denali driver knows what the hardware supports, though having a generic function that takes a table of supported strengths would work. > > > > static int denali_try_to_match_ecc_req(struct denali_nand_info *denali) > > { > > struct nand_chip *chip = &denali->nand; > > struct mtd_info *mtd = nand_to_mtd(chip); > > int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes; > > int ecc_steps, ecc_strength, ecc_bytes; > > int ecc_size = chip->ecc_step_ds; > > int ecc_strength = chip->ecc_strength_ds; > > > > /* > > * No information provided by the NAND chip, let the core > > * maximize the strength. > > */ > > if (!ecc_size || !ecc_strength) > > return -ENOTSUPP; > > > > if (ecc_size > 512) > > ecc_size = 1024; > > else > > ecc_size = 512; > > > > /* Adjust ECC step size based on hardware support. */ > > if (ecc_size == 1024 && > > !(denali->caps & DENALI_CAP_ECC_SIZE_1024)) > > ecc_size = 512; > > else if(ecc_size == 512 && > > !(denali->caps & DENALI_CAP_ECC_SIZE_512)) > > ecc_size = 1024; > > > > if (ecc_size < chip->ecc_size_ds) { > > /* > > * When the selected size if smaller than the expected > > * one we try to use the same strength but on 512 blocks > > * so that we can still fix the same number of errors > > * even if they are concentrated in the first 512bytes > > * of a 1024bytes portion. > > */ > > ecc_strength = chip->ecc_strength_ds; > > ecc_strength = denali_get_closest_ecc_strength(denali, > > ecc_strength); > > } else { > > /* Always prefer 1024bytes ECC blocks when possible. */ > > if (ecc_size != 1024 && > > (denali->caps & DENALI_CAP_ECC_SIZE_1024) && > > mtd->writesize > 1024) > > ecc_size = 1024; > > > > /* > > * Adjust the strength based on the selected ECC step > > * size. > > */ > > ecc_strength = DIV_ROUND_UP(ecc_size, > > chip->ecc_step_ds) * > > chip->ecc_strength_ds; > > } > > > > ecc_bytes = denali_calc_ecc_bytes(ecc_size, > > ecc_strength); > > ecc_bytes *= mtd->writesize / ecc_size; > > > > /* > > * If we don't have enough space, let the core maximize > > * the strength. > > */ > > if (ecc_bytes > max_ecc_bytes) > > return -ENOTSUPP; > > > > chip->ecc.strength = ecc_strength; > > chip->ecc.size = ecc_size; > > > > return 0; > > } > > > As a whole, this does not seem to driver-specific. It's almost controller-agnostic, except for the denali_calc_ecc_bytes() function, but I guess we could ask drivers to implement a hook that is passed the ECC step size and strength and returns the associated number of ECC bytes. > > > [1] A driver provides some pairs of (ecc_strength, ecc_size) > it can support. > > [2] The core framework knows the chip's requirement > (ecc_strength_ds, ecc_size_ds). > > > Then, the core framework provides a function > to return a most recommended (ecc_strength, ecc_size). > > > > struct nand_ecc_spec { > int ecc_strength; > int ecc_size; > }; > > /* > * This function choose the most recommented (ecc_str, ecc_size) > * "recommended" means: minimum ecc stregth that meets > * the chip's requirment. > * > * > * @chip - nand_chip > * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the > controller. (terminated by NULL as sentinel) > */ > struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip, > struct nand_ecc_spec > *controller_ecc_spec) > { > /* > * Return the pointer to the most recommended > * struct nand_ecc_spec. > * If nothing suitable found, return NULL. > */ > } > I like the idea, except I would do this slightly differently to avoid declaring all combinations of stepsize and strengths struct nand_ecc_stepsize_info { int stepsize; int nstrengths; int *strengths; }; struct nand_ecc_engine_caps { int nstepsizes; struct nand_ecc_stepsize_info *stepsizes; int (*calc_ecc_bytes)(int stepsize, int strength); }; int nand_try_to_match_ecc_req(struct nand_chip *chip, const struct nand_ecc_engine_caps *caps, struct nand_ecc_spec *spec) { /* * Find the most appropriate setting based on the ECC engine * caps and fill the spec object accordingly. * Returns 0 in case of success and a negative error code * otherwise. */ } Note that nand_try_to_match_ecc_req() has to be more generic than denali_try_to_match_ecc_req() WRT step sizes, which will probably complexify the logic. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html