Re: [PATCH] map-ram chip driver: 16-bit block transfer

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Matt,

On Mon, 28 Aug 2017 21:23:41 -0500
Matthew Weber <matthew.weber@rockwellcollins.com> wrote:

> All,
> 
> On Mon, Aug 14, 2017 at 3:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sat, Aug 12, 2017 at 7:39 PM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:  
> >> Le Fri, 11 Aug 2017 18:42:44 -0500,
> >> Sanjay Tandel <sanjay.tandel@rockwellcollins.com> a écrit :  
> >>> On Fri, Aug 11, 2017 at 4:30 PM, Arnd Bergmann <arnd@arndb.de> wrote:  
> >>> > On Fri, Aug 11, 2017 at 11:03 PM, Sanjay Tandel
> >>> > <sanjay.tandel@rockwellcollins.com> wrote:  
> >  
> >>> Before, it used to corrupt every 2nd byte(2nd,4rth,6th ... upto N) in memory,
> >>> with memcpy_toio() using 8-bit accesses for our 16-bit chip.
> >>>
> >>> Now, with backporting 7ddfe625cbc1/1bd46782d08b, it corrupts (N + 1)th byte
> >>> in memory, when I tried to write N number of bytes (where N is not aligned to
> >>> 4-byte boundary).That means, it still tries 8-bit access for last odd byte and
> >>> ends up corrupting next byte.  
> >>
> >> Yes, that's expected as the size is not 16bit aligned.
> >>
> >> Anyway, I read the whole thread again and AFAIU memcpy_to/fromio() is
> >> only appropriate if the bus controller the device is connected to is
> >> smart enough to hide all alignment problems to its users.
> >>
> >> Don't know what controller is causing trouble here, but you should
> >> probably have a dedicated driver (if that's not already the case) that
> >> overloads the ->copy_from()/->copy_to() methods to do the right thing
> >> for your memory controller.  
> >
> > Right, I think that is a good conclusion. My understanding from the discussion
> > so far is that the existing code in the core mtd layer works efficiently and
> > correctly on most bus interfaces, so we should keep that but have
> > workarounds in the controller driver for any bus interface where this is
> > not the case.
> >
> > I think it would be different if this was a regression and the core MTD support
> > worked correctly in the past, but from what I read, it always behaved like
> > this.
> >  
> 
> Related to a dedicated driver, we've reworked the proposed patches
> with the following description and file list.
> 
> This patch adds map driver for parallel flash chips interfaced over a
> NXP Integrated
> Flash Controller (IFC). This driver allows either 8-bit or 16-bit accesses,
> depending on bank-width, to parallel flash chips(like Everspin MR0A16A),
> which are physically mapped to CPU's memory space. For unaligned accesses,
> it performs read-modify-write operations to keep access size same as
> bank-width.
> 
>  Documentation/devicetree/bindings/mtd/ifc-mram.txt |  34 ++
>  drivers/mtd/maps/Kconfig                           |  13 +
>  drivers/mtd/maps/Makefile                          |   1 +
>  drivers/mtd/maps/ifc_mram.c                        | 343 +++++++++++++++++++++
> 
> 
> Thanks for taking a look before we submit,

The code is missing (we only the diffstat) :-)

> we'd like to prevent another version.

What's the problem with sending a new version the proper way?

Regards,

Boris