| Message ID | 20210527222706.10360-1-ryazanov.s.a@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ath79: rb4xx-nand: fix 512 byte pages compatibility | expand |
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. A few nitpicks: On 2021-05-28 00:27, Sergey Ryazanov wrote: > MikroTik boards with 512 byte NAND pages require the old YAFFS1 OOB > layout for compatibility with the RouterBoot bootloader. The RB4xx NAND > driver supports such OOB layout, but checks a NAND page size too early > before the flash identification, what effectively preventing the old > OOB > layout from being used. > > To fix this issue, move the page size check and OOB layout > configuration > to the chip attaching hook, which is specially intorduced for ECC and > OOB tweaking. > > While at it, copy a comment from the old AR71xx driver to make it > clear, > why do we need this OOB layout tweaking. > > Run tested with MikroTik RB411U board. > > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > --- > .../files/drivers/mtd/nand/raw/nand_rb4xx.c | 25 ++++++++++++++++--- > 1 file changed, 22 insertions(+), 3 deletions(-) > > diff --git > a/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c > b/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c > index 50bd69f6a4..00c65d14ae 100644 > --- a/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c > +++ b/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c > @@ -81,6 +81,23 @@ static const struct mtd_ooblayout_ops > rb4xx_nand_ecclayout_ops = { > .free = rb4xx_ooblayout_free, > }; > > +static int rb4xx_nand_attach_chip(struct nand_chip *chip) > +{ > + struct mtd_info *mtd = nand_to_mtd(chip); > + > + /* > + * Now we knows flash parameters and can tweak OOB the layout for old Rephrase to something like: Knowing the flash parameters, we can tweak the OOB layout for old > + * (usually 64MiB) flashes. > + * > + * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB > + * bootloader will not be able to find the kernel that we load. > + */ > + if (mtd->writesize == 512) > + mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); > + > + return 0; > +} > + > static u8 rb4xx_nand_read_byte(struct nand_chip *chip) > { > struct rb4xx_nand *nand = chip->priv; > @@ -135,6 +152,10 @@ static int rb4xx_nand_dev_ready(struct nand_chip > *chip) > return gpiod_get_value_cansleep(nand->rdy); > } > > +static const struct nand_controller_ops rb4xx_nand_controller_ops = { > + .attach_chip = rb4xx_nand_attach_chip, remove the , > +}; > + > static int rb4xx_nand_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -185,9 +206,6 @@ static int rb4xx_nand_probe(struct platform_device > *pdev) > mtd->dev.parent = dev; > mtd_set_of_node(mtd, dev->of_node); > > - if (mtd->writesize == 512) > - mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); > - > nand->chip.ecc.mode = NAND_ECC_SOFT; > nand->chip.ecc.algo = NAND_ECC_HAMMING; > nand->chip.options = NAND_NO_SUBPAGE_WRITE; > @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device > *pdev) > nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; > nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; > nand->chip.legacy.chip_delay = 25; > + nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops; Fix indentation for all nand->something assignments to line up the = sign > > ret = nand_scan(&nand->chip, 1); > if (ret)
Hello Bas, thank you for your review, please find my comments below. On Fri, May 28, 2021 at 11:41 AM Bas Mevissen <abuse@basmevissen.nl> wrote: > On 2021-05-28 00:27, Sergey Ryazanov wrote: [skipped] >> +static int rb4xx_nand_attach_chip(struct nand_chip *chip) >> +{ >> + struct mtd_info *mtd = nand_to_mtd(chip); >> + >> + /* >> + * Now we knows flash parameters and can tweak OOB the layout for old > > Rephrase to something like: > Knowing the flash parameters, we can tweak the OOB layout for old > Yeah, I am not happy with this comment too, but this is the best I was able to compose at 01:00 :) I will rephrase it if V2 will be needed. Here we need a comment that briefly and clearly states that the NAND params become known at this particular moment of initialization. Before this moment, we do not know the page size, after this moment it is too late to reconfigure something. Do you have any thoughts? >> + * (usually 64MiB) flashes. >> + * >> + * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB >> + * bootloader will not be able to find the kernel that we load. >> + */ >> + if (mtd->writesize == 512) >> + mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); >> + >> + return 0; >> +} >> + >> static u8 rb4xx_nand_read_byte(struct nand_chip *chip) >> { >> struct rb4xx_nand *nand = chip->priv; >> @@ -135,6 +152,10 @@ static int rb4xx_nand_dev_ready(struct nand_chip >> *chip) >> return gpiod_get_value_cansleep(nand->rdy); >> } >> >> +static const struct nand_controller_ops rb4xx_nand_controller_ops = { >> + .attach_chip = rb4xx_nand_attach_chip, > > remove the , I intentionally placed the comma here to make text git-friendly. If we will need to define more callbacks here, then we will just add a new new line, without modifying the earlier added lines. E.g. if commit this code without the comma, then a chip detaching callback patch will looks like this: static const struct nand_controller_ops rb4xx_nand_controller_ops = { - .attach_chip = rb4xx_nand_attach_chip + .attach_chip = rb4xx_nand_attach_chip, + .detach_chip = rb4xx_nand_detach_chip, }; With this intentionally placed comma, the same theoretical patch will looks like this: static const struct nand_controller_ops rb4xx_nand_controller_ops = { .attach_chip = rb4xx_nand_attach_chip, + .detach_chip = rb4xx_nand_detach_chip, }; So this comma is my investment in the future ;) >> +}; >> + >> static int rb4xx_nand_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -185,9 +206,6 @@ static int rb4xx_nand_probe(struct platform_device >> *pdev) >> mtd->dev.parent = dev; >> mtd_set_of_node(mtd, dev->of_node); >> >> - if (mtd->writesize == 512) >> - mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); >> - >> nand->chip.ecc.mode = NAND_ECC_SOFT; >> nand->chip.ecc.algo = NAND_ECC_HAMMING; >> nand->chip.options = NAND_NO_SUBPAGE_WRITE; >> @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device >> *pdev) >> nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; >> nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; >> nand->chip.legacy.chip_delay = 25; >> + nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops; > > Fix indentation for all nand->something assignments to line up the = > sign I do not think that this is a good idea for this patch. Here we add one line that configures the single field deep inside the structure. The line is placed after the configuration of "surface" fields. So the overall look should not be harmed dreadfully. In case we will rework the indentation with this patch, the 57 lines patch will become a ~70 lines patch. Where at least 12 lines will rework indentation, so the functional part could be easily lost. Also the moving text back and forth within an item line, turns the history to the white noise and makes git-blame(1) usage a pain. If you prefer, then I could insert an empty line before the ops setup to make it nice looking. E.g. turn this hunk: @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device *pdev) nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; nand->chip.legacy.chip_delay = 25; + nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops; into this: @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device *pdev) nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; nand->chip.legacy.chip_delay = 25; + + nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops; But I do not think this rework is worth the V2 on its own.
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. On 2021-05-28 13:55, Sergey Ryazanov wrote: > Hello Bas, > > thank you for your review, please find my comments below. > > On Fri, May 28, 2021 at 11:41 AM Bas Mevissen <abuse@basmevissen.nl> > wrote: >> On 2021-05-28 00:27, Sergey Ryazanov wrote: > > [skipped] > >>> +static int rb4xx_nand_attach_chip(struct nand_chip *chip) >>> +{ >>> + struct mtd_info *mtd = nand_to_mtd(chip); >>> + >>> + /* >>> + * Now we knows flash parameters and can tweak OOB the layout >>> for old >> >> Rephrase to something like: >> Knowing the flash parameters, we can tweak the OOB layout for old >> > > Yeah, I am not happy with this comment too, but this is the best I was > able to compose at 01:00 :) I will rephrase it if V2 will be needed. > > Here we need a comment that briefly and clearly states that the NAND > params become known at this particular moment of initialization. > Before this moment, we do not know the page size, after this moment it > is too late to reconfigure something. Do you have any thoughts? > The original comment with some spelling/grammar corrections looked fine. Having some hint that something is deliberately done at that stage is sufficient IMHO. >>> + * (usually 64MiB) flashes. >>> + * >>> + * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB >>> + * bootloader will not be able to find the kernel that we load. >>> + */ >>> + if (mtd->writesize == 512) >>> + mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); >>> + >>> + return 0; >>> +} >>> + >>> static u8 rb4xx_nand_read_byte(struct nand_chip *chip) >>> { >>> struct rb4xx_nand *nand = chip->priv; >>> @@ -135,6 +152,10 @@ static int rb4xx_nand_dev_ready(struct nand_chip >>> *chip) >>> return gpiod_get_value_cansleep(nand->rdy); >>> } >>> >>> +static const struct nand_controller_ops rb4xx_nand_controller_ops = >>> { >>> + .attach_chip = rb4xx_nand_attach_chip, >> >> remove the , > > I intentionally placed the comma here to make text git-friendly. If we > will need to define more callbacks here, then we will just add a new > new line, without modifying the earlier added lines. > Agreed. It actually sounds like a good practice. Learned something today :-) > E.g. if commit this code without the comma, then a chip detaching > callback patch will looks like this: > > static const struct nand_controller_ops rb4xx_nand_controller_ops = { > - .attach_chip = rb4xx_nand_attach_chip > + .attach_chip = rb4xx_nand_attach_chip, > + .detach_chip = rb4xx_nand_detach_chip, > }; > > With this intentionally placed comma, the same theoretical patch will > looks like this: > > static const struct nand_controller_ops rb4xx_nand_controller_ops = { > .attach_chip = rb4xx_nand_attach_chip, > + .detach_chip = rb4xx_nand_detach_chip, > }; > > So this comma is my investment in the future ;) > >>> +}; >>> + >>> static int rb4xx_nand_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -185,9 +206,6 @@ static int rb4xx_nand_probe(struct >>> platform_device >>> *pdev) >>> mtd->dev.parent = dev; >>> mtd_set_of_node(mtd, dev->of_node); >>> >>> - if (mtd->writesize == 512) >>> - mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); >>> - >>> nand->chip.ecc.mode = NAND_ECC_SOFT; >>> nand->chip.ecc.algo = NAND_ECC_HAMMING; >>> nand->chip.options = NAND_NO_SUBPAGE_WRITE; >>> @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct >>> platform_device >>> *pdev) >>> nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; >>> nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; >>> nand->chip.legacy.chip_delay = 25; >>> + nand->chip.legacy.dummy_controller.ops = >>> &rb4xx_nand_controller_ops; >> >> Fix indentation for all nand->something assignments to line up the = >> sign > > I do not think that this is a good idea for this patch. Here we add > one line that configures the single field deep inside the structure. > The line is placed after the configuration of "surface" fields. So the > overall look should not be harmed dreadfully. > > In case we will rework the indentation with this patch, the 57 lines > patch will become a ~70 lines patch. Where at least 12 lines will > rework indentation, so the functional part could be easily lost. Also > the moving text back and forth within an item line, turns the history > to the white noise and makes git-blame(1) usage a pain. > > If you prefer, then I could insert an empty line before the ops setup > to make it nice looking. E.g. turn this hunk: > > @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device > *pdev) > nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; > nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; > nand->chip.legacy.chip_delay = 25; > + nand->chip.legacy.dummy_controller.ops = > &rb4xx_nand_controller_ops; > > into this: > > @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device > *pdev) > nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; > nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; > nand->chip.legacy.chip_delay = 25; > + > + nand->chip.legacy.dummy_controller.ops = > &rb4xx_nand_controller_ops; > > But I do not think this rework is worth the V2 on its own. Agreed. For that reason, I stopped trying to align such lists. It usually gets messy anyway: if you start with something like: short_var.x = foo; short_var.val = 100; and add next week: long_long_long_name.y = blabla; long_long_long_name.long_other_name = blabla; the file as a whole still looks messy. I would say, just fix up the comment a bit and send a V2. Bas.
diff --git a/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c b/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c index 50bd69f6a4..00c65d14ae 100644 --- a/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c +++ b/target/linux/ath79/files/drivers/mtd/nand/raw/nand_rb4xx.c @@ -81,6 +81,23 @@ static const struct mtd_ooblayout_ops rb4xx_nand_ecclayout_ops = { .free = rb4xx_ooblayout_free, }; +static int rb4xx_nand_attach_chip(struct nand_chip *chip) +{ + struct mtd_info *mtd = nand_to_mtd(chip); + + /* + * Now we knows flash parameters and can tweak OOB the layout for old + * (usually 64MiB) flashes. + * + * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB + * bootloader will not be able to find the kernel that we load. + */ + if (mtd->writesize == 512) + mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); + + return 0; +} + static u8 rb4xx_nand_read_byte(struct nand_chip *chip) { struct rb4xx_nand *nand = chip->priv; @@ -135,6 +152,10 @@ static int rb4xx_nand_dev_ready(struct nand_chip *chip) return gpiod_get_value_cansleep(nand->rdy); } +static const struct nand_controller_ops rb4xx_nand_controller_ops = { + .attach_chip = rb4xx_nand_attach_chip, +}; + static int rb4xx_nand_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -185,9 +206,6 @@ static int rb4xx_nand_probe(struct platform_device *pdev) mtd->dev.parent = dev; mtd_set_of_node(mtd, dev->of_node); - if (mtd->writesize == 512) - mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops); - nand->chip.ecc.mode = NAND_ECC_SOFT; nand->chip.ecc.algo = NAND_ECC_HAMMING; nand->chip.options = NAND_NO_SUBPAGE_WRITE; @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device *pdev) nand->chip.legacy.cmd_ctrl = rb4xx_nand_cmd_ctrl; nand->chip.legacy.dev_ready = rb4xx_nand_dev_ready; nand->chip.legacy.chip_delay = 25; + nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops; ret = nand_scan(&nand->chip, 1); if (ret)
MikroTik boards with 512 byte NAND pages require the old YAFFS1 OOB layout for compatibility with the RouterBoot bootloader. The RB4xx NAND driver supports such OOB layout, but checks a NAND page size too early before the flash identification, what effectively preventing the old OOB layout from being used. To fix this issue, move the page size check and OOB layout configuration to the chip attaching hook, which is specially intorduced for ECC and OOB tweaking. While at it, copy a comment from the old AR71xx driver to make it clear, why do we need this OOB layout tweaking. Run tested with MikroTik RB411U board. Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> --- .../files/drivers/mtd/nand/raw/nand_rb4xx.c | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)