* Re: [PATCH 06/18] [POWERPC] Fix UARTLITE reg io for little-endian architectures (ie. microblaze)
From: Grant Likely @ 2007-09-28 20:52 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
In-Reply-To: <fa686aa40709281350y63475e93hd18e7da497c6f1b0@mail.gmail.com>
On 9/28/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> > On Fri, Sep 28, 2007 at 02:42:32PM -0600, Grant Likely wrote:
> > > On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> > > > Also, I'm not sure you need to cast port->membase to void*, do you? The
> > > > math will still be right since it's declared as char *.
> > >
> > > membase is now defined as u32*, so the cast is needed.
> >
> > Hm, I must have looked at a stale tree.
>
> No, wait. You're right. It is a char*. I'll drop the cast.
Wait, I'm wrong again... it's in/out_be32 that expects an (unsigned*).
The compiler complains without the cast.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 06/18] [POWERPC] Fix UARTLITE reg io for little-endian architectures (ie. microblaze)
From: Grant Likely @ 2007-09-28 20:50 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
In-Reply-To: <20070928204749.GD23749@lixom.net>
On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 28, 2007 at 02:42:32PM -0600, Grant Likely wrote:
> > On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> > > Also, I'm not sure you need to cast port->membase to void*, do you? The
> > > math will still be right since it's declared as char *.
> >
> > membase is now defined as u32*, so the cast is needed.
>
> Hm, I must have looked at a stale tree.
No, wait. You're right. It is a char*. I'll drop the cast.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 06/18] [POWERPC] Fix UARTLITE reg io for little-endian architectures (ie. microblaze)
From: Olof Johansson @ 2007-09-28 20:47 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <fa686aa40709281342h4f8ff3fdv265e8098d2da6f52@mail.gmail.com>
On Fri, Sep 28, 2007 at 02:42:32PM -0600, Grant Likely wrote:
> On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
>
> > Hmm, I see the start changed, and you're now reading/writing a full
> > 32-bit word instead of individual bytes. Still, looks a little fishy to
> > me. Wouldn't it be more appropriate to change the ULITE_RX offset to be
> > 3 higher and still read/write bytes?
> >
> > Or are the registers defined as 32-bit ones? (I don't remember, it was
> > so long since I touched uartlite myself. :-)
>
> All the registers are defined as 32 bit ones. I think it makes more
> sense to access the registers as they are documented, and it
> eliminates the 'magic' +3 needed to make it work now.
Ok, thaks for the clarification. Feel free to add it as motivation in
the patch description. :)
> > (Same for the other functions below, but the general principle applies.)
> >
> > Also, I'm not sure you need to cast port->membase to void*, do you? The
> > math will still be right since it's declared as char *.
>
> membase is now defined as u32*, so the cast is needed.
Hm, I must have looked at a stale tree.
Thanks,
-Olof
^ permalink raw reply
* Re: [PATCH 06/18] [POWERPC] Fix UARTLITE reg io for little-endian architectures (ie. microblaze)
From: Grant Likely @ 2007-09-28 20:42 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
In-Reply-To: <20070928203104.GC23749@lixom.net>
On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 28, 2007 at 12:17:13PM -0600, Grant Likely wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > Acked-by: John Williams <jwilliams@itee.uq.edu.au>
> > ---
> >
> > arch/ppc/syslib/virtex_devices.c | 2 +-
> > drivers/serial/uartlite.c | 32 ++++++++++++++++----------------
> > 2 files changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/ppc/syslib/virtex_devices.c b/arch/ppc/syslib/virtex_devices.c
> > index ace4ec0..270ad3a 100644
> > --- a/arch/ppc/syslib/virtex_devices.c
> > +++ b/arch/ppc/syslib/virtex_devices.c
> > @@ -28,7 +28,7 @@
> > .num_resources = 2, \
> > .resource = (struct resource[]) { \
> > { \
> > - .start = XPAR_UARTLITE_##num##_BASEADDR + 3, \
> > + .start = XPAR_UARTLITE_##num##_BASEADDR, \
> > .end = XPAR_UARTLITE_##num##_HIGHADDR, \
> > .flags = IORESOURCE_MEM, \
> > }, \
> > diff --git a/drivers/serial/uartlite.c b/drivers/serial/uartlite.c
> > index f5051cf..59b674a 100644
> > --- a/drivers/serial/uartlite.c
> > +++ b/drivers/serial/uartlite.c
> > @@ -61,7 +61,7 @@ static int ulite_receive(struct uart_port *port, int stat)
> > /* stats */
> > if (stat & ULITE_STATUS_RXVALID) {
> > port->icount.rx++;
> > - ch = readb(port->membase + ULITE_RX);
> > + ch = in_be32((void*)port->membase + ULITE_RX);
>
> Hmm, I see the start changed, and you're now reading/writing a full
> 32-bit word instead of individual bytes. Still, looks a little fishy to
> me. Wouldn't it be more appropriate to change the ULITE_RX offset to be
> 3 higher and still read/write bytes?
>
> Or are the registers defined as 32-bit ones? (I don't remember, it was
> so long since I touched uartlite myself. :-)
All the registers are defined as 32 bit ones. I think it makes more
sense to access the registers as they are documented, and it
eliminates the 'magic' +3 needed to make it work now.
>
> (Same for the other functions below, but the general principle applies.)
>
> Also, I'm not sure you need to cast port->membase to void*, do you? The
> math will still be right since it's declared as char *.
membase is now defined as u32*, so the cast is needed.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 02/18] Add Kconfig macros for Xilinx Virtex support
From: Grant Likely @ 2007-09-28 20:39 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
In-Reply-To: <20070928201946.GB23749@lixom.net>
On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 28, 2007 at 12:16:01PM -0600, Grant Likely wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > arch/powerpc/platforms/40x/Kconfig | 30 +++++++++++++++++-------------
> > 1 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> > index c3dce3b..1aae0e6 100644
> > --- a/arch/powerpc/platforms/40x/Kconfig
> > +++ b/arch/powerpc/platforms/40x/Kconfig
> > @@ -61,13 +61,14 @@ config WALNUT
> > help
> > This option enables support for the IBM PPC405GP evaluation board.
> >
> > -#config XILINX_ML300
> > -# bool "Xilinx-ML300"
> > -# depends on 40x
> > -# default y
> > -# select VIRTEX_II_PRO
> > -# help
> > -# This option enables support for the Xilinx ML300 evaluation board.
> > +config XILINX_VIRTEX_GENERIC_BOARD
> > + bool "Generic Xilinx Virtex board"
> > + depends on 40x
> > + default y
> > + select VIRTEX_II_PRO
> > + select VIRTEX_4_FX
> > + help
> > + This option enables generic support for Xilinx Virtex based boards.
>
> I would appreciate a bit verboser help text here, i.e. including what
> boards are considered generic. Maybe something like "...including ML403,
> <x>, <y>, and other 4FX/IIPro-based boards"?
Done.
This option enables generic support for Xilinx Virtex based boards.
+ The generic virtex board support matches any device tree which
+ specifies 'xilinx,virtex' in its compatible field. This includes
+ the Xilinx ML3xx and ML4xx reference designs using the powerpc
+ core.
+
+ Most Virtex designs should use this unless it needs to do some
+ special configuration at board probe time.
+
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH] cpm: Describe multi-user ram in its own device node.
From: Vitaly Bordug @ 2007-09-28 20:30 UTC (permalink / raw)
To: galak; +Cc: linuxppc-dev
In-Reply-To: <20070928190616.GB20213@loki.buserror.net>
Kumar,
Realizing this may suffer a bit from cleanest-dts flame war, but anyway I pretty much see a lot of
sense in getting this in during next merge window. Is this possible?
On Fri, 28 Sep 2007 14:06:16 -0500
Scott Wood wrote:
> The way the current CPM binding describes available multi-user (a.k.a.
> dual-ported) RAM doesn't work well when there are multiple free regions,
> and it doesn't work at all if the region doesn't begin at the start of
> the muram area (as the hardware needs to be programmed with offsets into
> this area). The latter situation can happen with SMC UARTs on CPM2, as its
> parameter RAM is relocatable, u-boot puts it at zero, and the kernel doesn't
> support moving it.
>
> It is now described with a muram node, similar to QE. The current CPM
> binding is sufficiently recent (i.e. never appeared in an official release)
> that compatibility with existing device trees is not an issue.
>
> The code supporting the new binding is shared between cpm1 and cpm2, rather
> than remain separated. QE should be able to use this code as well, once
> minor fixes are made to its device trees.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
Acked-by: Vitaly Bordug <vitb@kernel.crashing.org>
> ---
> Documentation/powerpc/booting-without-of.txt | 40 ++++++-
> arch/powerpc/Kconfig.debug | 6 +-
> arch/powerpc/boot/cpm-serial.c | 44 +++++--
> arch/powerpc/boot/dts/ep88xc.dts | 13 ++-
> arch/powerpc/boot/dts/mpc8272ads.dts | 11 ++
> arch/powerpc/boot/dts/mpc885ads.dts | 13 ++-
> arch/powerpc/boot/dts/pq2fads.dts | 13 ++-
> arch/powerpc/sysdev/commproc.c | 11 ++-
> arch/powerpc/sysdev/cpm2_common.c | 36 ++----
> arch/powerpc/sysdev/cpm_common.c | 159 ++++++++++++++++++++++++++
> drivers/serial/cpm_uart/cpm_uart_cpm2.c | 4 +-
> include/asm-powerpc/commproc.h | 12 ++
> include/asm-powerpc/cpm.h | 14 +++
> include/asm-powerpc/cpm2.h | 10 ++
> 14 files changed, 338 insertions(+), 48 deletions(-)
--
Sincerely, Vitaly
^ permalink raw reply
* Re: [PATCH 06/18] [POWERPC] Fix UARTLITE reg io for little-endian architectures (ie. microblaze)
From: Olof Johansson @ 2007-09-28 20:31 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20070928181704.18608.40317.stgit@trillian.cg.shawcable.net>
On Fri, Sep 28, 2007 at 12:17:13PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: John Williams <jwilliams@itee.uq.edu.au>
> ---
>
> arch/ppc/syslib/virtex_devices.c | 2 +-
> drivers/serial/uartlite.c | 32 ++++++++++++++++----------------
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/arch/ppc/syslib/virtex_devices.c b/arch/ppc/syslib/virtex_devices.c
> index ace4ec0..270ad3a 100644
> --- a/arch/ppc/syslib/virtex_devices.c
> +++ b/arch/ppc/syslib/virtex_devices.c
> @@ -28,7 +28,7 @@
> .num_resources = 2, \
> .resource = (struct resource[]) { \
> { \
> - .start = XPAR_UARTLITE_##num##_BASEADDR + 3, \
> + .start = XPAR_UARTLITE_##num##_BASEADDR, \
> .end = XPAR_UARTLITE_##num##_HIGHADDR, \
> .flags = IORESOURCE_MEM, \
> }, \
> diff --git a/drivers/serial/uartlite.c b/drivers/serial/uartlite.c
> index f5051cf..59b674a 100644
> --- a/drivers/serial/uartlite.c
> +++ b/drivers/serial/uartlite.c
> @@ -61,7 +61,7 @@ static int ulite_receive(struct uart_port *port, int stat)
> /* stats */
> if (stat & ULITE_STATUS_RXVALID) {
> port->icount.rx++;
> - ch = readb(port->membase + ULITE_RX);
> + ch = in_be32((void*)port->membase + ULITE_RX);
Hmm, I see the start changed, and you're now reading/writing a full
32-bit word instead of individual bytes. Still, looks a little fishy to
me. Wouldn't it be more appropriate to change the ULITE_RX offset to be
3 higher and still read/write bytes?
Or are the registers defined as 32-bit ones? (I don't remember, it was
so long since I touched uartlite myself. :-)
(Same for the other functions below, but the general principle applies.)
Also, I'm not sure you need to cast port->membase to void*, do you? The
math will still be right since it's declared as char *.
-Olof
^ permalink raw reply
* Re: [PATCH 01/18] Virtex: Add uartlite bootwrapper driver
From: Josh Boyer @ 2007-09-28 20:26 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, Arnd Bergmann
In-Reply-To: <fa686aa40709281304p62fe5e66k99ea934a4896d876@mail.gmail.com>
On Fri, 28 Sep 2007 14:04:04 -0600
"Grant Likely" <grant.likely@secretlab.ca> wrote:
> On 9/28/07, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 28 September 2007, Grant Likely wrote:
> > > +static void uartlite_putc(unsigned char c)
> > > +{
> > > +while ((in_be32(reg_base + 0x8) & 0x08) != 0); /* spin */
> > > +out_be32(reg_base + 0x4, c);
> > > +}
> >
> > When coding a spin-loop, it's better to do a cpu_relax() between
> > each attempt.
>
> Is cpu_relax even implemented in the bootwrapper?
No. And it doesn't need to be :)
josh
^ permalink raw reply
* Re: [PATCH 03/18] Virtex: add xilinx interrupt controller driver
From: Grant Likely @ 2007-09-28 20:26 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
In-Reply-To: <20070928201719.GA23749@lixom.net>
On 9/28/07, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 28, 2007 at 12:16:07PM -0600, Grant Likely wrote:
>
> > +/*
> > + * INTC Registers
> > + */
> > +#define ISR 0 /* Interrupt Status */
> > +#define IPR 4 /* Interrupt Pending */
> > +#define IER 8 /* Interrupt Enable */
> > +#define IAR 12 /* Interrupt Acknowledge */
> > +#define SIE 16 /* Set Interrupt Enable bits */
> > +#define CIE 20 /* Clear Interrupt Enable bits */
> > +#define IVR 24 /* Interrupt Vector */
> > +#define MER 28 /* Master Enable */
>
> The defines are fairly generic, I guess you haven't ran across cases
> where there's naming conflicts, but you might want to prefix them with
> something more unique just in case.
Will do
>
> > +static void xilinx_intc_ack(unsigned int virq)
> > +{
> > + int irq = irq_map[virq].hwirq;
> > + void * regs = get_irq_chip_data(virq);
> > + pr_debug("ack: %d\n", irq);
> > + out_be32(regs + IAR, 1 << irq);
> > +}
>
> I guess some of the above are open-coded instead of using virq_to_hw()
> for performance reasons, it could be useful to have comments regarding
> this so they aren't changed by some janitor down the road. Or, in case
> they're not performance-critical, change them to use virq_to_hw.
Or it was just that my example code from another driver wasn't using
virq_to_hw() either. I'll fix this.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH] cpm: Describe multi-user ram in its own device node.
From: Vitaly Bordug @ 2007-09-28 20:25 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <46FD5FCB.4010908@freescale.com>
Hello Scott,
On Fri, 28 Sep 2007 15:10:51 -0500
Scott Wood wrote:
> Vitaly Bordug wrote:
> > Hello Scott,
> >
> > Looks good, only one note:
> >
> > On Fri, 28 Sep 2007 14:06:16 -0500
> > Scott Wood wrote:
> >
> >> + im_dprambase = cpm2_immr->im_dprambase;
> >> +
> >> /* Attach the usable dpmem area */
> >> /* XXX: This is actually crap. CPM_DATAONLY_BASE and
> >> * CPM_DATAONLY_SIZE is only a subset of the available dpram. It
> >> * varies with the processor and the microcode patches activated.
> >> * But the following should be at least safe.
> >> */
> >> - rh_attach_region(&cpm_dpmem_info, 0, r.end - r.start + 1);
> >> + rh_attach_region(&cpm_dpmem_info, CPM_MAP_ADDR + CPM_DATAONLY_BASE,
> >> + CPM_DATAONLY_SIZE);
> >> }
> >>
> >
> > Can we have something to address upper comment? I mean,any way to
> > have dpram beginning and size encoded in the device tree? We seem to
> > be adding new bus, and still pulling the information from the
> > defines. Maybe I miss something here, but it looks a bit odd.
>
> This bit is #ifndef CONFIG_PPC_CPM_NEW_BINDING (and can come out once
> all arch/powerpc boards are converted and tested -- I think it's just
> mpc866ads and CPM mpc85xx left to go). The new code in
> arch/powerpc/sysdev/cpm_common.c does get it from the device tree.
>
ok, sorry for the noise. If so, I'll try to test-n-fix upper two soon. Unfortunately,
there are many 8xx in ppc, that may depend on cpm (need to check).
--
Sincerely, Vitaly
^ permalink raw reply
* Re: [PATCH 02/18] Add Kconfig macros for Xilinx Virtex support
From: Olof Johansson @ 2007-09-28 20:19 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20070928181558.18608.23527.stgit@trillian.cg.shawcable.net>
On Fri, Sep 28, 2007 at 12:16:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> arch/powerpc/platforms/40x/Kconfig | 30 +++++++++++++++++-------------
> 1 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index c3dce3b..1aae0e6 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -61,13 +61,14 @@ config WALNUT
> help
> This option enables support for the IBM PPC405GP evaluation board.
>
> -#config XILINX_ML300
> -# bool "Xilinx-ML300"
> -# depends on 40x
> -# default y
> -# select VIRTEX_II_PRO
> -# help
> -# This option enables support for the Xilinx ML300 evaluation board.
> +config XILINX_VIRTEX_GENERIC_BOARD
> + bool "Generic Xilinx Virtex board"
> + depends on 40x
> + default y
> + select VIRTEX_II_PRO
> + select VIRTEX_4_FX
> + help
> + This option enables generic support for Xilinx Virtex based boards.
I would appreciate a bit verboser help text here, i.e. including what
boards are considered generic. Maybe something like "...including ML403,
<x>, <y>, and other 4FX/IIPro-based boards"?
-Olof
^ permalink raw reply
* Re: [PATCH 1/2] qemu platform, v2
From: Rob Landley @ 2007-09-28 20:14 UTC (permalink / raw)
To: Segher Boessenkool
Cc: linuxppc-dev, Paul Mackerras, Christoph Hellwig, Milton Miller,
David Gibson
In-Reply-To: <4d27912d86d1373ca01d43bd296e237b@kernel.crashing.org>
On Friday 28 September 2007 11:53:28 am Segher Boessenkool wrote:
> >> I'd be following this more closely if compiling a device tree didn't
> >> currently
> >> require an external utility (dtc or some such) that doesn't come with
> >> the
> >> Linux kernel. No other target platform I've built kernels for
> >> requires such
> >> an environmental dependency.
> >
> > No? You haven't built kernels for other platforms that have external
> > dependencies such as perl, gcc, make, binutils, etc.? :)
>
> Two of the supported Linux archs cannot be built with a mainline
> compiler, even!
Strange definition of "supported"...
> And I have to install GNU sed/awk to get builds to work, too.
I extended busybox sed until it built everything I threw at it. (I even added
a "lie to autoconf" step that replies to --version to say "This is not gnu
sed 4.0" so a regex in autoconf doesn't do something stupid.)
That's how I got into busybox development in the first place...
> OTOH, it would be nice if we didn't need DTC -- it itself doesn't
> build out-of-the-box on all systems, either ;-)
I've built x86, x86-64, mips, arm, and sparc. None of them needed anything
but the seven packages I mentioned. I'm poking at adding m68k, alpha, bfin,
and powerpc, but my free time's been spoken for recently. (I'll become
interested in sh or parisc when qemu grows support for it. I'm only
interested in bfin because I was given some free blackfin hardware at OLS.)
I've even poked at running s390 under Hercules but the setup was insane enough
to throw it way the heck down on my todo list. (Step 1: download and
configure an IBM operating system image from the 1970's... Oh yeah, fills me
with enthusiasm...)
> >> (This is a problem both for hardwiring the
> >> device tree into the kernel and for building a new boot rom from the
> >> linux
> >> kernel's ppc boot wrapper that would contain such a device tree to
> >> feed to
> >> the kernel.)
> >
> > It's only really been a problem for ps3 so far, since the embedded
> > guys don't seem to have any difficulty with installing dtc. We are
> > looking at what to do for ps3 and prep, and the answer may well
> > involve bundling dtc in the kernel source (it's not too big, around
> > 3400 lines).
>
> If only a few platforms have this problem, we could instead include
> their .dtb files in the kernel source tree.
I've had it clarified that the recent qemu-ppc patches from Milton only
require dtc to build the ROM image to boot qemu with. The Linux kernel image
you build hasn't got a device tree in it (although that means it needs one
passed in from the rom image)...
Using some other patches that floated by, I did build a prep kernel a couple
of weeks ago, which qemu happily handed control off to... which then failed
to boot because it couldn't parse the incomplete residual data left over from
open hackware. The solution the ppc list recommended? Hardwire a device
tree into said linux kernel using dtc...
>
> Segher
Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.
^ permalink raw reply
* Re: [PATCH 03/18] Virtex: add xilinx interrupt controller driver
From: Olof Johansson @ 2007-09-28 20:17 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20070928181606.18608.30412.stgit@trillian.cg.shawcable.net>
On Fri, Sep 28, 2007 at 12:16:07PM -0600, Grant Likely wrote:
> +/*
> + * INTC Registers
> + */
> +#define ISR 0 /* Interrupt Status */
> +#define IPR 4 /* Interrupt Pending */
> +#define IER 8 /* Interrupt Enable */
> +#define IAR 12 /* Interrupt Acknowledge */
> +#define SIE 16 /* Set Interrupt Enable bits */
> +#define CIE 20 /* Clear Interrupt Enable bits */
> +#define IVR 24 /* Interrupt Vector */
> +#define MER 28 /* Master Enable */
The defines are fairly generic, I guess you haven't ran across cases
where there's naming conflicts, but you might want to prefix them with
something more unique just in case.
> +static struct irq_host *master_irqhost;
> +
> +/*
> + * IRQ Chip operations
> + */
> +static void xilinx_intc_mask(unsigned int virq)
> +{
> + int irq = irq_map[virq].hwirq;
> + void * regs = get_irq_chip_data(virq);
> + pr_debug("mask: %d\n", irq);
> + out_be32(regs + CIE, 1 << irq);
> +}
> +
> +static void xilinx_intc_unmask(unsigned int virq)
> +{
> + int irq = irq_map[virq].hwirq;
> + void * regs = get_irq_chip_data(virq);
> + pr_debug("unmask: %d\n", irq);
> + out_be32(regs + SIE, 1 << irq);
> +}
> +
> +static void xilinx_intc_ack(unsigned int virq)
> +{
> + int irq = irq_map[virq].hwirq;
> + void * regs = get_irq_chip_data(virq);
> + pr_debug("ack: %d\n", irq);
> + out_be32(regs + IAR, 1 << irq);
> +}
I guess some of the above are open-coded instead of using virq_to_hw()
for performance reasons, it could be useful to have comments regarding
this so they aren't changed by some janitor down the road. Or, in case
they're not performance-critical, change them to use virq_to_hw.
-Olof
^ permalink raw reply
* Re: [PATCH] cpm: Describe multi-user ram in its own device node.
From: Scott Wood @ 2007-09-28 20:10 UTC (permalink / raw)
To: Vitaly Bordug; +Cc: linuxppc-dev
In-Reply-To: <20070929000621.2d332e86@kernel.crashing.org>
Vitaly Bordug wrote:
> Hello Scott,
>
> Looks good, only one note:
>
> On Fri, 28 Sep 2007 14:06:16 -0500
> Scott Wood wrote:
>
>> + im_dprambase = cpm2_immr->im_dprambase;
>> +
>> /* Attach the usable dpmem area */
>> /* XXX: This is actually crap. CPM_DATAONLY_BASE and
>> * CPM_DATAONLY_SIZE is only a subset of the available dpram. It
>> * varies with the processor and the microcode patches activated.
>> * But the following should be at least safe.
>> */
>> - rh_attach_region(&cpm_dpmem_info, 0, r.end - r.start + 1);
>> + rh_attach_region(&cpm_dpmem_info, CPM_MAP_ADDR + CPM_DATAONLY_BASE,
>> + CPM_DATAONLY_SIZE);
>> }
>>
>
> Can we have something to address upper comment? I mean,any way to
> have dpram beginning and size encoded in the device tree? We seem to
> be adding new bus, and still pulling the information from the
> defines. Maybe I miss something here, but it looks a bit odd.
This bit is #ifndef CONFIG_PPC_CPM_NEW_BINDING (and can come out once
all arch/powerpc boards are converted and tested -- I think it's just
mpc866ads and CPM mpc85xx left to go). The new code in
arch/powerpc/sysdev/cpm_common.c does get it from the device tree.
-Scott
^ permalink raw reply
* Re: [PATCH] cpm: Describe multi-user ram in its own device node.
From: Vitaly Bordug @ 2007-09-28 20:06 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <20070928190616.GB20213@loki.buserror.net>
Hello Scott,
Looks good, only one note:
On Fri, 28 Sep 2007 14:06:16 -0500
Scott Wood wrote:
> + im_dprambase = cpm2_immr->im_dprambase;
> +
> /* Attach the usable dpmem area */
> /* XXX: This is actually crap. CPM_DATAONLY_BASE and
> * CPM_DATAONLY_SIZE is only a subset of the available dpram. It
> * varies with the processor and the microcode patches activated.
> * But the following should be at least safe.
> */
> - rh_attach_region(&cpm_dpmem_info, 0, r.end - r.start + 1);
> + rh_attach_region(&cpm_dpmem_info, CPM_MAP_ADDR + CPM_DATAONLY_BASE,
> + CPM_DATAONLY_SIZE);
> }
>
Can we have something to address upper comment? I mean,any way to have dpram beginning and size encoded in the device tree? We seem to be adding new bus, and still pulling the information from the defines. Maybe I miss something
here, but it looks a bit odd.
--
Sincerely, Vitaly
^ permalink raw reply
* Re: [PATCH 00/18] Virtex support in arch/powerpc
From: Grant Likely @ 2007-09-28 20:05 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200709282146.14944.arnd@arndb.de>
On 9/28/07, Arnd Bergmann <arnd@arndb.de> wrote:
> Hi Grant!
>
> On Friday 28 September 2007, Grant Likely wrote:
> > This series adds Xilinx Virtex support to arch/powerpc. Please review
> > and comment. It includes support for the uartlite and SystemACE devices
>
> Very nice set of patches, I looked at all of them but couldn't find much
> to complain about, just trivial details.
Thank you very much for the review!
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 01/18] Virtex: Add uartlite bootwrapper driver
From: Grant Likely @ 2007-09-28 20:04 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200709282145.09411.arnd@arndb.de>
On 9/28/07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 28 September 2007, Grant Likely wrote:
> > +static void uartlite_putc(unsigned char c)
> > +{
> > +while ((in_be32(reg_base + 0x8) & 0x08) != 0); /* spin */
> > +out_be32(reg_base + 0x4, c);
> > +}
>
> When coding a spin-loop, it's better to do a cpu_relax() between
> each attempt.
Is cpu_relax even implemented in the bootwrapper?
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 10/18] Uartlite: improve in-code comments
From: Grant Likely @ 2007-09-28 20:02 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200709282143.16815.arnd@arndb.de>
On 9/28/07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 28 September 2007, Grant Likely wrote:
> > +/* ---------------------------------------------------------------------
> > + * Core UART driver operations
> > + */
> > +
>
> This is a rather unusual style of commenting. IMHO it would be better if you
> left out the ----- line.
I find the horizontal breaks useful when parsing through the code. If
others agree with you, then I'll happily remove them.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 00/18] Virtex support in arch/powerpc
From: Arnd Bergmann @ 2007-09-28 19:46 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070928181421.18608.74224.stgit@trillian.cg.shawcable.net>
Hi Grant!
On Friday 28 September 2007, Grant Likely wrote:
> This series adds Xilinx Virtex support to arch/powerpc. =A0Please review
> and comment. =A0It includes support for the uartlite and SystemACE devices
Very nice set of patches, I looked at all of them but couldn't find much
to complain about, just trivial details.
Arnd <><
^ permalink raw reply
* Re: [PATCH 12/18] Uartlite: Let the console be initialized earlier
From: Grant Likely @ 2007-09-28 20:01 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
In-Reply-To: <200709282140.07000.arnd@arndb.de>
On 9/28/07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 28 September 2007, Grant Likely wrote:
> > +#else /* CONFIG_OF */
> > +static void __init ulite_console_of_find_device(int id) { /* do nothing */ }
> > +#endif /* CONFIG_OF */
>
> Shouldn't this be inline? It shouldn't matter much since most of the time
> gcc -funit-at-a-time takes care of this, but it's common to make the inlining
> explicit.
heh, I even had the inline in there on an earlier version of the
patch. I can add it back it.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
* Re: [PATCH 01/18] Virtex: Add uartlite bootwrapper driver
From: Arnd Bergmann @ 2007-09-28 19:45 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070928181551.18608.88418.stgit@trillian.cg.shawcable.net>
On Friday 28 September 2007, Grant Likely wrote:
> +static void uartlite_putc(unsigned char c)
> +{
> +=A0=A0=A0=A0=A0=A0=A0while ((in_be32(reg_base + 0x8) & 0x08) !=3D 0); /*=
spin */
> +=A0=A0=A0=A0=A0=A0=A0out_be32(reg_base + 0x4, c);
> +}
When coding a spin-loop, it's better to do a cpu_relax() between
each attempt.
Arnd <><
^ permalink raw reply
* Re: [PATCH 10/18] Uartlite: improve in-code comments
From: Arnd Bergmann @ 2007-09-28 19:43 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070928181740.18608.67126.stgit@trillian.cg.shawcable.net>
On Friday 28 September 2007, Grant Likely wrote:
> +/* ---------------------------------------------------------------------
> + * Core UART driver operations
> + */
> +
This is a rather unusual style of commenting. IMHO it would be better if you
left out the ----- line.
Arnd <><
^ permalink raw reply
* Re: [PATCH 12/18] Uartlite: Let the console be initialized earlier
From: Arnd Bergmann @ 2007-09-28 19:40 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070928181754.18608.52779.stgit@trillian.cg.shawcable.net>
On Friday 28 September 2007, Grant Likely wrote:
> +#else /* CONFIG_OF */
> +static void __init ulite_console_of_find_device(int id) { /* do nothing */ }
> +#endif /* CONFIG_OF */
Shouldn't this be inline? It shouldn't matter much since most of the time
gcc -funit-at-a-time takes care of this, but it's common to make the inlining
explicit.
Arnd <><
^ permalink raw reply
* Re: [PATCH 11/18] Virtex: Port UARTLITE driver to of-platform-bus
From: Arnd Bergmann @ 2007-09-28 19:32 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20070928181748.18608.62409.stgit@trillian.cg.shawcable.net>
T24gRnJpZGF5IDI4IFNlcHRlbWJlciAyMDA3LCBHcmFudCBMaWtlbHkgd3JvdGU6Cj4gLaCgoKCg
oKByZXQgPSB1YXJ0X3JlZ2lzdGVyX2RyaXZlcigmdWxpdGVfdWFydF9kcml2ZXIpOwo+IC2goKCg
oKCgaWYgKHJldCkKPiAtoKCgoKCgoKCgoKCgoKCgcmV0dXJuIHJldDsKPiAroKCgoKCgoHByX2Rl
YnVnKCJ1YXJ0bGl0ZTogY2FsbGluZyB1YXJ0X3JlZ2lzdGVyX2RyaXZlcigpXG4iKTsKPiAroKCg
oKCgoGlmICgocmV0ID0gdWFydF9yZWdpc3Rlcl9kcml2ZXIoJnVsaXRlX3VhcnRfZHJpdmVyKSkg
IT0gMCkKPiAroKCgoKCgoKCgoKCgoKCgZ290byBlcnJfdWFydDsKPiCgCj4gLaCgoKCgoKByZXQg
PSBwbGF0Zm9ybV9kcml2ZXJfcmVnaXN0ZXIoJnVsaXRlX3BsYXRmb3JtX2RyaXZlcik7Cj4gLaCg
oKCgoKBpZiAocmV0KQo+IC2goKCgoKCgoKCgoKCgoKB1YXJ0X3VucmVnaXN0ZXJfZHJpdmVyKCZ1
bGl0ZV91YXJ0X2RyaXZlcik7Cj4gK6CgoKCgoKBpZiAoKHJldCA9IHVsaXRlX29mX3JlZ2lzdGVy
KCkpICE9IDApCj4gK6CgoKCgoKCgoKCgoKCgoGdvdG8gZXJyX29mOwo+IKAKPiAroKCgoKCgoHBy
X2RlYnVnKCJ1YXJ0bGl0ZTogY2FsbGluZyBwbGF0Zm9ybV9kcml2ZXJfcmVnaXN0ZXIoKVxuIik7
Cj4gK6CgoKCgoKBpZiAoKHJldCA9IHBsYXRmb3JtX2RyaXZlcl9yZWdpc3RlcigmdWxpdGVfcGxh
dGZvcm1fZHJpdmVyKSkgIT0gMCkKPiAroKCgoKCgoKCgoKCgoKCgZ290byBlcnJfcGxhdDsKPiAr
Cj4gK6CgoKCgoKByZXR1cm4gMDsKCkkgZG9uJ3Qgc2VlIHRoaXMgYXMgbXVjaCBvZiBhbiBpbXBy
b3ZlbWVudC4gRG9pbmcgYW4gYXNzaWdubWVudCBpbiB0aGUKY29uZGl0aW9uIG1ha2VzIHRoZSBj
b2RlIGxlc3MgcmVhZGFibGUgSU1ITywgc28gaXQgc2hvdWxkIHJlYWxseQpiZSB3cml0dGVuIGFz
CgoJcmV0ID0gdHJ5X3NvbWV0aGluZygpOwoJaWYgKHJldCkKCQlnb3RvIHNvbWV0aGluZ19mYWls
ZWQ7CgpUaGlzIGtlZXBzIHRoZSBub3JtYWwgY29kZSB0byB0aGUgbGVmdCwgYW5kIHRoZSBlcnJv
ciBoYW5kbGluZyBpbmRlbnRlZAp0byB0aGUgcmlnaHQuCgoKCUFybmQgPD48Cg==
^ permalink raw reply
* Re: [PATCH 02/18] Add Kconfig macros for Xilinx Virtex support
From: Grant Likely @ 2007-09-28 19:35 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <46FD45FA.3000205@freescale.com>
On 9/28/07, Scott Wood <scottwood@freescale.com> wrote:
> Grant Likely wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > arch/powerpc/platforms/40x/Kconfig | 30 +++++++++++++++++-------------
> > 1 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> > index c3dce3b..1aae0e6 100644
> > --- a/arch/powerpc/platforms/40x/Kconfig
> > +++ b/arch/powerpc/platforms/40x/Kconfig
> > @@ -61,13 +61,14 @@ config WALNUT
> > help
> > This option enables support for the IBM PPC405GP evaluation board.
> >
> > -#config XILINX_ML300
> > -# bool "Xilinx-ML300"
> > -# depends on 40x
> > -# default y
> > -# select VIRTEX_II_PRO
> > -# help
> > -# This option enables support for the Xilinx ML300 evaluation board.
> > +config XILINX_VIRTEX_GENERIC_BOARD
> > + bool "Generic Xilinx Virtex board"
> > + depends on 40x
> > + default y
> > + select VIRTEX_II_PRO
> > + select VIRTEX_4_FX
> > + help
> > + This option enables generic support for Xilinx Virtex based boards.
>
> I don't think we want default y here.
I just followed the lead of Walnut here. Perhaps for the embedded
targets all of them should be 'default n'. Josh, thoughts?
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox