From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 3 Feb 2014 13:21:46 -0600 From: kodiak furr Message-ID: In-Reply-To: <20140203191837.GC4889@e106497-lin.cambridge.arm.com> References: <20140203191837.GC4889@e106497-lin.cambridge.arm.com> Subject: Re: [PATCH] arm64: Add architecture support for PCI MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============7143198835380267376==" Errors-To: linaro-kernel-bounces@lists.linaro.org Sender: linaro-kernel-bounces@lists.linaro.org To: Liviu Dudau Cc: "devicetree@vger.kernel.org" , linaro-kernel , Catalin Marinas , LKML , linux-pci , Bjorn Helgaas , LAKML List-ID: --===============7143198835380267376== Content-Type: multipart/alternative; boundary="52efec4a_327b23c6_a1" --52efec4a_327b23c6_a1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable kodiak furr liked your message with Boxer. On =46ebruary 3, 2014 at 1:18:= 38 PM CST, Liviu Dudau wrote:On Mon, =46eb 03, 2014 at 06:58:56PM +0000,= Arnd Bergmann wrote:> On Monday 03 =46ebruary 2014 18:43:48 Liviu Dudau = wrote:> > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/a= sm/io.h> > index 4cc813e..ce5bad2 100644> > --- a/arch/arm64/include/asm/= io.h> > +++ b/arch/arm64/include/asm/io.h> > =40=40 -120,9 +120,13 =40=40= static inline u64 =5F=5Fraw=5Freadq(const volatile void =5F=5Fiomem *add= r)> > /*> > * I/O port access primitives.> > */> > +=23define arch=5Fhas=5F= dev=5Fport() (0)> > Why not=3FMaybe I got it the wrong way around, but th= e comment in include/linux/io.h says:/* * Some systems do not have legacy= ISA devices. * /dev/port is not a valid interface on these systems. * So= for those archs, should define the following symbol. */So ... defining = it should mean no legacy ISA devices, right=3F> > > =23define IO=5FSPACE=5F= LIMIT 0xffff> > You probably want to increase this a bit, to allow multip= le host bridges> to have their own I/O space.OK, but to what size=3F> > >= =23define PCI=5FIOBASE ((void =5F=5Fiomem *)(MODULES=5FVADDR - SZ=5F2M))= > > And modify this location: There is no particular reason to have the I= /O space> mapped exactly 2MB below the loadable modules, as virtual addre= ss space is> essentially free.Will talk with Catalin about where to place= this.> > > +=23define ioport=5Fmap(port, nr) (PCI=5FIOBASE + ((port) & I= O=5FSPACE=5FLIMIT))> > +=23define ioport=5Funmap(addr)> > inline function= s=3FWill do, thanks=21> > > static inline u8 inb(unsigned long addr)> > =7B= > > return readb(addr + PCI=5FIOBASE);> > diff --git a/arch/arm64/include= /asm/pci.h b/arch/arm64/include/asm/pci.h> > new file mode 100644> > inde= x 0000000..dd084a3> > --- /dev/null> > +++ b/arch/arm64/include/asm/pci.h= > > =40=40 -0,0 +1,35 =40=40> > +=23ifndef =5F=5FASM=5FPCI=5FH> > +=23def= ine =5F=5FASM=5FPCI=5FH> > +=23ifdef =5F=5FKERNEL=5F=5F> > +> > +=23inclu= de > > +=23include > > +=23include > > +> > +=23include > > +=23include >= > +=23include > > +> > +=23define PCIBIOS=5FMIN=5FIO 0> > +=23define PCI= BIOS=5FMIN=5FMEM 0> > PCIBIOS=5FMIN=5FIO is normally set to 0x1000, to st= ay out of the ISA range.:) No ISA support=21 (Die ISA, die=21=21) > > > d= iff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c> > new file= mode 100644> > index 0000000..7b652cf> > --- /dev/null> > +++ b/arch/arm= 64/kernel/pci.c> > =40=40 -0,0 +1,112 =40=40> > None of this looks really= arm64 specific, nor should it be. I think> we should try a little harder= to move this as a default implementation> into common code, even if we s= tart out by having all architectures> override it.Agree. This is the R=46= C version. I didn't dare to post a patch with fixesfor all architectures.= :)> > > +int pci=5Fioremap=5Fio(unsigned int offset, phys=5Faddr=5Ft phy= s=5Faddr)> > +=7B> > + BUG=5FON(offset + SZ=5F64K - 1 > IO=5FSPACE=5FLIMI= T);> > +> > + return ioremap=5Fpage=5Frange((unsigned long)PCI=5FIOBASE += offset,> > + (unsigned long)PCI=5FIOBASE + offset + SZ=5F64K,> > + phys=5F= addr,> > + =5F=5Fpgprot(PROT=5FDEVICE=5FnGnRE));> > +=7D> > Not sure if w= e want to treat this one as architecture specific though.> It certainly w= on't be portable to x86, but it could be shared with> a couple of others.= We may also want to redesign the interface.> I've been thinking we could= make this function allocate space in the> Linux virtual I/O space apertu= re, and pass two resources into it> (physical I/O aperture and bus I/O ra= nge), and get the actual> io=5Foffset as the return value, or a negative = error number.Not sure I completely follow your idea.> > That way, you cou= ld have an arbitrary number of host bridges in the> system and each one g= ets a share of the virtual aperture until> it's full.One still needs to f= ix the pci=5Frequest=5Fregion use that checks againstioport=5Fresource. B= ut it is an interesting idea.> > Arnd> > Thanks for reviewing this patch=21= Liviu-- --52efec4a_327b23c6_a1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
kodiak furr liked your message with Boxer.


