On Thursday, November 27, 2014 at 03:20:53 AM, Scott Wood wrote:
On Sat, Oct 11, 2014 at 06:42:53PM +0200, Marek Vasut wrote:
+#ifdef CONFIG_S3C2410
#define S3C2410_NFCONF_TACLS(x) ((x)<<8) #define S3C2410_NFCONF_TWRPH0(x) ((x)<<4) #define S3C2410_NFCONF_TWRPH1(x) ((x)<<0)
+#else /* S3C2412, S3C2440 */ +#define S3C2410_NFCONF_TACLS(x) ((x)<<12) +#define S3C2410_NFCONF_TWRPH0(x) ((x)<<8) +#define S3C2410_NFCONF_TWRPH1(x) ((x)<<4) +#endif
Is there any chance there will be a third variant? Perhaps the S3C2440 variant should be explicitly requested with an #error if no variant is selected.
No, Samsung has moved on.
#define S3C2410_ADDR_NALE 4 #define S3C2410_ADDR_NCLE 8
@@ -42,25 +51,30 @@ static void s3c24x0_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
{
struct nand_chip *chip = mtd->priv; struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
uint32_t sel_reg, sel_bit;
debug("hwcontrol(): 0x%02x 0x%08x\n", cmd & 0xff, ctrl);
if (ctrl & NAND_CTRL_CHANGE) {
ulong IO_ADDR_W = (ulong)nand;
chip->IO_ADDR_W = &nand->nfconf;if (!(ctrl & NAND_CLE))
IO_ADDR_W |= S3C2410_ADDR_NCLE;
chip->IO_ADDR_W = &nand->nfaddr;if (!(ctrl & NAND_ALE))
IO_ADDR_W |= S3C2410_ADDR_NALE;
chip->IO_ADDR_W = &nand->nfcmd;
chip->IO_ADDR_W = (void *)IO_ADDR_W;+#ifdef CONFIG_S3C2410
sel_reg = (uint32_t)&nand->nfconf;sel_bit = S3C2410_NFCONF_nFCE;+#else
sel_reg = (uint32_t)&nand->nfcont;sel_bit = S3C2440_NFCONT_nFCE;+#endif
Why are you casting &nand->nfcon[ft] to an integer...
Where exactly ?
if (ctrl & NAND_NCE)
writel(readl(&nand->nfconf) & ~S3C2410_NFCONF_nFCE,&nand->nfconf);
clrbits_le32(sel_reg, sel_bit);else
writel(readl(&nand->nfconf) | S3C2410_NFCONF_nFCE,&nand->nfconf);
setbits_le32(sel_reg, sel_bit);...only to pass that integer to something that normally accepts a pointer, which doesn't cause a warning only because of ARM's crappy I/O accessors that don't do type checking?
At least the code that was there before used ulong for inappropriate pointer-to-integer casts rather than uint32_t. :-P
So what do you suggest ?
- writel(readl(&clk_power->clkcon) | (1 << 4), &clk_power->clkcon);
- setbits_le32(&clk_power->clkcon, 1 << 4);
This change seems unrelated to adding s3c2440 support. It'd be clearer if the {clr,set,clrset}bits_le32() conversion were done separately, before the s3c2440 stuff. I'm not insisting that you redo this one, but next time try to keep cleanup separate.
If that's the case, then please apply.