[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Victor Kamensky]

Hi Will, Ben, Russell, Thomas,

Please review second version of patch that fixes TLB asid issue in big endian
V7 image.

Changes from v1:
   Note previous patch subject line was 'ARM: tlb: 
   __flush_tlb_mm need to use int asid var for BE correct operation'

   Added 'unsigned int' cast into ASID macro itself rather
   then use intermediate 'int' variable in __flush_tlb_mm function. 
   This is done per v1 patch discussion at

   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html

Tested with Linaro BE topic branch on Arndale board. Both LE and BE
images were tested.

Thanks,
Victor

Victor Kamensky (1):
  ARM: tlb: ASID macro should give 32bit result for BE correct operation

 arch/arm/include/asm/mmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.1.4

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Victor Kamensky]

In order for ASID macro to be used as expression passed to
inline asm as 'r' operand it needs to give 32 bit unsigned result,
not unsigned 64bit expression.

Otherwise when 64bit ASID is passed to inline assembler statement
as 'r' operand (32bit) compiler behavior is not well specified.
For example when __flush_tlb_mm function compiled in big endian
case, and ASID is passed to tlb_op macro directly, 0 will be passed
as 'mcr	15, 0, r4, cr8, cr3, {2}' argument in r4, unless ASID
macro changed to produce 32 bit result.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm/include/asm/mmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/mmu.h b/arch/arm/include/asm/mmu.h
index 6f18da0..64fd151 100644
--- a/arch/arm/include/asm/mmu.h
+++ b/arch/arm/include/asm/mmu.h
@@ -16,7 +16,7 @@ typedef struct {
 #ifdef CONFIG_CPU_HAS_ASID
 #define ASID_BITS	8
 #define ASID_MASK	((~0ULL) << ASID_BITS)
-#define ASID(mm)	((mm)->context.id.counter & ~ASID_MASK)
+#define ASID(mm)	((unsigned int)((mm)->context.id.counter & ~ASID_MASK))
 #else
 #define ASID(mm)	(0)
 #endif
-- 
1.8.1.4

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Will Deacon]

On Mon, Oct 07, 2013 at 04:48:23PM +0100, Victor Kamensky wrote:
> In order for ASID macro to be used as expression passed to
> inline asm as 'r' operand it needs to give 32 bit unsigned result,
> not unsigned 64bit expression.
> 
> Otherwise when 64bit ASID is passed to inline assembler statement
> as 'r' operand (32bit) compiler behavior is not well specified.
> For example when __flush_tlb_mm function compiled in big endian
> case, and ASID is passed to tlb_op macro directly, 0 will be passed
> as 'mcr	15, 0, r4, cr8, cr3, {2}' argument in r4, unless ASID
> macro changed to produce 32 bit result.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  arch/arm/include/asm/mmu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/mmu.h b/arch/arm/include/asm/mmu.h
> index 6f18da0..64fd151 100644
> --- a/arch/arm/include/asm/mmu.h
> +++ b/arch/arm/include/asm/mmu.h
> @@ -16,7 +16,7 @@ typedef struct {
>  #ifdef CONFIG_CPU_HAS_ASID
>  #define ASID_BITS	8
>  #define ASID_MASK	((~0ULL) << ASID_BITS)
> -#define ASID(mm)	((mm)->context.id.counter & ~ASID_MASK)
> +#define ASID(mm)	((unsigned int)((mm)->context.id.counter & ~ASID_MASK))
>  #else
>  #define ASID(mm)	(0)
>  #endif

  Acked-by: Will Deacon <will.deacon@arm.com>

Cheers,

Will

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Russell King - ARM Linux]

On Mon, Oct 07, 2013 at 08:48:23AM -0700, Victor Kamensky wrote:
> -#define ASID(mm)	((mm)->context.id.counter & ~ASID_MASK)
> +#define ASID(mm)	((unsigned int)((mm)->context.id.counter & ~ASID_MASK))

Not a big problem, but "unsigned" is sufficient as per my suggestion.
See:

	http://en.wikipedia.org/wiki/C_data_types

or look it up in the C standards. :)  I'll take it either way.

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Victor Kamensky]

On 7 October 2013 08:52, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 07, 2013 at 08:48:23AM -0700, Victor Kamensky wrote:
>> -#define ASID(mm)     ((mm)->context.id.counter & ~ASID_MASK)
>> +#define ASID(mm)     ((unsigned int)((mm)->context.id.counter & ~ASID_MASK))
>
> Not a big problem, but "unsigned" is sufficient as per my suggestion.
> See:
>
>         http://en.wikipedia.org/wiki/C_data_types
>
> or look it up in the C standards. :)  I'll take it either way.

