[PATCH 0/3] Altera Quadspi Controller Version 2

[PATCH 0/3] Altera Quadspi Controller Version 2

[matthew.gerlach]

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

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

[matthew.gerlach]

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

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

[Marek Vasut]

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

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

[matthew.gerlach]

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
>

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

[Marek Vasut]

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

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

[matthew.gerlach]

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
>

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

[Marek Vasut]

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

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

[matthew.gerlach]

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
>

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

[Marek Vasut]

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

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

[matthew.gerlach]

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
>

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

[Marek Vasut]

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

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

[Rob Herring]

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

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

[Marek Vasut]

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

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

[matthew.gerlach]

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

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

[Marek Vasut]

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

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

[Rob Herring]

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

[PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[matthew.gerlach]

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 6b4395c..ae33fa6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -648,6 +648,13 @@ L:	linux-gpio@vger.kernel.org
 S:	Maintained
 F:	drivers/gpio/gpio-altera.c
 
+ALTERA QUADSPI FLASH DRIVER
+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:	inclulde/linux/mtd/altera-quadspi.h
+
 ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
 M:	Thor Thayer <thor.thayer@linux.intel.com>
 S:	Maintained
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 293c8a4..89fe425 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -113,4 +113,9 @@ config SPI_STM32_QUADSPI
 	  This enables support for the STM32 Quad SPI controller.
 	  We only connect the NOR to this controller.
 
+config SPI_ALTERA_QUADSPI
+        tristate "Altera Quad SPI Flash Controller II"
+        help
+          Enable support for version 2 of Altera Quad SPI Flash Controller.
+
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 285aab8..024c6ac 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -8,4 +8,6 @@ obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
 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
\ No newline at end of file
+obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
+obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
+
diff --git a/drivers/mtd/spi-nor/altera-quadspi.c b/drivers/mtd/spi-nor/altera-quadspi.c
new file mode 100644
index 0000000..de65453
--- /dev/null
+++ b/drivers/mtd/spi-nor/altera-quadspi.c
@@ -0,0 +1,676 @@
+/*
+ * 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/bitrev.h>
+#include <linux/module.h>
+#include <linux/mtd/altera-quadspi.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+
+#define ALTERA_QUADSPI_RESOURCE_NAME			"altera_quadspi"
+
+#define EPCS_OPCODE_ID					1
+#define NON_EPCS_OPCODE_ID				2
+
+#define WRITE_CHECK					1
+#define ERASE_CHECK					0
+
+#define QUADSPI_SR_REG					0x0
+#define QUADSPI_SR_MASK					0x0000000F
+
+/* defines for device id register */
+#define QUADSPI_SID_REG					0x4
+#define QUADSPI_RDID_REG				0x8
+#define QUADSPI_ID_MASK					0x000000FF
+
+/*
+ * QUADSPI_MEM_OP register offset
+ *
+ * The QUADSPI_MEM_OP register is used to do memory protect and erase operations
+ *
+ */
+#define QUADSPI_MEM_OP_REG				0xC
+
+#define QUADSPI_MEM_OP_CMD_MASK				0x00000003
+#define QUADSPI_MEM_OP_BULK_ERASE_CMD			0x00000001
+#define QUADSPI_MEM_OP_SECTOR_ERASE_CMD			0x00000002
+#define QUADSPI_MEM_OP_SECTOR_PROTECT_CMD		0x00000003
+#define QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD		0x00000004
+#define QUADSPI_MEM_OP_SECTOR_VALUE_MASK		0x0003FF00
+
+#define QUADSPI_MEM_OP_SECTOR_PROTECT_SHIFT		8
+#define QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK	0x00001F00
+/*
+ * QUADSPI_ISR register offset
+ *
+ * The QUADSPI_ISR register is used to determine whether an invalid write or
+ * erase operation trigerred an interrupt
+ *
+ */
+#define QUADSPI_ISR_REG					0x10
+
+#define QUADSPI_ISR_ILLEGAL_ERASE_MASK			0x00000001
+#define QUADSPI_ISR_ILLEGAL_WRITE_MASK			0x00000002
+
+/*
+ * QUADSPI_IMR register offset
+ *
+ * The QUADSPI_IMR register is used to mask the invalid erase or the invalid
+ * write interrupts.
+ *
+ */
+#define QUADSPI_IMR_REG					0x14
+#define QUADSPI_IMR_ILLEGAL_ERASE_MASK			0x00000001
+
+#define QUADSPI_IMR_ILLEGAL_WRITE_MASK			0x00000002
+
+#define QUADSPI_CHIP_SELECT_REG				0x18
+#define QUADSPI_CHIP_SELECT_MASK			0x00000007
+#define QUADSPI_CHIP_SELECT_0				0x00000001
+#define QUADSPI_CHIP_SELECT_1				0x00000002
+#define QUADSPI_CHIP_SELECT_2				0x00000004
+
+#define QUADSPI_FLAG_STATUS_REG				0x1C
+#define QUADSPI_DEV_ID_DATA_0				0x20
+#define QUADSPI_DEV_ID_DATA_1				0x24
+#define QUADSPI_DEV_ID_DATA_2				0x28
+#define QUADSPI_DEV_ID_DATA_3				0x2C
+#define QUADSPI_DEV_ID_DATA_4				0x30
+
+#define QUADSPI_WIN_OCC_REG				0x4
+#define QUADSPI_WIN_OCC_SFT				24
+
+#define QUADSPI_WIN_SEL_REG				0x8
+
+struct altera_quadspi {
+	u32 opcode_id;
+	void __iomem *csr_base;
+	void __iomem *data_base;
+	void __iomem *window_base;
+	size_t window_size;
+	u32 num_flashes;
+	u32 flags;
+	struct device *dev;
+	struct altera_quadspi_flash *flash[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
+	struct device_node *np[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
+};
+
+struct altera_quadspi_flash {
+	struct spi_nor nor;
+	struct altera_quadspi *q;
+	u32 bank;
+};
+
+struct flash_device {
+	char *name;
+	u32 opcode_id;
+	u32 device_id;
+};
+
+#ifdef DEBUG
+static inline u32 alt_qspi_readl(void __iomem *base, off_t offset)
+{
+	u32 val = readl(base + offset);
+
+	pr_info("%s 0x%x from offset 0x%lx\n", __func__, val, offset);
+	return val;
+}
+static inline void alt_qspi_writel(u32 val, void __iomem *base, off_t offset)
+{
+	writel(val, base + offset);
+	pr_info("%s 0x%x to offset 0x%lx\n", __func__, val, offset);
+}
+#else
+#define alt_qspi_readl(base, offset) readl(base+offset)
+#define alt_qspi_writel(val, base, offset) writel(val, base + offset)
+#endif
+
+static void altera_quadspi_chip_select(struct altera_quadspi *q, u32 bank)
+{
+	u32 val = 0;
+
+	switch (bank) {
+	case 0:
+		val = QUADSPI_CHIP_SELECT_0;
+		break;
+	case 1:
+		val = QUADSPI_CHIP_SELECT_1;
+		break;
+	case 2:
+		val = QUADSPI_CHIP_SELECT_2;
+		break;
+	default:
+		dev_err(q->dev, "invalid bank\n");
+		return;
+	}
+	alt_qspi_writel(val, q->csr_base, QUADSPI_CHIP_SELECT_REG);
+}
+
+static int altera_quadspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
+				    int len)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	switch (opcode) {
+	case SPINOR_OP_WREN:
+		dev_dbg(q->dev, "%s enabling write\n", __func__);
+		alt_qspi_writel(QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD,
+				q->csr_base, QUADSPI_MEM_OP_REG);
+		break;
+
+	case SPINOR_OP_CHIP_ERASE:
+		alt_qspi_writel(QUADSPI_MEM_OP_BULK_ERASE_CMD,
+				q->csr_base, QUADSPI_MEM_OP_REG);
+		break;
+
+	default:
+		dev_dbg(q->dev, "%s UNHANDLED write_reg 0x%x\n",
+			__func__, opcode);
+
+	}
+
+	return 0;
+}
+
+static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
+				   int len)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	u32 data = 0;
+
+	memset(val, 0, len);
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	switch (opcode) {
+	case SPINOR_OP_RDSR:
+		data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
+		dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
+		*val = (u8)data & QUADSPI_SR_MASK;
+		break;
+	case SPINOR_OP_RDID:
+		if (q->opcode_id == EPCS_OPCODE_ID)
+			data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
+		else
+			data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
+
+		*((u32 *)val) = data;
+		break;
+	case SPINOR_OP_RDFSR:
+		data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
+		dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
+		*val = (u8)(data & 0xff);
+		break;
+	default:
+		dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
+			__func__, opcode);
+		*val = 0;
+		break;
+	}
+	return 0;
+}
+
+static int altera_quadspi_write_erase_check(struct spi_nor *nor,
+					    bool write_erase)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	u32 val;
+	u32 mask;
+
+	if (write_erase)
+		mask = QUADSPI_ISR_ILLEGAL_WRITE_MASK;
+	else
+		mask = QUADSPI_ISR_ILLEGAL_ERASE_MASK;
+
+	val = alt_qspi_readl(q->csr_base, QUADSPI_ISR_REG);
+
+	if (val & mask) {
+		dev_err(nor->dev,
+			"write/erase failed, sector might be protected\n");
+		alt_qspi_writel(0, q->csr_base, QUADSPI_FLAG_STATUS_REG);
+
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int altera_quadspi_addr_to_sector(struct mtd_info *mtd, uint64_t offset)
+{
+	if (mtd->erasesize_shift)
+		return offset >> mtd->erasesize_shift;
+	do_div(offset, mtd->erasesize);
+	return offset;
+}
+
+static int altera_quadspi_erase(struct spi_nor *nor, loff_t offset)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	struct mtd_info *mtd = &nor->mtd;
+	u32 val;
+	int sector_value;
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	sector_value = altera_quadspi_addr_to_sector(mtd, offset);
+
+	dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
+
+	if (sector_value < 0)
+		return -EINVAL;
+
+	val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
+
+	val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
+
+	alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
+
+	dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
+		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
+		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
+
+	return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
+}
+
+#define WINDOW_ALIGN 4
+#define WINDOW_MASK (WINDOW_ALIGN - 1)
+
+static ssize_t altera_quadspi_windowed_read(struct altera_quadspi *q,
+					    loff_t from,
+					    size_t len, u_char *buf)
+{
+	size_t bytes_left = len;
+	size_t bytes_to_read, i;
+	loff_t next_window_off;
+	u64 start_window;
+	u32 window;
+	u32 *dst;
+
+	if ((from & WINDOW_MASK) || (len & WINDOW_MASK) ||
+	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
+		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
+			__func__);
+		return 0;
+	}
+
+	start_window = from;
+	do_div(start_window, q->window_size);
+	window = (u32)(start_window & 0xffffffff);
+
+	next_window_off = (window + 1) * q->window_size;
+
+	while (bytes_left > 0) {
+
+		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
+
+		bytes_to_read = min((size_t)bytes_left,
+				    (size_t)(next_window_off - from));
+
+		dev_dbg(q->dev,
+			"window%u fr0x%llx next0x%llx left%zu num0x%zx\n",
+			window,  from, next_window_off, bytes_left,
+			bytes_to_read);
+
+		dst = (u32 *)buf;
+		for (i = 0; i < bytes_to_read; i += 4, dst++)
+			*dst = readl(q->data_base +
+				     (from & (q->window_size - 1)) + i);
+
+		bytes_left -= bytes_to_read;
+		buf += bytes_to_read;
+		from += bytes_to_read;
+		window++;
+		next_window_off += q->window_size;
+	}
+
+	return len;
+}
+static ssize_t altera_quadspi_windowed_write(struct altera_quadspi *q,
+					     loff_t to, size_t len,
+					     const u_char *buf)
+{
+	size_t bytes_left = len;
+	u32 window_mask = q->window_size - 1;
+	u32 read_back;
+	size_t bytes_to_write, i;
+	loff_t next_window_off;
+	u64 start_window;
+	u32 window;
+	const u32 *src;
+	u32 words_can_write;
+
+	if ((to & WINDOW_MASK) || (len & WINDOW_MASK) ||
+	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
+		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
+			__func__);
+		return 0;
+	}
+
+	start_window = to;
+	do_div(start_window, q->window_size);
+	window = (u32)(start_window & 0xffffffff);
+
+	next_window_off = (window + 1) * q->window_size;
+
+	while (bytes_left > 0) {
+
+		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
+
+		bytes_to_write = min((size_t)bytes_left,
+				    (size_t)(next_window_off - to));
+
+		dev_dbg(q->dev,
+			"window%u to0x%llx next0x%llx left%zu num0x%zx\n",
+			window,  to, next_window_off, bytes_left,
+			bytes_to_write);
+
+		src = (u32 *)buf;
+		for (i = 0; i < bytes_to_write;) {
+			words_can_write =
+				readl(q->window_base + QUADSPI_WIN_OCC_REG) >>
+				      QUADSPI_WIN_OCC_SFT;
+			dev_dbg(q->dev, "can write 0x%x\n", words_can_write);
+
+			for (; words_can_write > 0; words_can_write--) {
+				writel(*src,
+				       q->data_base +
+				       (to & window_mask) + i);
+				read_back = readl(q->data_base +
+						  (to & window_mask) + i);
+				if (*src != read_back) {
+					dev_err(q->dev, "%s 0x%x != 0x%x\n",
+						__func__, *src, read_back);
+					return (len - bytes_left);
+				}
+				i += 4;
+				src++;
+			}
+		}
+
+		bytes_left -= bytes_to_write;
+		buf += bytes_to_write;
+		to += bytes_to_write;
+		window++;
+		next_window_off += q->window_size;
+	}
+
+	return len;
+}
+
+static ssize_t altera_quadspi_read(struct spi_nor *nor, loff_t from, size_t len,
+				   u_char *buf)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	size_t i;
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	if (q->window_size)
+		altera_quadspi_windowed_read(q, from, len, buf);
+	else
+		memcpy_fromio(buf, q->data_base + from, len);
+
+	if (q->flags & ALTERA_QUADSPI_FL_BITREV_READ) {
+		for (i = 0; i < len; i++, buf++)
+			*buf = bitrev8(*buf);
+	}
+
+	return len;
+}
+
+static ssize_t altera_quadspi_write(struct spi_nor *nor, loff_t to,
+				    size_t len, const u_char *buf)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	u_char *bitrev_buf = NULL;
+	const u_char *src;
+	u_char *dst;
+	size_t i;
+	int ret = 0;
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	if (q->flags & ALTERA_QUADSPI_FL_BITREV_WRITE) {
+		bitrev_buf = devm_kzalloc(q->dev, len, GFP_KERNEL);
+		if (!bitrev_buf)
+			return 0;
+
+		src = buf;
+		dst = bitrev_buf;
+		for (i = 0; i < len; i++, src++, dst++)
+			*dst = bitrev8(*src);
+
+		buf = bitrev_buf;
+	}
+
+	if (q->window_size)
+		altera_quadspi_windowed_write(q, to, len, buf);
+	else
+		memcpy_toio(q->data_base + to, buf, len);
+
+
+	if (bitrev_buf)
+		devm_kfree(q->dev, bitrev_buf);
+
+	ret = altera_quadspi_write_erase_check(nor, WRITE_CHECK);
+
+	return len;
+
+}
+
+static int altera_quadspi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	struct mtd_info *mtd = &nor->mtd;
+	uint32_t offset = ofs;
+	u32 sector_start, sector_end;
+	uint64_t num_sectors;
+	u32 mem_op;
+	u32 sr_bp;
+	u32 sr_tb;
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	sector_start = offset;
+	sector_end = altera_quadspi_addr_to_sector(mtd, offset + len);
+	num_sectors = mtd->size;
+	do_div(num_sectors, mtd->erasesize);
+
+	dev_dbg(nor->dev, "%s: sector start is %u,sector end is %u\n",
+		__func__, sector_start, sector_end);
+
+	if (sector_start >= num_sectors / 2) {
+		sr_bp = fls(num_sectors - 1 - sector_start) + 1;
+		sr_tb = 0;
+	} else if ((sector_end < num_sectors / 2) &&
+		  (q->opcode_id != EPCS_OPCODE_ID)) {
+		sr_bp = fls(sector_end) + 1;
+		sr_tb = 1;
+	} else {
+		sr_bp = 16;
+		sr_tb = 0;
+	}
+
+	mem_op = (sr_tb << 12) | (sr_bp << 8);
+	mem_op &= QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK;
+	mem_op |= QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
+
+	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
+
+	return 0;
+}
+
+static int altera_quadspi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	struct altera_quadspi_flash *flash = nor->priv;
+	struct altera_quadspi *q = flash->q;
+	u32 mem_op;
+
+	dev_dbg(nor->dev, "Unlock all protected area\n");
+
+	altera_quadspi_chip_select(q, flash->bank);
+
+	mem_op = QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
+	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
+
+	return 0;
+}
+
+static int altera_quadspi_setup_banks(struct device *dev,
+				      u32 bank, struct device_node *np)
+{
+	struct altera_quadspi *q = dev_get_drvdata(dev);
+	struct altera_quadspi_flash *flash;
+	struct spi_nor *nor;
+	int ret = 0;
+	char modalias[40] = {0};
+	struct spi_nor_hwcaps hwcaps = {
+		.mask = SNOR_HWCAPS_READ |
+			SNOR_HWCAPS_READ_FAST |
+			SNOR_HWCAPS_READ_1_1_2 |
+			SNOR_HWCAPS_READ_1_1_4 |
+			SNOR_HWCAPS_PP,
+	};
+
+	if (bank > q->num_flashes - 1)
+		return -EINVAL;
+
+	altera_quadspi_chip_select(q, bank);
+
+	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
+	if (!flash)
+		return -ENOMEM;
+
+	q->flash[bank] = flash;
+	nor = &flash->nor;
+	nor->dev = dev;
+	nor->priv = flash;
+	nor->mtd.priv = nor;
+	flash->q = q;
+	flash->bank = bank;
+	spi_nor_set_flash_node(nor, np);
+
+	/* spi nor framework*/
+	nor->read_reg = altera_quadspi_read_reg;
+	nor->write_reg = altera_quadspi_write_reg;
+	nor->read = altera_quadspi_read;
+	nor->write = altera_quadspi_write;
+	nor->erase = altera_quadspi_erase;
+	nor->flash_lock = altera_quadspi_lock;
+	nor->flash_unlock = altera_quadspi_unlock;
+
+	/* scanning flash and checking dev id */
+#ifdef CONFIG_OF
+	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
+		return -EINVAL;
+#endif
+
+	ret = spi_nor_scan(nor, modalias, &hwcaps);
+	if (ret) {
+		dev_err(nor->dev, "flash not found\n");
+		return ret;
+	}
+
+	ret =  mtd_device_register(&nor->mtd, NULL, 0);
+
+	altera_quadspi_unlock(nor, 0, 0);
+
+	return ret;
+}
+
+int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
+			  void __iomem *data_base, void __iomem *window_base,
+			  size_t window_size, u32 flags)
+{
+	struct altera_quadspi *q;
+
+	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+
+	q->dev = dev;
+	q->csr_base = csr_base;
+	q->data_base = data_base;
+	q->window_base = window_base;
+	q->window_size = window_size;
+
+	q->flags = flags;
+
+	dev_set_drvdata(dev, q);
+
+	dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
+		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
+		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(altera_quadspi_create);
+
+int altera_qspi_add_bank(struct device *dev,
+			 u32 bank, struct device_node *np)
+{
+	struct altera_quadspi *q = dev_get_drvdata(dev);
+
+	if (q->num_flashes >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP)
+		return -ENOMEM;
+
+	q->num_flashes++;
+
+	return altera_quadspi_setup_banks(dev, bank, np);
+}
+EXPORT_SYMBOL_GPL(altera_qspi_add_bank);
+
+int altera_quadspi_remove_banks(struct device *dev)
+{
+	struct altera_quadspi *q = dev_get_drvdata(dev);
+	struct altera_quadspi_flash *flash;
+	int i;
+	int ret = 0;
+
+	/* clean up for all nor flash */
+	for (i = 0; i < q->num_flashes; i++) {
+		flash = q->flash[i];
+		if (!flash)
+			continue;
+
+		/* clean up mtd stuff */
+		ret = mtd_device_unregister(&flash->nor.mtd);
+		if (ret) {
+			dev_err(dev, "error removing mtd\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(altera_quadspi_remove_banks);
+
+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 Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mtd/altera-quadspi.h b/include/linux/mtd/altera-quadspi.h
new file mode 100644
index 0000000..58f31ee
--- /dev/null
+++ b/include/linux/mtd/altera-quadspi.h
@@ -0,0 +1,28 @@
+/*
+ *
+ * Copyright 2017 Intel Corporation, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __ALTERA_QUADSPI_H
+#define __ALTERA_QUADSPI_H
+
+#include <linux/device.h>
+
+#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
+#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
+
+#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
+
+int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
+			  void __iomem *data_base, void __iomem *window_reg,
+			  size_t window_size, u32 flags);
+
+int altera_qspi_add_bank(struct device *dev,
+			 u32 bank, struct device_node *np);
+
+int altera_quadspi_remove_banks(struct device *dev);
+#endif
-- 
2.7.4

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]

Hi Matthew,

[auto build test WARNING on spi-nor/next]
[also build test WARNING on v4.12-rc7 next-20170627]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/matthew-gerlach-linux-intel-com/Altera-Quadspi-Controller-Version-2/20170627-120604
base:   git://github.com/spi-nor/linux next
config: m32r-allmodconfig (attached as .config)
compiler: m32r-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m32r 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:13,
                    from include/linux/list.h:8,
                    from include/linux/module.h:9,
                    from drivers/mtd/spi-nor/altera-quadspi.c:20:
   drivers/mtd/spi-nor/altera-quadspi.c: In function 'altera_quadspi_erase':
>> drivers/mtd/spi-nor/altera-quadspi.c:289:18: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'long unsigned int' [-Wformat=]
     dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
                     ^
   include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
>> drivers/mtd/spi-nor/altera-quadspi.c:289:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
     ^~~~~~~
   drivers/mtd/spi-nor/altera-quadspi.c:289:18: warning: format '%x' expects argument of type 'unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=]
     dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
                     ^
   include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
>> drivers/mtd/spi-nor/altera-quadspi.c:289:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
     ^~~~~~~
   drivers/mtd/spi-nor/altera-quadspi.c: In function 'altera_quadspi_create':
   drivers/mtd/spi-nor/altera-quadspi.c:625:15: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'long unsigned int' [-Wformat=]
     dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
                  ^
   include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
   drivers/mtd/spi-nor/altera-quadspi.c:625:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
     ^~~~~~~
   drivers/mtd/spi-nor/altera-quadspi.c:625:15: warning: format '%x' expects argument of type 'unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=]
     dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
                  ^
   include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
   drivers/mtd/spi-nor/altera-quadspi.c:625:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
     ^~~~~~~

vim +289 drivers/mtd/spi-nor/altera-quadspi.c

   273	
   274		altera_quadspi_chip_select(q, flash->bank);
   275	
   276		sector_value = altera_quadspi_addr_to_sector(mtd, offset);
   277	
   278		dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
   279	
   280		if (sector_value < 0)
   281			return -EINVAL;
   282	
   283		val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
   284	
   285		val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
   286	
   287		alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
   288	
 > 289		dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
   290			alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
   291			alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
   292	
   293		return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
   294	}
   295	
   296	#define WINDOW_ALIGN 4
   297	#define WINDOW_MASK (WINDOW_ALIGN - 1)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42462 bytes --]

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[Marek Vasut]

On 06/26/2017 06:13 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  MAINTAINERS                          |   7 +
>  drivers/mtd/spi-nor/Kconfig          |   5 +
>  drivers/mtd/spi-nor/Makefile         |   4 +-
>  drivers/mtd/spi-nor/altera-quadspi.c | 676 +++++++++++++++++++++++++++++++++++
>  include/linux/mtd/altera-quadspi.h   |  28 ++
>  5 files changed, 719 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi.c
>  create mode 100644 include/linux/mtd/altera-quadspi.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b4395c..ae33fa6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -648,6 +648,13 @@ L:	linux-gpio@vger.kernel.org
>  S:	Maintained
>  F:	drivers/gpio/gpio-altera.c
>  
> +ALTERA QUADSPI FLASH DRIVER
> +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:	inclulde/linux/mtd/altera-quadspi.h
> +
>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
>  M:	Thor Thayer <thor.thayer@linux.intel.com>
>  S:	Maintained
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 293c8a4..89fe425 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -113,4 +113,9 @@ config SPI_STM32_QUADSPI
>  	  This enables support for the STM32 Quad SPI controller.
>  	  We only connect the NOR to this controller.
>  
> +config SPI_ALTERA_QUADSPI
> +        tristate "Altera Quad SPI Flash Controller II"
> +        help
> +          Enable support for version 2 of Altera Quad SPI Flash Controller.

Some more description would be useful.

>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 285aab8..024c6ac 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -8,4 +8,6 @@ obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
>  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
> \ No newline at end of file
> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
> +obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o

Keep the list sorted

> diff --git a/drivers/mtd/spi-nor/altera-quadspi.c b/drivers/mtd/spi-nor/altera-quadspi.c
> new file mode 100644
> index 0000000..de65453
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/altera-quadspi.c
> @@ -0,0 +1,676 @@
> +/*
> + * 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/>.
> + */
> +
> +

One newline too many

> +#include <linux/bitrev.h>
> +#include <linux/module.h>
> +#include <linux/mtd/altera-quadspi.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spi-nor.h>
> +
> +#define ALTERA_QUADSPI_RESOURCE_NAME			"altera_quadspi"
> +
> +#define EPCS_OPCODE_ID					1
> +#define NON_EPCS_OPCODE_ID				2
> +
> +#define WRITE_CHECK					1
> +#define ERASE_CHECK					0
> +
> +#define QUADSPI_SR_REG					0x0
> +#define QUADSPI_SR_MASK					0x0000000F
> +
> +/* defines for device id register */
> +#define QUADSPI_SID_REG					0x4
> +#define QUADSPI_RDID_REG				0x8
> +#define QUADSPI_ID_MASK					0x000000FF
> +
> +/*
> + * QUADSPI_MEM_OP register offset
> + *
> + * The QUADSPI_MEM_OP register is used to do memory protect and erase operations
> + *
> + */
> +#define QUADSPI_MEM_OP_REG				0xC
> +
> +#define QUADSPI_MEM_OP_CMD_MASK				0x00000003
> +#define QUADSPI_MEM_OP_BULK_ERASE_CMD			0x00000001
> +#define QUADSPI_MEM_OP_SECTOR_ERASE_CMD			0x00000002
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_CMD		0x00000003
> +#define QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD		0x00000004
> +#define QUADSPI_MEM_OP_SECTOR_VALUE_MASK		0x0003FF00
> +
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_SHIFT		8
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK	0x00001F00
> +/*
> + * QUADSPI_ISR register offset
> + *
> + * The QUADSPI_ISR register is used to determine whether an invalid write or
> + * erase operation trigerred an interrupt
> + *
> + */
> +#define QUADSPI_ISR_REG					0x10
> +
> +#define QUADSPI_ISR_ILLEGAL_ERASE_MASK			0x00000001
> +#define QUADSPI_ISR_ILLEGAL_WRITE_MASK			0x00000002
> +
> +/*
> + * QUADSPI_IMR register offset
> + *
> + * The QUADSPI_IMR register is used to mask the invalid erase or the invalid
> + * write interrupts.
> + *
> + */
> +#define QUADSPI_IMR_REG					0x14
> +#define QUADSPI_IMR_ILLEGAL_ERASE_MASK			0x00000001
> +
> +#define QUADSPI_IMR_ILLEGAL_WRITE_MASK			0x00000002
> +
> +#define QUADSPI_CHIP_SELECT_REG				0x18
> +#define QUADSPI_CHIP_SELECT_MASK			0x00000007
> +#define QUADSPI_CHIP_SELECT_0				0x00000001
> +#define QUADSPI_CHIP_SELECT_1				0x00000002
> +#define QUADSPI_CHIP_SELECT_2				0x00000004

BIT() ?

> +#define QUADSPI_FLAG_STATUS_REG				0x1C
> +#define QUADSPI_DEV_ID_DATA_0				0x20
> +#define QUADSPI_DEV_ID_DATA_1				0x24
> +#define QUADSPI_DEV_ID_DATA_2				0x28
> +#define QUADSPI_DEV_ID_DATA_3				0x2C
> +#define QUADSPI_DEV_ID_DATA_4				0x30
> +
> +#define QUADSPI_WIN_OCC_REG				0x4
> +#define QUADSPI_WIN_OCC_SFT				24
> +
> +#define QUADSPI_WIN_SEL_REG				0x8
> +
> +struct altera_quadspi {
> +	u32 opcode_id;
> +	void __iomem *csr_base;
> +	void __iomem *data_base;
> +	void __iomem *window_base;
> +	size_t window_size;
> +	u32 num_flashes;
> +	u32 flags;
> +	struct device *dev;
> +	struct altera_quadspi_flash *flash[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
> +	struct device_node *np[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];

Is this np even necessary ? Or why don't you put it into
altera_quadspi_flash {} ?

> +};
> +
> +struct altera_quadspi_flash {
> +	struct spi_nor nor;
> +	struct altera_quadspi *q;
> +	u32 bank;
> +};
> +
> +struct flash_device {
> +	char *name;
> +	u32 opcode_id;
> +	u32 device_id;
> +};
> +
> +#ifdef DEBUG

Yet another magic NIH readl/writel debug ... sigh, drop it.

> +static inline u32 alt_qspi_readl(void __iomem *base, off_t offset)
> +{
> +	u32 val = readl(base + offset);
> +
> +	pr_info("%s 0x%x from offset 0x%lx\n", __func__, val, offset);
> +	return val;
> +}
> +static inline void alt_qspi_writel(u32 val, void __iomem *base, off_t offset)
> +{
> +	writel(val, base + offset);
> +	pr_info("%s 0x%x to offset 0x%lx\n", __func__, val, offset);
> +}
> +#else
> +#define alt_qspi_readl(base, offset) readl(base+offset)
> +#define alt_qspi_writel(val, base, offset) writel(val, base + offset)

This use of macros is wrong, but it'll be dropped anyway ...

> +#endif
> +
> +static void altera_quadspi_chip_select(struct altera_quadspi *q, u32 bank)
> +{
> +	u32 val = 0;
> +
> +	switch (bank) {
> +	case 0:
> +		val = QUADSPI_CHIP_SELECT_0;
> +		break;
> +	case 1:
> +		val = QUADSPI_CHIP_SELECT_1;
> +		break;
> +	case 2:
> +		val = QUADSPI_CHIP_SELECT_2;
> +		break;
> +	default:
> +		dev_err(q->dev, "invalid bank\n");
> +		return;
> +	}

So this is basically
switch ...
case 0:
 return BIT(0);
case 1:
 return BIT(1);
 ...

This can be simplified, right ? :)

> +	alt_qspi_writel(val, q->csr_base, QUADSPI_CHIP_SELECT_REG);
> +}
> +
> +static int altera_quadspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
> +				    int len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +
> +	altera_quadspi_chip_select(q, flash->bank);

Look at the mutex usage in cadence qspi driver ; if you have multiple
flashes and you access both at the same time, you'll run into a nasty
race if you don't have that locking in place. UBI on both can trigger
that. Look for "bus_mutex" in that driver.

> +	switch (opcode) {
> +	case SPINOR_OP_WREN:
> +		dev_dbg(q->dev, "%s enabling write\n", __func__);
> +		alt_qspi_writel(QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD,
> +				q->csr_base, QUADSPI_MEM_OP_REG);
> +		break;
> +
> +	case SPINOR_OP_CHIP_ERASE:
> +		alt_qspi_writel(QUADSPI_MEM_OP_BULK_ERASE_CMD,
> +				q->csr_base, QUADSPI_MEM_OP_REG);
> +		break;
> +
> +	default:
> +		dev_dbg(q->dev, "%s UNHANDLED write_reg 0x%x\n",
> +			__func__, opcode);
> +
> +	}
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
> +				   int len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 data = 0;
> +
> +	memset(val, 0, len);
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	switch (opcode) {
> +	case SPINOR_OP_RDSR:
> +		data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
> +		dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
> +		*val = (u8)data & QUADSPI_SR_MASK;
> +		break;
> +	case SPINOR_OP_RDID:
> +		if (q->opcode_id == EPCS_OPCODE_ID)
> +			data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
> +		else
> +			data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
> +
> +		*((u32 *)val) = data;

What are these awful casts ?

> +		break;
> +	case SPINOR_OP_RDFSR:
> +		data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
> +		dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
> +		*val = (u8)(data & 0xff);
> +		break;
> +	default:
> +		dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
> +			__func__, opcode);
> +		*val = 0;
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int altera_quadspi_write_erase_check(struct spi_nor *nor,
> +					    bool write_erase)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 val;
> +	u32 mask;
> +
> +	if (write_erase)
> +		mask = QUADSPI_ISR_ILLEGAL_WRITE_MASK;
> +	else
> +		mask = QUADSPI_ISR_ILLEGAL_ERASE_MASK;
> +
> +	val = alt_qspi_readl(q->csr_base, QUADSPI_ISR_REG);
> +
> +	if (val & mask) {
> +		dev_err(nor->dev,
> +			"write/erase failed, sector might be protected\n");
> +		alt_qspi_writel(0, q->csr_base, QUADSPI_FLAG_STATUS_REG);
> +
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_addr_to_sector(struct mtd_info *mtd, uint64_t offset)
> +{
> +	if (mtd->erasesize_shift)
> +		return offset >> mtd->erasesize_shift;
> +	do_div(offset, mtd->erasesize);
> +	return offset;
> +}
> +
> +static int altera_quadspi_erase(struct spi_nor *nor, loff_t offset)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	struct mtd_info *mtd = &nor->mtd;
> +	u32 val;
> +	int sector_value;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	sector_value = altera_quadspi_addr_to_sector(mtd, offset);
> +
> +	dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
> +
> +	if (sector_value < 0)
> +		return -EINVAL;
> +
> +	val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
> +
> +	val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
> +
> +	alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
> +
> +	return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
> +}
> +
> +#define WINDOW_ALIGN 4
> +#define WINDOW_MASK (WINDOW_ALIGN - 1)

What are these undocumented macros in the middle of the code ?

> +static ssize_t altera_quadspi_windowed_read(struct altera_quadspi *q,
> +					    loff_t from,
> +					    size_t len, u_char *buf)
> +{
> +	size_t bytes_left = len;
> +	size_t bytes_to_read, i;
> +	loff_t next_window_off;
> +	u64 start_window;
> +	u32 window;
> +	u32 *dst;
> +
> +	if ((from & WINDOW_MASK) || (len & WINDOW_MASK) ||
> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	start_window = from;
> +	do_div(start_window, q->window_size);
> +	window = (u32)(start_window & 0xffffffff);
> +
> +	next_window_off = (window + 1) * q->window_size;
> +
> +	while (bytes_left > 0) {
> +
> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
> +
> +		bytes_to_read = min((size_t)bytes_left,
> +				    (size_t)(next_window_off - from));
> +
> +		dev_dbg(q->dev,
> +			"window%u fr0x%llx next0x%llx left%zu num0x%zx\n",
> +			window,  from, next_window_off, bytes_left,
> +			bytes_to_read);
> +
> +		dst = (u32 *)buf;
> +		for (i = 0; i < bytes_to_read; i += 4, dst++)
> +			*dst = readl(q->data_base +
> +				     (from & (q->window_size - 1)) + i);

ioread32_rep and co. ? Again, look at the CQSPI driver.

> +		bytes_left -= bytes_to_read;
> +		buf += bytes_to_read;
> +		from += bytes_to_read;
> +		window++;
> +		next_window_off += q->window_size;
> +	}
> +
> +	return len;
> +}
> +static ssize_t altera_quadspi_windowed_write(struct altera_quadspi *q,
> +					     loff_t to, size_t len,
> +					     const u_char *buf)
> +{
> +	size_t bytes_left = len;
> +	u32 window_mask = q->window_size - 1;
> +	u32 read_back;
> +	size_t bytes_to_write, i;
> +	loff_t next_window_off;
> +	u64 start_window;
> +	u32 window;
> +	const u32 *src;
> +	u32 words_can_write;
> +
> +	if ((to & WINDOW_MASK) || (len & WINDOW_MASK) ||
> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	start_window = to;
> +	do_div(start_window, q->window_size);
> +	window = (u32)(start_window & 0xffffffff);
> +
> +	next_window_off = (window + 1) * q->window_size;
> +
> +	while (bytes_left > 0) {
> +
> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
> +
> +		bytes_to_write = min((size_t)bytes_left,
> +				    (size_t)(next_window_off - to));
> +
> +		dev_dbg(q->dev,
> +			"window%u to0x%llx next0x%llx left%zu num0x%zx\n",
> +			window,  to, next_window_off, bytes_left,
> +			bytes_to_write);

DTTO , either memcpy_toio or iowrite32_rep

> +		src = (u32 *)buf;
> +		for (i = 0; i < bytes_to_write;) {
> +			words_can_write =
> +				readl(q->window_base + QUADSPI_WIN_OCC_REG) >>
> +				      QUADSPI_WIN_OCC_SFT;
> +			dev_dbg(q->dev, "can write 0x%x\n", words_can_write);
> +
> +			for (; words_can_write > 0; words_can_write--) {
> +				writel(*src,
> +				       q->data_base +
> +				       (to & window_mask) + i);
> +				read_back = readl(q->data_base +
> +						  (to & window_mask) + i);
> +				if (*src != read_back) {
> +					dev_err(q->dev, "%s 0x%x != 0x%x\n",
> +						__func__, *src, read_back);
> +					return (len - bytes_left);
> +				}
> +				i += 4;
> +				src++;
> +			}
> +		}
> +
> +		bytes_left -= bytes_to_write;
> +		buf += bytes_to_write;
> +		to += bytes_to_write;
> +		window++;
> +		next_window_off += q->window_size;
> +	}
> +
> +	return len;
> +}
> +
> +static ssize_t altera_quadspi_read(struct spi_nor *nor, loff_t from, size_t len,
> +				   u_char *buf)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	size_t i;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	if (q->window_size)
> +		altera_quadspi_windowed_read(q, from, len, buf);
> +	else
> +		memcpy_fromio(buf, q->data_base + from, len);
> +
> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_READ) {
> +		for (i = 0; i < len; i++, buf++)
> +			*buf = bitrev8(*buf);
> +	}
> +
> +	return len;
> +}
> +
> +static ssize_t altera_quadspi_write(struct spi_nor *nor, loff_t to,
> +				    size_t len, const u_char *buf)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u_char *bitrev_buf = NULL;
> +	const u_char *src;
> +	u_char *dst;
> +	size_t i;
> +	int ret = 0;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_WRITE) {
> +		bitrev_buf = devm_kzalloc(q->dev, len, GFP_KERNEL);
> +		if (!bitrev_buf)
> +			return 0;
> +
> +		src = buf;
> +		dst = bitrev_buf;
> +		for (i = 0; i < len; i++, src++, dst++)
> +			*dst = bitrev8(*src);
> +
> +		buf = bitrev_buf;
> +	}
> +
> +	if (q->window_size)
> +		altera_quadspi_windowed_write(q, to, len, buf);
> +	else
> +		memcpy_toio(q->data_base + to, buf, len);
> +
> +
> +	if (bitrev_buf)
> +		devm_kfree(q->dev, bitrev_buf);
> +
> +	ret = altera_quadspi_write_erase_check(nor, WRITE_CHECK);
> +
> +	return len;
> +
> +}
> +
> +static int altera_quadspi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	struct mtd_info *mtd = &nor->mtd;
> +	uint32_t offset = ofs;
> +	u32 sector_start, sector_end;
> +	uint64_t num_sectors;
> +	u32 mem_op;
> +	u32 sr_bp;
> +	u32 sr_tb;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	sector_start = offset;
> +	sector_end = altera_quadspi_addr_to_sector(mtd, offset + len);
> +	num_sectors = mtd->size;
> +	do_div(num_sectors, mtd->erasesize);
> +
> +	dev_dbg(nor->dev, "%s: sector start is %u,sector end is %u\n",
> +		__func__, sector_start, sector_end);
> +
> +	if (sector_start >= num_sectors / 2) {
> +		sr_bp = fls(num_sectors - 1 - sector_start) + 1;
> +		sr_tb = 0;
> +	} else if ((sector_end < num_sectors / 2) &&
> +		  (q->opcode_id != EPCS_OPCODE_ID)) {
> +		sr_bp = fls(sector_end) + 1;
> +		sr_tb = 1;
> +	} else {
> +		sr_bp = 16;
> +		sr_tb = 0;
> +	}
> +
> +	mem_op = (sr_tb << 12) | (sr_bp << 8);
> +	mem_op &= QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK;
> +	mem_op |= QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
> +
> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 mem_op;
> +
> +	dev_dbg(nor->dev, "Unlock all protected area\n");
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	mem_op = QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_setup_banks(struct device *dev,
> +				      u32 bank, struct device_node *np)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +	struct altera_quadspi_flash *flash;
> +	struct spi_nor *nor;
> +	int ret = 0;
> +	char modalias[40] = {0};
> +	struct spi_nor_hwcaps hwcaps = {
> +		.mask = SNOR_HWCAPS_READ |
> +			SNOR_HWCAPS_READ_FAST |
> +			SNOR_HWCAPS_READ_1_1_2 |
> +			SNOR_HWCAPS_READ_1_1_4 |
> +			SNOR_HWCAPS_PP,
> +	};
> +
> +	if (bank > q->num_flashes - 1)
> +		return -EINVAL;
> +
> +	altera_quadspi_chip_select(q, bank);
> +
> +	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
> +	if (!flash)
> +		return -ENOMEM;
> +
> +	q->flash[bank] = flash;
> +	nor = &flash->nor;
> +	nor->dev = dev;
> +	nor->priv = flash;
> +	nor->mtd.priv = nor;
> +	flash->q = q;
> +	flash->bank = bank;
> +	spi_nor_set_flash_node(nor, np);
> +
> +	/* spi nor framework*/
> +	nor->read_reg = altera_quadspi_read_reg;
> +	nor->write_reg = altera_quadspi_write_reg;
> +	nor->read = altera_quadspi_read;
> +	nor->write = altera_quadspi_write;
> +	nor->erase = altera_quadspi_erase;
> +	nor->flash_lock = altera_quadspi_lock;
> +	nor->flash_unlock = altera_quadspi_unlock;
> +
> +	/* scanning flash and checking dev id */
> +#ifdef CONFIG_OF
> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
> +		return -EINVAL;
> +#endif
> +
> +	ret = spi_nor_scan(nor, modalias, &hwcaps);
> +	if (ret) {
> +		dev_err(nor->dev, "flash not found\n");
> +		return ret;
> +	}
> +
> +	ret =  mtd_device_register(&nor->mtd, NULL, 0);
> +
> +	altera_quadspi_unlock(nor, 0, 0);
> +
> +	return ret;
> +}
> +
> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
> +			  void __iomem *data_base, void __iomem *window_base,
> +			  size_t window_size, u32 flags)
> +{
> +	struct altera_quadspi *q;
> +
> +	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
> +	if (!q)
> +		return -ENOMEM;
> +
> +	q->dev = dev;
> +	q->csr_base = csr_base;
> +	q->data_base = data_base;
> +	q->window_base = window_base;
> +	q->window_size = window_size;
> +
> +	q->flags = flags;
> +
> +	dev_set_drvdata(dev, q);
> +
> +	dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(altera_quadspi_create);
> +
> +int altera_qspi_add_bank(struct device *dev,
> +			 u32 bank, struct device_node *np)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +
> +	if (q->num_flashes >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP)
> +		return -ENOMEM;
> +
> +	q->num_flashes++;
> +
> +	return altera_quadspi_setup_banks(dev, bank, np);
> +}
> +EXPORT_SYMBOL_GPL(altera_qspi_add_bank);
> +
> +int altera_quadspi_remove_banks(struct device *dev)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +	struct altera_quadspi_flash *flash;
> +	int i;
> +	int ret = 0;
> +
> +	/* clean up for all nor flash */
> +	for (i = 0; i < q->num_flashes; i++) {
> +		flash = q->flash[i];
> +		if (!flash)
> +			continue;
> +
> +		/* clean up mtd stuff */
> +		ret = mtd_device_unregister(&flash->nor.mtd);
> +		if (ret) {
> +			dev_err(dev, "error removing mtd\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(altera_quadspi_remove_banks);
> +
> +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 Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mtd/altera-quadspi.h b/include/linux/mtd/altera-quadspi.h
> new file mode 100644
> index 0000000..58f31ee
> --- /dev/null
> +++ b/include/linux/mtd/altera-quadspi.h
> @@ -0,0 +1,28 @@
> +/*
> + *
> + * Copyright 2017 Intel Corporation, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef __ALTERA_QUADSPI_H
> +#define __ALTERA_QUADSPI_H
> +
> +#include <linux/device.h>
> +
> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
> +
> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
> +
> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
> +			  void __iomem *data_base, void __iomem *window_reg,
> +			  size_t window_size, u32 flags);
> +
> +int altera_qspi_add_bank(struct device *dev,
> +			 u32 bank, struct device_node *np);
> +
> +int altera_quadspi_remove_banks(struct device *dev);

Why is this header needed at all ?

> +#endif
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[matthew.gerlach]

On Tue, 27 Jun 2017, Marek Vasut wrote:

Hi Marek,

Thanks again for the valuable 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>
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  MAINTAINERS                          |   7 +
>>  drivers/mtd/spi-nor/Kconfig          |   5 +
>>  drivers/mtd/spi-nor/Makefile         |   4 +-
>>  drivers/mtd/spi-nor/altera-quadspi.c | 676 +++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/altera-quadspi.h   |  28 ++
>>  5 files changed, 719 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi.c
>>  create mode 100644 include/linux/mtd/altera-quadspi.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6b4395c..ae33fa6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -648,6 +648,13 @@ L:	linux-gpio@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/gpio/gpio-altera.c
>>
>> +ALTERA QUADSPI FLASH DRIVER
>> +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:	inclulde/linux/mtd/altera-quadspi.h
>> +
>>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
>>  M:	Thor Thayer <thor.thayer@linux.intel.com>
>>  S:	Maintained
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 293c8a4..89fe425 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -113,4 +113,9 @@ config SPI_STM32_QUADSPI
>>  	  This enables support for the STM32 Quad SPI controller.
>>  	  We only connect the NOR to this controller.
>>
>> +config SPI_ALTERA_QUADSPI
>> +        tristate "Altera Quad SPI Flash Controller II"
>> +        help
>> +          Enable support for version 2 of Altera Quad SPI Flash Controller.
>
> Some more description would be useful.
>
>>  endif # MTD_SPI_NOR
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 285aab8..024c6ac 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -8,4 +8,6 @@ obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
>>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
>>  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
>> \ No newline at end of file
>> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>> +obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
>
> Keep the list sorted

Noted.

>
>> diff --git a/drivers/mtd/spi-nor/altera-quadspi.c b/drivers/mtd/spi-nor/altera-quadspi.c
>> new file mode 100644
>> index 0000000..de65453
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/altera-quadspi.c
>> @@ -0,0 +1,676 @@
>> +/*
>> + * 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/>.
>> + */
>> +
>> +
>
> One newline too many

Got it.  I suppose this is probably something too hard for checkpatch.pl 
to catch.

>
>> +#include <linux/bitrev.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/altera-quadspi.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/spi-nor.h>
>> +
>> +#define ALTERA_QUADSPI_RESOURCE_NAME			"altera_quadspi"
>> +
>> +#define EPCS_OPCODE_ID					1
>> +#define NON_EPCS_OPCODE_ID				2
>> +
>> +#define WRITE_CHECK					1
>> +#define ERASE_CHECK					0
>> +
>> +#define QUADSPI_SR_REG					0x0
>> +#define QUADSPI_SR_MASK					0x0000000F
>> +
>> +/* defines for device id register */
>> +#define QUADSPI_SID_REG					0x4
>> +#define QUADSPI_RDID_REG				0x8
>> +#define QUADSPI_ID_MASK					0x000000FF
>> +
>> +/*
>> + * QUADSPI_MEM_OP register offset
>> + *
>> + * The QUADSPI_MEM_OP register is used to do memory protect and erase operations
>> + *
>> + */
>> +#define QUADSPI_MEM_OP_REG				0xC
>> +
>> +#define QUADSPI_MEM_OP_CMD_MASK				0x00000003
>> +#define QUADSPI_MEM_OP_BULK_ERASE_CMD			0x00000001
>> +#define QUADSPI_MEM_OP_SECTOR_ERASE_CMD			0x00000002
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_CMD		0x00000003
>> +#define QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD		0x00000004
>> +#define QUADSPI_MEM_OP_SECTOR_VALUE_MASK		0x0003FF00
>> +
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_SHIFT		8
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK	0x00001F00
>> +/*
>> + * QUADSPI_ISR register offset
>> + *
>> + * The QUADSPI_ISR register is used to determine whether an invalid write or
>> + * erase operation trigerred an interrupt
>> + *
>> + */
>> +#define QUADSPI_ISR_REG					0x10
>> +
>> +#define QUADSPI_ISR_ILLEGAL_ERASE_MASK			0x00000001
>> +#define QUADSPI_ISR_ILLEGAL_WRITE_MASK			0x00000002
>> +
>> +/*
>> + * QUADSPI_IMR register offset
>> + *
>> + * The QUADSPI_IMR register is used to mask the invalid erase or the invalid
>> + * write interrupts.
>> + *
>> + */
>> +#define QUADSPI_IMR_REG					0x14
>> +#define QUADSPI_IMR_ILLEGAL_ERASE_MASK			0x00000001
>> +
>> +#define QUADSPI_IMR_ILLEGAL_WRITE_MASK			0x00000002
>> +
>> +#define QUADSPI_CHIP_SELECT_REG				0x18
>> +#define QUADSPI_CHIP_SELECT_MASK			0x00000007
>> +#define QUADSPI_CHIP_SELECT_0				0x00000001
>> +#define QUADSPI_CHIP_SELECT_1				0x00000002
>> +#define QUADSPI_CHIP_SELECT_2				0x00000004
>
> BIT() ?

These will probably be removed based on comments below.

>
>> +#define QUADSPI_FLAG_STATUS_REG				0x1C
>> +#define QUADSPI_DEV_ID_DATA_0				0x20
>> +#define QUADSPI_DEV_ID_DATA_1				0x24
>> +#define QUADSPI_DEV_ID_DATA_2				0x28
>> +#define QUADSPI_DEV_ID_DATA_3				0x2C
>> +#define QUADSPI_DEV_ID_DATA_4				0x30
>> +
>> +#define QUADSPI_WIN_OCC_REG				0x4
>> +#define QUADSPI_WIN_OCC_SFT				24
>> +
>> +#define QUADSPI_WIN_SEL_REG				0x8
>> +
>> +struct altera_quadspi {
>> +	u32 opcode_id;
>> +	void __iomem *csr_base;
>> +	void __iomem *data_base;
>> +	void __iomem *window_base;
>> +	size_t window_size;
>> +	u32 num_flashes;
>> +	u32 flags;
>> +	struct device *dev;
>> +	struct altera_quadspi_flash *flash[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
>> +	struct device_node *np[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
>
> Is this np even necessary ? Or why don't you put it into
> altera_quadspi_flash {} ?

I will look into removing or putting into altera_quadspi_flash.

>
>> +};
>> +
>> +struct altera_quadspi_flash {
>> +	struct spi_nor nor;
>> +	struct altera_quadspi *q;
>> +	u32 bank;
>> +};
>> +
>> +struct flash_device {
>> +	char *name;
>> +	u32 opcode_id;
>> +	u32 device_id;
>> +};
>> +
>> +#ifdef DEBUG
>
> Yet another magic NIH readl/writel debug ... sigh, drop it.

Ok, I will drop.  It was used to help the hardware person perform
simulation.

>
>> +static inline u32 alt_qspi_readl(void __iomem *base, off_t offset)
>> +{
>> +	u32 val = readl(base + offset);
>> +
>> +	pr_info("%s 0x%x from offset 0x%lx\n", __func__, val, offset);
>> +	return val;
>> +}
>> +static inline void alt_qspi_writel(u32 val, void __iomem *base, off_t offset)
>> +{
>> +	writel(val, base + offset);
>> +	pr_info("%s 0x%x to offset 0x%lx\n", __func__, val, offset);
>> +}
>> +#else
>> +#define alt_qspi_readl(base, offset) readl(base+offset)
>> +#define alt_qspi_writel(val, base, offset) writel(val, base + offset)
>
> This use of macros is wrong, but it'll be dropped anyway ...
>
>> +#endif
>> +
>> +static void altera_quadspi_chip_select(struct altera_quadspi *q, u32 bank)
>> +{
>> +	u32 val = 0;
>> +
>> +	switch (bank) {
>> +	case 0:
>> +		val = QUADSPI_CHIP_SELECT_0;
>> +		break;
>> +	case 1:
>> +		val = QUADSPI_CHIP_SELECT_1;
>> +		break;
>> +	case 2:
>> +		val = QUADSPI_CHIP_SELECT_2;
>> +		break;
>> +	default:
>> +		dev_err(q->dev, "invalid bank\n");
>> +		return;
>> +	}
>
> So this is basically
> switch ...
> case 0:
> return BIT(0);
> case 1:
> return BIT(1);
> ...
>
> This can be simplified, right ? :)

Good suggestion.

>
>> +	alt_qspi_writel(val, q->csr_base, QUADSPI_CHIP_SELECT_REG);
>> +}
>> +
>> +static int altera_quadspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
>> +				    int len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>
> Look at the mutex usage in cadence qspi driver ; if you have multiple
> flashes and you access both at the same time, you'll run into a nasty
> race if you don't have that locking in place. UBI on both can trigger
> that. Look for "bus_mutex" in that driver.

Excellent feedback!

>
>> +	switch (opcode) {
>> +	case SPINOR_OP_WREN:
>> +		dev_dbg(q->dev, "%s enabling write\n", __func__);
>> +		alt_qspi_writel(QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD,
>> +				q->csr_base, QUADSPI_MEM_OP_REG);
>> +		break;
>> +
>> +	case SPINOR_OP_CHIP_ERASE:
>> +		alt_qspi_writel(QUADSPI_MEM_OP_BULK_ERASE_CMD,
>> +				q->csr_base, QUADSPI_MEM_OP_REG);
>> +		break;
>> +
>> +	default:
>> +		dev_dbg(q->dev, "%s UNHANDLED write_reg 0x%x\n",
>> +			__func__, opcode);
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
>> +				   int len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 data = 0;
>> +
>> +	memset(val, 0, len);
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	switch (opcode) {
>> +	case SPINOR_OP_RDSR:
>> +		data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
>> +		dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
>> +		*val = (u8)data & QUADSPI_SR_MASK;
>> +		break;
>> +	case SPINOR_OP_RDID:
>> +		if (q->opcode_id == EPCS_OPCODE_ID)
>> +			data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
>> +		else
>> +			data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
>> +
>> +		*((u32 *)val) = data;
>
> What are these awful casts ?

This component requires reading the registers as 32 bit quantities. So it
seemed the right thing to do to me.

>
>> +		break;
>> +	case SPINOR_OP_RDFSR:
>> +		data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
>> +		dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
>> +		*val = (u8)(data & 0xff);
>> +		break;
>> +	default:
>> +		dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
>> +			__func__, opcode);
>> +		*val = 0;
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_write_erase_check(struct spi_nor *nor,
>> +					    bool write_erase)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 val;
>> +	u32 mask;
>> +
>> +	if (write_erase)
>> +		mask = QUADSPI_ISR_ILLEGAL_WRITE_MASK;
>> +	else
>> +		mask = QUADSPI_ISR_ILLEGAL_ERASE_MASK;
>> +
>> +	val = alt_qspi_readl(q->csr_base, QUADSPI_ISR_REG);
>> +
>> +	if (val & mask) {
>> +		dev_err(nor->dev,
>> +			"write/erase failed, sector might be protected\n");
>> +		alt_qspi_writel(0, q->csr_base, QUADSPI_FLAG_STATUS_REG);
>> +
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_addr_to_sector(struct mtd_info *mtd, uint64_t offset)
>> +{
>> +	if (mtd->erasesize_shift)
>> +		return offset >> mtd->erasesize_shift;
>> +	do_div(offset, mtd->erasesize);
>> +	return offset;
>> +}
>> +
>> +static int altera_quadspi_erase(struct spi_nor *nor, loff_t offset)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	struct mtd_info *mtd = &nor->mtd;
>> +	u32 val;
>> +	int sector_value;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	sector_value = altera_quadspi_addr_to_sector(mtd, offset);
>> +
>> +	dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
>> +
>> +	if (sector_value < 0)
>> +		return -EINVAL;
>> +
>> +	val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
>> +
>> +	val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
>> +
>> +	alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
>> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
>> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
>> +
>> +	return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
>> +}
>> +
>> +#define WINDOW_ALIGN 4
>> +#define WINDOW_MASK (WINDOW_ALIGN - 1)
>
> What are these undocumented macros in the middle of the code ?

The bindings document states that when a windowed bridge
is used, all accesses must be 32 bit.  I can comment/rename
and put at the top.

>
>> +static ssize_t altera_quadspi_windowed_read(struct altera_quadspi *q,
>> +					    loff_t from,
>> +					    size_t len, u_char *buf)
>> +{
>> +	size_t bytes_left = len;
>> +	size_t bytes_to_read, i;
>> +	loff_t next_window_off;
>> +	u64 start_window;
>> +	u32 window;
>> +	u32 *dst;
>> +
>> +	if ((from & WINDOW_MASK) || (len & WINDOW_MASK) ||
>> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
>> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
>> +			__func__);
>> +		return 0;
>> +	}
>> +
>> +	start_window = from;
>> +	do_div(start_window, q->window_size);
>> +	window = (u32)(start_window & 0xffffffff);
>> +
>> +	next_window_off = (window + 1) * q->window_size;
>> +
>> +	while (bytes_left > 0) {
>> +
>> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
>> +
>> +		bytes_to_read = min((size_t)bytes_left,
>> +				    (size_t)(next_window_off - from));
>> +
>> +		dev_dbg(q->dev,
>> +			"window%u fr0x%llx next0x%llx left%zu num0x%zx\n",
>> +			window,  from, next_window_off, bytes_left,
>> +			bytes_to_read);
>> +
>> +		dst = (u32 *)buf;
>> +		for (i = 0; i < bytes_to_read; i += 4, dst++)
>> +			*dst = readl(q->data_base +
>> +				     (from & (q->window_size - 1)) + i);
>
> ioread32_rep and co. ? Again, look at the CQSPI driver.

OK thanks for the pointer.

>
>> +		bytes_left -= bytes_to_read;
>> +		buf += bytes_to_read;
>> +		from += bytes_to_read;
>> +		window++;
>> +		next_window_off += q->window_size;
>> +	}
>> +
>> +	return len;
>> +}
>> +static ssize_t altera_quadspi_windowed_write(struct altera_quadspi *q,
>> +					     loff_t to, size_t len,
>> +					     const u_char *buf)
>> +{
>> +	size_t bytes_left = len;
>> +	u32 window_mask = q->window_size - 1;
>> +	u32 read_back;
>> +	size_t bytes_to_write, i;
>> +	loff_t next_window_off;
>> +	u64 start_window;
>> +	u32 window;
>> +	const u32 *src;
>> +	u32 words_can_write;
>> +
>> +	if ((to & WINDOW_MASK) || (len & WINDOW_MASK) ||
>> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
>> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
>> +			__func__);
>> +		return 0;
>> +	}
>> +
>> +	start_window = to;
>> +	do_div(start_window, q->window_size);
>> +	window = (u32)(start_window & 0xffffffff);
>> +
>> +	next_window_off = (window + 1) * q->window_size;
>> +
>> +	while (bytes_left > 0) {
>> +
>> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
>> +
>> +		bytes_to_write = min((size_t)bytes_left,
>> +				    (size_t)(next_window_off - to));
>> +
>> +		dev_dbg(q->dev,
>> +			"window%u to0x%llx next0x%llx left%zu num0x%zx\n",
>> +			window,  to, next_window_off, bytes_left,
>> +			bytes_to_write);
>
> DTTO , either memcpy_toio or iowrite32_rep

I observed memcpy_toio was perform 8 bit accesses.  I will look at 
iowrite32_rep.

>
>> +		src = (u32 *)buf;
>> +		for (i = 0; i < bytes_to_write;) {
>> +			words_can_write =
>> +				readl(q->window_base + QUADSPI_WIN_OCC_REG) >>
>> +				      QUADSPI_WIN_OCC_SFT;
>> +			dev_dbg(q->dev, "can write 0x%x\n", words_can_write);
>> +
>> +			for (; words_can_write > 0; words_can_write--) {
>> +				writel(*src,
>> +				       q->data_base +
>> +				       (to & window_mask) + i);
>> +				read_back = readl(q->data_base +
>> +						  (to & window_mask) + i);
>> +				if (*src != read_back) {
>> +					dev_err(q->dev, "%s 0x%x != 0x%x\n",
>> +						__func__, *src, read_back);
>> +					return (len - bytes_left);
>> +				}
>> +				i += 4;
>> +				src++;
>> +			}
>> +		}
>> +
>> +		bytes_left -= bytes_to_write;
>> +		buf += bytes_to_write;
>> +		to += bytes_to_write;
>> +		window++;
>> +		next_window_off += q->window_size;
>> +	}
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t altera_quadspi_read(struct spi_nor *nor, loff_t from, size_t len,
>> +				   u_char *buf)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	size_t i;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	if (q->window_size)
>> +		altera_quadspi_windowed_read(q, from, len, buf);
>> +	else
>> +		memcpy_fromio(buf, q->data_base + from, len);
>> +
>> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_READ) {
>> +		for (i = 0; i < len; i++, buf++)
>> +			*buf = bitrev8(*buf);
>> +	}
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t altera_quadspi_write(struct spi_nor *nor, loff_t to,
>> +				    size_t len, const u_char *buf)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u_char *bitrev_buf = NULL;
>> +	const u_char *src;
>> +	u_char *dst;
>> +	size_t i;
>> +	int ret = 0;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_WRITE) {
>> +		bitrev_buf = devm_kzalloc(q->dev, len, GFP_KERNEL);
>> +		if (!bitrev_buf)
>> +			return 0;
>> +
>> +		src = buf;
>> +		dst = bitrev_buf;
>> +		for (i = 0; i < len; i++, src++, dst++)
>> +			*dst = bitrev8(*src);
>> +
>> +		buf = bitrev_buf;
>> +	}
>> +
>> +	if (q->window_size)
>> +		altera_quadspi_windowed_write(q, to, len, buf);
>> +	else
>> +		memcpy_toio(q->data_base + to, buf, len);
>> +
>> +
>> +	if (bitrev_buf)
>> +		devm_kfree(q->dev, bitrev_buf);
>> +
>> +	ret = altera_quadspi_write_erase_check(nor, WRITE_CHECK);
>> +
>> +	return len;
>> +
>> +}
>> +
>> +static int altera_quadspi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	struct mtd_info *mtd = &nor->mtd;
>> +	uint32_t offset = ofs;
>> +	u32 sector_start, sector_end;
>> +	uint64_t num_sectors;
>> +	u32 mem_op;
>> +	u32 sr_bp;
>> +	u32 sr_tb;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	sector_start = offset;
>> +	sector_end = altera_quadspi_addr_to_sector(mtd, offset + len);
>> +	num_sectors = mtd->size;
>> +	do_div(num_sectors, mtd->erasesize);
>> +
>> +	dev_dbg(nor->dev, "%s: sector start is %u,sector end is %u\n",
>> +		__func__, sector_start, sector_end);
>> +
>> +	if (sector_start >= num_sectors / 2) {
>> +		sr_bp = fls(num_sectors - 1 - sector_start) + 1;
>> +		sr_tb = 0;
>> +	} else if ((sector_end < num_sectors / 2) &&
>> +		  (q->opcode_id != EPCS_OPCODE_ID)) {
>> +		sr_bp = fls(sector_end) + 1;
>> +		sr_tb = 1;
>> +	} else {
>> +		sr_bp = 16;
>> +		sr_tb = 0;
>> +	}
>> +
>> +	mem_op = (sr_tb << 12) | (sr_bp << 8);
>> +	mem_op &= QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK;
>> +	mem_op |= QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
>> +
>> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 mem_op;
>> +
>> +	dev_dbg(nor->dev, "Unlock all protected area\n");
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	mem_op = QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
>> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_setup_banks(struct device *dev,
>> +				      u32 bank, struct device_node *np)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +	struct altera_quadspi_flash *flash;
>> +	struct spi_nor *nor;
>> +	int ret = 0;
>> +	char modalias[40] = {0};
>> +	struct spi_nor_hwcaps hwcaps = {
>> +		.mask = SNOR_HWCAPS_READ |
>> +			SNOR_HWCAPS_READ_FAST |
>> +			SNOR_HWCAPS_READ_1_1_2 |
>> +			SNOR_HWCAPS_READ_1_1_4 |
>> +			SNOR_HWCAPS_PP,
>> +	};
>> +
>> +	if (bank > q->num_flashes - 1)
>> +		return -EINVAL;
>> +
>> +	altera_quadspi_chip_select(q, bank);
>> +
>> +	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
>> +	if (!flash)
>> +		return -ENOMEM;
>> +
>> +	q->flash[bank] = flash;
>> +	nor = &flash->nor;
>> +	nor->dev = dev;
>> +	nor->priv = flash;
>> +	nor->mtd.priv = nor;
>> +	flash->q = q;
>> +	flash->bank = bank;
>> +	spi_nor_set_flash_node(nor, np);
>> +
>> +	/* spi nor framework*/
>> +	nor->read_reg = altera_quadspi_read_reg;
>> +	nor->write_reg = altera_quadspi_write_reg;
>> +	nor->read = altera_quadspi_read;
>> +	nor->write = altera_quadspi_write;
>> +	nor->erase = altera_quadspi_erase;
>> +	nor->flash_lock = altera_quadspi_lock;
>> +	nor->flash_unlock = altera_quadspi_unlock;
>> +
>> +	/* scanning flash and checking dev id */
>> +#ifdef CONFIG_OF
>> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>> +		return -EINVAL;
>> +#endif
>> +
>> +	ret = spi_nor_scan(nor, modalias, &hwcaps);
>> +	if (ret) {
>> +		dev_err(nor->dev, "flash not found\n");
>> +		return ret;
>> +	}
>> +
>> +	ret =  mtd_device_register(&nor->mtd, NULL, 0);
>> +
>> +	altera_quadspi_unlock(nor, 0, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>> +			  void __iomem *data_base, void __iomem *window_base,
>> +			  size_t window_size, u32 flags)
>> +{
>> +	struct altera_quadspi *q;
>> +
>> +	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>> +	if (!q)
>> +		return -ENOMEM;
>> +
>> +	q->dev = dev;
>> +	q->csr_base = csr_base;
>> +	q->data_base = data_base;
>> +	q->window_base = window_base;
>> +	q->window_size = window_size;
>> +
>> +	q->flags = flags;
>> +
>> +	dev_set_drvdata(dev, q);
>> +
>> +	dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
>> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
>> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_quadspi_create);
>> +
>> +int altera_qspi_add_bank(struct device *dev,
>> +			 u32 bank, struct device_node *np)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +
>> +	if (q->num_flashes >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP)
>> +		return -ENOMEM;
>> +
>> +	q->num_flashes++;
>> +
>> +	return altera_quadspi_setup_banks(dev, bank, np);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_qspi_add_bank);
>> +
>> +int altera_quadspi_remove_banks(struct device *dev)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +	struct altera_quadspi_flash *flash;
>> +	int i;
>> +	int ret = 0;
>> +
>> +	/* clean up for all nor flash */
>> +	for (i = 0; i < q->num_flashes; i++) {
>> +		flash = q->flash[i];
>> +		if (!flash)
>> +			continue;
>> +
>> +		/* clean up mtd stuff */
>> +		ret = mtd_device_unregister(&flash->nor.mtd);
>> +		if (ret) {
>> +			dev_err(dev, "error removing mtd\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_quadspi_remove_banks);
>> +
>> +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 Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mtd/altera-quadspi.h b/include/linux/mtd/altera-quadspi.h
>> new file mode 100644
>> index 0000000..58f31ee
>> --- /dev/null
>> +++ b/include/linux/mtd/altera-quadspi.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + *
>> + * Copyright 2017 Intel Corporation, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#ifndef __ALTERA_QUADSPI_H
>> +#define __ALTERA_QUADSPI_H
>> +
>> +#include <linux/device.h>
>> +
>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>> +
>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>> +
>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>> +			  void __iomem *data_base, void __iomem *window_reg,
>> +			  size_t window_size, u32 flags);
>> +
>> +int altera_qspi_add_bank(struct device *dev,
>> +			 u32 bank, struct device_node *np);
>> +
>> +int altera_quadspi_remove_banks(struct device *dev);
>
> Why is this header needed at all ?

This header is needed because of the very different ways
FPGAs can be used with a processor running Linux.  In the case of a
soft processor in the FPGA or an ARM connected to a FPGA, this header
is not necessary because device trees are used to probe the driver.
However, if the FPGA is on a PCIe card connected to an x86, device trees 
are not generally used, and the pcie driver must enumerate the 
"sub-driver".

>
>> +#endif
>>
>
>
> -- 
> Best regards,
> Marek Vasut
>

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[Marek Vasut]

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

[...]

>>> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode,
>>> u8 *val,
>>> +                   int len)
>>> +{
>>> +    struct altera_quadspi_flash *flash = nor->priv;
>>> +    struct altera_quadspi *q = flash->q;
>>> +    u32 data = 0;
>>> +
>>> +    memset(val, 0, len);
>>> +
>>> +    altera_quadspi_chip_select(q, flash->bank);
>>> +
>>> +    switch (opcode) {
>>> +    case SPINOR_OP_RDSR:
>>> +        data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
>>> +        dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
>>> +        *val = (u8)data & QUADSPI_SR_MASK;
>>> +        break;
>>> +    case SPINOR_OP_RDID:
>>> +        if (q->opcode_id == EPCS_OPCODE_ID)
>>> +            data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
>>> +        else
>>> +            data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
>>> +
>>> +        *((u32 *)val) = data;
>>
>> What are these awful casts ?
> 
> This component requires reading the registers as 32 bit quantities. So it
> seemed the right thing to do to me.

Does this handle endianness well ?

>>> +        break;
>>> +    case SPINOR_OP_RDFSR:
>>> +        data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
>>> +        dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
>>> +        *val = (u8)(data & 0xff);
>>> +        break;
>>> +    default:
>>> +        dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
>>> +            __func__, opcode);
>>> +        *val = 0;
>>> +        break;
>>> +    }
>>> +    return 0;
>>> +}

[...]

>>> +#define WINDOW_ALIGN 4
>>> +#define WINDOW_MASK (WINDOW_ALIGN - 1)
>>
>> What are these undocumented macros in the middle of the code ?
> 
> The bindings document states that when a windowed bridge
> is used, all accesses must be 32 bit.  I can comment/rename
> and put at the top.

Yes please.

[...]

>>> diff --git a/include/linux/mtd/altera-quadspi.h
>>> b/include/linux/mtd/altera-quadspi.h
>>> new file mode 100644
>>> index 0000000..58f31ee
>>> --- /dev/null
>>> +++ b/include/linux/mtd/altera-quadspi.h
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + *
>>> + * Copyright 2017 Intel Corporation, Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + */
>>> +#ifndef __ALTERA_QUADSPI_H
>>> +#define __ALTERA_QUADSPI_H
>>> +
>>> +#include <linux/device.h>
>>> +
>>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>>> +
>>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>>> +
>>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>>> +              void __iomem *data_base, void __iomem *window_reg,
>>> +              size_t window_size, u32 flags);
>>> +
>>> +int altera_qspi_add_bank(struct device *dev,
>>> +             u32 bank, struct device_node *np);
>>> +
>>> +int altera_quadspi_remove_banks(struct device *dev);
>>
>> Why is this header needed at all ?
> 
> This header is needed because of the very different ways
> FPGAs can be used with a processor running Linux.  In the case of a
> soft processor in the FPGA or an ARM connected to a FPGA, this header
> is not necessary because device trees are used to probe the driver.
> However, if the FPGA is on a PCIe card connected to an x86, device trees
> are not generally used, and the pcie driver must enumerate the
> "sub-driver".

