| Message ID | 1360336339-10703-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Deferred, archived |
| Delegated to: | Tom Rini |
| Headers | show |
On 02/08/2013 09:11:57 AM, Simon Glass wrote: > These are available on other architectures, so add them on ppc. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None > > arch/powerpc/include/asm/io.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/powerpc/include/asm/io.h > b/arch/powerpc/include/asm/io.h > index 1f12c29..1bf12f5 100644 > --- a/arch/powerpc/include/asm/io.h > +++ b/arch/powerpc/include/asm/io.h > @@ -317,4 +317,12 @@ static inline phys_addr_t virt_to_phys(void * > vaddr) > #endif > } > > +/* > + * TODO: The kernel offers some more advanced versions of barriers, > it might > + * have some advantages to use them instead of the simple one here. > + */ > +#define dmb() __asm__ __volatile__ ("" : : : "memory") > +#define __iormb() dmb() > +#define __iowmb() dmb() What is the definition of these? Given that we already have an architecture-independent barrier(), I assume this is meant to be an actual hardware barrier rather than a compiler barrier, so this is not a correct implementation. -Scott
Hi Scott, On Mon, Feb 11, 2013 at 11:52 AM, Scott Wood <scottwood@freescale.com> wrote: > On 02/08/2013 09:11:57 AM, Simon Glass wrote: >> >> These are available on other architectures, so add them on ppc. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: None >> >> arch/powerpc/include/asm/io.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h >> index 1f12c29..1bf12f5 100644 >> --- a/arch/powerpc/include/asm/io.h >> +++ b/arch/powerpc/include/asm/io.h >> @@ -317,4 +317,12 @@ static inline phys_addr_t virt_to_phys(void * vaddr) >> #endif >> } >> >> +/* >> + * TODO: The kernel offers some more advanced versions of barriers, it >> might >> + * have some advantages to use them instead of the simple one here. >> + */ >> +#define dmb() __asm__ __volatile__ ("" : : : "memory") >> +#define __iormb() dmb() >> +#define __iowmb() dmb() > > > What is the definition of these? Given that we already have an > architecture-independent barrier(), I assume this is meant to be an actual > hardware barrier rather than a compiler barrier, so this is not a correct > implementation. They were introduced in ARM in commit 3c0659b5, so I am really just following along with that. Yes the naming doesn't make a lot of sense, but on the other hand I don't think we necessarily want an actual hardware barrier in our writel()s. This at least makes sure that the compiler writes in the right order - perhaps the intent is that that rest is best left to the hardware? Regards, Simon > > -Scott
On 02/12/2013 12:44:16 PM, Simon Glass wrote: > Hi Scott, > > On Mon, Feb 11, 2013 at 11:52 AM, Scott Wood > <scottwood@freescale.com> wrote: > > On 02/08/2013 09:11:57 AM, Simon Glass wrote: > >> > >> These are available on other architectures, so add them on ppc. > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> --- > >> Changes in v5: None > >> Changes in v4: None > >> Changes in v3: None > >> Changes in v2: None > >> > >> arch/powerpc/include/asm/io.h | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/arch/powerpc/include/asm/io.h > b/arch/powerpc/include/asm/io.h > >> index 1f12c29..1bf12f5 100644 > >> --- a/arch/powerpc/include/asm/io.h > >> +++ b/arch/powerpc/include/asm/io.h > >> @@ -317,4 +317,12 @@ static inline phys_addr_t virt_to_phys(void * > vaddr) > >> #endif > >> } > >> > >> +/* > >> + * TODO: The kernel offers some more advanced versions of > barriers, it > >> might > >> + * have some advantages to use them instead of the simple one > here. > >> + */ > >> +#define dmb() __asm__ __volatile__ ("" : : : "memory") > >> +#define __iormb() dmb() > >> +#define __iowmb() dmb() > > > > > > What is the definition of these? Given that we already have an > > architecture-independent barrier(), I assume this is meant to be an > actual > > hardware barrier rather than a compiler barrier, so this is not a > correct > > implementation. > > They were introduced in ARM in commit 3c0659b5, so I am really just > following along with that. Yes the naming doesn't make a lot of sense, > but on the other hand I don't think we necessarily want an actual > hardware barrier in our writel()s. We do have a hardware barrier in writel() on PPC (ignoring the broken never-used little-endian implementation, which should just be removed), and it should stay that way. I do not think we should be introducing anything that looks like a hardware barrier but isn't, unless the semantics of a particular barrier are guaranteed by a particular platform without needing a barrier instruction. And in that case there had better be a document somewhere that explains what the semantics are. > This at least makes sure that the > compiler writes in the right order - perhaps the intent is that that > rest is best left to the hardware? Regardless of what one might think is "best", it isn't left to hardware on many platforms, including PPC. -Scott
Hi Scott, On Tue, Feb 12, 2013 at 10:50 AM, Scott Wood <scottwood@freescale.com> wrote: > On 02/12/2013 12:44:16 PM, Simon Glass wrote: >> >> Hi Scott, >> >> On Mon, Feb 11, 2013 at 11:52 AM, Scott Wood <scottwood@freescale.com> >> wrote: >> > On 02/08/2013 09:11:57 AM, Simon Glass wrote: >> >> >> >> These are available on other architectures, so add them on ppc. >> >> >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> --- >> >> Changes in v5: None >> >> Changes in v4: None >> >> Changes in v3: None >> >> Changes in v2: None >> >> >> >> arch/powerpc/include/asm/io.h | 8 ++++++++ >> >> 1 file changed, 8 insertions(+) >> >> >> >> diff --git a/arch/powerpc/include/asm/io.h >> >> b/arch/powerpc/include/asm/io.h >> >> index 1f12c29..1bf12f5 100644 >> >> --- a/arch/powerpc/include/asm/io.h >> >> +++ b/arch/powerpc/include/asm/io.h >> >> @@ -317,4 +317,12 @@ static inline phys_addr_t virt_to_phys(void * >> >> vaddr) >> >> #endif >> >> } >> >> >> >> +/* >> >> + * TODO: The kernel offers some more advanced versions of barriers, it >> >> might >> >> + * have some advantages to use them instead of the simple one here. >> >> + */ >> >> +#define dmb() __asm__ __volatile__ ("" : : : "memory") >> >> +#define __iormb() dmb() >> >> +#define __iowmb() dmb() >> > >> > >> > What is the definition of these? Given that we already have an >> > architecture-independent barrier(), I assume this is meant to be an >> > actual >> > hardware barrier rather than a compiler barrier, so this is not a >> > correct >> > implementation. >> >> They were introduced in ARM in commit 3c0659b5, so I am really just >> following along with that. Yes the naming doesn't make a lot of sense, >> but on the other hand I don't think we necessarily want an actual >> hardware barrier in our writel()s. > > > We do have a hardware barrier in writel() on PPC (ignoring the broken > never-used little-endian implementation, which should just be removed), and > it should stay that way. > > I do not think we should be introducing anything that looks like a hardware > barrier but isn't, unless the semantics of a particular barrier are > guaranteed by a particular platform without needing a barrier instruction. > And in that case there had better be a document somewhere that explains what > the semantics are. Yes, agreed. > > >> This at least makes sure that the >> compiler writes in the right order - perhaps the intent is that that >> rest is best left to the hardware? > > > Regardless of what one might think is "best", it isn't left to hardware on > many platforms, including PPC. I really mean that my reading is that this is all that is needed in writel() and friends. In order words, we don't need to tell the hardware to strongly order, or whatever. I think I can just drop this patch and things will still build. Can you please take a look at this code too, from the patch which brings in post-relocation board init (board_r.c): #ifdef CONFIG_PPC /* TODO: Can we not use dmb() macros for this? */ asm("sync ; isync"); #endif Regards, Simon > > -Scott
On 02/12/2013 12:55:39 PM, Simon Glass wrote: > Can you please take a look at this code too, from the patch which > brings in post-relocation board init (board_r.c): > > #ifdef CONFIG_PPC > /* TODO: Can we not use dmb() macros for this? */ > asm("sync ; isync"); > #endif I'm not sure why we need that at all. It's been there since "Initial revision", so git history isn't helpful in explaining it. -Scott
Hi Scott, On Tue, Feb 12, 2013 at 11:04 AM, Scott Wood <scottwood@freescale.com> wrote: > On 02/12/2013 12:55:39 PM, Simon Glass wrote: >> >> Can you please take a look at this code too, from the patch which >> brings in post-relocation board init (board_r.c): >> >> #ifdef CONFIG_PPC >> /* TODO: Can we not use dmb() macros for this? */ >> asm("sync ; isync"); >> #endif > > > I'm not sure why we need that at all. It's been there since "Initial > revision", so git history isn't helpful in explaining it. OK great, I will punt it for generic board. > > -Scott Regards, Simon
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index 1f12c29..1bf12f5 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -317,4 +317,12 @@ static inline phys_addr_t virt_to_phys(void * vaddr) #endif } +/* + * TODO: The kernel offers some more advanced versions of barriers, it might + * have some advantages to use them instead of the simple one here. + */ +#define dmb() __asm__ __volatile__ ("" : : : "memory") +#define __iormb() dmb() +#define __iowmb() dmb() + #endif
These are available on other architectures, so add them on ppc. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None arch/powerpc/include/asm/io.h | 8 ++++++++ 1 file changed, 8 insertions(+)