Understood, I just tried to be consistent with the rest of file -
'unsigned int' was
used just few lines above place that got changed.

Thanks,
Victor

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 07/10/13 17:48, Victor Kamensky wrote:
> Hi Will, Ben, Russell, Thomas,
>
> Please review second version of patch that fixes TLB asid issue in big endian
> V7 image.
>
> Changes from v1:
>     Note previous patch subject line was 'ARM: tlb:
>     __flush_tlb_mm need to use int asid var for BE correct operation'
>
>     Added 'unsigned int' cast into ASID macro itself rather
>     then use intermediate 'int' variable in __flush_tlb_mm function.
>     This is done per v1 patch discussion at
>
>     http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>
> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
> images were tested.

If you are booting on the Arndale board, is there a patch to mark
the relevant Exynos devices as BE capable?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Victor Kamensky]

On 7 October 2013 08:57, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 07/10/13 17:48, Victor Kamensky wrote:
>>
>> Hi Will, Ben, Russell, Thomas,
>>
>> Please review second version of patch that fixes TLB asid issue in big
>> endian
>> V7 image.
>>
>> Changes from v1:
>>     Note previous patch subject line was 'ARM: tlb:
>>     __flush_tlb_mm need to use int asid var for BE correct operation'
>>
>>     Added 'unsigned int' cast into ASID macro itself rather
>>     then use intermediate 'int' variable in __flush_tlb_mm function.
>>     This is done per v1 patch discussion at
>>
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>
>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>> images were tested.
>
>
> If you are booting on the Arndale board, is there a patch to mark
> the relevant Exynos devices as BE capable?

Arndale need massive fixes in their BSP layer to be endian agnostic
ARM V7 platform. Unfortunate it is not as simple as with few others
that already marked as BE capable.

Please see
https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
more subtle changes (some of them similar to changs that you've
done for other platforms). Also there are known unfixed issues like
disabling CONFIG_MMC_DW_IDMAC config because idmac
DMA related code is not endian agnostic yet (btw interesting class
of BE related problem that was not seen before).

In Linaro we use Arndale and Pandaboard as reference platforms
therefore we have BE BSP fixes in our tree. But I am not sure
what is fate of those in long term. Also we consider these as
example of BSP changes that other BSP need to do.

If Exynos and OMAP owners will have any interest for BE images,
and would like to see these changes in main line, we gladly
will work on this. Otherwise changes like this can mess up with
BSP ongoing drivers development.

I think above position is consistent with similar discussion on
some of BE related threads - changing BSP to support BE mode
is BSP owners call.

Thanks,
Victor

> --
> Ben Dooks                               http://www.codethink.co.uk/
> Senior Engineer                         Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 07/10/13 18:37, Victor Kamensky wrote:
> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>
>>> Hi Will, Ben, Russell, Thomas,
>>>
>>> Please review second version of patch that fixes TLB asid issue in big
>>> endian
>>> V7 image.
>>>
>>> Changes from v1:
>>>      Note previous patch subject line was 'ARM: tlb:
>>>      __flush_tlb_mm need to use int asid var for BE correct operation'
>>>
>>>      Added 'unsigned int' cast into ASID macro itself rather
>>>      then use intermediate 'int' variable in __flush_tlb_mm function.
>>>      This is done per v1 patch discussion at
>>>
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>>
>>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>>> images were tested.
>>
>>
>> If you are booting on the Arndale board, is there a patch to mark
>> the relevant Exynos devices as BE capable?
>
> Arndale need massive fixes in their BSP layer to be endian agnostic
> ARM V7 platform. Unfortunate it is not as simple as with few others
> that already marked as BE capable.
>
> Please see
> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
> more subtle changes (some of them similar to changs that you've
> done for other platforms). Also there are known unfixed issues like
> disabling CONFIG_MMC_DW_IDMAC config because idmac
> DMA related code is not endian agnostic yet (btw interesting class
> of BE related problem that was not seen before).

Ok, will have a look. I'm interested as I may be able to
update my techcon paper and possibly insert a slide or two.

> In Linaro we use Arndale and Pandaboard as reference platforms
> therefore we have BE BSP fixes in our tree. But I am not sure
> what is fate of those in long term. Also we consider these as
> example of BSP changes that other BSP need to do.
>
> If Exynos and OMAP owners will have any interest for BE images,
> and would like to see these changes in main line, we gladly
> will work on this. Otherwise changes like this can mess up with
> BSP ongoing drivers development.
>
> I think above position is consistent with similar discussion on
> some of BE related threads - changing BSP to support BE mode
> is BSP owners call.

