Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1213316 > unrolled thread

Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings

Started byBrian Norris <computersforpeace@gmail.com>
First post2015-08-25 22:30 +0200
Last post2015-08-26 17:40 +0200
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings Brian Norris <computersforpeace@gmail.com> - 2015-08-25 22:30 +0200
    Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-08-26 17:40 +0200
      Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings Stefan Agner <stefan@agner.ch> - 2015-08-26 23:20 +0200
        Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings Brian Norris <computersforpeace@gmail.com> - 2015-08-26 23:30 +0200
    Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings Bill Pringlemeir <bpringle@sympatico.ca> - 2015-08-26 17:40 +0200

#1213316 — Re: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings

FromBrian Norris <computersforpeace@gmail.com>
Date2015-08-25 22:30 +0200
SubjectRe: [PATCH v10 3/5] mtd: nand: vf610_nfc: add device tree bindings
Message-ID<q1t10-vU-27@gated-at.bofh.it>
Sorry, I realized a potential issue here.

On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
> Acked-by: Shawn Guo <shawnguo@kernel.org>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  .../devicetree/bindings/mtd/vf610-nfc.txt          | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> new file mode 100644
> index 0000000..cae5f25
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> @@ -0,0 +1,45 @@
> +Freescale's NAND flash controller (NFC)
> +
> +This variant of the Freescale NAND flash controller (NFC) can be found on
> +Vybrid (vf610), MPC5125, MCF54418 and Kinetis K70.
> +
> +Required properties:
> +- compatible: Should be set to "fsl,vf610-nfc"
> +- reg: address range of the NFC
> +- interrupts: interrupt of the NFC
> +- nand-bus-width: see nand.txt
> +- nand-ecc-mode: see nand.txt
> +- nand-on-flash-bbt: see nand.txt

Stumbling across the "multi-CS" questions on the driver reminds me: it
typically makes sense to define new NAND bindings using separate NAND
*controller* and *flash* device nodes. The above 3 properties, at least,
would apply on a per-flash basis, not per-controller typically. See
sunxi-nand, for instance:

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt

brcmnand had a similar pattern:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt

