Hi Robin, Thanks a lot for your comments! Please, find my comments below.
> -----Original Message----- > From: Robin Murphy [mailto:[email protected]] > Sent: Tuesday, July 26, 2016 7:43 PM > To: 이광우(LEE KWANGWOO) MS SW; Russell King - ARM Linux; Catalin Marinas; Will > Deacon; Mark Rutland; > [email protected] > Cc: 김현철(KIM HYUNCHUL) MS SW; [email protected]; 정우석(CHUNG WOO SUK) > MS SW > Subject: Re: [PATCH v2] arm64: mm: convert __dma_* routines to use start, size > > On 26/07/16 08:34, Kwangwoo Lee wrote: > > v2) > > change __dma_* routine names using the terminoloy guidance: > > area: takes a start and size > > range: takes a start and end > > use __dma_flush_area() instead of __dma_flush_range() in dma-mapping.c > > > > v1) > > __dma_* routines have been converted to use start and size instread of > > start and end addresses. The patch was origianlly for adding > > __clean_dcache_area_poc() which will be used in pmem driver to clean > > dcache to the PoC(Point of Coherency) in arch_wb_cache_pmem(). > > > > The functionality of __clean_dcache_area_poc() was equivalent to > > __dma_clean_range(). The difference was __dma_clean_range() uses the end > > address, but __clean_dcache_area_poc() uses the size to clean. > > > > Thus, __clean_dcache_area_poc() has been revised with a fall through > > function of __dma_clean_range() after the change that __dma_* routines > > use start and size instead of using start and end. > > > > Signed-off-by: Kwangwoo Lee <[email protected]> > > --- > > Nit: the changelog relative to the previous posting wants to be here, > under the "---" separator; the commit message above should describe the > _current_ state of the patch, as that's all we'll really care about once > it's in the Git history. OK. I'll follow the convention and use the feature. Thank you very much for letting me know! > > arch/arm64/include/asm/cacheflush.h | 3 +- > > arch/arm64/mm/cache.S | 71 > > +++++++++++++++++++------------------ > > arch/arm64/mm/dma-mapping.c | 6 ++-- > > 3 files changed, 41 insertions(+), 39 deletions(-) > > > > diff --git a/arch/arm64/include/asm/cacheflush.h > > b/arch/arm64/include/asm/cacheflush.h > > index c64268d..2e5fb97 100644 > > --- a/arch/arm64/include/asm/cacheflush.h > > +++ b/arch/arm64/include/asm/cacheflush.h > > @@ -68,6 +68,7 @@ > > extern void flush_cache_range(struct vm_area_struct *vma, unsigned long > > start, unsigned long end); > > extern void flush_icache_range(unsigned long start, unsigned long end); > > extern void __flush_dcache_area(void *addr, size_t len); > > +extern void __clean_dcache_area_poc(void *addr, size_t len); > > extern void __clean_dcache_area_pou(void *addr, size_t len); > > extern long __flush_cache_user_range(unsigned long start, unsigned long > > end); > > > > @@ -85,7 +86,7 @@ static inline void flush_cache_page(struct vm_area_struct > > *vma, > > */ > > extern void __dma_map_area(const void *, size_t, int); > > extern void __dma_unmap_area(const void *, size_t, int); > > -extern void __dma_flush_range(const void *, const void *); > > +extern void __dma_flush_area(const void *, size_t); > > > > /* > > * Copy user data from/to a page which is mapped into a different > > diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S > > index 50ff9ba..4415c1b 100644 > > --- a/arch/arm64/mm/cache.S > > +++ b/arch/arm64/mm/cache.S > > @@ -110,14 +110,16 @@ ENDPROC(__clean_dcache_area_pou) > > * - end - end address of region > > */ > > ENTRY(__inval_cache_range) > > + sub x1, x1, x0 > > Rather than doing this, I think it would be more sensible to simply swap > the entry points. This is much better idea instead of adding sub instruction! :) Thanks! > > /* FALLTHROUGH */ > > > > /* > > - * __dma_inv_range(start, end) > > + * __dma_inv_area(start, size) > > * - start - virtual start address of region > > - * - end - virtual end address of region > > + * - size - size in question > > */ > > -__dma_inv_range: > > +__dma_inv_area: > > + add x1, x1, x0 > > dcache_line_size x2, x3 > > sub x3, x2, #1 > > tst x1, x3 // end cache line aligned? > > @@ -136,46 +138,47 @@ __dma_inv_range: > > dsb sy > > ret > > ENDPIPROC(__inval_cache_range) > > -ENDPROC(__dma_inv_range) > > +ENDPROC(__dma_inv_area) > > + > > +/* > > + * __clean_dcache_area_poc(kaddr, size) > > + * > > + * Ensure that any D-cache lines for the interval [kaddr, > > kaddr+size) > > + * are cleaned to the PoC. > > + * > > + * - kaddr - kernel address > > + * - size - size in question > > + */ > > +ENTRY(__clean_dcache_area_poc) > > + /* FALLTHROUGH */ > > > > /* > > - * __dma_clean_range(start, end) > > + * __dma_clean_area(start, size) > > * - start - virtual start address of region > > - * - end - virtual end address of region > > + * - size - size in question > > */ > > -__dma_clean_range: > > - dcache_line_size x2, x3 > > - sub x3, x2, #1 > > - bic x0, x0, x3 > > -1: > > +__dma_clean_area: > > alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE > > - dc cvac, x0 > > + dcache_by_line_op cvac, sy, x0, x1, x2, x3 > > alternative_else > > - dc civac, x0 > > + dcache_by_line_op civac, sy, x0, x1, x2, x3 > > dcache_by_line_op is a relatively large macro - is there any way we can > still apply the alternative to just the one instruction which needs it, > as opposed to having to patch the entire mostly-identical routine? I agree with your opinion. Then, how do you think about using CONFIG_* options like below? I think that alternative_* macros seems to keep the space for unused instruction. Is it necessary? Please, share your thought about the space. Thanks! +__dma_clean_area: +#if defined(CONFIG_ARM64_ERRATUM_826319) || \ + defined(CONFIG_ARM64_ERRATUM_827319) || \ + defined(CONFIG_ARM64_ERRATUM_824069) || \ + defined(CONFIG_ARM64_ERRATUM_819472) + dcache_by_line_op civac, sy, x0, x1, x2, x3 +#else + dcache_by_line_op cvac, sy, x0, x1, x2, x3 +#endif > Robin. Best Regards, Kwangwoo Lee
