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

[Marek Vasut]

On 06/27/2017 04:32 PM, matthew.gerlach@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx wrote:
>>> From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxx>
>>> ---
>>>  .../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