答复: 答复: [PATCH v2] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
Hi, Catalin Sorry late for you, I will submit the patch as soon as possible. Thanks! -邮件原件- 发件人: Catalin Marinas [mailto:catalin.mari...@arm.com] 发送时间: 2020年11月16日 18:47 收件人: liwei (CM) 抄送: Song Bao Hua (Barry Song) ; Mike Rapoport ; w...@kernel.org; Xiaqing (A) ; Chenfeng (puck) ; butao ; fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; Linuxarm 主题: Re: 答复: [PATCH v2] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP On Mon, Nov 16, 2020 at 08:42:17AM +, liwei (CM) wrote: > I have changed SECTION_SIZE_BITS to 27 in our products, but I don't > have to submit it. Well, if you send a patch, I'm happy to merge it. -- Catalin
答复: [PATCH v2] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
Hi, Barry I have changed SECTION_SIZE_BITS to 27 in our products, but I don't have to submit it. -邮件原件- 发件人: Song Bao Hua (Barry Song) 发送时间: 2020年11月16日 16:34 收件人: Catalin Marinas ; Mike Rapoport ; liwei (CM) 抄送: w...@kernel.org; Xiaqing (A) ; Chenfeng (puck) ; butao ; fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; Linuxarm 主题: RE: [PATCH v2] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP > -Original Message- > From: Catalin Marinas [mailto:catalin.mari...@arm.com] > Sent: Friday, September 4, 2020 12:06 AM > To: Mike Rapoport > Cc: liwei (CM) ; w...@kernel.org; Xiaqing (A) > ; Chenfeng (puck) > ; butao ; fengbaopeng > ; nsaenzjulie...@suse.de; > steve.cap...@arm.com; Song Bao Hua (Barry Song) > ; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; sujunfei > Subject: Re: [PATCH v2] arm64: mm: free unused memmap for sparse > memory model that define VMEMMAP > > On Mon, Aug 17, 2020 at 11:04:05AM +0300, Mike Rapoport wrote: > > On Wed, Aug 12, 2020 at 09:06:55AM +0800, Wei Li wrote: > > > For the memory hole, sparse memory model that define > SPARSEMEM_VMEMMAP > > > do not free the reserved memory for the page map, this patch do it. > > > > I've been thinking about it a bit more and it seems that instead of > > freeing unused memory map it would be better to allocate the exact > > memory map from the beginning. > > > > In sparse_init_nid() we can replace PAGES_PER_SECTION parameter to > > __populate_section_memmap() with the calculated value for > > architectures that define HAVE_ARCH_PFN_VALID. > > Or just use a smaller PAGES_PER_SECTION and reduce the waste ;). > > Just to be clear, are you suggesting that we should use pfn_valid() on > the pages within a section to calculate the actual range? The > pfn_valid() implementation on arm64 checks for the validity of a > sparse section, so this would be called from within the sparse_init() > code path. I hope there's no dependency but I haven't checked. If it > works, it's fine by me, it solves the FLATMEM mem_map freeing as well. > > With 4KB pages on arm64, vmemmap_populate() stops at the pmd level, so > it always allocates PMD_SIZE. Wei's patch also only frees in PMD_SIZE > amounts. So, with a sizeof(struct page) of 64 (2^6), a PMD_SIZE > mem_map section would cover 2^(21-6) pages, so that's equivalent to a > SECTION_SIZE_BITS of 21-6+12 = 27. > > If we reduce SECTION_SIZE_BITS to 27 or less, this patch is a no-op. It would be the simplest way to fix this issue. It seems X86_64 is also using 27. @wei, has you ever tried to send a patch to change SECTION_SIZE_BITS to 27 for ARM64? > > -- > Catalin Thanks Barry
答复: 答复: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
-邮件原件- 发件人: Mike Rapoport [mailto:r...@linux.ibm.com] 发送时间: 2020年7月23日 21:19 收件人: Catalin Marinas 抄送: liwei (CM) ; w...@kernel.org; Xiaqing (A) ; Chenfeng (puck) ; butao ; fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; Song Bao Hua (Barry Song) ; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; zhaojiapeng 主题: Re: 答复: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP On Thu, Jul 23, 2020 at 12:29:26PM +0100, Catalin Marinas wrote: > On Wed, Jul 22, 2020 at 01:40:34PM +0000, liwei (CM) wrote: > > Catalin Marinas wrote: > > > On Wed, Jul 22, 2020 at 08:41:17AM +, liwei (CM) wrote: > > > > Mike Rapoport wrote: > > > > > On Tue, Jul 21, 2020 at 03:32:03PM +0800, Wei Li wrote: > > > > > > For the memory hole, sparse memory model that define > > > > > > SPARSEMEM_VMEMMAP do not free the reserved memory for the > > > > > > page map, this patch do it. > > > > > > > > > > Are there numbers showing how much memory is actually freed? > > > > > > > > > > The freeing of empty memmap would become rather complex with > > > > > these changes, do the memory savings justify it? > > > > > > > > In the sparse memory model, the size of a section is 1 GB > > > > (SECTION_SIZE_BITS 30) by default. > > > > > > Can we reduce SECTION_SIZE_BITS instead? Say 26? > > > > Yes, you are right, reduce SECTION_SIZE_BITS to 26 can save almost > > the same memory as the patch. > > > > 1) However, it is not clear whether changing the section size has > > any other impact. > > Well, we should analyse this. > > > 2) Just like the flat memory model and the sparse memory model that > > does not define VMEMMAP, both of them have their own ways to free > > unused memmap. I think we've given a similar way for sparse memory > > define VMEMMAP. > > I think we did it for flatmem initially (on arm32) and added support > for sparsemem later on, so free_unused_memmap() had to cope with > sparse sections. On arm64 we introduced vmemmap support and didn't > bother with the freeing at all because of the added complexity of the > vmemmap page tables. > > I wonder whether we should just disallow flatmem and non-vmemmap > sparsemem on arm64. Is there any value in keeping them around? FLATMEM is useful for UMA systems with a single memory bank, so probably it's worth keeping it for low end machines. Non-vmemmap sparsemem is essentially disable in arch/arm64/Kconfig, so for NUMA configurations SPARSEMEM_VMEMMAP is the only choice. > > 3) This explicit free unused memmap method does reduce unnecessary > > memory waste for users who do not notice the section size > > modification. > > But if we changed SECTION_SIZE_BITS in the mainline kernel, then we > wouldn't need additional code to free the unused memmap. Moreover if we reduce SECTION_SIZE_BITS, we can drop free_unused_memmap() and since the arm64 memory map for sparse will not differ from other arches we can drop custom pfn_valid() as well. Hi, Mike & Catalin Let's think and discuss together about the impact of directly reducing the section size： 1) Currently, the memory of PC or Mobile devices are increasing. If the section size is reduced, the consumption of the section structure will also increase. 2) If the section size is too small, memory hotplug may be affected. Hotplug add or remove a memblock means that you need to online or offline many sections. In this case, software consumption may increase. Currently, the page map is wasted when the default section size is used. In some cases, the waste is serious. Please help to check whether the section size reduction has other impacts and whether it meets the long-term evolution. Thanks. > -- > Catalin -- Sincerely yours, Mike.
答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
-邮件原件- 发件人: Anshuman Khandual [mailto:anshuman.khand...@arm.com] 发送时间: 2020年7月23日 10:33 收件人: liwei (CM) ; catalin.mari...@arm.com; w...@kernel.org 抄送: Song Bao Hua (Barry Song) ; sujunfei ; Xiaqing (A) ; linux-arm-ker...@lists.infradead.org; steve.cap...@arm.com; Chenfeng (puck) ; linux-kernel@vger.kernel.org; r...@linux.ibm.com; fengbaopeng ; nsaenzjulie...@suse.de; butao 主题: Re: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP On 07/21/2020 01:02 PM, Wei Li wrote: > For the memory hole, sparse memory model that define SPARSEMEM_VMEMMAP > do not free the reserved memory for the page map, this patch do it. > > Signed-off-by: Wei Li > Signed-off-by: Chen Feng > Signed-off-by: Xia Qing > --- > arch/arm64/mm/init.c | 81 > +--- > 1 file changed, 71 insertions(+), 10 deletions(-) This patch does not compile on 5.8-rc6 with defconfig. Hi, We're working on patch v2 as soon as possible. Thanks.
答复: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
-邮件原件- 发件人: Catalin Marinas [mailto:catalin.mari...@arm.com] 发送时间: 2020年7月22日 20:49 收件人: liwei (CM) 抄送: Mike Rapoport ; w...@kernel.org; Xiaqing (A) ; Chenfeng (puck) ; butao ; fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; Song Bao Hua (Barry Song) ; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; zhaojiapeng 主题: Re: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP On Wed, Jul 22, 2020 at 08:41:17AM +, liwei (CM) wrote: > Mike Rapoport wrote: > > On Tue, Jul 21, 2020 at 03:32:03PM +0800, Wei Li wrote: > > > For the memory hole, sparse memory model that define > > > SPARSEMEM_VMEMMAP do not free the reserved memory for the page map, this > > > patch do it. > > > > Are there numbers showing how much memory is actually freed? > > > > The freeing of empty memmap would become rather complex with these > > changes, do the memory savings justify it? > > In the sparse memory model, the size of a section is 1 GB > (SECTION_SIZE_BITS 30) by default. Can we reduce SECTION_SIZE_BITS instead? Say 26? Hi, Catalin Yes, you are right, reduce SECTION_SIZE_BITS to 26 can save almost the same memory as the patch. 1) However, it is not clear whether changing the section size has any other impact. 2) Just like the flat memory model and the sparse memory model that does not define VMEMMAP, both of them have their own ways to free unused memmap. I think we've given a similar way for sparse memory define VMEMMAP. 3) This explicit free unused memmap method does reduce unnecessary memory waste for users who do not notice the section size modification. Hope you will reconsider the purpose and significance of this patch, thanks. -- Catalin
答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
-邮件原件-
发件人: Mike Rapoport [mailto:r...@linux.ibm.com]
发送时间: 2020年7月22日 14:07
收件人: liwei (CM)
抄送: catalin.mari...@arm.com; w...@kernel.org; Xiaqing (A)
; Chenfeng (puck) ; butao
; fengbaopeng ;
nsaenzjulie...@suse.de; steve.cap...@arm.com; Song Bao Hua (Barry Song)
; linux-arm-ker...@lists.infradead.org;
linux-kernel@vger.kernel.org; sujunfei
主题: Re: [PATCH] arm64: mm: free unused memmap for sparse memory model that
define VMEMMAP
Hi,
On Tue, Jul 21, 2020 at 03:32:03PM +0800, Wei Li wrote:
> For the memory hole, sparse memory model that define SPARSEMEM_VMEMMAP
> do not free the reserved memory for the page map, this patch do it.
Are there numbers showing how much memory is actually freed?
The freeing of empty memmap would become rather complex with these changes, do
the memory savings justify it?
Hi, Mike
In the sparse memory model, the size of a section is 1 GB (SECTION_SIZE_BITS
30) by default. Therefore, when the memory is less than the size of a section,
that is, when there is a hole, the patch takes effect：
1) For example, the DDR size used by our platform is 8 GB, however, 3.5 ~ 4 GB
is the space where the SOC register is located. Therefore, if the page map is
performed on memory3 (3 ~4 GB), 512 MB/4 KB x 64 bytes = 8 MB is wasted because
the page map is not required for the 3.5~4 GB space; However, the DDR memory
space of 3.5~4 GB is shift to 16~16.5 GB. In this case, memory is wasted
because the page map is not required for 16.5~17 GB, the patch can also save 8
MB. Therefore, the total saved memory is 16 MB.
2) In the reserved memory, some modules need to reserve a large amount of
memory (no-map attr). On our platform, the modem module needs to reserve more
than 256 MB memory, and the patch can save 4 MB. Actually, if the reserved
memory is greater than 128 MB, the patch can free unnecessary page map memory.
It may be possible to save some waste by reducing the section size, but free
the waste page map that defines VMEMMAP is another approach similar to flat
memory model and sparse memory mode that does not define VMEMMAP, and makes the
entire code look more complete.
If you have a better idea, I'd be happy to discuss it with you.
Thanks!
> Signed-off-by: Wei Li
> Signed-off-by: Chen Feng
> Signed-off-by: Xia Qing
> ---
> arch/arm64/mm/init.c | 81
> +---
> 1 file changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index
> 1e93cfc7c47a..d1b56b47d5ba 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -441,7 +441,48 @@ void __init bootmem_init(void)
> memblock_dump_all();
> }
>
> -#ifndef CONFIG_SPARSEMEM_VMEMMAP
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> +#define VMEMMAP_PAGE_INUSE 0xFD
> +static inline void free_memmap(unsigned long start_pfn, unsigned long
> +end_pfn) {
> + unsigned long addr, end;
> + unsigned long next;
> + pmd_t *pmd;
> + void *page_addr;
> + phys_addr_t phys_addr;
> +
> + addr = (unsigned long)pfn_to_page(start_pfn);
> + end = (unsigned long)pfn_to_page(end_pfn);
> +
> + pmd = pmd_offset(pud_offset(pgd_offset_k(addr), addr), addr);
> + for (; addr < end; addr = next, pmd++) {
> + next = pmd_addr_end(addr, end);
> +
> + if (!pmd_present(*pmd))
> + continue;
> +
> + if (IS_ALIGNED(addr, PMD_SIZE) &&
> + IS_ALIGNED(next, PMD_SIZE)) {
> + phys_addr = __pfn_to_phys(pmd_pfn(*pmd));
> + free_bootmem(phys_addr, PMD_SIZE);
> + pmd_clear(pmd);
> + } else {
> + /* If here, we are freeing vmemmap pages. */
> + memset((void *)addr, VMEMMAP_PAGE_INUSE, next - addr);
> + page_addr = page_address(pmd_page(*pmd));
> +
> + if (!memchr_inv(page_addr, VMEMMAP_PAGE_INUSE,
> + PMD_SIZE)) {
> + phys_addr = __pfn_to_phys(pmd_pfn(*pmd));
> + free_bootmem(phys_addr, PMD_SIZE);
> + pmd_clear(pmd);
> + }
> + }
> + }
> +
> + flush_tlb_all();
> +}
> +#else
> static inline void free_memmap(unsigned long start_pfn, unsigned long
> end_pfn) {
> struct page *start_pg, *end_pg;
> @@ -468,31 +509,53 @@ static inline void free_memmap(unsigned long start_pfn,
> unsigned long end_pfn)
> memblock_free(pg, pgend - pg);
> }
>
> +#endif
> +
> /*
> * The mem_map array can get very big. Free the unused area of the memory
> map.
> */
> static void __init free_unus
答复: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
-邮件原件- 发件人: Mike Rapoport [mailto:r...@linux.ibm.com] 发送时间: 2020年7月21日 14:35 收件人: liwei (CM) 抄送: Song Bao Hua (Barry Song) ; catalin.mari...@arm.com; w...@kernel.org; fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; Xiaqing (A) ; Yaobaofeng (Yaobaofeng) 主题: Re: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP Hi, On Tue, Jul 21, 2020 at 01:56:33AM +, liwei (CM) wrote: > Hi, all > > I'm sorry to bother you, but still very hope you can give comments or > suggestions to this patch, thank you very much. I cannot find your patch neither in Inbox nor in the public archives. Can you resend it please? Hi, Mike I resend the patch, if you see please give advice, thanks! > -邮件原件- > 发件人: Song Bao Hua (Barry Song) > 发送时间: 2020年7月9日 20:27 > 收件人: liwei (CM) ; catalin.mari...@arm.com; > w...@kernel.org > 抄送: fengbaopeng ; nsaenzjulie...@suse.de; > steve.cap...@arm.com; r...@linux.ibm.com; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > sujunfei ; Xiaqing (A) > ; Yaobaofeng (Yaobaofeng) > > 主题: RE: [PATCH] arm64: mm: free unused memmap for sparse memory model > that define VMEMMAP > > > > > -Original Message- > > From: liwei (CM) > > Sent: Wednesday, July 8, 2020 7:52 PM > > To: Song Bao Hua (Barry Song) ; > > catalin.mari...@arm.com; w...@kernel.org > > Cc: fengbaopeng ; > > nsaenzjulie...@suse.de; steve.cap...@arm.com; r...@linux.ibm.com; > > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > > sujunfei ; Xiaqing (A) > > ; Yaobaofeng (Yaobaofeng) > > > > Subject: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory > > model that define VMEMMAP > > > > Hi, baohua > > > > Thank you for your attention. > > > > In my understanding of the MEMORY_HOTPLUG this patch has no effect on it. > > The reason is that in sparse_add_one_section() the memory that > > memmap needs from Slab if kernel start completed，this memory has > > nothing to do with memblock alloc/ free memory in the process of kernel > > start. > > > > You may have a look vmemmap_alloc_block () this function. > > > > If I don't understand right welcome pointed out in a timely manner. > > At the first glance of this patch, I suspect that this bootmem may be used by > hot-added memory. > If you confirm this won't happen, please ignore my noise. > > BTW, next time, bear in mind that top-post is not a good way to reply > mail :-) > > > > > Thanks! > > > > > > -邮件原件- > > 发件人: Song Bao Hua (Barry Song) > > 发送时间: 2020年7月8日 15:19 > > 收件人: liwei (CM) ; catalin.mari...@arm.com; > > w...@kernel.org > > 抄送: fengbaopeng ; > > nsaenzjulie...@suse.de; steve.cap...@arm.com; r...@linux.ibm.com; > > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > > sujunfei > > 主题: RE: [PATCH] arm64: mm: free unused memmap for sparse memory > > model that define VMEMMAP > > > > > > > > > -Original Message- > > > From: liwei (CM) > > > Sent: Wednesday, July 8, 2020 1:56 PM > > > To: catalin.mari...@arm.com; w...@kernel.org > > > Cc: liwei (CM) ; fengbaopeng > > > ; nsaenzjulie...@suse.de; > > > steve.cap...@arm.com; r...@linux.ibm.com; Song Bao Hua (Barry > > > Song) ; > > > linux-arm-ker...@lists.infradead.org; > > > linux-kernel@vger.kernel.org; sujunfei > > > Subject: [PATCH] arm64: mm: free unused memmap for sparse memory > > model > > > that define VMEMMAP > > > > > > For the memory hole, sparse memory model that define > > SPARSEMEM_VMEMMAP > > > do not free the reserved memory for the page map, this patch do it. > > > > Hello Wei, > > Just curious if this patch breaks MEMORY_HOTPLUG? > > > > > > > > Signed-off-by: Wei Li > > > Signed-off-by: Chen Feng > > > Signed-off-by: Xia Qing > > > --- > > > arch/arm64/mm/init.c | 81 > > > +--- > > > 1 file changed, 71 insertions(+), 10 deletions(-) > > > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index > > > 1e93cfc7c47a..d1b56b47d5ba 100644 > > > --- a/arch/arm64/mm/init.c > > > +++ b/arch/arm64/mm/init.c > > > @@ -441,7 +441,48 @@ void __init bootmem_init(void) > > > memblock_dump_all(); > > > } > > > > > > > Thanks > > Barry > -- Sincerely yours, Mike.
答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
Hi, all I'm sorry to bother you, but still very hope you can give comments or suggestions to this patch, thank you very much. -邮件原件- 发件人: Song Bao Hua (Barry Song) 发送时间: 2020年7月9日 20:27 收件人: liwei (CM) ; catalin.mari...@arm.com; w...@kernel.org 抄送: fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; r...@linux.ibm.com; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei ; Xiaqing (A) ; Yaobaofeng (Yaobaofeng) 主题: RE: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP > -Original Message- > From: liwei (CM) > Sent: Wednesday, July 8, 2020 7:52 PM > To: Song Bao Hua (Barry Song) ; > catalin.mari...@arm.com; w...@kernel.org > Cc: fengbaopeng ; nsaenzjulie...@suse.de; > steve.cap...@arm.com; r...@linux.ibm.com; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > sujunfei ; Xiaqing (A) > ; Yaobaofeng (Yaobaofeng) > > Subject: 答复: [PATCH] arm64: mm: free unused memmap for sparse memory > model that define VMEMMAP > > Hi, baohua > > Thank you for your attention. > > In my understanding of the MEMORY_HOTPLUG this patch has no effect on it. > The reason is that in sparse_add_one_section() the memory that memmap > needs from Slab if kernel start completed，this memory has nothing to > do with memblock alloc/ free memory in the process of kernel start. > > You may have a look vmemmap_alloc_block () this function. > > If I don't understand right welcome pointed out in a timely manner. At the first glance of this patch, I suspect that this bootmem may be used by hot-added memory. If you confirm this won't happen, please ignore my noise. BTW, next time, bear in mind that top-post is not a good way to reply mail :-) > > Thanks! > > > -邮件原件- > 发件人: Song Bao Hua (Barry Song) > 发送时间: 2020年7月8日 15:19 > 收件人: liwei (CM) ; catalin.mari...@arm.com; > w...@kernel.org > 抄送: fengbaopeng ; nsaenzjulie...@suse.de; > steve.cap...@arm.com; r...@linux.ibm.com; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > sujunfei > 主题: RE: [PATCH] arm64: mm: free unused memmap for sparse memory model > that define VMEMMAP > > > > > -----Original Message- > > From: liwei (CM) > > Sent: Wednesday, July 8, 2020 1:56 PM > > To: catalin.mari...@arm.com; w...@kernel.org > > Cc: liwei (CM) ; fengbaopeng > > ; nsaenzjulie...@suse.de; > > steve.cap...@arm.com; r...@linux.ibm.com; Song Bao Hua (Barry Song) > > ; linux-arm-ker...@lists.infradead.org; > > linux-kernel@vger.kernel.org; sujunfei > > Subject: [PATCH] arm64: mm: free unused memmap for sparse memory > model > > that define VMEMMAP > > > > For the memory hole, sparse memory model that define > SPARSEMEM_VMEMMAP > > do not free the reserved memory for the page map, this patch do it. > > Hello Wei, > Just curious if this patch breaks MEMORY_HOTPLUG? > > > > > Signed-off-by: Wei Li > > Signed-off-by: Chen Feng > > Signed-off-by: Xia Qing > > --- > > arch/arm64/mm/init.c | 81 > > +--- > > 1 file changed, 71 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index > > 1e93cfc7c47a..d1b56b47d5ba 100644 > > --- a/arch/arm64/mm/init.c > > +++ b/arch/arm64/mm/init.c > > @@ -441,7 +441,48 @@ void __init bootmem_init(void) > > memblock_dump_all(); > > } > > > > Thanks > Barry
答复: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP
Hi, baohua Thank you for your attention. In my understanding of the MEMORY_HOTPLUG this patch has no effect on it. The reason is that in sparse_add_one_section() the memory that memmap needs from Slab if kernel start completed，this memory has nothing to do with memblock alloc/ free memory in the process of kernel start. You may have a look vmemmap_alloc_block () this function. If I don't understand right welcome pointed out in a timely manner. Thanks! -邮件原件- 发件人: Song Bao Hua (Barry Song) 发送时间: 2020年7月8日 15:19 收件人: liwei (CM) ; catalin.mari...@arm.com; w...@kernel.org 抄送: fengbaopeng ; nsaenzjulie...@suse.de; steve.cap...@arm.com; r...@linux.ibm.com; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; sujunfei 主题: RE: [PATCH] arm64: mm: free unused memmap for sparse memory model that define VMEMMAP > -Original Message- > From: liwei (CM) > Sent: Wednesday, July 8, 2020 1:56 PM > To: catalin.mari...@arm.com; w...@kernel.org > Cc: liwei (CM) ; fengbaopeng > ; nsaenzjulie...@suse.de; > steve.cap...@arm.com; r...@linux.ibm.com; Song Bao Hua (Barry Song) > ; linux-arm-ker...@lists.infradead.org; > linux-kernel@vger.kernel.org; sujunfei > Subject: [PATCH] arm64: mm: free unused memmap for sparse memory model > that define VMEMMAP > > For the memory hole, sparse memory model that define SPARSEMEM_VMEMMAP > do not free the reserved memory for the page map, this patch do it. Hello Wei, Just curious if this patch breaks MEMORY_HOTPLUG? > > Signed-off-by: Wei Li > Signed-off-by: Chen Feng > Signed-off-by: Xia Qing > --- > arch/arm64/mm/init.c | 81 > +--- > 1 file changed, 71 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index > 1e93cfc7c47a..d1b56b47d5ba 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -441,7 +441,48 @@ void __init bootmem_init(void) > memblock_dump_all(); > } > Thanks Barry
答复: [PATCH v2 3/3] scsi: ufs: Add HI3670 SoC UFS driver support
Fine to me. Thanks!
Acked-by: Wei Li
-邮件原件-
发件人: Manivannan Sadhasivam [mailto:manivannan.sadhasi...@linaro.org]
发送时间: 2019年1月5日 15:29
收件人: vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com; liwei (CM); robh...@kernel.org
抄送: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org;
john.stu...@linaro.org; amit.kuche...@linaro.org; guodong...@linaro.org;
Manivannan Sadhasivam
主题: [PATCH v2 3/3] scsi: ufs: Add HI3670 SoC UFS driver support
Add HI3670 SoC UFS driver support by extending the common ufs-hisi
driver. One major difference between HI3660 ad HI3670 SoCs interms of
UFS is the PHY. HI3670 has a 10nm variant PHY and hence this parameter is
used to distinguish the configuration.
Signed-off-by: Manivannan Sadhasivam
---
drivers/scsi/ufs/ufs-hisi.c | 127 +---
drivers/scsi/ufs/ufs-hisi.h | 4 ++
2 files changed, 109 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 452e19f8fb47..f2d3df357a97 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -66,7 +66,7 @@ static int ufs_hisi_check_hibern8(struct ufs_hba *hba)
return err;
}
-static void ufs_hi3660_clk_init(struct ufs_hba *hba)
+static void ufs_hisi_clk_init(struct ufs_hba *hba)
{
struct ufs_hisi_host *host = ufshcd_get_variant(hba);
@@ -80,7 +80,7 @@ static void ufs_hi3660_clk_init(struct ufs_hba *hba)
ufs_sys_ctrl_set_bits(host, BIT_SYSCTRL_REF_CLOCK_EN, PHY_CLK_CTRL);
}
-static void ufs_hi3660_soc_init(struct ufs_hba *hba)
+static void ufs_hisi_soc_init(struct ufs_hba *hba)
{
struct ufs_hisi_host *host = ufshcd_get_variant(hba);
u32 reg;
@@ -139,6 +139,7 @@ static void ufs_hi3660_soc_init(struct ufs_hba *hba)
static int ufs_hisi_link_startup_pre_change(struct ufs_hba *hba)
{
+ struct ufs_hisi_host *host = ufshcd_get_variant(hba);
int err;
uint32_t value;
uint32_t reg;
@@ -153,6 +154,14 @@ static int ufs_hisi_link_startup_pre_change(struct ufs_hba
*hba)
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8121, 0x0), 0x2D);
/* MPHY CBOVRCTRL3 */
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8122, 0x0), 0x1);
+
+ if (host->caps & UFS_HISI_CAP_PHY10nm) {
+ /* MPHY CBOVRCTRL4 */
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8127, 0x0), 0x98);
+ /* MPHY CBOVRCTRL5 */
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8128, 0x0), 0x1);
+ }
+
/* Unipro VS_MphyCfgUpdt */
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xD085, 0x0), 0x1);
/* MPHY RXOVRCTRL4 rx0 */
@@ -173,10 +182,21 @@ static int ufs_hisi_link_startup_pre_change(struct
ufs_hba *hba)
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8113, 0x0), 0x1);
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xD085, 0x0), 0x1);
- /* Tactive RX */
- ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008F, 0x4), 0x7);
- /* Tactive RX */
- ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008F, 0x5), 0x7);
+ if (host->caps & UFS_HISI_CAP_PHY10nm) {
+ /* RX_Hibern8Time_Capability*/
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x0092, 0x4), 0xA);
+ /* RX_Hibern8Time_Capability*/
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x0092, 0x5), 0xA);
+ /* RX_Min_ActivateTime */
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008f, 0x4), 0xA);
+ /* RX_Min_ActivateTime*/
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008f, 0x5), 0xA);
+ } else {
+ /* Tactive RX */
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008F, 0x4), 0x7);
+ /* Tactive RX */
+ ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x008F, 0x5), 0x7);
+ }
/* Gear3 Synclength */
ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x0095, 0x4), 0x4F);
@@ -208,7 +228,8 @@ static int ufs_hisi_link_startup_pre_change(struct ufs_hba
*hba)
if (err)
dev_err(hba->dev, "ufs_hisi_check_hibern8 error\n");
- ufshcd_writel(hba, UFS_HCLKDIV_NORMAL_VALUE, UFS_REG_HCLKDIV);
+ if (!(host->caps & UFS_HISI_CAP_PHY10nm))
+ ufshcd_writel(hba, UFS_HCLKDIV_NORMAL_VALUE, UFS_REG_HCLKDIV);
/* disable auto H8 */
reg = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
@@ -253,7 +274,7 @@ static int ufs_hisi_link_startup_post_change(struct ufs_hba
*hba)
return 0;
}
-static int ufs_hi3660_link_startup_notify(struct ufs_hba *hba,
+static int ufs_hisi_link_startup_notify(struct ufs_hba *hba,
enum ufs_notify_change_status status)
{
int err = 0;
@@ -391,6 +412,28 @@ static void ufs_hisi_set_dev_cap(struct
ufs_hisi_dev_params *hisi_param)
static void ufs_hisi_pwr_change_pre_change(struct ufs_hba *hba)
{
答复: [PATCH v2 2/3] arm64: dts: hisilicon: hi3670: Add UFS controller support
Fine to me. Thanks!
Acked-by: Wei Li
-邮件原件-
发件人: Manivannan Sadhasivam [mailto:manivannan.sadhasi...@linaro.org]
发送时间: 2019年1月5日 15:29
收件人: vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com; liwei (CM); robh...@kernel.org
抄送: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org;
john.stu...@linaro.org; amit.kuche...@linaro.org; guodong...@linaro.org;
Manivannan Sadhasivam
主题: [PATCH v2 2/3] arm64: dts: hisilicon: hi3670: Add UFS controller support
Add UFS controller support for HiSilicon HI3670 SoC.
Signed-off-by: Manivannan Sadhasivam
---
arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 18 ++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index 6ccdf5040ffd..285219dd657f 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -654,6 +654,24 @@
clock-names = "apb_pclk";
};
+ /* UFS */
+ ufs: ufs@ff3c {
+ compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
+ /* 0: HCI standard */
+ /* 1: UFS SYS CTRL */
+ reg = <0x0 0xff3c 0x0 0x1000>,
+ <0x0 0xff3e 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = ;
+ clocks = <&crg_ctrl HI3670_CLK_GATE_UFSIO_REF>,
+ <&crg_ctrl HI3670_CLK_GATE_UFS_SUBSYS>;
+ clock-names = "ref_clk", "phy_clk";
+ freq-table-hz = <0 0>, <0 0>;
+ /* offset: 0x84; bit: 12 */
+ resets = <&crg_rst 0x84 12>;
+ reset-names = "rst";
+ };
+
/* SD */
dwmmc1: dwmmc1@ff37f000 {
compatible = "hisilicon,hi3670-dw-mshc";
--
2.17.1
答复: [PATCH v2 1/3] dt-bindings: ufs: Add HI3670 UFS controller binding
Fine to me. Thanks! Acked-by: Wei Li -邮件原件- 发件人: Manivannan Sadhasivam [mailto:manivannan.sadhasi...@linaro.org] 发送时间: 2019年1月5日 15:29 收件人: vinholika...@gmail.com; j...@linux.vnet.ibm.com; martin.peter...@oracle.com; liwei (CM); robh...@kernel.org 抄送: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; john.stu...@linaro.org; amit.kuche...@linaro.org; guodong...@linaro.org; Manivannan Sadhasivam 主题: [PATCH v2 1/3] dt-bindings: ufs: Add HI3670 UFS controller binding Add devicetree binding for HI3670 UFS controller. HI3760 SoC is very similar to HI3660 SoC with almost same IPs. Only major difference in terms of UFS is the PHY. HI3670 has 10nm PHY. But since the original driver (HI3660 UFS) cannot make HI3670 UFS functional, a separate compatible is added for HI3670 without any fallback. Signed-off-by: Manivannan Sadhasivam --- Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt index a48c44817367..0b83df1a5418 100644 --- a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt @@ -6,9 +6,10 @@ Each UFS Host Controller should have its own node. Required properties: - compatible: compatible list, contains one of the following - "hisilicon,hi3660-ufs", "jedec,ufs-1.1" for hisi ufs - host controller present on Hi36xx chipset. + host controller present on Hi3660 chipset. + "hisilicon,hi3670-ufs", "jedec,ufs-2.1" for hisi ufs + host controller present on Hi3670 chipset. - reg : should contain UFS register address space & UFS SYS CTRL register address, -- interrupt-parent : interrupt device - interrupts: interrupt number - clocks : List of phandle and clock specifier pairs - clock-names : List of clock input name strings sorted in the same -- 2.17.1
答复: 答复: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Rob
-邮件原件-
发件人: Rob Herring [mailto:r...@kernel.org]
发送时间: 2018年5月16日 21:16
收件人: liwei (CM)
抄送: mark.rutl...@arm.com; catalin.mari...@arm.com; will.dea...@arm.com;
vinholika...@gmail.com; j...@linux.vnet.ibm.com; martin.peter...@oracle.com;
khil...@baylibre.com; a...@arndb.de; gregory.clem...@free-electrons.com;
thomas.petazz...@free-electrons.com; yamada.masah...@socionext.com;
riku.voi...@linaro.org; tred...@nvidia.com; k...@kernel.org;
devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; linux-s...@vger.kernel.org; zangleigang;
Gengjianfeng; guodong...@linaro.org
主题: Re: 答复: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
On Tue, Apr 24, 2018 at 8:54 AM, liwei (CM) wrote:
> Hi, Rob
>
> Thanks for your patience.
>
> Hi, Arnd
>
> From Rob's suggestion, we have to list the properties node in ufs-hisi.txt
> bingings even if documented in the common binding.
>
> -邮件原件-
> 发件人: Rob Herring [mailto:r...@kernel.org]
> 发送时间: 2018年4月24日 20:58
> 收件人: liwei (CM)
> 抄送: mark.rutl...@arm.com; catalin.mari...@arm.com; will.dea...@arm.com;
> vinholika...@gmail.com; j...@linux.vnet.ibm.com; martin.peter...@oracle.com;
> khil...@baylibre.com; a...@arndb.de; gregory.clem...@free-electrons.com;
> thomas.petazz...@free-electrons.com; yamada.masah...@socionext.com;
> riku.voi...@linaro.org; tred...@nvidia.com; k...@kernel.org;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-s...@vger.kernel.org;
> zangleigang; Gengjianfeng; guodong...@linaro.org
> 主题: Re: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
>
> On Tue, Apr 17, 2018 at 10:08:11PM +0800, Li Wei wrote:
>> add ufs node document for Hisilicon.
>>
>> Signed-off-by: Li Wei
>> ---
>> Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 29
>> ++
>> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 10 +---
>> 2 files changed, 36 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> new file mode 100644
>> index ..d49ab7d8f31d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> @@ -0,0 +1,29 @@
>> +* Hisilicon Universal Flash Storage (UFS) Host Controller
>> +
>> +UFS nodes are defined to describe on-chip UFS hardware macro.
>> +Each UFS Host Controller should have its own node.
>> +
>> +Required properties:
>> +- compatible: compatible list, contains one of the following -
>> + "hisilicon,hi3660-ufs",
>> "jedec,ufs-1.1" for hisi ufs
>> + host controller present on Hi36xx
>> chipset.
>> +- reg : should contain UFS register address space & UFS SYS
>> CTRL register address,
>> +- interrupt-parent : interrupt device
>> +- interrupts: interrupt number
>> +- resets: reset node register, the "arst" corresponds to reset
>> the APB/AXI bus.
>
> arst belongs in reset-names.
>
> OK, I will fix it in next patch;
>
>> +- reset-names : describe reset node register
>
> What happened to clocks? You still have to list which ones apply even if
> documented in the common binding.
>
> OK, I will fix it in next patch;
>
>> +
>> +Example:
>> +
>> + ufs: ufs@ff3b {
>> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
>> + /* 0: HCI standard */
>> + /* 1: UFS SYS CTRL */
>> + reg = <0x0 0xff3b 0x0 0x1000>,
>> + <0x0 0xff3b1000 0x0 0x1000>;
>> + interrupt-parent = <&gic>;
>> + interrupts = ;
>> + /* offset: 0x84; bit: 7 */
>> + resets = <&crg_rst 0x84 7>;
>> + reset-names = "arst";
>> + };
>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> index c39dfef76a18..adcfb79f63f5 100644
>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> @@ -41,6 +41,8 @@ Optional properties:
>> -lanes-per-direction : number of lanes available per direction - either 1
>> or 2.
>> Note that it
答复: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor
+jinguojun jingbing
-邮件原件-
发件人: Shawn Lin [mailto:shawn@rock-chips.com]
发送时间: 2018年4月8日 9:52
收件人: Ryan Grachek
抄送: shawn@rock-chips.com; Jaehoon Chung; Ulf Hansson;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Zhangfei Gao; liwei
(CM); Suzhuangluan
主题: Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor
On 2018/4/6 21:41, Ryan Grachek wrote:
> On Wed, Apr 4, 2018 at 7:51 PM, Shawn Lin wrote:
>> [+ Zhangfei Gao who added support for hi6220]
>>
>> On 2018/4/4 23:31, Ryan Grachek wrote:
>>>
>>> On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin >> <mailto:shawn@rock-chips.com>> wrote:
>>>
>>> On 2018/3/30 2:24, oscardagrach wrote:
>>>
>>> Need at least one line commit body.
>>>
>>> Signed-off-by: oscardagrach >> <mailto:r...@edited.us>>
>>>
>>> ---
>>> drivers/mmc/host/dw_mmc-k3.c | 10 --
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc-k3.c
>>> b/drivers/mmc/host/dw_mmc-k3.c
>>> index 89cdb3d533bb..efc546cb4db8 100644
>>> --- a/drivers/mmc/host/dw_mmc-k3.c
>>> +++ b/drivers/mmc/host/dw_mmc-k3.c
>>> @@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
>>> dw_mci *host, struct mmc_ios *ios)
>>> int ret;
>>> unsigned int clock;
>>> - clock = (ios->clock <= 2500) ? 2500 : ios->clock;
>>> -
>>> + /* CLKDIV must be 1 for DDR52/8-bit mode */
>>> + if (ios->bus_width == MMC_BUS_WIDTH_8 &&
>>> + ios->timing == MMC_TIMING_MMC_DDR52) {
>>> + mci_writel(host, CLKDIV, 0x1);
>>> + clock = ios->clock;
>>> + } else {
>>> + clock = (ios->clock <= 2500) ? 2500 :
>>> ios->clock;
>>> + }
>>>
>>>
>>> I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the
>>> following
>>> change is more sensible?
>>>
>>> if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
>>> MMC_TIMING_MMC_DDR52)
>>> clock = ios->clock * 2;
>>> else
>>> clock = (ios->clock <= 2500) ? 2500 :
>>> ios->clock;
>>>
>>>
>>> The reason is ios->clock is 52MHz and you could claim 104MHz from the
>>> clock provider and let dw_mmc core take care of the divder to be 1.
>>> Otherwise, you just force it to be DDR52/8-bit with a clk rate
>>> of 26MHz.
>>>
>>>
>>> ret = clk_set_rate(host->biu_clk, clock);
>>> if (ret)
>>> dev_warn(host->dev, "failed to set rate
>>> %uHz\n", clock);
>>>
>>>
>>>
>>
>> For future wise, please use plain mode mail, but not HTML format.
>>
>>> Your feedback is correct. I see the Rockchip dwmmc driver has a
>>> similar implementation. After applying your suggested changes,
>>> however, my board reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate
>>> 10400Hz"
>>> during intialization of eMMC. In addition, I do not see CLKDIV being
>>> set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
>>> the driver.
>>>
>>> If I set CLKDIV where I did prior, with your changes, the device
>>> fails to set the clock and falls back to 52 MHz (26 MHz) and works
>>> fine, but again, CLKDIV is reported as 0 (even though it is 1.) One
>>> thing of interest to note is when I manually set the clock by doing:
>>> (echo 10400 > /sys/kernel/debug/mmc0/clock) the device reports
>>> back 'mmc_host mmc0: Bus speed (slot 0) = 19840Hz (slot req 10400Hz,
>>>actual 9920HZ div = 1)' which works reliably and clk_set_rate
>>> does not report any error.
>>>
>>
>> When looking closely into the code, at least dw_mci_hi6220_set_ios
>> goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
>> "b" stands for bus, and "c" stands for card IMHO, however bus_hz
>> describs the clock to the card, pro
答复: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Thanks for your patiences. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年3月28日 20:50 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: Re: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Mar 27, 2018 at 8:15 AM, liwei (CM) wrote: > Hi, Arnd > > At present our ufs module mainly has four clocks from the outside: > hclk_ufs: main clock of ufs controller ,freq is 207.5MHz > cfg_phy_clk: configuration clock of MPHY, freq is 51.875MHz > ref_phy_clk: reference clock of MPHY from PMU, freq is 19.2MHz > ref_io_clk:reference clock for the external interface to the device, freq > is 19.2MHz > > We control two clocks "ref_io_clk" and "cfg_phy_clk" in the driver > because the other two are controlled by main clock module and pmu. I'm not completely sure what you mean with "control" here. Do you mean setting the rate and disabling them during runtime power management? What does it mean for the clock to be controlled by teh "main clock module and pmu"? In the driver we only disable/enable "ref_io_clk" and "cfg_phy_clk" during runtime power management. > for this patch, cfg_phy_clk corresponds to "phy_clk", ref_io_clk corresponds > to "ref_clk". I'm not sure I understand the difference between ref_phy_clk and ref_io_clk, but it sounds like we should give both of those names in the ufs-platform binding. Your hclk_ufs would appear to correspond to what qualcomm calls core_clk, so maybe use that name as well. cfg_phy_clk seems to be something that qcom would not have, but it's also generic enough to list it in the common binding. Ok, let's add a describe for phy_clk in the common binding. > So the clks in the patch you give appear to be unsuitable for describing this > .And the following clks of qcom are internal clock? > We didn't describe or pay attention to the clock inside the ufs module. > > PHY to controller symbol synchronization clocks: > "rx_lane0_sync_clk" - RX Lane 0 > "rx_lane1_sync_clk" - RX Lane 1 > "tx_lane0_sync_clk" - TX Lane 0 > "tx_lane1_sync_clk" - TX Lane 1 Right, let's leave those for the qcom private binding. Arnd
答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd At present our ufs module mainly has four clocks from the outside: hclk_ufs: main clock of ufs controller ,freq is 207.5MHz cfg_phy_clk: configuration clock of MPHY, freq is 51.875MHz ref_phy_clk: reference clock of MPHY from PMU, freq is 19.2MHz ref_io_clk:reference clock for the external interface to the device, freq is 19.2MHz We control two clocks "ref_io_clk" and "cfg_phy_clk" in the driver because the other two are controlled by main clock module and pmu. for this patch, cfg_phy_clk corresponds to "phy_clk", ref_io_clk corresponds to "ref_clk". So the clks in the patch you give appear to be unsuitable for describing this .And the following clks of qcom are internal clock? We didn't describe or pay attention to the clock inside the ufs module. PHY to controller symbol synchronization clocks: "rx_lane0_sync_clk" - RX Lane 0 "rx_lane1_sync_clk" - RX Lane 1 "tx_lane0_sync_clk" - TX Lane 0 "tx_lane1_sync_clk" - TX Lane 1 -邮件原件- 发件人: liwei (CM) 发送时间: 2018年3月26日 20:02 收件人: 'Arnd Bergmann' 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs Hi, Arnd I'll ask our soc colleagues for help and give a detailed and accurate explanation aosp. Thanks! -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年3月26日 18:42 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: Re: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Mon, Mar 26, 2018 at 12:26 PM, liwei (CM) wrote: > 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd > Bergmann > > 主题: Re: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for > > hisi-ufs On Fri, Mar 23, 2018 at 3:22 AM, liwei (CM) > > wrote: > >> The clock names sound generic enough, should we have both in the generic > >> binding? > >> > >> Do you mean that add a "phy_clk" to ufshcd-pltfrm 's bindings? > >> At present, it seems that in the implementation of generic code, > >> apart from "ref_clk" may have special processing, other clk will > >> not have special processing and simply parse and enable; Referring > >> to ufs-qcom binding, I think "phy_clk" can be named "iface_clk", > >> this "iface_clk" exists in ufshcd-pltfrm bindings;If so, "ref_clk", > >> "iface_clk" are both in the generic binding,we will remove them here. Is > >> that okay? > > > I'm looking at the generic binding again, and it seems we never > > quite managed to fix some minor problems with it. See below for a possible > > way to clarify it. > > phy_clk is actually given to the phy. But as previously mentioned , we > do not have a separate phy to configure ; The clks in the patch you > give appear to be unsuitable for describing this . > Here we can't describe phy_clk in the node "ufsphy1: ufsphy@fc597000" like > qcom. > So can we put it here in our own binding like this? I think the concept of having a phy clk is generic enough that it's better to have that in the common part, others will surely have the same thing, and in this case, qcom would be the exception that does not use one. There are apparently a couple of things related to the phy that may or may not require a clk: - ref_clk: The reference clock on the mipi bus, this is what qcom have, this would be the 19.2 MHz clock signal. - one clock to drive the logic block for the PHY itself, if it is included within the same logical portion of an SoC as the ufshcd, but uses a separate clock. - Looking at the Android kernel as distributed by google/qualcomm, they have four separate clocks described as PHY to controller symbol synchronization clocks: "rx_lane0_sync_clk" - RX Lane 0 "rx_lane1_sync_clk" - RX Lane 1 "tx_lane0_sync_clk" - TX Lane 0 "tx_lane1_sync_clk" - TX Lane 1 Which of the above would your phy_clk refer to? Arnd [1] https://android.googlesource.com/kernel/msm/+/android-msm-bullhead-3.10-marshmallow-dr/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt?autodive=0%2F%2F%2F%2F%2F
答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd I'll ask our soc colleagues for help and give a detailed and accurate explanation aosp. Thanks! -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年3月26日 18:42 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: Re: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Mon, Mar 26, 2018 at 12:26 PM, liwei (CM) wrote: > 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd > Bergmann > > 主题: Re: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for > > hisi-ufs On Fri, Mar 23, 2018 at 3:22 AM, liwei (CM) > > wrote: > >> The clock names sound generic enough, should we have both in the generic > >> binding? > >> > >> Do you mean that add a "phy_clk" to ufshcd-pltfrm 's bindings? > >> At present, it seems that in the implementation of generic code, > >> apart from "ref_clk" may have special processing, other clk will > >> not have special processing and simply parse and enable; Referring > >> to ufs-qcom binding, I think "phy_clk" can be named "iface_clk", > >> this "iface_clk" exists in ufshcd-pltfrm bindings;If so, "ref_clk", > >> "iface_clk" are both in the generic binding,we will remove them here. Is > >> that okay? > > > I'm looking at the generic binding again, and it seems we never > > quite managed to fix some minor problems with it. See below for a possible > > way to clarify it. > > phy_clk is actually given to the phy. But as previously mentioned , we > do not have a separate phy to configure ; The clks in the patch you > give appear to be unsuitable for describing this . > Here we can't describe phy_clk in the node "ufsphy1: ufsphy@fc597000" like > qcom. > So can we put it here in our own binding like this? I think the concept of having a phy clk is generic enough that it's better to have that in the common part, others will surely have the same thing, and in this case, qcom would be the exception that does not use one. There are apparently a couple of things related to the phy that may or may not require a clk: - ref_clk: The reference clock on the mipi bus, this is what qcom have, this would be the 19.2 MHz clock signal. - one clock to drive the logic block for the PHY itself, if it is included within the same logical portion of an SoC as the ufshcd, but uses a separate clock. - Looking at the Android kernel as distributed by google/qualcomm, they have four separate clocks described as PHY to controller symbol synchronization clocks: "rx_lane0_sync_clk" - RX Lane 0 "rx_lane1_sync_clk" - RX Lane 1 "tx_lane0_sync_clk" - TX Lane 0 "tx_lane1_sync_clk" - TX Lane 1 Which of the above would your phy_clk refer to? Arnd [1] https://android.googlesource.com/kernel/msm/+/android-msm-bullhead-3.10-marshmallow-dr/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt?autodive=0%2F%2F%2F%2F%2F
答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年3月26日 17:14 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: Re: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Fri, Mar 23, 2018 at 3:22 AM, liwei (CM) wrote: >> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt >> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt >> new file mode 100644 >> index ..0d21b57496cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt >> @@ -0,0 +1,37 @@ >> +* Hisilicon Universal Flash Storage (UFS) Host Controller >> + >> +UFS nodes are defined to describe on-chip UFS hardware macro. >> +Each UFS Host Controller should have its own node. >> + >> +Required properties: >> +- compatible: compatible list, contains one of the following - >> + "hisilicon,hi3660-ufs", >> "jedec,ufs-1.1" for hisi ufs >> + host controller present on Hi36xx >> chipset. >> +- reg : should contain UFS register address space & UFS SYS >> CTRL register address, >> +- interrupt-parent : interrupt device >> +- interrupts: interrupt number >> +- clocks : List of phandle and clock specifier pairs >> +- clock-names : List of clock input name strings sorted in the same >> + order as the clocks property. >> +"ref_clk", "phy_clk" is optional > > The clock names sound generic enough, should we have both in the generic > binding? > > Do you mean that add a "phy_clk" to ufshcd-pltfrm 's bindings? > At present, it seems that in the implementation of generic code, apart > from "ref_clk" may have special processing, other clk will not have special > processing and simply parse and enable; Referring to ufs-qcom binding, I > think "phy_clk" can be named "iface_clk", this "iface_clk" exists in > ufshcd-pltfrm bindings;If so, "ref_clk", "iface_clk" are both in the generic > binding,we will remove them here. Is that okay? I'm looking at the generic binding again, and it seems we never quite managed to fix some minor problems with it. See below for a possible way to clarify it. phy_clk is actually given to the phy. But as previously mentioned , we do not have a separate phy to configure ; The clks in the patch you give appear to be unsuitable for describing this . Here we can't describe phy_clk in the node "ufsphy1: ufsphy@fc597000" like qcom. So can we put it here in our own binding like this? >> +- resets: reset node register, one reset the clk and the other >> reset the controller >> +- reset-names : describe reset node register > > This looks incomplete. What is the name of the reset line supposed to be? > I'd also suggest you document it in the ufshcd binding instead. > > The "rst" corresponds to reset the whole UFS IP, and " arst " only reset the > APB/AXI bus. Discussed with our soc colleagues that "arst" is assert by > default and needs to deassert . > But I think it may be difficult to add this to common code, or it may > not be necessary; Other manufacturers may not need to do this soc init > because they probably already done in the bootloader phase. Even if they need > to do it, it's probably different from us. > We need to make sure that our ufs works even if not do soc init during the > bootloader phase. In the suggested patch below, I have documented one "rst" line that is used to reset the ufshcd device. The second reset line as I understand now is used in a rather nonstandard way and gets asserted only while setting up the additional registers for your glue logic, so that one seems better left documented in your own binding. Yes, the second reset line is used in a rather nonstandard way , if rst will into the common document, ,I will left the second reset line documented in our own binding. I've added a "jedec,ufshci-3.0" compatible string, which appears to be the latest version of the ufshci itself, and I've documented four clocks that are already used by the qua
答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Sorry to bother you again, please take the time to review the patch. Are there any other suggestions? Looking forward to your reply. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年2月19日 17:58 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Feb 13, 2018 at 11:14 AM, Li Wei wrote: > add ufs node document for Hisilicon. > > Signed-off-by: Li Wei > --- > Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 37 > ++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt I'm pretty sure we've discussed it before, but can you make this so that the generic properties are part of the ufshcd binding, and you refer to it from here, only describing in what ways the hisi ufs binding differs from the standard? > diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > new file mode 100644 > index ..0d21b57496cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > @@ -0,0 +1,37 @@ > +* Hisilicon Universal Flash Storage (UFS) Host Controller > + > +UFS nodes are defined to describe on-chip UFS hardware macro. > +Each UFS Host Controller should have its own node. > + > +Required properties: > +- compatible: compatible list, contains one of the following - > + "hisilicon,hi3660-ufs", > "jedec,ufs-1.1" for hisi ufs > + host controller present on Hi36xx > chipset. > +- reg : should contain UFS register address space & UFS SYS > CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts: interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. > +"ref_clk", "phy_clk" is optional The clock names sound generic enough, should we have both in the generic binding? Do you mean that add a "phy_clk" to ufshcd-pltfrm 's bindings? At present, it seems that in the implementation of generic code, apart from "ref_clk" may have special processing, other clk will not have special processing and simply parse and enable; Referring to ufs-qcom binding, I think "phy_clk" can be named "iface_clk", this "iface_clk" exists in ufshcd-pltfrm bindings;If so, "ref_clk", "iface_clk" are both in the generic binding,we will remove them here. Is that okay? > +- resets: reset node register, one reset the clk and the other > reset the controller > +- reset-names : describe reset node register This looks incomplete. What is the name of the reset line supposed to be? I'd also suggest you document it in the ufshcd binding instead. The "rst" corresponds to reset the whole UFS IP, and " arst " only reset the APB/AXI bus. Discussed with our soc colleagues that "arst" is assert by default and needs to deassert . But I think it may be difficult to add this to common code, or it may not be necessary; Other manufacturers may not need to do this soc init because they probably already done in the bootloader phase. Even if they need to do it, it's probably different from us. We need to make sure that our ufs works even if not do soc init during the bootloader phase. Arnd
答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi，Arnd Sorry to bother you again, please take the time to review the patch. Are there any other suggestions? Looking forward to your reply. Thanks! -邮件原件- 发件人: liwei (CM) 发送时间: 2018年2月23日 16:36 收件人: 'Arnd Bergmann' 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept) 主题: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs Hi, Arnd Sorry late for you. The following two suggestions we have really discussed https://lkml.org/lkml/2017/11/30/1077 -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年2月19日 17:58 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Feb 13, 2018 at 11:14 AM, Li Wei wrote: > add ufs node document for Hisilicon. > > Signed-off-by: Li Wei > --- > Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 37 > ++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt I'm pretty sure we've discussed it before, but can you make this so that the generic properties are part of the ufshcd binding, and you refer to it from here, only describing in what ways the hisi ufs binding differs from the standard? > diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > new file mode 100644 > index ..0d21b57496cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > @@ -0,0 +1,37 @@ > +* Hisilicon Universal Flash Storage (UFS) Host Controller > + > +UFS nodes are defined to describe on-chip UFS hardware macro. > +Each UFS Host Controller should have its own node. > + > +Required properties: > +- compatible: compatible list, contains one of the following - > + "hisilicon,hi3660-ufs", > "jedec,ufs-1.1" for hisi ufs > + host controller present on Hi36xx > chipset. > +- reg : should contain UFS register address space & UFS SYS > CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts: interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. > +"ref_clk", "phy_clk" is optional The clock names sound generic enough, should we have both in the generic binding? "ref_clk" is in the ufshcd-pltfrm binding, but "phy_clk" is not; what do you mean is that these two don't need to document here? > +- resets: reset node register, one reset the clk and the other > reset the controller > +- reset-names : describe reset node register This looks incomplete. What is the name of the reset line supposed to be? I'd also suggest you document it in the ufshcd binding instead. As discussed in https://lkml.org/lkml/2017/11/30/1077; If document it in the ufshcd binding, I think it needs some codes to parse them in ufshcd.c/ufshcd-pltfrm.c, but I think these codes may not be applicable to other SOC manufacturers. Arnd
答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Sorry late for you. The following two suggestions we have really discussed https://lkml.org/lkml/2017/11/30/1077 -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年2月19日 17:58 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Feb 13, 2018 at 11:14 AM, Li Wei wrote: > add ufs node document for Hisilicon. > > Signed-off-by: Li Wei > --- > Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 37 > ++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt I'm pretty sure we've discussed it before, but can you make this so that the generic properties are part of the ufshcd binding, and you refer to it from here, only describing in what ways the hisi ufs binding differs from the standard? > diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > new file mode 100644 > index ..0d21b57496cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > @@ -0,0 +1,37 @@ > +* Hisilicon Universal Flash Storage (UFS) Host Controller > + > +UFS nodes are defined to describe on-chip UFS hardware macro. > +Each UFS Host Controller should have its own node. > + > +Required properties: > +- compatible: compatible list, contains one of the following - > + "hisilicon,hi3660-ufs", > "jedec,ufs-1.1" for hisi ufs > + host controller present on Hi36xx > chipset. > +- reg : should contain UFS register address space & UFS SYS > CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts: interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. > +"ref_clk", "phy_clk" is optional The clock names sound generic enough, should we have both in the generic binding? "ref_clk" is in the ufshcd-pltfrm binding, but "phy_clk" is not; what do you mean is that these two don't need to document here? > +- resets: reset node register, one reset the clk and the other > reset the controller > +- reset-names : describe reset node register This looks incomplete. What is the name of the reset line supposed to be? I'd also suggest you document it in the ufshcd binding instead. As discussed in https://lkml.org/lkml/2017/11/30/1077; If document it in the ufshcd binding, I think it needs some codes to parse them in ufshcd.c/ufshcd-pltfrm.c, but I think these codes may not be applicable to other SOC manufacturers. Arnd
答复: [PATCH v7 0/5] scsi: ufs: add ufs driver code for Hisilicon Hi3660 SoC
Hi. Zhangfei Thank you, I will add it in the next patch. -邮件原件- 发件人: zhangfei [mailto:zhangfei@linaro.org] 发送时间: 2018年1月8日 9:40 收件人: liwei (CM); robh...@kernel.org; mark.rutl...@arm.com; xuwei (O); catalin.mari...@arm.com; will.dea...@arm.com; vinholika...@gmail.com; j...@linux.vnet.ibm.com; martin.peter...@oracle.com; khil...@baylibre.com; a...@arndb.de; gregory.clem...@free-electrons.com; thomas.petazz...@free-electrons.com; yamada.masah...@socionext.com; riku.voi...@linaro.org; tred...@nvidia.com; k...@kernel.org; e...@anholt.net; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-s...@vger.kernel.org 抄送: zangleigang; Gengjianfeng; guodong...@linaro.org; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v7 0/5] scsi: ufs: add ufs driver code for Hisilicon Hi3660 SoC Hi, Wei On 2018年01月06日 17:51, Li Wei wrote: > This patchset adds driver support for UFS for Hi3660 SoC. It is verified on > HiKey960 board. Usually here should list the change compared with the last change set, to make it easier to reviewer, who may pay more attention to the differences. For example v7:xxx //change since v6 v6:xxx // change since v5 > Li Wei (5): >scsi: ufs: add Hisilicon ufs driver code >dt-bindings: scsi: ufs: add document for hisi-ufs >arm64: dts: add ufs dts node >arm64: defconfig: enable configs for Hisilicon ufs >arm64: defconfig: enable f2fs and squashfs > > Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 43 ++ > arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 20 + > arch/arm64/configs/defconfig | 11 + > drivers/scsi/ufs/Kconfig | 9 + > drivers/scsi/ufs/Makefile | 1 + > drivers/scsi/ufs/ufs-hisi.c| 621 > + > drivers/scsi/ufs/ufs-hisi.h| 116 > 7 files changed, 821 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt > create mode 100644 drivers/scsi/ufs/ufs-hisi.c > create mode 100644 drivers/scsi/ufs/ufs-hisi.h >
答复: [PATCH v6 1/5] scsi: ufs: add Hisilicon ufs driver code
Hi,Philippe, Thank you for your suggestion, and I'll consider that next patch. -邮件原件- 发件人: Philippe Ombredanne [mailto:pombreda...@nexb.com] 发送时间: 2017年12月7日 18:34 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; vinholika...@gmail.com; James E.J. Bottomley; Martin K. Petersen; khil...@baylibre.com; Arnd Bergmann; gregory.clem...@free-electrons.com; Thomas Petazzoni; yamada.masah...@socionext.com; riku.voi...@linaro.org; tred...@nvidia.com; k...@kernel.org; e...@anholt.net; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS; LKML; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE; linux-s...@vger.kernel.org; zangleigang; Gengjianfeng; guodong...@linaro.org; zhangfei@linaro.org; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v6 1/5] scsi: ufs: add Hisilicon ufs driver code Dear Li, On Thu, Dec 7, 2017 at 11:20 AM, Li Wei wrote: > add Hisilicon ufs driver code. > > Signed-off-by: Li Wei > Signed-off-by: Geng Jianfeng > Signed-off-by: Zang Leigang > Signed-off-by: Yu Jianfeng [] > --- /dev/null > +++ b/drivers/scsi/ufs/ufs-hisi.c > @@ -0,0 +1,624 @@ > +/* > + * > + * HiSilicon Hi UFS Driver > + * > + * Copyright (c) 2016-2017 Linaro Ltd. > + * Copyright (c) 2016-2017 HiSilicon Technologies Co., Ltd. > + * > + * This program is free software; you can redistribute it and/or > +modify > + * it under the terms of the GNU General Public License as published > +by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ Would you consider using the new SPDX license ids instead? Check Thomas doc patches for instructions. This would be great and while you are it may be this could be adopted for all HiSilicon and Huawei past, present and future contributions? Thank you for your kind consideration! -- Cordially Philippe Ombredanne
答复: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd
Sorry to bother you, some questions about this patch will trouble you to give
some advice:
+ ufs: ufs@ff3b {
+ compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
+ /* 0: HCI standard */
+ /* 1: UFS SYS CTRL */
+ reg = <0x0 0xff3b 0x0 0x1000>,
+ <0x0 0xff3b1000 0x0 0x1000>;
+ interrupt-parent = <&gic>;
+ interrupts = ;
+ clocks = <&crg_ctrl HI3660_CLK_GATE_UFSIO_REF>,
+ <&crg_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
+ clock-names = "clk_ref", "clk_phy";
+ freq-table-hz = <0 0>, <0 0>;
+ /* offset: 0x84; bit: 12 */
+ /* offset: 0x84; bit: 7 */
+ resets = <&crg_rst 0x84 12>,
+ <&crg_rst 0x84 7>;
+ reset-names = "rst", "assert";
+ };
1. our UFS host soc implementation can be divided into two parts: UFS
controller and related peripheral circuit, that "HCI standard"<-> UFS
controller, "UFS SYS CTRL"<-> related peripheral circuit, and PHY is part of
the peripheral circuit. So the "UFS SYS CTRL" area does not correspond
completely to what Qualcomm have described as their PHY implementation. In
fact, we do not have an independent register space to control the PHY.
2. From our soc chip colleague, "rst", "assert" is not generic and related with
our soc implementation. In fact，it is not just a rst and assert of the UFS
controller, but for the entire UFS IP ,so I don't think it's very helpful for
others.
I think the above places will be reserved, do you have any better advices.
Thank you very much.
-邮件原件-
发件人: liwei (CM)
发送时间: 2017年10月31日 20:35
收件人: 'Arnd Bergmann'
抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak
Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory
CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding;
Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM;
linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z);
wangyupeng (A); zangleigang
主题: 答复: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd
Thank you for your reply.
-邮件原件-
发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann
发送时间: 2017年10月30日 23:22
收件人: liwei (CM)
抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak
Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory
CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding;
Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM;
linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z);
wangyupeng (A)
主题: Re: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
On Tue, Oct 24, 2017 at 11:06 AM, liwei (CM) wrote:
> what's your opinion about my explanation and revision method?
> I am looking forward to your reply, thanks!
Sorry for the delay, I was travelling last week.
> 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd
> Bergmann On Fri, Oct 20, 2017 at 10:52 AM, Li Wei wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Universal Flash Storage (UFS) Host Controller
>> +
>> +UFS nodes are defined to describe on-chip UFS hardware macro.
>> +Each UFS Host Controller should have its own node.
>> +
>> +Required properties:
>> +- compatible: compatible list, contains one of the following -
>> + "hisilicon,hi3660-ufs" for hisi ufs host controller
>> +present on Hi3660 chipset.
One more thing I just noticed: you don't describe the device as compatible with
the ufshcd spec as defined in the generic binding. Shouldn't we have both
compatible strings listed here?
Ok, I will fix it in patch v6;
> In particular, I wonder if what you describe as the "UFS SYS CTRL"
> area corresponds to what Qualcomm have described as their PHY implementation.
> It certainly seems to driver some of the properties that would normally be
> associated with a PHY.
>
> Liwei:Yes, a part of "UFS SYS CTRL" is associated with a PHY, but from our
> chip colleague that we assure "UFS SYS CTRL" is associated with
> clk/reset/power on/power off and so on.
> In fact, in addition to the controller itself, the controller related
> periphery are all in this area. So it's not appropriate to put this into a
>
答复: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Thank you for your reply. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2017年10月30日 23:22 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: Re: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Oct 24, 2017 at 11:06 AM, liwei (CM) wrote: > what's your opinion about my explanation and revision method? > I am looking forward to your reply, thanks! Sorry for the delay, I was travelling last week. > 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd > Bergmann On Fri, Oct 20, 2017 at 10:52 AM, Li Wei wrote: >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt >> @@ -0,0 +1,46 @@ >> +* Hisilicon Universal Flash Storage (UFS) Host Controller >> + >> +UFS nodes are defined to describe on-chip UFS hardware macro. >> +Each UFS Host Controller should have its own node. >> + >> +Required properties: >> +- compatible: compatible list, contains one of the following - >> + "hisilicon,hi3660-ufs" for hisi ufs host controller >> +present on Hi3660 chipset. One more thing I just noticed: you don't describe the device as compatible with the ufshcd spec as defined in the generic binding. Shouldn't we have both compatible strings listed here? Ok, I will fix it in patch v6; > In particular, I wonder if what you describe as the "UFS SYS CTRL" > area corresponds to what Qualcomm have described as their PHY implementation. > It certainly seems to driver some of the properties that would normally be > associated with a PHY. > > Liwei:Yes, a part of "UFS SYS CTRL" is associated with a PHY, but from our > chip colleague that we assure "UFS SYS CTRL" is associated with > clk/reset/power on/power off and so on. > In fact, in addition to the controller itself, the controller related > periphery are all in this area. So it's not appropriate to put this into a > separate phy node. I'm not sure I understand here. Do you mean the reset handle is for resetting the PHY rather than the UFS HCD? Maybe my description is not clear enough, our UFS host soc implementation can be divided into two parts: UFS controller and related peripheral circuit, that "HCI standard"<-> UFS controller, "UFS SYS CTRL"<-> related peripheral circuit, and PHY is part of the peripheral circuit. So the "UFS SYS CTRL" area does not correspond completely to what Qualcomm have described as their PHY implementation. The root reason is that our UFS host had not divided into UFS controller and PHY； > > For the "clock-names" property, you specify "clk_ref", which I assume is > > the same as what Qualcomm call "ref_clk". I'd suggest you use the existing > > name and add that as the default name in the ufshcd-pltfrm.txt binding > > document. > > Liwei:" ref_clk " is already in the ufshcd-pltfrm.txt binding > document, and parse in ufshcd.c, so we will replace "clk_ref" with > "ref_clk". I will fix it in patch v6; ok > > The "clk_phy" property appears to be related to the PHY, so it might be > > better to have a separate phy node with either just the clk, or with the > > clk plus the "UFS SYS CTRL" register area, whichever matches your hardware > > better, and then use teh "phys/phy-names" property to refer to that. > > Liwei: OK, I will add a separate phy node and fix it in patch v6; Thanks. >> The reset handling you describe here (both resets and reset-gpios) appears >> to be completely generic, so I'd suggest adding those to ufshcd-pltfrm.txt >> instead of your own binding, to ensure that future drivers use the same >> identifiers. > > Liwei: From our soc chip colleague, reset include "rst", "assert" is > not generic and related with our soc implementation, you can see > ufs_hisi_soc_init() in drivers/scsi/ufs/ufs-hisi.c, the position of > rst and assert is very special, it's hard to put it in a generic > process; It seems odd that the ability to reset a device is specific to your implementation. What I meant is that other implementations may also require a reset, so describing the way you refer to this into the generic binding wo
答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Sorry to bother you, what's your opinion about my explanation and revision method? I am looking forward to your reply, thanks! -邮件原件- 发件人: liwei (CM) 发送时间: 2017年10月21日 17:59 收件人: 'Arnd Bergmann' 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs Hi, Bergmann Sorry late for the reply,and thank you very much for your patience. My reply is as follows. I look forward to your further reply. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2017年10月20日 17:16 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: Re: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Fri, Oct 20, 2017 at 10:52 AM, Li Wei wrote: > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > @@ -0,0 +1,46 @@ > +* Hisilicon Universal Flash Storage (UFS) Host Controller > + > +UFS nodes are defined to describe on-chip UFS hardware macro. > +Each UFS Host Controller should have its own node. > + > +Required properties: > +- compatible: compatible list, contains one of the following - > + "hisilicon,hi3660-ufs" for hisi ufs host controller > +present on Hi3660 chipset. > +- reg : should contain UFS register address space & UFS SYS > CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts: interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. "clk_ref", "clk_phy" is > optional > +- resets: reset node register, one reset the clk and the other > reset the controller > +- reset-names : describe reset node register I think I've asked about this before, but I think this should be done more consistently with the other UFS bindings. In particular, I wonder if what you describe as the "UFS SYS CTRL" area corresponds to what Qualcomm have described as their PHY implementation. It certainly seems to driver some of the properties that would normally be associated with a PHY. Liwei:Yes, a part of "UFS SYS CTRL" is associated with a PHY, but from our chip colleague that we assure "UFS SYS CTRL" is associated with clk/reset/power on/power off and so on. In fact, in addition to the controller itself, the controller related periphery are all in this area. So it's not appropriate to put this into a separate phy node. For the "clock-names" property, you specify "clk_ref", which I assume is the same as what Qualcomm call "ref_clk". I'd suggest you use the existing name and add that as the default name in the ufshcd-pltfrm.txt binding document. Liwei:" ref_clk " is already in the ufshcd-pltfrm.txt binding document, and parse in ufshcd.c, so we will replace "clk_ref" with "ref_clk". I will fix it in patch v6; The "clk_phy" property appears to be related to the PHY, so it might be better to have a separate phy node with either just the clk, or with the clk plus the "UFS SYS CTRL" register area, whichever matches your hardware better, and then use teh "phys/phy-names" property to refer to that. Liwei: OK, I will add a separate phy node and fix it in patch v6; The reset handling you describe here (both resets and reset-gpios) appears to be completely generic, so I'd suggest adding those to ufshcd-pltfrm.txt instead of your own binding, to ensure that future drivers use the same identifiers. Liwei: From our soc chip colleague, reset include "rst", "assert" is not generic and related with our soc implementation, you can see ufs_hisi_soc_init() in drivers/scsi/ufs/ufs-hisi.c, the position of rst and assert is very special, it's hard to put it in a generic process; reset-gpios is used to solve a defect of the SOC chip reset function and it is not generic , but our chip has been updated, so this is no longer needed, and I will remove it in the patch v6; Thanks! Arnd
答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Bergmann Sorry late for the reply,and thank you very much for your patience. My reply is as follows. I look forward to your further reply. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2017年10月20日 17:16 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: Re: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Fri, Oct 20, 2017 at 10:52 AM, Li Wei wrote: > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt > @@ -0,0 +1,46 @@ > +* Hisilicon Universal Flash Storage (UFS) Host Controller > + > +UFS nodes are defined to describe on-chip UFS hardware macro. > +Each UFS Host Controller should have its own node. > + > +Required properties: > +- compatible: compatible list, contains one of the following - > + "hisilicon,hi3660-ufs" for hisi ufs host controller > +present on Hi3660 chipset. > +- reg : should contain UFS register address space & UFS SYS > CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts: interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. "clk_ref", "clk_phy" is > optional > +- resets: reset node register, one reset the clk and the other > reset the controller > +- reset-names : describe reset node register I think I've asked about this before, but I think this should be done more consistently with the other UFS bindings. In particular, I wonder if what you describe as the "UFS SYS CTRL" area corresponds to what Qualcomm have described as their PHY implementation. It certainly seems to driver some of the properties that would normally be associated with a PHY. Liwei:Yes, a part of "UFS SYS CTRL" is associated with a PHY, but from our chip colleague that we assure "UFS SYS CTRL" is associated with clk/reset/power on/power off and so on. In fact, in addition to the controller itself, the controller related periphery are all in this area. So it's not appropriate to put this into a separate phy node. For the "clock-names" property, you specify "clk_ref", which I assume is the same as what Qualcomm call "ref_clk". I'd suggest you use the existing name and add that as the default name in the ufshcd-pltfrm.txt binding document. Liwei:" ref_clk " is already in the ufshcd-pltfrm.txt binding document, and parse in ufshcd.c, so we will replace "clk_ref" with "ref_clk". I will fix it in patch v6; The "clk_phy" property appears to be related to the PHY, so it might be better to have a separate phy node with either just the clk, or with the clk plus the "UFS SYS CTRL" register area, whichever matches your hardware better, and then use teh "phys/phy-names" property to refer to that. Liwei: OK, I will add a separate phy node and fix it in patch v6; The reset handling you describe here (both resets and reset-gpios) appears to be completely generic, so I'd suggest adding those to ufshcd-pltfrm.txt instead of your own binding, to ensure that future drivers use the same identifiers. Liwei: From our soc chip colleague, reset include "rst", "assert" is not generic and related with our soc implementation, you can see ufs_hisi_soc_init() in drivers/scsi/ufs/ufs-hisi.c, the position of rst and assert is very special, it's hard to put it in a generic process; reset-gpios is used to solve a defect of the SOC chip reset function and it is not generic , but our chip has been updated, so this is no longer needed, and I will remove it in the patch v6; Thanks! Arnd
答复: [PATCH v3 1/5] scsi: ufs: add Hisilicon ufs driver code
Hi, Arnd
Thanks for your suggestions, I hope you'll reply again:
-邮件原件-
发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann
发送时间: 2017年9月7日 6:47
收件人: liwei (CM)
抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak
Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory
CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding;
Krzysztof Kozlowski; Eric Anholt; devicet...@vger.kernel.org; Linux Kernel
Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin
Solution Dept)
主题: Re: [PATCH v3 1/5] scsi: ufs: add Hisilicon ufs driver code
On Tue, Aug 29, 2017 at 10:41 AM, Li Wei wrote:
itel(host, UFS_ARESET, PERRSTDIS3_OFFSET);
> +
> + /* disable lp_reset_n */
> + ufs_sys_ctrl_set_bits(host, BIT_SYSCTRL_LP_RESET_N, RESET_CTRL_EN);
> + mdelay(1);
> +
> + if (gpio_is_valid(host->reset_gpio))
> + gpio_direction_output(host->reset_gpio, 1);
> +
> + ufs_sys_ctrl_writel(host, MASK_UFS_DEVICE_RESET |
> BIT_UFS_DEVICE_RESET,
> + UFS_DEVICE_RESET_CTRL);
> +
> + mdelay(20);
Could those mdelay() be turned into msleep() functions?
I will fix it in patch v4.
> +static int ufs_hisi_get_resource(struct ufs_hisi_host *host) {
> + struct resource *mem_res;
> + struct device_node *np = NULL;
> + struct device *dev = host->hba->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> +
> + /* get resource of ufs sys ctrl */
> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + host->ufs_sys_ctrl = devm_ioremap_resource(dev, mem_res);
> + if (IS_ERR(host->ufs_sys_ctrl))
> + return PTR_ERR(host->ufs_sys_ctrl);
> +
> + np = of_find_compatible_node(NULL, NULL,
> + "hisilicon,hi3660-crgctrl");
It's generally not a good idea to look up one device by its "compatible"
string. What is the "crgctrl"? Does it have a proper DT binding?
Maybe there should be a driver for it, or you could make it a "syscon"
device and look it up by phandle instead.
ok, crgctrl is our common register, if look up device by its "compatible" is
not appropriate,
I will add a properties in ufs node, like this:
ufs: ufs@ff3b {
compatible = "jedec,ufs-1.1", "hisilicon,hi3660-ufs";
/* 0: HCI standard */
/* 1: UFS SYS CTRL */
reg = <0x0 0xff3b 0x0 0x1000>,
<0x0 0xff3b1000 0x0 0x1000>;
interrupt-parent = <&gic>;
interrupts = ;
clocks = <&crg_ctrl HI3660_CLK_GATE_UFSIO_REF>,
<&crg_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
clock-names = "clk_ref", "clk_phy";
freq-table-hz = <0 0>, <0 0>;
+ /* offset: 0x84; bit: 12 */
+ /* offset: 0x84; bit: 7 */
+ resets = <&crg_rst 0x84 12>,
+ <&crg_rst 0x84 7>;
+ reset-names = "rst", "assert";
}
And find that by, is it OK?
+ host->rst = devm_reset_control_get(dev, "rst");
+ host->assert = devm_reset_control_get(dev, "assert");
> diff --git a/drivers/scsi/ufs/ufs-hisi.h b/drivers/scsi/ufs/ufs-hisi.h
> new file mode 100644 index ..52430a2aca90
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufs-hisi.h
If the header is only used in one file, you don't need it, just move
the definitions
into the other file.
Currently only one file use ufs-hisi.h, but I think so many definitions are
defined in a .h file is more clearer, like ufs-qcom.h.
If you think it isn't necessary, I will move it into ufs-hisi.c?
Arnd
答复: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
Hi, Ulf
Thank you very much for your advice.
1. Version history is really great information, however it doesn't belong
inside the change log itsefl. Instead add "---" and a newline here, then put it
all below that.
【LiWei】OK，I will fix it;
2. We have an API, mmc_regulator_set_vqmmc(), that you probably should use here
instead.
【LiWei】Yes, you are right.I use mmc_regulator_set_vqmmc() instead and test OK,
I will modify it in patch v9;
3. Overall the code looks okay to me, however I am wondering about the
relationship with the original k3 driver code and hi6220 code. Almost no code
is being re-used between the different variants. Why is that?
【LiWei】So far, it seems that dw_mci_hi3660_set_ios() and
dw_mci_hi3660_switch_voltage() are not re-used, I think the main reason is that
Hi3660 support uhs-sdr12/-uhs-sdr25/uhs-sdr50/uhs-sdr104, On the other hand,
we have custom register settings, such as:
#define AO_SCTRL_SEL18 BIT(10)
#define AO_SCTRL_CTRL3 0x40C
#define SOC_SCTRL_SCPERCTRL5(0x314)
#define SDCARD_IO_SEL18 BIT(2)
So we can't merge relevant code.
Please help review patch v9, thank you!
-邮件原件-
发件人: Ulf Hansson [mailto:ulf.hans...@linaro.org]
发送时间: 2017年8月8日 19:04
收件人: liwei (CM)
抄送: Adrian Hunter; Jaehoon Chung; Shawn Lin; Wolfram Sang; Heiner Kallweit;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Guodong Xu
主题: Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660
On 2 August 2017 at 05:17, Li Wei wrote:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei
> Signed-off-by: Chen Jun
>
> Major changes in v3:
> - solve review comments from Heiner Kallweit.
>*use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>*use usleep_range() replace udelay() and mdelay().
>
> Major changes in v4:
> - solve review comments from Jaehoon Chung.
>*move common register for dwmmc controller to dwmmc header file.
>*modify definitions type of some register variables.
>*get rid of the magic numbers.
>
> Major changes in v5:
> - further improve coding style.
>
> Major changes in v6:
> - solve review comments for Jaehoon Chung.
>*modify dw_mci_hi3660_set_ios() to static.
>*fix the comment style.
>
> Major changes in v7:
> - solve review comments for John Stultz.
>*remove reset code in dw_mmc-k3.c,use reset in core mmc.
>
> Major changes in v8:
> - modify patch v7 name and dependency order.
Version history is really great information, however it doesn't belong inside
the change log itsefl. Instead add "---" and a newline here, then put it all
below that.
[...]
> +static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
> + struct mmc_ios *ios) {
> + int ret;
> + int min_uv = 0;
> + int max_uv = 0;
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> + struct k3_priv *priv;
> + struct dw_mci *host;
> +
> + host = slot->host;
> + priv = host->priv;
> +
> + if (!priv || !priv->reg)
> + return 0;
> +
> + if (priv->ctrl_id == DWMMC_SDIO_ID)
> + return 0;
> +
> + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> + ret = dw_mci_set_sel18(host, 0);
> + if (ret)
> + return ret;
> + min_uv = 295;
> + max_uv = 295;
> + } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> + ret = dw_mci_set_sel18(host, 1);
> + if (ret)
> + return ret;
> + min_uv = 180;
> + max_uv = 180;
> + }
> +
> + if (IS_ERR_OR_NULL(mmc->supply.vqmmc))
> + return 0;
> +
> + ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv,
> + max_uv);
We have an API, mmc_regulator_set_vqmmc(), that you probably should use here
instead.
> + if (ret) {
> + dev_err(host->dev, "Regulator set error %d: %d - %d\n",
> + ret, min_uv, max_uv);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static const struct dw_mci_drv_data hi3660_data = {
> + .init = dw_mci_hi3660_init,
> + .set_ios = dw_mci_hi3660_set_ios,
> + .parse_dt = dw_mci_hi6220_parse_dt,
> + .execute_tuning = dw_mci_hi3660_execute_tuning,
> + .switch_voltage = dw_mci_hi3660_switch_voltage, };
> +
> static const struct of_device_id dw_mci_k3_match[] = {
> + { .compatible = "hisilicon,hi3660-dw-mshc", .data =
> + &hi3660_data, },
> { .compatible = "hisilicon,hi4511-
答复: [PATCH v5] mmc: dw_mmc-k3: add sd support for hi3660
Hi,all
Sorry to bother you, could you spare some time to help review patch v6 that I
sended. Would you please give us your opinion?
Thnak you very much.
-邮件原件-
发件人: liwei (CM)
发送时间: 2017年7月6日 15:03
收件人: 'Guodong Xu'; Jaehoon Chung
抄送: Ulf Hansson; adrian.hun...@intel.com; Shawn Lin;
wsa+rene...@sang-engineering.com; hkallwe...@gmail.com;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
主题: 答复: [PATCH v5] mmc: dw_mmc-k3: add sd support for hi3660
Hi, Jaehoon
Thank you for your advice during your busy schedule, 1."Need the patch for
adding "hi3660-dw-mshc" compatible
Documentation/devicetree/bindings/k3-dw-mshc.txt"
This "hi3660-dw-mshc" compatible string has been integrated into platform
maintainer's tree together with dts changes. It's now in linux-next [1]；need
this one more?
2. And added the minor comments.
Iwill fix them in patch v6.
Thank you very much.
-邮件原件-
发件人: Guodong Xu [mailto:guodong...@linaro.org]
发送时间: 2017年7月6日 14:45
收件人: Jaehoon Chung; liwei (CM)
抄送: Ulf Hansson; adrian.hun...@intel.com; Shawn Lin;
wsa+rene...@sang-engineering.com; hkallwe...@gmail.com;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH v5] mmc: dw_mmc-k3: add sd support for hi3660
On Thu, Jul 6, 2017 at 1:13 PM, Jaehoon Chung wrote:
> Hi,
>
> To Guodong, if you can forward this to Li Wei, plz do it. Sorry.
>
Sure. Put Li Wei in cc.
And add my response.
> On 07/03/2017 10:06 AM, liwei wrote:
>>
>> Add sd card support for hi3660 soc
>
> Need the patch for adding "hi3660-dw-mshc" compatible
> Documentation/devicetree/bindings/k3-dw-mshc.txt
>
> And added the minor comments.
>
Hi, Jaehoo,
This "hi3660-dw-mshc" compatible string has been integrated into platform
maintainer's tree together with dts changes. It's now in linux-next [1], and in
Linus' master branch [2], will be in mainline v4.13-rc's.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt?h=next-20170706
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
-Guodong
>>
>> Major changes in v3:
>> - solve review comments from Heiner Kallweit.
>>*use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>>*use usleep_range() replace udelay() and mdelay().
>>
>> Major changes in v4:
>> - solve review comments from Jaehoon Chung.
>>*move common register for dwmmc controller to dwmmc header file.
>>*modify definitions type of some register variables.
>>*get rid of the magic numbers.
>>
>> Major changes in v5:
>> - further improve coding style.
>> ---
>> drivers/mmc/host/dw_mmc-k3.c | 315
>> +++
>> drivers/mmc/host/dw_mmc.h| 2 +
>> 2 files changed, 317 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-k3.c
>> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..a28eb8c7da82
>> 100644
>> --- a/drivers/mmc/host/dw_mmc-k3.c
>> +++ b/drivers/mmc/host/dw_mmc-k3.c
>> @@ -8,6 +8,8 @@
>> * (at your option) any later version.
>> */
>>
>> +#include
>> +#include
>> #include
>> #include
>> #include
>> @@ -28,7 +30,38 @@
>> #define AO_SCTRL_SEL18 BIT(10)
>> #define AO_SCTRL_CTRL3 0x40C
>>
>> +#define DWMMC_SDIO_ID 2
>> +
>> +#define SOC_SCTRL_SCPERCTRL5(0x314)
>> +#define SDCARD_IO_SEL18 BIT(2)
>> +
>> +#define SDCARD_RD_THRESHOLD (512)
>> +
>> +#define GENCLK_DIV (7)
>> +
>> +#define GPIO_CLK_ENABLE BIT(16)
>> +#define GPIO_CLK_DIV_MASK GENMASK(11, 8)
>> +#define GPIO_USE_SAMPLE_DLY_MASK GENMASK(13, 13)
>> +#define UHS_REG_EXT_SAMPLE_PHASE_MASK GENMASK(20, 16)
>> +#define UHS_REG_EXT_SAMPLE_DRVPHASE_MASK GENMASK(25, 21)
>> +#define UHS_REG_EXT_SAMPLE_DLY_MASK GENMASK(30, 26)
>> +
>> +#define TIMING_MODE 3
>> +#define TIMING_CFG_NUM 10
>> +
>> +#define PULL_DOWN BIT(1)
>> +#define PULL_UP BIT(0)
>> +
>> +#define NUM_PHASES (40)
>> +
>> +#define ENABLE_SHIFT_MIN_SMPL (4)
>> +#define ENABLE_SHIFT_MAX_SMPL (12)
>> +#define USE_DLY_MIN_SMPL (11)
>> +#define USE_DLY_MAX_SMPL (14)
>> +
>> struct k3_priv {
>> + int ctrl_id;
>> + u32 cur_speed;
>> struct regmap *reg;
>> };
>>
>> @@ -38,6 +71,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
>> 0
>> };
>&g
答复: [PATCH v5] mmc: dw_mmc-k3: add sd support for hi3660
Hi, Jaehoon
Thank you for your advice during your busy schedule,
1."Need the patch for adding "hi3660-dw-mshc" compatible
Documentation/devicetree/bindings/k3-dw-mshc.txt"
This "hi3660-dw-mshc" compatible string has been integrated into platform
maintainer's tree together with dts changes. It's now in linux-next [1]；need
this one more?
2. And added the minor comments.
Iwill fix them in patch v6.
Thank you very much.
-邮件原件-
发件人: Guodong Xu [mailto:guodong...@linaro.org]
发送时间: 2017年7月6日 14:45
收件人: Jaehoon Chung; liwei (CM)
抄送: Ulf Hansson; adrian.hun...@intel.com; Shawn Lin;
wsa+rene...@sang-engineering.com; hkallwe...@gmail.com;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH v5] mmc: dw_mmc-k3: add sd support for hi3660
On Thu, Jul 6, 2017 at 1:13 PM, Jaehoon Chung wrote:
> Hi,
>
> To Guodong, if you can forward this to Li Wei, plz do it. Sorry.
>
Sure. Put Li Wei in cc.
And add my response.
> On 07/03/2017 10:06 AM, liwei wrote:
>>
>> Add sd card support for hi3660 soc
>
> Need the patch for adding "hi3660-dw-mshc" compatible
> Documentation/devicetree/bindings/k3-dw-mshc.txt
>
> And added the minor comments.
>
Hi, Jaehoo,
This "hi3660-dw-mshc" compatible string has been integrated into platform
maintainer's tree together with dts changes. It's now in linux-next [1], and in
Linus' master branch [2], will be in mainline v4.13-rc's.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt?h=next-20170706
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
-Guodong
>>
>> Major changes in v3:
>> - solve review comments from Heiner Kallweit.
>>*use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>>*use usleep_range() replace udelay() and mdelay().
>>
>> Major changes in v4:
>> - solve review comments from Jaehoon Chung.
>>*move common register for dwmmc controller to dwmmc header file.
>>*modify definitions type of some register variables.
>>*get rid of the magic numbers.
>>
>> Major changes in v5:
>> - further improve coding style.
>> ---
>> drivers/mmc/host/dw_mmc-k3.c | 315
>> +++
>> drivers/mmc/host/dw_mmc.h| 2 +
>> 2 files changed, 317 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-k3.c
>> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..a28eb8c7da82
>> 100644
>> --- a/drivers/mmc/host/dw_mmc-k3.c
>> +++ b/drivers/mmc/host/dw_mmc-k3.c
>> @@ -8,6 +8,8 @@
>> * (at your option) any later version.
>> */
>>
>> +#include
>> +#include
>> #include
>> #include
>> #include
>> @@ -28,7 +30,38 @@
>> #define AO_SCTRL_SEL18 BIT(10)
>> #define AO_SCTRL_CTRL3 0x40C
>>
>> +#define DWMMC_SDIO_ID 2
>> +
>> +#define SOC_SCTRL_SCPERCTRL5(0x314)
>> +#define SDCARD_IO_SEL18 BIT(2)
>> +
>> +#define SDCARD_RD_THRESHOLD (512)
>> +
>> +#define GENCLK_DIV (7)
>> +
>> +#define GPIO_CLK_ENABLE BIT(16)
>> +#define GPIO_CLK_DIV_MASK GENMASK(11, 8)
>> +#define GPIO_USE_SAMPLE_DLY_MASK GENMASK(13, 13)
>> +#define UHS_REG_EXT_SAMPLE_PHASE_MASK GENMASK(20, 16)
>> +#define UHS_REG_EXT_SAMPLE_DRVPHASE_MASK GENMASK(25, 21)
>> +#define UHS_REG_EXT_SAMPLE_DLY_MASK GENMASK(30, 26)
>> +
>> +#define TIMING_MODE 3
>> +#define TIMING_CFG_NUM 10
>> +
>> +#define PULL_DOWN BIT(1)
>> +#define PULL_UP BIT(0)
>> +
>> +#define NUM_PHASES (40)
>> +
>> +#define ENABLE_SHIFT_MIN_SMPL (4)
>> +#define ENABLE_SHIFT_MAX_SMPL (12)
>> +#define USE_DLY_MIN_SMPL (11)
>> +#define USE_DLY_MAX_SMPL (14)
>> +
>> struct k3_priv {
>> + int ctrl_id;
>> + u32 cur_speed;
>> struct regmap *reg;
>> };
>>
>> @@ -38,6 +71,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
>> 0
>> };
>>
>> +struct hs_timing {
>> + u32 drv_phase;
>> + u32 smpl_dly;
>> + u32 smpl_phase_max;
>> + u32 smpl_phase_min;
>> +};
>> +
>> +struct hs_timing hs_timing_cfg[TIMING_MODE][TIMING_CFG_NUM] = {
>> + { /* reserved */ },
>> + { /* SD */
>> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
>> + {6, 0, 4, 4,}, /* 1: MMC_HS */
>> + {6, 0,
答复: [PATCH v4] mmc: dw_mmc-k3: add sd support for hi3660
Hi,all
Sorry to bother you, but could you spare some time to help review patch v5 that
I will send later. In this patch，the coding style is further improved and solve
guodong's review.
Thank you very much.
-邮件原件-
发件人: Guodong Xu [mailto:guodong...@linaro.org]
发送时间: 2017年6月30日 21:28
收件人: liwei (CM)
抄送: Ulf Hansson; adrian.hun...@intel.com; Jaehoon Chung; Shawn Lin;
wsa+rene...@sang-engineering.com; hkallwe...@gmail.com;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH v4] mmc: dw_mmc-k3: add sd support for hi3660
Hi, Li Wei
I have some minor comments to your patch.
> On Tue, Jun 13, 2017 at 3:25 PM, liwei wrote:
First of all, please modify author part. It is recommended to use "Li Wei",
instead of what you are using now "liwei". If you are using git format-patch,
send-email, please modify your ~/.gitconfig
Add a proper [user] section.
For an existing commit, you can modify the author information by running git
commit --amend --author="xxx"
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei
> Signed-off-by: Chen Jun
>
> Major changes in v3:
> - solve review comments from Heiner Kallweit.
>*use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>*use usleep_range() replace udelay() and mdelay().
>
> Major changes in v4:
> - solve review comments from Jaehoon Chung.
>*move common register for dwmmc controller to dwmmc header file.
>*modify definitions type of some register variables.
>*get rid of the magic numbers.
> ---
> drivers/mmc/host/dw_mmc-k3.c | 313
> +++
> drivers/mmc/host/dw_mmc.h| 2 +
> 2 files changed, 315 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-k3.c
> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..c63e91ab775e 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -8,6 +8,8 @@
> * (at your option) any later version.
> */
>
> +#include
> +#include
> #include
> #include
> #include
> @@ -28,7 +30,38 @@
> #define AO_SCTRL_SEL18 BIT(10)
> #define AO_SCTRL_CTRL3 0x40C
>
> +#define DWMMC_SDIO_ID 2
> +
> +#define SOC_SCTRL_SCPERCTRL5(0x314)
> +#define SDCARD_IO_SEL18 BIT(2)
> +
> +#define SDCARD_RD_THRESHOLD (512)
> +
> +#define GENCLK_DIV (7)
> +
> +#define GPIO_CLK_ENABLE BIT(16)
> +#define GPIO_CLK_DIV_MASK GENMASK(11, 8)
> +#define GPIO_USE_SAMPLE_DLY_MASK GENMASK(13, 13)
> +#define UHS_REG_EXT_SAMPLE_PHASE_MASK GENMASK(20, 16)
> +#define UHS_REG_EXT_SAMPLE_DRVPHASE_MASK GENMASK(25, 21)
> +#define UHS_REG_EXT_SAMPLE_DLY_MASK GENMASK(30, 26)
> +
> +#define TIMING_MODE 3
> +#define TIMING_CFG_NUM 10
> +
> +#define PULL_DOWN BIT(1)
> +#define PULL_UP BIT(0)
> +
> +#define NUM_PHASES (40)
> +
> +#define ENABLE_SHIFT_MIN_SMPL (4)
> +#define ENABLE_SHIFT_MAX_SMPL (12)
> +#define USE_DLY_MIN_SMPL (11)
> +#define USE_DLY_MAX_SMPL (14)
> +
> struct k3_priv {
> + int ctrl_id;
> + u32 cur_speed;
> struct regmap *reg;
> };
>
> @@ -38,6 +71,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
> 0
> };
>
> +struct hs_timing {
> + u32 drv_phase;
> + u32 smpl_dly;
> + u32 smpl_phase_max;
> + u32 smpl_phase_min;
> +};
> +
> +struct hs_timing hs_timing_cfg[TIMING_MODE][TIMING_CFG_NUM] = {
> + { /* reserved */ },
> + { /* SD */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {6, 0, 4, 4,}, /* 1: MMC_HS */
> + {6, 0, 3, 3,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 2, 2,}, /* 4: SDR25 */
> + {4, 0, 11, 0,}, /* 5: SDR50 */
> + {6, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + },
> + { /* SDIO */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {0}, /* 1: MMC_HS */
> + {6, 0, 15, 15,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 0, 0,}, /* 4: SDR25 */
> + {4, 0, 12, 0,}, /* 5: SDR50 */
> + {5, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + }
> +};
> +
> static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios
> *ios)
答复: [PATCH v3] mmc: dw_mmc-k3: add sd support for hi3660
Hi, Jaehoon
Thank you very much for your advices; I'll follow your advices and fix all the
other places but except for the following one:
"You have assumption...always.
mshc0 -> eMMC,
mshc1 -> SD
mshc2 -> SDIO"
yes, mshc0 -> eMMC, mshc1 -> SD,mshc2 -> SDIO. Can you point it out in detail?
Thanks!
-邮件原件-
发件人: Jaehoon Chung [mailto:jh80.ch...@gmail.com]
发送时间: 2017年6月12日 22:28
收件人: liwei (CM); ulf.hans...@linaro.org; adrian.hun...@intel.com;
jh80.ch...@samsung.com; shawn@rock-chips.com;
wsa+rene...@sang-engineering.com; hkallwe...@gmail.com;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
抄送: guodong...@linaro.org
主题: Re: [PATCH v3] mmc: dw_mmc-k3: add sd support for hi3660
Hi Li,
On 2017년 06월 12일 16:46, liwei wrote:
> Add sd card support for hi3660 soc
>
> Major changes in v3:
> - solve review comments from Heiner Kallweit.
>*use the GENMASK and FIELD_PREP macros replace the bit shift operation.
>*use usleep_range() replace udelay() and mdelay().
I had added the some comments about your previous patch.
Refer to below...
https://patchwork.kernel.org/patch/9747495/
>
> Signed-off-by: Li Wei
> Signed-off-by: Chen Jun
> ---
Locate changelog at here.
Best Regards,
Jaehoon Chung
> drivers/mmc/host/dw_mmc-k3.c | 314
> +++
> 1 file changed, 314 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-k3.c
> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..a6e13bd83b9f 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -8,6 +8,8 @@
> * (at your option) any later version.
> */
>
> +#include
> +#include
> #include
> #include
> #include
> @@ -28,7 +30,40 @@
> #define AO_SCTRL_SEL18 BIT(10)
> #define AO_SCTRL_CTRL3 0x40C
>
> +#define DWMMC_SD_ID 1
> +#define DWMMC_SDIO_ID 2
> +
> +#define SOC_SCTRL_SCPERCTRL5(0x314)
> +#define SDCARD_IO_SEL18 BIT(2)
> +
> +#define GENCLK_DIV (7)
> +
> +#define GPIO_CLK_ENABLE BIT(16)
> +#define GPIO_CLK_DIV_MASK GENMASK(11, 8)
> +#define GPIO_USE_SAMPLE_DLY_MASK GENMASK(13, 13)
> +#define UHS_REG_EXT_SAMPLE_PHASE_MASK GENMASK(20, 16)
> +#define UHS_REG_EXT_SAMPLE_DRVPHASE_MASK GENMASK(25, 21)
> +#define UHS_REG_EXT_SAMPLE_DLY_MASK GENMASK(30, 26)
> +
> +#define SDMMC_UHS_REG_EXT0x108
> +#define SDMMC_ENABLE_SHIFT 0x110
> +
> +#define TIMING_MODE 3
> +#define TIMING_CFG_NUM 10
> +
> +#define PULL_DOWN BIT(1)
> +#define PULL_UP BIT(0)
> +
> +#define NUM_PHASES (40)
> +
> +#define ENABLE_SHIFT_MIN_SMPL (4)
> +#define ENABLE_SHIFT_MAX_SMPL (12)
> +#define USE_DLY_MIN_SMPL (11)
> +#define USE_DLY_MAX_SMPL (14)
> +
> struct k3_priv {
> + u8 ctrl_id;
> + u32 cur_speed;
> struct regmap *reg;
> };
>
> @@ -38,6 +73,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
> 0
> };
>
> +struct hs_timing {
> + int drv_phase;
> + int sam_dly;
> + int sam_phase_max;
> + int sam_phase_min;
> +};
> +
> +struct hs_timing hs_timing_cfg[TIMING_MODE][TIMING_CFG_NUM] = {
> + { /* reserved */ },
> + { /* SD */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {6, 0, 4, 4,}, /* 1: MMC_HS */
> + {6, 0, 3, 3,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 2, 2,}, /* 4: SDR25 */
> + {4, 0, 11, 0,}, /* 5: SDR50 */
> + {6, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + },
> + { /* SDIO */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {0}, /* 1: MMC_HS */
> + {6, 0, 15, 15,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 0, 0,}, /* 4: SDR25 */
> + {4, 0, 12, 0,}, /* 5: SDR50 */
> + {5, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + }
> +};
> +
> static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios
> *ios) {
> int ret;
> @@ -66,6 +136,10 @@ static int dw_mci_hi6220_parse_dt(struct dw_mci *host)
> if (IS_ERR(priv->reg))
> priv->reg = NULL;
>
> + priv->ctrl_id = of_alias_get_id(host->dev->of_node, "mshc");
> + if (priv->ctrl_id < 0)
> + pri
答复: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660
Hi, Heiner
Sorry late for reply you, thank you very much for your advice.
My changes are as follows and I will send the PATCH-V3 later.
-邮件原件-
发件人: Heiner Kallweit [mailto:hkallwe...@gmail.com]
发送时间: 2017年5月17日 5:52
收件人: liwei (CM); ulf.hans...@linaro.org; adrian.hun...@intel.com;
jh80.ch...@samsung.com; shawn@rock-chips.com;
wsa+rene...@sang-engineering.com; linux-...@vger.kernel.org;
linux-kernel@vger.kernel.org
抄送: guodong...@linaro.org
主题: Re: [PATCH] mmc:dw_mmc-k3: add sd support for hi3660
Am 16.05.2017 um 14:26 schrieb liwei:
> Add sd card support for hi3660 soc
>
> Signed-off-by: Li Wei
> Signed-off-by: Chen Jun
> ---
> drivers/mmc/host/dw_mmc-k3.c | 311
> +++
> 1 file changed, 311 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-k3.c
> b/drivers/mmc/host/dw_mmc-k3.c index e38fb0020bb1..59d6e0870fb1 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -8,6 +8,7 @@
> * (at your option) any later version.
> */
>
> +#include
> #include
> #include
> #include
> @@ -28,7 +29,44 @@
> #define AO_SCTRL_SEL18 BIT(10)
> #define AO_SCTRL_CTRL3 0x40C
>
> +#define DWMMC_SD_ID 1
> +#define DWMMC_SDIO_ID 2
> +
> +#define SOC_SCTRL_SCPERCTRL5(0x314)
> +#define SDCARD_IO_SEL18 BIT(2)
> +
> +#define GENCLK_DIV (7)
> +
> +#define GPIO_CLK_ENABLE BIT(16)
> +#define GPIO_CLK_DIV(x) (((x) & 0xf) << 8)
> +#define GPIO_USE_SAMPLE_DLY(x) (((x) & 0x1) << 13)
> +#define UHS_REG_EXT_SAMPLE_PHASE(x) (((x) & 0x1f) << 16)
> +#define UHS_REG_EXT_SAMPLE_DLY(x) (((x) & 0x1f) << 26)
> +#define UHS_REG_EXT_SAMPLE_DRVPHASE(x) (((x) & 0x1f) << 21)
Using the GENMASK and FIELD_PREP macros may be a good alternative here,
e.g.:
#define GPIO_CLK_DIV_MASK GENMASK(11, 8) Then in the code use
FIELD_PREP(GPIO_CLK_DIV_MASK, x)
And the bit field definitions should follow the register defines, e.g.:
#define REG_1 0x00
#define REG_1_FIELD_1 GENMASK(a, b)
#define REG_1_FIELD_2 GENMASK(c, d)
This makes it easier for people checking the code against the chip spec.
【liwei】I'll fix this issue and update the patch;
> +#define SDMMC_UHS_REG_EXT_VALUE(x, y, z) (UHS_REG_EXT_SAMPLE_PHASE(x) |\
> + UHS_REG_EXT_SAMPLE_DLY(y) |\
> + UHS_REG_EXT_SAMPLE_DRVPHASE(z))
> #define SDMMC_GPIO_VALUE(x, y)
> +(GPIO_CLK_DIV(x) | GPIO_USE_SAMPLE_DLY(y))
Both macros are used only once. So are they actually needed?
【liwei】Yes, they are not needed, I'll update the patch;
> +
> +#define SDMMC_UHS_REG_EXT0x108
> +#define SDMMC_ENABLE_SHIFT 0x110
> +
> +#define TIMING_MODE 3
> +#define TIMING_CFG_NUM 10
> +
> +#define PULL_DOWN BIT(1)
> +#define PULL_UP BIT(0)
> +
> +#define NUM_PHASES (40)
> +
> +#define ENABLE_SHIFT_MIN_SMPL (4)
> +#define ENABLE_SHIFT_MAX_SMPL (12)
> +#define USE_DLY_MIN_SMPL (11)
> +#define USE_DLY_MAX_SMPL (14)
> +
> struct k3_priv {
> + u8 ctrl_id;
> + u32 cur_speed;
> struct regmap *reg;
> };
>
> @@ -38,6 +76,41 @@ static unsigned long dw_mci_hi6220_caps[] = {
> 0
> };
>
> +struct hs_timing {
> + int drv_phase;
> + int sam_dly;
> + int sam_phase_max;
> + int sam_phase_min;
> +};
> +
> +struct hs_timing hs_timing_cfg[TIMING_MODE][TIMING_CFG_NUM] = {
> + { /* reserved */ },
> + { /* SD */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {6, 0, 4, 4,}, /* 1: MMC_HS */
> + {6, 0, 3, 3,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 2, 2,}, /* 4: SDR25 */
> + {4, 0, 11, 0,}, /* 5: SDR50 */
> + {6, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + },
> + { /* SDIO */
> + {7, 0, 15, 15,}, /* 0: LEGACY 400k */
> + {0}, /* 1: MMC_HS */
> + {6, 0, 15, 15,}, /* 2: SD_HS */
> + {6, 0, 15, 15,}, /* 3: SDR12 */
> + {6, 0, 0, 0,}, /* 4: SDR25 */
> + {4, 0, 12, 0,}, /* 5: SDR50 */
> + {5, 4, 15, 0,}, /* 6: SDR104 */
> + {0}, /* 7: DDR50 */
> + {0}, /* 8: DDR52 */
> + {0}, /* 9: HS200 */
> + }
> +};
> +
> static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios
> *ios) {
>