But we don't support that later part, do we ?

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[matthew.gerlach]

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/27/2017 04:57 PM, matthew.gerlach@linux.intel.com wrote:
>
> [...]
>
>>>> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode,
>>>> u8 *val,
>>>> +                   int len)
>>>> +{
>>>> +    struct altera_quadspi_flash *flash = nor->priv;
>>>> +    struct altera_quadspi *q = flash->q;
>>>> +    u32 data = 0;
>>>> +
>>>> +    memset(val, 0, len);
>>>> +
>>>> +    altera_quadspi_chip_select(q, flash->bank);
>>>> +
>>>> +    switch (opcode) {
>>>> +    case SPINOR_OP_RDSR:
>>>> +        data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
>>>> +        dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
>>>> +        *val = (u8)data & QUADSPI_SR_MASK;
>>>> +        break;
>>>> +    case SPINOR_OP_RDID:
>>>> +        if (q->opcode_id == EPCS_OPCODE_ID)
>>>> +            data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
>>>> +        else
>>>> +            data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
>>>> +
>>>> +        *((u32 *)val) = data;
>>>
>>> What are these awful casts ?
>>
>> This component requires reading the registers as 32 bit quantities. So it
>> seemed the right thing to do to me.
>
> Does this handle endianness well ?

Good point.  This would break horribly on a big endian cpu.

>
>>>> +        break;
>>>> +    case SPINOR_OP_RDFSR:
>>>> +        data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
>>>> +        dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
>>>> +        *val = (u8)(data & 0xff);
>>>> +        break;
>>>> +    default:
>>>> +        dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
>>>> +            __func__, opcode);
>>>> +        *val = 0;
>>>> +        break;
>>>> +    }
>>>> +    return 0;
>>>> +}
>
> [...]
>
>>>> +#define WINDOW_ALIGN 4
>>>> +#define WINDOW_MASK (WINDOW_ALIGN - 1)
>>>
>>> What are these undocumented macros in the middle of the code ?
>>
>> The bindings document states that when a windowed bridge
>> is used, all accesses must be 32 bit.  I can comment/rename
>> and put at the top.
>
> Yes please.
>
> [...]
>
>>>> diff --git a/include/linux/mtd/altera-quadspi.h
>>>> b/include/linux/mtd/altera-quadspi.h
>>>> new file mode 100644
>>>> index 0000000..58f31ee
>>>> --- /dev/null
>>>> +++ b/include/linux/mtd/altera-quadspi.h
>>>> @@ -0,0 +1,28 @@
>>>> +/*
>>>> + *
>>>> + * Copyright 2017 Intel Corporation, Inc.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + */
>>>> +#ifndef __ALTERA_QUADSPI_H
>>>> +#define __ALTERA_QUADSPI_H
>>>> +
>>>> +#include <linux/device.h>
>>>> +
>>>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>>>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>>>> +
>>>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>>>> +
>>>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>>>> +              void __iomem *data_base, void __iomem *window_reg,
>>>> +              size_t window_size, u32 flags);
>>>> +
>>>> +int altera_qspi_add_bank(struct device *dev,
>>>> +             u32 bank, struct device_node *np);
>>>> +
>>>> +int altera_quadspi_remove_banks(struct device *dev);
>>>
>>> Why is this header needed at all ?
>>
>> This header is needed because of the very different ways
>> FPGAs can be used with a processor running Linux.  In the case of a
>> soft processor in the FPGA or an ARM connected to a FPGA, this header
>> is not necessary because device trees are used to probe the driver.
>> However, if the FPGA is on a PCIe card connected to an x86, device trees
>> are not generally used, and the pcie driver must enumerate the
>> "sub-driver".
>
> But we don't support that later part, do we ?

