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

Groups > linux.kernel > #1674918 > unrolled thread

[PATCH 0/3] Altera Quadspi Controller Version 2

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Altera Quadspi Controller Version 2 matthew.gerlach@linux.intel.com - 2017-06-26 18:20 +0200
    [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2 matthew.gerlach@linux.intel.com - 2017-06-26 18:20 +0200
      Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 12:30 +0200
        Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 matthew.gerlach@linux.intel.com - 2017-06-27 16:40 +0200
          Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 17:20 +0200
            Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 matthew.gerlach@linux.intel.com - 2017-06-27 18:00 +0200
              Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 18:30 +0200
                Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 matthew.gerlach@linux.intel.com - 2017-06-27 19:20 +0200
                  Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 20:00 +0200
                    Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 matthew.gerlach@linux.intel.com - 2017-06-27 21:40 +0200
                      Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 22:00 +0200
              Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Rob Herring <robh@kernel.org> - 2017-06-29 01:10 +0200
                Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-29 11:50 +0200
                  Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 matthew.gerlach@linux.intel.com - 2017-06-29 17:10 +0200
                    Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Marek Vasut <marek.vasut@gmail.com> - 2017-06-29 17:40 +0200
      Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller  Version 2 Rob Herring <robh@kernel.org> - 2017-06-29 01:20 +0200
    [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver matthew.gerlach@linux.intel.com - 2017-06-26 18:20 +0200
      Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2  Platform driver Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 12:30 +0200
        Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2  Platform driver matthew.gerlach@linux.intel.com - 2017-06-27 17:40 +0200
          Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2  Platform driver Marek Vasut <marek.vasut@gmail.com> - 2017-06-27 18:30 +0200
            Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2  Platform driver matthew.gerlach@linux.intel.com - 2017-06-27 19:40 +0200
      Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2  Platform driver kbuild test robot <lkp@intel.com> - 2017-06-27 13:00 +0200

Page 1 of 2  [1] 2  Next page →

#1674918 — [PATCH 0/3] Altera Quadspi Controller Version 2

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Hi Everyone,

This patch set adds support for Version 2 of the Altera
Quadspi Controller.  Version 1 of the Altera Quadspi
Controller only supported 1 byte of flash id instead of
the 3 bytes required for a JEDEC ID.  As such the driver
for Version 1 was rejected.  Please see 
https://patchwork.kernel.org/patch/7025681

Patch 1: add device tree bindings document for platform driver

Patch 2: add core code for driver

Patch 3: add platform driver

Matthew Gerlach (3):
  ARM: dts: Bindings for Altera Quadspi Controller Version 2
  mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2
  mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver

 .../devicetree/bindings/mtd/altera-quadspi-v2.txt  |  37 ++
 MAINTAINERS                                        |   8 +
 drivers/mtd/spi-nor/Kconfig                        |  10 +
 drivers/mtd/spi-nor/Makefile                       |   5 +-
 drivers/mtd/spi-nor/altera-quadspi-platform.c      | 137 +++++
 drivers/mtd/spi-nor/altera-quadspi.c               | 676 +++++++++++++++++++++
 include/linux/mtd/altera-quadspi.h                 |  28 +
 7 files changed, 900 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
 create mode 100644 drivers/mtd/spi-nor/altera-quadspi-platform.c
 create mode 100644 drivers/mtd/spi-nor/altera-quadspi.c
 create mode 100644 include/linux/mtd/altera-quadspi.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1674920 — [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Device Tree bindings for Version 2 of the Altera Quadspi Controller
that can be optionally paired with a windowed bridge.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt

diff --git a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
new file mode 100644
index 0000000..8ba63d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
@@ -0,0 +1,37 @@
+* Altera Quad SPI Controller Version 2
+
+Required properties:
+- compatible : Should be "altr,quadspi-v2".
+- reg : Contains at least two entries, and possibly three entries, each of
+	which is a tuple consisting of a physical address and length.
+- reg-names : Should contain the names "avl_csr" and "avl_mem" corresponding
+	      to the control and status registers and qspi memory, respectively.
+
+
+The Altera Quad SPI Controller Version 2 can be paired with a windowed bridge
+in order to reduce the footprint of the memory interface.  When a windowed
+bridge is used, reads and writes of data must be 32 bits wide.
+
+Optional properties:
+- reg-names : Should contain the name "avl_window", if the windowed bridge
+	      is used.  This name corresponds to the register space that
+	      controls the window.
+- window-size : The size of the window which must be an even power of 2.
+- read-bit-reverse : A boolean indicating the data read from the flash should
+		     be bit reversed on a byte by byte basis before being
+		     delivered to the MTD layer.
+- write-bit-reverse : A boolean indicating the data written to the flash should
+		      be bit reversed on a byte by byte basis.
+
+Example:
+
+qspi: spi@a0001000 {
+	compatible = "altr,quadspi-v2";
+	reg = <0xa0001000 0x40>, <0xb0000000 0x4000000>;
+	reg-names = "avl_csr", "avl_mem";
+
+	flash@0 {
+		reg = <0>;
+		label = "FPGA Image";
+	};
+};
-- 
2.7.4

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

#1675527 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Device Tree bindings for Version 2 of the Altera Quadspi Controller
> that can be optionally paired with a windowed bridge.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> new file mode 100644
> index 0000000..8ba63d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> @@ -0,0 +1,37 @@
> +* Altera Quad SPI Controller Version 2
> +
> +Required properties:
> +- compatible : Should be "altr,quadspi-v2".
> +- reg : Contains at least two entries, and possibly three entries, each of
> +	which is a tuple consisting of a physical address and length.
> +- reg-names : Should contain the names "avl_csr" and "avl_mem" corresponding
> +	      to the control and status registers and qspi memory, respectively.
> +
> +
> +The Altera Quad SPI Controller Version 2 can be paired with a windowed bridge
> +in order to reduce the footprint of the memory interface.  When a windowed
> +bridge is used, reads and writes of data must be 32 bits wide.
> +
> +Optional properties:
> +- reg-names : Should contain the name "avl_window", if the windowed bridge
> +	      is used.  This name corresponds to the register space that
> +	      controls the window.
> +- window-size : The size of the window which must be an even power of 2.
> +- read-bit-reverse : A boolean indicating the data read from the flash should
> +		     be bit reversed on a byte by byte basis before being
> +		     delivered to the MTD layer.
> +- write-bit-reverse : A boolean indicating the data written to the flash should
> +		      be bit reversed on a byte by byte basis.

Is there ever a usecase where you need to set just one of these props ?
Also, they're altera specific, so altr, prefix should be added.

> +Example:
> +
> +qspi: spi@a0001000 {
> +	compatible = "altr,quadspi-v2";
> +	reg = <0xa0001000 0x40>, <0xb0000000 0x4000000>;
> +	reg-names = "avl_csr", "avl_mem";
> +
> +	flash@0 {
> +		reg = <0>;
> +		label = "FPGA Image";
> +	};
> +};
> 


-- 
Best regards,
Marek Vasut

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

#1675839 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Tue, 27 Jun 2017, Marek Vasut wrote:

Hi Marek,

Thanks for the feedback.  See my comments below.

Matthew Gerlach

> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>> that can be optionally paired with a windowed bridge.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37 ++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>> new file mode 100644
>> index 0000000..8ba63d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>> @@ -0,0 +1,37 @@
>> +* Altera Quad SPI Controller Version 2
>> +
>> +Required properties:
>> +- compatible : Should be "altr,quadspi-v2".
>> +- reg : Contains at least two entries, and possibly three entries, each of
>> +	which is a tuple consisting of a physical address and length.
>> +- reg-names : Should contain the names "avl_csr" and "avl_mem" corresponding
>> +	      to the control and status registers and qspi memory, respectively.
>> +
>> +
>> +The Altera Quad SPI Controller Version 2 can be paired with a windowed bridge
>> +in order to reduce the footprint of the memory interface.  When a windowed
>> +bridge is used, reads and writes of data must be 32 bits wide.
>> +
>> +Optional properties:
>> +- reg-names : Should contain the name "avl_window", if the windowed bridge
>> +	      is used.  This name corresponds to the register space that
>> +	      controls the window.
>> +- window-size : The size of the window which must be an even power of 2.
>> +- read-bit-reverse : A boolean indicating the data read from the flash should
>> +		     be bit reversed on a byte by byte basis before being
>> +		     delivered to the MTD layer.
>> +- write-bit-reverse : A boolean indicating the data written to the flash should
>> +		      be bit reversed on a byte by byte basis.
>
> Is there ever a usecase where you need to set just one of these props ?
> Also, they're altera specific, so altr, prefix should be added.

In general, I think if bit reversal is required, it would be required in
both directions.  However, anything is possible when using FPGAs.  So
I thought separate booleans would be future proofing the bindings.
Thinking about this binding more, I wonder if the binding name(s)
should be (read|write)-bit8-reverse to indicate reversings the bits
in a byte as opposed to reversing the bits in a 32 bit word?

I don't think bit reversal is specific to Altera/Intel components. I see
a nand driver performing bit reversal, and I think I've recently seen 
other FPGA based drivers requiring bit reversal.

>
>> +Example:
>> +
>> +qspi: spi@a0001000 {
>> +	compatible = "altr,quadspi-v2";
>> +	reg = <0xa0001000 0x40>, <0xb0000000 0x4000000>;
>> +	reg-names = "avl_csr", "avl_mem";
>> +
>> +	flash@0 {
>> +		reg = <0>;
>> +		label = "FPGA Image";
>> +	};
>> +};
>>
>
>
> -- 
> Best regards,
> Marek Vasut
>

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

#1675882 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 27 Jun 2017, Marek Vasut wrote:
> 
> Hi Marek,
> 
> Thanks for the feedback.  See my comments below.
> 
> Matthew Gerlach
> 
>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>
>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>> that can be optionally paired with a windowed bridge.
>>>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> ---
>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>> ++++++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>> new file mode 100644
>>> index 0000000..8ba63d7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>> @@ -0,0 +1,37 @@
>>> +* Altera Quad SPI Controller Version 2
>>> +
>>> +Required properties:
>>> +- compatible : Should be "altr,quadspi-v2".
>>> +- reg : Contains at least two entries, and possibly three entries,
>>> each of
>>> +    which is a tuple consisting of a physical address and length.
>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>> corresponding
>>> +          to the control and status registers and qspi memory,
>>> respectively.
>>> +
>>> +
>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>> windowed bridge
>>> +in order to reduce the footprint of the memory interface.  When a
>>> windowed
>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>> +
>>> +Optional properties:
>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>> bridge
>>> +          is used.  This name corresponds to the register space that
>>> +          controls the window.
>>> +- window-size : The size of the window which must be an even power
>>> of 2.
>>> +- read-bit-reverse : A boolean indicating the data read from the
>>> flash should
>>> +             be bit reversed on a byte by byte basis before being
>>> +             delivered to the MTD layer.
>>> +- write-bit-reverse : A boolean indicating the data written to the
>>> flash should
>>> +              be bit reversed on a byte by byte basis.
>>
>> Is there ever a usecase where you need to set just one of these props ?
>> Also, they're altera specific, so altr, prefix should be added.
> 
> In general, I think if bit reversal is required, it would be required in
> both directions.  However, anything is possible when using FPGAs.  So
> I thought separate booleans would be future proofing the bindings.

Maybe we should drop this whole thing and add it when this is actually
required.

Are there any users of this in the wild currently ?

What is the purpose of doing this per-byte bit reverse instead of
storing th bits in the original order ?

> Thinking about this binding more, I wonder if the binding name(s)
> should be (read|write)-bit8-reverse to indicate reversings the bits
> in a byte as opposed to reversing the bits in a 32 bit word?
>
> I don't think bit reversal is specific to Altera/Intel components. I see
> a nand driver performing bit reversal, and I think I've recently seen
> other FPGA based drivers requiring bit reversal.

$ git grep bit.reverse Documentation/devicetree/ | wc -l

0

So we don't have such a generic binding . It's up to Rob (I guess) to
decide whether this is SoC specific and should've altr, prefix or not.
IMO it is.

-- 
Best regards,
Marek Vasut

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

#1675939 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>
>> Hi Marek,
>>
>> Thanks for the feedback.  See my comments below.
>>
>> Matthew Gerlach
>>
>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>> that can be optionally paired with a windowed bridge.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>> ++++++++++++++++++++++
>>>>  1 file changed, 37 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>> new file mode 100644
>>>> index 0000000..8ba63d7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>> @@ -0,0 +1,37 @@
>>>> +* Altera Quad SPI Controller Version 2
>>>> +
>>>> +Required properties:
>>>> +- compatible : Should be "altr,quadspi-v2".
>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>> each of
>>>> +    which is a tuple consisting of a physical address and length.
>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>> corresponding
>>>> +          to the control and status registers and qspi memory,
>>>> respectively.
>>>> +
>>>> +
>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>> windowed bridge
>>>> +in order to reduce the footprint of the memory interface.  When a
>>>> windowed
>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>> +
>>>> +Optional properties:
>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>> bridge
>>>> +          is used.  This name corresponds to the register space that
>>>> +          controls the window.
>>>> +- window-size : The size of the window which must be an even power
>>>> of 2.
>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>> flash should
>>>> +             be bit reversed on a byte by byte basis before being
>>>> +             delivered to the MTD layer.
>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>> flash should
>>>> +              be bit reversed on a byte by byte basis.
>>>
>>> Is there ever a usecase where you need to set just one of these props ?
>>> Also, they're altera specific, so altr, prefix should be added.
>>
>> In general, I think if bit reversal is required, it would be required in
>> both directions.  However, anything is possible when using FPGAs.  So
>> I thought separate booleans would be future proofing the bindings.
>
> Maybe we should drop this whole thing and add it when this is actually
> required.
>
> Are there any users of this in the wild currently ?
>
> What is the purpose of doing this per-byte bit reverse instead of
> storing th bits in the original order ?

Hi Marek,

Yes, there is hardware that has been in the wild for years that needs this 
bit reversal.  The specific use case is when a flash chip is connected to
a FPGA, and the contents of the flash is used to configure the 
FPGA on power up.  In this use case, there is no processor involved with 
configuring the FPGA.  I am most familiar with this feature/bug with 
Altera FPGAs, but I believe this issue exists with other programmable 
devices.

>
>> Thinking about this binding more, I wonder if the binding name(s)
>> should be (read|write)-bit8-reverse to indicate reversings the bits
>> in a byte as opposed to reversing the bits in a 32 bit word?
>>
>> I don't think bit reversal is specific to Altera/Intel components. I see
>> a nand driver performing bit reversal, and I think I've recently seen
>> other FPGA based drivers requiring bit reversal.
>
> $ git grep bit.reverse Documentation/devicetree/ | wc -l
> 0
>
> So we don't have such a generic binding . It's up to Rob (I guess) to
> decide whether this is SoC specific and should've altr, prefix or not.
> IMO it is.

I agree there is no generic binding at this time, and I look forward
to any input from Rob and anyone else on this issue.  I think it is worth 
pointing out that this really isn't an issue of an SoC, but rather it is an
issue of how data in the flash chip is accessed.  I think what makes this issue
"weird" is that we have different hardware accessing the data in the flash 
with a different perspective.  The FPGA looks at the data from one 
perspective on power up, and a processor trying to update the flash has a 
different perspective.

Thanks again,

Matthew Gerlach
>
> -- 
> Best regards,
> Marek Vasut
>

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

#1675975 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/27/2017 05:57 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 27 Jun 2017, Marek Vasut wrote:
> 
>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>
>>> Hi Marek,
>>>
>>> Thanks for the feedback.  See my comments below.
>>>
>>> Matthew Gerlach
>>>
>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>
>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>> that can be optionally paired with a windowed bridge.
>>>>>
>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>> ---
>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>> ++++++++++++++++++++++
>>>>>  1 file changed, 37 insertions(+)
>>>>>  create mode 100644
>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>> new file mode 100644
>>>>> index 0000000..8ba63d7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>> @@ -0,0 +1,37 @@
>>>>> +* Altera Quad SPI Controller Version 2
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>> each of
>>>>> +    which is a tuple consisting of a physical address and length.
>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>> corresponding
>>>>> +          to the control and status registers and qspi memory,
>>>>> respectively.
>>>>> +
>>>>> +
>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>> windowed bridge
>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>> windowed
>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>> +
>>>>> +Optional properties:
>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>> bridge
>>>>> +          is used.  This name corresponds to the register space that
>>>>> +          controls the window.
>>>>> +- window-size : The size of the window which must be an even power
>>>>> of 2.
>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>> flash should
>>>>> +             be bit reversed on a byte by byte basis before being
>>>>> +             delivered to the MTD layer.
>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>> flash should
>>>>> +              be bit reversed on a byte by byte basis.
>>>>
>>>> Is there ever a usecase where you need to set just one of these props ?
>>>> Also, they're altera specific, so altr, prefix should be added.
>>>
>>> In general, I think if bit reversal is required, it would be required in
>>> both directions.  However, anything is possible when using FPGAs.  So
>>> I thought separate booleans would be future proofing the bindings.
>>
>> Maybe we should drop this whole thing and add it when this is actually
>> required.
>>
>> Are there any users of this in the wild currently ?
>>
>> What is the purpose of doing this per-byte bit reverse instead of
>> storing th bits in the original order ?
> 
> Hi Marek,
> 
> Yes, there is hardware that has been in the wild for years that needs
> this bit reversal.  The specific use case is when a flash chip is
> connected to
> a FPGA, and the contents of the flash is used to configure the FPGA on
> power up.  In this use case, there is no processor involved with
> configuring the FPGA.  I am most familiar with this feature/bug with
> Altera FPGAs, but I believe this issue exists with other programmable
> devices.

So the EPCQ/EPCS flash stores the bitstream in reverse or something ?
What are you storing in that flash except for the bitstream, filesystem?
Feel free to go into details, I believe it'd be useful to know exactly
what the problem is you're trying to solve here.

>>> Thinking about this binding more, I wonder if the binding name(s)
>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>
>>> I don't think bit reversal is specific to Altera/Intel components. I see
>>> a nand driver performing bit reversal, and I think I've recently seen
>>> other FPGA based drivers requiring bit reversal.
>>
>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>> 0
>>
>> So we don't have such a generic binding . It's up to Rob (I guess) to
>> decide whether this is SoC specific and should've altr, prefix or not.
>> IMO it is.
> 
> I agree there is no generic binding at this time, and I look forward
> to any input from Rob and anyone else on this issue.  I think it is
> worth pointing out that this really isn't an issue of an SoC, but rather
> it is an
> issue of how data in the flash chip is accessed.I think what makes
> this issue
> "weird" is that we have different hardware accessing the data in the
> flash with a different perspective.  The FPGA looks at the data from one
> perspective on power up, and a processor trying to update the flash has
> a different perspective.

Another thing I'd ask here is, is that bit-reverse a hardware property
or is that some software configuration thing ?

-- 
Best regards,
Marek Vasut

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

#1676014 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/27/2017 05:57 PM, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>
>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>
>>>> Hi Marek,
>>>>
>>>> Thanks for the feedback.  See my comments below.
>>>>
>>>> Matthew Gerlach
>>>>
>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>
>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>
>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>> ++++++++++++++++++++++
>>>>>>  1 file changed, 37 insertions(+)
>>>>>>  create mode 100644
>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..8ba63d7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> @@ -0,0 +1,37 @@
>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>> each of
>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>> corresponding
>>>>>> +          to the control and status registers and qspi memory,
>>>>>> respectively.
>>>>>> +
>>>>>> +
>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>> windowed bridge
>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>> windowed
>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>>> bridge
>>>>>> +          is used.  This name corresponds to the register space that
>>>>>> +          controls the window.
>>>>>> +- window-size : The size of the window which must be an even power
>>>>>> of 2.
>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>> flash should
>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>> +             delivered to the MTD layer.
>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>> flash should
>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>
>>>>> Is there ever a usecase where you need to set just one of these props ?
>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>
>>>> In general, I think if bit reversal is required, it would be required in
>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>> I thought separate booleans would be future proofing the bindings.
>>>
>>> Maybe we should drop this whole thing and add it when this is actually
>>> required.
>>>
>>> Are there any users of this in the wild currently ?
>>>
>>> What is the purpose of doing this per-byte bit reverse instead of
>>> storing th bits in the original order ?
>>
>> Hi Marek,
>>
>> Yes, there is hardware that has been in the wild for years that needs
>> this bit reversal.  The specific use case is when a flash chip is
>> connected to
>> a FPGA, and the contents of the flash is used to configure the FPGA on
>> power up.  In this use case, there is no processor involved with
>> configuring the FPGA.  I am most familiar with this feature/bug with
>> Altera FPGAs, but I believe this issue exists with other programmable
>> devices.
>
> So the EPCQ/EPCS flash stores the bitstream in reverse or something ?
> What are you storing in that flash except for the bitstream, filesystem?
> Feel free to go into details, I believe it'd be useful to know exactly
> what the problem is you're trying to solve here.

Hi Marek,

I am trying to write an MTD/spi-nor driver for version 2 of the
Altera Quadspi contoller.  This controller is soft IP that is deployed in 
a FPGA.  As such, this component/driver can be used in wide range of use 
cases.  The controller could be used to update EPCQ/EPCS flash stores 
containing bit streams, but this component could be used for flash for 
filesystems or any non-volatile data store.  My hope is that all possible 
use cases should be covered by this driver.



>
>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>
>>>> I don't think bit reversal is specific to Altera/Intel components. I see
>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>> other FPGA based drivers requiring bit reversal.
>>>
>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>> 0
>>>
>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>> decide whether this is SoC specific and should've altr, prefix or not.
>>> IMO it is.
>>
>> I agree there is no generic binding at this time, and I look forward
>> to any input from Rob and anyone else on this issue.  I think it is
>> worth pointing out that this really isn't an issue of an SoC, but rather
>> it is an
>> issue of how data in the flash chip is accessed.I think what makes
>> this issue
>> "weird" is that we have different hardware accessing the data in the
>> flash with a different perspective.  The FPGA looks at the data from one
>> perspective on power up, and a processor trying to update the flash has
>> a different perspective.
>
> Another thing I'd ask here is, is that bit-reverse a hardware property
> or is that some software configuration thing ?

I would say the bit reversal is a property of the FPGA that is reading the 
flash at power up.

Matthew Gerlach
>
> -- 
> Best regards,
> Marek Vasut
>

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

#1676048 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/27/2017 07:18 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 27 Jun 2017, Marek Vasut wrote:
> 
>> On 06/27/2017 05:57 PM, matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>
>>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>
>>>>>
>>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> Thanks for the feedback.  See my comments below.
>>>>>
>>>>> Matthew Gerlach
>>>>>
>>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>
>>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>>
>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>>> ++++++++++++++++++++++
>>>>>>>  1 file changed, 37 insertions(+)
>>>>>>>  create mode 100644
>>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..8ba63d7
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> @@ -0,0 +1,37 @@
>>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>>> each of
>>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>>> corresponding
>>>>>>> +          to the control and status registers and qspi memory,
>>>>>>> respectively.
>>>>>>> +
>>>>>>> +
>>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>>> windowed bridge
>>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>>> windowed
>>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>>>> bridge
>>>>>>> +          is used.  This name corresponds to the register space
>>>>>>> that
>>>>>>> +          controls the window.
>>>>>>> +- window-size : The size of the window which must be an even power
>>>>>>> of 2.
>>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>>> flash should
>>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>>> +             delivered to the MTD layer.
>>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>>> flash should
>>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>>
>>>>>> Is there ever a usecase where you need to set just one of these
>>>>>> props ?
>>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>>
>>>>> In general, I think if bit reversal is required, it would be
>>>>> required in
>>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>>> I thought separate booleans would be future proofing the bindings.
>>>>
>>>> Maybe we should drop this whole thing and add it when this is actually
>>>> required.
>>>>
>>>> Are there any users of this in the wild currently ?
>>>>
>>>> What is the purpose of doing this per-byte bit reverse instead of
>>>> storing th bits in the original order ?
>>>
>>> Hi Marek,
>>>
>>> Yes, there is hardware that has been in the wild for years that needs
>>> this bit reversal.  The specific use case is when a flash chip is
>>> connected to
>>> a FPGA, and the contents of the flash is used to configure the FPGA on
>>> power up.  In this use case, there is no processor involved with
>>> configuring the FPGA.  I am most familiar with this feature/bug with
>>> Altera FPGAs, but I believe this issue exists with other programmable
>>> devices.
>>
>> So the EPCQ/EPCS flash stores the bitstream in reverse or something ?
>> What are you storing in that flash except for the bitstream, filesystem?
>> Feel free to go into details, I believe it'd be useful to know exactly
>> what the problem is you're trying to solve here.
> 
> Hi Marek,
> 
> I am trying to write an MTD/spi-nor driver for version 2 of the
> Altera Quadspi contoller.  This controller is soft IP that is deployed
> in a FPGA.  As such, this component/driver can be used in wide range of
> use cases.  The controller could be used to update EPCQ/EPCS flash
> stores containing bit streams, but this component could be used for
> flash for filesystems or any non-volatile data store.  My hope is that
> all possible use cases should be covered by this driver.

How does this particular case where you have to reverse the bits look like ?

>>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>>
>>>>> I don't think bit reversal is specific to Altera/Intel components.
>>>>> I see
>>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>>> other FPGA based drivers requiring bit reversal.
>>>>
>>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>>> 0
>>>>
>>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>>> decide whether this is SoC specific and should've altr, prefix or not.
>>>> IMO it is.
>>>
>>> I agree there is no generic binding at this time, and I look forward
>>> to any input from Rob and anyone else on this issue.  I think it is
>>> worth pointing out that this really isn't an issue of an SoC, but rather
>>> it is an
>>> issue of how data in the flash chip is accessed.I think what makes
>>> this issue
>>> "weird" is that we have different hardware accessing the data in the
>>> flash with a different perspective.  The FPGA looks at the data from one
>>> perspective on power up, and a processor trying to update the flash has
>>> a different perspective.
>>
>> Another thing I'd ask here is, is that bit-reverse a hardware property
>> or is that some software configuration thing ?
> 
> I would say the bit reversal is a property of the FPGA that is reading
> the flash at power up.

So it's not a property of the block, but rather of the bus somewhere ?

-- 
Best regards,
Marek Vasut

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

#1676105 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/27/2017 07:18 PM, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>
>>> On 06/27/2017 05:57 PM, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>
>>>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> Thanks for the feedback.  See my comments below.
>>>>>>
>>>>>> Matthew Gerlach
>>>>>>
>>>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>>
>>>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>  1 file changed, 37 insertions(+)
>>>>>>>>  create mode 100644
>>>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..8ba63d7
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>>>> each of
>>>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>>>> corresponding
>>>>>>>> +          to the control and status registers and qspi memory,
>>>>>>>> respectively.
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>>>> windowed bridge
>>>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>>>> windowed
>>>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>>>> +
>>>>>>>> +Optional properties:
>>>>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>>>>> bridge
>>>>>>>> +          is used.  This name corresponds to the register space
>>>>>>>> that
>>>>>>>> +          controls the window.
>>>>>>>> +- window-size : The size of the window which must be an even power
>>>>>>>> of 2.
>>>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>>>> flash should
>>>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>>>> +             delivered to the MTD layer.
>>>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>>>> flash should
>>>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>>>
>>>>>>> Is there ever a usecase where you need to set just one of these
>>>>>>> props ?
>>>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>>>
>>>>>> In general, I think if bit reversal is required, it would be
>>>>>> required in
>>>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>>>> I thought separate booleans would be future proofing the bindings.
>>>>>
>>>>> Maybe we should drop this whole thing and add it when this is actually
>>>>> required.
>>>>>
>>>>> Are there any users of this in the wild currently ?
>>>>>
>>>>> What is the purpose of doing this per-byte bit reverse instead of
>>>>> storing th bits in the original order ?
>>>>
>>>> Hi Marek,
>>>>
>>>> Yes, there is hardware that has been in the wild for years that needs
>>>> this bit reversal.  The specific use case is when a flash chip is
>>>> connected to
>>>> a FPGA, and the contents of the flash is used to configure the FPGA on
>>>> power up.  In this use case, there is no processor involved with
>>>> configuring the FPGA.  I am most familiar with this feature/bug with
>>>> Altera FPGAs, but I believe this issue exists with other programmable
>>>> devices.
>>>
>>> So the EPCQ/EPCS flash stores the bitstream in reverse or something ?
>>> What are you storing in that flash except for the bitstream, filesystem?
>>> Feel free to go into details, I believe it'd be useful to know exactly
>>> what the problem is you're trying to solve here.
>>
>> Hi Marek,
>>
>> I am trying to write an MTD/spi-nor driver for version 2 of the
>> Altera Quadspi contoller.  This controller is soft IP that is deployed
>> in a FPGA.  As such, this component/driver can be used in wide range of
>> use cases.  The controller could be used to update EPCQ/EPCS flash
>> stores containing bit streams, but this component could be used for
>> flash for filesystems or any non-volatile data store.  My hope is that
>> all possible use cases should be covered by this driver.
>
> How does this particular case where you have to reverse the bits look like ?

The use case for reversing the bits involves a processor updating 
EPCQ/EPCS flash whose contents are read by the FPGA on power up.  The 
processor and Altera Quadspi component, inside the configured FPGA, access 
the bits in one way serially, but the hardware that accesses the flash 
during power accesses the bits in the opposite way serially.

>
>>>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>>>
>>>>>> I don't think bit reversal is specific to Altera/Intel components.
>>>>>> I see
>>>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>>>> other FPGA based drivers requiring bit reversal.
>>>>>
>>>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>>>> 0
>>>>>
>>>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>>>> decide whether this is SoC specific and should've altr, prefix or not.
>>>>> IMO it is.
>>>>
>>>> I agree there is no generic binding at this time, and I look forward
>>>> to any input from Rob and anyone else on this issue.  I think it is
>>>> worth pointing out that this really isn't an issue of an SoC, but rather
>>>> it is an
>>>> issue of how data in the flash chip is accessed.I think what makes
>>>> this issue
>>>> "weird" is that we have different hardware accessing the data in the
>>>> flash with a different perspective.  The FPGA looks at the data from one
>>>> perspective on power up, and a processor trying to update the flash has
>>>> a different perspective.
>>>
>>> Another thing I'd ask here is, is that bit-reverse a hardware property
>>> or is that some software configuration thing ?
>>
>> I would say the bit reversal is a property of the FPGA that is reading
>> the flash at power up.
>
> So it's not a property of the block, but rather of the bus somewhere ?

You are correct, it is not a property of the Altera Quadspi component, but
a property of the fpga and external hardware that access the flash on 
power up.

Thanks again,
Matthew Gerlach

>
> -- 
> Best regards,
> Marek Vasut
>

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

#1676138 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/27/2017 09:32 PM, matthew.gerlach@linux.intel.com wrote:

[...]

>>> Hi Marek,
>>>
>>> I am trying to write an MTD/spi-nor driver for version 2 of the
>>> Altera Quadspi contoller.  This controller is soft IP that is deployed
>>> in a FPGA.  As such, this component/driver can be used in wide range of
>>> use cases.  The controller could be used to update EPCQ/EPCS flash
>>> stores containing bit streams, but this component could be used for
>>> flash for filesystems or any non-volatile data store.  My hope is that
>>> all possible use cases should be covered by this driver.
>>
>> How does this particular case where you have to reverse the bits look
>> like ?
> 
> The use case for reversing the bits involves a processor updating
> EPCQ/EPCS flash whose contents are read by the FPGA on power up.  The
> processor and Altera Quadspi component, inside the configured FPGA,
> access the bits in one way serially, but the hardware that accesses the
> flash during power accesses the bits in the opposite way serially.

So it's the same crap Xilinx does, they have some tool to do this with
their bitstream before they write it into flash.

But then, you only have to do it with your bitstream. If you put ie. UBI
after the bitstream part, the UBI can be in normal bit ordering.
So instead of polluting the DT bindings with this, make a similar tool
to what Xilinx has, do the bit shuffling using that tool and then write
the bitstream into the flash in the correct order.

>>>>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>>>>
>>>>>>> I don't think bit reversal is specific to Altera/Intel components.
>>>>>>> I see
>>>>>>> a nand driver performing bit reversal, and I think I've recently
>>>>>>> seen
>>>>>>> other FPGA based drivers requiring bit reversal.
>>>>>>
>>>>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>>>>> 0
>>>>>>
>>>>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>>>>> decide whether this is SoC specific and should've altr, prefix or
>>>>>> not.
>>>>>> IMO it is.
>>>>>
>>>>> I agree there is no generic binding at this time, and I look forward
>>>>> to any input from Rob and anyone else on this issue.  I think it is
>>>>> worth pointing out that this really isn't an issue of an SoC, but
>>>>> rather
>>>>> it is an
>>>>> issue of how data in the flash chip is accessed.I think what makes
>>>>> this issue
>>>>> "weird" is that we have different hardware accessing the data in the
>>>>> flash with a different perspective.  The FPGA looks at the data
>>>>> from one
>>>>> perspective on power up, and a processor trying to update the flash
>>>>> has
>>>>> a different perspective.
>>>>
>>>> Another thing I'd ask here is, is that bit-reverse a hardware property
>>>> or is that some software configuration thing ?
>>>
>>> I would say the bit reversal is a property of the FPGA that is reading
>>> the flash at power up.
>>
>> So it's not a property of the block, but rather of the bus somewhere ?
> 
> You are correct, it is not a property of the Altera Quadspi component, but
> a property of the fpga and external hardware that access the flash on
> power up.

So yes, it's a property of that small thing which loads the bitstream
from the EPCS/EPCQ and programs the cells in the FPGA. This shouldn't be
in this driver nor it's bindings, see above.

-- 
Best regards,
Marek Vasut

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

#1677262 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Tue, Jun 27, 2017 at 08:57:14AM -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 27 Jun 2017, Marek Vasut wrote:
> 
> > On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
> > > 
> > > 
> > > On Tue, 27 Jun 2017, Marek Vasut wrote:
> > > 
> > > Hi Marek,
> > > 
> > > Thanks for the feedback.  See my comments below.
> > > 
> > > Matthew Gerlach
> > > 
> > > > On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
> > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > 
> > > > > Device Tree bindings for Version 2 of the Altera Quadspi Controller
> > > > > that can be optionally paired with a windowed bridge.
> > > > > 
> > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > ---
> > > > >  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
> > > > > ++++++++++++++++++++++
> > > > >  1 file changed, 37 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> > > > > b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> > > > > new file mode 100644
> > > > > index 0000000..8ba63d7
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
> > > > > @@ -0,0 +1,37 @@
> > > > > +* Altera Quad SPI Controller Version 2
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "altr,quadspi-v2".
> > > > > +- reg : Contains at least two entries, and possibly three entries,
> > > > > each of
> > > > > +    which is a tuple consisting of a physical address and length.
> > > > > +- reg-names : Should contain the names "avl_csr" and "avl_mem"
> > > > > corresponding
> > > > > +          to the control and status registers and qspi memory,
> > > > > respectively.
> > > > > +
> > > > > +
> > > > > +The Altera Quad SPI Controller Version 2 can be paired with a
> > > > > windowed bridge
> > > > > +in order to reduce the footprint of the memory interface.  When a
> > > > > windowed
> > > > > +bridge is used, reads and writes of data must be 32 bits wide.
> > > > > +
> > > > > +Optional properties:
> > > > > +- reg-names : Should contain the name "avl_window", if the windowed
> > > > > bridge
> > > > > +          is used.  This name corresponds to the register space that
> > > > > +          controls the window.
> > > > > +- window-size : The size of the window which must be an even power
> > > > > of 2.
> > > > > +- read-bit-reverse : A boolean indicating the data read from the
> > > > > flash should
> > > > > +             be bit reversed on a byte by byte basis before being
> > > > > +             delivered to the MTD layer.
> > > > > +- write-bit-reverse : A boolean indicating the data written to the
> > > > > flash should
> > > > > +              be bit reversed on a byte by byte basis.
> > > > 
> > > > Is there ever a usecase where you need to set just one of these props ?
> > > > Also, they're altera specific, so altr, prefix should be added.
> > > 
> > > In general, I think if bit reversal is required, it would be required in
> > > both directions.  However, anything is possible when using FPGAs.  So
> > > I thought separate booleans would be future proofing the bindings.
> > 
> > Maybe we should drop this whole thing and add it when this is actually
> > required.
> > 
> > Are there any users of this in the wild currently ?
> > 
> > What is the purpose of doing this per-byte bit reverse instead of
> > storing th bits in the original order ?
> 
> Hi Marek,
> 
> Yes, there is hardware that has been in the wild for years that needs this
> bit reversal.  The specific use case is when a flash chip is connected to
> a FPGA, and the contents of the flash is used to configure the FPGA on power
> up.  In this use case, there is no processor involved with configuring the
> FPGA.  I am most familiar with this feature/bug with Altera FPGAs, but I
> believe this issue exists with other programmable devices.
> 
> > 
> > > Thinking about this binding more, I wonder if the binding name(s)
> > > should be (read|write)-bit8-reverse to indicate reversings the bits
> > > in a byte as opposed to reversing the bits in a 32 bit word?
> > > 
> > > I don't think bit reversal is specific to Altera/Intel components. I see
> > > a nand driver performing bit reversal, and I think I've recently seen
> > > other FPGA based drivers requiring bit reversal.
> > 
> > $ git grep bit.reverse Documentation/devicetree/ | wc -l
> > 0
> > 
> > So we don't have such a generic binding . It's up to Rob (I guess) to
> > decide whether this is SoC specific and should've altr, prefix or not.
> > IMO it is.
> 
> I agree there is no generic binding at this time, and I look forward
> to any input from Rob and anyone else on this issue.  I think it is worth
> pointing out that this really isn't an issue of an SoC, but rather it is an
> issue of how data in the flash chip is accessed.  I think what makes this issue
> "weird" is that we have different hardware accessing the data in the flash
> with a different perspective.  The FPGA looks at the data from one
> perspective on power up, and a processor trying to update the flash has a
> different perspective.

Given the comment that it is reversing bits in each byte, that seems 
fairly Altera specific. I'd be more in favor of a generic property if it 
was flipping all the bits in a word (for any size word).

Rob

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

#1677567 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/29/2017 01:09 AM, Rob Herring wrote:
> On Tue, Jun 27, 2017 at 08:57:14AM -0700, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>
>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>
>>>> Hi Marek,
>>>>
>>>> Thanks for the feedback.  See my comments below.
>>>>
>>>> Matthew Gerlach
>>>>
>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>
>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>
>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>> ++++++++++++++++++++++
>>>>>>  1 file changed, 37 insertions(+)
>>>>>>  create mode 100644
>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..8ba63d7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>> @@ -0,0 +1,37 @@
>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>> each of
>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>> corresponding
>>>>>> +          to the control and status registers and qspi memory,
>>>>>> respectively.
>>>>>> +
>>>>>> +
>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>> windowed bridge
>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>> windowed
>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>>> bridge
>>>>>> +          is used.  This name corresponds to the register space that
>>>>>> +          controls the window.
>>>>>> +- window-size : The size of the window which must be an even power
>>>>>> of 2.
>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>> flash should
>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>> +             delivered to the MTD layer.
>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>> flash should
>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>
>>>>> Is there ever a usecase where you need to set just one of these props ?
>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>
>>>> In general, I think if bit reversal is required, it would be required in
>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>> I thought separate booleans would be future proofing the bindings.
>>>
>>> Maybe we should drop this whole thing and add it when this is actually
>>> required.
>>>
>>> Are there any users of this in the wild currently ?
>>>
>>> What is the purpose of doing this per-byte bit reverse instead of
>>> storing th bits in the original order ?
>>
>> Hi Marek,
>>
>> Yes, there is hardware that has been in the wild for years that needs this
>> bit reversal.  The specific use case is when a flash chip is connected to
>> a FPGA, and the contents of the flash is used to configure the FPGA on power
>> up.  In this use case, there is no processor involved with configuring the
>> FPGA.  I am most familiar with this feature/bug with Altera FPGAs, but I
>> believe this issue exists with other programmable devices.
>>
>>>
>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>
>>>> I don't think bit reversal is specific to Altera/Intel components. I see
>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>> other FPGA based drivers requiring bit reversal.
>>>
>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>> 0
>>>
>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>> decide whether this is SoC specific and should've altr, prefix or not.
>>> IMO it is.
>>
>> I agree there is no generic binding at this time, and I look forward
>> to any input from Rob and anyone else on this issue.  I think it is worth
>> pointing out that this really isn't an issue of an SoC, but rather it is an
>> issue of how data in the flash chip is accessed.  I think what makes this issue
>> "weird" is that we have different hardware accessing the data in the flash
>> with a different perspective.  The FPGA looks at the data from one
>> perspective on power up, and a processor trying to update the flash has a
>> different perspective.
> 
> Given the comment that it is reversing bits in each byte, that seems 
> fairly Altera specific. I'd be more in favor of a generic property if it 
> was flipping all the bits in a word (for any size word).

Actually, I'd prefer to fix up the FPGA bitstream in software and then
write the fixed up bitstream into the flash. That way there's no need
for any such DT property and other FPGA vendors (ie. xilinx) do it that
way already.

-- 
Best regards,
Marek Vasut

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

#1677852 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Thu, 29 Jun 2017, Marek Vasut wrote:

> On 06/29/2017 01:09 AM, Rob Herring wrote:
>> On Tue, Jun 27, 2017 at 08:57:14AM -0700, matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>
>>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>
>>>>>
>>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> Thanks for the feedback.  See my comments below.
>>>>>
>>>>> Matthew Gerlach
>>>>>
>>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>
>>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>>
>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>>> ++++++++++++++++++++++
>>>>>>>  1 file changed, 37 insertions(+)
>>>>>>>  create mode 100644
>>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..8ba63d7
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>> @@ -0,0 +1,37 @@
>>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>>> each of
>>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>>> corresponding
>>>>>>> +          to the control and status registers and qspi memory,
>>>>>>> respectively.
>>>>>>> +
>>>>>>> +
>>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>>> windowed bridge
>>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>>> windowed
>>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- reg-names : Should contain the name "avl_window", if the windowed
>>>>>>> bridge
>>>>>>> +          is used.  This name corresponds to the register space that
>>>>>>> +          controls the window.
>>>>>>> +- window-size : The size of the window which must be an even power
>>>>>>> of 2.
>>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>>> flash should
>>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>>> +             delivered to the MTD layer.
>>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>>> flash should
>>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>>
>>>>>> Is there ever a usecase where you need to set just one of these props ?
>>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>>
>>>>> In general, I think if bit reversal is required, it would be required in
>>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>>> I thought separate booleans would be future proofing the bindings.
>>>>
>>>> Maybe we should drop this whole thing and add it when this is actually
>>>> required.
>>>>
>>>> Are there any users of this in the wild currently ?
>>>>
>>>> What is the purpose of doing this per-byte bit reverse instead of
>>>> storing th bits in the original order ?
>>>
>>> Hi Marek,
>>>
>>> Yes, there is hardware that has been in the wild for years that needs this
>>> bit reversal.  The specific use case is when a flash chip is connected to
>>> a FPGA, and the contents of the flash is used to configure the FPGA on power
>>> up.  In this use case, there is no processor involved with configuring the
>>> FPGA.  I am most familiar with this feature/bug with Altera FPGAs, but I
>>> believe this issue exists with other programmable devices.
>>>
>>>>
>>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>>
>>>>> I don't think bit reversal is specific to Altera/Intel components. I see
>>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>>> other FPGA based drivers requiring bit reversal.
>>>>
>>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>>> 0
>>>>
>>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>>> decide whether this is SoC specific and should've altr, prefix or not.
>>>> IMO it is.
>>>
>>> I agree there is no generic binding at this time, and I look forward
>>> to any input from Rob and anyone else on this issue.  I think it is worth
>>> pointing out that this really isn't an issue of an SoC, but rather it is an
>>> issue of how data in the flash chip is accessed.  I think what makes this issue
>>> "weird" is that we have different hardware accessing the data in the flash
>>> with a different perspective.  The FPGA looks at the data from one
>>> perspective on power up, and a processor trying to update the flash has a
>>> different perspective.
>>
>> Given the comment that it is reversing bits in each byte, that seems
>> fairly Altera specific. I'd be more in favor of a generic property if it
>> was flipping all the bits in a word (for any size word).
>
> Actually, I'd prefer to fix up the FPGA bitstream in software and then
> write the fixed up bitstream into the flash. That way there's no need
> for any such DT property and other FPGA vendors (ie. xilinx) do it that
> way already.
>
> -- 
> Best regards,
> Marek Vasut
>

While I totally understand Marek's point of view regarding bit flipping,
I think it is instructive to explore the other point of view.  For me the 
issue of flipping the bits in user space versus kernel space came down to 
laziness, which along with impatience and hubris are considered by some to 
be good traits for a software engineer.  I looked around for an existing user 
space to perform the function, but I did not find an existing tool, but I 
suppose I could look more closely at Xilinx's tools.  I do not want to 
create my own tool, and then it was pointed out to me that the necessary 
bit flipping functions already exist in the kernel.

Even if we decided that performing the bit flipping in the kernel makes 
sense, my original binding proposal is clearly inadequate.  As Rob pointed 
such a binding should be more generic to support flipping the bits in any word 
size (i.e. 8, 16, 32, 64 ...).  Additionally, Marek has pointed out that a 
portion of the flash could requiring flipping, and another portion of the 
same flash would not require flipping.  Therefore, a bit flipping binding 
is not a property of the flash controller, but rather it is a property of 
a particular flash partition.

Matthew Gerlach

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

#1677873 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On 06/29/2017 05:03 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Thu, 29 Jun 2017, Marek Vasut wrote:
> 
>> On 06/29/2017 01:09 AM, Rob Herring wrote:
>>> On Tue, Jun 27, 2017 at 08:57:14AM -0700,
>>> matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>
>>>>> On 06/27/2017 04:32 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, 27 Jun 2017, Marek Vasut wrote:
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> Thanks for the feedback.  See my comments below.
>>>>>>
>>>>>> Matthew Gerlach
>>>>>>
>>>>>>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>>
>>>>>>>> Device Tree bindings for Version 2 of the Altera Quadspi Controller
>>>>>>>> that can be optionally paired with a windowed bridge.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>  1 file changed, 37 insertions(+)
>>>>>>>>  create mode 100644
>>>>>>>> Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..8ba63d7
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt
>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>> +* Altera Quad SPI Controller Version 2
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible : Should be "altr,quadspi-v2".
>>>>>>>> +- reg : Contains at least two entries, and possibly three entries,
>>>>>>>> each of
>>>>>>>> +    which is a tuple consisting of a physical address and length.
>>>>>>>> +- reg-names : Should contain the names "avl_csr" and "avl_mem"
>>>>>>>> corresponding
>>>>>>>> +          to the control and status registers and qspi memory,
>>>>>>>> respectively.
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +The Altera Quad SPI Controller Version 2 can be paired with a
>>>>>>>> windowed bridge
>>>>>>>> +in order to reduce the footprint of the memory interface.  When a
>>>>>>>> windowed
>>>>>>>> +bridge is used, reads and writes of data must be 32 bits wide.
>>>>>>>> +
>>>>>>>> +Optional properties:
>>>>>>>> +- reg-names : Should contain the name "avl_window", if the
>>>>>>>> windowed
>>>>>>>> bridge
>>>>>>>> +          is used.  This name corresponds to the register space
>>>>>>>> that
>>>>>>>> +          controls the window.
>>>>>>>> +- window-size : The size of the window which must be an even power
>>>>>>>> of 2.
>>>>>>>> +- read-bit-reverse : A boolean indicating the data read from the
>>>>>>>> flash should
>>>>>>>> +             be bit reversed on a byte by byte basis before being
>>>>>>>> +             delivered to the MTD layer.
>>>>>>>> +- write-bit-reverse : A boolean indicating the data written to the
>>>>>>>> flash should
>>>>>>>> +              be bit reversed on a byte by byte basis.
>>>>>>>
>>>>>>> Is there ever a usecase where you need to set just one of these
>>>>>>> props ?
>>>>>>> Also, they're altera specific, so altr, prefix should be added.
>>>>>>
>>>>>> In general, I think if bit reversal is required, it would be
>>>>>> required in
>>>>>> both directions.  However, anything is possible when using FPGAs.  So
>>>>>> I thought separate booleans would be future proofing the bindings.
>>>>>
>>>>> Maybe we should drop this whole thing and add it when this is actually
>>>>> required.
>>>>>
>>>>> Are there any users of this in the wild currently ?
>>>>>
>>>>> What is the purpose of doing this per-byte bit reverse instead of
>>>>> storing th bits in the original order ?
>>>>
>>>> Hi Marek,
>>>>
>>>> Yes, there is hardware that has been in the wild for years that
>>>> needs this
>>>> bit reversal.  The specific use case is when a flash chip is
>>>> connected to
>>>> a FPGA, and the contents of the flash is used to configure the FPGA
>>>> on power
>>>> up.  In this use case, there is no processor involved with
>>>> configuring the
>>>> FPGA.  I am most familiar with this feature/bug with Altera FPGAs,
>>>> but I
>>>> believe this issue exists with other programmable devices.
>>>>
>>>>>
>>>>>> Thinking about this binding more, I wonder if the binding name(s)
>>>>>> should be (read|write)-bit8-reverse to indicate reversings the bits
>>>>>> in a byte as opposed to reversing the bits in a 32 bit word?
>>>>>>
>>>>>> I don't think bit reversal is specific to Altera/Intel components.
>>>>>> I see
>>>>>> a nand driver performing bit reversal, and I think I've recently seen
>>>>>> other FPGA based drivers requiring bit reversal.
>>>>>
>>>>> $ git grep bit.reverse Documentation/devicetree/ | wc -l
>>>>> 0
>>>>>
>>>>> So we don't have such a generic binding . It's up to Rob (I guess) to
>>>>> decide whether this is SoC specific and should've altr, prefix or not.
>>>>> IMO it is.
>>>>
>>>> I agree there is no generic binding at this time, and I look forward
>>>> to any input from Rob and anyone else on this issue.  I think it is
>>>> worth
>>>> pointing out that this really isn't an issue of an SoC, but rather
>>>> it is an
>>>> issue of how data in the flash chip is accessed.  I think what makes
>>>> this issue
>>>> "weird" is that we have different hardware accessing the data in the
>>>> flash
>>>> with a different perspective.  The FPGA looks at the data from one
>>>> perspective on power up, and a processor trying to update the flash
>>>> has a
>>>> different perspective.
>>>
>>> Given the comment that it is reversing bits in each byte, that seems
>>> fairly Altera specific. I'd be more in favor of a generic property if it
>>> was flipping all the bits in a word (for any size word).
>>
>> Actually, I'd prefer to fix up the FPGA bitstream in software and then
>> write the fixed up bitstream into the flash. That way there's no need
>> for any such DT property and other FPGA vendors (ie. xilinx) do it that
>> way already.
>>
>> -- 
>> Best regards,
>> Marek Vasut
>>
> 
> While I totally understand Marek's point of view regarding bit flipping,
> I think it is instructive to explore the other point of view.  For me
> the issue of flipping the bits in user space versus kernel space came
> down to laziness, which along with impatience and hubris are considered
> by some to be good traits for a software engineer.  I looked around for
> an existing user space to perform the function, but I did not find an
> existing tool, but I suppose I could look more closely at Xilinx's
> tools.  I do not want to create my own tool, and then it was pointed out
> to me that the necessary bit flipping functions already exist in the
> kernel.

Random internet search gives you ie.
https://www.xilinx.com/itp/xilinx10/isehelp/pim_r_promformatter_files.htm

I recall there was some perl script to do the same too.

> Even if we decided that performing the bit flipping in the kernel makes
> sense, my original binding proposal is clearly inadequate.  As Rob
> pointed such a binding should be more generic to support flipping the
> bits in any word size (i.e. 8, 16, 32, 64 ...).  Additionally, Marek has
> pointed out that a portion of the flash could requiring flipping, and
> another portion of the same flash would not require flipping. 
> Therefore, a bit flipping binding is not a property of the flash
> controller, but rather it is a property of a particular flash partition.

And so, we're starting to invent a convoluted scheme to describe policy
in DT ; I don't like this.

What would you do once the FPGA requires another bit order, would you
add another DT property and more stuff to the kernel to handle it ? I
don't think this scales. Going ad-absurdum, I might want to be lazy and
try to dd a SOF file into a partition and hope the kernel will convert
it to RBF and do the right thing ...

But we won't put a SOF-to-RBF parser into the kernel. I don't see a
reason why we should put this bit swapping into the kernel. Just let the
user (in this case, FPGA developer) prepare the bitstream in the correct
format and write it into the flash.

> Matthew Gerlach


-- 
Best regards,
Marek Vasut

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

#1677266 — Re: [PATCH 1/3] ARM: dts: Bindings for Altera Quadspi Controller Version 2

On Mon, Jun 26, 2017 at 09:13:37AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

"dt-bindings: mtd: ..." for the subject.

Other than the discussion about bit reverse, the rest looks fine.

> 
> Device Tree bindings for Version 2 of the Altera Quadspi Controller
> that can be optionally paired with a windowed bridge.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  .../devicetree/bindings/mtd/altera-quadspi-v2.txt  | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/altera-quadspi-v2.txt

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

#1674921 — [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 MAINTAINERS                                   |   1 +
 drivers/mtd/spi-nor/Kconfig                   |   5 +
 drivers/mtd/spi-nor/Makefile                  |   1 +
 drivers/mtd/spi-nor/altera-quadspi-platform.c | 137 ++++++++++++++++++++++++++
 4 files changed, 144 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/altera-quadspi-platform.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae33fa6..c32bb98 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -653,6 +653,7 @@ M:	Matthew Gerlach <matthew.gerlach@linux.intel.com>
 L:	linux-mtd@lists.infradead.org
 S:	Maintained
 F:	drivers/mtd/spi-nor/altera-quadspi.c
+F:	drivers/mtd/spi-nor/altera-quadspi-platform.c
 F:	inclulde/linux/mtd/altera-quadspi.h
 
 ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 89fe425..f3d5c01 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -118,4 +118,9 @@ config SPI_ALTERA_QUADSPI
         help
           Enable support for version 2 of Altera Quad SPI Flash Controller.
 
+config SPI_ALTERA_QUADSPI_PLATFORM
+	tristate "Platform support for Altera Quad SPI Flash Controller II"
+	help
+	  Platform driver support for  Altera Quad SPI Flash Controller II"
+
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 024c6ac..042f87e 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_SPI_INTEL_SPI)	+= intel-spi.o
 obj-$(CONFIG_SPI_INTEL_SPI_PLATFORM)	+= intel-spi-platform.o
 obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
 obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
+obj-$(CONFIG_SPI_ALTERA_QUADSPI_PLATFORM) += altera-quadspi-platform.o
 
diff --git a/drivers/mtd/spi-nor/altera-quadspi-platform.c b/drivers/mtd/spi-nor/altera-quadspi-platform.c
new file mode 100644
index 0000000..c8d2a47
--- /dev/null
+++ b/drivers/mtd/spi-nor/altera-quadspi-platform.c
@@ -0,0 +1,137 @@
+/*
+ * Copyright (C) 2014 Altera Corporation. All rights reserved.
+ * Copyright (C) 2017 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/mtd/altera-quadspi.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+
+static int altera_quadspi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *csr_base;
+	void __iomem *data_base;
+	void __iomem *window_base = NULL;
+	u32 window_size = 0;
+	u32 flags = 0;
+	u32 bank;
+	int ret;
+	struct device_node *pp;
+
+	if (!np) {
+		dev_err(dev, "no device found\n");
+		return -ENODEV;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_csr");
+	csr_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(csr_base)) {
+		dev_err(dev, "%s: ERROR: failed to map csr base\n", __func__);
+		return PTR_ERR(csr_base);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_mem");
+	data_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data_base)) {
+		dev_err(dev, "%s: ERROR: failed to map data base\n", __func__);
+		return PTR_ERR(data_base);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_window");
+	if (res) {
+		window_base = NULL;
+		window_base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(window_base)) {
+			dev_err(dev, "%s: ERROR: failed to map window base\n",
+				__func__);
+			return PTR_ERR(data_base);
+		}
+
+		of_property_read_u32(dev->of_node, "window-size", &window_size);
+
+		if (!window_size) {
+			dev_err(dev,
+				"alv_window defined, %s",
+				"but no window-size defined\n");
+			return -EINVAL;
+		}
+	}
+
+	if (of_property_read_bool(np, "read-bit-reverse"))
+		flags |= ALTERA_QUADSPI_FL_BITREV_READ;
+
+	if (of_property_read_bool(np, "write-bit-reverse"))
+		flags |= ALTERA_QUADSPI_FL_BITREV_WRITE;
+
+	ret = altera_quadspi_create(dev, csr_base, data_base,
+				    window_base, (size_t)window_size, flags);
+
+	if (ret) {
+		dev_err(dev, "failed to create qspi device\n");
+		return ret;
+	}
+
+	for_each_available_child_of_node(np, pp) {
+		of_property_read_u32(pp, "reg", &bank);
+		if (bank >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP) {
+			dev_err(dev, "bad reg value %u >= %u\n", bank,
+				ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP);
+			goto error;
+		}
+
+		if (altera_qspi_add_bank(dev, bank, pp)) {
+			dev_err(dev, "failed to add bank %u\n", bank);
+			goto error;
+		}
+	}
+
+	return 0;
+error:
+	altera_quadspi_remove_banks(dev);
+	return -EIO;
+}
+
+static int altera_quadspi_remove(struct platform_device *pdev)
+{
+	return altera_quadspi_remove_banks(&pdev->dev);
+}
+
+static const struct of_device_id altera_quadspi_id_table[] = {
+
+	{ .compatible = "altr,quadspi-v2",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, altera_quadspi_id_table);
+
+static struct platform_driver altera_quadspi_driver = {
+	.driver = {
+		.name = "altera_quadspi_platform",
+		.of_match_table = altera_quadspi_id_table,
+	},
+	.probe = altera_quadspi_probe,
+	.remove = altera_quadspi_remove,
+};
+module_platform_driver(altera_quadspi_driver);
+
+MODULE_AUTHOR("Viet Nga Dao <vndao@altera.com>");
+MODULE_AUTHOR("Yong Sern Lau <lau.yong.sern@intel.com>");
+MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
+MODULE_DESCRIPTION("Altera QuadSPI Version 2 Platform Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

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

#1675524 — Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver

On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Just wrap it into the Altera QSPI driver , no need for separate platform
driver IMO.

> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  MAINTAINERS                                   |   1 +
>  drivers/mtd/spi-nor/Kconfig                   |   5 +
>  drivers/mtd/spi-nor/Makefile                  |   1 +
>  drivers/mtd/spi-nor/altera-quadspi-platform.c | 137 ++++++++++++++++++++++++++
>  4 files changed, 144 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi-platform.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae33fa6..c32bb98 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -653,6 +653,7 @@ M:	Matthew Gerlach <matthew.gerlach@linux.intel.com>
>  L:	linux-mtd@lists.infradead.org
>  S:	Maintained
>  F:	drivers/mtd/spi-nor/altera-quadspi.c
> +F:	drivers/mtd/spi-nor/altera-quadspi-platform.c
>  F:	inclulde/linux/mtd/altera-quadspi.h
>  
>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 89fe425..f3d5c01 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -118,4 +118,9 @@ config SPI_ALTERA_QUADSPI
>          help
>            Enable support for version 2 of Altera Quad SPI Flash Controller.
>  
> +config SPI_ALTERA_QUADSPI_PLATFORM
> +	tristate "Platform support for Altera Quad SPI Flash Controller II"
> +	help
> +	  Platform driver support for  Altera Quad SPI Flash Controller II"
> +
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 024c6ac..042f87e 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_SPI_INTEL_SPI)	+= intel-spi.o
>  obj-$(CONFIG_SPI_INTEL_SPI_PLATFORM)	+= intel-spi-platform.o
>  obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>  obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
> +obj-$(CONFIG_SPI_ALTERA_QUADSPI_PLATFORM) += altera-quadspi-platform.o
>  
> diff --git a/drivers/mtd/spi-nor/altera-quadspi-platform.c b/drivers/mtd/spi-nor/altera-quadspi-platform.c
> new file mode 100644
> index 0000000..c8d2a47
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/altera-quadspi-platform.c
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2014 Altera Corporation. All rights reserved.
> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/altera-quadspi.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +
> +static int altera_quadspi_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *csr_base;
> +	void __iomem *data_base;
> +	void __iomem *window_base = NULL;
> +	u32 window_size = 0;
> +	u32 flags = 0;
> +	u32 bank;
> +	int ret;
> +	struct device_node *pp;
> +
> +	if (!np) {
> +		dev_err(dev, "no device found\n");
> +		return -ENODEV;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_csr");
> +	csr_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(csr_base)) {
> +		dev_err(dev, "%s: ERROR: failed to map csr base\n", __func__);
> +		return PTR_ERR(csr_base);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_mem");
> +	data_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data_base)) {
> +		dev_err(dev, "%s: ERROR: failed to map data base\n", __func__);
> +		return PTR_ERR(data_base);
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_window");
> +	if (res) {
> +		window_base = NULL;
> +		window_base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(window_base)) {
> +			dev_err(dev, "%s: ERROR: failed to map window base\n",
> +				__func__);
> +			return PTR_ERR(data_base);
> +		}
> +
> +		of_property_read_u32(dev->of_node, "window-size", &window_size);
> +
> +		if (!window_size) {
> +			dev_err(dev,
> +				"alv_window defined, %s",
> +				"but no window-size defined\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (of_property_read_bool(np, "read-bit-reverse"))
> +		flags |= ALTERA_QUADSPI_FL_BITREV_READ;
> +
> +	if (of_property_read_bool(np, "write-bit-reverse"))
> +		flags |= ALTERA_QUADSPI_FL_BITREV_WRITE;
> +
> +	ret = altera_quadspi_create(dev, csr_base, data_base,
> +				    window_base, (size_t)window_size, flags);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to create qspi device\n");
> +		return ret;
> +	}
> +
> +	for_each_available_child_of_node(np, pp) {
> +		of_property_read_u32(pp, "reg", &bank);
> +		if (bank >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP) {
> +			dev_err(dev, "bad reg value %u >= %u\n", bank,
> +				ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP);
> +			goto error;
> +		}
> +
> +		if (altera_qspi_add_bank(dev, bank, pp)) {
> +			dev_err(dev, "failed to add bank %u\n", bank);
> +			goto error;
> +		}
> +	}
> +
> +	return 0;
> +error:
> +	altera_quadspi_remove_banks(dev);
> +	return -EIO;
> +}
> +
> +static int altera_quadspi_remove(struct platform_device *pdev)
> +{
> +	return altera_quadspi_remove_banks(&pdev->dev);
> +}
> +
> +static const struct of_device_id altera_quadspi_id_table[] = {
> +
> +	{ .compatible = "altr,quadspi-v2",},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, altera_quadspi_id_table);
> +
> +static struct platform_driver altera_quadspi_driver = {
> +	.driver = {
> +		.name = "altera_quadspi_platform",
> +		.of_match_table = altera_quadspi_id_table,
> +	},
> +	.probe = altera_quadspi_probe,
> +	.remove = altera_quadspi_remove,
> +};
> +module_platform_driver(altera_quadspi_driver);
> +
> +MODULE_AUTHOR("Viet Nga Dao <vndao@altera.com>");
> +MODULE_AUTHOR("Yong Sern Lau <lau.yong.sern@intel.com>");
> +MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
> +MODULE_DESCRIPTION("Altera QuadSPI Version 2 Platform Driver");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Best regards,
Marek Vasut

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

#1675920 — Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Just wrap it into the Altera QSPI driver , no need for separate platform
> driver IMO.

Hi Marek,

I answered this question when you asked why the header file was necessary, 
but I think further discussion could be helpful, since this problem is 
becoming more prevelent.  The Altera Quadspi component is a soft IP in a
FPGA, and the processor using the component may or may not have device 
tree support compiled into the Linux kernel.  Since device tree support 
may or may not be available, the device tree specific code must be separated
from the core driver code.

One can certainly make the case, that device tree support could/should
be available everywhere, but the current reality is most x86 Linux
kernel configurations do not include device tree support.

For the record, I believe device trees, and more specifically device tree 
overlays, are the best way for Linux to use FPGAs, but I have to deal with 
the current realities.

Thanks again for all the great feedback.

Matthew Gerlach

>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  MAINTAINERS                                   |   1 +
>>  drivers/mtd/spi-nor/Kconfig                   |   5 +
>>  drivers/mtd/spi-nor/Makefile                  |   1 +
>>  drivers/mtd/spi-nor/altera-quadspi-platform.c | 137 ++++++++++++++++++++++++++
>>  4 files changed, 144 insertions(+)
>>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi-platform.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ae33fa6..c32bb98 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -653,6 +653,7 @@ M:	Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>  L:	linux-mtd@lists.infradead.org
>>  S:	Maintained
>>  F:	drivers/mtd/spi-nor/altera-quadspi.c
>> +F:	drivers/mtd/spi-nor/altera-quadspi-platform.c
>>  F:	inclulde/linux/mtd/altera-quadspi.h
>>
>>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 89fe425..f3d5c01 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -118,4 +118,9 @@ config SPI_ALTERA_QUADSPI
>>          help
>>            Enable support for version 2 of Altera Quad SPI Flash Controller.
>>
>> +config SPI_ALTERA_QUADSPI_PLATFORM
>> +	tristate "Platform support for Altera Quad SPI Flash Controller II"
>> +	help
>> +	  Platform driver support for  Altera Quad SPI Flash Controller II"
>> +
>>  endif # MTD_SPI_NOR
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 024c6ac..042f87e 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -10,4 +10,5 @@ obj-$(CONFIG_SPI_INTEL_SPI)	+= intel-spi.o
>>  obj-$(CONFIG_SPI_INTEL_SPI_PLATFORM)	+= intel-spi-platform.o
>>  obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>>  obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
>> +obj-$(CONFIG_SPI_ALTERA_QUADSPI_PLATFORM) += altera-quadspi-platform.o
>>
>> diff --git a/drivers/mtd/spi-nor/altera-quadspi-platform.c b/drivers/mtd/spi-nor/altera-quadspi-platform.c
>> new file mode 100644
>> index 0000000..c8d2a47
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/altera-quadspi-platform.c
>> @@ -0,0 +1,137 @@
>> +/*
>> + * Copyright (C) 2014 Altera Corporation. All rights reserved.
>> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/mtd/altera-quadspi.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +
>> +static int altera_quadspi_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *csr_base;
>> +	void __iomem *data_base;
>> +	void __iomem *window_base = NULL;
>> +	u32 window_size = 0;
>> +	u32 flags = 0;
>> +	u32 bank;
>> +	int ret;
>> +	struct device_node *pp;
>> +
>> +	if (!np) {
>> +		dev_err(dev, "no device found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_csr");
>> +	csr_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(csr_base)) {
>> +		dev_err(dev, "%s: ERROR: failed to map csr base\n", __func__);
>> +		return PTR_ERR(csr_base);
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_mem");
>> +	data_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data_base)) {
>> +		dev_err(dev, "%s: ERROR: failed to map data base\n", __func__);
>> +		return PTR_ERR(data_base);
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_window");
>> +	if (res) {
>> +		window_base = NULL;
>> +		window_base = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(window_base)) {
>> +			dev_err(dev, "%s: ERROR: failed to map window base\n",
>> +				__func__);
>> +			return PTR_ERR(data_base);
>> +		}
>> +
>> +		of_property_read_u32(dev->of_node, "window-size", &window_size);
>> +
>> +		if (!window_size) {
>> +			dev_err(dev,
>> +				"alv_window defined, %s",
>> +				"but no window-size defined\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	if (of_property_read_bool(np, "read-bit-reverse"))
>> +		flags |= ALTERA_QUADSPI_FL_BITREV_READ;
>> +
>> +	if (of_property_read_bool(np, "write-bit-reverse"))
>> +		flags |= ALTERA_QUADSPI_FL_BITREV_WRITE;
>> +
>> +	ret = altera_quadspi_create(dev, csr_base, data_base,
>> +				    window_base, (size_t)window_size, flags);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "failed to create qspi device\n");
>> +		return ret;
>> +	}
>> +
>> +	for_each_available_child_of_node(np, pp) {
>> +		of_property_read_u32(pp, "reg", &bank);
>> +		if (bank >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP) {
>> +			dev_err(dev, "bad reg value %u >= %u\n", bank,
>> +				ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP);
>> +			goto error;
>> +		}
>> +
>> +		if (altera_qspi_add_bank(dev, bank, pp)) {
>> +			dev_err(dev, "failed to add bank %u\n", bank);
>> +			goto error;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +error:
>> +	altera_quadspi_remove_banks(dev);
>> +	return -EIO;
>> +}
>> +
>> +static int altera_quadspi_remove(struct platform_device *pdev)
>> +{
>> +	return altera_quadspi_remove_banks(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id altera_quadspi_id_table[] = {
>> +
>> +	{ .compatible = "altr,quadspi-v2",},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, altera_quadspi_id_table);
>> +
>> +static struct platform_driver altera_quadspi_driver = {
>> +	.driver = {
>> +		.name = "altera_quadspi_platform",
>> +		.of_match_table = altera_quadspi_id_table,
>> +	},
>> +	.probe = altera_quadspi_probe,
>> +	.remove = altera_quadspi_remove,
>> +};
>> +module_platform_driver(altera_quadspi_driver);
>> +
>> +MODULE_AUTHOR("Viet Nga Dao <vndao@altera.com>");
>> +MODULE_AUTHOR("Yong Sern Lau <lau.yong.sern@intel.com>");
>> +MODULE_AUTHOR("Matthew Gerlach <matthew.gerlach@linux.intel.com>");
>> +MODULE_DESCRIPTION("Altera QuadSPI Version 2 Platform Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
>
> -- 
> Best regards,
> Marek Vasut
>

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

#1675974 — Re: [PATCH 3/3] mtd: spi-nor: Altera Quadspi Flash Controller v2 Platform driver

On 06/27/2017 05:15 PM, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 27 Jun 2017, Marek Vasut wrote:
> 
>> On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Just wrap it into the Altera QSPI driver , no need for separate platform
>> driver IMO.
> 
> Hi Marek,
> 
> I answered this question when you asked why the header file was
> necessary, but I think further discussion could be helpful, since this
> problem is becoming more prevelent.  The Altera Quadspi component is a
> soft IP in a
> FPGA, and the processor using the component may or may not have device
> tree support compiled into the Linux kernel.  Since device tree support
> may or may not be available, the device tree specific code must be
> separated
> from the core driver code.

I see, that's fine, although there is no PCIe or other support in this
submission. Is that planned ?

> One can certainly make the case, that device tree support could/should
> be available everywhere, but the current reality is most x86 Linux
> kernel configurations do not include device tree support.
> 
> For the record, I believe device trees, and more specifically device
> tree overlays, are the best way for Linux to use FPGAs, but I have to
> deal with the current realities.
> 
> Thanks again for all the great feedback.
> 
> Matthew Gerlach

[...]

-- 
Best regards,
Marek Vasut

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web