I'm not sure if it will ever be interesting to OMAP, especially
as it is pretty much a dead-end.

It would be nice to see the Exynos support it.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Santosh Shilimkar]

Victor,

On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
> On 7 October 2013 08:57, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>
>>> Hi Will, Ben, Russell, Thomas,
>>>
>>> Please review second version of patch that fixes TLB asid issue in big
>>> endian
>>> V7 image.
>>>
>>> Changes from v1:
>>>     Note previous patch subject line was 'ARM: tlb:
>>>     __flush_tlb_mm need to use int asid var for BE correct operation'
>>>
>>>     Added 'unsigned int' cast into ASID macro itself rather
>>>     then use intermediate 'int' variable in __flush_tlb_mm function.
>>>     This is done per v1 patch discussion at
>>>
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>>
>>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>>> images were tested.
>>
>>
>> If you are booting on the Arndale board, is there a patch to mark
>> the relevant Exynos devices as BE capable?
> 
> Arndale need massive fixes in their BSP layer to be endian agnostic
> ARM V7 platform. Unfortunate it is not as simple as with few others
> that already marked as BE capable.
> 
> Please see
> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
> more subtle changes (some of them similar to changs that you've
> done for other platforms). Also there are known unfixed issues like
> disabling CONFIG_MMC_DW_IDMAC config because idmac
> DMA related code is not endian agnostic yet (btw interesting class
> of BE related problem that was not seen before).
> 
> In Linaro we use Arndale and Pandaboard as reference platforms
> therefore we have BE BSP fixes in our tree. But I am not sure
> what is fate of those in long term. Also we consider these as
> example of BSP changes that other BSP need to do.
> 
> If Exynos and OMAP owners will have any interest for BE images,
> and would like to see these changes in main line, we gladly
> will work on this. Otherwise changes like this can mess up with
> BSP ongoing drivers development.
> 
BE support in mainline is definitely we are interested for OMAP
and rest of the TI SoCs.

> I think above position is consistent with similar discussion on
> some of BE related threads - changing BSP to support BE mode
> is BSP owners call.
> 
Am just wondering a better method than the patch [1] which touches
many drivers for readl/writel() replacement. Drivers are using
that as standard based on device driver guide and was thinking
we should not change that rule to support BE. We definitely need
to get the byte swap achieved but probably through some other
means. 

Regards,
Santosh
[1] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commit;h=9074a67ab082c62545683e5ec909368a23c33655

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Victor Kamensky]

Hi Santosh,

Please see inline.

On 7 October 2013 15:49, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:
> Victor,
>
> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>> On 7 October 2013 08:57, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>>
>>>> Hi Will, Ben, Russell, Thomas,
>>>>
>>>> Please review second version of patch that fixes TLB asid issue in big
>>>> endian
>>>> V7 image.
>>>>
>>>> Changes from v1:
>>>>     Note previous patch subject line was 'ARM: tlb:
>>>>     __flush_tlb_mm need to use int asid var for BE correct operation'
>>>>
>>>>     Added 'unsigned int' cast into ASID macro itself rather
>>>>     then use intermediate 'int' variable in __flush_tlb_mm function.
>>>>     This is done per v1 patch discussion at
>>>>
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>>>
>>>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>>>> images were tested.
>>>
>>>
>>> If you are booting on the Arndale board, is there a patch to mark
>>> the relevant Exynos devices as BE capable?
>>
>> Arndale need massive fixes in their BSP layer to be endian agnostic
>> ARM V7 platform. Unfortunate it is not as simple as with few others
>> that already marked as BE capable.
>>
>> Please see
>> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
>> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
>> more subtle changes (some of them similar to changs that you've
>> done for other platforms). Also there are known unfixed issues like
>> disabling CONFIG_MMC_DW_IDMAC config because idmac
>> DMA related code is not endian agnostic yet (btw interesting class
>> of BE related problem that was not seen before).
>>
>> In Linaro we use Arndale and Pandaboard as reference platforms
>> therefore we have BE BSP fixes in our tree. But I am not sure
>> what is fate of those in long term. Also we consider these as
>> example of BSP changes that other BSP need to do.
>>
>> If Exynos and OMAP owners will have any interest for BE images,
>> and would like to see these changes in main line, we gladly
>> will work on this. Otherwise changes like this can mess up with
>> BSP ongoing drivers development.
>>
> BE support in mainline is definitely we are interested for OMAP
> and rest of the TI SoCs.