There is currently v2 patch set for the intel-fpga PCIe driver being 
reviewed where I am adding support for version 2 of the Altera Quadspi 
controller.

This technique of separating core driver code from platform/device tree 
code has been reviewed and accepted for the Altera Partial Reconfiguration 
IP, Altera Freeze Bridge, and the fpga region.

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

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[Marek Vasut]

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

[...]

>>>>> +#ifndef __ALTERA_QUADSPI_H
>>>>> +#define __ALTERA_QUADSPI_H
>>>>> +
>>>>> +#include <linux/device.h>
>>>>> +
>>>>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>>>>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>>>>> +
>>>>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>>>>> +
>>>>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>>>>> +              void __iomem *data_base, void __iomem *window_reg,
>>>>> +              size_t window_size, u32 flags);
>>>>> +
>>>>> +int altera_qspi_add_bank(struct device *dev,
>>>>> +             u32 bank, struct device_node *np);
>>>>> +
>>>>> +int altera_quadspi_remove_banks(struct device *dev);
>>>>
>>>> Why is this header needed at all ?
>>>
>>> This header is needed because of the very different ways
>>> FPGAs can be used with a processor running Linux.  In the case of a
>>> soft processor in the FPGA or an ARM connected to a FPGA, this header
>>> is not necessary because device trees are used to probe the driver.
>>> However, if the FPGA is on a PCIe card connected to an x86, device trees
>>> are not generally used, and the pcie driver must enumerate the
>>> "sub-driver".
>>
>> But we don't support that later part, do we ?
> 
> There is currently v2 patch set for the intel-fpga PCIe driver being
> reviewed where I am adding support for version 2 of the Altera Quadspi
> controller.

