| Message ID | 1425552233.14897.189.camel@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. As a result, the solution below doesn't work for us. I do have a much cleaner patch to re-submit but I haven't applied it on top of your patches yet & tested yet. >> >> 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? > 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) > >
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) >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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)