OK, let's get agreement on __raw_xxx issue as per your note below.
We will have discussion about OMAP BE changes in mainline,
within LNG BE group and I think we could try to push OMAP BE
changes out.

>> I think above position is consistent with similar discussion on
>> some of BE related threads - changing BSP to support BE mode
>> is BSP owners call.
>>
> Am just wondering a better method than the patch [1] which touches
> many drivers for readl/writel() replacement. Drivers are using
> that as standard based on device driver guide

Do you have pointer for above?

> and was thinking
> we should not change that rule to support BE. We definitely need
> to get the byte swap achieved but probably through some other
> means.

__raw_read[lw] and __raw_write[lw] change idea for BE images was
already discussed in [1]. Semantics of __raw_xxx function, vs xxx,
vs xxx_relaxed, somewhat explained at [2]. [2] references LKML
thread on similar subject.

As it stands now (at least in my understanding):
__raw_xxx - is native byte order operations, non mem barriers, cannot
      be used to read LE hw in endian agnostic way
xxx - reads/writes LE h/w byteswapa to target byte order if needed, uses
      mem barriers, it is already endian agnostic
xxx_relaxed - read LE h/w,  byteswapa to target byte order if needed,
      no mem barriers, should be used to read LE h/w in endian agnostic
      way, they are the same as __raw_xxx plus proper byteswaps.

That should go for all CPU types. As Will wrote on [1] it will be really
hard to change it. I agree with that.

Having said above I would like to notice that practically all cases where
__raw_xxx are used directly in Pandaboard and Exynos drivers
should be replaced with xxx_relaxed variant. I run into only one exception [3].

In fact when I get update, I apply hack patch that makes all inline functions
in arch/arm/include/asm/io.h instead of __raw_xxx function to use
_raw_xxx function which are added as exact copies of __raw_xxx, and
then I run small script with 'readelf --debug-dump vmlinux' and awk
that catches all direct uses of __raw_xxx functions, except [3] I
never saw case where __raw_xxx should not be replaced with xxx_relaxed.

I agree on your sentiment that when big diff with just
simple rename in many places will go in, it could be very disruptive,
and it will take time to change people's habits, but personally I  don't
see any other way.

Thanks,
Victor

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186000.html
[2] http://stackoverflow.com/questions/9818657/whats-the-difference-b-w-raw-readl-raw-writel-and-readl-writel-in-linux-ker
[3] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commitdiff;h=7c78d140b4a3a46e7fa2979889c341f297e3fd6c

> Regards,
> Santosh
> [1] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commit;h=9074a67ab082c62545683e5ec909368a23c33655

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Kim Phillips]

On Mon, 7 Oct 2013 18:49:20 -0400
Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:

> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
> > On 7 October 2013 08:57, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> >> On 07/10/13 17:48, Victor Kamensky wrote:
> >> If you are booting on the Arndale board, is there a patch to mark
> >> the relevant Exynos devices as BE capable?
> > 
> > Arndale need massive fixes in their BSP layer to be endian agnostic
> > ARM V7 platform. Unfortunate it is not as simple as with few others
> > that already marked as BE capable.
> > 
> > Please see
> > https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
> > Mostly it is __raw_xxx conversion to xxx_relaxed, but there are

apologies if this was explained earlier in the thread, but what has
__raw_xxx -> xxx_relaxed have to do with endianness?  the relaxed
accessor variants allow the compiler to reorder the instruction: it's
got nothing to do with byte swapping the data, no?

> > I think above position is consistent with similar discussion on
> > some of BE related threads - changing BSP to support BE mode
> > is BSP owners call.
> > 
> Am just wondering a better method than the patch [1] which touches
> many drivers for readl/writel() replacement. Drivers are using
> that as standard based on device driver guide and was thinking
> we should not change that rule to support BE. We definitely need