(Perhaps it's time we standardized this a little more formally...)

> +- assigned-clocks: main clock from the SoC, for Vybrid <&clks VF610_CLK_NFC>;
> +- assigned-clock-rates: The NAND bus timing is derived from this clock
> +    rate and should not exceed maximum timing for any NAND memory chip
> +    in a board stuffing. Typical NAND memory timings derived from this
> +    clock are found in the SoC hardware reference manual. Furthermore,
> +    there might be restrictions on maximum rates when using hardware ECC.
> +
> +- #address-cells, #size-cells : Must be present if the device has sub-nodes
> +  representing partitions.
> +
> +Required properties for hardware ECC:
> +- nand-ecc-strength: supported strengths are 24 and 32 bit (see nand.txt)
> +- nand-ecc-step-size: step size equals page size, currently only 2k pages are
> +    supported
> +
> +Example:
> +
> +	nfc: nand@400e0000 {
> +		compatible = "fsl,vf610-nfc";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x400e0000 0x4000>;
> +		interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clks VF610_CLK_NFC>;
> +		clock-names = "nfc";
> +		assigned-clocks = <&clks VF610_CLK_NFC>;
> +		assigned-clock-rates = <33000000>;
> +		nand-bus-width = <8>;
> +		nand-ecc-mode = "hw";
> +		nand-ecc-strength = <32>;
> +		nand-ecc-step-size = <2048>;
> +		nand-on-flash-bbt;
> +	};

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1213963

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2015-08-26 17:40 +0200
Message-ID<q1KXT-1pg-5@gated-at.bofh.it>
In reply to#1213316
Hi Bill,

On Wed, 26 Aug 2015 11:26:36 -0400
Bill Pringlemeir <bpringle@sympatico.ca> wrote:

> On 25 Aug 2015, computersforpeace@gmail.com wrote:
> 
> > Sorry, I realized a potential issue here.
> 
> > On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
> >> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
> >> Acked-by: Shawn Guo <shawnguo@kernel.org>
> >> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
> >> 1 file changed, 45 insertions(+) create mode 100644
> >> Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> 
> >> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >> new file mode 100644
> >> index 0000000..cae5f25
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >>>> -0,0 +1,45 @@
> >> +- nand-bus-width: see nand.txt
> >> +- nand-ecc-mode: see nand.txt
> >> +- nand-on-flash-bbt: see nand.txt
> 
> > Stumbling across the "multi-CS" questions on the driver reminds me: it
> > typically makes sense to define new NAND bindings using separate NAND
> > *controller* and *flash* device nodes. The above 3 properties, at
> > least, would apply on a per-flash basis, not per-controller
> > typically. See sunxi-nand, for instance:
> 
> > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
> 
> > brcmnand had a similar pattern:
> 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> 
> > (Perhaps it's time we standardized this a little more formally...)
> 
> These would apply per chip, but the controller has to be configured to
> support each and every one.  Every time an operation was performed, we
> would have to check the chip type and reconfigure the controller.
> Currently, the driver does not support this and it would add a lot of
> overhead in some cases unless a register cache was used.
> 
> Is the flexibility of using a system with combined 8/16bit devices
> really worth all the overhead?  Isn't it sort of brain dead hardware not
> to make all of the chips similar?  Why would everyone have to pay for
> such a crazy setup?
> 
> To separate it would at least be a lie versus the code in the current
> form.  As well, there are only a few SOC which support multiple chip
> selects.  The 'multi-CS' register bits of this controller varies between
> PowerPC, 68K/Coldfire and ARM platforms.
> 
> I looked briefly at the brcmnand.c  and it seems that it is not
> supporting different ECC per chip even though the nodes are broken out
> this way.  In fact, if some raw functions are called, I think it will
> put it in ECC mode even if it wasn't before?  Well, I agree that this
> would be good generically, I think it puts a lot of effort in the
> drivers for not so much payoff?

Hm, the sunxi driver supports it, and it does not add such a big
overhead...
The only thing you have to do is cache a bunch of register values
per-chip and restore/apply them when the chip is selected
(in your ->select_chip() implementation).

Anyway, even if the suggested DT representation is a lie in regards to
your implementation, it's actually pretty accurate from an hardware
POV, and this is exactly what DT is supposed to represent.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214198

FromStefan Agner <stefan@agner.ch>
Date2015-08-26 23:20 +0200
Message-ID<q1QgX-GC-23@gated-at.bofh.it>
In reply to#1213963
On 2015-08-26 08:39, Boris Brezillon wrote:
> Hi Bill,
> 
> On Wed, 26 Aug 2015 11:26:36 -0400
> Bill Pringlemeir <bpringle@sympatico.ca> wrote:
> 
>> On 25 Aug 2015, computersforpeace@gmail.com wrote:
>>
>> > Sorry, I realized a potential issue here.
>>
>> > On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
>> >> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
>> >> Acked-by: Shawn Guo <shawnguo@kernel.org>
>> >> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
>> >> 1 file changed, 45 insertions(+) create mode 100644
>> >> Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>>
>> >> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >> new file mode 100644
>> >> index 0000000..cae5f25
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >>>> -0,0 +1,45 @@
>> >> +- nand-bus-width: see nand.txt
>> >> +- nand-ecc-mode: see nand.txt
>> >> +- nand-on-flash-bbt: see nand.txt
>>
>> > Stumbling across the "multi-CS" questions on the driver reminds me: it
>> > typically makes sense to define new NAND bindings using separate NAND
>> > *controller* and *flash* device nodes. The above 3 properties, at
>> > least, would apply on a per-flash basis, not per-controller
>> > typically. See sunxi-nand, for instance:
>>
>> > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
>>
>> > brcmnand had a similar pattern:
>>
>> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>>
>> > (Perhaps it's time we standardized this a little more formally...)
>>
>> These would apply per chip, but the controller has to be configured to
>> support each and every one.  Every time an operation was performed, we
>> would have to check the chip type and reconfigure the controller.
>> Currently, the driver does not support this and it would add a lot of
>> overhead in some cases unless a register cache was used.
>>
>> Is the flexibility of using a system with combined 8/16bit devices
>> really worth all the overhead?  Isn't it sort of brain dead hardware not
>> to make all of the chips similar?  Why would everyone have to pay for
>> such a crazy setup?
>>
>> To separate it would at least be a lie versus the code in the current
>> form.  As well, there are only a few SOC which support multiple chip
>> selects.  The 'multi-CS' register bits of this controller varies between
>> PowerPC, 68K/Coldfire and ARM platforms.

The DT can be a lie versus the code. The DT should reflect how the
hardware is wired, afaik, if we take shortcuts in the driver code, that
is fine. If we don't support a certain configuration right now (e.g.
second NAND chip), the driver can just return an appropriate error code.
>>
>> I looked briefly at the brcmnand.c  and it seems that it is not
>> supporting different ECC per chip even though the nodes are broken out
>> this way.  In fact, if some raw functions are called, I think it will
>> put it in ECC mode even if it wasn't before?  Well, I agree that this
>> would be good generically, I think it puts a lot of effort in the
>> drivers for not so much payoff?
> 
> Hm, the sunxi driver supports it, and it does not add such a big
> overhead...
> The only thing you have to do is cache a bunch of register values
> per-chip and restore/apply them when the chip is selected
> (in your ->select_chip() implementation).
> 
> Anyway, even if the suggested DT representation is a lie in regards to
> your implementation, it's actually pretty accurate from an hardware
> POV, and this is exactly what DT is supposed to represent.

I agree with both of you. I don't see much value implementing multi-NAND
chip support, especially with different configurations, at the moment. I
am not aware of any hardware making use of that now.

I will update the driver to parse a NAND sub node and get the ECC
properties from the per flash configuration. However, I won't add chip
select or multi-NAND support right now...

Any objection?

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214201

FromBrian Norris <computersforpeace@gmail.com>
Date2015-08-26 23:30 +0200
Message-ID<q1QqB-RP-3@gated-at.bofh.it>
In reply to#1214198
On Wed, Aug 26, 2015 at 02:15:45PM -0700, Stefan Agner wrote:
> On 2015-08-26 08:39, Boris Brezillon wrote:
> > On Wed, 26 Aug 2015 11:26:36 -0400
> > Bill Pringlemeir <bpringle@sympatico.ca> wrote:
> >> These would apply per chip, but the controller has to be configured to
> >> support each and every one.  Every time an operation was performed, we
> >> would have to check the chip type and reconfigure the controller.
> >> Currently, the driver does not support this and it would add a lot of
> >> overhead in some cases unless a register cache was used.
> >>
> >> Is the flexibility of using a system with combined 8/16bit devices
> >> really worth all the overhead?  Isn't it sort of brain dead hardware not
> >> to make all of the chips similar?  Why would everyone have to pay for
> >> such a crazy setup?
> >>
> >> To separate it would at least be a lie versus the code in the current
> >> form.  As well, there are only a few SOC which support multiple chip
> >> selects.  The 'multi-CS' register bits of this controller varies between
> >> PowerPC, 68K/Coldfire and ARM platforms.
> 
> The DT can be a lie versus the code. The DT should reflect how the
> hardware is wired, afaik, if we take shortcuts in the driver code, that
> is fine. If we don't support a certain configuration right now (e.g.
> second NAND chip), the driver can just return an appropriate error code.

Right, I was only asking for:
(1) a more accurate DT and
(2) clarity in the driver; the clarity might just be "we don't support
    multi-CS"

> >> I looked briefly at the brcmnand.c  and it seems that it is not
> >> supporting different ECC per chip even though the nodes are broken out
> >> this way.  In fact, if some raw functions are called, I think it will
> >> put it in ECC mode even if it wasn't before?  Well, I agree that this
> >> would be good generically, I think it puts a lot of effort in the
> >> drivers for not so much payoff?
> > 
> > Hm, the sunxi driver supports it, and it does not add such a big
> > overhead...
> > The only thing you have to do is cache a bunch of register values
> > per-chip and restore/apply them when the chip is selected
> > (in your ->select_chip() implementation).
> > 
> > Anyway, even if the suggested DT representation is a lie in regards to
> > your implementation, it's actually pretty accurate from an hardware
> > POV, and this is exactly what DT is supposed to represent.
> 
> I agree with both of you. I don't see much value implementing multi-NAND
> chip support, especially with different configurations, at the moment. I
> am not aware of any hardware making use of that now.
> 
> I will update the driver to parse a NAND sub node and get the ECC
> properties from the per flash configuration. However, I won't add chip
> select or multi-NAND support right now...
> 
> Any objection?

Nope, sounds good to me.

A few tips:
 * be defensive; i.e., error out if someone specifies 2 flash in the DT
 * use the 'reg' property to be the addressing index in the flash
   sub-node; i.e., the chip-select. This fits the practice done by most
   others, I think.

Regards,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213967

FromBill Pringlemeir <bpringle@sympatico.ca>
Date2015-08-26 17:40 +0200
Message-ID<q1KXT-1pg-7@gated-at.bofh.it>
In reply to#1213316
On 25 Aug 2015, computersforpeace@gmail.com wrote:

> Sorry, I realized a potential issue here.

> On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
>> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
>> Acked-by: Shawn Guo <shawnguo@kernel.org>
>> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
>> 1 file changed, 45 insertions(+) create mode 100644
>> Documentation/devicetree/bindings/mtd/vf610-nfc.txt

>> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> new file mode 100644
>> index 0000000..cae5f25
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>>>> -0,0 +1,45 @@
>> +- nand-bus-width: see nand.txt
>> +- nand-ecc-mode: see nand.txt
>> +- nand-on-flash-bbt: see nand.txt

> Stumbling across the "multi-CS" questions on the driver reminds me: it
> typically makes sense to define new NAND bindings using separate NAND
> *controller* and *flash* device nodes. The above 3 properties, at
> least, would apply on a per-flash basis, not per-controller
> typically. See sunxi-nand, for instance:

> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt

> brcmnand had a similar pattern:

> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt

> (Perhaps it's time we standardized this a little more formally...)

These would apply per chip, but the controller has to be configured to
support each and every one.  Every time an operation was performed, we
would have to check the chip type and reconfigure the controller.
Currently, the driver does not support this and it would add a lot of
overhead in some cases unless a register cache was used.

Is the flexibility of using a system with combined 8/16bit devices
really worth all the overhead?  Isn't it sort of brain dead hardware not
to make all of the chips similar?  Why would everyone have to pay for
such a crazy setup?

To separate it would at least be a lie versus the code in the current
form.  As well, there are only a few SOC which support multiple chip
selects.  The 'multi-CS' register bits of this controller varies between
PowerPC, 68K/Coldfire and ARM platforms.

I looked briefly at the brcmnand.c  and it seems that it is not
supporting different ECC per chip even though the nodes are broken out
this way.  In fact, if some raw functions are called, I think it will
put it in ECC mode even if it wasn't before?  Well, I agree that this
would be good generically, I think it puts a lot of effort in the
drivers for not so much payoff?

Fwiw,
Bill Pringlemeir.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web