Re: [PATCH V5 13/23] MIPS: BMIPS: Flush the readahead cache after DMA

[Florian Fainelli]

2014-12-20 4:44 GMT-08:00 Jonas Gorski <jogo@xxxxxxxxxxx>:
> On Sat, Dec 20, 2014 at 2:39 AM, Kevin Cernekee <cernekee@xxxxxxxxx> wrote:
>> On Mon, Dec 15, 2014 at 1:43 AM, Jonas Gorski <jogo@xxxxxxxxxxx> wrote:
>>> On Fri, Dec 12, 2014 at 11:07 PM, Kevin Cernekee <cernekee@xxxxxxxxx> wrote:
>>>> BMIPS 3300/435x/438x CPUs have a readahead cache that is separate from
>>>> the L1/L2.  During a DMA operation, accesses adjacent to a DMA buffer
>>>> may cause parts of the DMA buffer to be prefetched into the RAC.  To
>>>> avoid possible coherency problems, flush the RAC upon DMA completion.
>>>
>>> According to what I have, any cpu [d-]cache invalidate operation
>>> should already flush the full RAC unless explicitly disabled in the
>>> RAC configuration - is this intended as an optimization/shortcut?
>>
>> Correct - performing a RAC flush instead of blasting the entire range
>> again via CACHE instructions should be considerably faster in most
>> cases.  CACHE instructions are not pipelined on BMIPS3300/43xx.
>>
>> There may be a couple of old CPU versions (possibly 130nm) that don't
>> automatically perform the RAC flush on each CACHE instruction.  Also,
>> a fun bit of trivia: MVA based cache flushes on B15 do flush the RAC,
>> but index based instructions do not.
>
> Because I'm laz^W^Wstill need to do some christmas shopping, I'll ask
> a few dumb questions:
>
> Since a RAC flush won't flush the I/D-caches themselves, I assume
> there is no cache invalidate needed for BMIPS? Also is it still needed
> if the RAC is setup to only prefetch instructions (which it seems to
> be on bcm963xx)?
>
> I also fail to find any RAC flushing on either bcm963xx or bcm947xx
> SDK kernels, that's why I'm a bit wondering whether they really need
> it. But maybe they always do explicit syncs, haven't checked that.
>
> Furthermore, I see code to enable data prefetching in setup on
> bcm963xx, so I wonder if it wouldn't make sense to add the RAC as an
> extra node in DT / register/enable/configure it from bmips setup code
> (because then we can also properly setup the address range in case the
> bootloader didn't).
>
>>>>  static inline int cpu_needs_post_dma_flush(struct device *dev)
>>>>  {
>>>
>>> The place for it seems a bit misplaced; I would not expect
>>> cpu_needs_post_dma_flush() to have any side effects.
>>
>> Maybe we should rename the function?  To just cpu_post_dma_flush()?
>
> Hm, not sure. Add a feature flag for that, or a callback. It is
> essentially a second level cache I guess.
>
> Also while reading dma-default.c, I wonder why dma_unmap_page checks
> if cpu needs to flush, but dma_unmap_sg doesn't (disclamer: I don't
> know anything about sg).
>
>>
>> (Or call a separate function from each site - but that seems unnecessary.)
>>
>>>> +       if (boot_cpu_type() == CPU_BMIPS3300 ||
>>>> +           boot_cpu_type() == CPU_BMIPS4350 ||
>>>> +           boot_cpu_type() == CPU_BMIPS4380) {
>>>> +               void __iomem *cbr = BMIPS_GET_CBR();
>>>> +
>>>> +               /* Flush stale data out of the readahead cache */
>>>> +               __raw_writel(0x100, cbr + BMIPS_RAC_CONFIG);
>>>
>>> Hm, according to what I have, bits [6:0] of RAC_CONFIG are R/W
>>> configuration bits, and this will overwrite them:
>>>
>>> CFE> dm 0xff400000 4
>>> ff400000: 02a07015                                        ..p.
>>> CFE> sm 0xff400000 0x100 4
>>> ff400000: 02a00000                                        ....
>>>
>>> (As far as I can tell, RAC was previously enabled for instruction
>>> cache misses , and now isn't any more for anything, so effectively
>>> disabled?)
>>>
>>> Also for BMIPS4350 (and I guess 4380) there seems to be a second
>>> RAC_CONFIG register at 0x8, I guess for the second thread? Does it
>>> need flushing, too?
>>
>> I'll defer to Florian for the final word since he has access to the
>> documentation, but going from memory:
>>
>> RAC_CONFIG should probably be a read/modify/write.  I'm pretty sure
>> there are important RW configuration bits in there.  I may have
>> incorrectly translated the "set bit 8" code from here:
>>
>> https://github.com/Broadcom/stblinux-3.3/blob/master/linux/arch/mips/mm/c-brcmstb.c#L374
>>
>> There is only one RAC for all CPUs, and we've never had to flush
>> anything via CBR+0x08.
>
>
> What I see in recent bcm963xx SDKs is this:
>
> void __init plat_mem_setup(void)
> {
> ...
>         volatile unsigned long *cr;
>         uint32 mipsBaseAddr = MIPS_BASE;
>
>         cr = (void *)(mipsBaseAddr + MIPS_RAC_CR0);
>         *cr = *cr | RAC_D | RAC_PF_D;
>
> #if defined(MIPS_RAC_CR1)
>         cr = (void *)(mipsBaseAddr + MIPS_RAC_CR1);
>         *cr = *cr | RAC_D | RAC_PF_D;
> #endif
> }
>
> RAC_CR1 seems to be defined for BMIPS4350 based SoCs, while BMIPS3300
> ones don't (well, 6318 doesn't. The older SDKs named the address range
> register RAC_CR1, so for them this check is quite wrong :P)
>
> But no references to RAC_FLH anywhere.

RAC_CR (0x1FA0_0000) controls the RAC settings for TP0, while RAC_CR1
(0x1FA0_0008) controls the RAC settings for TP1, hence the reason why
BMIPS3300 does not have it.

Surprisingly (or not), FLH only exists in RAC_CR, because the RAC is
shared between TPs just like the L2.

In your previous dump, bit 4 is set, aka C_INV, which means that the
RAC will flush its I-cache blocks when we issue "cache" instructions,
effectively making the RAC transparent as far as I understand it, and
D-cache prefetching is indeed disabled.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/