I think readl/writel were originally devised for accessing PCI devices
(else why would readl's definition include an __le32_to_cpu byteswap)?
In any case, this makes read/writel incompatible with big endian
devices.

> to get the byte swap achieved but probably through some other
> means. 

back when I looked into this, I found in/out_be32() accessors were Power
arch centric, read/writel ARM arch centric, whereas ioread/writebe32
were available in other arches.  See e.g., upstream commit
0c69fb037a6bb1faf06ea776872da54a6705c154 "mtd: fsl_ifc_nand: use more
portable i/o accessors".

Kim

> [1] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commit;h=9074a67ab082c62545683e5ec909368a23c33655

if anything, a patch with 'endian fix' in the subject would be
*removing* sparse endianness warnings, not leaving them the same.

To run sparse endian checks, use:

make C=2 CF="-D__CHECK_ENDIAN__"

see Documentation/sparse.txt for details.

Thanks,

Kim

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 08/10/13 02:55, Kim Phillips wrote:
> On Mon, 7 Oct 2013 18:49:20 -0400
> Santosh Shilimkar<santosh.shilimkar@ti.com>  wrote:
>
>> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>> If you are booting on the Arndale board, is there a patch to mark
>>>> the relevant Exynos devices as BE capable?
>>>
>>> Arndale need massive fixes in their BSP layer to be endian agnostic
>>> ARM V7 platform. Unfortunate it is not as simple as with few others
>>> that already marked as BE capable.
>>>
>>> Please see
>>> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
>>> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
>
> apologies if this was explained earlier in the thread, but what has
> __raw_xxx ->  xxx_relaxed have to do with endianness?  the relaxed
> accessor variants allow the compiler to reorder the instruction: it's
> got nothing to do with byte swapping the data, no?
>
>>> I think above position is consistent with similar discussion on
>>> some of BE related threads - changing BSP to support BE mode
>>> is BSP owners call.
>>>
>> Am just wondering a better method than the patch [1] which touches
>> many drivers for readl/writel() replacement. Drivers are using
>> that as standard based on device driver guide and was thinking
>> we should not change that rule to support BE. We definitely need
>
> I think readl/writel were originally devised for accessing PCI devices
> (else why would readl's definition include an __le32_to_cpu byteswap)?
> In any case, this makes read/writel incompatible with big endian
> devices.

Generally, in ARM, the devices stay little endian when the CPU is
running big endian, therefore we need this in there.

If we end up having to change all the drivers, then this series
is going to become a huge incovenience to everyone and at that
point it will probably be better to drop it.

>> to get the byte swap achieved but probably through some other
>> means.
>
> back when I looked into this, I found in/out_be32() accessors were Power
> arch centric, read/writel ARM arch centric, whereas ioread/writebe32
> were available in other arches.  See e.g., upstream commit
> 0c69fb037a6bb1faf06ea776872da54a6705c154 "mtd: fsl_ifc_nand: use more
> portable i/o accessors".

readl and writel should work pretty much everywhere.

I'm not sure if device drivers /should/ know much about what
endian bus or the endian-ness of the peripheral they are managing,
especially if we have the case the bus-bus bridge is doing a swap.

Personally, I'd just like to rip out the IO accessors we have at
the moment and just replace it with a much more BSD like system
where the device struct provides a set of accessor functions. Then
if we are on strange busses, or multi-function devices, or the
latest in inter-periphreal connect we don't have to keep re-working
drivers.

It'd even make regmap nicer as it could sit in a layer below any
device and would not need and explicit changes for a driver to use
it.

> Kim
>
>> [1] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commit;h=9074a67ab082c62545683e5ec909368a23c33655
>
> if anything, a patch with 'endian fix' in the subject would be
> *removing* sparse endianness warnings, not leaving them the same.

A few warnings are better than a system that does not work.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Kim Phillips]

On Tue, 08 Oct 2013 09:09:51 +0200
Ben Dooks <ben.dooks@codethink.co.uk> wrote:

> On 08/10/13 02:55, Kim Phillips wrote:
> > On Mon, 7 Oct 2013 18:49:20 -0400
> > Santosh Shilimkar<santosh.shilimkar@ti.com>  wrote:
> >
> >> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
> >>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
> >>>> On 07/10/13 17:48, Victor Kamensky wrote:
> > apologies if this was explained earlier in the thread, but what has
> > __raw_xxx ->  xxx_relaxed have to do with endianness?  the relaxed
> > accessor variants allow the compiler to reorder the instruction: it's
> > got nothing to do with byte swapping the data, no?

[I've since read the thread explaining this, thanks Victor]

> >>> I think above position is consistent with similar discussion on
> >>> some of BE related threads - changing BSP to support BE mode
> >>> is BSP owners call.
> >>>
> >> Am just wondering a better method than the patch [1] which touches
> >> many drivers for readl/writel() replacement. Drivers are using
> >> that as standard based on device driver guide and was thinking
> >> we should not change that rule to support BE. We definitely need
> >
> > I think readl/writel were originally devised for accessing PCI devices
> > (else why would readl's definition include an __le32_to_cpu byteswap)?
> > In any case, this makes read/writel incompatible with big endian
> > devices.
> 
> Generally, in ARM, the devices stay little endian when the CPU is
> running big endian, therefore we need this in there.

generally, yes, but with SoC vendors switching to arch ARM, more ARM
cores will be having to access legacy big endian devices :)

> If we end up having to change all the drivers, then this series
> is going to become a huge incovenience to everyone and at that
> point it will probably be better to drop it.

well that depends on the change: a simple search and replace vs. one
requiring more knowledge of the device, and what you mean by 'all
drivers' (the devices the arm defconfigs configure are mostly drivers
for LE devices, at least for the time being).  If we need to transition
from an old set of accessors to new ones, drivers can be changed
incrementally, and by their maintainers - we just need to establish a
standard set of guidelines.

> >> to get the byte swap achieved but probably through some other
> >> means.
> >
> > back when I looked into this, I found in/out_be32() accessors were Power
> > arch centric, read/writel ARM arch centric, whereas ioread/writebe32
> > were available in other arches.  See e.g., upstream commit
> > 0c69fb037a6bb1faf06ea776872da54a6705c154 "mtd: fsl_ifc_nand: use more
> > portable i/o accessors".
> 
> readl and writel should work pretty much everywhere.

well, they currently work unfavourably for big endian devices on
arch/arm.

> I'm not sure if device drivers /should/ know much about what
> endian bus or the endian-ness of the peripheral they are managing,
> especially if we have the case the bus-bus bridge is doing a swap.
> 
> Personally, I'd just like to rip out the IO accessors we have at
> the moment and just replace it with a much more BSD like system
> where the device struct provides a set of accessor functions. Then
> if we are on strange busses, or multi-function devices, or the
> latest in inter-periphreal connect we don't have to keep re-working
> drivers.
> 
> It'd even make regmap nicer as it could sit in a layer below any
> device and would not need and explicit changes for a driver to use
> it.

yes, some devices can be endianness programmable at runtime - and with
mixed-endian VMs having direct access to devices, it may be better to
leave it to a combination of the driver and its bus.

OTOH, the compiler would lose the ability to inline the accessor
functions for the simple, more popular cases.

> >> [1] https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=commit;h=9074a67ab082c62545683e5ec909368a23c33655
> >
> > if anything, a patch with 'endian fix' in the subject would be
> > *removing* sparse endianness warnings, not leaving them the same.
> 
> A few warnings are better than a system that does not work.

sure, I'm saying let's try and improve the situation for the sake of
future development - endianness developers using and conforming to
sparse warnings is an easy way to accomplish that.

Kim

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 09/10/13 03:22, Kim Phillips wrote:
> On Tue, 08 Oct 2013 09:09:51 +0200
> Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>
>> On 08/10/13 02:55, Kim Phillips wrote:
>>> On Mon, 7 Oct 2013 18:49:20 -0400
>>> Santosh Shilimkar<santosh.shilimkar@ti.com>   wrote:
>>>
>>>> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>>>>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>   wrote:
>>>>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>> apologies if this was explained earlier in the thread, but what has
>>> __raw_xxx ->   xxx_relaxed have to do with endianness?  the relaxed
>>> accessor variants allow the compiler to reorder the instruction: it's
>>> got nothing to do with byte swapping the data, no?
>
> [I've since read the thread explaining this, thanks Victor]
>
>>>>> I think above position is consistent with similar discussion on
>>>>> some of BE related threads - changing BSP to support BE mode
>>>>> is BSP owners call.
>>>>>
>>>> Am just wondering a better method than the patch [1] which touches
>>>> many drivers for readl/writel() replacement. Drivers are using
>>>> that as standard based on device driver guide and was thinking
>>>> we should not change that rule to support BE. We definitely need
>>>
>>> I think readl/writel were originally devised for accessing PCI devices
>>> (else why would readl's definition include an __le32_to_cpu byteswap)?
>>> In any case, this makes read/writel incompatible with big endian
>>> devices.
>>
>> Generally, in ARM, the devices stay little endian when the CPU is
>> running big endian, therefore we need this in there.
>
> generally, yes, but with SoC vendors switching to arch ARM, more ARM
> cores will be having to access legacy big endian devices :)
>
>> If we end up having to change all the drivers, then this series
>> is going to become a huge incovenience to everyone and at that
>> point it will probably be better to drop it.
>
> well that depends on the change: a simple search and replace vs. one
> requiring more knowledge of the device, and what you mean by 'all
> drivers' (the devices the arm defconfigs configure are mostly drivers
> for LE devices, at least for the time being).  If we need to transition
> from an old set of accessors to new ones, drivers can be changed
> incrementally, and by their maintainers - we just need to establish a
> standard set of guidelines.
>
>>>> to get the byte swap achieved but probably through some other
>>>> means.
>>>
>>> back when I looked into this, I found in/out_be32() accessors were Power
>>> arch centric, read/writel ARM arch centric, whereas ioread/writebe32
>>> were available in other arches.  See e.g., upstream commit
>>> 0c69fb037a6bb1faf06ea776872da54a6705c154 "mtd: fsl_ifc_nand: use more
>>> portable i/o accessors".
>>
>> readl and writel should work pretty much everywhere.
>
> well, they currently work unfavourably for big endian devices on
> arch/arm.

I've not see any big endian devices on ARM as of yet. Pretty much
everything I have seen is little endian.

In this case, read/writel are not the issue, it is the __raw versions
that get used in bits of ARM that are. I'm still not sure of how many
are of `native endian` devices or just little endian devices that have
never been tested with a big endian cpu core.

>> I'm not sure if device drivers /should/ know much about what
>> endian bus or the endian-ness of the peripheral they are managing,
>> especially if we have the case the bus-bus bridge is doing a swap.
>>
>> Personally, I'd just like to rip out the IO accessors we have at
>> the moment and just replace it with a much more BSD like system
>> where the device struct provides a set of accessor functions. Then
>> if we are on strange busses, or multi-function devices, or the
>> latest in inter-periphreal connect we don't have to keep re-working
>> drivers.
>>
>> It'd even make regmap nicer as it could sit in a layer below any
>> device and would not need and explicit changes for a driver to use
>> it.
>
> yes, some devices can be endianness programmable at runtime - and with
> mixed-endian VMs having direct access to devices, it may be better to
> leave it to a combination of the driver and its bus.

It doesn't really matter what endian-ness the VM is as long as it
treats device memory as little-endian just as it would have to do
if it was running on a device.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 08/10/13 02:55, Kim Phillips wrote:
> On Mon, 7 Oct 2013 18:49:20 -0400
> Santosh Shilimkar<santosh.shilimkar@ti.com>  wrote:
>
>> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>> If you are booting on the Arndale board, is there a patch to mark
>>>> the relevant Exynos devices as BE capable?
>>>
>>> Arndale need massive fixes in their BSP layer to be endian agnostic
>>> ARM V7 platform. Unfortunate it is not as simple as with few others
>>> that already marked as BE capable.
>>>
>>> Please see
>>> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
>>> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
>
> apologies if this was explained earlier in the thread, but what has
> __raw_xxx ->  xxx_relaxed have to do with endianness?  the relaxed
> accessor variants allow the compiler to reorder the instruction: it's
> got nothing to do with byte swapping the data, no?

the __raw have similar properties, however the readl/writel relaxed
are basically __raw with byte-swapping and no attempt to do any other
flush/drain/etc.

Thus I thought these would be the best ones to go for.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Ben Dooks]

