Re: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Thor Thayer <tthayer@opensource.altera.com>]

On 03/05/2015 03:54 PM, Andy Shevchenko wrote:
> On Thu, 2015-03-05 at 14:41 -0600, Thor Thayer wrote:
>> Hi Andy,
>>
>> On 03/05/2015 04:43 AM, Andy Shevchenko wrote:
>>> On Wed, 2015-03-04 at 16:01 -0600, Thor Thayer wrote:
>>>> Hi Andy,
>>>>
>>>> On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
>>>>> On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
>>>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>>>
>>>>>> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
>>>>>> DesignWare SPI peripheral registers are on 32bit boundaries so this
>>>>>> patch is minimal. Function pointers are used to select 32bit access
>>>>>> or 16bit accesses.
>>>>>
>>>>>
>>>>> So, what is exactly the issue when we read only half of the register?
>>>>> Bus lock, or what?
>>>>>
>>>>
>>>> The read actually works on our chip but I changed both read and write to
>>>> be consistent. For Arria10, on a 16 bit write the data isn't written
>>>> into the DesignWare register.
>>>
>>> How did you exactly check this?
>>>
>>
>> Sorry, I should have been more clear. The architecture of the Arria10
>> SoC enforces 32 bit writes - even for the APB bus. The Arria10's
>> interconnect only allows 32 bit writes.
>
> Hmm… So, reads are okay, but writes are 32 bit only? Have you any link
> to the documentation where I could read this?
>

Currently Table 7-6 in the Arria10 Technical Reference Manual [1] on 
page 141 shows SPI Master as a 32 bit wide interface but it doesn't 
explicitly note the 32bit write requirement. We're adding a note to 
table 7-6 as a result of this discussion.

[1] http://www.altera.com/literature/hb/arria-10/a10_5v4.pdf

>> As a result, the solution below doesn't work for us.
>
> My thought was about post writes. That's why I was wondering and still
> wonder if something to clarify in the documentation.
>
>>
>> I do have a much cleaner patch to re-submit but I haven't applied it on
>> top of your patches yet & tested yet.
>
> Okay, please, apply the above as a part of commit message.
> I also would like to test it on Intel Medfield.
>
>>
>>>>
>>>> In reply to your other email, yes it does support the DW_apb_ssi but the
>>>> Arria10 architecture requires 32 bit access (actually as you point out,
>>>> 32 bit writes). We're using the original driver on our older chips but
>>>> Arria10 requires upstream changes.
>>>
>>> Can you check if the following helps in your case:
>>>
>>> --- a/drivers/spi/spi-dw.h
>>> +++ b/drivers/spi/spi-dw.h
>>> @@ -170,6 +170,8 @@ static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
>>>    static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>>>    {
>>>           __raw_writew(val, dws->regs + offset);
>>> +       mmiowb():
>>> +       __raw_readw(dws->regs + offset);
>>>    }
>>>
>>>    static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>>>
>
>