| Message ID | 20200427082028.394719-1-boris.brezillon@collabora.com |
|---|---|
| Headers | show |
| Series | mtd: rawnand: cafe: Convert to exec_op() (and more) | expand |
Hello, +Lubomir Rintel in Cc. On Mon, 27 Apr 2020 10:20:10 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > A bit of context to explain the motivation behind those conversions > I've been sending for the last couple of weeks. The raw NAND subsystem > carries a lot of history which makes any rework not only painful, but > also subject to regressions which we only detect when someone dares to > update its kernel on one of those ancient HW. While carrying drivers > for old HW is not a problem per se, carrying ancient and unmaintained > drivers that are not converted to new APIs is a maintenance burden, > hence this massive conversion attempt I'm conducting here. > > So here it is, a series converting the CAFE NAND controller driver to > exec_op(), plus a bunch of minor improvements done along the way. > I hope I'll find someone to test those changes, but if there's no one > still owning OLPC HW or no interest in keeping it supported in recent > kernel versions, we should definitely consider removing the driver > instead. Lubomir Rintel (in Cc) has very recently added defconfigs to Buildroot to support the two OLPC platforms (the Intel based one and the Marvell MMP based one). I suppose this means he has access to the hardware, so hopefully he should be able to test these NAND driver changes. Best regards, Thomas
Hi Thomas, On Wed, 29 Apr 2020 08:37:35 +0200 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > +Lubomir Rintel in Cc. > > On Mon, 27 Apr 2020 10:20:10 +0200 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > A bit of context to explain the motivation behind those conversions > > I've been sending for the last couple of weeks. The raw NAND subsystem > > carries a lot of history which makes any rework not only painful, but > > also subject to regressions which we only detect when someone dares to > > update its kernel on one of those ancient HW. While carrying drivers > > for old HW is not a problem per se, carrying ancient and unmaintained > > drivers that are not converted to new APIs is a maintenance burden, > > hence this massive conversion attempt I'm conducting here. > > > > So here it is, a series converting the CAFE NAND controller driver to > > exec_op(), plus a bunch of minor improvements done along the way. > > I hope I'll find someone to test those changes, but if there's no one > > still owning OLPC HW or no interest in keeping it supported in recent > > kernel versions, we should definitely consider removing the driver > > instead. > > Lubomir Rintel (in Cc) has very recently added defconfigs to Buildroot > to support the two OLPC platforms (the Intel based one and the Marvell > MMP based one). I suppose this means he has access to the hardware, so > hopefully he should be able to test these NAND driver changes. Oh, that's great news! Thanks a lot for letting me know. Lubomir, let me know if you'd be okay to test those changes. I can provide a branch if that's easier. Regards, Boris
On Fri, 1 May 2020 07:52:09 +0200 Lubomir Rintel <lkundrak@v3.sk> wrote: > On Wed, Apr 29, 2020 at 10:28:41AM +0200, Boris Brezillon wrote: > > Hi Thomas, > > > > On Wed, 29 Apr 2020 08:37:35 +0200 > > Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > > > > Hello, > > > > > > +Lubomir Rintel in Cc. > > > > > > On Mon, 27 Apr 2020 10:20:10 +0200 > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > A bit of context to explain the motivation behind those conversions > > > > I've been sending for the last couple of weeks. The raw NAND subsystem > > > > carries a lot of history which makes any rework not only painful, but > > > > also subject to regressions which we only detect when someone dares to > > > > update its kernel on one of those ancient HW. While carrying drivers > > > > for old HW is not a problem per se, carrying ancient and unmaintained > > > > drivers that are not converted to new APIs is a maintenance burden, > > > > hence this massive conversion attempt I'm conducting here. > > > > > > > > So here it is, a series converting the CAFE NAND controller driver to > > > > exec_op(), plus a bunch of minor improvements done along the way. > > > > I hope I'll find someone to test those changes, but if there's no one > > > > still owning OLPC HW or no interest in keeping it supported in recent > > > > kernel versions, we should definitely consider removing the driver > > > > instead. > > > > > > Lubomir Rintel (in Cc) has very recently added defconfigs to Buildroot > > > to support the two OLPC platforms (the Intel based one and the Marvell > > > MMP based one). I suppose this means he has access to the hardware, so > > > hopefully he should be able to test these NAND driver changes. > > > > Oh, that's great news! Thanks a lot for letting me know. > > > > Lubomir, let me know if you'd be okay to test those changes. I can > > provide a branch if that's easier. > > Boris, I'd be happy to test the changes. A branch would help. Here it is [1]. > I've > applied the patches on top of 5.7-rc3 and while they applied cleanly, > they've failed to build: > > drivers/mtd/nand/raw/cafe_nand.c: In function ‘cafe_nand_exec_subop’: > drivers/mtd/nand/raw/cafe_nand.c:581:19: error: ‘const struct nand_subop’ has no member named ‘cs’ > 581 | if (WARN_ON(subop->cs > 1)) > | ^~ Yep, it depends on a patch that was part of a separate series. I'm here to help if you have any issues. Thanks, Boris [1]https://github.com/bbrezillon/linux/tree/nand/exec-op-conversion
On Sat, 2 May 2020 13:27:32 +0200 Lubomir Rintel <lkundrak@v3.sk> wrote: > Boris Brezillon wrote: > > Hello, > > > > A bit of context to explain the motivation behind those conversions > > I've been sending for the last couple of weeks. The raw NAND subsystem > > carries a lot of history which makes any rework not only painful, but > > also subject to regressions which we only detect when someone dares to > > update its kernel on one of those ancient HW. While carrying drivers > > for old HW is not a problem per se, carrying ancient and unmaintained > > drivers that are not converted to new APIs is a maintenance burden, > > hence this massive conversion attempt I'm conducting here. > > > > So here it is, a series converting the CAFE NAND controller driver to > > exec_op(), plus a bunch of minor improvements done along the way. > > I hope I'll find someone to test those changes, but if there's no one > > still owning OLPC HW or no interest in keeping it supported in recent > > kernel versions, we should definitely consider removing the driver > > instead. > > > > Regards, > > > > Boris > > > > Boris Brezillon (17): > > mtd: rawnand: cafe: Get rid of an inaccurate kernel doc header > > mtd: rawnand: cafe: Rename cafe_nand_write_page_lowlevel() > > mtd: rawnand: cafe: Use a correct ECC mode and pass the ECC alg > > mtd: rawnand: cafe: Include linux/io.h instead of asm/io.h > > mtd: rawnand: cafe: Demistify register fields > > mtd: rawnand: cafe: Factor out the controller initialization logic > > mtd: rawnand: cafe: Get rid of the debug module param > > mtd: rawnand: cafe: Use devm_kzalloc and devm_request_irq() > > mtd: rawnand: cafe: Get rid of a useless label > > mtd: rawnand: cafe: Explicitly inherit from nand_controller > > mtd: rawnand: cafe: Don't leave ECC enabled in the write path > > mtd: rawnand: cafe: Don't split things when reading/writing a page > > mtd: rawnand: cafe: Add exec_op() support > > mtd: rawnand: cafe: Get rid of the legacy interface implementation > > mtd: rawnand: cafe: Adjust the cafe_{read,write}_buf() prototypes > > mtd: rawnand: cafe: Handle non-32bit aligned reads/writes > > mtd: rawnand: cafe: s/uint{8,16,32}_t/u{8,16,32}/ > > > > drivers/mtd/nand/raw/cafe_nand.c | 805 ++++++++++++++++--------------- > > 1 file changed, 423 insertions(+), 382 deletions(-) > > Thanks for doing this. With a couple of changes I've indicated in responses to > some of the patches this has been: > > Tested-by: Lubomir Rintel <lkundrak@v3.sk> Thanks a lot for testing *and debugging* my changes. I must admit that was unexpected, and I'm amazed by how fast I got feedback on that one :-). Kudos to Thomas as well for noticing the email and getting us in touch. > > Other than that, I have a couple of suggestions (I'm not really in a position > to demand them, but they would've done the review easier for me): > > 1.) I'm wondering if we could remove these: > > /* Make it easier to switch to PIO if we need to */ > #define cafe_readl(cafe, addr) readl((cafe)->mmio + CAFE_##addr) > #define cafe_writel(cafe, datum, addr) writel(datum, (cafe)->mmio + CAFE_##addr) > > Or at least don't add new calls to them in our patches and call > readl()/writel() directly. The string pasting makes it impossible to grep > for the register names. > > It's not like a switch to PIO is ever going to happen and with an instance > of readl_poll_timeout() added it's not like it would be a matter of > rewriting those macros. I can certainly do that. > > 2.) When the block after a conditional is multiple lines, could you please > include the curly braces? That is: > > if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) { > cafe_read_buf(chip, > subop->instrs[data_instr].ctx.data.buf.in + > nand_subop_get_data_start_off(subop, data_instr), > nand_subop_get_data_len(subop, data_instr)); > } > > Instead of: > > if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) > cafe_read_buf(chip, > subop->instrs[data_instr].ctx.data.buf.in + > nand_subop_get_data_start_off(subop, data_instr), > nand_subop_get_data_len(subop, data_instr)); > > This makes things significantly easier to read for me, not to mention that it > comes handy to have the braces around for printf debugging. I do prefer the version without brackets, but given you debugged it, I'd be okay changing that one ;-) (assuming Miquel is okay with that too, of course).
> > 2.) When the block after a conditional is multiple lines, could you please > > include the curly braces? That is: > > > > if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) { > > cafe_read_buf(chip, > > subop->instrs[data_instr].ctx.data.buf.in + > > nand_subop_get_data_start_off(subop, data_instr), > > nand_subop_get_data_len(subop, data_instr)); > > } > > > > Instead of: > > > > if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) > > cafe_read_buf(chip, > > subop->instrs[data_instr].ctx.data.buf.in + > > nand_subop_get_data_start_off(subop, data_instr), > > nand_subop_get_data_len(subop, data_instr)); > > > > This makes things significantly easier to read for me, not to mention that it > > comes handy to have the braces around for printf debugging. > > I do prefer the version without brackets, but given you debugged it, > I'd be okay changing that one ;-) (assuming Miquel is okay with that > too, of course). I also prefer having curly braces around single-multiline-instructions :)
Hello, A bit of context to explain the motivation behind those conversions I've been sending for the last couple of weeks. The raw NAND subsystem carries a lot of history which makes any rework not only painful, but also subject to regressions which we only detect when someone dares to update its kernel on one of those ancient HW. While carrying drivers for old HW is not a problem per se, carrying ancient and unmaintained drivers that are not converted to new APIs is a maintenance burden, hence this massive conversion attempt I'm conducting here. So here it is, a series converting the CAFE NAND controller driver to exec_op(), plus a bunch of minor improvements done along the way. I hope I'll find someone to test those changes, but if there's no one still owning OLPC HW or no interest in keeping it supported in recent kernel versions, we should definitely consider removing the driver instead. Regards, Boris Boris Brezillon (17): mtd: rawnand: cafe: Get rid of an inaccurate kernel doc header mtd: rawnand: cafe: Rename cafe_nand_write_page_lowlevel() mtd: rawnand: cafe: Use a correct ECC mode and pass the ECC alg mtd: rawnand: cafe: Include linux/io.h instead of asm/io.h mtd: rawnand: cafe: Demistify register fields mtd: rawnand: cafe: Factor out the controller initialization logic mtd: rawnand: cafe: Get rid of the debug module param mtd: rawnand: cafe: Use devm_kzalloc and devm_request_irq() mtd: rawnand: cafe: Get rid of a useless label mtd: rawnand: cafe: Explicitly inherit from nand_controller mtd: rawnand: cafe: Don't leave ECC enabled in the write path mtd: rawnand: cafe: Don't split things when reading/writing a page mtd: rawnand: cafe: Add exec_op() support mtd: rawnand: cafe: Get rid of the legacy interface implementation mtd: rawnand: cafe: Adjust the cafe_{read,write}_buf() prototypes mtd: rawnand: cafe: Handle non-32bit aligned reads/writes mtd: rawnand: cafe: s/uint{8,16,32}_t/u{8,16,32}/ drivers/mtd/nand/raw/cafe_nand.c | 805 ++++++++++++++++--------------- 1 file changed, 423 insertions(+), 382 deletions(-)