On =46ebruary 3, 2014 at 1:18:38 PM CST, Liviu Dudau wrote:
On Mon, =46eb 03, 2014 at 06:58:56PM +0000, Arnd Bergmann wrote:
> On Monday 03 =46ebruary 2014 18:43:48 Liviu Dudau wrote:
> > dif= f --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
= > > index 4cc813e..ce5bad2 100644
> > --- a/arch/arm64/include/asm/i= o.h
> > +++ b/arch/arm64/include/asm/io.h
> > =40=40 -120,9 +12= 0,13 =40=40 static inline u64 =5F=5Fraw=5Freadq(const volatile void =5F=5F= iomem *addr)
> > /*
> > * I/O port access primitives.
= > > */
> > +=23define arch=5Fhas=5Fdev=5Fport() (0)
>
>= Why not=3F

Maybe I got it the wrong way around, but the comme= nt in include/linux/io.h says:

/*
* Some systems do not = have legacy ISA devices.
* /dev/port is not a valid interface on th= ese systems.
* So for those archs, should define the fol= lowing symbol.
*/

So ... defining it should mean no lega= cy ISA devices, right=3F

>
> > =23define IO=5FSPACE=5FL= IMIT 0xffff
>
> You probably want to increase this a bit, to = allow multiple host bridges
> to have their own I/O space.

OK, but to what size=3F

>
> > =23define PCI=5FIOBASE = ((void =5F=5Fiomem *)(MODULES=5FVADDR - SZ=5F2M))
>
> And mod= ify this location: There is no particular reason to have the I/O space> mapped exactly 2MB below the loadable modules, as virtual address sp= ace is
> essentially free.

Will talk with Catalin about w= here to place this.

>
> > +=23define ioport=5Fmap(port, = nr) (PCI=5FIOBASE + ((port) & IO=5FSPACE=5FLIMIT))
> > +=23define io= port=5Funmap(addr)
>
> inline functions=3F

Will do,= thanks=21

>
> > static inline u8 inb(unsigned long add= r)
> > =7B
> > return readb(addr + PCI=5FIOBASE);
> > d= iff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h> > new file mode 100644
> > index 0000000..dd084a3
> > ---= /dev/null
> > +++ b/arch/arm64/include/asm/pci.h
> > =40=40 -0= ,0 +1,35 =40=40
> > +=23ifndef =5F=5FASM=5FPCI=5FH
> > +=23defi= ne =5F=5FASM=5FPCI=5FH
> > +=23ifdef =5F=5FKERNEL=5F=5F
> > +> > +=23include
> > +=23include > > +=23include
> > +
> > +=23include=
> > +=23include
> > +=23= include
> > +
> > +=23define PCI= BIOS=5FMIN=5FIO 0
> > +=23define PCIBIOS=5FMIN=5FMEM 0
>
> PCIBIOS=5FMIN=5FIO is normally set to 0x1000, to stay out of the ISA = range.

:) No ISA support=21 (Die ISA, die=21=21)

>=
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c=
> > new file mode 100644
> > index 0000000..7b652cf
> > -= -- /dev/null
> > +++ b/arch/arm64/kernel/pci.c
> > =40=40 -0,0 = +1,112 =40=40
>
> None of this looks really arm64 specific, no= r should it be. I think
> we should try a little harder to move this= as a default implementation
> into common code, even if we start ou= t by having all architectures
> override it.

Agree. This = is the R=46C version. I didn't dare to post a patch with fixes
for a= ll architectures. :)

>
> > +int pci=5Fioremap=5Fio(unsig= ned int offset, phys=5Faddr=5Ft phys=5Faddr)
> > +=7B
> > + BUG= =5FON(offset + SZ=5F64K - 1 > IO=5FSPACE=5FLIMIT);
> > +
> > + = return ioremap=5Fpage=5Frange((unsigned long)PCI=5FIOBASE + offset,
= > > + (unsigned long)PCI=5FIOBASE + offset + SZ=5F64K,
> > + p= hys=5Faddr,
> > + =5F=5Fpgprot(PROT=5FDEVICE=5FnGnRE));
> > = +=7D
>
> Not sure if we want to treat this one as architecture= specific though.
> It certainly won't be portable to x86, but it co= uld be shared with
> a couple of others. We may also want to redesig= n the interface.
> I've been thinking we could make this function al= locate space in the
> Linux virtual I/O space aperture, and pass two= resources into it
> (physical I/O aperture and bus I/O range), and = get the actual
> io=5Foffset as the return value, or a negative erro= r number.

Not sure I completely follow your idea.

>=
> That way, you could have an arbitrary number of host bridges in = the
> system and each one gets a share of the virtual aperture until=
> it's full.

One still needs to fix the pci=5Frequest=5F= region use that checks against
ioport=5Fresource. But it is an inter= esting idea.

>
> Arnd
>
>

Thanks= for reviewing this patch=21

Liviu

--
--52efec4a_327b23c6_a1-- --===============7143198835380267376== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linaro-kernel mailing list linaro-kernel@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-kernel --===============7143198835380267376==--