goldfish: NAND flash driver
PATCH 1/1 goldfish: NAND flash driver
From: Arve Hjønnevåg <[email protected]>
Fold together the NAND driver for Goldfish from Arve with cleanups by Jun Nakajima and a tidy up to 3.7 and checkpatch.
---
drivers/mtd/devices/Kconfig | 7 drivers/mtd/devices/Makefile | 1 drivers/mtd/devices/goldfish_nand.c | 444 +++++++++++++++++++++++++++++++ drivers/mtd/devices/goldfish_nand_reg.h | 72 +++++ 4 files changed, 524 insertions(+) create mode 100644 drivers/mtd/devices/goldfish_nand.c create mode 100644 drivers/mtd/devices/goldfish_nand_reg.h
drivers/mtd/devices/Kconfig +7 -0
@@ -343,4 +343,11 @@ config MTD_DOCPROBE_55AA LinuxBIOS or if you need to recover a DiskOnChip Millennium on which you have managed to wipe the first block. +config MTD_GOLDFISH_NAND + tristate "Goldfish NAND device" + depends on GOLDFISH + help + Drives the emulated NAND flash device on the Google Goldfish + Android virtual device. + endmenu
drivers/mtd/devices/Makefile +1 -0
@@ -21,6 +21,7 @@ obj-$(CONFIG_MTD_NAND_OMAP_BCH) += elm.o obj-$(CONFIG_MTD_SPEAR_SMI) += spear_smi.o obj-$(CONFIG_MTD_SST25L) += sst25l.o obj-$(CONFIG_MTD_BCM47XXSFLASH) += bcm47xxsflash.o +obj-$(CONFIG_MTD_GOLDFISH_NAND) += goldfish_nand.o CFLAGS_docg3.o += -I$(src)
drivers/mtd/devices/goldfish_nand.c new file +444 -0
444 lines not on this page · raw message
drivers/mtd/devices/goldfish_nand_reg.h new file +72 -0
72 lines not on this page · raw message
Alan Cox <[email protected]> writes:
... zip ...
Hi Alan,
I have a couple of questions on your patch.
Quoted 21 lines
drivers/mtd/devices/goldfish_nand.c new file +21
-0+static u32 goldfish_nand_cmd_with_params (struct mtd_info *mtd, +enum nand_cmd cmd,u64 addr,u32 len, +void *ptr,u32 *rv) +{ +u32 cmdp; +struct goldfish_nand *nand = mtd->priv; +struct cmd_params *cps = nand->cmd_params; +unsigned char __iomem *base = nand->base; + +if (cps ==NULL ) +return -1 ; + +switch (cmd) { +case NAND_CMD_ERASE : + cmdp =NAND_CMD_ERASE_WITH_PARAMS ; +break ; +case NAND_CMD_READ : + cmdp =NAND_CMD_READ_WITH_PARAMS ; +break ; +case NAND_CMD_WRITE : + cmdp =NAND_CMD_WRITE_WITH_PARAMS ;
drivers/mtd/devices/goldfish_nand.c line 71
Quoted 10 lines
drivers/mtd/devices/goldfish_nand.c new file +10
-0+break ; +default : +return -1 ; + } + cps->dev = mtd - nand->mtd; + cps->addr_high = (u32)(addr >>32 ); + cps->addr_low = (u32 )addr; + cps->transfer_size = len; + cps->data = (u32 )ptr; +writel (cmdp, base +NAND_COMMAND );
What guarantee do you have on the order of writes here ? Isn't a write barrier required here ?
drivers/mtd/devices/goldfish_nand.c line 84
Quoted 13 lines
drivers/mtd/devices/goldfish_nand.c new file +13
-0+ *rv = cps->result; +return 0 ; +} + +static u32 goldfish_nand_cmd (struct mtd_info *mtd,enum nand_cmd cmd, +u64 addr,u32 len,void *ptr) +{ +struct goldfish_nand *nand = mtd->priv; +u32 rv; +unsigned long irq_flags; +unsigned char __iomem *base = nand->base; + +spin_lock_irqsave (&nand->lock, irq_flags);
Why this spin_lock and not a mutex ? I didn't see any interrupts used in this driver, have I missed something ?
drivers/mtd/devices/goldfish_nand.c line 92
Quoted 8 lines
drivers/mtd/devices/goldfish_nand.c new file +8
-0+if (goldfish_nand_cmd_with_params (mtd, cmd, addr, len, ptr, &rv)) { +writel (mtd - nand->mtd, base +NAND_DEV ); +writel ((u32)(addr >>32 ), base +NAND_ADDR_HIGH ); +writel ((u32 )addr, base +NAND_ADDR_LOW ); +writel (len, base +NAND_TRANSFER_SIZE ); +writel ((u32 )ptr, base +NAND_DATA ); +writel (cmd, base +NAND_COMMAND ); + rv =readl (base +NAND_RESULT );
Same question here on the order of the read wrt to previous writes.
drivers/mtd/devices/goldfish_nand.c line 105
Quoted 13 lines
drivers/mtd/devices/goldfish_nand.c new file +13
-0+ } +spin_unlock_irqrestore (&nand->lock, irq_flags); +return rv; +} + +static int goldfish_nand_erase (struct mtd_info *mtd,struct erase_info *instr) +{ +loff_t ofs = instr->addr; +u32 len = instr->len; +u32 rem; + +if (ofs + len > mtd->size) + goto invalid_arg;
I don't think that test is required, the MTD API gives already that guarantee AFAIR.
... zip ...
drivers/mtd/devices/goldfish_nand.c line 138
Quoted 7 lines
drivers/mtd/devices/goldfish_nand.c new file +7
-0+static int goldfish_nand_read_oob (struct mtd_info *mtd,loff_t ofs, +struct mtd_oob_ops *ops) +{ +u32 rem; + +if (ofs + ops->len > mtd->size) + goto invalid_arg;
Ditto.
...zip...
drivers/mtd/devices/goldfish_nand.c line 170
Quoted 7 lines
drivers/mtd/devices/goldfish_nand.c new file +7
-0+static int goldfish_nand_write_oob (struct mtd_info *mtd,loff_t ofs, +struct mtd_oob_ops *ops) +{ +u32 rem; + +if (ofs + ops->len > mtd->size) + goto invalid_arg;
Ditto.
...zip...
drivers/mtd/devices/goldfish_nand.c line 202
Quoted 7 lines
drivers/mtd/devices/goldfish_nand.c new file +7
-0+static int goldfish_nand_read (struct mtd_info *mtd,loff_t from,size_t len, +size_t *retlen,u_char *buf) +{ +u32 rem; + +if (from + len > mtd->size) + goto invalid_arg;
Ditto.
..zip...
drivers/mtd/devices/goldfish_nand.c line 226
Quoted 7 lines
drivers/mtd/devices/goldfish_nand.c new file +7
-0+static int goldfish_nand_write (struct mtd_info *mtd,loff_t to,size_t len, +size_t *retlen,const u_char *buf) +{ +u32 rem; + +if (to + len > mtd->size) + goto invalid_arg;
Ditto.
drivers/mtd/devices/goldfish_nand.c line 299
Quoted 12 lines
drivers/mtd/devices/goldfish_nand.c new file +12
-0+static int nand_setup_cmd_params (struct platform_device *pdev, +struct goldfish_nand *nand) +{ +u64 paddr; +unsigned char __iomem *base = nand->base; + + nand->cmd_params =devm_kzalloc (&pdev->dev, +sizeof (struct cmd_params ),GFP_KERNEL ); +if (!nand->cmd_params) +return -1 ; + + paddr =__pa (nand->cmd_params);
That looks weird (the __pa()) usage. I thought drivers should not use __pa() directly.
Quoted 7 lines
drivers/mtd/devices/goldfish_nand.c new file +7
-0+writel ((u32)(paddr >>32 ), base +NAND_CMD_PARAMS_ADDR_HIGH ); +writel ((u32 )paddr, base +NAND_CMD_PARAMS_ADDR_LOW ); +return 0 ; +} + +static int goldfish_nand_init_device (struct platform_device *pdev, +struct goldfish_nand *nand,int id)
drivers/mtd/devices/goldfish_nand.c line 316
Quoted 10 lines
drivers/mtd/devices/goldfish_nand.c new file +10
-0+{ +u32 name_len; +u32 result; +u32 flags; +unsigned long irq_flags; +unsigned char __iomem *base = nand->base; +struct mtd_info *mtd = &nand->mtd[id]; +char *name; + +spin_lock_irqsave (&nand->lock, irq_flags);
Again same spin_lock question.
Cheers.
Signature · 1 line
Robert
Quoted 3 lines
drivers/mtd/devices/goldfish_nand.c line 71
Quoted 1 line
drivers/mtd/devices/goldfish_nand.c new file +1
-0+writel (cmdp, base +NAND_COMMAND );What guarantee do you have on the order of writes here ? Isn't a write barrier required here ?
Its a virtual platform powered by an emulator - so no barriers needed that I can see.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ spin_lock_irqsave(&nand->lock, irq_flags);Why this
spin_lockand not a mutex ? I didn't see any interrupts used in this driver, have I missed something ?
The driver doesn't require it not sure about all the callers.
Quoted 11 lines
drivers/mtd/devices/goldfish_nand.c line 92
Quoted 10 lines
(quoted diff) +1
-0+ if (goldfish_nand_cmd_with_params(mtd, cmd, addr, len,ptr, &rv)) {
drivers/mtd/devices/goldfish_nand.c new file +7
-0+writel (mtd - nand->mtd, base +NAND_DEV ); +writel ((u32)(addr >>32 ), base +NAND_ADDR_HIGH ); +writel ((u32 )addr, base +NAND_ADDR_LOW ); +writel (len, base +NAND_TRANSFER_SIZE ); +writel ((u32 )ptr, base +NAND_DATA ); +writel (cmd, base +NAND_COMMAND ); + rv =readl (base +NAND_RESULT );Same question here on the order of the read wrt to previous writes.
reads wont pass write anyway as its a sane platform.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (ofs + len > mtd->size) + goto invalid_arg;I don't think that test is required, the MTD API gives already that guarantee AFAIR.
Ok
Quoted 9 lines
drivers/mtd/devices/goldfish_nand.c line 299
Quoted 7 lines
(quoted diff) +2
-0+ nand->cmd_params = devm_kzalloc(&pdev->dev, + sizeof(struct cmd_params),
GFP_KERNEL);drivers/mtd/devices/goldfish_nand.c new file +4
-0+if (!nand->cmd_params) +return -1 ; + + paddr =__pa (nand->cmd_params);That looks weird (the
__pa()) usage. I thought drivers should not use__pa()directly.
Will look at using dma_alloc_coherent for it.
Quoted 2 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ spin_lock_irqsave(&nand->lock, irq_flags);Again same
spin_lockquestion.
I'm very wary of changing this but will take a look. It's actually not that important because its not real flash so it has unusually excellent performance via the emulator.
Alan
Quoted 6 lines · Alan Cox
Quoted 3 lines
drivers/mtd/devices/goldfish_nand.c line 71
Quoted 1 line
drivers/mtd/devices/goldfish_nand.c new file +1
-0+writel (cmdp, base +NAND_COMMAND );What guarantee do you have on the order of writes here ? Isn't a write barrier required here ?
Its a virtual platform powered by an emulator - so no barriers needed that I can see.
OK.
...zip...
Quoted 11 lines
Quoted 10 lines
drivers/mtd/devices/goldfish_nand.c line 92
Quoted 9 lines
(quoted diff) +1
-0+ if (goldfish_nand_cmd_with_params(mtd, cmd, addr, len,ptr, &rv)) {
drivers/mtd/devices/goldfish_nand.c new file +7
-0+writel (mtd - nand->mtd, base +NAND_DEV ); +writel ((u32)(addr >>32 ), base +NAND_ADDR_HIGH ); +writel ((u32 )addr, base +NAND_ADDR_LOW ); +writel (len, base +NAND_TRANSFER_SIZE ); +writel ((u32 )ptr, base +NAND_DATA ); +writel (cmd, base +NAND_COMMAND ); + rv =readl (base +NAND_RESULT );Same question here on the order of the read wrt to previous writes.
Quoted 1 line
reads wont pass write anyway as its a sane platform.
Euh how so ? Assuming I understood correctly the "an emulated platform" part, and the reads+writes end up in RAM, then an out-of-order execution core can reorder independent read/writes. For example if base+NAND_RESULT is already in cache, the core can perform the readl before the last writel, can't it ?
Quoted 4 lines
Quoted 2 lines
That looks weird (the
__pa()) usage. I thought drivers should not use__pa()directly.Will look at using
dma_alloc_coherentfor it.
Cool.
Quoted 6 lines
Quoted 2 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ spin_lock_irqsave(&nand->lock, irq_flags);Again same
spin_lockquestion.I'm very wary of changing this but will take a look. It's actually not that important because its not real flash so it has unusually excellent performance via the emulator.
Sure.
BTW, I rechecked the patch a bit more, especially goldfish_nand_write_oob(). I recall that a write_oob() function should first check the mtd_oob_ops.mode and act depending on its value : MTD_OPS_PLACE_OOB, MTD_OPS_RAW, and MTD_OPS_AUTO_OOB.
The action is different on each value (or -EINVAL returned if not handled).
Cheers.
--
Robert
PS: I have the responsiveness of a diseased turttle, and I don't expect things will improve in the next days as I changed my home, so please be patient (or convince my boss :))
Quoted 3 lines
PS: I have the responsiveness of a diseased turttle, and I don't expect things will improve in the next days as I changed my home, so please be patient (or convince my boss :))
No problem. It'll be a while before I can test the lock changes so I've pushed it to staging for now with your original TODO items as the fix list. That way we have a root fs on Goldfish with a base kernel and can get the other changes done over time.
Alan