On 08/10/13 00:49, Santosh Shilimkar wrote:
> Victor,
>
> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>>
>>>> Hi Will, Ben, Russell, Thomas,
>>>>
>>>> Please review second version of patch that fixes TLB asid issue in big
>>>> endian
>>>> V7 image.
>>>>
>>>> Changes from v1:
>>>>      Note previous patch subject line was 'ARM: tlb:
>>>>      __flush_tlb_mm need to use int asid var for BE correct operation'
>>>>
>>>>      Added 'unsigned int' cast into ASID macro itself rather
>>>>      then use intermediate 'int' variable in __flush_tlb_mm function.
>>>>      This is done per v1 patch discussion at
>>>>
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>>>
>>>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>>>> images were tested.
>>>
>>>
>>> If you are booting on the Arndale board, is there a patch to mark
>>> the relevant Exynos devices as BE capable?
>>
>> Arndale need massive fixes in their BSP layer to be endian agnostic
>> ARM V7 platform. Unfortunate it is not as simple as with few others
>> that already marked as BE capable.
>>
>> Please see
>> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
>> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
>> more subtle changes (some of them similar to changs that you've
>> done for other platforms). Also there are known unfixed issues like
>> disabling CONFIG_MMC_DW_IDMAC config because idmac
>> DMA related code is not endian agnostic yet (btw interesting class
>> of BE related problem that was not seen before).
>>
>> In Linaro we use Arndale and Pandaboard as reference platforms
>> therefore we have BE BSP fixes in our tree. But I am not sure
>> what is fate of those in long term. Also we consider these as
>> example of BSP changes that other BSP need to do.
>>
>> If Exynos and OMAP owners will have any interest for BE images,
>> and would like to see these changes in main line, we gladly
>> will work on this. Otherwise changes like this can mess up with
>> BSP ongoing drivers development.
>>
> BE support in mainline is definitely we are interested for OMAP
> and rest of the TI SoCs.

