| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <61255.192.168.10.89.1240984645.squirrel@dbdmail.itg.ti.com>
Date: Wed, 29 Apr 2009 11:27:25 +0530 (IST)
From: "vimal singh" <vimalsingh@...com>
To: "Andrew Morton" <akpm@...ux-foundation.org>
Cc: linux-mtd@...ts.infradead.org, dedekind@...radead.org,
linux-kernel@...r.kernel.org
Subject: Re: [resending] [PATCH] [MTD] [NAND] Add OMAP2 / OMAP3 NAND driver
Andrew,
Thanks for your valuable comments.
On Wed, Apr 29, 2009 at 10:19 AM, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Mon, 27 Apr 2009 12:40:26 +0530 (IST) "vimal singh" <vimalsingh@...com>
wrote:
>
>> I am sending this patch again, after Fixing Artem's comments. Also CC'ed to
lkml.
>>
>> --
>> Best regards,
>> vimal
>>
>>
>> This driver is present in the OMAP tree, now pushing it to MTD.
>>
>> Original author(s):
>> Jian Zhang <jzhang@...com>
>
> That's not a particularly illuminating changelog.
>
>> +config MTD_NAND_OMAP2
>> + tristate "NAND Flash device on OMAP2 and OMAP3"
>> + depends on ARM && MTD_NAND && (ARCH_OMAP2 || ARCH_OMAP3)
>> + help
>> + Support for NAND flash on Texas Instruments OMAP2 and OMAP3
platforms.
>
> OK, that tells us more.
>
>
> Please pass this (and all other) patches through scripts/checkpatch.pl.
> checkpatch finds problem in this patch which you would have fixed, had
> you known about them.
I took this code from Linux-omap tree, so ignored to run checkpatch.pl.
I'll do the same this time.
>
>> +#define TF(value) (value ? 1 : 0)
>> +
>> +#define P2048e(a) (TF(a & NAND_Ecc_P2048e) << 0)
>> +#define P2048o(a) (TF(a & NAND_Ecc_P2048o) << 1)
>> +#define P1e(a) (TF(a & NAND_Ecc_P1e) << 2)
>> +#define P1o(a) (TF(a & NAND_Ecc_P1o) << 3)
>> +#define P2e(a) (TF(a & NAND_Ecc_P2e) << 4)
>> +#define P2o(a) (TF(a & NAND_Ecc_P2o) << 5)
>> +#define P4e(a) (TF(a & NAND_Ecc_P4e) << 6)
>> +#define P4o(a) (TF(a & NAND_Ecc_P4o) << 7)
>> +
>> +#define P8e(a) (TF(a & NAND_Ecc_P8e) << 0)
>> +#define P8o(a) (TF(a & NAND_Ecc_P8o) << 1)
>> +#define P16e(a) (TF(a & NAND_Ecc_P16e) << 2)
>> +#define P16o(a) (TF(a & NAND_Ecc_P16o) << 3)
>> +#define P32e(a) (TF(a & NAND_Ecc_P32e) << 4)
>> +#define P32o(a) (TF(a & NAND_Ecc_P32o) << 5)
>> +#define P64e(a) (TF(a & NAND_Ecc_P64e) << 6)
>> +#define P64o(a) (TF(a & NAND_Ecc_P64o) << 7)
>> +
>> +#define P128e(a) (TF(a & NAND_Ecc_P128e) << 0)
>> +#define P128o(a) (TF(a & NAND_Ecc_P128o) << 1)
>> +#define P256e(a) (TF(a & NAND_Ecc_P256e) << 2)
>> +#define P256o(a) (TF(a & NAND_Ecc_P256o) << 3)
>> +#define P512e(a) (TF(a & NAND_Ecc_P512e) << 4)
>> +#define P512o(a) (TF(a & NAND_Ecc_P512o) << 5)
>> +#define P1024e(a) (TF(a & NAND_Ecc_P1024e) << 6)
>> +#define P1024o(a) (TF(a & NAND_Ecc_P1024o) << 7)
>> +
>> +#define P8e_s(a) (TF(a & NAND_Ecc_P8e) << 0)
>> +#define P8o_s(a) (TF(a & NAND_Ecc_P8o) << 1)
>> +#define P16e_s(a) (TF(a & NAND_Ecc_P16e) << 2)
>> +#define P16o_s(a) (TF(a & NAND_Ecc_P16o) << 3)
>> +#define P1e_s(a) (TF(a & NAND_Ecc_P1e) << 4)
>> +#define P1o_s(a) (TF(a & NAND_Ecc_P1o) << 5)
>> +#define P2e_s(a) (TF(a & NAND_Ecc_P2e) << 6)
>> +#define P2o_s(a) (TF(a & NAND_Ecc_P2o) << 7)
>> +
>> +#define P4e_s(a) (TF(a & NAND_Ecc_P4e) << 0)
>> +#define P4o_s(a) (TF(a & NAND_Ecc_P4o) << 1)
>
> (ick)
>
> I suspect that the above will expand into quite large amounts of poor
> code at the sites where they are invoked.
>
> eg:
>
> ecc_buf[2] = ~(P4o(tmp) | P4e(tmp) | P2o(tmp) | P2e(tmp) | P1o(tmp) |
> P1e(tmp) | P2048o(tmp) | P2048e(tmp));
>
> this might cause the generation of eight test-n-branch operations,
> whereas it could have been done with one.
Actually I have plan to change HW ECC correction method, in a separate patch,
where all these macros will get removed. So, I did not bother much at this.
I can try to simplify these, if you still insist.
>
>> +/**
>> + * omap_nand_wp - This function enable or disable the Write Protect feature on
>> + * NAND device
>
> Alas, kerneldoc doesn't support that. The description ("This function
> enable or disable the Write Protect feature on NAND device") must all
> be on a single line.
I'll fix this.
>
>> + * @mtd: MTD device structure
>> + * @mode: WP ON/OFF
>> + */
>> +static void omap_nand_wp(struct mtd_info *mtd, int mode)
>> +{
>> + struct omap_nand_info *info = container_of(mtd,
>> + struct omap_nand_info, mtd);
>> +
>> + unsigned long config = __raw_readl(info->gpmc_baseaddr + GPMC_CONFIG);
>> +
>> + if (mode)
>> + config &= ~(NAND_WP_BIT); /* WP is ON */
>> + else
>> + config |= (NAND_WP_BIT); /* WP is OFF */
>> +
>> + __raw_writel(config, (info->gpmc_baseaddr + GPMC_CONFIG));
>> +}
>> +
>> +/**
>> + * hardware specific access to control-lines
>> + * NOTE: boards may use different bits for these!!
>> + *
>> + * @ctrl:
>> + * NAND_NCE: bit 0 - don't care
>> + * NAND_CLE: bit 1 -> Command Latch
>> + * NAND_ALE: bit 2 -> Address Latch
>> + */
>
> This comment purports to be a kerneldoc comment (it starts with /**) ,
> but in fact it is not a kerneldoc comment.
>
> Please review the comments in this patch.
OK.
>
>> + GPMC_CS_NAND_DATA;
>> + break;
>> +
>> + case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
>> + info->nand.IO_ADDR_W = info->gpmc_cs_baseaddr +
>> + GPMC_CS_NAND_ADDRESS;
>> + info->nand.IO_ADDR_R = info->gpmc_cs_baseaddr +
>> + GPMC_CS_NAND_DATA;
>> + break;
>> +
>> + case NAND_CTRL_CHANGE | NAND_NCE:
>> + info->nand.IO_ADDR_W = info->gpmc_cs_baseaddr +
>> + GPMC_CS_NAND_DATA;
>> + info->nand.IO_ADDR_R = info->gpmc_cs_baseaddr +
>> + GPMC_CS_NAND_DATA;
>> + break;
>> + }
>> +
>> + if (cmd != NAND_CMD_NONE)
>> + __raw_writeb(cmd, info->nand.IO_ADDR_W);
>> +}
>> +
>>
>> ...
>>
>> +/**
>> + * omap_calcuate_ecc - Generate non-inverted ECC bytes.
>> + * Using noninverted ECC can be considered ugly since writing a blank
>> + * page ie. padding will clear the ECC bytes. This is no problem as long
>> + * nobody is trying to write data on the seemingly unused page. Reading
>> + * an erased page will produce an ECC mismatch between generated and read
>> + * ECC bytes that has to be dealt with separately.
>> + * @mtd: MTD device structure
>> + * @dat: The pointer to data on which ecc is computed
>> + * @ecc_code: The ecc_code buffer
>> + */
>
> I believe the description of the function arguments must precede the
> general discussion.
Yes, I'll fix this.
>
>> +static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
>> + u_char *ecc_code)
>> +{
>> + struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>> + mtd);
>> + unsigned long val = 0x0;
>> + unsigned long reg;
>> +
>> + /* Start Reading from HW ECC1_Result = 0x200 */
>> + reg = (unsigned long)(info->gpmc_baseaddr + GPMC_ECC1_RESULT);
>> + val = __raw_readl(reg);
>> + *ecc_code++ = val; /* P128e, ..., P1e */
>> + *ecc_code++ = val >> 16; /* P128o, ..., P1o */
>> + /* P2048o, P1024o, P512o, P256o, P2048e, P1024e, P512e, P256e */
>> + *ecc_code++ = ((val >> 8) & 0x0f) | ((val >> 20) & 0xf0);
>> + reg += 4;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * omap_enable_hwecc - This function enables the hardware ecc functionality
>> + * @mtd: MTD device structure
>> + * @mode: Read/Write mode
>> + */
>> +static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
>> +{
>> + struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>> + mtd);
>> + register struct nand_chip *chip = mtd->priv;
>
> gcc has ignored keyword `register' for a decade at least (unless
> invoked with -O0). Please zap.
sure, I'll remove this.
>
>> + unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
>> + unsigned long val = __raw_readl(info->gpmc_baseaddr + GPMC_ECC_CONFIG);
>> +
>> + switch (mode) {
>> + case NAND_ECC_READ :
>> + __raw_writel(0x101, info->gpmc_baseaddr + GPMC_ECC_CONTROL);
>> + /* (ECC 16 or 8 bit col) | ( CS ) | ECC Enable */
>> + val = (dev_width << 7) | (info->gpmc_cs << 1) | (0x1);
>> + break;
>> + case NAND_ECC_READSYN :
>> + __raw_writel(0x100, info->gpmc_baseaddr + GPMC_ECC_CONTROL);
>> + /* (ECC 16 or 8 bit col) | ( CS ) | ECC Enable */
>> + val = (dev_width << 7) | (info->gpmc_cs << 1) | (0x1);
>> + break;
>> + case NAND_ECC_WRITE :
>> + __raw_writel(0x101, info->gpmc_baseaddr + GPMC_ECC_CONTROL);
>> + /* (ECC 16 or 8 bit col) | ( CS ) | ECC Enable */
>> + val = (dev_width << 7) | (info->gpmc_cs << 1) | (0x1);
>> + break;
>> + default:
>> + DEBUG(MTD_DEBUG_LEVEL0, "Error: Unrecognized Mode[%d]!\n",
>> + mode);
>> + break;
>> + }
>> +
>> + __raw_writel(val, info->gpmc_baseaddr + GPMC_ECC_CONFIG);
>> +}
>> +#endif
>> +
>> +/**
>> + * omap_wait - Wait function is called during Program and erase
>> + * operations and the way it is called from MTD layer, we should wait
>> + * till the NAND chip is ready after the programming/erase operation
>> + * has completed.
>> + * @mtd: MTD device structure
>> + * @chip: NAND Chip structure
>> + */
>> +static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
>> +{
>> + register struct nand_chip *this = mtd->priv;
>> + struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>> + mtd);
>> + int status = 0;
>> +
>> + this->IO_ADDR_W = (void *) info->gpmc_cs_baseaddr +
>> + GPMC_CS_NAND_COMMAND;
>> + this->IO_ADDR_R = (void *) info->gpmc_cs_baseaddr + GPMC_CS_NAND_DATA;
>> +
>> + while (!(status & 0x40)) {
>> + __raw_writeb(NAND_CMD_STATUS & 0xFF, this->IO_ADDR_W);
>> + status = __raw_readb(this->IO_ADDR_R);
>> + }
>
> This loop looks like it could consume rather a lot of energy. Can it
> be optimised?
Well... here improvement could be to introduce a timeout. I'll do this.
I'll fix all your comments and resend the patch.
---
Best Regards,
\/ | |\/| /-\ |_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists