[PATCH v2] mtd: nand: Sunxi calculate timing cfg
Roy Spliet
r.spliet at ultimaker.com
Tue Jun 9 06:18:58 PDT 2015
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 at 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 at 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
>
--
IMAGINE IT >> MAKE IT
Meet us online at Twitter <http://twitter.com/ultimaker>, Facebook
<http://facebook.com/ultimaker>, Google+ <http://google.com/+Ultimaker>
www.ultimaker.com
More information about the linux-arm-kernel
mailing list