mtd: nand: Sunxi calculate timing cfg
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
Values derived from the A83 user manual
V2: fix crippled comments
--- drivers/mtd/nand/sunxi_nand.c | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-)
drivers/mtd/nand/sunxi_nand.c +37 -5
@@ -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) {
Signature · 12 lines
2.4.2 -- 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
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
Hi Roy,
On Tue, 9 Jun 2015 13:31:38 +0200
Roy Spliet <[email protected]> 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).
Quoted 1 line
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.
Quoted 2 lines
V2: fix crippled comments
Put the changelog...
Quoted 3 lines
Signed-off-by Roy Spliet <[email protected]>---
... here, so that it doesn't appear in the final commit.
Quoted 32 lines
drivers/mtd/nand/sunxi_nand.c | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-)drivers/mtd/nand/sunxi_nand.c +3
-0@@ -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;
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 814
Quoted 5 lines
drivers/mtd/nand/sunxi_nand.c +2
-0@@ -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).
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 816
Quoted 2 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+ +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'
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 818
Quoted 2 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+{ +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 ?
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 821
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c +3
-0+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);
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 827
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c +6
-0+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.
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c +3
-0+} +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)
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 898
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c +3
-0@@ -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).
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 909
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c +11
-0+ 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.
Quoted 21 lines
drivers/mtd/nand/sunxi_nand.c +1
-2/* 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);
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_probe(struct platform_device *pdev) · line 1384
Quoted 9 lines
drivers/mtd/nand/sunxi_nand.c +1
-3@@ -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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
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:
Quoted 14 lines
Hi Roy,
On Tue, 9 Jun 2015 13:31:38 +0200
Roy Spliet <[email protected]> wrote:For the patch title, could you use the "mtd: nand: sunxi: " prefix, then a short description of what you're modifying ("fix
TIMING_CFGcalculation" for example).Quoted 1 line
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.
Quoted 8 lines
Quoted 1 line
V2: fix crippled comments
Put the changelog...
Quoted 2 lines
Signed-off-by Roy Spliet <[email protected]>---
... here, so that it doesn't appear in the final commit.
Quoted 1 line
drivers/mtd/nand/sunxi_nand.c | 42
+++++++++++++++++++++++++++++++++++++-----
Quoted 3 lines
Review
Quoted 3 lines
1 file changed, 37 insertions(+), 5 deletions(-)(quoted diff) +0
-0
b/drivers/mtd/nand/sunxi_nand.c
Quoted 12 lines
Quoted 12 lines
index 6f93b29..86de7e3 100644
drivers/mtd/nand/sunxi_nand.c +1
-0@@ -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 {
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c static void sunxi_nfc_select_chip(struct
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c +1
-0struct 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)
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c static int
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c +1
-0} } +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,
Quoted 7 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 814
Quoted 5 lines
drivers/mtd/nand/sunxi_nand.c +2
-0return 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.
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c static int · line 816
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+ +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32
period, u32 clk_period)
Quoted 3 lines
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).
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 818
Quoted 2 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+{ +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?
Quoted 32 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 821
Quoted 4 lines
drivers/mtd/nand/sunxi_nand.c +3
-0+int i; + +for (i =0 ; lut[i] !=-1 ; i++) {I'd prefer to have the
lut_sizepassed in argument instead of this -1 entry. Note that you'll be able to use theARRAY_SIZEmacro when calling thesunxi_nand_lookup_timing()function:sunxi_nand_lookup_timing(tRHW_lut,
ARRAY_SIZE(tRHW_lut), timings->tRHW_min, min_clk_period);drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 827
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c +6
-0+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.Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c +3
-0+} +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;
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c +0
-0/* 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,
Quoted 20 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 898
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c +3
-0if (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_periodargument on the opening parenthesis (the same comments applies in other places).drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 909
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c +11
-0+ 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 */
Quoted 2 lines
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.
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c +0
-0/* 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,
Quoted 9 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_init(struct device
Quoted 9 lines
drivers/mtd/nand/sunxi_nand.c +0
-2*/ 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,
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_probe(struct
Quoted 8 lines
drivers/mtd/nand/sunxi_nand.c +1
-0chip->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)
Quoted 9 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_probe(struct platform_device *pdev) · line 1384
Quoted 9 lines
drivers/mtd/nand/sunxi_nand.c +1
-3platform_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 );
Quoted 4 lines
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
Quoted 7 lines
Thanks.
Best Regards,
Boris
Signature · 6 lines
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
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
Quoted 11 lines · On Tue, 09 Jun 2015 15:18:58 +0200 Roy Spliet <[email protected]> wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32period, u32
clk_period)Quoted 3 lines
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 forperiod_psor perhapstiming_period_ps(with matchingclk_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.
Quoted 10 lines
Quoted 7 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 818
Quoted 3 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+{ +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_psis acceptable, how aboutperiod_cycleshere?
I'd rather use clk_cycles, or just cycles then.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c +11
-0+ 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 */
Quoted 7 lines
Quoted 2 lines
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.
Quoted 12 lines
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_probe(struct platform_device *pdev) · line 1384
Quoted 7 lines
/*drivers/mtd/nand/sunxi_nand.c +1
-3-* 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) ?
Quoted 4 lines
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).
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
Hey Boris!
Op 09-06-15 om 16:20 schreef Boris Brezillon:
Quoted 10 lines
Quoted 7 lines · On Tue, 09 Jun 2015 15:18:58 +0200 Roy Spliet<[email protected]> wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32period, u32
clk_period)Quoted 3 lines
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'
Quoted 8 lines
Quoted 4 lines
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 forperiod_psor perhapstiming_period_ps(with matchingclk_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!
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd, · line 818
Quoted 2 lines
drivers/mtd/nand/sunxi_nand.c +2
-0+{ +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_psis acceptable, how aboutperiod_cycleshere?I'd rather use
clk_cycles, or just cycles then.
clk_cycles it is.
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 909
Quoted 12 lines
drivers/mtd/nand/sunxi_nand.c +11
-0+ 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
drivers/mtd/nand/sunxi_nand.c static int sunxi_nfc_probe(struct platform_device *pdev) · line 1384
Quoted 7 lines
/*drivers/mtd/nand/sunxi_nand.c +1
-3-* 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) ?
Quoted 7 lines
Quoted 4 lines
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
Signature · 6 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
Hi Roy,
Quoted 22 lines · On Wed, 10 Jun 2015 09:30:11 +0200 Roy Spliet <[email protected]> wrote:
Hey Boris!
Op 09-06-15 om 16:20 schreef Boris Brezillon:
Quoted 11 lines
Quoted 7 lines · On Tue, 09 Jun 2015 15:18:58 +0200 Roy Spliet<[email protected]> wrote:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ +static s32 sunxi_nand_lookup_timing(const s32 *lut, lut, u32period, u32
clk_period)Quoted 3 lines
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'Quoted 8 lines
Quoted 4 lines
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 forperiod_psor perhapstiming_period_ps(with matchingclk_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.
Quoted 14 lines
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
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 mtd: nand: Sunxi calculate timing cfg
In reply to Roy SplietQuoted 4 lines · On Tue, 9 Jun 2015 13:31:38 +0200 Roy Spliet <[email protected]> wrote:
drivers/mtd/nand/sunxi_nand.c +3
-0+/* T16 - T19 + tCAD */ + tWB =sunxi_nand_lookup_timing (tWB_lut, timings->tWB_max, + min_clk_period);
drivers/mtd/nand/sunxi_nand.c static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip, · line 908
Quoted 10 lines
drivers/mtd/nand/sunxi_nand.c +10
-0+ 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);
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com