* Re: [PATCH] mpc52xx-psc-spi: refactor probe and remove to make use of of_register_spi_devices()
From: Stephen Rothwell @ 2009-10-31 5:32 UTC (permalink / raw)
To: Wolfram Sang
Cc: Kári Davíðsson, spi-devel-general, linuxppc-dev
In-Reply-To: <1256931852-13255-1-git-send-email-w.sang@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 901 bytes --]
Hi Wolfram,
On Fri, 30 Oct 2009 20:44:12 +0100 Wolfram Sang <w.sang@pengutronix.de> wrote:
>
> static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
> .owner = THIS_MODULE,
> - .name = "mpc52xx-psc-spi",
> + .name = DRIVER_NAME,
You no longer need to set either owner or name in the of_platform driver,
just in the included struct driver (as is done below), so you could just
remove the above two lines.
> .match_table = mpc52xx_psc_spi_of_match,
> .probe = mpc52xx_psc_spi_of_probe,
> .remove = __exit_p(mpc52xx_psc_spi_of_remove),
> .driver = {
> - .name = "mpc52xx-psc-spi",
> + .name = DRIVER_NAME,
> .owner = THIS_MODULE,
> },
> };
I am hoping that we can remove the owner and name fields from struct
of_platform_driver sometime.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Regarding MPIC pluggable interrupt handler
From: Thirumalai @ 2009-10-31 7:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
Hi Ben,
Is there any standard way to write pluggable interrupt handler for
my FPGA. I have seen the reference code on 85xx/socrates_fpga_pic.c. But it
is not sufficient to understand. Kindly give any reference.
Thank you
T.
**************** CAUTION - Disclaimer *****************This email may contain confidential and privileged material for the
sole use of the intended recipient(s). Any review, use, retention, distribution or disclosure by others is strictly prohibited. If you are not the intended recipient (or authorized to receive for the recipient), please contact the sender by reply email and delete all copies of this message. Also, email is susceptible to data corruption, interception, tampering, unauthorized amendment and viruses. We only send and receive emails on the basis that we are not liable for any such corruption, interception, tampering, amendment or viruses or any consequence thereof.
*********** End of Disclaimer ***********DataPatterns ITS Group**********
^ permalink raw reply
* Re: [PATCH] mpc52xx-psc-spi: refactor probe and remove to make use of of_register_spi_devices()
From: Wolfram Sang @ 2009-10-31 9:03 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Kári Davíðsson, spi-devel-general, linuxppc-dev
In-Reply-To: <20091031163259.5c651773.sfr@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 988 bytes --]
Hi Stephen,
> > static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
> > .owner = THIS_MODULE,
> > - .name = "mpc52xx-psc-spi",
> > + .name = DRIVER_NAME,
>
> You no longer need to set either owner or name in the of_platform driver,
> just in the included struct driver (as is done below), so you could just
> remove the above two lines.
Okay, thanks. Will drop it.
>
> > .match_table = mpc52xx_psc_spi_of_match,
> > .probe = mpc52xx_psc_spi_of_probe,
> > .remove = __exit_p(mpc52xx_psc_spi_of_remove),
> > .driver = {
> > - .name = "mpc52xx-psc-spi",
> > + .name = DRIVER_NAME,
> > .owner = THIS_MODULE,
> > },
> > };
>
> I am hoping that we can remove the owner and name fields from struct
> of_platform_driver sometime.
Sounds good! :)
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 0/8] Fix 8xx MMU/TLB
From: Joakim Tjernlund @ 2009-10-31 10:31 UTC (permalink / raw)
To: Scott Wood; +Cc: Rex Feany, linuxppc-dev@ozlabs.org
In-Reply-To: <20091030173749.GA855@loki.buserror.net>
Scott Wood <scottwood@freescale.com> wrote on 30/10/2009 18:37:49:
>
> On Fri, Oct 30, 2009 at 12:16:07PM -0500, Scott Wood wrote:
> > On Sat, Oct 17, 2009 at 02:01:38PM +0200, Joakim Tjernlund wrote:
> > > + mfspr r10, SPRN_SRR0
> > > DO_8xx_CPU6(0x3780, r3)
> > > mtspr SPRN_MD_EPN, r10
> > > mfspr r11, SPRN_M_TWB /* Get level 1 table entry address */
> > > - lwz r11, 0(r11) /* Get the level 1 entry */
> > > + cmplwi cr0, r11, 0x0800
> > > + blt- 3f /* Branch if user space */
> > > + lis r11, swapper_pg_dir@h
> > > + ori r11, r11, swapper_pg_dir@l
> > > + rlwimi r11, r11, 0, 2, 19
> >
> > That rlwimi is a no-op -- I think you meant to use a different register
> > here?
> >
> > > +3: lwz r11, 0(r11) /* Get the level 1 entry */
> > > DO_8xx_CPU6(0x3b80, r3)
> > > mtspr SPRN_MD_TWC, r11 /* Load pte table base address */
> > > mfspr r11, SPRN_MD_TWC /* ....and get the pte address */
> > > lwz r11, 0(r11) /* Get the pte */
> > > /* concat physical page address(r11) and page offset(r10) */
> > > rlwimi r11, r10, 0, 20, 31
> >
> > But r10 here contains SRR0 from above, and this is a data TLB error.
>
> Never mind that last one, forgot that you'd be wanting to load the
> instruction. :-P
>
> But the rlwimi is what's causing the machine checks. I replaced it with:
Yes, I see now that it is wrong.
> rlwinm r11, r11, 0, 0x3ffff000
> rlwimi r11, r10, 22, 0xffc
>
> and things seem to work. You could probably replace the rlwinm by
> subtracting PAGE_OFFSET from swapper_pg_dir instead.
Would you mind produce a proper path on top of my series? I am blind
here so I can only guess what will work or not.
Then Rex can give it some beating and we can push this to Ben
^ permalink raw reply
* Re: [PATCH 19/27] Export symbols for KVM module
From: Alexander Graf @ 2009-10-31 12:02 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Kevin Wolf, Arnd Bergmann, Hollis Blanchard, Marcelo Tosatti,
kvm-ppc, linuxppc-dev@ozlabs.org, Avi Kivity, kvm@vger.kernel.org,
bphilips@suse.de, Olof Johansson
In-Reply-To: <20091031153719.10a4e61b.sfr@canb.auug.org.au>
Am 31.10.2009 um 05:37 schrieb Stephen Rothwell <sfr@canb.auug.org.au>:
> Hi Alexander,
>
> On Fri, 30 Oct 2009 16:47:19 +0100 Alexander Graf <agraf@suse.de>
> wrote:
>>
>> diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/
>> ppc_ksyms.c
>> index c8b27bb..baf778c 100644
>> --- a/arch/powerpc/kernel/ppc_ksyms.c
>> +++ b/arch/powerpc/kernel/ppc_ksyms.c
>> @@ -163,11 +163,12 @@ EXPORT_SYMBOL(screen_info);
>> #ifdef CONFIG_PPC32
>> EXPORT_SYMBOL(timer_interrupt);
>> EXPORT_SYMBOL(irq_desc);
>> -EXPORT_SYMBOL(tb_ticks_per_jiffy);
>> EXPORT_SYMBOL(cacheable_memcpy);
>> EXPORT_SYMBOL(cacheable_memzero);
>> #endif
>>
>> +EXPORT_SYMBOL(tb_ticks_per_jiffy);
>
> Since you are moving this anyway, how about moving it into
> arch/powerpc/kernel/time.c where tb_ticks_per_jiffy is defined.
Well the fun part is that the hrtimer conversion patch actually
deprecates this change.
I merely forgot to change the export back.
So I suppose I'll leave things here as is and then revert this chunk
in the hrtimer patch.
Alex
^ permalink raw reply
* Re: mpc512x/clock: fix clk_get logic
From: Wolfram Sang @ 2009-10-31 13:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Wolfgang Denk
In-Reply-To: <1256939657.6372.81.camel@pasglop>
[-- Attachment #1: Type: text/plain, Size: 846 bytes --]
Hello Ben,
> Have you considered switching to my proposed device-tree based clock
> reprentation ?
You mean this one?
http://patchwork.ozlabs.org/patch/31551/
Sorry, I have missed it up to now :(
While I think (after one glimpse) this moves things into the right direction,
my priority at the moment is to get the mscan driver working on 512x (and then
to mainline). For that, I needed the patch I posted. I hope I can have another
look at your proposal later on, it looks really worth it.
Until then I'd propose to consider this patch as it fixes a bug in clock
assignment. But I leave the final decision to you maintainers, of course.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* RE: Accessing flash directly from User Space [SOLVED]
From: Joakim Tjernlund @ 2009-10-31 13:26 UTC (permalink / raw)
To: Jonathan Haws
Cc: scottwood@freescale.com, Alessandro Rubini, bgat@billgatliff.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <BB99A6BA28709744BF22A68E6D7EB51F0330E231BE@midas.usurf.usu.edu>
>
> > On Friday 30 October 2009 16:08:55 Alessandro Rubini wrote:
> > > > asm("eieio; sync");
> > >
> > > Hmm...
> > > : : : "memory"
> > >
> > > And, doesn't ";" start a comment in assembly? (no, not on powerpc
> > it seems)
> >
> > Yes, I think the barrier is wrong.
> > Please try with
> >
> > #define mb() __asm__ __volatile__("eieio\n sync\n" : : :
> > "memory")
>
> That definition worked great. I must have missed the : : : "memory" bit when
> I was digging through code.
>
> Thanks, that gives me about a 2x speedup over the msync() calls.
Exactly when should you use the barrier? At every access,
every read or when changing from write to read?
Jocke
^ permalink raw reply
* Re: Accessing flash directly from User Space [SOLVED]
From: Michael Buesch @ 2009-10-31 16:42 UTC (permalink / raw)
To: Joakim Tjernlund
Cc: scottwood@freescale.com, Alessandro Rubini, bgat@billgatliff.com,
linuxppc-dev@lists.ozlabs.org, Jonathan Haws
In-Reply-To: <OF8458219C.9E5B54DB-ONC1257660.0049761D-C1257660.0049DD9C@transmode.se>
On Saturday 31 October 2009 14:26:48 Joakim Tjernlund wrote:
> >
> > > On Friday 30 October 2009 16:08:55 Alessandro Rubini wrote:
> > > > > asm("eieio; sync");
> > > >
> > > > Hmm...
> > > > : : : "memory"
> > > >
> > > > And, doesn't ";" start a comment in assembly? (no, not on powerpc
> > > it seems)
> > >
> > > Yes, I think the barrier is wrong.
> > > Please try with
> > >
> > > #define mb() __asm__ __volatile__("eieio\n sync\n" : : :
> > > "memory")
> >
> > That definition worked great. I must have missed the : : : "memory" bit when
> > I was digging through code.
> >
> > Thanks, that gives me about a 2x speedup over the msync() calls.
>
> Exactly when should you use the barrier? At every access,
> every read or when changing from write to read?
Well, it depends on the device you are accessing. I'll give you a small pseudo example.
mmio[0] = address;
mmio[1] = data;
mb();
mmio[3] |= 0x01; /* This triggers an operation -> address=data */
/* probably also need an mb() here, if the following code
* depends on the operation to be triggered. */
--
Greetings, Michael.
^ permalink raw reply
* Re: Accessing flash directly from User Space [SOLVED]
From: Joakim Tjernlund @ 2009-10-31 20:14 UTC (permalink / raw)
To: Michael Buesch
Cc: scottwood@freescale.com, Alessandro Rubini, bgat@billgatliff.com,
linuxppc-dev@lists.ozlabs.org, Jonathan Haws
In-Reply-To: <200910311742.56280.mb@bu3sch.de>
Michael Buesch <mb@bu3sch.de> wrote on 31/10/2009 17:42:54:
>
> On Saturday 31 October 2009 14:26:48 Joakim Tjernlund wrote:
> > >
> > > > On Friday 30 October 2009 16:08:55 Alessandro Rubini wrote:
> > > > > > asm("eieio; sync");
> > > > >
> > > > > Hmm...
> > > > > : : : "memory"
> > > > >
> > > > > And, doesn't ";" start a comment in assembly? (no, not on powerpc
> > > > it seems)
> > > >
> > > > Yes, I think the barrier is wrong.
> > > > Please try with
> > > >
> > > > #define mb() __asm__ __volatile__("eieio\n sync\n" : : :
> > > > "memory")
> > >
> > > That definition worked great. I must have missed the : : : "memory" bit when
> > > I was digging through code.
> > >
> > > Thanks, that gives me about a 2x speedup over the msync() calls.
> >
> > Exactly when should you use the barrier? At every access,
> > every read or when changing from write to read?
>
> Well, it depends on the device you are accessing. I'll give you a small pseudo example.
>
> mmio[0] = address;
> mmio[1] = data;
> mb();
> mmio[3] |= 0x01; /* This triggers an operation -> address=data */
> /* probably also need an mb() here, if the following code
> * depends on the operation to be triggered. */
So anything that depends on the previous accesses needs a mb()
hmm, the mmio[0] and mmio[1] are written in order I hope?
Jocke
^ permalink raw reply
* Re: Accessing flash directly from User Space [SOLVED]
From: Michael Buesch @ 2009-10-31 20:35 UTC (permalink / raw)
To: Joakim Tjernlund
Cc: scottwood@freescale.com, Alessandro Rubini, bgat@billgatliff.com,
linuxppc-dev@lists.ozlabs.org, Jonathan Haws
In-Reply-To: <OF7FB0F99B.2962ECBA-ONC1257660.006EDDB5-C1257660.006F2808@transmode.se>
On Saturday 31 October 2009 21:14:07 Joakim Tjernlund wrote:
> Michael Buesch <mb@bu3sch.de> wrote on 31/10/2009 17:42:54:
> >
> > On Saturday 31 October 2009 14:26:48 Joakim Tjernlund wrote:
> > > >
> > > > > On Friday 30 October 2009 16:08:55 Alessandro Rubini wrote:
> > > > > > > asm("eieio; sync");
> > > > > >
> > > > > > Hmm...
> > > > > > : : : "memory"
> > > > > >
> > > > > > And, doesn't ";" start a comment in assembly? (no, not on powerpc
> > > > > it seems)
> > > > >
> > > > > Yes, I think the barrier is wrong.
> > > > > Please try with
> > > > >
> > > > > #define mb() __asm__ __volatile__("eieio\n sync\n" : : :
> > > > > "memory")
> > > >
> > > > That definition worked great. I must have missed the : : : "memory" bit when
> > > > I was digging through code.
> > > >
> > > > Thanks, that gives me about a 2x speedup over the msync() calls.
> > >
> > > Exactly when should you use the barrier? At every access,
> > > every read or when changing from write to read?
> >
> > Well, it depends on the device you are accessing. I'll give you a small pseudo example.
> >
> > mmio[0] = address;
> > mmio[1] = data;
> > mb();
> > mmio[3] |= 0x01; /* This triggers an operation -> address=data */
> > /* probably also need an mb() here, if the following code
> > * depends on the operation to be triggered. */
>
> So anything that depends on the previous accesses needs a mb()
No, not really. I would always put an mb() where the comment is.
Imagine you have two instances of the above (probably in a loop):
mmio[0] = address;
mmio[1] = data;
mb();
mmio[3] |= 0x01; /* This triggers an operation -> address=data */
mb();
mmio[0] = address;
mmio[1] = data;
mb();
mmio[3] |= 0x01; /* This triggers an operation -> address=data */
mb();
> hmm, the mmio[0] and mmio[1] are written in order I hope?
We do not care in this example, as the write to [3] does trigger
the device operation. We do only care that [0] and [1] are set
when [3] is written. We do not care in what order [0] and [1] are written.
This is just an artificial example, but I think you get my point.
My point just being is that an mb() does only belong where it matters.
And what matters is defined by your device specifications.
--
Greetings, Michael.
^ permalink raw reply
* Re: mpc512x/clock: fix clk_get logic
From: Benjamin Herrenschmidt @ 2009-10-31 20:48 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev, Wolfgang Denk
In-Reply-To: <20091031130131.GA23009@pengutronix.de>
On Sat, 2009-10-31 at 14:01 +0100, Wolfram Sang wrote:
> Hello Ben,
>
> > Have you considered switching to my proposed device-tree based clock
> > reprentation ?
>
> You mean this one?
>
> http://patchwork.ozlabs.org/patch/31551/
>
> Sorry, I have missed it up to now :(
>
> While I think (after one glimpse) this moves things into the right direction,
> my priority at the moment is to get the mscan driver working on 512x (and then
> to mainline). For that, I needed the patch I posted. I hope I can have another
> look at your proposal later on, it looks really worth it.
>
> Until then I'd propose to consider this patch as it fixes a bug in clock
> assignment. But I leave the final decision to you maintainers, of course.
Sure I don't have major objections against your patch (though who is
formally mpc512x maintainer to ack it ?), I just wanted to make sure you
had a look at my proposal since this is almost the only platform to use
the clock API today in arch/powerpc.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 20/27] Split init_new_context and destroy_context
From: Alexander Graf @ 2009-10-31 21:20 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Kevin Wolf, Arnd Bergmann, Hollis Blanchard, Marcelo Tosatti,
kvm-ppc, linuxppc-dev, Avi Kivity, kvm, bphilips, Olof Johansson
In-Reply-To: <20091031154059.750d7213.sfr@canb.auug.org.au>
Stephen Rothwell wrote:
> Hi Alexander,
>
> On Fri, 30 Oct 2009 16:47:20 +0100 Alexander Graf <agraf@suse.de> wrote:
>
>> --- a/arch/powerpc/include/asm/mmu_context.h
>> +++ b/arch/powerpc/include/asm/mmu_context.h
>> @@ -23,6 +23,11 @@ extern void switch_slb(struct task_struct *tsk, struct mm_struct *mm);
>> extern void set_context(unsigned long id, pgd_t *pgd);
>>
>> #ifdef CONFIG_PPC_BOOK3S_64
>> +extern int __init_new_context(void);
>> +extern void __destroy_context(int context_id);
>> +#endif
>> +
>> +#ifdef CONFIG_PPC_BOOK3S_64
>>
>
> don't add the #endif/#ifdef pair ...
Any other comments? I'd like to wind up a final patch set so I can stop
spamming on all those MLs :-).
Alex
^ permalink raw reply
* Re: [PATCH 20/27] Split init_new_context and destroy_context
From: Benjamin Herrenschmidt @ 2009-10-31 21:37 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Stephen Rothwell, Arnd Bergmann, Hollis Blanchard,
Marcelo Tosatti, kvm-ppc, linuxppc-dev, Avi Kivity, kvm, bphilips,
Olof Johansson
In-Reply-To: <4AECAA0A.1080304@suse.de>
On Sat, 2009-10-31 at 22:20 +0100, Alexander Graf wrote:
> Stephen Rothwell wrote:
> > Hi Alexander,
> >
> > On Fri, 30 Oct 2009 16:47:20 +0100 Alexander Graf <agraf@suse.de> wrote:
> >
> >> --- a/arch/powerpc/include/asm/mmu_context.h
> >> +++ b/arch/powerpc/include/asm/mmu_context.h
> >> @@ -23,6 +23,11 @@ extern void switch_slb(struct task_struct *tsk, struct mm_struct *mm);
> >> extern void set_context(unsigned long id, pgd_t *pgd);
> >>
> >> #ifdef CONFIG_PPC_BOOK3S_64
> >> +extern int __init_new_context(void);
> >> +extern void __destroy_context(int context_id);
> >> +#endif
> >> +
> >> +#ifdef CONFIG_PPC_BOOK3S_64
> >>
> >
> > don't add the #endif/#ifdef pair ...
>
> Any other comments? I'd like to wind up a final patch set so I can stop
> spamming on all those MLs :-).
Just send an update for -that- patch to the list.
Ben.
^ permalink raw reply
* Re: Accessing flash directly from User Space [SOLVED]
From: Joakim Tjernlund @ 2009-10-31 22:31 UTC (permalink / raw)
To: Michael Buesch
Cc: scottwood@freescale.com, Alessandro Rubini, bgat@billgatliff.com,
linuxppc-dev@lists.ozlabs.org, Jonathan Haws
In-Reply-To: <200910312135.34196.mb@bu3sch.de>
Michael Buesch <mb@bu3sch.de> wrote on 31/10/2009 21:35:31:
>
> On Saturday 31 October 2009 21:14:07 Joakim Tjernlund wrote:
> > Michael Buesch <mb@bu3sch.de> wrote on 31/10/2009 17:42:54:
> > >
> > > On Saturday 31 October 2009 14:26:48 Joakim Tjernlund wrote:
> > > > >
> > > > > > On Friday 30 October 2009 16:08:55 Alessandro Rubini wrote:
> > > > > > > > asm("eieio; sync");
> > > > > > >
> > > > > > > Hmm...
> > > > > > > : : : "memory"
> > > > > > >
> > > > > > > And, doesn't ";" start a comment in assembly? (no, not on powerpc
> > > > > > it seems)
> > > > > >
> > > > > > Yes, I think the barrier is wrong.
> > > > > > Please try with
> > > > > >
> > > > > > #define mb() __asm__ __volatile__("eieio\n sync\n" : : :
> > > > > > "memory")
> > > > >
> > > > > That definition worked great. I must have missed the : : : "memory" bit when
> > > > > I was digging through code.
> > > > >
> > > > > Thanks, that gives me about a 2x speedup over the msync() calls.
> > > >
> > > > Exactly when should you use the barrier? At every access,
> > > > every read or when changing from write to read?
> > >
> > > Well, it depends on the device you are accessing. I'll give you a small
> pseudo example.
> > >
> > > mmio[0] = address;
> > > mmio[1] = data;
> > > mb();
> > > mmio[3] |= 0x01; /* This triggers an operation -> address=data */
> > > /* probably also need an mb() here, if the following code
> > > * depends on the operation to be triggered. */
> >
> > So anything that depends on the previous accesses needs a mb()
>
> No, not really. I would always put an mb() where the comment is.
> Imagine you have two instances of the above (probably in a loop):
>
> mmio[0] = address;
> mmio[1] = data;
> mb();
> mmio[3] |= 0x01; /* This triggers an operation -> address=data */
> mb();
>
> mmio[0] = address;
> mmio[1] = data;
> mb();
> mmio[3] |= 0x01; /* This triggers an operation -> address=data */
> mb();
>
> > hmm, the mmio[0] and mmio[1] are written in order I hope?
>
> We do not care in this example, as the write to [3] does trigger
> the device operation. We do only care that [0] and [1] are set
> when [3] is written. We do not care in what order [0] and [1] are written.
In this example yes, I was wondering in general.
>
> This is just an artificial example, but I think you get my point.
> My point just being is that an mb() does only belong where it matters.
> And what matters is defined by your device specifications.
So what does guarded memory mapping on ppc mean really? If I need
that much mb(), guarded does not seem to do much.
Jocke
^ permalink raw reply
* Re: [PATCH] mpc52xx-psc-spi: refactor probe and remove to make use of of_register_spi_devices()
From: Grant Likely @ 2009-11-01 1:03 UTC (permalink / raw)
To: Wolfram Sang
Cc: Kári Davíðsson, spi-devel-general, linuxppc-dev
In-Reply-To: <1256931852-13255-1-git-send-email-w.sang@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 10450 bytes --]
Hi Wolfram,
Thanks for this work. Comments below.
On Fri, Oct 30, 2009 at 1:44 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> This driver has a generic part for the probe/remove routines which probably
> used to be called from a platform driver and an of_platform driver. Meanwhile,
> the driver is of_platform only, so there is no need for the generic part
> anymore. Unifying probe/remove finally enables us to call
> of_register_spi_devices(), so spi-device nodes from the device tree will be
> parsed. This was also mentioned by:
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-June/073273.html
I haven't really been happy with any of the patches that added the
of_register_spi_devices() call, but I never got around to dealing with
it properly, or even replying. I found Jon's patch too invasive, and
the patch referenced above is a little ugly. Adding the call should
be really simple. I've drafted a patch to do only that step and
attached it to this mail. If this one works for you, then I'll merge
it immediately into -next.
> On the way, some patterns have been changed to current best practices (like
> using '!ptr' and 'struct resource'). Also, an older, yet uncommented, patch
> from me has been incorporated to improve the checks if the selected PSC is
> valid:
>
> http://patchwork.ozlabs.org/patch/36780/
There are at least 3 discrete changes here. I prefer for each to be
provided as a separate patch so I can cherry pick the ones that are
ready. I'll go back and comment on the patch you referenced above
patch right now.
Also, I'm resistant to changing the probe layout on this driver at
this time. With the work being done to generalize the OF support
code, there is a strong possibility that of_platform will be
deprecated in favor of going back to using the platform bus directly
(just like how OF support works for i2c, spi, etc). I'd rather not
refactor the driver until I'm certain of the direction that things are
going to go.
Cheers,
g.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Kári Davíðsson <kari.davidsson@marel.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> drivers/spi/mpc52xx_psc_spi.c | 110 +++++++++++++++++++---------------------
> 1 files changed, 52 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
> index 1b74d5c..3fa8c78 100644
> --- a/drivers/spi/mpc52xx_psc_spi.c
> +++ b/drivers/spi/mpc52xx_psc_spi.c
> @@ -17,6 +17,7 @@
> #include <linux/errno.h>
> #include <linux/interrupt.h>
> #include <linux/of_platform.h>
> +#include <linux/of_spi.h>
> #include <linux/workqueue.h>
> #include <linux/completion.h>
> #include <linux/io.h>
> @@ -27,6 +28,7 @@
> #include <asm/mpc52xx.h>
> #include <asm/mpc52xx_psc.h>
>
> +#define DRIVER_NAME "mpc52xx-psc-spi"
> #define MCLK 20000000 /* PSC port MClk in hz */
>
> struct mpc52xx_psc_spi {
> @@ -358,32 +360,54 @@ static irqreturn_t mpc52xx_psc_spi_isr(int irq, void *dev_id)
> return IRQ_NONE;
> }
>
> -/* bus_num is used only for the case dev->platform_data == NULL */
> -static int __init mpc52xx_psc_spi_do_probe(struct device *dev, u32 regaddr,
> - u32 size, unsigned int irq, s16 bus_num)
> +static int __init mpc52xx_psc_spi_of_probe(struct of_device *op,
> + const struct of_device_id *match)
> {
> - struct fsl_spi_platform_data *pdata = dev->platform_data;
> + struct fsl_spi_platform_data *pdata = op->dev.platform_data;
> struct mpc52xx_psc_spi *mps;
> struct spi_master *master;
> + struct resource mem;
> + s16 id = -1;
> int ret;
>
> - master = spi_alloc_master(dev, sizeof *mps);
> - if (master == NULL)
> + /* get PSC id (only 1,2,3,6 can do SPI) */
> + if (!pdata) {
> + const u32 *psc_nump;
> +
> + psc_nump = of_get_property(op->node, "cell-index", NULL);
> + if (!psc_nump || (*psc_nump > 2 && *psc_nump != 5)) {
> + dev_err(&op->dev, "PSC can't do SPI\n");
> + return -EINVAL;
> + }
> + id = *psc_nump + 1;
> + }
> +
> + ret = of_address_to_resource(op->node, 0, &mem);
> + if (ret)
> + return ret;
> +
> + master = spi_alloc_master(&op->dev, sizeof *mps);
> + if (!master)
> return -ENOMEM;
>
> - dev_set_drvdata(dev, master);
> + if (!request_mem_region(mem.start, resource_size(&mem), DRIVER_NAME)) {
> + ret = -ENOMEM;
> + goto free_master;
> + }
> +
> + dev_set_drvdata(&op->dev, master);
> mps = spi_master_get_devdata(master);
>
> /* the spi->mode bits understood by this driver: */
> master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
>
> - mps->irq = irq;
> - if (pdata == NULL) {
> - dev_warn(dev, "probe called without platform data, no "
> + mps->irq = irq_of_parse_and_map(op->node, 0);
> + if (!pdata) {
> + dev_info(&op->dev, "probe called without platform data, no "
> "cs_control function will be called\n");
> mps->cs_control = NULL;
> mps->sysclk = 0;
> - master->bus_num = bus_num;
> + master->bus_num = id;
> master->num_chipselect = 255;
> } else {
> mps->cs_control = pdata->cs_control;
> @@ -395,19 +419,18 @@ static int __init mpc52xx_psc_spi_do_probe(struct device *dev, u32 regaddr,
> master->transfer = mpc52xx_psc_spi_transfer;
> master->cleanup = mpc52xx_psc_spi_cleanup;
>
> - mps->psc = ioremap(regaddr, size);
> + mps->psc = ioremap(mem.start, resource_size(&mem));
> if (!mps->psc) {
> - dev_err(dev, "could not ioremap I/O port range\n");
> + dev_err(&op->dev, "could not ioremap I/O port range\n");
> ret = -EFAULT;
> - goto free_master;
> + goto free_unmap;
> }
> /* On the 5200, fifo regs are immediately ajacent to the psc regs */
> mps->fifo = ((void __iomem *)mps->psc) + sizeof(struct mpc52xx_psc);
>
> - ret = request_irq(mps->irq, mpc52xx_psc_spi_isr, 0, "mpc52xx-psc-spi",
> - mps);
> + ret = request_irq(mps->irq, mpc52xx_psc_spi_isr, 0, DRIVER_NAME, mps);
> if (ret)
> - goto free_master;
> + goto free_unmap;
>
> ret = mpc52xx_psc_spi_port_config(master->bus_num, mps);
> if (ret < 0)
> @@ -429,24 +452,29 @@ static int __init mpc52xx_psc_spi_do_probe(struct device *dev, u32 regaddr,
> if (ret < 0)
> goto unreg_master;
>
> + of_register_spi_devices(master, op->node);
> +
> return ret;
>
> unreg_master:
> destroy_workqueue(mps->workqueue);
> free_irq:
> free_irq(mps->irq, mps);
> -free_master:
> +free_unmap:
> if (mps->psc)
> iounmap(mps->psc);
> + release_mem_region(mem.start, resource_size(&mem));
> +free_master:
> spi_master_put(master);
>
> return ret;
> }
>
> -static int __exit mpc52xx_psc_spi_do_remove(struct device *dev)
> +static int __exit mpc52xx_psc_spi_of_remove(struct of_device *op)
> {
> - struct spi_master *master = dev_get_drvdata(dev);
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> struct mpc52xx_psc_spi *mps = spi_master_get_devdata(master);
> + struct resource mem;
>
> flush_workqueue(mps->workqueue);
> destroy_workqueue(mps->workqueue);
> @@ -454,46 +482,12 @@ static int __exit mpc52xx_psc_spi_do_remove(struct device *dev)
> free_irq(mps->irq, mps);
> if (mps->psc)
> iounmap(mps->psc);
> + if (of_address_to_resource(op->node, 0, &mem) == 0)
> + release_mem_region(mem.start, resource_size(&mem));
>
> return 0;
> }
>
> -static int __init mpc52xx_psc_spi_of_probe(struct of_device *op,
> - const struct of_device_id *match)
> -{
> - const u32 *regaddr_p;
> - u64 regaddr64, size64;
> - s16 id = -1;
> -
> - regaddr_p = of_get_address(op->node, 0, &size64, NULL);
> - if (!regaddr_p) {
> - printk(KERN_ERR "Invalid PSC address\n");
> - return -EINVAL;
> - }
> - regaddr64 = of_translate_address(op->node, regaddr_p);
> -
> - /* get PSC id (1..6, used by port_config) */
> - if (op->dev.platform_data == NULL) {
> - const u32 *psc_nump;
> -
> - psc_nump = of_get_property(op->node, "cell-index", NULL);
> - if (!psc_nump || *psc_nump > 5) {
> - printk(KERN_ERR "mpc52xx_psc_spi: Device node %s has invalid "
> - "cell-index property\n", op->node->full_name);
> - return -EINVAL;
> - }
> - id = *psc_nump + 1;
> - }
> -
> - return mpc52xx_psc_spi_do_probe(&op->dev, (u32)regaddr64, (u32)size64,
> - irq_of_parse_and_map(op->node, 0), id);
> -}
> -
> -static int __exit mpc52xx_psc_spi_of_remove(struct of_device *op)
> -{
> - return mpc52xx_psc_spi_do_remove(&op->dev);
> -}
> -
> static struct of_device_id mpc52xx_psc_spi_of_match[] = {
> { .compatible = "fsl,mpc5200-psc-spi", },
> { .compatible = "mpc5200-psc-spi", }, /* old */
> @@ -504,12 +498,12 @@ MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
>
> static struct of_platform_driver mpc52xx_psc_spi_of_driver = {
> .owner = THIS_MODULE,
> - .name = "mpc52xx-psc-spi",
> + .name = DRIVER_NAME,
> .match_table = mpc52xx_psc_spi_of_match,
> .probe = mpc52xx_psc_spi_of_probe,
> .remove = __exit_p(mpc52xx_psc_spi_of_remove),
> .driver = {
> - .name = "mpc52xx-psc-spi",
> + .name = DRIVER_NAME,
> .owner = THIS_MODULE,
> },
> };
> --
> 1.6.3.3
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[-- Attachment #2: 0001-spi-mpc5200-Register-SPI-devices-described-in-device.patch --]
[-- Type: text/x-patch, Size: 1601 bytes --]
From 7629d40dc343ff216b752d5c68654dc9d30f0c91 Mon Sep 17 00:00:00 2001
From: Grant Likely <grant.likely@secretlab.ca>
Date: Sat, 31 Oct 2009 17:49:38 -0600
Subject: [PATCH] spi/mpc5200: Register SPI devices described in device tree
Add call to of_register_spi_devices() to register SPI devices described
in the OF device tree.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
drivers/spi/mpc52xx_psc_spi.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.c
index 1b74d5c..b445464 100644
--- a/drivers/spi/mpc52xx_psc_spi.c
+++ b/drivers/spi/mpc52xx_psc_spi.c
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/interrupt.h>
#include <linux/of_platform.h>
+#include <linux/of_spi.h>
#include <linux/workqueue.h>
#include <linux/completion.h>
#include <linux/io.h>
@@ -464,6 +465,7 @@ static int __init mpc52xx_psc_spi_of_probe(struct of_device *op,
const u32 *regaddr_p;
u64 regaddr64, size64;
s16 id = -1;
+ int rc;
regaddr_p = of_get_address(op->node, 0, &size64, NULL);
if (!regaddr_p) {
@@ -485,8 +487,12 @@ static int __init mpc52xx_psc_spi_of_probe(struct of_device *op,
id = *psc_nump + 1;
}
- return mpc52xx_psc_spi_do_probe(&op->dev, (u32)regaddr64, (u32)size64,
+ rc = mpc52xx_psc_spi_do_probe(&op->dev, (u32)regaddr64, (u32)size64,
irq_of_parse_and_map(op->node, 0), id);
+ if (!rc)
+ of_register_spi_devices(dev_get_drvdata(&op->dev), op->node);
+
+ return rc;
}
static int __exit mpc52xx_psc_spi_of_remove(struct of_device *op)
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] spi/mpc52xx: check for invalid PSC usage
From: Grant Likely @ 2009-11-01 1:22 UTC (permalink / raw)
To: Wolfram Sang; +Cc: spi-devel-general, David Brownell, linuxppc-dev
In-Reply-To: <1256297157-28246-1-git-send-email-w.sang@pengutronix.de>
On Fri, Oct 23, 2009 at 5:25 AM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:
> Add checks to allow only PSCs capable of SPI.
> Also turn printk to dev_err while we are here.
Hi Wolfram,
I wouldn't even bother. It's not actively dangerous to try and use
PSC{4,5} in SPI mode. It just not going to work. Besides, the
MPC5200 common code already checks for an invalid PSC number when
setting the clock divisor.
Have you seen cases of users trying to do the wrong thing with the
crippled PSCs?
g.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> ---
>
> Grant, if the patch is OK, maybe it can also go via your tree?
>
> =A0drivers/spi/mpc52xx_psc_spi.c | =A0 =A09 ++++-----
> =A01 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/mpc52xx_psc_spi.c b/drivers/spi/mpc52xx_psc_spi.=
c
> index 1b74d5c..1d11a6d 100644
> --- a/drivers/spi/mpc52xx_psc_spi.c
> +++ b/drivers/spi/mpc52xx_psc_spi.c
> @@ -467,19 +467,18 @@ static int __init mpc52xx_psc_spi_of_probe(struct o=
f_device *op,
>
> =A0 =A0 =A0 =A0regaddr_p =3D of_get_address(op->node, 0, &size64, NULL);
> =A0 =A0 =A0 =A0if (!regaddr_p) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "Invalid PSC address\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "Invalid PSC address\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0regaddr64 =3D of_translate_address(op->node, regaddr_p);
>
> - =A0 =A0 =A0 /* get PSC id (1..6, used by port_config) */
> + =A0 =A0 =A0 /* get PSC id (only 1,2,3,6 can do SPI) */
> =A0 =A0 =A0 =A0if (op->dev.platform_data =3D=3D NULL) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const u32 *psc_nump;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0psc_nump =3D of_get_property(op->node, "ce=
ll-index", NULL);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!psc_nump || *psc_nump > 5) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR "mpc52xx_ps=
c_spi: Device node %s has invalid "
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 "cell-index property\n", op->node->full_name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!psc_nump || (*psc_nump > 2 && *psc_num=
p !=3D 5)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "PSC can'=
t do SPI\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0id =3D *psc_nump + 1;
> --
> 1.6.3.3
>
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable
From: Grant Likely @ 2009-11-01 6:47 UTC (permalink / raw)
To: Albrecht Dreß; +Cc: Linux PPC Development
In-Reply-To: <1254771740.5503.2@antares>
On Mon, Oct 5, 2009 at 1:42 PM, Albrecht Dre=DF <albrecht.dress@arcor.de> w=
rote:
> Am 05.10.09 20:16 schrieb(en) Grant Likely:
>>
>> Hmmm. =A0I've not been comfortable with this change, but it took me a wh=
ile
>> to put my finger on exactly why. =A0In principle, I think it is a good i=
dea.
>> =A0However, I don't want to merge it without any in-tree users.
>
> I could provide my modified microcode (which differs in two words only), =
for
> the "standard" task, just with byte swapping for the LPB, but I wonder if=
it
> is of much use. =A0No progress with crc32 calculation in BestComm so far.=
..
> :-(
>
>> The other concern is that I don't like that the patch is only applicable
>> to gen_bd tasks.
>
> IMHO the big difference between gen_bd on one hand and ATA and FEC on the
> other is that (afaik) we have *no* other choice than using the implemente=
d
> tasks for the latter two, obviously using the Freescale's example microco=
de.
> =A0Gen_bd can apparently be used for a plenty of purposes, including LPB =
(as
> shown in your driver), maybe PCI (there are examples in Freescale's code)=
,
> PSC, etc. =A0Thus, the FEC and ATA drivers should not have an option as t=
o
> change the microcode, whereas it is just convenient for gen_bd.
agreed.
>> I've been thinking about this for a while, and I'm not exactly thrilled
>> with the bestcomm layout which keeps the bestcomm firmware separate from=
the
>> only driver that actually uses it (ie FEC firmware & support code is
>> separate from the FEC driver. =A0Same for ATA). =A0I don't like the fact=
that
>> code which is only ever used by the ATA driver is maintained completely
>> separate from it.
>
> That's a good point! =A0The separate microcode files for ATA and FEC are
> somewhat confusing. =A0I guess it's a result of simply copying the Freesc=
ale
> examples into separate files (and that might be a good reason for keeping
> the microcode separated). =A0Maybe create new sub-folders below
> arch/powerpc/sysdev/bestcomm for fec, ata, and gen_bd? =A0At least some
> documentation would be great.
No, that doesn't really help much. The ATA and FEC microcode +
support functions really have no business living in the common
bestcomm directory at all. I should look into moving that stuff right
into the FEC and ATA drivers. The Bestcomm API is probably designed
to the wrong level, and the base API should expose the same task
loading interface to all uses, FEC, ATA, GenBD, and custom tasks.
>> But I may be splitting hairs here and maybe I shouldn't be too concerned
>> about it. =A0Regardless, I'd think I'd rather see something that isn't t=
otally
>> gen_bd specific. =A0Perhaps *microcode should be part of the bcom_task
>> structure?
>
> See above - I believe FEC and ATA are somewhat special as they need
> *exactly* the provided microcode. =A0And if something cannot be changed
> without breaking the functionality, there should not be a chance to do so=
...
> ;-)
Hence the FEC and ATA microcode should live in (*gasp*) the FEC and
ATA drivers. ;-)
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/40x: Add new PPC440EPx based board HCU5 of Netstal Maschinen
From: Grant Likely @ 2009-11-01 7:12 UTC (permalink / raw)
To: Niklaus Giger; +Cc: linuxppc-dev, niklaus.giger
In-Reply-To: <1255075865-4025-2-git-send-email-niklaus.giger@member.fsf.org>
On Fri, Oct 9, 2009 at 2:11 AM, Niklaus Giger
<niklaus.giger@member.fsf.org> wrote:
> Adds support for a HCU5 PPC405EPx based board from Netstal Maschinen AG.
>
> Signed-off-by: Niklaus Giger <niklaus.giger@member.fsf.org>
> ---
> =A0arch/powerpc/boot/dts/hcu5.dts =A0 =A0 =A0 =A0 =A0| =A0254 +++++++
> =A0arch/powerpc/configs/44x/hcu5_defconfig | 1166 +++++++++++++++++++++++=
++++++++
Do you really need your own defconfig? Can you instead add support
for your board to ppc44x_defconfig? Josh is the maintainer here, so
it is his decision, but on 5200 stuff I've been pushing back on adding
yet-another-board-specific-defconfig when there isn't a really
compelling reason to do so. It adds a lot of lines for not a lot of
benefit. Besides, if you add your board to the ppc44x_defconfig, you
gain the advantage of your code getting compiled by others more
frequently.
g.
^ permalink raw reply
* Fail building 2.6.31.x for CHRP using gcc-4.4.x
From: Giuseppe Coviello @ 2009-11-01 9:17 UTC (permalink / raw)
To: linuxppc-dev
Hi, we are having trouble while building kernel 2.6.31.x and also
2.6.32rc for CHRP platforms using gcc-4.4.x, the error that give us
the compilation is:
CC arch/powerpc/sysdev/indirect_pci.o
CC arch/powerpc/sysdev/i8259.o
LD arch/powerpc/sysdev/built-in.o
CC arch/powerpc/platforms/chrp/setup.o
cc1: warnings being treated as errors
arch/powerpc/platforms/chrp/setup.c: In function 'chrp_event_scan':
arch/powerpc/platforms/chrp/setup.c:378: error: the frame size of 1040
bytes is larger than 1024 bytes
make[2]: *** [arch/powerpc/platforms/chrp/setup.o] Error 1
make[1]: *** [arch/powerpc/platforms/chrp] Error 2
make: *** [arch/powerpc/platforms] Error
The same kernel, with the same configuration builds fine using gcc-4.3.x.
Regards, Giuseppe
^ permalink raw reply
* Re: Accessing flash directly from User Space [SOLVED]
From: Segher Boessenkool @ 2009-11-01 10:07 UTC (permalink / raw)
To: Joakim Tjernlund
Cc: bgat@billgatliff.com, Alessandro Rubini, Jonathan Haws,
scottwood@freescale.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <OFA2803682.B247ED64-ONC1257660.007B44F5-C1257660.007BB531@transmode.se>
>>>> mmio[0] = address;
>>>> mmio[1] = data;
>>>> mb();
eieio is enough here.
>>>> mmio[3] |= 0x01; /* This triggers an operation -> address=data */
>>>> /* probably also need an mb() here, if the following code
>>>> * depends on the operation to be triggered. */
No, a sync does not guarantee the device has seen the store yet;
you need something specific to the device to guarantee this.
Usually a load (from the same register!) followed by code that
makes sure the load has finished is sufficient (and necessary).
>>> hmm, the mmio[0] and mmio[1] are written in order I hope?
>>
>> We do not care in this example, as the write to [3] does trigger
>> the device operation. We do only care that [0] and [1] are set
>> when [3] is written. We do not care in what order [0] and [1] are
>> written.
>
> In this example yes, I was wondering in general.
The writes will not be reordered, but they can be combined,
unless you put an eieio (or sync) inbetween.
> So what does guarded memory mapping on ppc mean really? If I need
> that much mb(), guarded does not seem to do much.
Loosely speaking, guarded means no prefetch is done.
Segher
^ permalink raw reply
* Re: Fail building 2.6.31.x for CHRP using gcc-4.4.x
From: Stephen Rothwell @ 2009-11-01 11:41 UTC (permalink / raw)
To: Giuseppe Coviello; +Cc: linuxppc-dev
In-Reply-To: <a265241c0911010117m65335b80g94457a07d0610cbe@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]
Hi Giuseppe,
On Sun, 1 Nov 2009 10:17:00 +0100 Giuseppe Coviello <cjg@cruxppc.org> wrote:
>
> Hi, we are having trouble while building kernel 2.6.31.x and also
> 2.6.32rc for CHRP platforms using gcc-4.4.x, the error that give us
> the compilation is:
>
> CC arch/powerpc/sysdev/indirect_pci.o
> CC arch/powerpc/sysdev/i8259.o
> LD arch/powerpc/sysdev/built-in.o
> CC arch/powerpc/platforms/chrp/setup.o
> cc1: warnings being treated as errors
> arch/powerpc/platforms/chrp/setup.c: In function 'chrp_event_scan':
> arch/powerpc/platforms/chrp/setup.c:378: error: the frame size of 1040
> bytes is larger than 1024 bytes
> make[2]: *** [arch/powerpc/platforms/chrp/setup.o] Error 1
> make[1]: *** [arch/powerpc/platforms/chrp] Error 2
> make: *** [arch/powerpc/platforms] Error
>
> The same kernel, with the same configuration builds fine using gcc-4.3.x.
For now, just enable CONFIG_PPC_DISABLE_WERROR.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 08/27] Add SLB switching code for entry/exit
From: Michael Neuling @ 2009-11-01 23:23 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Arnd Bergmann, Hollis Blanchard, Marcelo Tosatti,
kvm-ppc, linuxppc-dev, Avi Kivity, kvm, bphilips, Olof Johansson
In-Reply-To: <1256917647-6200-9-git-send-email-agraf@suse.de>
> This is the really low level of guest entry/exit code.
>
> Book3s_64 has an SLB, which stores all ESID -> VSID mappings we're
> currently aware of.
>
> The segments in the guest differ from the ones on the host, so we need
> to switch the SLB to tell the MMU that we're in a new context.
>
> So we store a shadow of the guest's SLB in the PACA, switch to that on
> entry and only restore bolted entries on exit, leaving the rest to the
> Linux SLB fault handler.
>
> That way we get a really clean way of switching the SLB.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> arch/powerpc/kvm/book3s_64_slb.S | 277 ++++++++++++++++++++++++++++++++++++
++
> 1 files changed, 277 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/kvm/book3s_64_slb.S
>
> diff --git a/arch/powerpc/kvm/book3s_64_slb.S b/arch/powerpc/kvm/book3s_64_sl
b.S
> new file mode 100644
> index 0000000..00a8367
> --- /dev/null
> +++ b/arch/powerpc/kvm/book3s_64_slb.S
> @@ -0,0 +1,277 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + * Copyright SUSE Linux Products GmbH 2009
> + *
> + * Authors: Alexander Graf <agraf@suse.de>
> + */
> +
> +/***************************************************************************
***
> + *
*
> + * Entry code
*
> + *
*
> + ***************************************************************************
**/
> +
> +.global kvmppc_handler_trampoline_enter
> +kvmppc_handler_trampoline_enter:
> +
> + /* Required state:
> + *
> + * MSR = ~IR|DR
> + * R13 = PACA
> + * R9 = guest IP
> + * R10 = guest MSR
> + * R11 = free
> + * R12 = free
> + * PACA[PACA_EXMC + EX_R9] = guest R9
> + * PACA[PACA_EXMC + EX_R10] = guest R10
> + * PACA[PACA_EXMC + EX_R11] = guest R11
> + * PACA[PACA_EXMC + EX_R12] = guest R12
> + * PACA[PACA_EXMC + EX_R13] = guest R13
> + * PACA[PACA_EXMC + EX_CCR] = guest CR
> + * PACA[PACA_EXMC + EX_R3] = guest XER
> + */
> +
> + mtsrr0 r9
> + mtsrr1 r10
> +
> + mtspr SPRN_SPRG_SCRATCH0, r0
> +
> + /* Remove LPAR shadow entries */
> +
> +#if SLB_NUM_BOLTED == 3
You could alternatively check the persistent entry in the slb_shawdow
buffer. This would give you a run time check. Not sure what's best
though.
> +
> + ld r12, PACA_SLBSHADOWPTR(r13)
> + ld r10, 0x10(r12)
> + ld r11, 0x18(r12)
Can you define something in asm-offsets.c for these magic constants 0x10
and 0x18. Similarly below.
> + /* Invalid? Skip. */
> + rldicl. r0, r10, 37, 63
> + beq slb_entry_skip_1
> + xoris r9, r10, SLB_ESID_V@h
> + std r9, 0x10(r12)
> +slb_entry_skip_1:
> + ld r9, 0x20(r12)
> + /* Invalid? Skip. */
> + rldicl. r0, r9, 37, 63
> + beq slb_entry_skip_2
> + xoris r9, r9, SLB_ESID_V@h
> + std r9, 0x20(r12)
> +slb_entry_skip_2:
> + ld r9, 0x30(r12)
> + /* Invalid? Skip. */
> + rldicl. r0, r9, 37, 63
> + beq slb_entry_skip_3
> + xoris r9, r9, SLB_ESID_V@h
> + std r9, 0x30(r12)
Can these 3 be made into a macro?
> +slb_entry_skip_3:
> +
> +#else
> +#error unknown number of bolted entries
> +#endif
> +
> + /* Flush SLB */
> +
> + slbia
> +
> + /* r0 = esid & ESID_MASK */
> + rldicr r10, r10, 0, 35
> + /* r0 |= CLASS_BIT(VSID) */
> + rldic r12, r11, 56 - 36, 36
> + or r10, r10, r12
> + slbie r10
> +
> + isync
> +
> + /* Fill SLB with our shadow */
> +
> + lbz r12, PACA_KVM_SLB_MAX(r13)
> + mulli r12, r12, 16
> + addi r12, r12, PACA_KVM_SLB
> + add r12, r12, r13
> +
> + /* for (r11 = kvm_slb; r11 < kvm_slb + kvm_slb_size; r11+=slb_entry) */
> + li r11, PACA_KVM_SLB
> + add r11, r11, r13
> +
> +slb_loop_enter:
> +
> + ld r10, 0(r11)
> +
> + rldicl. r0, r10, 37, 63
> + beq slb_loop_enter_skip
> +
> + ld r9, 8(r11)
> + slbmte r9, r10
If you're updating the first 3 slbs, you need to make sure the slb
shadow is updated at the same time (BTW dumb question: can we run this
under PHYP?)
> +
> +slb_loop_enter_skip:
> + addi r11, r11, 16
> + cmpd cr0, r11, r12
> + blt slb_loop_enter
> +
> +slb_do_enter:
> +
> + /* Enter guest */
> +
> + mfspr r0, SPRN_SPRG_SCRATCH0
> +
> + ld r9, (PACA_EXMC+EX_R9)(r13)
> + ld r10, (PACA_EXMC+EX_R10)(r13)
> + ld r12, (PACA_EXMC+EX_R12)(r13)
> +
> + lwz r11, (PACA_EXMC+EX_CCR)(r13)
> + mtcr r11
> +
> + ld r11, (PACA_EXMC+EX_R3)(r13)
> + mtxer r11
> +
> + ld r11, (PACA_EXMC+EX_R11)(r13)
> + ld r13, (PACA_EXMC+EX_R13)(r13)
> +
> + RFI
> +kvmppc_handler_trampoline_enter_end:
> +
> +
> +
> +/***************************************************************************
***
> + *
*
> + * Exit code
*
> + *
*
> + ***************************************************************************
**/
> +
> +.global kvmppc_handler_trampoline_exit
> +kvmppc_handler_trampoline_exit:
> +
> + /* Register usage at this point:
> + *
> + * SPRG_SCRATCH0 = guest R13
> + * R01 = host R1
> + * R02 = host R2
> + * R10 = guest PC
> + * R11 = guest MSR
> + * R12 = exit handler id
> + * R13 = PACA
> + * PACA.exmc.CCR = guest CR
> + * PACA.exmc.R9 = guest R1
> + * PACA.exmc.R10 = guest R10
> + * PACA.exmc.R11 = guest R11
> + * PACA.exmc.R12 = guest R12
> + * PACA.exmc.R13 = guest R2
> + *
> + */
> +
> + /* Save registers */
> +
> + std r0, (PACA_EXMC+EX_SRR0)(r13)
> + std r9, (PACA_EXMC+EX_R3)(r13)
> + std r10, (PACA_EXMC+EX_LR)(r13)
> + std r11, (PACA_EXMC+EX_DAR)(r13)
> +
> + /*
> + * In order for us to easily get the last instruction,
> + * we got the #vmexit at, we exploit the fact that the
> + * virtual layout is still the same here, so we can just
> + * ld from the guest's PC address
> + */
> +
> + /* We only load the last instruction when it's safe */
> + cmpwi r12, BOOK3S_INTERRUPT_DATA_STORAGE
> + beq ld_last_inst
> + cmpwi r12, BOOK3S_INTERRUPT_PROGRAM
> + beq ld_last_inst
> +
> + b no_ld_last_inst
> +
> +ld_last_inst:
> + /* Save off the guest instruction we're at */
> + /* 1) enable paging for data */
> + mfmsr r9
> + ori r11, r9, MSR_DR /* Enable paging for data */
> + mtmsr r11
> + /* 2) fetch the instruction */
> + lwz r0, 0(r10)
> + /* 3) disable paging again */
> + mtmsr r9
> +
> +no_ld_last_inst:
> +
> + /* Restore bolted entries from the shadow and fix it along the way */
> +
> + /* We don't store anything in entry 0, so we don't need to take care of
that */
> + slbia
> + isync
> +
> +#if SLB_NUM_BOLTED == 3
> +
> + ld r11, PACA_SLBSHADOWPTR(r13)
> +
> + ld r10, 0x10(r11)
> + cmpdi r10, 0
> + beq slb_exit_skip_1
> + oris r10, r10, SLB_ESID_V@h
> + ld r9, 0x18(r11)
> + slbmte r9, r10
> + std r10, 0x10(r11)
> +slb_exit_skip_1:
> +
> + ld r10, 0x20(r11)
> + cmpdi r10, 0
> + beq slb_exit_skip_2
> + oris r10, r10, SLB_ESID_V@h
> + ld r9, 0x28(r11)
> + slbmte r9, r10
> + std r10, 0x20(r11)
> +slb_exit_skip_2:
> +
> + ld r10, 0x30(r11)
> + cmpdi r10, 0
> + beq slb_exit_skip_3
> + oris r10, r10, SLB_ESID_V@h
> + ld r9, 0x38(r11)
> + slbmte r9, r10
> + std r10, 0x30(r11)
> +slb_exit_skip_3:
Again, a macro here?
> +
> +#else
> +#error unknown number of bolted entries
> +#endif
> +
> +slb_do_exit:
> +
> + /* Restore registers */
> +
> + ld r11, (PACA_EXMC+EX_DAR)(r13)
> + ld r10, (PACA_EXMC+EX_LR)(r13)
> + ld r9, (PACA_EXMC+EX_R3)(r13)
> +
> + /* Save last inst */
> + stw r0, (PACA_EXMC+EX_LR)(r13)
> +
> + /* Save DAR and DSISR before going to paged mode */
> + mfdar r0
> + std r0, (PACA_EXMC+EX_DAR)(r13)
> + mfdsisr r0
> + stw r0, (PACA_EXMC+EX_DSISR)(r13)
> +
> + /* RFI into the highmem handler */
> + mfmsr r0
> + ori r0, r0, MSR_IR|MSR_DR|MSR_RI /* Enable paging */
> + mtsrr1 r0
> + ld r0, PACASAVEDMSR(r13) /* Highmem handler address */
> + mtsrr0 r0
> +
> + mfspr r0, SPRN_SPRG_SCRATCH0
> +
> + RFI
> +kvmppc_handler_trampoline_exit_end:
> +
> --
> 1.6.0.2
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply
* Re: [PATCH 11/27] Add book3s_64 Host MMU handling
From: Michael Neuling @ 2009-11-01 23:39 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Arnd Bergmann, Hollis Blanchard, Marcelo Tosatti,
kvm-ppc, linuxppc-dev, Avi Kivity, kvm, bphilips, Olof Johansson
In-Reply-To: <1256917647-6200-12-git-send-email-agraf@suse.de>
<snip>
> +static void invalidate_pte(struct hpte_cache *pte)
> +{
> + dprintk_mmu("KVM: Flushing SPT %d: 0x%llx (0x%llx) -> 0x%llx\n",
> + i, pte->pte.eaddr, pte->pte.vpage, pte->host_va);
> +
> + ppc_md.hpte_invalidate(pte->slot, pte->host_va,
> + MMU_PAGE_4K, MMU_SEGSIZE_256M,
> + false);
Are we assuming 256M segments here (and elsewhere)?
<snip>
> +static int kvmppc_mmu_next_segment(struct kvm_vcpu *vcpu, ulong esid)
> +{
> + int i;
> + int max_slb_size = 64;
> + int found_inval = -1;
> + int r;
> +
> + if (!get_paca()->kvm_slb_max)
> + get_paca()->kvm_slb_max = 1;
> +
> + /* Are we overwriting? */
> + for (i = 1; i < get_paca()->kvm_slb_max; i++) {
> + if (!(get_paca()->kvm_slb[i].esid & SLB_ESID_V))
> + found_inval = i;
> + else if ((get_paca()->kvm_slb[i].esid & ESID_MASK) == esid)
> + return i;
> + }
> +
> + /* Found a spare entry that was invalidated before */
> + if (found_inval > 0)
> + return found_inval;
> +
> + /* No spare invalid entry, so create one */
> +
> + if (mmu_slb_size < 64)
> + max_slb_size = mmu_slb_size;
Can we just use the global mmu_slb_size eliminate max_slb_size?
<snip>
Mikey
^ permalink raw reply
* [PATCH] powerpc: Avoid giving out RTC dates below EPOCH
From: Benjamin Herrenschmidt @ 2009-11-02 5:11 UTC (permalink / raw)
To: linuxppc-dev
Doing so causes xtime to be negative which crashes the timekeeping
code in funny ways when doing suspend/resume
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 92dc844..6a7ce0e 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -777,7 +777,7 @@ int update_persistent_clock(struct timespec now)
return ppc_md.set_rtc_time(&tm);
}
-void read_persistent_clock(struct timespec *ts)
+static void __read_persistent_clock(struct timespec *ts)
{
struct rtc_time tm;
static int first = 1;
@@ -800,10 +800,23 @@ void read_persistent_clock(struct timespec *ts)
return;
}
ppc_md.get_rtc_time(&tm);
+
ts->tv_sec = mktime(tm.tm_year+1900, tm.tm_mon+1, tm.tm_mday,
tm.tm_hour, tm.tm_min, tm.tm_sec);
}
+void read_persistent_clock(struct timespec *ts)
+{
+ __read_persistent_clock(&ts);
+
+ /* Sanitize it in case real time clock is set below EPOCH */
+ if (ts->tv_sec < 0) {
+ ts->tv_sec = 0;
+ ts->tv_nsec = 0;
+ }
+
+}
+
/* clocksource code */
static cycle_t rtc_read(struct clocksource *cs)
{
^ permalink raw reply related
* Re: [PATCH] powerpc: Avoid giving out RTC dates below EPOCH
From: Benjamin Herrenschmidt @ 2009-11-02 5:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1257138663.7907.32.camel@pasglop>
On Mon, 2009-11-02 at 16:11 +1100, Benjamin Herrenschmidt wrote:
> Doing so causes xtime to be negative which crashes the timekeeping
> code in funny ways when doing suspend/resume
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> +void read_persistent_clock(struct timespec *ts)
> +{
> + __read_persistent_clock(&ts);
Should read
+ __read_persistent_clock(ts);
Forgot a quilt ref ;-)
Cheers,
Ben.
> + /* Sanitize it in case real time clock is set below EPOCH */
> + if (ts->tv_sec < 0) {
> + ts->tv_sec = 0;
> + ts->tv_nsec = 0;
> + }
> +
> +}
> +
> /* clocksource code */
> static cycle_t rtc_read(struct clocksource *cs)
> {
>
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ 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