[PATCH] ath10k: Replace ioread with wmb for data sync
poh at qca.qualcomm.com
Mon Feb 2 09:33:40 PST 2015
On 02/02/2015 05:02 AM, Johannes Berg wrote:
> On Fri, 2015-01-30 at 14:53 -0800, Peter Oh wrote:
>> I admit that I/O ordering and posted write are looked different in
>> theory and at glance since posted write could be related cache
>> But wmb are still related to both.
>> As I addressed wmb uses dsb (in arm arch) and here is the description of
>> arm architecture.
>> * DSB drains write buffer.
>> * DSB is architecturally defined to include all cache, TLB and branch
>> prediction maintenance operations as well as explicit memory operations
>> These are the reasons why I mentioned wmb does both.
>> * captured from ARMv7 Architecture Manual
>> --- Notes ---
>> Historically, this operation was referred to as Drain Write Buffer or
>> Data Write Barrier (DWB). From ARMv6, these
>> names and the use of DWB were deprecated in favor of the new Data
>> Synchronization Barrier name and DSB
>> abbreviation. DSB better reflects the functionality provided from ARMv6,
>> because DSB is architecturally defined
>> to include all cache, TLB and branch prediction maintenance operations
>> as well as explicit memory operations
>> --- A DSB completes when: ---
>> ? all explicit memory accesses that are observed by Pe before the DSB is
>> executed, are of the required access
>> types, and are from observers in the same required shareability domain
>> as Pe, are complete for the set of
>> observers in the required shareability domain.
>> ? if the required accesses types of the DSB is reads and writes, all
>> cache and branch predictor maintenance
>> operations issued by Pe before the DSB are complete for the required
>> shareability domain.
>> ? if the required accesses types of the DSB is reads and writes, all TLB
>> maintenance operations issued by Pe
>> before the DSB are complete for the required shareability domain.
> I cannot read from this in any way that it can post writes to the PCIe
> bus. In fact, architecturally, I cannot think of any reason how it even
> could do that from the CPU.
>> Furthermore this is the comparison of the compiled assembly code between
>> ath10k_pci_read32 and wmb.
>> bac: e5932008 ldr r2, [r3, #8]
>> bb0: f57ff04f dsb sy
>> bb4: e2883d52 add r3, r8, #5248 ; 0x1480
>> bb8: e283303c add r3, r3, #60 ; 0x3c
>> bbc: e593300c ldr r3, [r3, #12]
>> bc0: e2833a09 add r3, r3, #36864 ; 0x9000
>> b9c: f57ff04e dsb st
>> ath10k_pci_read32 does register operation except dsb and there is no
>> cache invalidate related commands.
> I don't think this is relevant. The question is "what are you trying to
>> So that if wmb is not enough for the purpose then ath10k_pci_read32 is
>> also not enough for that.
>> Also refer the section "ACQUIRES VS I/O ACCESSES" in
>> It gives an example with PCI bridge and introduces readl as an
>> alternative method to mmiowb which weaker form of wmb.
>> Please give your opinion.
> Again - the question is - what are you trying to achieve?
> The code (as it is before your patch) implies that it's trying to make
> sure that before it continues, any previous writes to the PCIe device's
> registers are posted. The only way to ensure that is to do a read to the
> registers, as the code does now.
Do you know how the read ensure that although the read code does not
check the return value?
Can you explain how a read ensures that posted write reaches PCIe device?
> What you're describing is something else entirely - you're describing a
> way to make sure that some data was flushed out to DRAM from the CPU
> These two things are not related in any way.
> In an interrupt routine, it would make sense to ensure that the write
> was posted (e.g. to mask interrupts, or to acknowledge them, or similar,
> before the routine can be re-invoked.)
> To me, flushing memory writes to DRAM makes less sense in an interrupt
> handlers unless the device was somehow using DMA to coordinate
> interrupts , which seems unlikely but I haven't checked.
> Anyway - I have no particular interest in this discussion, I was merely
> trying to help you out with this :) You can make whatever change you
> want, of course :P
>  incidentally, our device [iwlwifi] does in fact do something like
> that, but it's read-only for the driver so no need for such a thing
> ath10k mailing list
> ath10k at lists.infradead.org
More information about the ath10k