Patchwork [U-Boot,v5,01/23] ppc: Add initial memory barrier macros

login
register
mail settings
Submitter Simon Glass
Date Feb. 8, 2013, 3:11 p.m.
Message ID <1360336339-10703-2-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/219174/
State Deferred, archived
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - Feb. 8, 2013, 3:11 p.m.
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(+)
Scott Wood - Feb. 11, 2013, 7:52 p.m.
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
Simon Glass - Feb. 12, 2013, 6:44 p.m.
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
Scott Wood - Feb. 12, 2013, 6:50 p.m.
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
Simon Glass - Feb. 12, 2013, 6:55 p.m.
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
Scott Wood - Feb. 12, 2013, 7:04 p.m.
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
Simon Glass - Feb. 12, 2013, 7:06 p.m.
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

Patch

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