Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310652 > unrolled thread

Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-16 01:40 +0100
    Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@linux-mips.org> - 2016-01-16 20:40 +0100
    Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() Brian Norris <computersforpeace@gmail.com> - 2016-01-23 23:00 +0100
      Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-24 10:50 +0100
      Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@imgtec.com> - 2016-01-24 21:30 +0100
        Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-24 22:40 +0100
          Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@imgtec.com> - 2016-01-25 00:10 +0100

#1310652 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On 9 January 2016 at 03:10, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Fri, 8 Jan 2016, Brian Norris wrote:
>> > > It results in different address than KSEG0ADDR:
>> > > [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
>> > > [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
>> > >
>> > > But it still works as expected! :)
>> > > [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
>> > > [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":
>> >
>> >  It is a functional change though and I think the change from a cached to
>> > uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a
>> > separate patch, so that both changes can be reviewed independently.
>>
>> As I noted before sending my patch, I don't think this driver should
>> have been using KSEG0 anyway; it should have been KSEG1, right? I can
>> note that in the patch description, but I don't really see why it needs
>> to be a separate patch.
>
>  You did mention that, but didn't actually justify why an uncached mapping
> is required here.
>
>  This code is in a function called `bcm47xxsflash_read' and reads from an
> MMIO region, presumably flash memory which behaves like ordinary memory on
> reads (i.e. no side effects).  Therefore using a cached mapping will in
> most cases result in much better performance as the CPU will load
> (prefetch) data in cacheline-sized quantities rather than hitting the
> external bus every time with a word-sized quantity transferred only.

So I wanted to stick to the cached mapping, but it appears it's not
possible with devm_*. We have two options:
1) devm_ioremap_nocache - it obviously won't be cached mapping
2) devm_ioremap_resource - it uses devm_ioremap which uses ioremap
which is nocache on MIPS

Should I introduce a new
devm_ioremap_resource_cache
with some
devm_ioremap_cache
helper? And then use it in bcm47xxsflash?

-- 
Rafał

[toc] | [next] | [standalone]

#1311017 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Sat, 16 Jan 2016, Rafał Miłecki wrote:

> >  This code is in a function called `bcm47xxsflash_read' and reads from an
> > MMIO region, presumably flash memory which behaves like ordinary memory on
> > reads (i.e. no side effects).  Therefore using a cached mapping will in
> > most cases result in much better performance as the CPU will load
> > (prefetch) data in cacheline-sized quantities rather than hitting the
> > external bus every time with a word-sized quantity transferred only.
> 
> So I wanted to stick to the cached mapping, but it appears it's not
> possible with devm_*. We have two options:
> 1) devm_ioremap_nocache - it obviously won't be cached mapping
> 2) devm_ioremap_resource - it uses devm_ioremap which uses ioremap
> which is nocache on MIPS
> 
> Should I introduce a new
> devm_ioremap_resource_cache
> with some
> devm_ioremap_cache
> helper? And then use it in bcm47xxsflash?

 This sounds like a plan to me, except that with `devm_ioremap_wc' also in 
the view and all the three `devm_ioremap*' functions being identical -- 
except from the use of a different `ioremap_*' call -- it looks to me it 
really asks for a `mode' argument and all the code to be unified.  
Compatibility macros (or, perhaps better, static inline functions) could 
be provided to preserve the internal API for the existing code.

 I.e. there would be a new `devm_ioremap_resource_mode' entry point which 
calls `devm_ioremap_mode', which then selects, perhaps with `switch' on 
`mode', from the available `ioremap*' calls.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1315762 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Sat, Jan 16, 2016 at 01:38:11AM +0100, Rafał Miłecki wrote:
> So I wanted to stick to the cached mapping, [...]

I mentioned this earlier on, but I don't feel like I've gotten a clear
answer. Is a cached mapping actually safe here? From the looks of it,
the memory mapping is a read-only memory-mapped flash, and flash writes
/ erasures are done through a different bus (register writes vis BCMA
bus). So if we have a cached mapping of that memory, it doens't
naturally synchronize with any write/erase operations. Doesn't this mean
you might get stale data if you do a sequence of read / erase / read,
for instance, since the 2nd read will return cached data from the 1st
read?

IIUC, this could be solved by:
(a) using an uncached mapping or
(b) explicitly invalidating the relevant region after doing flash writes
or erasures

But I wonder why you haven't seen any problems if you've been using
KSEG0 (cached) this whole time. Maybe just luck? Or you don't actually
write to the flash that much?

Brian

[toc] | [prev] | [next] | [standalone]

#1315853

On 23 January 2016 at 22:49, Brian Norris <computersforpeace@gmail.com> wrote:
> On Sat, Jan 16, 2016 at 01:38:11AM +0100, Rafał Miłecki wrote:
>> So I wanted to stick to the cached mapping, [...]
>
> I mentioned this earlier on, but I don't feel like I've gotten a clear
> answer. Is a cached mapping actually safe here? From the looks of it,
> the memory mapping is a read-only memory-mapped flash, and flash writes
> / erasures are done through a different bus (register writes vis BCMA
> bus). So if we have a cached mapping of that memory, it doens't
> naturally synchronize with any write/erase operations. Doesn't this mean
> you might get stale data if you do a sequence of read / erase / read,
> for instance, since the 2nd read will return cached data from the 1st
> read?
>
> IIUC, this could be solved by:
> (a) using an uncached mapping or
> (b) explicitly invalidating the relevant region after doing flash writes
> or erasures
>
> But I wonder why you haven't seen any problems if you've been using
> KSEG0 (cached) this whole time. Maybe just luck? Or you don't actually
> write to the flash that much?

Now you pointed this difference between reads and writes I sounds
worrying indeed. I'm not aware of ever hitting this problem but maybe
I just didn't use flash in a way triggering it?

I'm looking for a way to test it. Using user space I could try doing
something like:
echo foo > a.txt
cat a.txt
echo bar > a.txt
cat a.txt

I guess even more reliable test would to be test in in kernel space. I
guess I could modify bcm47xxsflash_write to read flash region that is
going to be modified: before modification and after. Both reads using
KSEG0ADDR. Then compare if the second read matches was was written.

Does my idea for tests make sense?

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1315971 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Sat, 23 Jan 2016, Brian Norris wrote:

> IIUC, this could be solved by:
> (a) using an uncached mapping or
> (b) explicitly invalidating the relevant region after doing flash writes
> or erasures

 Flash writes are usually much, much less frequent than reads, so 
optimising for reads is IMO the right direction.  So a cached mapping is a 
good choice, however invalidation must then be done after a write.

> But I wonder why you haven't seen any problems if you've been using
> KSEG0 (cached) this whole time. Maybe just luck? Or you don't actually
> write to the flash that much?

 That depends on cache usage, any stale lines may well have usually gone 
in the course of regular cache line replacement, making the issue remain 
unnoticed.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1315984

On 24 January 2016 at 21:26, Maciej W. Rozycki <macro@imgtec.com> wrote:
> On Sat, 23 Jan 2016, Brian Norris wrote:
>
>> IIUC, this could be solved by:
>> (a) using an uncached mapping or
>> (b) explicitly invalidating the relevant region after doing flash writes
>> or erasures
>
>  Flash writes are usually much, much less frequent than reads, so
> optimising for reads is IMO the right direction.  So a cached mapping is a
> good choice, however invalidation must then be done after a write.

Can you give me some hint where to look at for cache invalidation?

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1316137 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Sun, 24 Jan 2016, Rafał Miłecki wrote:

> On 24 January 2016 at 21:26, Maciej W. Rozycki <macro@imgtec.com> wrote:
> > On Sat, 23 Jan 2016, Brian Norris wrote:
> >
> >> IIUC, this could be solved by:
> >> (a) using an uncached mapping or
> >> (b) explicitly invalidating the relevant region after doing flash writes
> >> or erasures
> >
> >  Flash writes are usually much, much less frequent than reads, so
> > optimising for reads is IMO the right direction.  So a cached mapping is a
> > good choice, however invalidation must then be done after a write.
> 
> Can you give me some hint where to look at for cache invalidation?

 There is `flush_data_cache_page' only it would seem, which is also 
supported by the MIPS platform only.  It makes unnecessary writebacks 
before invalidation, however these aren't really supposed to happen as no 
cache line involved is expected to be dirty.

 Implementing `invalidate_data_cache_page', which would avoid these 
unnecessary writebacks, should be straightforward as hardware provides the 
necessary operations and actually the MIPS port has suitable low-level 
helpers already implemented, for use by `dma_cache_inv'.  So that would 
merely be a semi-mechanical copy, paste, rename operation applied to our 
source.

 The bigger problem is the lack of portability of this interface to other 
platforms, although I suspect some hardware may simply fail to provide 
required operations.  For example x86 only defines the sledgehammer 
INVD/WBINVD instructions, which operate on the whole cache hierarchy at 
once rather than on a line-by-line and cache level/part basis.

  Maciej

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web