It'd be real nice to mention that in the cover letter with a link to
that patchset , otherwise it's real hard to understand why you did this.

> This technique of separating core driver code from platform/device tree
> code has been reviewed and accepted for the Altera Partial
> Reconfiguration IP, Altera Freeze Bridge, and the fpga region.

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[matthew.gerlach]

On Tue, 27 Jun 2017, Marek Vasut wrote:

> On 06/27/2017 07:26 PM, matthew.gerlach@linux.intel.com wrote:
>
> [...]
>
>>>>>> +#ifndef __ALTERA_QUADSPI_H
>>>>>> +#define __ALTERA_QUADSPI_H
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +
>>>>>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>>>>>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>>>>>> +
>>>>>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>>>>>> +
>>>>>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>>>>>> +              void __iomem *data_base, void __iomem *window_reg,
>>>>>> +              size_t window_size, u32 flags);
>>>>>> +
>>>>>> +int altera_qspi_add_bank(struct device *dev,
>>>>>> +             u32 bank, struct device_node *np);
>>>>>> +
>>>>>> +int altera_quadspi_remove_banks(struct device *dev);
>>>>>
>>>>> Why is this header needed at all ?
>>>>
>>>> This header is needed because of the very different ways
>>>> FPGAs can be used with a processor running Linux.  In the case of a
>>>> soft processor in the FPGA or an ARM connected to a FPGA, this header
>>>> is not necessary because device trees are used to probe the driver.
>>>> However, if the FPGA is on a PCIe card connected to an x86, device trees
>>>> are not generally used, and the pcie driver must enumerate the
>>>> "sub-driver".
>>>
>>> But we don't support that later part, do we ?
>>
>> There is currently v2 patch set for the intel-fpga PCIe driver being
>> reviewed where I am adding support for version 2 of the Altera Quadspi
>> controller.
>
> It'd be real nice to mention that in the cover letter with a link to
> that patchset , otherwise it's real hard to understand why you did this.

