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

[Simon Glass]

Hi Scott,

On Tue, Feb 12, 2013 at 10:50 AM, Scott Wood <scottwood at 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 at 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 at 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