[patch 2/4] [ARM] mmp: support marvell ARMADA610

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

Haojian,

Several suggestions:

1. entry-macro.S, could we simplify (and improve the performance a bit) to
something like (haven't tested yet - let me know the result):

	.macro	get_irqnr_preamble, base, tmp
	mrc	p15, 0, \tmp, c0, c0, 0		@ CPUID
	mov	\tmp, \tmp, lsr #4
	and	\tmp, \tmp, #0xfff
	cmp	\tmp, #0x581			@ MMP2
	moveq	\base, #MMP2_ICU_PJ4_IRQ_SEL
	movne	\base, #ICU_AP_IRQ_SEL_INT_NUM
	.endm

	.macro	arch_ret_to_user, tmp1, tmp2
	.endm

	.macro	get_irqnr_and_base, irqnr, irqstat, base, tmp
	ldr	\tmp, [\base, #0]
	and	\irqnr, \tmp, #0x3f		@ Interrupt Number
	tst	\tmp, #(1 << 6)			@ Interrupt Pending ?
	.endm

2. arch/arm/mach-mmp/irq.c

We could well separate irq.c to irq-pxa168.c and irq-mmp2.c since the
difference looks much.

3. ARMADA610 - I'd still prefer it being called MMP2

4. in timer_config()

would be nicer to have: ccr &= (cpu_is_mmp2()) ? TMR_CCR_CS_0(0) :
TMR_CCR_CS_0(3)

a little bit cleaner - but it is nit picking indeed :)

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

On Mon, Dec 28, 2009 at 10:49 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> Haojian,
>
> Several suggestions:
>
> 1. entry-macro.S, could we simplify (and improve the performance a bit) to
> something like (haven't tested yet - let me know the result):
>
> ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
> ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
> ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
> ? ? ? ?and ? ? \tmp, \tmp, #0xfff

We shouldn't use #0xfff in and instruction. Immediate number shouldn't
beyond 8bit. We have to use ldr instruction or multiple instrunctions.

> ? ? ? ?cmp ? ? \tmp, #0x581 ? ? ? ? ? ? ? ? ? ?@ MMP2
> ? ? ? ?moveq ? \base, #MMP2_ICU_PJ4_IRQ_SEL
> ? ? ? ?movne ? \base, #ICU_AP_IRQ_SEL_INT_NUM
> ? ? ? ?.endm
>
> ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
> ? ? ? ?.endm
>
> ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
> ? ? ? ?ldr ? ? \tmp, [\base, #0]
> ? ? ? ?and ? ? \irqnr, \tmp, #0x3f ? ? ? ? ? ? @ Interrupt Number
> ? ? ? ?tst ? ? \tmp, #(1 << 6) ? ? ? ? ? ? ? ? @ Interrupt Pending ?
> ? ? ? ?.endm
>

I don't like this way. Although more jump instructions are used in irq
entry, it can be extended easier for supporting more silicons.

I've updated these patches in attached mail.

Thanks
Haojian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mmp-support-marvell-MMP2.patch
Type: text/x-patch
Size: 21538 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091231/acb4ede1/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mmp-add-device-support-in-mmp2.patch
Type: text/x-patch
Size: 17396 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091231/acb4ede1/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-mmp-support-flint-development-board.patch
Type: text/x-patch
Size: 3889 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091231/acb4ede1/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004--ARM-mmp-add-mmp2-configuration.patch
Type: text/x-patch
Size: 30784 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091231/acb4ede1/attachment-0007.bin>

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

On Thu, Dec 31, 2009 at 10:42 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Mon, Dec 28, 2009 at 10:49 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> Haojian,
>>
>> Several suggestions:
>>
>> 1. entry-macro.S, could we simplify (and improve the performance a bit) to
>> something like (haven't tested yet - let me know the result):
>>
>> ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
>> ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
>> ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
>> ? ? ? ?and ? ? \tmp, \tmp, #0xfff
>
> We shouldn't use #0xfff in and instruction. Immediate number shouldn't
> beyond 8bit. We have to use ldr instruction or multiple instrunctions.

Well, 0xff0 then, or ldr, whichever I don't mind.

>
>> ? ? ? ?cmp ? ? \tmp, #0x581 ? ? ? ? ? ? ? ? ? ?@ MMP2
>> ? ? ? ?moveq ? \base, #MMP2_ICU_PJ4_IRQ_SEL
>> ? ? ? ?movne ? \base, #ICU_AP_IRQ_SEL_INT_NUM
>> ? ? ? ?.endm
>>
>> ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
>> ? ? ? ?.endm
>>
>> ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
>> ? ? ? ?ldr ? ? \tmp, [\base, #0]
>> ? ? ? ?and ? ? \irqnr, \tmp, #0x3f ? ? ? ? ? ? @ Interrupt Number
>> ? ? ? ?tst ? ? \tmp, #(1 << 6) ? ? ? ? ? ? ? ? @ Interrupt Pending ?
>> ? ? ? ?.endm
>>
>
> I don't like this way. Although more jump instructions are used in irq
> entry, it can be extended easier for supporting more silicons.
>

It really hurt performance a lot. While the difference between MMP series
is just the offset of the IRQ_NUM register, it could be calculated in
get_irqnr_preamble, which doesn't have to be executed each time
in the IRQ handling loop. See arch/arm/kernel/entry-armv.S

PXA is different since PXA25x checks the IRQ status by MMIO
register while PXA27x and above check that by co-processor.

> I've updated these patches in attached mail.
>

Thanks, I'll take a look and happy new year.

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

On Thu, Dec 31, 2009 at 10:46 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Thu, Dec 31, 2009 at 10:42 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Mon, Dec 28, 2009 at 10:49 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>> Haojian,
>>>
>>> Several suggestions:
>>>
>>> 1. entry-macro.S, could we simplify (and improve the performance a bit) to
>>> something like (haven't tested yet - let me know the result):
>>>
>>> ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
>>> ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
>>> ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
>>> ? ? ? ?and ? ? \tmp, \tmp, #0xfff
>>
>> We shouldn't use #0xfff in and instruction. Immediate number shouldn't
>> beyond 8bit. We have to use ldr instruction or multiple instrunctions.
>
> Well, 0xff0 then, or ldr, whichever I don't mind.
>
>>
>>> ? ? ? ?cmp ? ? \tmp, #0x581 ? ? ? ? ? ? ? ? ? ?@ MMP2
>>> ? ? ? ?moveq ? \base, #MMP2_ICU_PJ4_IRQ_SEL
>>> ? ? ? ?movne ? \base, #ICU_AP_IRQ_SEL_INT_NUM
>>> ? ? ? ?.endm
>>>
>>> ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
>>> ? ? ? ?.endm
>>>
>>> ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
>>> ? ? ? ?ldr ? ? \tmp, [\base, #0]
>>> ? ? ? ?and ? ? \irqnr, \tmp, #0x3f ? ? ? ? ? ? @ Interrupt Number
>>> ? ? ? ?tst ? ? \tmp, #(1 << 6) ? ? ? ? ? ? ? ? @ Interrupt Pending ?
>>> ? ? ? ?.endm
>>>
>>
>> I don't like this way. Although more jump instructions are used in irq
>> entry, it can be extended easier for supporting more silicons.
>>
>
> It really hurt performance a lot. While the difference between MMP series
> is just the offset of the IRQ_NUM register, it could be calculated in
> get_irqnr_preamble, which doesn't have to be executed each time
> in the IRQ handling loop. See arch/arm/kernel/entry-armv.S

get_irqnr_preamble can only carries one base parameter and one tmp
parameter. base parameter is already used for interrupt control
register. Only one tmp parameter is left for use. If we want to check
chip id, one parameter is not enought.

So putting these code into get_irqnr_and_base macro is more reasonable.

Happy new year! :)

Best Regards
Haojian

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

On Mon, Jan 4, 2010 at 10:27 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Thu, Dec 31, 2009 at 10:46 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> On Thu, Dec 31, 2009 at 10:42 PM, Haojian Zhuang
>> <haojian.zhuang@gmail.com> wrote:
>>> On Mon, Dec 28, 2009 at 10:49 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>>> Haojian,
>>>>
>>>> Several suggestions:
>>>>
>>>> 1. entry-macro.S, could we simplify (and improve the performance a bit) to
>>>> something like (haven't tested yet - let me know the result):
>>>>
>>>> ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
>>>> ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
>>>> ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
>>>> ? ? ? ?and ? ? \tmp, \tmp, #0xfff
>>>
>>> We shouldn't use #0xfff in and instruction. Immediate number shouldn't
>>> beyond 8bit. We have to use ldr instruction or multiple instrunctions.
>>
>> Well, 0xff0 then, or ldr, whichever I don't mind.
>>
>>>
>>>> ? ? ? ?cmp ? ? \tmp, #0x581 ? ? ? ? ? ? ? ? ? ?@ MMP2
>>>> ? ? ? ?moveq ? \base, #MMP2_ICU_PJ4_IRQ_SEL
>>>> ? ? ? ?movne ? \base, #ICU_AP_IRQ_SEL_INT_NUM
>>>> ? ? ? ?.endm
>>>>
>>>> ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
>>>> ? ? ? ?.endm
>>>>
>>>> ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
>>>> ? ? ? ?ldr ? ? \tmp, [\base, #0]
>>>> ? ? ? ?and ? ? \irqnr, \tmp, #0x3f ? ? ? ? ? ? @ Interrupt Number
>>>> ? ? ? ?tst ? ? \tmp, #(1 << 6) ? ? ? ? ? ? ? ? @ Interrupt Pending ?
>>>> ? ? ? ?.endm
>>>>
>>>
>>> I don't like this way. Although more jump instructions are used in irq
>>> entry, it can be extended easier for supporting more silicons.
>>>
>>
>> It really hurt performance a lot. While the difference between MMP series
>> is just the offset of the IRQ_NUM register, it could be calculated in
>> get_irqnr_preamble, which doesn't have to be executed each time
>> in the IRQ handling loop. See arch/arm/kernel/entry-armv.S
>
> get_irqnr_preamble can only carries one base parameter and one tmp
> parameter. base parameter is already used for interrupt control
> register. Only one tmp parameter is left for use. If we want to check
> chip id, one parameter is not enought.
>

I'm a bit confused, but those are just registers that the macro is able to use,
not something as parameters??

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

>>> It really hurt performance a lot. While the difference between MMP series
>>> is just the offset of the IRQ_NUM register, it could be calculated in
>>> get_irqnr_preamble, which doesn't have to be executed each time
>>> in the IRQ handling loop. See arch/arm/kernel/entry-armv.S
>>
>> get_irqnr_preamble can only carries one base parameter and one tmp
>> parameter. base parameter is already used for interrupt control
>> register. Only one tmp parameter is left for use. If we want to check
>> chip id, one parameter is not enought.
>>
>
> I'm a bit confused, but those are just registers that the macro is able to use,
> not something as parameters??
>

I could totaly use two parameters / registers. One is already used for
irq controller. Loading the value from irq controller register needs
one register. Storing camparison needs another register. At least, I
need two registers except irq controller register. But there's no
enough parameter / register in get_irqnr_preamble.

Thanks
Haojian

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

On Mon, Jan 4, 2010 at 11:29 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
>>>> It really hurt performance a lot. While the difference between MMP series
>>>> is just the offset of the IRQ_NUM register, it could be calculated in
>>>> get_irqnr_preamble, which doesn't have to be executed each time
>>>> in the IRQ handling loop. See arch/arm/kernel/entry-armv.S
>>>
>>> get_irqnr_preamble can only carries one base parameter and one tmp
>>> parameter. base parameter is already used for interrupt control
>>> register. Only one tmp parameter is left for use. If we want to check
>>> chip id, one parameter is not enought.
>>>
>>
>> I'm a bit confused, but those are just registers that the macro is able to use,
>> not something as parameters??
>>
>
> I could totaly use two parameters / registers. One is already used for
> irq controller. Loading the value from irq controller register needs
> one register. Storing camparison needs another register. At least, I
> need two registers except irq controller register. But there's no
> enough parameter / register in get_irqnr_preamble.
>

Two solutions:

1. use the first two nibbles 0x58, 0x80, 0x84 to decide if it's mmp1
or mmp2, e.g.

	.macro	get_irqnr_preamble, base, tmp
	mrc	p15, 0, \tmp, c0, c0, 0		@ CPUID
	mov	\tmp, \tmp, lsr #4
	and	\tmp, \tmp, #0xff0
	cmp	\tmp, #0x580
	ldrne	\base, =ICU_AP_IRQ_SEL_INT_NUM
	ldreq	\base, =ICU_MMP2_PJ4_IRQ_SEL
	.endm

	.macro	arch_ret_to_user, tmp1, tmp2
	.endm

	.macro	get_irqnr_and_base, irqnr, irqstat, base, tmp
	ldr	\tmp, [\base]		@ AP INT SEL register
	and	\irqnr, \tmp, #0x3f
	tst	\tmp, #(1 << 6)
	.endm

2. use \base temporarily, which is un-necessary and less preferable:

	.macro	get_irqnr_preamble, base, tmp
	mrc	p15, 0, \tmp, c0, c0, 0		@ CPUID
	mov	\tmp, \tmp, lsr #4
	and	\tmp, \tmp, #0xff0
	and	\tmp, \tmp, #0x00f
	ldr	\base, =0x581
	cmp	\tmp, \base
	ldreq	\base, =ICU_MMP2_PJ4_IRQ_SEL
	mov	\base, #0x840
	cmp	\tmp, \base
	ldreq	\base, =ICU_PXA168_IRQ_SEL
	mov	\base, #0x800
	cmp	\tmp, \base
	ldreq	\base, =ICU_PXA910_IRQ_SEL
	/* panic other wise */
	.endm

	.macro	arch_ret_to_user, tmp1, tmp2
	.endm

	.macro	get_irqnr_and_base, irqnr, irqstat, base, tmp
	ldr	\tmp, [\base]		@ AP INT SEL register
	and	\irqnr, \tmp, #0x3f
	tst	\tmp, #(1 << 6)
	.endm

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

On Sun, Jan 3, 2010 at 11:27 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Mon, Jan 4, 2010 at 11:29 AM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>>>>> It really hurt performance a lot. While the difference between MMP series
>>>>> is just the offset of the IRQ_NUM register, it could be calculated in
>>>>> get_irqnr_preamble, which doesn't have to be executed each time
>>>>> in the IRQ handling loop. See arch/arm/kernel/entry-armv.S
>>>>
>>>> get_irqnr_preamble can only carries one base parameter and one tmp
>>>> parameter. base parameter is already used for interrupt control
>>>> register. Only one tmp parameter is left for use. If we want to check
>>>> chip id, one parameter is not enought.
>>>>
>>>
>>> I'm a bit confused, but those are just registers that the macro is able to use,
>>> not something as parameters??
>>>
>>
>> I could totaly use two parameters / registers. One is already used for
>> irq controller. Loading the value from irq controller register needs
>> one register. Storing camparison needs another register. At least, I
>> need two registers except irq controller register. But there's no
>> enough parameter / register in get_irqnr_preamble.
>>
>
> Two solutions:
>
> 1. use the first two nibbles 0x58, 0x80, 0x84 to decide if it's mmp1
> or mmp2, e.g.
>
> ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
> ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
> ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
> ? ? ? ?and ? ? \tmp, \tmp, #0xff0
> ? ? ? ?cmp ? ? \tmp, #0x580

I have some concern on compressing 12-bit to 8-bit. Maybe it would not
fit chips in future.

> ? ? ? ?ldrne ? \base, =ICU_AP_IRQ_SEL_INT_NUM
> ? ? ? ?ldreq ? \base, =ICU_MMP2_PJ4_IRQ_SEL
> ? ? ? ?.endm
>
> ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
> ? ? ? ?.endm
>
> ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
> ? ? ? ?ldr ? ? \tmp, [\base] ? ? ? ? ? @ AP INT SEL register
> ? ? ? ?and ? ? \irqnr, \tmp, #0x3f
> ? ? ? ?tst ? ? \tmp, #(1 << 6)
> ? ? ? ?.endm
>
It's wrong at here. Whatever we need to access ICU register in
get_irqnr_and_base.

        .macro irq_handler
        get_irqnr_preamble  r5, lr
1:     get_irqnr_and_base  r0, r6, r5, lr
        ...
        bne  asm_do_IRQ

If we just keep accessing ICU register in get_irqnr_preamble, we'll
find lr register used in asm_do_IRQ. When the loop came back to
get_irqnr_and_base(), we'll meet unpredicated value.

So I think that there's some flaw on this optimization.

Thanks
Haojian

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Russell King - ARM Linux]

On Mon, Jan 04, 2010 at 05:15:22AM -0500, Haojian Zhuang wrote:
> On Sun, Jan 3, 2010 at 11:27 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> > ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
> > ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
> > ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
> > ? ? ? ?and ? ? \tmp, \tmp, #0xff0
> > ? ? ? ?cmp ? ? \tmp, #0x580
> 
> I have some concern on compressing 12-bit to 8-bit. Maybe it would not
> fit chips in future.

The spec for immediate constants is: any 32-bit constant, which may be
rotated by an even number of shift places to form a single 8-bit
constant.

So, 0x0ff00000 is legal (0xff rotated left 20).  0x07f800000 is not (0xff
rotated left 19).  0xfc000003 is legal (0xff rotated left 26).

This comes directly from the instruction coding, which is an 8-bit
constant plus a 4-bit shift.

> > ? ? ? ?ldrne ? \base, =ICU_AP_IRQ_SEL_INT_NUM
> > ? ? ? ?ldreq ? \base, =ICU_MMP2_PJ4_IRQ_SEL
> > ? ? ? ?.endm
> >
> > ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
> > ? ? ? ?.endm
> >
> > ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
> > ? ? ? ?ldr ? ? \tmp, [\base] ? ? ? ? ? @ AP INT SEL register
> > ? ? ? ?and ? ? \irqnr, \tmp, #0x3f
> > ? ? ? ?tst ? ? \tmp, #(1 << 6)
> > ? ? ? ?.endm
> >
> It's wrong at here. Whatever we need to access ICU register in
> get_irqnr_and_base.
> 
>         .macro irq_handler
>         get_irqnr_preamble  r5, lr
> 1:     get_irqnr_and_base  r0, r6, r5, lr
>         ...
>         bne  asm_do_IRQ
> 
> If we just keep accessing ICU register in get_irqnr_preamble, we'll
> find lr register used in asm_do_IRQ. When the loop came back to
> get_irqnr_and_base(), we'll meet unpredicated value.

I don't see what you're getting at - Eric only uses the 'tmp' register
as a local temporary in each macro.  It's entirely local to that
macro, and each macro doesn't care what value it had previously.

The first thing that get_irqnr_and_base does is overwrite 'lr' with
the value from the INT SEL register.

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Russell King - ARM Linux]

On Mon, Jan 04, 2010 at 10:49:05AM +0800, Eric Miao wrote:
> On Mon, Jan 4, 2010 at 10:27 AM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
> > get_irqnr_preamble can only carries one base parameter and one tmp
> > parameter. base parameter is already used for interrupt control
> > register. Only one tmp parameter is left for use. If we want to check
> > chip id, one parameter is not enought.
> 
> I'm a bit confused, but those are just registers that the macro is able to use,
> not something as parameters??

Correct - they're for sanity with the rest of the code.  They're a way
of saying "this macro can use these registers", and the rest of the code
which calls the macros really doesn't care what you do with them in the
macro.

Imagine what the situation would be if we didn't do this, and everyone
used their own sets of registers in these macros - it'd quickly become
impossible to safely modify the surrounding code, since we wouldn't
know what registers the macro used.

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

On Mon, Jan 4, 2010 at 5:27 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 04, 2010 at 05:15:22AM -0500, Haojian Zhuang wrote:
>> On Sun, Jan 3, 2010 at 11:27 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> > ? ? ? ?.macro ?get_irqnr_preamble, base, tmp
>> > ? ? ? ?mrc ? ? p15, 0, \tmp, c0, c0, 0 ? ? ? ? @ CPUID
>> > ? ? ? ?mov ? ? \tmp, \tmp, lsr #4
>> > ? ? ? ?and ? ? \tmp, \tmp, #0xff0
>> > ? ? ? ?cmp ? ? \tmp, #0x580
>>
>> I have some concern on compressing 12-bit to 8-bit. Maybe it would not
>> fit chips in future.
>
> The spec for immediate constants is: any 32-bit constant, which may be
> rotated by an even number of shift places to form a single 8-bit
> constant.
>
> So, 0x0ff00000 is legal (0xff rotated left 20). ?0x07f800000 is not (0xff
> rotated left 19). ?0xfc000003 is legal (0xff rotated left 26).
>
> This comes directly from the instruction coding, which is an 8-bit
> constant plus a 4-bit shift.
>
>> > ? ? ? ?ldrne ? \base, =ICU_AP_IRQ_SEL_INT_NUM
>> > ? ? ? ?ldreq ? \base, =ICU_MMP2_PJ4_IRQ_SEL
>> > ? ? ? ?.endm
>> >
>> > ? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
>> > ? ? ? ?.endm
>> >
>> > ? ? ? ?.macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
>> > ? ? ? ?ldr ? ? \tmp, [\base] ? ? ? ? ? @ AP INT SEL register
>> > ? ? ? ?and ? ? \irqnr, \tmp, #0x3f
>> > ? ? ? ?tst ? ? \tmp, #(1 << 6)
>> > ? ? ? ?.endm
>> >
>> It's wrong at here. Whatever we need to access ICU register in
>> get_irqnr_and_base.
>>
>> ? ? ? ? .macro irq_handler
>> ? ? ? ? get_irqnr_preamble ?r5, lr
>> 1: ? ? get_irqnr_and_base ?r0, r6, r5, lr
>> ? ? ? ? ...
>> ? ? ? ? bne ?asm_do_IRQ
>>
>> If we just keep accessing ICU register in get_irqnr_preamble, we'll
>> find lr register used in asm_do_IRQ. When the loop came back to
>> get_irqnr_and_base(), we'll meet unpredicated value.
>
> I don't see what you're getting at - Eric only uses the 'tmp' register
> as a local temporary in each macro. ?It's entirely local to that
> macro, and each macro doesn't care what value it had previously.
>
> The first thing that get_irqnr_and_base does is overwrite 'lr' with
> the value from the INT SEL register.
>

OK. Update the patches.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001--ARM-mmp-support-marvell-MMP2.patch
Type: text/x-patch
Size: 21193 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100104/cfdd9ce8/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002--ARM-mmp-add-device-support-in-mmp2.patch
Type: text/x-patch
Size: 17402 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100104/cfdd9ce8/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003--ARM-mmp-support-flint-development-board.patch
Type: text/x-patch
Size: 3895 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100104/cfdd9ce8/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004--ARM-mmp-add-mmp2-configuration.patch
Type: text/x-patch
Size: 30784 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100104/cfdd9ce8/attachment-0007.bin>

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

> OK. Update the patches.
>

Applied to branch 'mmp2' and made some changes as follows
(please let me know your concerns and help test this):

1. irq-mmp2.c: separate the difference into different irq_chip
and individual demux handler for those MUXed IRQs, naming
is changed a bit as well. Having a single irq_chip and demux
handler isn't performance friendly - having to go through a lengthy
condition branches each time - and that's why irq_chip is invented.

2. merge patch 1, 2, 3 together so mmp2 can build (my build
here complains about no machine_desc defined if flint support
isn't there)

And I'd like you to help co-maintaine MMP2 together with me,
please kindly Ack if you're fine with the patch follows:

commit 2004b15682f0f612b85088d730164e91fca2eec8
Author: Eric Miao <eric.y.miao@gmail.com>
Date:   Tue Jan 5 15:28:26 2010 +0800

    MAINTAINERS: add maintainers for Marvell MMP2 (aka ARMADA610) support

    Cc: Haojian Zhuang <haojian.zhuang@marvell.com>
    Signed-off-by: Eric Miao <eric.y.miao@gmail.com>

diff --git a/MAINTAINERS b/MAINTAINERS
index 66f5f7d..33978ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4439,6 +4439,13 @@ L:	linux-arm-kernel at lists.infradead.org
(moderated for non-subscribers)
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
 S:	Maintained

+MMP2 SUPPORT (aka ARMADA610)
+M:	Haojian Zhuang <haojian.zhuang@marvell.com>
+M:	Eric Miao <eric.y.miao@gmail.com>
+L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
+S:	Maintained
+
 PXA MMCI DRIVER
 S:	Orphan

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Haojian Zhuang]

On Tue, Jan 5, 2010 at 2:30 AM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> OK. Update the patches.
>>
>
> Applied to branch 'mmp2' and made some changes as follows
> (please let me know your concerns and help test this):
>
> 1. irq-mmp2.c: separate the difference into different irq_chip
> and individual demux handler for those MUXed IRQs, naming
> is changed a bit as well. Having a single irq_chip and demux
> handler isn't performance friendly - having to go through a lengthy
> condition branches each time - and that's why irq_chip is invented.
>
> 2. merge patch 1, 2, 3 together so mmp2 can build (my build
> here complains about no machine_desc defined if flint support
> isn't there)
>
> And I'd like you to help co-maintaine MMP2 together with me,
> please kindly Ack if you're fine with the patch follows:
>
> commit 2004b15682f0f612b85088d730164e91fca2eec8
> Author: Eric Miao <eric.y.miao@gmail.com>
> Date: ? Tue Jan 5 15:28:26 2010 +0800
>
> ? ?MAINTAINERS: add maintainers for Marvell MMP2 (aka ARMADA610) support
>
> ? ?Cc: Haojian Zhuang <haojian.zhuang@marvell.com>
> ? ?Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 66f5f7d..33978ed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4439,6 +4439,13 @@ L: ? ? ? linux-arm-kernel at lists.infradead.org
> (moderated for non-subscribers)
> ?T: ? ? git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
> ?S: ? ? Maintained
>
> +MMP2 SUPPORT (aka ARMADA610)
> +M: ? ? Haojian Zhuang <haojian.zhuang@marvell.com>
> +M: ? ? Eric Miao <eric.y.miao@gmail.com>
> +L: ? ? linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
> +T: ? ? git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
> +S: ? ? Maintained
> +
> ?PXA MMCI DRIVER
> ?S: ? ? Orphan
>

Acked.

Best Regards
Haojian

[patch 2/4] [ARM] mmp: support marvell ARMADA610

[Eric Miao]

On Tue, Jan 5, 2010 at 5:13 PM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
> On Tue, Jan 5, 2010 at 2:30 AM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>> OK. Update the patches.
>>>
>>
>> Applied to branch 'mmp2' and made some changes as follows
>> (please let me know your concerns and help test this):
>>
>> 1. irq-mmp2.c: separate the difference into different irq_chip
>> and individual demux handler for those MUXed IRQs, naming
>> is changed a bit as well. Having a single irq_chip and demux
>> handler isn't performance friendly - having to go through a lengthy
>> condition branches each time - and that's why irq_chip is invented.
>>
>> 2. merge patch 1, 2, 3 together so mmp2 can build (my build
>> here complains about no machine_desc defined if flint support
>> isn't there)
>>
>> And I'd like you to help co-maintaine MMP2 together with me,
>> please kindly Ack if you're fine with the patch follows:
>>
>> commit 2004b15682f0f612b85088d730164e91fca2eec8
>> Author: Eric Miao <eric.y.miao@gmail.com>
>> Date: ? Tue Jan 5 15:28:26 2010 +0800
>>
>> ? ?MAINTAINERS: add maintainers for Marvell MMP2 (aka ARMADA610) support
>>
>> ? ?Cc: Haojian Zhuang <haojian.zhuang@marvell.com>
>> ? ?Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 66f5f7d..33978ed 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4439,6 +4439,13 @@ L: ? ? ? linux-arm-kernel at lists.infradead.org
>> (moderated for non-subscribers)
>> ?T: ? ? git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
>> ?S: ? ? Maintained
>>
>> +MMP2 SUPPORT (aka ARMADA610)
>> +M: ? ? Haojian Zhuang <haojian.zhuang@marvell.com>
>> +M: ? ? Eric Miao <eric.y.miao@gmail.com>
>> +L: ? ? linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
>> +T: ? ? git git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git
>> +S: ? ? Maintained
>> +
>> ?PXA MMCI DRIVER
>> ?S: ? ? Orphan
>>
>
> Acked.
>

OK, thanks