The v2 patch set does not have my changes in it, but adding discussion in 
the cover letter is great idea!

Many thanks,

Matthew Gerlach
>
>> This technique of separating core driver code from platform/device tree
>> code has been reviewed and accepted for the Altera Partial
>> Reconfiguration IP, Altera Freeze Bridge, and the fpga region.
>
> -- 
> Best regards,
> Marek Vasut
>

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[Cyrille Pitchen]

Hi Matthew,


Le 26/06/2017 à 18:13, matthew.gerlach@linux.intel.com a écrit :
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  MAINTAINERS                          |   7 +
>  drivers/mtd/spi-nor/Kconfig          |   5 +
>  drivers/mtd/spi-nor/Makefile         |   4 +-
>  drivers/mtd/spi-nor/altera-quadspi.c | 676 +++++++++++++++++++++++++++++++++++
>  include/linux/mtd/altera-quadspi.h   |  28 ++
>  5 files changed, 719 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi.c
>  create mode 100644 include/linux/mtd/altera-quadspi.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b4395c..ae33fa6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -648,6 +648,13 @@ L:	linux-gpio@vger.kernel.org
>  S:	Maintained
>  F:	drivers/gpio/gpio-altera.c
>  
> +ALTERA QUADSPI FLASH DRIVER
> +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:	inclulde/linux/mtd/altera-quadspi.h
> +
>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
>  M:	Thor Thayer <thor.thayer@linux.intel.com>
>  S:	Maintained
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 293c8a4..89fe425 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -113,4 +113,9 @@ config SPI_STM32_QUADSPI
>  	  This enables support for the STM32 Quad SPI controller.
>  	  We only connect the NOR to this controller.
>  
> +config SPI_ALTERA_QUADSPI
> +        tristate "Altera Quad SPI Flash Controller II"
> +        help
> +          Enable support for version 2 of Altera Quad SPI Flash Controller.
> +
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 285aab8..024c6ac 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -8,4 +8,6 @@ obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
>  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
> \ No newline at end of file
> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
> +obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
> +
> diff --git a/drivers/mtd/spi-nor/altera-quadspi.c b/drivers/mtd/spi-nor/altera-quadspi.c
> new file mode 100644
> index 0000000..de65453
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/altera-quadspi.c
> @@ -0,0 +1,676 @@
> +/*
> + * 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/bitrev.h>
> +#include <linux/module.h>
> +#include <linux/mtd/altera-quadspi.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spi-nor.h>
> +
> +#define ALTERA_QUADSPI_RESOURCE_NAME			"altera_quadspi"
> +
> +#define EPCS_OPCODE_ID					1
> +#define NON_EPCS_OPCODE_ID				2
> +
> +#define WRITE_CHECK					1
> +#define ERASE_CHECK					0
> +
> +#define QUADSPI_SR_REG					0x0
> +#define QUADSPI_SR_MASK					0x0000000F
> +
> +/* defines for device id register */
> +#define QUADSPI_SID_REG					0x4
> +#define QUADSPI_RDID_REG				0x8
> +#define QUADSPI_ID_MASK					0x000000FF
> +
> +/*
> + * QUADSPI_MEM_OP register offset
> + *
> + * The QUADSPI_MEM_OP register is used to do memory protect and erase operations
> + *
> + */
> +#define QUADSPI_MEM_OP_REG				0xC
> +
> +#define QUADSPI_MEM_OP_CMD_MASK				0x00000003
> +#define QUADSPI_MEM_OP_BULK_ERASE_CMD			0x00000001
> +#define QUADSPI_MEM_OP_SECTOR_ERASE_CMD			0x00000002
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_CMD		0x00000003
> +#define QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD		0x00000004
> +#define QUADSPI_MEM_OP_SECTOR_VALUE_MASK		0x0003FF00
> +
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_SHIFT		8
> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK	0x00001F00
> +/*
> + * QUADSPI_ISR register offset
> + *
> + * The QUADSPI_ISR register is used to determine whether an invalid write or
> + * erase operation trigerred an interrupt
> + *
> + */
> +#define QUADSPI_ISR_REG					0x10
> +
> +#define QUADSPI_ISR_ILLEGAL_ERASE_MASK			0x00000001
> +#define QUADSPI_ISR_ILLEGAL_WRITE_MASK			0x00000002
> +
> +/*
> + * QUADSPI_IMR register offset
> + *
> + * The QUADSPI_IMR register is used to mask the invalid erase or the invalid
> + * write interrupts.
> + *
> + */
> +#define QUADSPI_IMR_REG					0x14
> +#define QUADSPI_IMR_ILLEGAL_ERASE_MASK			0x00000001
> +
> +#define QUADSPI_IMR_ILLEGAL_WRITE_MASK			0x00000002
> +
> +#define QUADSPI_CHIP_SELECT_REG				0x18
> +#define QUADSPI_CHIP_SELECT_MASK			0x00000007
> +#define QUADSPI_CHIP_SELECT_0				0x00000001
> +#define QUADSPI_CHIP_SELECT_1				0x00000002
> +#define QUADSPI_CHIP_SELECT_2				0x00000004
> +
> +#define QUADSPI_FLAG_STATUS_REG				0x1C
> +#define QUADSPI_DEV_ID_DATA_0				0x20
> +#define QUADSPI_DEV_ID_DATA_1				0x24
> +#define QUADSPI_DEV_ID_DATA_2				0x28
> +#define QUADSPI_DEV_ID_DATA_3				0x2C
> +#define QUADSPI_DEV_ID_DATA_4				0x30
> +
> +#define QUADSPI_WIN_OCC_REG				0x4
> +#define QUADSPI_WIN_OCC_SFT				24
> +
> +#define QUADSPI_WIN_SEL_REG				0x8
> +
> +struct altera_quadspi {
> +	u32 opcode_id;
> +	void __iomem *csr_base;
> +	void __iomem *data_base;
> +	void __iomem *window_base;
> +	size_t window_size;
> +	u32 num_flashes;
> +	u32 flags;
> +	struct device *dev;
> +	struct altera_quadspi_flash *flash[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
> +	struct device_node *np[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
> +};
> +
> +struct altera_quadspi_flash {
> +	struct spi_nor nor;
> +	struct altera_quadspi *q;
> +	u32 bank;
> +};
> +
> +struct flash_device {
> +	char *name;
> +	u32 opcode_id;
> +	u32 device_id;
> +};
> +
> +#ifdef DEBUG
> +static inline u32 alt_qspi_readl(void __iomem *base, off_t offset)
> +{
> +	u32 val = readl(base + offset);
> +
> +	pr_info("%s 0x%x from offset 0x%lx\n", __func__, val, offset);
> +	return val;
> +}
> +static inline void alt_qspi_writel(u32 val, void __iomem *base, off_t offset)
> +{
> +	writel(val, base + offset);
> +	pr_info("%s 0x%x to offset 0x%lx\n", __func__, val, offset);
> +}
> +#else
> +#define alt_qspi_readl(base, offset) readl(base+offset)
> +#define alt_qspi_writel(val, base, offset) writel(val, base + offset)
> +#endif
> +
> +static void altera_quadspi_chip_select(struct altera_quadspi *q, u32 bank)
> +{
> +	u32 val = 0;
> +
> +	switch (bank) {
> +	case 0:
> +		val = QUADSPI_CHIP_SELECT_0;
> +		break;
> +	case 1:
> +		val = QUADSPI_CHIP_SELECT_1;
> +		break;
> +	case 2:
> +		val = QUADSPI_CHIP_SELECT_2;
> +		break;
> +	default:
> +		dev_err(q->dev, "invalid bank\n");
> +		return;
> +	}
> +	alt_qspi_writel(val, q->csr_base, QUADSPI_CHIP_SELECT_REG);
> +}
> +
> +static int altera_quadspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
> +				    int len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	switch (opcode) {
> +	case SPINOR_OP_WREN:
> +		dev_dbg(q->dev, "%s enabling write\n", __func__);
> +		alt_qspi_writel(QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD,
> +				q->csr_base, QUADSPI_MEM_OP_REG);
> +		break;
> +
> +	case SPINOR_OP_CHIP_ERASE:
> +		alt_qspi_writel(QUADSPI_MEM_OP_BULK_ERASE_CMD,
> +				q->csr_base, QUADSPI_MEM_OP_REG);
> +		break;
> +
> +	default:
> +		dev_dbg(q->dev, "%s UNHANDLED write_reg 0x%x\n",
> +			__func__, opcode);

Looking at the code I assume, the hardware can only send
predefined/hard-coded SPI op codes but not those actually chosen by
spi-nor.c so what will happen when spi-nor introduces new op codes?
Does it mean that this driver will immediately be broken?

> +
> +	}
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
> +				   int len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 data = 0;
> +
> +	memset(val, 0, len);
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	switch (opcode) {
> +	case SPINOR_OP_RDSR:
> +		data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
> +		dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
> +		*val = (u8)data & QUADSPI_SR_MASK;
> +		break;
> +	case SPINOR_OP_RDID:
> +		if (q->opcode_id == EPCS_OPCODE_ID)

Where do you initialize opcode_id ?

> +			data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
> +		else
> +			data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);

Why 2 differents registers for the JEDEC ID? Is data actually the result
of some READ JEDEC ID (9Fh) command or some hard-coded value?

> +
> +		*((u32 *)val) = data;
> +		break;
> +	case SPINOR_OP_RDFSR:
> +		data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
> +		dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
> +		*val = (u8)(data & 0xff);
> +		break;

This is a "Micron only" register, other chunks of this patch let me
think that the Altera controller can only work this Micron/EPCS
memories, am I wrong?

OK, then let assume a Micron memory: what if one day spi-nor.c needs to
read the VCR and/or EVCR registers of Micron memories to check some
specific bits? This driver is very likely to fail.

> +	default:
> +		dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
> +			__func__, opcode);
> +		*val = 0;
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int altera_quadspi_write_erase_check(struct spi_nor *nor,
> +					    bool write_erase)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 val;
> +	u32 mask;
> +
> +	if (write_erase)
> +		mask = QUADSPI_ISR_ILLEGAL_WRITE_MASK;
> +	else
> +		mask = QUADSPI_ISR_ILLEGAL_ERASE_MASK;
> +
> +	val = alt_qspi_readl(q->csr_base, QUADSPI_ISR_REG);
> +
> +	if (val & mask) {
> +		dev_err(nor->dev,
> +			"write/erase failed, sector might be protected\n");
> +		alt_qspi_writel(0, q->csr_base, QUADSPI_FLAG_STATUS_REG);
> +
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_addr_to_sector(struct mtd_info *mtd, uint64_t offset)
> +{
> +	if (mtd->erasesize_shift)
> +		return offset >> mtd->erasesize_shift;
> +	do_div(offset, mtd->erasesize);
> +	return offset;
> +}
> +
> +static int altera_quadspi_erase(struct spi_nor *nor, loff_t offset)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	struct mtd_info *mtd = &nor->mtd;
> +	u32 val;
> +	int sector_value;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	sector_value = altera_quadspi_addr_to_sector(mtd, offset);

Here the driver uses nor->mtd.erasesize to do some conversion of the
offset into some kind of sector index.
However this function doesn't care about nor->erase_opcode. Then I guess
that the hardware can only use a hard-coded op code for Block/Sector
Erase operations.

I don't know which erase op code is actually used by the hardware but
whatever... How do you know that you've computed the right sector index
before asking the hardware to execute some erase operation since this
index is based on nor->mtd.erasesize ?

nor->mtd.erasesize depends on the chosen nor->erase_opcode.
For instance, depending on whether CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is
defined or not, there will be at least one case where nor->erase_opcode
doesn't match the hard-coded op code used by the Altera controller:
different op codes, different sector sizes, different sector indexes...

buggy nor->erase() function


> +
> +	dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
> +
> +	if (sector_value < 0)
> +		return -EINVAL;
> +
> +	val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
> +
> +	val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
> +
> +	alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
> +
> +	return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
> +}
> +
> +#define WINDOW_ALIGN 4
> +#define WINDOW_MASK (WINDOW_ALIGN - 1)
> +
> +static ssize_t altera_quadspi_windowed_read(struct altera_quadspi *q,
> +					    loff_t from,
> +					    size_t len, u_char *buf)
> +{
> +	size_t bytes_left = len;
> +	size_t bytes_to_read, i;
> +	loff_t next_window_off;
> +	u64 start_window;
> +	u32 window;
> +	u32 *dst;
> +
> +	if ((from & WINDOW_MASK) || (len & WINDOW_MASK) ||
> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	start_window = from;
> +	do_div(start_window, q->window_size);
> +	window = (u32)(start_window & 0xffffffff);
> +
> +	next_window_off = (window + 1) * q->window_size;
> +
> +	while (bytes_left > 0) {
> +
> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
> +
> +		bytes_to_read = min((size_t)bytes_left,
> +				    (size_t)(next_window_off - from));
> +
> +		dev_dbg(q->dev,
> +			"window%u fr0x%llx next0x%llx left%zu num0x%zx\n",
> +			window,  from, next_window_off, bytes_left,
> +			bytes_to_read);
> +
> +		dst = (u32 *)buf;
> +		for (i = 0; i < bytes_to_read; i += 4, dst++)
> +			*dst = readl(q->data_base +
> +				     (from & (q->window_size - 1)) + i);
> +
> +		bytes_left -= bytes_to_read;
> +		buf += bytes_to_read;
> +		from += bytes_to_read;
> +		window++;
> +		next_window_off += q->window_size;
> +	}
> +
> +	return len;
> +}
> +static ssize_t altera_quadspi_windowed_write(struct altera_quadspi *q,
> +					     loff_t to, size_t len,
> +					     const u_char *buf)
> +{
> +	size_t bytes_left = len;
> +	u32 window_mask = q->window_size - 1;
> +	u32 read_back;
> +	size_t bytes_to_write, i;
> +	loff_t next_window_off;
> +	u64 start_window;
> +	u32 window;
> +	const u32 *src;
> +	u32 words_can_write;
> +
> +	if ((to & WINDOW_MASK) || (len & WINDOW_MASK) ||
> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	start_window = to;
> +	do_div(start_window, q->window_size);
> +	window = (u32)(start_window & 0xffffffff);
> +
> +	next_window_off = (window + 1) * q->window_size;
> +
> +	while (bytes_left > 0) {
> +
> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
> +
> +		bytes_to_write = min((size_t)bytes_left,
> +				    (size_t)(next_window_off - to));
> +
> +		dev_dbg(q->dev,
> +			"window%u to0x%llx next0x%llx left%zu num0x%zx\n",
> +			window,  to, next_window_off, bytes_left,
> +			bytes_to_write);
> +
> +		src = (u32 *)buf;
> +		for (i = 0; i < bytes_to_write;) {
> +			words_can_write =
> +				readl(q->window_base + QUADSPI_WIN_OCC_REG) >>
> +				      QUADSPI_WIN_OCC_SFT;
> +			dev_dbg(q->dev, "can write 0x%x\n", words_can_write);
> +
> +			for (; words_can_write > 0; words_can_write--) {
> +				writel(*src,
> +				       q->data_base +
> +				       (to & window_mask) + i);
> +				read_back = readl(q->data_base +
> +						  (to & window_mask) + i);
> +				if (*src != read_back) {
> +					dev_err(q->dev, "%s 0x%x != 0x%x\n",
> +						__func__, *src, read_back);
> +					return (len - bytes_left);
> +				}
> +				i += 4;
> +				src++;
> +			}
> +		}
> +
> +		bytes_left -= bytes_to_write;
> +		buf += bytes_to_write;
> +		to += bytes_to_write;
> +		window++;
> +		next_window_off += q->window_size;
> +	}
> +
> +	return len;
> +}
> +
> +static ssize_t altera_quadspi_read(struct spi_nor *nor, loff_t from, size_t len,
> +				   u_char *buf)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	size_t i;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	if (q->window_size)
> +		altera_quadspi_windowed_read(q, from, len, buf);
> +	else
> +		memcpy_fromio(buf, q->data_base + from, len);
> +
> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_READ) {
> +		for (i = 0; i < len; i++, buf++)
> +			*buf = bitrev8(*buf);
> +	}
> +
> +	return len;
> +}
> +
> +static ssize_t altera_quadspi_write(struct spi_nor *nor, loff_t to,
> +				    size_t len, const u_char *buf)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u_char *bitrev_buf = NULL;
> +	const u_char *src;
> +	u_char *dst;
> +	size_t i;
> +	int ret = 0;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_WRITE) {
> +		bitrev_buf = devm_kzalloc(q->dev, len, GFP_KERNEL);
> +		if (!bitrev_buf)
> +			return 0;
> +
> +		src = buf;
> +		dst = bitrev_buf;
> +		for (i = 0; i < len; i++, src++, dst++)
> +			*dst = bitrev8(*src);
> +
> +		buf = bitrev_buf;
> +	}
> +
> +	if (q->window_size)
> +		altera_quadspi_windowed_write(q, to, len, buf);
> +	else
> +		memcpy_toio(q->data_base + to, buf, len);
> +
> +
> +	if (bitrev_buf)
> +		devm_kfree(q->dev, bitrev_buf);
> +
> +	ret = altera_quadspi_write_erase_check(nor, WRITE_CHECK);
> +
> +	return len;
> +
> +}
> +
> +static int altera_quadspi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	struct mtd_info *mtd = &nor->mtd;
> +	uint32_t offset = ofs;
> +	u32 sector_start, sector_end;
> +	uint64_t num_sectors;
> +	u32 mem_op;
> +	u32 sr_bp;
> +	u32 sr_tb;
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	sector_start = offset;
> +	sector_end = altera_quadspi_addr_to_sector(mtd, offset + len);
> +	num_sectors = mtd->size;
> +	do_div(num_sectors, mtd->erasesize);
> +
> +	dev_dbg(nor->dev, "%s: sector start is %u,sector end is %u\n",
> +		__func__, sector_start, sector_end);
> +
> +	if (sector_start >= num_sectors / 2) {
> +		sr_bp = fls(num_sectors - 1 - sector_start) + 1;
> +		sr_tb = 0;
> +	} else if ((sector_end < num_sectors / 2) &&
> +		  (q->opcode_id != EPCS_OPCODE_ID)) {
> +		sr_bp = fls(sector_end) + 1;
> +		sr_tb = 1;
> +	} else {
> +		sr_bp = 16;
> +		sr_tb = 0;
> +	}
> +
> +	mem_op = (sr_tb << 12) | (sr_bp << 8);
> +	mem_op &= QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK;
> +	mem_op |= QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
> +
> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	struct altera_quadspi_flash *flash = nor->priv;
> +	struct altera_quadspi *q = flash->q;
> +	u32 mem_op;
> +
> +	dev_dbg(nor->dev, "Unlock all protected area\n");
> +
> +	altera_quadspi_chip_select(q, flash->bank);
> +
> +	mem_op = QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
> +
> +	return 0;
> +}
> +
> +static int altera_quadspi_setup_banks(struct device *dev,
> +				      u32 bank, struct device_node *np)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +	struct altera_quadspi_flash *flash;
> +	struct spi_nor *nor;
> +	int ret = 0;
> +	char modalias[40] = {0};
> +	struct spi_nor_hwcaps hwcaps = {
> +		.mask = SNOR_HWCAPS_READ |
> +			SNOR_HWCAPS_READ_FAST |
> +			SNOR_HWCAPS_READ_1_1_2 |
> +			SNOR_HWCAPS_READ_1_1_4 |
> +			SNOR_HWCAPS_PP,
> +	};

