Re: [PATCH 1/3 v8] ARC: Set IO-coherency aperture base to LINUX_LINK_BASE

[Eugeniy Paltsev <Eugeniy.Paltsev-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>]

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 <Eugeniy.Paltsev@synopsys.com>
> > ---
> >   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