Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Le sam. 12 mai 2018 à 20:02, Boris Brezillona écrit : On Sat, 12 May 2018 19:42:49 +0200 Paul Cercueil wrote: >> >> My motivation is to get rid of this (move it to devicetree): >> >> >> >> >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 >> >> And enable the support of other boards with custom OOB layouts. >> > >> > Can you list the different layouts you have? I'm pretty sure >> there's a >> > pattern. Maybe we can even deduce the layout from the page size >> or OOB >> > size. >> >> This is the other layout I have for another ingenic device: >> >> http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 >> >> Page size and OOB size are the same between these two devices. > > Indeed. Do you know if there are other kind of layouts in the wild? I'm getting a new board in a few weeks, I'll be able to check that out. > Note that can be a string, so if each each board is > defining its own layout, you could specify the board name here. > Otherwise, if you just have those 2 patterns, you can just name them > "contiguous" and "interleaved". I don't like the idea of adding board-specific data inside the driver... I'd prefer to use the method I used in this patch, but inside the jz4740-nand driver, if you're OK with it. Please don't. Encoding such detailed description in the DT has almost always proven to be poor choice. Also, people are likely to get it wrong, and then you'll have to fix all DTs, while, with a single unique ID representing the layout, you can - re-use existing layouts easily without having to describe everything again in the DT - fix the driver without getting in trouble with people who claim that DT is a stable ABI and don't want to update their DT So, please just pick user-friendly IDs and add layout definitions in the driver. If you don't want to leak board info in the driver, then don't name the layout with the board name. BTW, I still hope you'll only have 2 kind of layouts (contiguous and interleaved). Alright. Thanks for the insights. -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Le sam. 12 mai 2018 à 20:02, Boris Brezillon a écrit : On Sat, 12 May 2018 19:42:49 +0200 Paul Cercueil wrote: >> >> My motivation is to get rid of this (move it to devicetree): >> >> >> >> >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 >> >> And enable the support of other boards with custom OOB layouts. >> > >> > Can you list the different layouts you have? I'm pretty sure >> there's a >> > pattern. Maybe we can even deduce the layout from the page size >> or OOB >> > size. >> >> This is the other layout I have for another ingenic device: >> >> http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 >> >> Page size and OOB size are the same between these two devices. > > Indeed. Do you know if there are other kind of layouts in the wild? I'm getting a new board in a few weeks, I'll be able to check that out. > Note that can be a string, so if each each board is > defining its own layout, you could specify the board name here. > Otherwise, if you just have those 2 patterns, you can just name them > "contiguous" and "interleaved". I don't like the idea of adding board-specific data inside the driver... I'd prefer to use the method I used in this patch, but inside the jz4740-nand driver, if you're OK with it. Please don't. Encoding such detailed description in the DT has almost always proven to be poor choice. Also, people are likely to get it wrong, and then you'll have to fix all DTs, while, with a single unique ID representing the layout, you can - re-use existing layouts easily without having to describe everything again in the DT - fix the driver without getting in trouble with people who claim that DT is a stable ABI and don't want to update their DT So, please just pick user-friendly IDs and add layout definitions in the driver. If you don't want to leak board info in the driver, then don't name the layout with the board name. BTW, I still hope you'll only have 2 kind of layouts (contiguous and interleaved). Alright. Thanks for the insights. -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 19:42:49 +0200 Paul Cercueilwrote: > >> >> My motivation is to get rid of this (move it to devicetree): > >> >> > >> >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> > >> >> And enable the support of other boards with custom OOB layouts. > >> > > >> > Can you list the different layouts you have? I'm pretty sure > >> there's a > >> > pattern. Maybe we can even deduce the layout from the page size > >> or OOB > >> > size. > >> > >> This is the other layout I have for another ingenic device: > >> > >> http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > >> > >> Page size and OOB size are the same between these two devices. > > > > Indeed. Do you know if there are other kind of layouts in the wild? > > I'm getting a new board in a few weeks, I'll be able to check that out. > > > Note that can be a string, so if each each board is > > defining its own layout, you could specify the board name here. > > Otherwise, if you just have those 2 patterns, you can just name them > > "contiguous" and "interleaved". > > I don't like the idea of adding board-specific data inside the driver... > I'd prefer to use the method I used in this patch, but inside the > jz4740-nand driver, if you're OK with it. Please don't. Encoding such detailed description in the DT has almost always proven to be poor choice. Also, people are likely to get it wrong, and then you'll have to fix all DTs, while, with a single unique ID representing the layout, you can - re-use existing layouts easily without having to describe everything again in the DT - fix the driver without getting in trouble with people who claim that DT is a stable ABI and don't want to update their DT So, please just pick user-friendly IDs and add layout definitions in the driver. If you don't want to leak board info in the driver, then don't name the layout with the board name. BTW, I still hope you'll only have 2 kind of layouts (contiguous and interleaved).
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 19:42:49 +0200 Paul Cercueil wrote: > >> >> My motivation is to get rid of this (move it to devicetree): > >> >> > >> >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> > >> >> And enable the support of other boards with custom OOB layouts. > >> > > >> > Can you list the different layouts you have? I'm pretty sure > >> there's a > >> > pattern. Maybe we can even deduce the layout from the page size > >> or OOB > >> > size. > >> > >> This is the other layout I have for another ingenic device: > >> > >> http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > >> > >> Page size and OOB size are the same between these two devices. > > > > Indeed. Do you know if there are other kind of layouts in the wild? > > I'm getting a new board in a few weeks, I'll be able to check that out. > > > Note that can be a string, so if each each board is > > defining its own layout, you could specify the board name here. > > Otherwise, if you just have those 2 patterns, you can just name them > > "contiguous" and "interleaved". > > I don't like the idea of adding board-specific data inside the driver... > I'd prefer to use the method I used in this patch, but inside the > jz4740-nand driver, if you're OK with it. Please don't. Encoding such detailed description in the DT has almost always proven to be poor choice. Also, people are likely to get it wrong, and then you'll have to fix all DTs, while, with a single unique ID representing the layout, you can - re-use existing layouts easily without having to describe everything again in the DT - fix the driver without getting in trouble with people who claim that DT is a stable ABI and don't want to update their DT So, please just pick user-friendly IDs and add layout definitions in the driver. If you don't want to leak board info in the driver, then don't name the layout with the board name. BTW, I still hope you'll only have 2 kind of layouts (contiguous and interleaved).
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
>> My motivation is to get rid of this (move it to devicetree): >> >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 >> And enable the support of other boards with custom OOB layouts. > > Can you list the different layouts you have? I'm pretty sure there's a > pattern. Maybe we can even deduce the layout from the page size or OOB > size. This is the other layout I have for another ingenic device: http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 Page size and OOB size are the same between these two devices. Indeed. Do you know if there are other kind of layouts in the wild? I'm getting a new board in a few weeks, I'll be able to check that out. Note that can be a string, so if each each board is defining its own layout, you could specify the board name here. Otherwise, if you just have those 2 patterns, you can just name them "contiguous" and "interleaved". I don't like the idea of adding board-specific data inside the driver... I'd prefer to use the method I used in this patch, but inside the jz4740-nand driver, if you're OK with it. Thanks, -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
>> My motivation is to get rid of this (move it to devicetree): >> >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 >> And enable the support of other boards with custom OOB layouts. > > Can you list the different layouts you have? I'm pretty sure there's a > pattern. Maybe we can even deduce the layout from the page size or OOB > size. This is the other layout I have for another ingenic device: http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 Page size and OOB size are the same between these two devices. Indeed. Do you know if there are other kind of layouts in the wild? I'm getting a new board in a few weeks, I'll be able to check that out. Note that can be a string, so if each each board is defining its own layout, you could specify the board name here. Otherwise, if you just have those 2 patterns, you can just name them "contiguous" and "interleaved". I don't like the idea of adding board-specific data inside the driver... I'd prefer to use the method I used in this patch, but inside the jz4740-nand driver, if you're OK with it. Thanks, -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 11:38:26 -0300 Paul Cercueilwrote: > Le sam. 12 mai 2018 à 10:42, Boris Brezillon > a écrit : > > On Sat, 12 May 2018 08:55:40 -0300 > > Paul Cercueil wrote: > > > >> Hi Boris, > >> > >> Le 12 mai 2018 02:55, Boris Brezillon > >> a écrit : > >> > > >> > Hi Paul, > >> > > >> > On Fri, 11 May 2018 23:29:12 +0200 > >> > Paul Cercueil wrote: > >> > > >> > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", > >> it is > >> > > now possible to specify from devicetree where the ECC data is > >> located > >> > > inside the OOB region. > >> > > >> > Why would we want to do that? I mean, ECC/free regions are ECC > >> > controller dependent (and NAND chip dependent for the OOB size > >> part), > >> > so there's no reason to describe it in the DT. And more > >> importantly, > >> > people are likely to get it wrong. > >> > > >> > I'm curious, why do you need that? > >> > >> Good question. > >> > >> The reason is that some SoCs have no ECC controller. > >> The various boards for these SoCs then all use a different layout. > > > > Okay. Still think defining the layouts in the DT is a bad idea. We > > can add a jz4740 specific property to define the layout id > > (ingenic,nand-oob-layout = ), but not a generic way to > > define custom layouts for all kind of NAND controller. > > Okay. > > >> > >> My motivation is to get rid of this (move it to devicetree): > >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> And enable the support of other boards with custom OOB layouts. > > > > Can you list the different layouts you have? I'm pretty sure there's a > > pattern. Maybe we can even deduce the layout from the page size or OOB > > size. > > This is the other layout I have for another ingenic device: > http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > > Page size and OOB size are the same between these two devices. Indeed. Do you know if there are other kind of layouts in the wild? Note that can be a string, so if each each board is defining its own layout, you could specify the board name here. Otherwise, if you just have those 2 patterns, you can just name them "contiguous" and "interleaved".
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 11:38:26 -0300 Paul Cercueil wrote: > Le sam. 12 mai 2018 à 10:42, Boris Brezillon > a écrit : > > On Sat, 12 May 2018 08:55:40 -0300 > > Paul Cercueil wrote: > > > >> Hi Boris, > >> > >> Le 12 mai 2018 02:55, Boris Brezillon > >> a écrit : > >> > > >> > Hi Paul, > >> > > >> > On Fri, 11 May 2018 23:29:12 +0200 > >> > Paul Cercueil wrote: > >> > > >> > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", > >> it is > >> > > now possible to specify from devicetree where the ECC data is > >> located > >> > > inside the OOB region. > >> > > >> > Why would we want to do that? I mean, ECC/free regions are ECC > >> > controller dependent (and NAND chip dependent for the OOB size > >> part), > >> > so there's no reason to describe it in the DT. And more > >> importantly, > >> > people are likely to get it wrong. > >> > > >> > I'm curious, why do you need that? > >> > >> Good question. > >> > >> The reason is that some SoCs have no ECC controller. > >> The various boards for these SoCs then all use a different layout. > > > > Okay. Still think defining the layouts in the DT is a bad idea. We > > can add a jz4740 specific property to define the layout id > > (ingenic,nand-oob-layout = ), but not a generic way to > > define custom layouts for all kind of NAND controller. > > Okay. > > >> > >> My motivation is to get rid of this (move it to devicetree): > >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> And enable the support of other boards with custom OOB layouts. > > > > Can you list the different layouts you have? I'm pretty sure there's a > > pattern. Maybe we can even deduce the layout from the page size or OOB > > size. > > This is the other layout I have for another ingenic device: > http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > > Page size and OOB size are the same between these two devices. Indeed. Do you know if there are other kind of layouts in the wild? Note that can be a string, so if each each board is defining its own layout, you could specify the board name here. Otherwise, if you just have those 2 patterns, you can just name them "contiguous" and "interleaved".
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Le sam. 12 mai 2018 à 10:42, Boris Brezillona écrit : On Sat, 12 May 2018 08:55:40 -0300 Paul Cercueil wrote: Hi Boris, Le 12 mai 2018 02:55, Boris Brezillon a écrit : > > Hi Paul, > > On Fri, 11 May 2018 23:29:12 +0200 > Paul Cercueil wrote: > > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > > now possible to specify from devicetree where the ECC data is located > > inside the OOB region. > > Why would we want to do that? I mean, ECC/free regions are ECC > controller dependent (and NAND chip dependent for the OOB size part), > so there's no reason to describe it in the DT. And more importantly, > people are likely to get it wrong. > > I'm curious, why do you need that? Good question. The reason is that some SoCs have no ECC controller. The various boards for these SoCs then all use a different layout. Okay. Still think defining the layouts in the DT is a bad idea. We can add a jz4740 specific property to define the layout id (ingenic,nand-oob-layout = ), but not a generic way to define custom layouts for all kind of NAND controller. Okay. My motivation is to get rid of this (move it to devicetree): https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 And enable the support of other boards with custom OOB layouts. Can you list the different layouts you have? I'm pretty sure there's a pattern. Maybe we can even deduce the layout from the page size or OOB size. This is the other layout I have for another ingenic device: http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 Page size and OOB size are the same between these two devices. -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Le sam. 12 mai 2018 à 10:42, Boris Brezillon a écrit : On Sat, 12 May 2018 08:55:40 -0300 Paul Cercueil wrote: Hi Boris, Le 12 mai 2018 02:55, Boris Brezillon a écrit : > > Hi Paul, > > On Fri, 11 May 2018 23:29:12 +0200 > Paul Cercueil wrote: > > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > > now possible to specify from devicetree where the ECC data is located > > inside the OOB region. > > Why would we want to do that? I mean, ECC/free regions are ECC > controller dependent (and NAND chip dependent for the OOB size part), > so there's no reason to describe it in the DT. And more importantly, > people are likely to get it wrong. > > I'm curious, why do you need that? Good question. The reason is that some SoCs have no ECC controller. The various boards for these SoCs then all use a different layout. Okay. Still think defining the layouts in the DT is a bad idea. We can add a jz4740 specific property to define the layout id (ingenic,nand-oob-layout = ), but not a generic way to define custom layouts for all kind of NAND controller. Okay. My motivation is to get rid of this (move it to devicetree): https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 And enable the support of other boards with custom OOB layouts. Can you list the different layouts you have? I'm pretty sure there's a pattern. Maybe we can even deduce the layout from the page size or OOB size. This is the other layout I have for another ingenic device: http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 Page size and OOB size are the same between these two devices. -Paul
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 08:55:40 -0300 Paul Cercueilwrote: > Hi Boris, > > Le 12 mai 2018 02:55, Boris Brezillon a écrit : > > > > Hi Paul, > > > > On Fri, 11 May 2018 23:29:12 +0200 > > Paul Cercueil wrote: > > > > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > > > now possible to specify from devicetree where the ECC data is located > > > inside the OOB region. > > > > Why would we want to do that? I mean, ECC/free regions are ECC > > controller dependent (and NAND chip dependent for the OOB size part), > > so there's no reason to describe it in the DT. And more importantly, > > people are likely to get it wrong. > > > > I'm curious, why do you need that? > > Good question. > > The reason is that some SoCs have no ECC controller. > The various boards for these SoCs then all use a different layout. Okay. Still think defining the layouts in the DT is a bad idea. We can add a jz4740 specific property to define the layout id (ingenic,nand-oob-layout = ), but not a generic way to define custom layouts for all kind of NAND controller. > > My motivation is to get rid of this (move it to devicetree): > https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > And enable the support of other boards with custom OOB layouts. Can you list the different layouts you have? I'm pretty sure there's a pattern. Maybe we can even deduce the layout from the page size or OOB size.
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 08:55:40 -0300 Paul Cercueil wrote: > Hi Boris, > > Le 12 mai 2018 02:55, Boris Brezillon a écrit : > > > > Hi Paul, > > > > On Fri, 11 May 2018 23:29:12 +0200 > > Paul Cercueil wrote: > > > > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > > > now possible to specify from devicetree where the ECC data is located > > > inside the OOB region. > > > > Why would we want to do that? I mean, ECC/free regions are ECC > > controller dependent (and NAND chip dependent for the OOB size part), > > so there's no reason to describe it in the DT. And more importantly, > > people are likely to get it wrong. > > > > I'm curious, why do you need that? > > Good question. > > The reason is that some SoCs have no ECC controller. > The various boards for these SoCs then all use a different layout. Okay. Still think defining the layouts in the DT is a bad idea. We can add a jz4740 specific property to define the layout id (ingenic,nand-oob-layout = ), but not a generic way to define custom layouts for all kind of NAND controller. > > My motivation is to get rid of this (move it to devicetree): > https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > And enable the support of other boards with custom OOB layouts. Can you list the different layouts you have? I'm pretty sure there's a pattern. Maybe we can even deduce the layout from the page size or OOB size.
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Hi Paul, On Fri, 11 May 2018 23:29:12 +0200 Paul Cercueilwrote: > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > now possible to specify from devicetree where the ECC data is located > inside the OOB region. Why would we want to do that? I mean, ECC/free regions are ECC controller dependent (and NAND chip dependent for the OOB size part), so there's no reason to describe it in the DT. And more importantly, people are likely to get it wrong. I'm curious, why do you need that? Regards, Boris > > Signed-off-by: Paul Cercueil > --- > Documentation/devicetree/bindings/mtd/nand.txt | 7 + > drivers/mtd/nand/raw/nand_base.c | 42 > ++ > 2 files changed, 49 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/nand.txt > b/Documentation/devicetree/bindings/mtd/nand.txt > index 8bb11d809429..118ea92787cb 100644 > --- a/Documentation/devicetree/bindings/mtd/nand.txt > +++ b/Documentation/devicetree/bindings/mtd/nand.txt > @@ -45,6 +45,13 @@ Optional NAND chip properties: >as reliable as possible. > - nand-rb: shall contain the native Ready/Busy ids. > > +- nand-oob-ecc: couples of integers, specifying the offset > + and length of the ECC data in the OOB region. There can be > more > + than one couple. > +- nand-oob-free: couples of integers, specifying the offset > + and length of a free-to-use area in the OOB region. There > can be > + more than one couple. > + > The ECC strength and ECC step size properties define the correction > capability > of a controller. Together, they say a controller can correct "{strength} bit > errors per {size} bytes". > diff --git a/drivers/mtd/nand/raw/nand_base.c > b/drivers/mtd/nand/raw/nand_base.c > index 72f3a89da513..c905531effb0 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -213,6 +213,43 @@ static const struct mtd_ooblayout_ops > nand_ooblayout_lp_hamming_ops = { > .free = nand_ooblayout_free_lp_hamming, > }; > > +static int nand_oob_of(struct device_node *np, int section, > +struct mtd_oob_region *oobregion, const char *prop) > +{ > + int ret = of_property_read_u32_index(np, prop, > + section * 2, >offset); > + if (ret == -EOVERFLOW) > + return -ERANGE; /* We're done */ > + if (ret) > + return ret; > + > + ret = of_property_read_u32_index(np, prop, > + section * 2 + 1, >length); > + if (ret == -EOVERFLOW) > + return -EINVAL; /* We must have an even number of integers */ > + > + return ret; > +} > + > +static int nand_ooblayout_ecc_of(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + return nand_oob_of(mtd->dev.of_node, section, > + oobregion, "nand-oob-ecc"); > +} > + > +static int nand_ooblayout_free_of(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + return nand_oob_of(mtd->dev.of_node, section, > + oobregion, "nand-oob-free"); > +} > + > +static const struct mtd_ooblayout_ops nand_ooblayout_of_ops = { > + .ecc = nand_ooblayout_ecc_of, > + .free = nand_ooblayout_free_of, > +}; > + > static int check_offs_len(struct mtd_info *mtd, > loff_t ofs, uint64_t len) > { > @@ -5843,6 +5880,11 @@ static int nand_dt_init(struct nand_chip *chip) > if (of_property_read_bool(dn, "nand-ecc-maximize")) > chip->ecc.options |= NAND_ECC_MAXIMIZE; > > + if (!chip->mtd.ooblayout && > + of_property_read_bool(dn, "nand-oob-ecc") && > + of_property_read_bool(dn, "nand-oob-free")) > + chip->mtd.ooblayout = _ooblayout_of_ops; > + > return 0; > } >
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Hi Paul, On Fri, 11 May 2018 23:29:12 +0200 Paul Cercueil wrote: > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > now possible to specify from devicetree where the ECC data is located > inside the OOB region. Why would we want to do that? I mean, ECC/free regions are ECC controller dependent (and NAND chip dependent for the OOB size part), so there's no reason to describe it in the DT. And more importantly, people are likely to get it wrong. I'm curious, why do you need that? Regards, Boris > > Signed-off-by: Paul Cercueil > --- > Documentation/devicetree/bindings/mtd/nand.txt | 7 + > drivers/mtd/nand/raw/nand_base.c | 42 > ++ > 2 files changed, 49 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/nand.txt > b/Documentation/devicetree/bindings/mtd/nand.txt > index 8bb11d809429..118ea92787cb 100644 > --- a/Documentation/devicetree/bindings/mtd/nand.txt > +++ b/Documentation/devicetree/bindings/mtd/nand.txt > @@ -45,6 +45,13 @@ Optional NAND chip properties: >as reliable as possible. > - nand-rb: shall contain the native Ready/Busy ids. > > +- nand-oob-ecc: couples of integers, specifying the offset > + and length of the ECC data in the OOB region. There can be > more > + than one couple. > +- nand-oob-free: couples of integers, specifying the offset > + and length of a free-to-use area in the OOB region. There > can be > + more than one couple. > + > The ECC strength and ECC step size properties define the correction > capability > of a controller. Together, they say a controller can correct "{strength} bit > errors per {size} bytes". > diff --git a/drivers/mtd/nand/raw/nand_base.c > b/drivers/mtd/nand/raw/nand_base.c > index 72f3a89da513..c905531effb0 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -213,6 +213,43 @@ static const struct mtd_ooblayout_ops > nand_ooblayout_lp_hamming_ops = { > .free = nand_ooblayout_free_lp_hamming, > }; > > +static int nand_oob_of(struct device_node *np, int section, > +struct mtd_oob_region *oobregion, const char *prop) > +{ > + int ret = of_property_read_u32_index(np, prop, > + section * 2, >offset); > + if (ret == -EOVERFLOW) > + return -ERANGE; /* We're done */ > + if (ret) > + return ret; > + > + ret = of_property_read_u32_index(np, prop, > + section * 2 + 1, >length); > + if (ret == -EOVERFLOW) > + return -EINVAL; /* We must have an even number of integers */ > + > + return ret; > +} > + > +static int nand_ooblayout_ecc_of(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + return nand_oob_of(mtd->dev.of_node, section, > + oobregion, "nand-oob-ecc"); > +} > + > +static int nand_ooblayout_free_of(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + return nand_oob_of(mtd->dev.of_node, section, > + oobregion, "nand-oob-free"); > +} > + > +static const struct mtd_ooblayout_ops nand_ooblayout_of_ops = { > + .ecc = nand_ooblayout_ecc_of, > + .free = nand_ooblayout_free_of, > +}; > + > static int check_offs_len(struct mtd_info *mtd, > loff_t ofs, uint64_t len) > { > @@ -5843,6 +5880,11 @@ static int nand_dt_init(struct nand_chip *chip) > if (of_property_read_bool(dn, "nand-ecc-maximize")) > chip->ecc.options |= NAND_ECC_MAXIMIZE; > > + if (!chip->mtd.ooblayout && > + of_property_read_bool(dn, "nand-oob-ecc") && > + of_property_read_bool(dn, "nand-oob-free")) > + chip->mtd.ooblayout = _ooblayout_of_ops; > + > return 0; > } >
[PATCH] mtd: nand: Add support for reading ooblayout from device tree
By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is now possible to specify from devicetree where the ECC data is located inside the OOB region. Signed-off-by: Paul Cercueil--- Documentation/devicetree/bindings/mtd/nand.txt | 7 + drivers/mtd/nand/raw/nand_base.c | 42 ++ 2 files changed, 49 insertions(+) diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt index 8bb11d809429..118ea92787cb 100644 --- a/Documentation/devicetree/bindings/mtd/nand.txt +++ b/Documentation/devicetree/bindings/mtd/nand.txt @@ -45,6 +45,13 @@ Optional NAND chip properties: as reliable as possible. - nand-rb: shall contain the native Ready/Busy ids. +- nand-oob-ecc: couples of integers, specifying the offset +and length of the ECC data in the OOB region. There can be more +than one couple. +- nand-oob-free: couples of integers, specifying the offset +and length of a free-to-use area in the OOB region. There can be +more than one couple. + The ECC strength and ECC step size properties define the correction capability of a controller. Together, they say a controller can correct "{strength} bit errors per {size} bytes". diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 72f3a89da513..c905531effb0 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -213,6 +213,43 @@ static const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = { .free = nand_ooblayout_free_lp_hamming, }; +static int nand_oob_of(struct device_node *np, int section, + struct mtd_oob_region *oobregion, const char *prop) +{ + int ret = of_property_read_u32_index(np, prop, + section * 2, >offset); + if (ret == -EOVERFLOW) + return -ERANGE; /* We're done */ + if (ret) + return ret; + + ret = of_property_read_u32_index(np, prop, + section * 2 + 1, >length); + if (ret == -EOVERFLOW) + return -EINVAL; /* We must have an even number of integers */ + + return ret; +} + +static int nand_ooblayout_ecc_of(struct mtd_info *mtd, int section, +struct mtd_oob_region *oobregion) +{ + return nand_oob_of(mtd->dev.of_node, section, + oobregion, "nand-oob-ecc"); +} + +static int nand_ooblayout_free_of(struct mtd_info *mtd, int section, +struct mtd_oob_region *oobregion) +{ + return nand_oob_of(mtd->dev.of_node, section, + oobregion, "nand-oob-free"); +} + +static const struct mtd_ooblayout_ops nand_ooblayout_of_ops = { + .ecc = nand_ooblayout_ecc_of, + .free = nand_ooblayout_free_of, +}; + static int check_offs_len(struct mtd_info *mtd, loff_t ofs, uint64_t len) { @@ -5843,6 +5880,11 @@ static int nand_dt_init(struct nand_chip *chip) if (of_property_read_bool(dn, "nand-ecc-maximize")) chip->ecc.options |= NAND_ECC_MAXIMIZE; + if (!chip->mtd.ooblayout && + of_property_read_bool(dn, "nand-oob-ecc") && + of_property_read_bool(dn, "nand-oob-free")) + chip->mtd.ooblayout = _ooblayout_of_ops; + return 0; } -- 2.11.0
[PATCH] mtd: nand: Add support for reading ooblayout from device tree
By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is now possible to specify from devicetree where the ECC data is located inside the OOB region. Signed-off-by: Paul Cercueil --- Documentation/devicetree/bindings/mtd/nand.txt | 7 + drivers/mtd/nand/raw/nand_base.c | 42 ++ 2 files changed, 49 insertions(+) diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt index 8bb11d809429..118ea92787cb 100644 --- a/Documentation/devicetree/bindings/mtd/nand.txt +++ b/Documentation/devicetree/bindings/mtd/nand.txt @@ -45,6 +45,13 @@ Optional NAND chip properties: as reliable as possible. - nand-rb: shall contain the native Ready/Busy ids. +- nand-oob-ecc: couples of integers, specifying the offset +and length of the ECC data in the OOB region. There can be more +than one couple. +- nand-oob-free: couples of integers, specifying the offset +and length of a free-to-use area in the OOB region. There can be +more than one couple. + The ECC strength and ECC step size properties define the correction capability of a controller. Together, they say a controller can correct "{strength} bit errors per {size} bytes". diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 72f3a89da513..c905531effb0 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -213,6 +213,43 @@ static const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = { .free = nand_ooblayout_free_lp_hamming, }; +static int nand_oob_of(struct device_node *np, int section, + struct mtd_oob_region *oobregion, const char *prop) +{ + int ret = of_property_read_u32_index(np, prop, + section * 2, >offset); + if (ret == -EOVERFLOW) + return -ERANGE; /* We're done */ + if (ret) + return ret; + + ret = of_property_read_u32_index(np, prop, + section * 2 + 1, >length); + if (ret == -EOVERFLOW) + return -EINVAL; /* We must have an even number of integers */ + + return ret; +} + +static int nand_ooblayout_ecc_of(struct mtd_info *mtd, int section, +struct mtd_oob_region *oobregion) +{ + return nand_oob_of(mtd->dev.of_node, section, + oobregion, "nand-oob-ecc"); +} + +static int nand_ooblayout_free_of(struct mtd_info *mtd, int section, +struct mtd_oob_region *oobregion) +{ + return nand_oob_of(mtd->dev.of_node, section, + oobregion, "nand-oob-free"); +} + +static const struct mtd_ooblayout_ops nand_ooblayout_of_ops = { + .ecc = nand_ooblayout_ecc_of, + .free = nand_ooblayout_free_of, +}; + static int check_offs_len(struct mtd_info *mtd, loff_t ofs, uint64_t len) { @@ -5843,6 +5880,11 @@ static int nand_dt_init(struct nand_chip *chip) if (of_property_read_bool(dn, "nand-ecc-maximize")) chip->ecc.options |= NAND_ECC_MAXIMIZE; + if (!chip->mtd.ooblayout && + of_property_read_bool(dn, "nand-oob-ecc") && + of_property_read_bool(dn, "nand-oob-free")) + chip->mtd.ooblayout = _ooblayout_of_ops; + return 0; } -- 2.11.0