since aletera_quadspi_{read|erase} just don't care about
nor->read_opcode, nor->program_opcode and so on and anyway override all
settings chosen by spi-nor.c, it means they will use Dual or Quad SPI
controllers as they want, whether SNOR_HWCAPS_READ_1_1_{2|4} are set or not.
Then I think it's risky to declare the READ_1_1_2 and READ_1_1_4 hwcaps
because it may trigger additionnal calls of nor->read_reg() /
nor->write_reg() from spi_nor_scan() with op codes not supported by
altera_quadspi_{read|write}_reg().

> +
> +	if (bank > q->num_flashes - 1)
> +		return -EINVAL;
> +
> +	altera_quadspi_chip_select(q, bank);
> +
> +	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
> +	if (!flash)
> +		return -ENOMEM;
> +
> +	q->flash[bank] = flash;
> +	nor = &flash->nor;
> +	nor->dev = dev;
> +	nor->priv = flash;
> +	nor->mtd.priv = nor;
> +	flash->q = q;
> +	flash->bank = bank;
> +	spi_nor_set_flash_node(nor, np);
> +
> +	/* spi nor framework*/
> +	nor->read_reg = altera_quadspi_read_reg;
> +	nor->write_reg = altera_quadspi_write_reg;
> +	nor->read = altera_quadspi_read;
> +	nor->write = altera_quadspi_write;
> +	nor->erase = altera_quadspi_erase;
> +	nor->flash_lock = altera_quadspi_lock;
> +	nor->flash_unlock = altera_quadspi_unlock;

nor->flash_lock and nor->flash_unlock are described as "FLASH SPECIFIC"
in include/linux/mtd/spi-nor.h as opposed to "DRIVER SPECIFIC" functions
like nor->read, nor->read_reg, ...

It means the actual implementations should be provided by the spi-nor
sub-system but not by each SPI controller driver.



For me, it really sounds like a bad idea that this driver tries so much
to mystify the spi-nor sub-system.

I can understand that you have to cope with the hardware design and its
limitations but clearly it looks the spi-nor API is not suited to this
hardware. This driver ignores and by-passes any settings selected by
spi_nor_scan().
Duplicating code is generally a bad idea but in this case, I don't know
if trying to reuse spi_nor_read() / spi_nor_write() and spi_nor_erase()
from spi-nor.c is that helpful.

Why not directly plug your driver into the above mtd layer implementing
you own version of mtd->_read(), mtd->_write() and mtd->_erase() then
registering the mtd device? It may be not the way to go but at least we
should study this alternative.

*IF* I have understood the hardware constraints of the Altera Quad SPI
controller and what this driver tries to discover by calling
spi_nor_scan(), I think you actually don't need spi_nor_scan() but only
to have access to the spi_nor_ids[] array to initialize mtd->erasesize
with info->sector_size. Am I right?

Then why not just exporting spi_nor_ids[] so your driver could directly
search into this table to get the pieces of information it needs?

I'm not confortable with the idea of a driver pretending to be compliant
with the API of spi-nor when obviously it is not.

Even if the Altera hardware looks really limited, I think we can still
try to help you finding a solution to add its support into mainline but
I would like to find a solution with less "hacks" because I'm pretty
sure that if we let pass a driver like this one, it would often be
broken and it would be a real pain to maintain.

Besides, I generally claim that SPI controller drivers should not use
SPINOR_OP_* macros because when they do so, they almost always try to
implement some kind of hack/by-pass of the spi-nor sub-system and the
result is maintainance issues when we update or add new features in
spi-nor.{c|h}.

IMHO, this driver goes in the wrong direction. Marek, do have an idea of
a better/cleaner solution to add support to Altera hardware in mainline?

Best regards,

Cyrille