It would be great to get some more SoCs supported.

>> I think above position is consistent with similar discussion on
>> some of BE related threads - changing BSP to support BE mode
>> is BSP owners call.
>>
> Am just wondering a better method than the patch [1] which touches
> many drivers for readl/writel() replacement. Drivers are using
> that as standard based on device driver guide and was thinking
> we should not change that rule to support BE. We definitely need
> to get the byte swap achieved but probably through some other
> means.

read{b,w,l} and write{b,w,l} work fine.  It is the use of the
__raw_ versions that is the problem. I will be publishing a
paper on this for ARM TechCon.

I have not yet suggested we also change the __raw functions as
we are not yet sure if there are places where we could end up
with mixed-endian systems.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH v2] ARM: tlb: ASID macro should give 32bit result for BE correct operation

[Santosh Shilimkar]

Ben, Victor,

On Tuesday 08 October 2013 02:59 AM, Ben Dooks wrote:
> On 08/10/13 00:49, Santosh Shilimkar wrote:
>> Victor,
>>
>> On Monday 07 October 2013 12:37 PM, Victor Kamensky wrote:
>>> On 7 October 2013 08:57, Ben Dooks<ben.dooks@codethink.co.uk>  wrote:
>>>> On 07/10/13 17:48, Victor Kamensky wrote:
>>>>>
>>>>> Hi Will, Ben, Russell, Thomas,
>>>>>
>>>>> Please review second version of patch that fixes TLB asid issue in big
>>>>> endian
>>>>> V7 image.
>>>>>
>>>>> Changes from v1:
>>>>>      Note previous patch subject line was 'ARM: tlb:
>>>>>      __flush_tlb_mm need to use int asid var for BE correct operation'
>>>>>
>>>>>      Added 'unsigned int' cast into ASID macro itself rather
>>>>>      then use intermediate 'int' variable in __flush_tlb_mm function.
>>>>>      This is done per v1 patch discussion at
>>>>>
>>>>>
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202583.html
>>>>>
>>>>> Tested with Linaro BE topic branch on Arndale board. Both LE and BE
>>>>> images were tested.
>>>>
>>>>
>>>> If you are booting on the Arndale board, is there a patch to mark
>>>> the relevant Exynos devices as BE capable?
>>>
>>> Arndale need massive fixes in their BSP layer to be endian agnostic
>>> ARM V7 platform. Unfortunate it is not as simple as with few others
>>> that already marked as BE capable.
>>>
>>> Please see
>>> https://git.linaro.org/gitweb?p=people/victor.kamensky/linux-linaro-tracking-be.git;a=shortlog;h=refs/heads/llct-be-topic
>>> Mostly it is __raw_xxx conversion to xxx_relaxed, but there are
>>> more subtle changes (some of them similar to changs that you've
>>> done for other platforms). Also there are known unfixed issues like
>>> disabling CONFIG_MMC_DW_IDMAC config because idmac
>>> DMA related code is not endian agnostic yet (btw interesting class
>>> of BE related problem that was not seen before).
>>>
>>> In Linaro we use Arndale and Pandaboard as reference platforms
>>> therefore we have BE BSP fixes in our tree. But I am not sure
>>> what is fate of those in long term. Also we consider these as
>>> example of BSP changes that other BSP need to do.
>>>
>>> If Exynos and OMAP owners will have any interest for BE images,
>>> and would like to see these changes in main line, we gladly
>>> will work on this. Otherwise changes like this can mess up with
>>> BSP ongoing drivers development.
>>>
>> BE support in mainline is definitely we are interested for OMAP
>> and rest of the TI SoCs.
> 
> It would be great to get some more SoCs supported.
> 
>>> I think above position is consistent with similar discussion on
>>> some of BE related threads - changing BSP to support BE mode
>>> is BSP owners call.
>>>
>> Am just wondering a better method than the patch [1] which touches
>> many drivers for readl/writel() replacement. Drivers are using
>> that as standard based on device driver guide and was thinking
>> we should not change that rule to support BE. We definitely need
>> to get the byte swap achieved but probably through some other
>> means.
> 
> read{b,w,l} and write{b,w,l} work fine.  It is the use of the
> __raw_ versions that is the problem. I will be publishing a
> paper on this for ARM TechCon.
> 
> I have not yet suggested we also change the __raw functions as
> we are not yet sure if there are places where we could end up
> with mixed-endian systems.
>
Ok. Replacing _raw functions shouldn't be problem if we plan do
that but as you said we might have hardwares IPs which can support
endian swapping at IP level.

Will keep an eye on your updates on this part.

Regards,
Santosh