| Message ID | 1439668968-3882-5-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Hans de Goede |
| Headers | show |
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote: > In syndrome mode we set the NFC_SEQ bit in the command register, so the > spare-area register is not used. Also the value currently being written is > actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" > not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE. > > So the current code only serves to confuse the user -> remove it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> There's a bunch of other uses of the syndrome parameter in this function. Does syndrome=true work even without this particular bit of code? I suppose I'm asking, should the paramter and the other uses be removed? Or should an ASSERT(!syndrome) be added, or am I worrying about nothing and everything is just fine as it is after this patch? I suspect the latter, so if that is indeed the case: Acked-by: Ian Campbell < ijc@hellion.org.uk >
Hi, On 17-08-15 10:19, Ian Campbell wrote: > On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote: >> In syndrome mode we set the NFC_SEQ bit in the command register, so the >> spare-area register is not used. Also the value currently being written is >> actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" >> not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE. >> >> So the current code only serves to confuse the user -> remove it. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > There's a bunch of other uses of the syndrome parameter in this > function. Does syndrome=true work even without this particular bit of > code? Yes, since in syndrome we are passing in the NFC_SEQ bit in the command register, which tells the controller that the ecc data is directly behind the actual data, the spare-area address register is not used. I stumbled over this because the code we had was writing the wrong address for the ecc data to the spare-area address register, leading me to wonder why syndrome mode was working at all. > I suppose I'm asking, should the paramter and the other uses be > removed? Or should an ASSERT(!syndrome) be added, or am I worrying > about nothing and everything is just fine as it is after this patch? Everything is just fine after this patch, actually it was fine before this patch except that it did an unnecessary write, with the wrong ecc data address which I found confusing, hence this patch to remove that write. > > I suspect the latter, so if that is indeed the case: > Acked-by: Ian Campbell < ijc@hellion.org.uk > > Thanks for all the reviews. Regards, Hans
On Mon, 2015-08-17 at 10:59 +0200, Hans de Goede wrote: > Hi, > > On 17-08-15 10:19, Ian Campbell wrote: > > On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote: > > > In syndrome mode we set the NFC_SEQ bit in the command register, > > > so the > > > spare-area register is not used. Also the value currently being > > > written is > > > actual wrong, the ecc sits at "column + > > > CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" > > > not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE. > > > > > > So the current code only serves to confuse the user -> remove it. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > There's a bunch of other uses of the syndrome parameter in this > > function. Does syndrome=true work even without this particular bit > > of > > code? > > Yes, since in syndrome we are passing in the NFC_SEQ bit in the > command > register, which tells the controller that the ecc data is directly > behind > the actual data, the spare-area address register is not used. > > I stumbled over this because the code we had was writing the wrong address > for the ecc data to the spare-area address register, leading me to wonder > why syndrome mode was working at all. Right, that was the line of thinking which took me here too. > > I suppose I'm asking, should the paramter and the other uses be > > removed? Or should an ASSERT(!syndrome) be added, or am I worrying > > about nothing and everything is just fine as it is after this > > patch? > > Everything is just fine after this patch, actually it was fine before > this patch except that it did an unnecessary write, with the wrong > ecc data address which I found confusing, hence this patch to remove > that write. Super, thanks for the explanation. The Ack stands then. > > > > > I suspect the latter, so if that is indeed the case: > > Acked-by: Ian Campbell < ijc@hellion.org.uk > > > > > Thanks for all the reviews. > > Regards, > > Hans >
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 56c0be0..f6f4928 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -256,10 +256,7 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, val = readl(SUNXI_NFC_BASE + NFC_CTL); writel(val | NFC_CTL_RAM_METHOD, SUNXI_NFC_BASE + NFC_CTL); - if (syndrome) { - writel(CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, - SUNXI_NFC_BASE + NFC_SPARE_AREA); - } else { + if (!syndrome) { oob_offset = CONFIG_NAND_SUNXI_SPL_PAGE_SIZE + (column / CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) * ecc_off;
In syndrome mode we set the NFC_SEQ bit in the command register, so the spare-area register is not used. Also the value currently being written is actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE. So the current code only serves to confuse the user -> remove it. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mtd/nand/sunxi_nand_spl.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)