> +
> +	/* scanning flash and checking dev id */
> +#ifdef CONFIG_OF
> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
> +		return -EINVAL;
> +#endif
> +
> +	ret = spi_nor_scan(nor, modalias, &hwcaps);
> +	if (ret) {
> +		dev_err(nor->dev, "flash not found\n");
> +		return ret;
> +	}
> +
> +	ret =  mtd_device_register(&nor->mtd, NULL, 0);
> +
> +	altera_quadspi_unlock(nor, 0, 0);
> +
> +	return ret;
> +}
> +
> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
> +			  void __iomem *data_base, void __iomem *window_base,
> +			  size_t window_size, u32 flags)
> +{
> +	struct altera_quadspi *q;
> +
> +	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
> +	if (!q)
> +		return -ENOMEM;
> +
> +	q->dev = dev;
> +	q->csr_base = csr_base;
> +	q->data_base = data_base;
> +	q->window_base = window_base;
> +	q->window_size = window_size;
> +
> +	q->flags = flags;
> +
> +	dev_set_drvdata(dev, q);
> +
> +	dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(altera_quadspi_create);
> +
> +int altera_qspi_add_bank(struct device *dev,
> +			 u32 bank, struct device_node *np)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +
> +	if (q->num_flashes >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP)
> +		return -ENOMEM;
> +
> +	q->num_flashes++;
> +
> +	return altera_quadspi_setup_banks(dev, bank, np);
> +}
> +EXPORT_SYMBOL_GPL(altera_qspi_add_bank);
> +
> +int altera_quadspi_remove_banks(struct device *dev)
> +{
> +	struct altera_quadspi *q = dev_get_drvdata(dev);
> +	struct altera_quadspi_flash *flash;
> +	int i;
> +	int ret = 0;
> +
> +	/* clean up for all nor flash */
> +	for (i = 0; i < q->num_flashes; i++) {
> +		flash = q->flash[i];
> +		if (!flash)
> +			continue;
> +
> +		/* clean up mtd stuff */
> +		ret = mtd_device_unregister(&flash->nor.mtd);
> +		if (ret) {
> +			dev_err(dev, "error removing mtd\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(altera_quadspi_remove_banks);
> +
> +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 Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mtd/altera-quadspi.h b/include/linux/mtd/altera-quadspi.h
> new file mode 100644
> index 0000000..58f31ee
> --- /dev/null
> +++ b/include/linux/mtd/altera-quadspi.h
> @@ -0,0 +1,28 @@
> +/*
> + *
> + * Copyright 2017 Intel Corporation, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef __ALTERA_QUADSPI_H
> +#define __ALTERA_QUADSPI_H
> +
> +#include <linux/device.h>
> +
> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
> +
> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
> +
> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
> +			  void __iomem *data_base, void __iomem *window_reg,
> +			  size_t window_size, u32 flags);
> +
> +int altera_qspi_add_bank(struct device *dev,
> +			 u32 bank, struct device_node *np);
> +
> +int altera_quadspi_remove_banks(struct device *dev);
> +#endif
> 

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[Michal Suchanek]

On 4 July 2017 at 02:00, Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote:
> Hi Matthew,
>
>
> Le 26/06/2017 à 18:13, matthew.gerlach@linux.intel.com a écrit :
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

>> +static int altera_quadspi_setup_banks(struct device *dev,
>> +                                   u32 bank, struct device_node *np)
>> +{
>> +     struct altera_quadspi *q = dev_get_drvdata(dev);
>> +     struct altera_quadspi_flash *flash;
>> +     struct spi_nor *nor;
>> +     int ret = 0;
>> +     char modalias[40] = {0};
>> +     struct spi_nor_hwcaps hwcaps = {
>> +             .mask = SNOR_HWCAPS_READ |
>> +                     SNOR_HWCAPS_READ_FAST |
>> +                     SNOR_HWCAPS_READ_1_1_2 |
>> +                     SNOR_HWCAPS_READ_1_1_4 |
>> +                     SNOR_HWCAPS_PP,
>> +     };
>
> since aletera_quadspi_{read|erase} just don't care about
> nor->read_opcode, nor->program_opcode and so on and anyway override all
> settings chosen by spi-nor.c, it means they will use Dual or Quad SPI
> controllers as they want, whether SNOR_HWCAPS_READ_1_1_{2|4} are set or not.
> Then I think it's risky to declare the READ_1_1_2 and READ_1_1_4 hwcaps
> because it may trigger additionnal calls of nor->read_reg() /
> nor->write_reg() from spi_nor_scan() with op codes not supported by
> altera_quadspi_{read|write}_reg().
>
>> +
>> +     if (bank > q->num_flashes - 1)
>> +             return -EINVAL;
>> +
>> +     altera_quadspi_chip_select(q, bank);
>> +
>> +     flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
>> +     if (!flash)
>> +             return -ENOMEM;
>> +
>> +     q->flash[bank] = flash;
>> +     nor = &flash->nor;
>> +     nor->dev = dev;
>> +     nor->priv = flash;
>> +     nor->mtd.priv = nor;
>> +     flash->q = q;
>> +     flash->bank = bank;
>> +     spi_nor_set_flash_node(nor, np);
>> +
>> +     /* spi nor framework*/
>> +     nor->read_reg = altera_quadspi_read_reg;
>> +     nor->write_reg = altera_quadspi_write_reg;
>> +     nor->read = altera_quadspi_read;
>> +     nor->write = altera_quadspi_write;
>> +     nor->erase = altera_quadspi_erase;
>> +     nor->flash_lock = altera_quadspi_lock;
>> +     nor->flash_unlock = altera_quadspi_unlock;
>
> nor->flash_lock and nor->flash_unlock are described as "FLASH SPECIFIC"
> in include/linux/mtd/spi-nor.h as opposed to "DRIVER SPECIFIC" functions
> like nor->read, nor->read_reg, ...
>
> It means the actual implementations should be provided by the spi-nor
> sub-system but not by each SPI controller driver.
>
>
>
> For me, it really sounds like a bad idea that this driver tries so much
> to mystify the spi-nor sub-system.
>
> I can understand that you have to cope with the hardware design and its
> limitations but clearly it looks the spi-nor API is not suited to this
> hardware. This driver ignores and by-passes any settings selected by
> spi_nor_scan().
> Duplicating code is generally a bad idea but in this case, I don't know
> if trying to reuse spi_nor_read() / spi_nor_write() and spi_nor_erase()
> from spi-nor.c is that helpful.
>
> Why not directly plug your driver into the above mtd layer implementing
> you own version of mtd->_read(), mtd->_write() and mtd->_erase() then
> registering the mtd device? It may be not the way to go but at least we
> should study this alternative.

AFAICT fsl-quadspi does just that preventing the use of the SPI
controller for non-flash devices.

There is at least one accelerated driver that is passed the opcodes to
program in the controller for read acceleration in spi_flash_read so
reusing that should be viable. If the opcodes can be programmed or
match what is hardcoded in the controller use the acceleration and
fallback to plain spi transfer if there is mismatch between what
m25p80_read requests and what the controller can do.

If this works and you can still use the plain SPI trnsfers the
controller will be much morer useful than fsl-quadspi.

Thanks

Michal

Re: [PATCH 2/3] mtd: spi-nor: core code for the Altera Quadspi Flash Controller v2

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 29166 bytes --]



On Tue, 4 Jul 2017, Cyrille Pitchen wrote:

Hi Cyrille,

Thanks for all the great feedback.  Clearly, I've got some work to do.
Please see my comments inline.

Matthew Gerlach


> Hi Matthew,
>
>
> Le 26/06/2017 à 18:13, matthew.gerlach@linux.intel.com a écrit :
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  MAINTAINERS                          |   7 +
>>  drivers/mtd/spi-nor/Kconfig          |   5 +
>>  drivers/mtd/spi-nor/Makefile         |   4 +-
>>  drivers/mtd/spi-nor/altera-quadspi.c | 676 +++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/altera-quadspi.h   |  28 ++
>>  5 files changed, 719 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mtd/spi-nor/altera-quadspi.c
>>  create mode 100644 include/linux/mtd/altera-quadspi.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6b4395c..ae33fa6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -648,6 +648,13 @@ L:	linux-gpio@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/gpio/gpio-altera.c
>>
>> +ALTERA QUADSPI FLASH DRIVER
>> +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:	inclulde/linux/mtd/altera-quadspi.h
>> +
>>  ALTERA SYSTEM RESOURCE DRIVER FOR ARRIA10 DEVKIT
>>  M:	Thor Thayer <thor.thayer@linux.intel.com>
>>  S:	Maintained
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 293c8a4..89fe425 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -113,4 +113,9 @@ config SPI_STM32_QUADSPI
>>  	  This enables support for the STM32 Quad SPI controller.
>>  	  We only connect the NOR to this controller.
>>
>> +config SPI_ALTERA_QUADSPI
>> +        tristate "Altera Quad SPI Flash Controller II"
>> +        help
>> +          Enable support for version 2 of Altera Quad SPI Flash Controller.
>> +
>>  endif # MTD_SPI_NOR
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 285aab8..024c6ac 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -8,4 +8,6 @@ obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
>>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
>>  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
>> \ No newline at end of file
>> +obj-$(CONFIG_SPI_STM32_QUADSPI)	+= stm32-quadspi.o
>> +obj-$(CONFIG_SPI_ALTERA_QUADSPI) += altera-quadspi.o
>> +
>> diff --git a/drivers/mtd/spi-nor/altera-quadspi.c b/drivers/mtd/spi-nor/altera-quadspi.c
>> new file mode 100644
>> index 0000000..de65453
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/altera-quadspi.c
>> @@ -0,0 +1,676 @@
>> +/*
>> + * 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/bitrev.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/altera-quadspi.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/spi-nor.h>
>> +
>> +#define ALTERA_QUADSPI_RESOURCE_NAME			"altera_quadspi"
>> +
>> +#define EPCS_OPCODE_ID					1
>> +#define NON_EPCS_OPCODE_ID				2
>> +
>> +#define WRITE_CHECK					1
>> +#define ERASE_CHECK					0
>> +
>> +#define QUADSPI_SR_REG					0x0
>> +#define QUADSPI_SR_MASK					0x0000000F
>> +
>> +/* defines for device id register */
>> +#define QUADSPI_SID_REG					0x4
>> +#define QUADSPI_RDID_REG				0x8
>> +#define QUADSPI_ID_MASK					0x000000FF
>> +
>> +/*
>> + * QUADSPI_MEM_OP register offset
>> + *
>> + * The QUADSPI_MEM_OP register is used to do memory protect and erase operations
>> + *
>> + */
>> +#define QUADSPI_MEM_OP_REG				0xC
>> +
>> +#define QUADSPI_MEM_OP_CMD_MASK				0x00000003
>> +#define QUADSPI_MEM_OP_BULK_ERASE_CMD			0x00000001
>> +#define QUADSPI_MEM_OP_SECTOR_ERASE_CMD			0x00000002
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_CMD		0x00000003
>> +#define QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD		0x00000004
>> +#define QUADSPI_MEM_OP_SECTOR_VALUE_MASK		0x0003FF00
>> +
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_SHIFT		8
>> +#define QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK	0x00001F00
>> +/*
>> + * QUADSPI_ISR register offset
>> + *
>> + * The QUADSPI_ISR register is used to determine whether an invalid write or
>> + * erase operation trigerred an interrupt
>> + *
>> + */
>> +#define QUADSPI_ISR_REG					0x10
>> +
>> +#define QUADSPI_ISR_ILLEGAL_ERASE_MASK			0x00000001
>> +#define QUADSPI_ISR_ILLEGAL_WRITE_MASK			0x00000002
>> +
>> +/*
>> + * QUADSPI_IMR register offset
>> + *
>> + * The QUADSPI_IMR register is used to mask the invalid erase or the invalid
>> + * write interrupts.
>> + *
>> + */
>> +#define QUADSPI_IMR_REG					0x14
>> +#define QUADSPI_IMR_ILLEGAL_ERASE_MASK			0x00000001
>> +
>> +#define QUADSPI_IMR_ILLEGAL_WRITE_MASK			0x00000002
>> +
>> +#define QUADSPI_CHIP_SELECT_REG				0x18
>> +#define QUADSPI_CHIP_SELECT_MASK			0x00000007
>> +#define QUADSPI_CHIP_SELECT_0				0x00000001
>> +#define QUADSPI_CHIP_SELECT_1				0x00000002
>> +#define QUADSPI_CHIP_SELECT_2				0x00000004
>> +
>> +#define QUADSPI_FLAG_STATUS_REG				0x1C
>> +#define QUADSPI_DEV_ID_DATA_0				0x20
>> +#define QUADSPI_DEV_ID_DATA_1				0x24
>> +#define QUADSPI_DEV_ID_DATA_2				0x28
>> +#define QUADSPI_DEV_ID_DATA_3				0x2C
>> +#define QUADSPI_DEV_ID_DATA_4				0x30
>> +
>> +#define QUADSPI_WIN_OCC_REG				0x4
>> +#define QUADSPI_WIN_OCC_SFT				24
>> +
>> +#define QUADSPI_WIN_SEL_REG				0x8
>> +
>> +struct altera_quadspi {
>> +	u32 opcode_id;
>> +	void __iomem *csr_base;
>> +	void __iomem *data_base;
>> +	void __iomem *window_base;
>> +	size_t window_size;
>> +	u32 num_flashes;
>> +	u32 flags;
>> +	struct device *dev;
>> +	struct altera_quadspi_flash *flash[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
>> +	struct device_node *np[ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP];
>> +};
>> +
>> +struct altera_quadspi_flash {
>> +	struct spi_nor nor;
>> +	struct altera_quadspi *q;
>> +	u32 bank;
>> +};
>> +
>> +struct flash_device {
>> +	char *name;
>> +	u32 opcode_id;
>> +	u32 device_id;
>> +};
>> +
>> +#ifdef DEBUG
>> +static inline u32 alt_qspi_readl(void __iomem *base, off_t offset)
>> +{
>> +	u32 val = readl(base + offset);
>> +
>> +	pr_info("%s 0x%x from offset 0x%lx\n", __func__, val, offset);
>> +	return val;
>> +}
>> +static inline void alt_qspi_writel(u32 val, void __iomem *base, off_t offset)
>> +{
>> +	writel(val, base + offset);
>> +	pr_info("%s 0x%x to offset 0x%lx\n", __func__, val, offset);
>> +}
>> +#else
>> +#define alt_qspi_readl(base, offset) readl(base+offset)
>> +#define alt_qspi_writel(val, base, offset) writel(val, base + offset)
>> +#endif
>> +
>> +static void altera_quadspi_chip_select(struct altera_quadspi *q, u32 bank)
>> +{
>> +	u32 val = 0;
>> +
>> +	switch (bank) {
>> +	case 0:
>> +		val = QUADSPI_CHIP_SELECT_0;
>> +		break;
>> +	case 1:
>> +		val = QUADSPI_CHIP_SELECT_1;
>> +		break;
>> +	case 2:
>> +		val = QUADSPI_CHIP_SELECT_2;
>> +		break;
>> +	default:
>> +		dev_err(q->dev, "invalid bank\n");
>> +		return;
>> +	}
>> +	alt_qspi_writel(val, q->csr_base, QUADSPI_CHIP_SELECT_REG);
>> +}
>> +
>> +static int altera_quadspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *val,
>> +				    int len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	switch (opcode) {
>> +	case SPINOR_OP_WREN:
>> +		dev_dbg(q->dev, "%s enabling write\n", __func__);
>> +		alt_qspi_writel(QUADSPI_MEM_OP_SECTOR_WRITE_ENABLE_CMD,
>> +				q->csr_base, QUADSPI_MEM_OP_REG);
>> +		break;
>> +
>> +	case SPINOR_OP_CHIP_ERASE:
>> +		alt_qspi_writel(QUADSPI_MEM_OP_BULK_ERASE_CMD,
>> +				q->csr_base, QUADSPI_MEM_OP_REG);
>> +		break;
>> +
>> +	default:
>> +		dev_dbg(q->dev, "%s UNHANDLED write_reg 0x%x\n",
>> +			__func__, opcode);
>
> Looking at the code I assume, the hardware can only send
> predefined/hard-coded SPI op codes but not those actually chosen by
> spi-nor.c so what will happen when spi-nor introduces new op codes?
> Does it mean that this driver will immediately be broken?

Good point.  I will have to investigate this problem.

>
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *val,
>> +				   int len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 data = 0;
>> +
>> +	memset(val, 0, len);
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	switch (opcode) {
>> +	case SPINOR_OP_RDSR:
>> +		data = alt_qspi_readl(q->csr_base, QUADSPI_SR_REG);
>> +		dev_dbg(q->dev, "%s RDSR 0x%x\n", __func__, data);
>> +		*val = (u8)data & QUADSPI_SR_MASK;
>> +		break;
>> +	case SPINOR_OP_RDID:
>> +		if (q->opcode_id == EPCS_OPCODE_ID)
>
> Where do you initialize opcode_id ?
>
>> +			data = alt_qspi_readl(q->csr_base, QUADSPI_SID_REG);
>> +		else
>> +			data = alt_qspi_readl(q->csr_base, QUADSPI_RDID_REG);
>
> Why 2 differents registers for the JEDEC ID? Is data actually the result
> of some READ JEDEC ID (9Fh) command or some hard-coded value?

This is some left over code from when the driver was trying to support 
both original version of the Altera Quadspi controller and version 2 of 
the component.  It should be removed.

>
>> +
>> +		*((u32 *)val) = data;
>> +		break;
>> +	case SPINOR_OP_RDFSR:
>> +		data = alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG);
>> +		dev_dbg(q->dev, "%s RDFSR 0x%x\n", __func__, data);
>> +		*val = (u8)(data & 0xff);
>> +		break;
>
> This is a "Micron only" register, other chunks of this patch let me
> think that the Altera controller can only work this Micron/EPCS
> memories, am I wrong?

My understanding is that the component should be able to support 
non-Micron devices, but I could be wrong.  I will have to investigate 
more.

>
> OK, then let assume a Micron memory: what if one day spi-nor.c needs to
> read the VCR and/or EVCR registers of Micron memories to check some
> specific bits? This driver is very likely to fail.

Clearly, there is a problem here.

>
>> +	default:
>> +		dev_dbg(q->dev, "%s UNHANDLED read_reg 0x%x\n",
>> +			__func__, opcode);
>> +		*val = 0;
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_write_erase_check(struct spi_nor *nor,
>> +					    bool write_erase)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 val;
>> +	u32 mask;
>> +
>> +	if (write_erase)
>> +		mask = QUADSPI_ISR_ILLEGAL_WRITE_MASK;
>> +	else
>> +		mask = QUADSPI_ISR_ILLEGAL_ERASE_MASK;
>> +
>> +	val = alt_qspi_readl(q->csr_base, QUADSPI_ISR_REG);
>> +
>> +	if (val & mask) {
>> +		dev_err(nor->dev,
>> +			"write/erase failed, sector might be protected\n");
>> +		alt_qspi_writel(0, q->csr_base, QUADSPI_FLAG_STATUS_REG);
>> +
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_addr_to_sector(struct mtd_info *mtd, uint64_t offset)
>> +{
>> +	if (mtd->erasesize_shift)
>> +		return offset >> mtd->erasesize_shift;
>> +	do_div(offset, mtd->erasesize);
>> +	return offset;
>> +}
>> +
>> +static int altera_quadspi_erase(struct spi_nor *nor, loff_t offset)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	struct mtd_info *mtd = &nor->mtd;
>> +	u32 val;
>> +	int sector_value;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	sector_value = altera_quadspi_addr_to_sector(mtd, offset);
>
> Here the driver uses nor->mtd.erasesize to do some conversion of the
> offset into some kind of sector index.
> However this function doesn't care about nor->erase_opcode. Then I guess
> that the hardware can only use a hard-coded op code for Block/Sector
> Erase operations.
>
> I don't know which erase op code is actually used by the hardware but
> whatever... How do you know that you've computed the right sector index
> before asking the hardware to execute some erase operation since this
> index is based on nor->mtd.erasesize ?
>
> nor->mtd.erasesize depends on the chosen nor->erase_opcode.
> For instance, depending on whether CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is
> defined or not, there will be at least one case where nor->erase_opcode
> doesn't match the hard-coded op code used by the Altera controller:
> different op codes, different sector sizes, different sector indexes...
>
> buggy nor->erase() function

Again, I will have to investigate more.

>
>
>> +
>> +	dev_dbg(q->dev, "%s sector %d\n", __func__, sector_value);
>> +
>> +	if (sector_value < 0)
>> +		return -EINVAL;
>> +
>> +	val = (sector_value << 8) & QUADSPI_MEM_OP_SECTOR_VALUE_MASK;
>> +
>> +	val |= QUADSPI_MEM_OP_SECTOR_ERASE_CMD;
>> +
>> +	alt_qspi_writel(val, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	dev_dbg(q->dev, "%s SR=0x%x FSR=0x%x\n", __func__,
>> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
>> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
>> +
>> +	return altera_quadspi_write_erase_check(nor, ERASE_CHECK);
>> +}
>> +
>> +#define WINDOW_ALIGN 4
>> +#define WINDOW_MASK (WINDOW_ALIGN - 1)
>> +
>> +static ssize_t altera_quadspi_windowed_read(struct altera_quadspi *q,
>> +					    loff_t from,
>> +					    size_t len, u_char *buf)
>> +{
>> +	size_t bytes_left = len;
>> +	size_t bytes_to_read, i;
>> +	loff_t next_window_off;
>> +	u64 start_window;
>> +	u32 window;
>> +	u32 *dst;
>> +
>> +	if ((from & WINDOW_MASK) || (len & WINDOW_MASK) ||
>> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
>> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
>> +			__func__);
>> +		return 0;
>> +	}
>> +
>> +	start_window = from;
>> +	do_div(start_window, q->window_size);
>> +	window = (u32)(start_window & 0xffffffff);
>> +
>> +	next_window_off = (window + 1) * q->window_size;
>> +
>> +	while (bytes_left > 0) {
>> +
>> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
>> +
>> +		bytes_to_read = min((size_t)bytes_left,
>> +				    (size_t)(next_window_off - from));
>> +
>> +		dev_dbg(q->dev,
>> +			"window%u fr0x%llx next0x%llx left%zu num0x%zx\n",
>> +			window,  from, next_window_off, bytes_left,
>> +			bytes_to_read);
>> +
>> +		dst = (u32 *)buf;
>> +		for (i = 0; i < bytes_to_read; i += 4, dst++)
>> +			*dst = readl(q->data_base +
>> +				     (from & (q->window_size - 1)) + i);
>> +
>> +		bytes_left -= bytes_to_read;
>> +		buf += bytes_to_read;
>> +		from += bytes_to_read;
>> +		window++;
>> +		next_window_off += q->window_size;
>> +	}
>> +
>> +	return len;
>> +}
>> +static ssize_t altera_quadspi_windowed_write(struct altera_quadspi *q,
>> +					     loff_t to, size_t len,
>> +					     const u_char *buf)
>> +{
>> +	size_t bytes_left = len;
>> +	u32 window_mask = q->window_size - 1;
>> +	u32 read_back;
>> +	size_t bytes_to_write, i;
>> +	loff_t next_window_off;
>> +	u64 start_window;
>> +	u32 window;
>> +	const u32 *src;
>> +	u32 words_can_write;
>> +
>> +	if ((to & WINDOW_MASK) || (len & WINDOW_MASK) ||
>> +	    !IS_ALIGNED((unsigned long)buf, WINDOW_ALIGN)) {
>> +		dev_err(q->dev, "%s only 32 bit aligned accesses allowed\n",
>> +			__func__);
>> +		return 0;
>> +	}
>> +
>> +	start_window = to;
>> +	do_div(start_window, q->window_size);
>> +	window = (u32)(start_window & 0xffffffff);
>> +
>> +	next_window_off = (window + 1) * q->window_size;
>> +
>> +	while (bytes_left > 0) {
>> +
>> +		writel(window, q->window_base + QUADSPI_WIN_SEL_REG);
>> +
>> +		bytes_to_write = min((size_t)bytes_left,
>> +				    (size_t)(next_window_off - to));
>> +
>> +		dev_dbg(q->dev,
>> +			"window%u to0x%llx next0x%llx left%zu num0x%zx\n",
>> +			window,  to, next_window_off, bytes_left,
>> +			bytes_to_write);
>> +
>> +		src = (u32 *)buf;
>> +		for (i = 0; i < bytes_to_write;) {
>> +			words_can_write =
>> +				readl(q->window_base + QUADSPI_WIN_OCC_REG) >>
>> +				      QUADSPI_WIN_OCC_SFT;
>> +			dev_dbg(q->dev, "can write 0x%x\n", words_can_write);
>> +
>> +			for (; words_can_write > 0; words_can_write--) {
>> +				writel(*src,
>> +				       q->data_base +
>> +				       (to & window_mask) + i);
>> +				read_back = readl(q->data_base +
>> +						  (to & window_mask) + i);
>> +				if (*src != read_back) {
>> +					dev_err(q->dev, "%s 0x%x != 0x%x\n",
>> +						__func__, *src, read_back);
>> +					return (len - bytes_left);
>> +				}
>> +				i += 4;
>> +				src++;
>> +			}
>> +		}
>> +
>> +		bytes_left -= bytes_to_write;
>> +		buf += bytes_to_write;
>> +		to += bytes_to_write;
>> +		window++;
>> +		next_window_off += q->window_size;
>> +	}
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t altera_quadspi_read(struct spi_nor *nor, loff_t from, size_t len,
>> +				   u_char *buf)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	size_t i;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	if (q->window_size)
>> +		altera_quadspi_windowed_read(q, from, len, buf);
>> +	else
>> +		memcpy_fromio(buf, q->data_base + from, len);
>> +
>> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_READ) {
>> +		for (i = 0; i < len; i++, buf++)
>> +			*buf = bitrev8(*buf);
>> +	}
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t altera_quadspi_write(struct spi_nor *nor, loff_t to,
>> +				    size_t len, const u_char *buf)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u_char *bitrev_buf = NULL;
>> +	const u_char *src;
>> +	u_char *dst;
>> +	size_t i;
>> +	int ret = 0;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	if (q->flags & ALTERA_QUADSPI_FL_BITREV_WRITE) {
>> +		bitrev_buf = devm_kzalloc(q->dev, len, GFP_KERNEL);
>> +		if (!bitrev_buf)
>> +			return 0;
>> +
>> +		src = buf;
>> +		dst = bitrev_buf;
>> +		for (i = 0; i < len; i++, src++, dst++)
>> +			*dst = bitrev8(*src);
>> +
>> +		buf = bitrev_buf;
>> +	}
>> +
>> +	if (q->window_size)
>> +		altera_quadspi_windowed_write(q, to, len, buf);
>> +	else
>> +		memcpy_toio(q->data_base + to, buf, len);
>> +
>> +
>> +	if (bitrev_buf)
>> +		devm_kfree(q->dev, bitrev_buf);
>> +
>> +	ret = altera_quadspi_write_erase_check(nor, WRITE_CHECK);
>> +
>> +	return len;
>> +
>> +}
>> +
>> +static int altera_quadspi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	struct mtd_info *mtd = &nor->mtd;
>> +	uint32_t offset = ofs;
>> +	u32 sector_start, sector_end;
>> +	uint64_t num_sectors;
>> +	u32 mem_op;
>> +	u32 sr_bp;
>> +	u32 sr_tb;
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	sector_start = offset;
>> +	sector_end = altera_quadspi_addr_to_sector(mtd, offset + len);
>> +	num_sectors = mtd->size;
>> +	do_div(num_sectors, mtd->erasesize);
>> +
>> +	dev_dbg(nor->dev, "%s: sector start is %u,sector end is %u\n",
>> +		__func__, sector_start, sector_end);
>> +
>> +	if (sector_start >= num_sectors / 2) {
>> +		sr_bp = fls(num_sectors - 1 - sector_start) + 1;
>> +		sr_tb = 0;
>> +	} else if ((sector_end < num_sectors / 2) &&
>> +		  (q->opcode_id != EPCS_OPCODE_ID)) {
>> +		sr_bp = fls(sector_end) + 1;
>> +		sr_tb = 1;
>> +	} else {
>> +		sr_bp = 16;
>> +		sr_tb = 0;
>> +	}
>> +
>> +	mem_op = (sr_tb << 12) | (sr_bp << 8);
>> +	mem_op &= QUADSPI_MEM_OP_SECTOR_PROTECT_VALUE_MASK;
>> +	mem_op |= QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
>> +
>> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +	struct altera_quadspi_flash *flash = nor->priv;
>> +	struct altera_quadspi *q = flash->q;
>> +	u32 mem_op;
>> +
>> +	dev_dbg(nor->dev, "Unlock all protected area\n");
>> +
>> +	altera_quadspi_chip_select(q, flash->bank);
>> +
>> +	mem_op = QUADSPI_MEM_OP_SECTOR_PROTECT_CMD;
>> +	alt_qspi_writel(mem_op, q->csr_base, QUADSPI_MEM_OP_REG);
>> +
>> +	return 0;
>> +}
>> +
>> +static int altera_quadspi_setup_banks(struct device *dev,
>> +				      u32 bank, struct device_node *np)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +	struct altera_quadspi_flash *flash;
>> +	struct spi_nor *nor;
>> +	int ret = 0;
>> +	char modalias[40] = {0};
>> +	struct spi_nor_hwcaps hwcaps = {
>> +		.mask = SNOR_HWCAPS_READ |
>> +			SNOR_HWCAPS_READ_FAST |
>> +			SNOR_HWCAPS_READ_1_1_2 |
>> +			SNOR_HWCAPS_READ_1_1_4 |
>> +			SNOR_HWCAPS_PP,
>> +	};
>
> since aletera_quadspi_{read|erase} just don't care about
> nor->read_opcode, nor->program_opcode and so on and anyway override all
> settings chosen by spi-nor.c, it means they will use Dual or Quad SPI
> controllers as they want, whether SNOR_HWCAPS_READ_1_1_{2|4} are set or not.
> Then I think it's risky to declare the READ_1_1_2 and READ_1_1_4 hwcaps
> because it may trigger additionnal calls of nor->read_reg() /
> nor->write_reg() from spi_nor_scan() with op codes not supported by
> altera_quadspi_{read|write}_reg().
>
>> +
>> +	if (bank > q->num_flashes - 1)
>> +		return -EINVAL;
>> +
>> +	altera_quadspi_chip_select(q, bank);
>> +
>> +	flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
>> +	if (!flash)
>> +		return -ENOMEM;
>> +
>> +	q->flash[bank] = flash;
>> +	nor = &flash->nor;
>> +	nor->dev = dev;
>> +	nor->priv = flash;
>> +	nor->mtd.priv = nor;
>> +	flash->q = q;
>> +	flash->bank = bank;
>> +	spi_nor_set_flash_node(nor, np);
>> +
>> +	/* spi nor framework*/
>> +	nor->read_reg = altera_quadspi_read_reg;
>> +	nor->write_reg = altera_quadspi_write_reg;
>> +	nor->read = altera_quadspi_read;
>> +	nor->write = altera_quadspi_write;
>> +	nor->erase = altera_quadspi_erase;
>> +	nor->flash_lock = altera_quadspi_lock;
>> +	nor->flash_unlock = altera_quadspi_unlock;
>
> nor->flash_lock and nor->flash_unlock are described as "FLASH SPECIFIC"
> in include/linux/mtd/spi-nor.h as opposed to "DRIVER SPECIFIC" functions
> like nor->read, nor->read_reg, ...
>
> It means the actual implementations should be provided by the spi-nor
> sub-system but not by each SPI controller driver.
>
>
>
> For me, it really sounds like a bad idea that this driver tries so much
> to mystify the spi-nor sub-system.

I aggree.  This driver is doing to much of the work that the spi layer 
should be doing.  It is in my best interest to let the spi-nor layer do as 
much as possible.

>
> I can understand that you have to cope with the hardware design and its
> limitations but clearly it looks the spi-nor API is not suited to this
> hardware. This driver ignores and by-passes any settings selected by
> spi_nor_scan().
> Duplicating code is generally a bad idea but in this case, I don't know
> if trying to reuse spi_nor_read() / spi_nor_write() and spi_nor_erase()
> from spi-nor.c is that helpful.

The component is an FPGA component; so i should be able to get it "fixed" 
to work properly.

>
> Why not directly plug your driver into the above mtd layer implementing
> you own version of mtd->_read(), mtd->_write() and mtd->_erase() then
> registering the mtd device? It may be not the way to go but at least we
> should study this alternative.

I aggree this alternative should be explored.


>
> *IF* I have understood the hardware constraints of the Altera Quad SPI
> controller and what this driver tries to discover by calling
> spi_nor_scan(), I think you actually don't need spi_nor_scan() but only
> to have access to the spi_nor_ids[] array to initialize mtd->erasesize
> with info->sector_size. Am I right?
>
> Then why not just exporting spi_nor_ids[] so your driver could directly
> search into this table to get the pieces of information it needs?
>
> I'm not confortable with the idea of a driver pretending to be compliant
> with the API of spi-nor when obviously it is not.
>
> Even if the Altera hardware looks really limited, I think we can still
> try to help you finding a solution to add its support into mainline but
> I would like to find a solution with less "hacks" because I'm pretty
> sure that if we let pass a driver like this one, it would often be
> broken and it would be a real pain to maintain.

I would like to find a solution with less "hacks" too because I don't want 
to spend a lot of time maintaining the hacks.

>
> Besides, I generally claim that SPI controller drivers should not use
> SPINOR_OP_* macros because when they do so, they almost always try to
> implement some kind of hack/by-pass of the spi-nor sub-system and the
> result is maintainance issues when we update or add new features in
> spi-nor.{c|h}.
>
> IMHO, this driver goes in the wrong direction. Marek, do have an idea of
> a better/cleaner solution to add support to Altera hardware in mainline?

OK, I get that this going in the wrong.  I would like to "turn it around" 
and go in the right direction.

>
> Best regards,
>
> Cyrille
>
>> +
>> +	/* scanning flash and checking dev id */
>> +#ifdef CONFIG_OF
>> +	if (np && (of_modalias_node(np, modalias, sizeof(modalias)) < 0))
>> +		return -EINVAL;
>> +#endif
>> +
>> +	ret = spi_nor_scan(nor, modalias, &hwcaps);
>> +	if (ret) {
>> +		dev_err(nor->dev, "flash not found\n");
>> +		return ret;
>> +	}
>> +
>> +	ret =  mtd_device_register(&nor->mtd, NULL, 0);
>> +
>> +	altera_quadspi_unlock(nor, 0, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>> +			  void __iomem *data_base, void __iomem *window_base,
>> +			  size_t window_size, u32 flags)
>> +{
>> +	struct altera_quadspi *q;
>> +
>> +	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>> +	if (!q)
>> +		return -ENOMEM;
>> +
>> +	q->dev = dev;
>> +	q->csr_base = csr_base;
>> +	q->data_base = data_base;
>> +	q->window_base = window_base;
>> +	q->window_size = window_size;
>> +
>> +	q->flags = flags;
>> +
>> +	dev_set_drvdata(dev, q);
>> +
>> +	dev_dbg(dev, "%s SR=0x%x FSR=0x%x\n", __func__,
>> +		alt_qspi_readl(q->csr_base, QUADSPI_SR_REG),
>> +		alt_qspi_readl(q->csr_base, QUADSPI_FLAG_STATUS_REG));
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_quadspi_create);
>> +
>> +int altera_qspi_add_bank(struct device *dev,
>> +			 u32 bank, struct device_node *np)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +
>> +	if (q->num_flashes >= ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP)
>> +		return -ENOMEM;
>> +
>> +	q->num_flashes++;
>> +
>> +	return altera_quadspi_setup_banks(dev, bank, np);
>> +}
>> +EXPORT_SYMBOL_GPL(altera_qspi_add_bank);
>> +
>> +int altera_quadspi_remove_banks(struct device *dev)
>> +{
>> +	struct altera_quadspi *q = dev_get_drvdata(dev);
>> +	struct altera_quadspi_flash *flash;
>> +	int i;
>> +	int ret = 0;
>> +
>> +	/* clean up for all nor flash */
>> +	for (i = 0; i < q->num_flashes; i++) {
>> +		flash = q->flash[i];
>> +		if (!flash)
>> +			continue;
>> +
>> +		/* clean up mtd stuff */
>> +		ret = mtd_device_unregister(&flash->nor.mtd);
>> +		if (ret) {
>> +			dev_err(dev, "error removing mtd\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(altera_quadspi_remove_banks);
>> +
>> +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 Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mtd/altera-quadspi.h b/include/linux/mtd/altera-quadspi.h
>> new file mode 100644
>> index 0000000..58f31ee
>> --- /dev/null
>> +++ b/include/linux/mtd/altera-quadspi.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + *
>> + * Copyright 2017 Intel Corporation, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#ifndef __ALTERA_QUADSPI_H
>> +#define __ALTERA_QUADSPI_H
>> +
>> +#include <linux/device.h>
>> +
>> +#define ALTERA_QUADSPI_FL_BITREV_READ BIT(0)
>> +#define ALTERA_QUADSPI_FL_BITREV_WRITE BIT(1)
>> +
>> +#define ALTERA_QUADSPI_MAX_NUM_FLASH_CHIP 3
>> +
>> +int altera_quadspi_create(struct device *dev, void __iomem *csr_base,
>> +			  void __iomem *data_base, void __iomem *window_reg,
>> +			  size_t window_size, u32 flags);
>> +
>> +int altera_qspi_add_bank(struct device *dev,
>> +			 u32 bank, struct device_node *np);
>> +
>> +int altera_quadspi_remove_banks(struct device *dev);
>> +#endif
>>
>
>

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

[matthew.gerlach]

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

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

[Marek Vasut]

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

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

[matthew.gerlach]

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
>

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

[Marek Vasut]

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

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

[matthew.gerlach]

On Tue, 27 Jun 2017, Marek Vasut wrote:

> 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 ?

You probably would not see a PCIe driver for a card with a FPGA that would 
only have the Altera Quadspi component on it.  Usually a FPGA has several 
components, each requiring their own driver which are considered 
sub-drivers of the PCIe driver.  I will be adding Altera Quadspi support 
to the Intel-FPGA PCIe driver that is currently under review.  We have 
also seen people use the Altera Quadspi with a NIOS-II soft processor, and 
I expect someone would want to use the component with ARM SOCFPGAs.

Matthew Gerlach

>
>> 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
>

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

Hi Matthew,

[auto build test ERROR on spi-nor/next]
[also build test ERROR on next-20170627]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/matthew-gerlach-linux-intel-com/Altera-Quadspi-Controller-Version-2/20170627-120604
base:   git://github.com/spi-nor/linux next
config: um-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um 

All errors (new ones prefixed by >>):

   arch/um/drivers/built-in.o: In function `vde_open_real':
   (.text+0xc9f1): warning: Using 'getgrnam' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `vde_open_real':
   (.text+0xc83c): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `vde_open_real':
   (.text+0xcb55): warning: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `pcap_nametoaddr':
   (.text+0x1d5e5): warning: Using 'gethostbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `pcap_nametonetaddr':
   (.text+0x1d685): warning: Using 'getnetbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `pcap_nametoproto':
   (.text+0x1d8a5): warning: Using 'getprotobyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   arch/um/drivers/built-in.o: In function `pcap_nametoport':
   (.text+0x1d6d7): warning: Using 'getservbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
   drivers/built-in.o: In function `altera_quadspi_probe':
>> drivers/mtd/spi-nor/altera-quadspi-platform.c:44: undefined reference to `devm_ioremap_resource'
   drivers/mtd/spi-nor/altera-quadspi-platform.c:51: undefined reference to `devm_ioremap_resource'
   drivers/mtd/spi-nor/altera-quadspi-platform.c:60: undefined reference to `devm_ioremap_resource'
   drivers/built-in.o: In function `img_ascii_lcd_probe':
   drivers/auxdisplay/img-ascii-lcd.c:386: undefined reference to `devm_ioremap_resource'
   collect2: error: ld returned 1 exit status

vim +44 drivers/mtd/spi-nor/altera-quadspi-platform.c

    38		if (!np) {
    39			dev_err(dev, "no device found\n");
    40			return -ENODEV;
    41		}
    42	
    43		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "avl_csr");
  > 44		csr_base = devm_ioremap_resource(dev, res);
    45		if (IS_ERR(csr_base)) {
    46			dev_err(dev, "%s: ERROR: failed to map csr base\n", __func__);
    47			return PTR_ERR(csr_base);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19420 bytes --]