| Message ID | 1433849498-3270-1-git-send-email-r.spliet@ultimaker.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Roy,
On Tue, 9 Jun 2015 13:31:38 +0200
Roy Spliet <r.spliet@ultimaker.com> wrote:
For the patch title, could you use the "mtd: nand: sunxi: " prefix,
then a short description of what you're modifying ("fix TIMING_CFG
calculation" for example).
> Values derived from the A83 user manual
The commit message should describe what you're doing. You can specify
where you retrieved the relevant information from, but that's not
enough.
>
> V2: fix crippled comments
Put the changelog...
>
> Signed-off-by: Roy Spliet <r.spliet@ultimaker.com>
> ---
... here, so that it doesn't appear in the final commit.
> drivers/mtd/nand/sunxi_nand.c | 42 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> index 6f93b29..86de7e3 100644
> --- a/drivers/mtd/nand/sunxi_nand.c
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -208,6 +208,7 @@ struct sunxi_nand_hw_ecc {
> * @nand: base NAND chip structure
> * @mtd: base MTD structure
> * @clk_rate: clk_rate required for this NAND chip
> + * @timing_cfg TIMING_CFG register value for this NAND chip
> * @selected: current active CS
> * @nsels: number of CS lines required by the NAND chip
> * @sels: array of CS lines descriptions
> @@ -217,6 +218,7 @@ struct sunxi_nand_chip {
> struct nand_chip nand;
> struct mtd_info mtd;
> unsigned long clk_rate;
> + u32 timing_cfg;
> int selected;
> int nsels;
> struct sunxi_nand_chip_sel sels[0];
> @@ -403,6 +405,7 @@ static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip)
> }
> }
>
> + writel(sunxi_nand->timing_cfg, nfc->regs + NFC_REG_TIMING_CFG);
> writel(ctl, nfc->regs + NFC_REG_CTL);
>
> sunxi_nand->selected = chip;
> @@ -807,10 +810,28 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
> return 0;
> }
>
> +static const s32 tWB_lut[] = {6, 12, 16, 20, -1};
> +static const s32 tRHW_lut[] = {4, 8, 12, 20, -1};
Nitpick: if you want to align the assignment, use tab instead of spaces
(I'm personally not a big fan of these sort of alignment).
> +
> +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32 period, u32 clk_period)
I'm not sure the period name is appropriate here, what you're actually
passing is the timing you're expecting.
How about renaming it 'min_timing'
> +{
> + u32 clks = (period + clk_period - 1) / clk_period;
u32 clks = DIV_ROUND_UP(period, clk_period);
And again, IMO the variable name does not match what it's really
encoding. How about div or divisor ?
> + int i;
> +
> + for (i = 0; lut[i] != -1; i++) {
I'd prefer to have the lut_size passed in argument instead of this -1
entry. Note that you'll be able to use the ARRAY_SIZE macro when
calling the sunxi_nand_lookup_timing() function:
sunxi_nand_lookup_timing(tRHW_lut,
ARRAY_SIZE(tRHW_lut),
timings->tRHW_min,
min_clk_period);
> + if (clks <= lut[i])
> + return i;
> + }
> +
> + /* Return max value */
> + return i - 1;
Shouldn't we complain and fail (instead of silently returning the last
entry) if we didn't find any suitable value ?
I mean, if we configure the controller with invalid timings, we might
encounter some trouble when launching specific operations.
> +}
> +
> static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
> const struct nand_sdr_timings *timings)
> {
> u32 min_clk_period = 0;
> + u32 tWB, tADL, tWHR, tRHW, tCAD;
>
> /* T1 <=> tCLS */
> if (timings->tCLS_min > min_clk_period)
> @@ -872,6 +893,20 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
> if (timings->tWC_min > (min_clk_period * 2))
> min_clk_period = DIV_ROUND_UP(timings->tWC_min, 2);
>
> + /* T16 - T19 + tCAD */
> + tWB = sunxi_nand_lookup_timing(tWB_lut, timings->tWB_max,
> + min_clk_period);
Please align min_clk_period argument on the opening parenthesis (the
same comments applies in other places).
> + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3;
> + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3;
> + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min,
> + min_clk_period);
> + tCAD = 0x7;
> + chip->timing_cfg = (tWB & 0x3) |
> + (tADL & 0x3) << 2 |
> + (tWHR & 0x3) << 4 |
> + (tRHW & 0x3) << 6 |
> + (tCAD & 0x7) << 8;
> + /* \todo A83 has some more bits for CDQSS, CS, CLHZ, CCS, WC */
Use the "TODO:" or "FIXME:" keywords so that people greping on these
pattern will be able to find them.
>
> /* Convert min_clk_period from picoseconds to nanoseconds */
> min_clk_period = DIV_ROUND_UP(min_clk_period, 1000);
> @@ -884,8 +919,6 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
> */
> chip->clk_rate = (2 * NSEC_PER_SEC) / min_clk_period;
>
> - /* TODO: configure T16-T19 */
> -
> return 0;
> }
>
> @@ -1168,6 +1201,7 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
>
> chip->nsels = nsels;
> chip->selected = -1;
> + chip->timing_cfg = 0x7ff;
>
> for (i = 0; i < nsels; i++) {
> ret = of_property_read_u32_index(np, "reg", i, &tmp);
> @@ -1377,11 +1411,9 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, nfc);
>
> /*
> - * TODO: replace these magic values with proper flags as soon as we
> - * know what they are encoding.
> + * TODO: replace this magic values with EDO flag
> */
> writel(0x100, nfc->regs + NFC_REG_TIMING_CTL);
> - writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG);
Are you planning to post another patch defining the EDO mode flag and
using it instead of keeping this magic value (note that I'm not
asking you to make this change in the same patch, but that would be
great to have it in the same patch series) ?
Thanks.
Best Regards,
Boris
On Tue, 9 Jun 2015 13:31:38 +0200 Roy Spliet <r.spliet@ultimaker.com> wrote: > > + /* T16 - T19 + tCAD */ > + tWB = sunxi_nand_lookup_timing(tWB_lut, timings->tWB_max, > + min_clk_period); > + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3; > + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3; > + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min, > + min_clk_period); > + tCAD = 0x7; > + chip->timing_cfg = (tWB & 0x3) | > + (tADL & 0x3) << 2 | > + (tWHR & 0x3) << 4 | > + (tRHW & 0x3) << 6 | > + (tCAD & 0x7) << 8; Yet another comment: could you define a macro to create the timing_cfg value ? #define NFC_TIMING_CFG(tWB, tADL, tWHR, tRHW, tCAD) \ ((tWB) & 0x3) | (((tADL) & 0x3) << 2) | \ (((tWHR) & 0x3) << 4) | (((tRHW) & 0x3) << 6) | \ (((tCAD) & 0x7) << 8);
Hello Boris, Thank you for your feedback. Where no comments in-line I will pick it up for V3 as suggested. Op 09-06-15 om 14:23 schreef Boris Brezillon: > Hi Roy, > > On Tue, 9 Jun 2015 13:31:38 +0200 > Roy Spliet <r.spliet@ultimaker.com> wrote: > > For the patch title, could you use the "mtd: nand: sunxi: " prefix, > then a short description of what you're modifying ("fix TIMING_CFG > calculation" for example). > >> Values derived from the A83 user manual > The commit message should describe what you're doing. You can specify > where you retrieved the relevant information from, but that's not > enough. > >> V2: fix crippled comments > Put the changelog... > >> Signed-off-by: Roy Spliet <r.spliet@ultimaker.com> >> --- > ... here, so that it doesn't appear in the final commit. > >> drivers/mtd/nand/sunxi_nand.c | 42 +++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 37 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c >> index 6f93b29..86de7e3 100644 >> --- a/drivers/mtd/nand/sunxi_nand.c >> +++ b/drivers/mtd/nand/sunxi_nand.c >> @@ -208,6 +208,7 @@ struct sunxi_nand_hw_ecc { >> * @nand: base NAND chip structure >> * @mtd: base MTD structure >> * @clk_rate: clk_rate required for this NAND chip >> + * @timing_cfg TIMING_CFG register value for this NAND chip >> * @selected: current active CS >> * @nsels: number of CS lines required by the NAND chip >> * @sels: array of CS lines descriptions >> @@ -217,6 +218,7 @@ struct sunxi_nand_chip { >> struct nand_chip nand; >> struct mtd_info mtd; >> unsigned long clk_rate; >> + u32 timing_cfg; >> int selected; >> int nsels; >> struct sunxi_nand_chip_sel sels[0]; >> @@ -403,6 +405,7 @@ static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip) >> } >> } >> >> + writel(sunxi_nand->timing_cfg, nfc->regs + NFC_REG_TIMING_CFG); >> writel(ctl, nfc->regs + NFC_REG_CTL); >> >> sunxi_nand->selected = chip; >> @@ -807,10 +810,28 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, >> return 0; >> } >> >> +static const s32 tWB_lut[] = {6, 12, 16, 20, -1}; >> +static const s32 tRHW_lut[] = {4, 8, 12, 20, -1}; > Nitpick: if you want to align the assignment, use tab instead of spaces > (I'm personally not a big fan of these sort of alignment). Tabs are a bit too wide to align this (esp. for the array values). I'd rather skip alignment then. > >> + >> +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32 period, u32 clk_period) > I'm not sure the period name is appropriate here, what you're actually > passing is the timing you're expecting. > How about renaming it 'min_timing' What it encodes is the minimum or maximum time or delay for given timing parameter in picoseconds. Given this definition, I think period actually describes the parameter better than "min_timing". Alternatively, I could go for period_ps or perhaps timing_period_ps (with matching clk_period_ps). >> +{ >> + u32 clks = (period + clk_period - 1) / clk_period; > u32 clks = DIV_ROUND_UP(period, clk_period); > > And again, IMO the variable name does not match what it's really > encoding. How about div or divisor ? This value encodes the period in number of clock cycles. If period_ps is acceptable, how about period_cycles here? > >> + int i; >> + >> + for (i = 0; lut[i] != -1; i++) { > I'd prefer to have the lut_size passed in argument instead of this -1 > entry. Note that you'll be able to use the ARRAY_SIZE macro when > calling the sunxi_nand_lookup_timing() function: > > sunxi_nand_lookup_timing(tRHW_lut, > ARRAY_SIZE(tRHW_lut), > timings->tRHW_min, > min_clk_period); > >> + if (clks <= lut[i]) >> + return i; >> + } >> + >> + /* Return max value */ >> + return i - 1; > Shouldn't we complain and fail (instead of silently returning the last > entry) if we didn't find any suitable value ? > I mean, if we configure the controller with invalid timings, we might > encounter some trouble when launching specific operations. > >> +} >> + >> static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, >> const struct nand_sdr_timings *timings) >> { >> u32 min_clk_period = 0; >> + u32 tWB, tADL, tWHR, tRHW, tCAD; >> >> /* T1 <=> tCLS */ >> if (timings->tCLS_min > min_clk_period) >> @@ -872,6 +893,20 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, >> if (timings->tWC_min > (min_clk_period * 2)) >> min_clk_period = DIV_ROUND_UP(timings->tWC_min, 2); >> >> + /* T16 - T19 + tCAD */ >> + tWB = sunxi_nand_lookup_timing(tWB_lut, timings->tWB_max, >> + min_clk_period); > Please align min_clk_period argument on the opening parenthesis (the > same comments applies in other places). > >> + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3; >> + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3; >> + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min, >> + min_clk_period); >> + tCAD = 0x7; >> + chip->timing_cfg = (tWB & 0x3) | >> + (tADL & 0x3) << 2 | >> + (tWHR & 0x3) << 4 | >> + (tRHW & 0x3) << 6 | >> + (tCAD & 0x7) << 8; >> + /* \todo A83 has some more bits for CDQSS, CS, CLHZ, CCS, WC */ > Use the "TODO:" or "FIXME:" keywords so that people greping on these > pattern will be able to find them. I used \todo as the (assumed) preferred notation for doxygen (... but omitted the second * denoting doxygen documentation formatting) Eclipse picks it up fine, but if you prefer TODO: for your toolchain, it's the same to me. >> /* Convert min_clk_period from picoseconds to nanoseconds */ >> min_clk_period = DIV_ROUND_UP(min_clk_period, 1000); >> @@ -884,8 +919,6 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, >> */ >> chip->clk_rate = (2 * NSEC_PER_SEC) / min_clk_period; >> >> - /* TODO: configure T16-T19 */ >> - >> return 0; >> } >> >> @@ -1168,6 +1201,7 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc, >> >> chip->nsels = nsels; >> chip->selected = -1; >> + chip->timing_cfg = 0x7ff; >> >> for (i = 0; i < nsels; i++) { >> ret = of_property_read_u32_index(np, "reg", i, &tmp); >> @@ -1377,11 +1411,9 @@ static int sunxi_nfc_probe(struct platform_device *pdev) >> platform_set_drvdata(pdev, nfc); >> >> /* >> - * TODO: replace these magic values with proper flags as soon as we >> - * know what they are encoding. >> + * TODO: replace this magic values with EDO flag >> */ >> writel(0x100, nfc->regs + NFC_REG_TIMING_CTL); >> - writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG); > Are you planning to post another patch defining the EDO mode flag and > using it instead of keeping this magic value (note that I'm not > asking you to make this change in the same patch, but that would be > great to have it in the same patch series) ? I agree that it would be good, but it is derived from the 6th ID byte, which I have already seen omitted in several datasheets. I'm not sure if we can do this without at least offering the option for overriding this value in the DT? Thank you for your time, and I look forward to hearing back. Yours, Roy > > Thanks. > > Best Regards, > > Boris >
On Tue, 09 Jun 2015 15:18:58 +0200 Roy Spliet <r.spliet@ultimaker.com> wrote: > >> + > >> +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32 > period, u32 clk_period) > > I'm not sure the period name is appropriate here, what you're actually > > passing is the timing you're expecting. > > How about renaming it 'min_timing' > > What it encodes is the minimum or maximum time or delay for given timing > parameter in picoseconds. Given this definition, I think period actually > describes the parameter better than "min_timing". Alternatively, I could go > for period_ps or perhaps timing_period_ps (with matching clk_period_ps). Hm, I don't agree here. From my POV, a period is something used to describe cyclic operations. If you take a clk, the period encodes the time required a clk cycle (a rising and a falling edge). Here the timings you're passing are not cyclic at all. > > >> +{ > >> + u32 clks = (period + clk_period - 1) / clk_period; > > u32 clks = DIV_ROUND_UP(period, clk_period); > > > > And again, IMO the variable name does not match what it's really > > encoding. How about div or divisor ? > > This value encodes the period in number of clock cycles. If period_ps is > acceptable, how about period_cycles here? I'd rather use clk_cycles, or just cycles then. > >> + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3; > >> + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3; > >> + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min, > >> + min_clk_period); > >> + tCAD = 0x7; > >> + chip->timing_cfg = (tWB & 0x3) | > >> + (tADL & 0x3) << 2 | > >> + (tWHR & 0x3) << 4 | > >> + (tRHW & 0x3) << 6 | > >> + (tCAD & 0x7) << 8; > >> + /* \todo A83 has some more bits for CDQSS, CS, CLHZ, CCS, WC */ > > Use the "TODO:" or "FIXME:" keywords so that people greping on these > > pattern will be able to find them. > > I used \todo as the (assumed) preferred notation for doxygen (... but > omitted > the second * denoting doxygen documentation formatting) Eclipse picks it up > fine, but if you prefer TODO: for your toolchain, it's the same to me. It depends on the kernel coding style rules, and AFAIR they use the TODO and FIXME (in capital letters) keywords. > >> /* > >> - * TODO: replace these magic values with proper flags as soon as we > >> - * know what they are encoding. > >> + * TODO: replace this magic values with EDO flag > >> */ > >> writel(0x100, nfc->regs + NFC_REG_TIMING_CTL); > >> - writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG); > > Are you planning to post another patch defining the EDO mode flag and > > using it instead of keeping this magic value (note that I'm not > > asking you to make this change in the same patch, but that would be > > great to have it in the same patch series) ? > > I agree that it would be good, but it is derived from the 6th ID byte, which > I have already seen omitted in several datasheets. I'm not sure if we can do > this without at least offering the option for overriding this value in > the DT? Actually, I was just asking for a flag definition. You can keep the assignment here till we find a proper solution to extract it from the read-id (or ONFI information).
Hey Boris! Op 09-06-15 om 16:20 schreef Boris Brezillon: > On Tue, 09 Jun 2015 15:18:58 +0200 > Roy Spliet<r.spliet@ultimaker.com> wrote: > >> >> + >> >> +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32 >> period, u32 clk_period) >> > I'm not sure the period name is appropriate here, what you're actually >> > passing is the timing you're expecting. >> > How about renaming it 'min_timing' >> >> What it encodes is the minimum or maximum time or delay for given timing >> parameter in picoseconds. Given this definition, I think period actually >> describes the parameter better than "min_timing". Alternatively, I could go >> for period_ps or perhaps timing_period_ps (with matching clk_period_ps). > Hm, I don't agree here. From my POV, a period is something used to > describe cyclic operations. If you take a clk, the period encodes the > time required a clk cycle (a rising and a falling edge). > Here the timings you're passing are not cyclic at all. For the sake of argument: within a cyclic event (wave theory) you are quite right with this definition: the period describes the time to complete one cycle (or oscillation). However, in English[1] a period is simply a length of time, carrying no further specification about any recurrence of events. Having said that, you're the person who still has to look at this code three weeks from now. I personally find "timing" a confusing name because it is more abstract than what the value holds (the interval/period/duration of a memory "state"), but if you find that clearer, it's your final call! >> >> +{ >> >> + u32 clks = (period + clk_period - 1) / clk_period; >> > u32 clks = DIV_ROUND_UP(period, clk_period); >> > >> > And again, IMO the variable name does not match what it's really >> > encoding. How about div or divisor ? >> >> This value encodes the period in number of clock cycles. If period_ps is >> acceptable, how about period_cycles here? > I'd rather use clk_cycles, or just cycles then. clk_cycles it is. > >> >> + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3; >> >> + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3; >> >> + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min, >> >> + min_clk_period); >> >> + tCAD = 0x7; >> >> + chip->timing_cfg = (tWB & 0x3) | >> >> + (tADL & 0x3) << 2 | >> >> + (tWHR & 0x3) << 4 | >> >> + (tRHW & 0x3) << 6 | >> >> + (tCAD & 0x7) << 8; >> >> + /* \todo A83 has some more bits for CDQSS, CS, CLHZ, CCS, WC */ >> > Use the "TODO:" or "FIXME:" keywords so that people greping on these >> > pattern will be able to find them. >> >> I used \todo as the (assumed) preferred notation for doxygen (... but >> omitted >> the second * denoting doxygen documentation formatting) Eclipse picks it up >> fine, but if you prefer TODO: for your toolchain, it's the same to me. > It depends on the kernel coding style rules, and AFAIR they use the > TODO and FIXME (in capital letters) keywords. Works for me. >> >> /* >> >> - * TODO: replace these magic values with proper flags as soon as we >> >> - * know what they are encoding. >> >> + * TODO: replace this magic values with EDO flag >> >> */ >> >> writel(0x100, nfc->regs + NFC_REG_TIMING_CTL); >> >> - writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG); >> > Are you planning to post another patch defining the EDO mode flag and >> > using it instead of keeping this magic value (note that I'm not >> > asking you to make this change in the same patch, but that would be >> > great to have it in the same patch series) ? >> >> I agree that it would be good, but it is derived from the 6th ID byte, which >> I have already seen omitted in several datasheets. I'm not sure if we can do >> this without at least offering the option for overriding this value in >> the DT? > Actually, I was just asking for a flag definition. You can keep the > assignment here till we find a proper solution to extract it from the > read-id (or ONFI information). I hope to send you a V3 today once we get that bike shed painted :-). Yours, Roy [1] http://dictionary.cambridge.org/dictionary/business-english/period
Hi Roy, On Wed, 10 Jun 2015 09:30:11 +0200 Roy Spliet <r.spliet@ultimaker.com> wrote: > Hey Boris! > > Op 09-06-15 om 16:20 schreef Boris Brezillon: > > On Tue, 09 Jun 2015 15:18:58 +0200 > > Roy Spliet<r.spliet@ultimaker.com> wrote: > > > > > >> >> + > >> >> +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32 > >> period, u32 clk_period) > >> > I'm not sure the period name is appropriate here, what you're actually > >> > passing is the timing you're expecting. > >> > How about renaming it 'min_timing' > >> > >> What it encodes is the minimum or maximum time or delay for given timing > >> parameter in picoseconds. Given this definition, I think period actually > >> describes the parameter better than "min_timing". Alternatively, I could go > >> for period_ps or perhaps timing_period_ps (with matching clk_period_ps). > > Hm, I don't agree here. From my POV, a period is something used to > > describe cyclic operations. If you take a clk, the period encodes the > > time required a clk cycle (a rising and a falling edge). > > Here the timings you're passing are not cyclic at all. > For the sake of argument: within a cyclic event (wave theory) you are > quite right with > this definition: the period describes the time to complete one cycle (or > oscillation). > However, in English[1] a period is simply a length of time, carrying no > further > specification about any recurrence of events. > Having said that, you're the person who still has to look at this code > three weeks from > now. I personally find "timing" a confusing name because it is more > abstract than > what the value holds (the interval/period/duration of a memory "state"), > but if you find > that clearer, it's your final call! You're right about the English definition of period, however, what bothers me here is that the clk_period is actually representing the time of clk cycle, while the period argument is not related at all with a cyclic event, and this can lead misinterpretations. Anyway, this is just a detail (but I'd still prefer duration over period ;-)). Best Regards, Boris
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c index 6f93b29..86de7e3 100644 --- a/drivers/mtd/nand/sunxi_nand.c +++ b/drivers/mtd/nand/sunxi_nand.c @@ -208,6 +208,7 @@ struct sunxi_nand_hw_ecc { * @nand: base NAND chip structure * @mtd: base MTD structure * @clk_rate: clk_rate required for this NAND chip + * @timing_cfg TIMING_CFG register value for this NAND chip * @selected: current active CS * @nsels: number of CS lines required by the NAND chip * @sels: array of CS lines descriptions @@ -217,6 +218,7 @@ struct sunxi_nand_chip { struct nand_chip nand; struct mtd_info mtd; unsigned long clk_rate; + u32 timing_cfg; int selected; int nsels; struct sunxi_nand_chip_sel sels[0]; @@ -403,6 +405,7 @@ static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip) } } + writel(sunxi_nand->timing_cfg, nfc->regs + NFC_REG_TIMING_CFG); writel(ctl, nfc->regs + NFC_REG_CTL); sunxi_nand->selected = chip; @@ -807,10 +810,28 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, return 0; } +static const s32 tWB_lut[] = {6, 12, 16, 20, -1}; +static const s32 tRHW_lut[] = {4, 8, 12, 20, -1}; + +static s32 sunxi_nand_lookup_timing(const s32 *lut, u32 period, u32 clk_period) +{ + u32 clks = (period + clk_period - 1) / clk_period; + int i; + + for (i = 0; lut[i] != -1; i++) { + if (clks <= lut[i]) + return i; + } + + /* Return max value */ + return i - 1; +} + static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, const struct nand_sdr_timings *timings) { u32 min_clk_period = 0; + u32 tWB, tADL, tWHR, tRHW, tCAD; /* T1 <=> tCLS */ if (timings->tCLS_min > min_clk_period) @@ -872,6 +893,20 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, if (timings->tWC_min > (min_clk_period * 2)) min_clk_period = DIV_ROUND_UP(timings->tWC_min, 2); + /* T16 - T19 + tCAD */ + tWB = sunxi_nand_lookup_timing(tWB_lut, timings->tWB_max, + min_clk_period); + tADL = DIV_ROUND_UP(timings->tADL_min, min_clk_period) >> 3; + tWHR = DIV_ROUND_UP(timings->tWHR_min, min_clk_period) >> 3; + tRHW = sunxi_nand_lookup_timing(tRHW_lut, timings->tRHW_min, + min_clk_period); + tCAD = 0x7; + chip->timing_cfg = (tWB & 0x3) | + (tADL & 0x3) << 2 | + (tWHR & 0x3) << 4 | + (tRHW & 0x3) << 6 | + (tCAD & 0x7) << 8; + /* \todo A83 has some more bits for CDQSS, CS, CLHZ, CCS, WC */ /* Convert min_clk_period from picoseconds to nanoseconds */ min_clk_period = DIV_ROUND_UP(min_clk_period, 1000); @@ -884,8 +919,6 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, */ chip->clk_rate = (2 * NSEC_PER_SEC) / min_clk_period; - /* TODO: configure T16-T19 */ - return 0; } @@ -1168,6 +1201,7 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc, chip->nsels = nsels; chip->selected = -1; + chip->timing_cfg = 0x7ff; for (i = 0; i < nsels; i++) { ret = of_property_read_u32_index(np, "reg", i, &tmp); @@ -1377,11 +1411,9 @@ static int sunxi_nfc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, nfc); /* - * TODO: replace these magic values with proper flags as soon as we - * know what they are encoding. + * TODO: replace this magic values with EDO flag */ writel(0x100, nfc->regs + NFC_REG_TIMING_CTL); - writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG); ret = sunxi_nand_chips_init(dev, nfc); if (ret) {
Values derived from the A83 user manual V2: fix crippled comments Signed-off-by: Roy Spliet <r.spliet@ultimaker.com> --- drivers/mtd/nand/sunxi_nand.c | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-)