From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934026AbdHYP5t (ORCPT ); Fri, 25 Aug 2017 11:57:49 -0400 Received: from smtprelay.synopsys.com ([198.182.60.111]:33325 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933738AbdHYP5r (ORCPT ); Fri, 25 Aug 2017 11:57:47 -0400 From: Eugeniy Paltsev To: Vineet Gupta , "linux-snps-arc@lists.infradead.org" CC: "linux-kernel@vger.kernel.org" , "Vineet.Gupta1@synopsys.com" , "Alexey.Brodkin@synopsys.com" , "robh+dt@kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH 1/3 v8] ARC: Set IO-coherency aperture base to LINUX_LINK_BASE Thread-Topic: [PATCH 1/3 v8] ARC: Set IO-coherency aperture base to LINUX_LINK_BASE Thread-Index: AQHS+vLoiujB7L+NHEaVm/F0deVnNqKRCDIAgARWJQA= Date: Fri, 25 Aug 2017 15:57:43 +0000 Message-ID: <1503676662.15555.8.camel@synopsys.com> References: <20170712094023.23226-1-Eugeniy.Paltsev@synopsys.com> <20170712094023.23226-2-Eugeniy.Paltsev@synopsys.com> <5184d3eb-31bc-c456-3e65-6137a3ba72f7@synopsys.com> In-Reply-To: <5184d3eb-31bc-c456-3e65-6137a3ba72f7@synopsys.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.121.8.106] Content-Type: text/plain; charset="utf-8" Content-ID: <6521BCF736599944ABCA91AEAE720997@internal.synopsys.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v7PFvrMH007221 On Tue, 2017-08-22 at 14:44 -0700, Vineet Gupta wrote: > On 07/12/2017 02:40 AM, Eugeniy Paltsev wrote: > > Most of the time we indeed use the one and only LINUX_LINK_BASE > > set to 0x8000_0000. But there might be good reasons to move > > the kernel to another location like 0x9z etc. > > And we want IOC aperture to cover entire area used by the kernel, > > so let's make its base matching link base > > How about something below.... > > Currently IOC aperture base is hardcoded to 0x8000_0000 which may not > be true for  > non default values of CONFIG_LINUX_LINK_BASE, so use the config value > > > and add required asserts: > > checking IOC aperture base address and size to be supported by IOC. > > .... And while at it, also add the required asserts expected by the > IOC  > programming model. > > > > > Signed-off-by: Eugeniy Paltsev > > --- > >   arch/arc/mm/cache.c | 33 ++++++++++++++++++++++++--------- > >   1 file changed, 24 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c > > index a867575..383ff77 100644 > > --- a/arch/arc/mm/cache.c > > +++ b/arch/arc/mm/cache.c > > @@ -1083,7 +1083,8 @@ SYSCALL_DEFINE3(cacheflush, uint32_t, start, > > uint32_t, sz, uint32_t, flags) > >    */ > >   noinline void __init arc_ioc_setup(void) > >   { > > - unsigned int ap_sz; > > + unsigned int ap_base; > > + long ap_size; > > Is there a reason they are different types ? > Also the way you use it below, best to call it mem_size ! Ok, I used long for ap_size as it simply populated by arc_get_mem_sz ------------->8---------- ap_size = arc_get_mem_sz(); ------------->8---------- and arc_get_mem_sz return long. Probably I should fix arc_get_mem_sz because it simply return low_mem_sz which is unsigned long! ------------->8---------- static unsigned long low_mem_sz; long __init arc_get_mem_sz(void) { return low_mem_sz; } ------------->8---------- > >    > >    /* Flush + invalidate + disable L1 dcache */ > >    __dc_disable(); > > @@ -1092,18 +1093,32 @@ noinline void __init arc_ioc_setup(void) > >    if (read_aux_reg(ARC_REG_SLC_BCR)) > >    slc_entire_op(OP_FLUSH_N_INV); > >    > > - /* IOC Aperture start: TDB: handle non default > > CONFIG_LINUX_LINK_BASE */ > > - write_aux_reg(ARC_REG_IO_COH_AP0_BASE, 0x80000); > > - > >    /* > > -  * IOC Aperture size: > > -  *   decoded as 2 ^ (SIZE + 2) KB: so setting 0x11 implies > > 512M > > -  * TBD: fix for PGU + 1GB of low mem > > +  * IOC Aperture size is equal to memory size. > > +  * TBD: fix for PGU + 1GiB of low mem > > Not really averse to this per-se, but conventionally we've not used > KiB or GiB  > etc, so I'd prefer KB, GB... just for consistency and ability to grep > correctly. But we also use KiB, MiB, GiB for ARC: $ grep -r -I -e "MiB" -e "GiB" arch/arc/ reg = <0x0 0x80000000 0x0 0x20000000 /* 512 MiB low mem */ 0x1 0xc0000000 0x0 0x40000000>; /* 1 GiB highmem */ reg = <0x0 0x80000000 0x0 0x20000000 /* 512 MiB low mem */ 0x1 0xc0000000 0x0 0x40000000>; /* 1 GiB highmem */ reg = <0x0 0x80000000 0x0 0x1b000000>; /* (512 - 32) MiB */ reg = <0x80000000 0x20000000>; /* 512MiB */ reg = <0x80000000 0x20000000>; /* 512MiB */ .......... > >     * TBD: fix for PAE > >     */ > > - ap_sz = order_base_2(arc_get_mem_sz()/1024) - 2; > > - write_aux_reg(ARC_REG_IO_COH_AP0_SIZE, ap_sz); > > + ap_size = arc_get_mem_sz(); > > + > > + if (!is_power_of_2(ap_size) || ap_size < 4096) > > + panic("IOC Aperture size must be power of 2 larger > > than 4KiB"); > > + > > + /* > > +  * IOC Aperture size decoded as 2 ^ (SIZE + 2) KiB, > > +  * so setting 0x11 implies 512MiB, 0x12 implies 1G... > > +  */ > > + write_aux_reg(ARC_REG_IO_COH_AP0_SIZE, > > order_base_2(ap_size / 1024) - 2); > > for (ap_size / 1024) can you use (ap_size >> 10) I absolutely sure what compiler will implement this division  ap_size/1024 as right shift by itself. So lets don't make things look more complicated than they are :) > > + > > + /* > > +  * For now we assume IOC aperture to cover all the memory > > used by the > > +  * kernel. > > +  */ > > + ap_base = CONFIG_LINUX_LINK_BASE; > > + > > + if (ap_base % ap_size != 0) > > + panic("IOC Aperture start must be aligned to the > > size of the aperture"); > > This is good. > > >    > > + write_aux_reg(ARC_REG_IO_COH_AP0_BASE, ap_base >> 12); > >    write_aux_reg(ARC_REG_IO_COH_PARTIAL, 1); > >    write_aux_reg(ARC_REG_IO_COH_ENABLE, 1); > >    > > > > --  Eugeniy Paltsev