Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[reinaldo rojas]

Enviado desde mi iPad

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Mike Frysinger]

On Thu, Apr 7, 2011 at 14:39, RONETIX - Asen Dimov wrote:
> On 11/30/2010 09:25 PM, Daniel Walker wrote:
>> This driver adds a basic console that uses the arm JTAG
>> DCC to transfer data back and forth. It has support for
>> ARMv6 and ARMv7.
>>
>> This console is created under the HVC driver, and should be named
>> /dev/hvcX (or /dev/hvc0 for example).
>>
>>  drivers/char/Kconfig   |    9 +++
>>  drivers/char/Makefile  |    1 +
>>  drivers/char/hvc_dcc.c |  133
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>
> ...
>
> this DCC driver implements "one channel", but what about implementing
> "multiple channels". For example reserve few(3) bits for channel number,
> and two bits for carried data, then fill the rest bytes with with some data
> and send the word(32 bits) over DCC. On the Linux side writing on /dev/hvcX
> puts the number X as channel number, and on the other side the CPU
> emulator gets the data and redistribute it to TCP/IP socket.
>
> I have started write some code implementing this. Are there any one
> interested
> in this multiple channels, and are there any one started to work on it?

this sort of multiplexing of the data stream sounds like the job for
userspace ?  or maybe a line discipline ?  inserting structured data
into the kernel driver doesnt sound right to me ...
-mike

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Mike Frysinger]

On Thu, Apr 7, 2011 at 14:39, RONETIX - Asen Dimov wrote:
> On 11/30/2010 09:25 PM, Daniel Walker wrote:
>> This driver adds a basic console that uses the arm JTAG
>> DCC to transfer data back and forth. It has support for
>> ARMv6 and ARMv7.
>>
>> This console is created under the HVC driver, and should be named
>> /dev/hvcX (or /dev/hvc0 for example).
>>
>> =C2=A0drivers/char/Kconfig =C2=A0 | =C2=A0 =C2=A09 +++
>> =C2=A0drivers/char/Makefile =C2=A0| =C2=A0 =C2=A01 +
>> =C2=A0drivers/char/hvc_dcc.c | =C2=A0133
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>
> ...
>
> this DCC driver implements "one channel", but what about implementing
> "multiple channels". For example reserve few(3) bits for channel number,
> and two bits for carried data, then fill the rest bytes with with some da=
ta
> and send the word(32 bits) over DCC. On the Linux side writing on /dev/hv=
cX
> puts the number X as channel number, and on the other side the CPU
> emulator gets the data and redistribute it to TCP/IP socket.
>
> I have started write some code implementing this. Are there any one
> interested
> in this multiple channels, and are there any one started to work on it?

this sort of multiplexing of the data stream sounds like the job for
userspace ?  or maybe a line discipline ?  inserting structured data
into the kernel driver doesnt sound right to me ...
-mike

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[RONETIX - Asen Dimov]

Hello,

On 11/30/2010 09:25 PM, Daniel Walker wrote:
> This driver adds a basic console that uses the arm JTAG
> DCC to transfer data back and forth. It has support for
> ARMv6 and ARMv7.
>
> This console is created under the HVC driver, and should be named
> /dev/hvcX (or /dev/hvc0 for example).
>
> Cc: Tony Lindgren<tony@atomide.com>
> Cc: Arnd Bergmann<arnd@arndb.de>
> Cc: Nicolas Pitre<nico@fluxnic.net>
> Cc: Greg Kroah-Hartman<gregkh@suse.de>
> Cc: Mike Frysinger<vapier@gentoo.org>
> Signed-off-by: Daniel Walker<dwalker@codeaurora.org>
> ---
>   drivers/char/Kconfig   |    9 +++
>   drivers/char/Makefile  |    1 +
>   drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++
...

this DCC driver implements "one channel", but what about implementing
"multiple channels". For example reserve few(3) bits for channel number,
and two bits for carried data, then fill the rest bytes with with some data
and send the word(32 bits) over DCC. On the Linux side writing on /dev/hvcX
puts the number X as channel number, and on the other side the CPU
emulator gets the data and redistribute it to TCP/IP socket.

I have started write some code implementing this. Are there any one 
interested
in this multiple channels, and are there any one started to work on it?


Regards,
Asen

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[RONETIX - Asen Dimov]

Hello,

On 11/30/2010 09:25 PM, Daniel Walker wrote:
> This driver adds a basic console that uses the arm JTAG
> DCC to transfer data back and forth. It has support for
> ARMv6 and ARMv7.
>
> This console is created under the HVC driver, and should be named
> /dev/hvcX (or /dev/hvc0 for example).
>
> Cc: Tony Lindgren<tony@atomide.com>
> Cc: Arnd Bergmann<arnd@arndb.de>
> Cc: Nicolas Pitre<nico@fluxnic.net>
> Cc: Greg Kroah-Hartman<gregkh@suse.de>
> Cc: Mike Frysinger<vapier@gentoo.org>
> Signed-off-by: Daniel Walker<dwalker@codeaurora.org>
> ---
>   drivers/char/Kconfig   |    9 +++
>   drivers/char/Makefile  |    1 +
>   drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++
...

this DCC driver implements "one channel", but what about implementing
"multiple channels". For example reserve few(3) bits for channel number,
and two bits for carried data, then fill the rest bytes with with some data
and send the word(32 bits) over DCC. On the Linux side writing on /dev/hvcX
puts the number X as channel number, and on the other side the CPU
emulator gets the data and redistribute it to TCP/IP socket.

I have started write some code implementing this. Are there any one 
interested
in this multiple channels, and are there any one started to work on it?


Regards,
Asen

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 01/14/2011 11:19 AM, Tony Lindgren wrote:
> * Stephen Boyd <sboyd@codeaurora.org> [101207 11:00]:
>> On 12/01/2010 12:20 PM, Stephen Boyd wrote:
>>> Definitely for TX since it seems like a redundant loop, but I agree RX
>>> code has changed. Instead of
>>>
>>> If RX buffer full
>>> Poll for RX buffer full
>>> Read character from RX buffer
>>>
>>> we would have
>>>
>>> If RX buffer full
>>> Read character from RX buffer
>>>
>>> which doesn't seem all that different assuming the RX buffer doesn't go
>>> from full to empty between the If and Poll steps. Hopefully Tony knows more.
>>>
>>
>> Tony, any thoughts?
>
> Sorry for the delay, looks like I'm only 1 month behind with email..
> Sounds like it should work to me. I can try it out if you point me
> to a patch.

I think you acked the patches to make this change. You can test them out
by applying the "hvc_dcc cleanups and fixes" patches (Message-Id:
<1292875718-7980-1-git-send-email-sboyd@codeaurora.org>) on top of this
patch. They were sent as a reply to this thread.

Stephen

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 01/14/2011 11:19 AM, Tony Lindgren wrote:
> * Stephen Boyd <sboyd@codeaurora.org> [101207 11:00]:
>> On 12/01/2010 12:20 PM, Stephen Boyd wrote:
>>> Definitely for TX since it seems like a redundant loop, but I agree RX
>>> code has changed. Instead of
>>>
>>> If RX buffer full
>>> Poll for RX buffer full
>>> Read character from RX buffer
>>>
>>> we would have
>>>
>>> If RX buffer full
>>> Read character from RX buffer
>>>
>>> which doesn't seem all that different assuming the RX buffer doesn't go
>>> from full to empty between the If and Poll steps. Hopefully Tony knows more.
>>>
>>
>> Tony, any thoughts?
>
> Sorry for the delay, looks like I'm only 1 month behind with email..
> Sounds like it should work to me. I can try it out if you point me
> to a patch.

I think you acked the patches to make this change. You can test them out
by applying the "hvc_dcc cleanups and fixes" patches (Message-Id:
<1292875718-7980-1-git-send-email-sboyd@codeaurora.org>) on top of this
patch. They were sent as a reply to this thread.

Stephen

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Tony Lindgren]

* Stephen Boyd <sboyd@codeaurora.org> [101207 11:00]:
> On 12/01/2010 12:20 PM, Stephen Boyd wrote:
> > Definitely for TX since it seems like a redundant loop, but I agree RX
> > code has changed. Instead of
> >
> > If RX buffer full
> > Poll for RX buffer full
> > Read character from RX buffer
> >
> > we would have
> >
> > If RX buffer full
> > Read character from RX buffer
> >
> > which doesn't seem all that different assuming the RX buffer doesn't go
> > from full to empty between the If and Poll steps. Hopefully Tony knows more.
> >
> 
> Tony, any thoughts?

Sorry for the delay, looks like I'm only 1 month behind with email..
Sounds like it should work to me. I can try it out if you point me
to a patch.

Regards,

Tony

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Tony Lindgren]

* Stephen Boyd <sboyd@codeaurora.org> [101207 11:00]:
> On 12/01/2010 12:20 PM, Stephen Boyd wrote:
> > Definitely for TX since it seems like a redundant loop, but I agree RX
> > code has changed. Instead of
> >
> > If RX buffer full
> > Poll for RX buffer full
> > Read character from RX buffer
> >
> > we would have
> >
> > If RX buffer full
> > Read character from RX buffer
> >
> > which doesn't seem all that different assuming the RX buffer doesn't go
> > from full to empty between the If and Poll steps. Hopefully Tony knows more.
> >
> 
> Tony, any thoughts?

Sorry for the delay, looks like I'm only 1 month behind with email..
Sounds like it should work to me. I can try it out if you point me
to a patch.

Regards,

Tony

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 12/01/2010 12:20 PM, Stephen Boyd wrote:
> Definitely for TX since it seems like a redundant loop, but I agree RX
> code has changed. Instead of
>
> If RX buffer full
> Poll for RX buffer full
> Read character from RX buffer
>
> we would have
>
> If RX buffer full
> Read character from RX buffer
>
> which doesn't seem all that different assuming the RX buffer doesn't go
> from full to empty between the If and Poll steps. Hopefully Tony knows more.
>

Tony, any thoughts?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 12/01/2010 12:20 PM, Stephen Boyd wrote:
> Definitely for TX since it seems like a redundant loop, but I agree RX
> code has changed. Instead of
>
> If RX buffer full
> Poll for RX buffer full
> Read character from RX buffer
>
> we would have
>
> If RX buffer full
> Read character from RX buffer
>
> which doesn't seem all that different assuming the RX buffer doesn't go
> from full to empty between the If and Poll steps. Hopefully Tony knows more.
>

Tony, any thoughts?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 12/01/2010 10:54 AM, Daniel Walker wrote:
> Are you talking about __dcc_getstatus only? I don't think adding
> volatile is going to hurt anything, if not having it causes problems.
>

Just marking __dcc_getstatus volatile gives me

00000038 <hvc_dcc_get_chars>:
  38:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  3c:   1afffffd        bne     38 <hvc_dcc_get_chars>
  40:   ee103e15        mrc     14, 0, r3, cr0, cr5, {0}
  44:   e3a00000        mov     r0, #0  ; 0x0
  48:   e6ef3073        uxtb    r3, r3
  4c:   ea000004        b       64 <hvc_dcc_get_chars+0x2c>
  50:   ee10ce11        mrc     14, 0, ip, cr0, cr1, {0}
  54:   e31c0101        tst     ip, #1073741824 ; 0x40000000
  58:   012fff1e        bxeq    lr
  5c:   e7c13000        strb    r3, [r1, r0]
  60:   e2800001        add     r0, r0, #1      ; 0x1
  64:   e1500002        cmp     r0, r2
  68:   bafffff8        blt     50 <hvc_dcc_get_chars+0x18>
  6c:   e12fff1e        bx      lr

Seems the compiler keeps the value of __dcc_getchar() in r3 for the
duration of the loop. So we need to mark that one volatile too. I don't
think __dcc_putchar() needs to be marked volatile but it probably
doesn't hurt.

>
> We could maybe drop the looping for TX, but RX has no C based looping
> even tho for v7 it's recommended that we loop (presumably for v6 it's
> not recommended).
>

Definitely for TX since it seems like a redundant loop, but I agree RX
code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

we would have

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go
from full to empty between the If and Poll steps. Hopefully Tony knows more.

> Like this?
>
> 	for (i = 0; i < count; ++i) {
>
> 		if (__dcc_getstatus() & DCC_STATUS_RX)
> 			buf[i] = __dcc_getchar();
> 		else
> 			break;
> 	}
>
> It's a micro clean up I guess ..

Yes, it's much clearer that way.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 12/01/2010 10:54 AM, Daniel Walker wrote:
> Are you talking about __dcc_getstatus only? I don't think adding
> volatile is going to hurt anything, if not having it causes problems.
>

Just marking __dcc_getstatus volatile gives me

00000038 <hvc_dcc_get_chars>:
  38:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  3c:   1afffffd        bne     38 <hvc_dcc_get_chars>
  40:   ee103e15        mrc     14, 0, r3, cr0, cr5, {0}
  44:   e3a00000        mov     r0, #0  ; 0x0
  48:   e6ef3073        uxtb    r3, r3
  4c:   ea000004        b       64 <hvc_dcc_get_chars+0x2c>
  50:   ee10ce11        mrc     14, 0, ip, cr0, cr1, {0}
  54:   e31c0101        tst     ip, #1073741824 ; 0x40000000
  58:   012fff1e        bxeq    lr
  5c:   e7c13000        strb    r3, [r1, r0]
  60:   e2800001        add     r0, r0, #1      ; 0x1
  64:   e1500002        cmp     r0, r2
  68:   bafffff8        blt     50 <hvc_dcc_get_chars+0x18>
  6c:   e12fff1e        bx      lr

Seems the compiler keeps the value of __dcc_getchar() in r3 for the
duration of the loop. So we need to mark that one volatile too. I don't
think __dcc_putchar() needs to be marked volatile but it probably
doesn't hurt.

>
> We could maybe drop the looping for TX, but RX has no C based looping
> even tho for v7 it's recommended that we loop (presumably for v6 it's
> not recommended).
>

Definitely for TX since it seems like a redundant loop, but I agree RX
code has changed. Instead of

If RX buffer full
Poll for RX buffer full
Read character from RX buffer

we would have

If RX buffer full
Read character from RX buffer

which doesn't seem all that different assuming the RX buffer doesn't go
from full to empty between the If and Poll steps. Hopefully Tony knows more.

> Like this?
>
> 	for (i = 0; i < count; ++i) {
>
> 		if (__dcc_getstatus() & DCC_STATUS_RX)
> 			buf[i] = __dcc_getchar();
> 		else
> 			break;
> 	}
>
> It's a micro clean up I guess ..

Yes, it's much clearer that way.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Greg KH]

On Wed, Dec 01, 2010 at 10:54:56AM -0800, Daniel Walker wrote:
> On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> > On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > > @@ -682,6 +682,15 @@ config HVC_UDBG
> > >         select HVC_DRIVER
> > >         default n
> > >  
> > > +config HVC_DCC
> > > +       bool "ARM JTAG DCC console"
> > > +       depends on ARM
> > > +       select HVC_DRIVER
> > > +       help
> > > +         This console uses the JTAG DCC on ARM to create a console under the HVC
> > 
> > Looks like you added one too many spaces for indent here.
> 
> The first line is fine, but the other two might need an extra space.

For this, or any other changes you want, I'll gladly take a follow-on
patch as this one is already in my tty-next tree.

thanks,

greg k-h

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Greg KH]

On Wed, Dec 01, 2010 at 10:54:56AM -0800, Daniel Walker wrote:
> On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> > On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > > @@ -682,6 +682,15 @@ config HVC_UDBG
> > >         select HVC_DRIVER
> > >         default n
> > >  
> > > +config HVC_DCC
> > > +       bool "ARM JTAG DCC console"
> > > +       depends on ARM
> > > +       select HVC_DRIVER
> > > +       help
> > > +         This console uses the JTAG DCC on ARM to create a console under the HVC
> > 
> > Looks like you added one too many spaces for indent here.
> 
> The first line is fine, but the other two might need an extra space.

For this, or any other changes you want, I'll gladly take a follow-on
patch as this one is already in my tty-next tree.

thanks,

greg k-h

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Daniel Walker]

On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > @@ -682,6 +682,15 @@ config HVC_UDBG
> >         select HVC_DRIVER
> >         default n
> >  
> > +config HVC_DCC
> > +       bool "ARM JTAG DCC console"
> > +       depends on ARM
> > +       select HVC_DRIVER
> > +       help
> > +         This console uses the JTAG DCC on ARM to create a console under the HVC
> 
> Looks like you added one too many spaces for indent here.

The first line is fine, but the other two might need an extra space.

> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> > new file mode 100644
> > index 0000000..6470f63
> > --- /dev/null
> > +++ b/drivers/char/hvc_dcc.c
> > +static inline u32 __dcc_getstatus(void)
> > +{
> > +	u32 __ret;
> > +
> > +	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
> > +		: "=r" (__ret) : : "cc");
> > +
> > +	return __ret;
> > +}
> 
> Without marking this asm volatile my compiler decides it can cache the
> value of __ret in a register and then check the value of it continually
> in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
> fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).
> 
> 
> 00000000 <hvc_dcc_put_chars>:
>    0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
>    4:   e3a0c000        mov     ip, #0  ; 0x0
>    8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
>    c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
>   10:   e3530000        cmp     r3, #0  ; 0x0
>   14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
>   18:   e7d1000c        ldrb    r0, [r1, ip]
>   1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
>   20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
>   24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
>   28:   e28cc001        add     ip, ip, #1      ; 0x1
>   2c:   e15c0002        cmp     ip, r2
>   30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
>   34:   e1a00002        mov     r0, r2
>   38:   e12fff1e        bx      lr
> 
> As you can see, the value of the mrc is checked against DCC_STATUS_TX
> (bit 29) and then stored in r3 for later use. Marking this volatile
> produces the following:
> 
> 00000000 <hvc_dcc_put_chars>:
>    0:   e3a03000        mov     r3, #0  ; 0x0
>    4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
>    8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
>    c:   e3100202        tst     r0, #536870912  ; 0x20000000
>   10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
>   14:   e7d10003        ldrb    r0, [r1, r3]
>   18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
>   1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
>   20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
>   24:   e2833001        add     r3, r3, #1      ; 0x1
>   28:   e1530002        cmp     r3, r2
>   2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
>   30:   e1a00002        mov     r0, r2
>   34:   e12fff1e        bx      lr
> 
> which looks better.
> 
> I marked all the asm in this driver as volatile. Is that correct?

Are you talking about __dcc_getstatus only? I don't think adding
volatile is going to hurt anything, if not having it causes problems.

> > +#if defined(CONFIG_CPU_V7)
> > +static inline char __dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
> > +			bne get_wait                                       \n\
> > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#else
> > +static inline char __dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c));
> > +
> > +	return __c;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +static inline void __dcc_putchar(char c)
> > +{
> > +	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
> > +			bcs put_wait                              \n\
> > +			mcr p14, 0, %0, c0, c5, 0                   "
> > +	: : "r" (c) : "cc");
> > +}
> > +#else
> > +static inline void __dcc_putchar(char c)
> > +{
> > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > +		: /* no output register */
> > +		: "r" (c));
> > +}
> > +#endif
> > +
> 
> I don't think both the v7 and v6 functions are necessary. It seems I can
> get away with just the second version of __dcc_(get|put)char() on a v7.
> The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
> condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
> also do the same thing if I read the manuals right. The test in the
> inline assembly is saying, wait for a character to be ready or wait for
> a character to be read then actually write a character or read one. The
> code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
> slightly different form. Instead of getting the status bits put into the
> condition codes and looping with bne or bcs it will read the register,
> and it with bit 29 or bit 28 to see if it should wait and then continue
> with the writing/reading. I think you can just drop the looping for the
> v7 version of the functions and have this driver work on v6 and v7.

We could maybe drop the looping for TX, but RX has no C based looping
even tho for v7 it's recommended that we loop (presumably for v6 it's
not recommended).

> Alternatively, you can make some function that says tx buffer is empty,
> rx buffer is full or something but I don't see how saving a couple
> instructions buys us much when we can have one driver for v6 and v7.
> 
> I see that Tony Lindgren modified the DCC macros for v7 in commit
> 200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
> sure why though, since it seems that a v6 and a v7 should really do the
> same thing by waiting for the buffers to be ready before filling them or
> reading them. Which probably means we can get low-level dcc debugging on
> all targets if I'm not mistaken.

This is primarily why these function look they way they do. I'd like to
hear from Tony, because he apparent didn't think they did the same
thing.

> > +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		while (__dcc_getstatus() & DCC_STATUS_TX)
> > +			cpu_relax();
> > +
> > +		__dcc_putchar((char)(buf[i] & 0xFF));
> 
> Is this & 0xFF and cast to char unnecessary? buf is a char array, and
> chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

Yeah, doesn't seem like it.

> > +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; ++i) {
> > +		int c = -1;
> > +
> > +		if (__dcc_getstatus() & DCC_STATUS_RX)
> > +			c = __dcc_getchar();
> > +		if (c < 0)
> > +			break;
> > +		buf[i] = c;
> > +	}
> 
> I think this for loop can be simplified. __dcc_getchar() returns a char.
> It never returns -1, so the check for c < 0 can't be taken if
> __dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
> loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
> can have a simple if-else and assign buf[i] in the if branch and break
> in the else branch.
> 

Like this?

	for (i = 0; i < count; ++i) {

		if (__dcc_getstatus() & DCC_STATUS_RX)
			buf[i] = __dcc_getchar();
		else
			break;
	}

It's a micro clean up I guess ..

Daniel
-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Daniel Walker]

On Tue, 2010-11-30 at 21:30 -0800, Stephen Boyd wrote:
> On 11/30/2010 11:25 AM, Daniel Walker wrote:
> > @@ -682,6 +682,15 @@ config HVC_UDBG
> >         select HVC_DRIVER
> >         default n
> >  
> > +config HVC_DCC
> > +       bool "ARM JTAG DCC console"
> > +       depends on ARM
> > +       select HVC_DRIVER
> > +       help
> > +         This console uses the JTAG DCC on ARM to create a console under the HVC
> 
> Looks like you added one too many spaces for indent here.

The first line is fine, but the other two might need an extra space.

> > diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> > new file mode 100644
> > index 0000000..6470f63
> > --- /dev/null
> > +++ b/drivers/char/hvc_dcc.c
> > +static inline u32 __dcc_getstatus(void)
> > +{
> > +	u32 __ret;
> > +
> > +	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
> > +		: "=r" (__ret) : : "cc");
> > +
> > +	return __ret;
> > +}
> 
> Without marking this asm volatile my compiler decides it can cache the
> value of __ret in a register and then check the value of it continually
> in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
> fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).
> 
> 
> 00000000 <hvc_dcc_put_chars>:
>    0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
>    4:   e3a0c000        mov     ip, #0  ; 0x0
>    8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
>    c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
>   10:   e3530000        cmp     r3, #0  ; 0x0
>   14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
>   18:   e7d1000c        ldrb    r0, [r1, ip]
>   1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
>   20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
>   24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
>   28:   e28cc001        add     ip, ip, #1      ; 0x1
>   2c:   e15c0002        cmp     ip, r2
>   30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
>   34:   e1a00002        mov     r0, r2
>   38:   e12fff1e        bx      lr
> 
> As you can see, the value of the mrc is checked against DCC_STATUS_TX
> (bit 29) and then stored in r3 for later use. Marking this volatile
> produces the following:
> 
> 00000000 <hvc_dcc_put_chars>:
>    0:   e3a03000        mov     r3, #0  ; 0x0
>    4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
>    8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
>    c:   e3100202        tst     r0, #536870912  ; 0x20000000
>   10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
>   14:   e7d10003        ldrb    r0, [r1, r3]
>   18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
>   1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
>   20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
>   24:   e2833001        add     r3, r3, #1      ; 0x1
>   28:   e1530002        cmp     r3, r2
>   2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
>   30:   e1a00002        mov     r0, r2
>   34:   e12fff1e        bx      lr
> 
> which looks better.
> 
> I marked all the asm in this driver as volatile. Is that correct?

Are you talking about __dcc_getstatus only? I don't think adding
volatile is going to hurt anything, if not having it causes problems.

> > +#if defined(CONFIG_CPU_V7)
> > +static inline char __dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
> > +			bne get_wait                                       \n\
> > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#else
> > +static inline char __dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c));
> > +
> > +	return __c;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +static inline void __dcc_putchar(char c)
> > +{
> > +	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
> > +			bcs put_wait                              \n\
> > +			mcr p14, 0, %0, c0, c5, 0                   "
> > +	: : "r" (c) : "cc");
> > +}
> > +#else
> > +static inline void __dcc_putchar(char c)
> > +{
> > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > +		: /* no output register */
> > +		: "r" (c));
> > +}
> > +#endif
> > +
> 
> I don't think both the v7 and v6 functions are necessary. It seems I can
> get away with just the second version of __dcc_(get|put)char() on a v7.
> The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
> condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
> also do the same thing if I read the manuals right. The test in the
> inline assembly is saying, wait for a character to be ready or wait for
> a character to be read then actually write a character or read one. The
> code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
> slightly different form. Instead of getting the status bits put into the
> condition codes and looping with bne or bcs it will read the register,
> and it with bit 29 or bit 28 to see if it should wait and then continue
> with the writing/reading. I think you can just drop the looping for the
> v7 version of the functions and have this driver work on v6 and v7.

We could maybe drop the looping for TX, but RX has no C based looping
even tho for v7 it's recommended that we loop (presumably for v6 it's
not recommended).

> Alternatively, you can make some function that says tx buffer is empty,
> rx buffer is full or something but I don't see how saving a couple
> instructions buys us much when we can have one driver for v6 and v7.
> 
> I see that Tony Lindgren modified the DCC macros for v7 in commit
> 200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
> sure why though, since it seems that a v6 and a v7 should really do the
> same thing by waiting for the buffers to be ready before filling them or
> reading them. Which probably means we can get low-level dcc debugging on
> all targets if I'm not mistaken.

This is primarily why these function look they way they do. I'd like to
hear from Tony, because he apparent didn't think they did the same
thing.

> > +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		while (__dcc_getstatus() & DCC_STATUS_TX)
> > +			cpu_relax();
> > +
> > +		__dcc_putchar((char)(buf[i] & 0xFF));
> 
> Is this & 0xFF and cast to char unnecessary? buf is a char array, and
> chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

Yeah, doesn't seem like it.

> > +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; ++i) {
> > +		int c = -1;
> > +
> > +		if (__dcc_getstatus() & DCC_STATUS_RX)
> > +			c = __dcc_getchar();
> > +		if (c < 0)
> > +			break;
> > +		buf[i] = c;
> > +	}
> 
> I think this for loop can be simplified. __dcc_getchar() returns a char.
> It never returns -1, so the check for c < 0 can't be taken if
> __dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
> loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
> can have a simple if-else and assign buf[i] in the if branch and break
> in the else branch.
> 

Like this?

	for (i = 0; i < count; ++i) {

		if (__dcc_getstatus() & DCC_STATUS_RX)
			buf[i] = __dcc_getchar();
		else
			break;
	}

It's a micro clean up I guess ..

Daniel
-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 11/30/2010 11:25 AM, Daniel Walker wrote:
> @@ -682,6 +682,15 @@ config HVC_UDBG
>         select HVC_DRIVER
>         default n
>  
> +config HVC_DCC
> +       bool "ARM JTAG DCC console"
> +       depends on ARM
> +       select HVC_DRIVER
> +       help
> +         This console uses the JTAG DCC on ARM to create a console under the HVC

Looks like you added one too many spaces for indent here.

> diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> new file mode 100644
> index 0000000..6470f63
> --- /dev/null
> +++ b/drivers/char/hvc_dcc.c
> +static inline u32 __dcc_getstatus(void)
> +{
> +	u32 __ret;
> +
> +	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
> +		: "=r" (__ret) : : "cc");
> +
> +	return __ret;
> +}

Without marking this asm volatile my compiler decides it can cache the
value of __ret in a register and then check the value of it continually
in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).


00000000 <hvc_dcc_put_chars>:
   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX
(bit 29) and then stored in r3 for later use. Marking this volatile
produces the following:

00000000 <hvc_dcc_put_chars>:
   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better.

I marked all the asm in this driver as volatile. Is that correct?

> +#if defined(CONFIG_CPU_V7)
> +static inline char __dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
> +			bne get_wait                                       \n\
> +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#else
> +static inline char __dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c));
> +
> +	return __c;
> +}
> +#endif
> +
> +#if defined(CONFIG_CPU_V7)
> +static inline void __dcc_putchar(char c)
> +{
> +	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
> +			bcs put_wait                              \n\
> +			mcr p14, 0, %0, c0, c5, 0                   "
> +	: : "r" (c) : "cc");
> +}
> +#else
> +static inline void __dcc_putchar(char c)
> +{
> +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> +		: /* no output register */
> +		: "r" (c));
> +}
> +#endif
> +

I don't think both the v7 and v6 functions are necessary. It seems I can
get away with just the second version of __dcc_(get|put)char() on a v7.
The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
also do the same thing if I read the manuals right. The test in the
inline assembly is saying, wait for a character to be ready or wait for
a character to be read then actually write a character or read one. The
code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
slightly different form. Instead of getting the status bits put into the
condition codes and looping with bne or bcs it will read the register,
and it with bit 29 or bit 28 to see if it should wait and then continue
with the writing/reading. I think you can just drop the looping for the
v7 version of the functions and have this driver work on v6 and v7.
Alternatively, you can make some function that says tx buffer is empty,
rx buffer is full or something but I don't see how saving a couple
instructions buys us much when we can have one driver for v6 and v7.

I see that Tony Lindgren modified the DCC macros for v7 in commit
200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
sure why though, since it seems that a v6 and a v7 should really do the
same thing by waiting for the buffers to be ready before filling them or
reading them. Which probably means we can get low-level dcc debugging on
all targets if I'm not mistaken.

> +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		while (__dcc_getstatus() & DCC_STATUS_TX)
> +			cpu_relax();
> +
> +		__dcc_putchar((char)(buf[i] & 0xFF));

Is this & 0xFF and cast to char unnecessary? buf is a char array, and
chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

> +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; ++i) {
> +		int c = -1;
> +
> +		if (__dcc_getstatus() & DCC_STATUS_RX)
> +			c = __dcc_getchar();
> +		if (c < 0)
> +			break;
> +		buf[i] = c;
> +	}

I think this for loop can be simplified. __dcc_getchar() returns a char.
It never returns -1, so the check for c < 0 can't be taken if
__dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
can have a simple if-else and assign buf[i] in the if branch and break
in the else branch.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Stephen Boyd]

On 11/30/2010 11:25 AM, Daniel Walker wrote:
> @@ -682,6 +682,15 @@ config HVC_UDBG
>         select HVC_DRIVER
>         default n
>  
> +config HVC_DCC
> +       bool "ARM JTAG DCC console"
> +       depends on ARM
> +       select HVC_DRIVER
> +       help
> +         This console uses the JTAG DCC on ARM to create a console under the HVC

Looks like you added one too many spaces for indent here.

> diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
> new file mode 100644
> index 0000000..6470f63
> --- /dev/null
> +++ b/drivers/char/hvc_dcc.c
> +static inline u32 __dcc_getstatus(void)
> +{
> +	u32 __ret;
> +
> +	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
> +		: "=r" (__ret) : : "cc");
> +
> +	return __ret;
> +}

Without marking this asm volatile my compiler decides it can cache the
value of __ret in a register and then check the value of it continually
in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).


00000000 <hvc_dcc_put_chars>:
   0:   ee103e11        mrc     14, 0, r3, cr0, cr1, {0}
   4:   e3a0c000        mov     ip, #0  ; 0x0
   8:   e2033202        and     r3, r3, #536870912      ; 0x20000000
   c:   ea000006        b       2c <hvc_dcc_put_chars+0x2c>
  10:   e3530000        cmp     r3, #0  ; 0x0
  14:   1afffffd        bne     10 <hvc_dcc_put_chars+0x10>
  18:   e7d1000c        ldrb    r0, [r1, ip]
  1c:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  20:   2afffffd        bcs     1c <hvc_dcc_put_chars+0x1c>
  24:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  28:   e28cc001        add     ip, ip, #1      ; 0x1
  2c:   e15c0002        cmp     ip, r2
  30:   bafffff6        blt     10 <hvc_dcc_put_chars+0x10>
  34:   e1a00002        mov     r0, r2
  38:   e12fff1e        bx      lr

As you can see, the value of the mrc is checked against DCC_STATUS_TX
(bit 29) and then stored in r3 for later use. Marking this volatile
produces the following:

00000000 <hvc_dcc_put_chars>:
   0:   e3a03000        mov     r3, #0  ; 0x0
   4:   ea000007        b       28 <hvc_dcc_put_chars+0x28>
   8:   ee100e11        mrc     14, 0, r0, cr0, cr1, {0}
   c:   e3100202        tst     r0, #536870912  ; 0x20000000
  10:   1afffffc        bne     8 <hvc_dcc_put_chars+0x8>
  14:   e7d10003        ldrb    r0, [r1, r3]
  18:   ee10fe11        mrc     14, 0, pc, cr0, cr1, {0}
  1c:   2afffffd        bcs     18 <hvc_dcc_put_chars+0x18>
  20:   ee000e15        mcr     14, 0, r0, cr0, cr5, {0}
  24:   e2833001        add     r3, r3, #1      ; 0x1
  28:   e1530002        cmp     r3, r2
  2c:   bafffff5        blt     8 <hvc_dcc_put_chars+0x8>
  30:   e1a00002        mov     r0, r2
  34:   e12fff1e        bx      lr

which looks better.

I marked all the asm in this driver as volatile. Is that correct?

> +#if defined(CONFIG_CPU_V7)
> +static inline char __dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
> +			bne get_wait                                       \n\
> +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#else
> +static inline char __dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c));
> +
> +	return __c;
> +}
> +#endif
> +
> +#if defined(CONFIG_CPU_V7)
> +static inline void __dcc_putchar(char c)
> +{
> +	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
> +			bcs put_wait                              \n\
> +			mcr p14, 0, %0, c0, c5, 0                   "
> +	: : "r" (c) : "cc");
> +}
> +#else
> +static inline void __dcc_putchar(char c)
> +{
> +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> +		: /* no output register */
> +		: "r" (c));
> +}
> +#endif
> +

I don't think both the v7 and v6 functions are necessary. It seems I can
get away with just the second version of __dcc_(get|put)char() on a v7.
The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
also do the same thing if I read the manuals right. The test in the
inline assembly is saying, wait for a character to be ready or wait for
a character to be read then actually write a character or read one. The
code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
slightly different form. Instead of getting the status bits put into the
condition codes and looping with bne or bcs it will read the register,
and it with bit 29 or bit 28 to see if it should wait and then continue
with the writing/reading. I think you can just drop the looping for the
v7 version of the functions and have this driver work on v6 and v7.
Alternatively, you can make some function that says tx buffer is empty,
rx buffer is full or something but I don't see how saving a couple
instructions buys us much when we can have one driver for v6 and v7.

I see that Tony Lindgren modified the DCC macros for v7 in commit
200b7a8 (ARM: 5884/1: arm: Fix DCC console for v7, 2010-01-19). I'm not
sure why though, since it seems that a v6 and a v7 should really do the
same thing by waiting for the buffers to be ready before filling them or
reading them. Which probably means we can get low-level dcc debugging on
all targets if I'm not mistaken.

> +static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		while (__dcc_getstatus() & DCC_STATUS_TX)
> +			cpu_relax();
> +
> +		__dcc_putchar((char)(buf[i] & 0xFF));

Is this & 0xFF and cast to char unnecessary? buf is a char array, and
chars are always 8 bits. Can't we just do __dcc_putchar(buf[i])?

> +static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; ++i) {
> +		int c = -1;
> +
> +		if (__dcc_getstatus() & DCC_STATUS_RX)
> +			c = __dcc_getchar();
> +		if (c < 0)
> +			break;
> +		buf[i] = c;
> +	}

I think this for loop can be simplified. __dcc_getchar() returns a char.
It never returns -1, so the check for c < 0 can't be taken if
__dcc_getstatus() & DCC_STATUS_RX is true. The only case you break the
loop in then is if __dcc_getstatus() & DCC_STATUS_RX is false. So you
can have a simple if-else and assign buf[i] in the if branch and break
in the else branch.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Arnd Bergmann]

On Tuesday 30 November 2010, Nicolas Pitre wrote:
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nicolas Pitre <nico@fluxnic.net>
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Cc: Mike Frysinger <vapier@gentoo.org>
> > Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> 
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> This doesn't support both ARMv6 and ARMv7 at run time, but this can 
> trivially be added later when needed.

I was about to make a similar comment when I saw yours ;-)

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Arnd Bergmann]

On Tuesday 30 November 2010, Nicolas Pitre wrote:
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nicolas Pitre <nico@fluxnic.net>
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Cc: Mike Frysinger <vapier@gentoo.org>
> > Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> 
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> This doesn't support both ARMv6 and ARMv7 at run time, but this can 
> trivially be added later when needed.

I was about to make a similar comment when I saw yours ;-)

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Nicolas Pitre]

On Tue, 30 Nov 2010, Daniel Walker wrote:

> This driver adds a basic console that uses the arm JTAG
> DCC to transfer data back and forth. It has support for
> ARMv6 and ARMv7.
> 
> This console is created under the HVC driver, and should be named
> /dev/hvcX (or /dev/hvc0 for example).
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

This doesn't support both ARMv6 and ARMv7 at run time, but this can 
trivially be added later when needed.


Nicolas

Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Nicolas Pitre]

On Tue, 30 Nov 2010, Daniel Walker wrote:

> This driver adds a basic console that uses the arm JTAG
> DCC to transfer data back and forth. It has support for
> ARMv6 and ARMv7.
> 
> This console is created under the HVC driver, and should be named
> /dev/hvcX (or /dev/hvc0 for example).
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

This doesn't support both ARMv6 and ARMv7 at run time, but this can 
trivially be added later when needed.


Nicolas

[PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Daniel Walker]

This driver adds a basic console that uses the arm JTAG
DCC to transfer data back and forth. It has support for
ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named
/dev/hvcX (or /dev/hvc0 for example).

Cc: Tony Lindgren <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/char/Kconfig   |    9 +++
 drivers/char/Makefile  |    1 +
 drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/hvc_dcc.c

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 43d3395..d4a7776 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
 
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC
+	 driver. This console is used through a JTAG only on ARM. If you don't have
+	 a JTAG then you probably don't want this option.
+
 config VIRTIO_CONSOLE
 	tristate "Virtio console"
 	depends on VIRTIO
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index ba53ec9..fa0b824 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
 obj-$(CONFIG_HVC_ISERIES)	+= hvc_iseries.o
 obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
 obj-$(CONFIG_HVC_TILE)		+= hvc_tile.o
+obj-$(CONFIG_HVC_DCC)		+= hvc_dcc.o
 obj-$(CONFIG_HVC_BEAT)		+= hvc_beat.o
 obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
 obj-$(CONFIG_HVC_IRQ)		+= hvc_irq.o
diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
new file mode 100644
index 0000000..6470f63
--- /dev/null
+++ b/drivers/char/hvc_dcc.c
@@ -0,0 +1,133 @@
+/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+
+#include <asm/processor.h>
+
+#include "hvc_console.h"
+
+/* DCC Status Bits */
+#define DCC_STATUS_RX		(1 << 30)
+#define DCC_STATUS_TX		(1 << 29)
+
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+		: "=r" (__ret) : : "cc");
+
+	return __ret;
+}
+
+
+#if defined(CONFIG_CPU_V7)
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+			bne get_wait                                       \n\
+			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#else
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c));
+
+	return __c;
+}
+#endif
+
+#if defined(CONFIG_CPU_V7)
+static inline void __dcc_putchar(char c)
+{
+	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+			bcs put_wait                              \n\
+			mcr p14, 0, %0, c0, c5, 0                   "
+	: : "r" (c) : "cc");
+}
+#else
+static inline void __dcc_putchar(char c)
+{
+	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+		: /* no output register */
+		: "r" (c));
+}
+#endif
+
+static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		while (__dcc_getstatus() & DCC_STATUS_TX)
+			cpu_relax();
+
+		__dcc_putchar((char)(buf[i] & 0xFF));
+	}
+
+	return count;
+}
+
+static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i) {
+		int c = -1;
+
+		if (__dcc_getstatus() & DCC_STATUS_RX)
+			c = __dcc_getchar();
+		if (c < 0)
+			break;
+		buf[i] = c;
+	}
+
+	return i;
+}
+
+static const struct hv_ops hvc_dcc_get_put_ops = {
+	.get_chars = hvc_dcc_get_chars,
+	.put_chars = hvc_dcc_put_chars,
+};
+
+static int __init hvc_dcc_console_init(void)
+{
+	hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
+	return 0;
+}
+console_initcall(hvc_dcc_console_init);
+
+static int __init hvc_dcc_init(void)
+{
+	hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
+	return 0;
+}
+device_initcall(hvc_dcc_init);
-- 
1.7.1

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] drivers: char: hvc: add arm JTAG DCC console support

[Daniel Walker]

This driver adds a basic console that uses the arm JTAG
DCC to transfer data back and forth. It has support for
ARMv6 and ARMv7.

This console is created under the HVC driver, and should be named
/dev/hvcX (or /dev/hvc0 for example).

Cc: Tony Lindgren <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/char/Kconfig   |    9 +++
 drivers/char/Makefile  |    1 +
 drivers/char/hvc_dcc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/hvc_dcc.c

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 43d3395..d4a7776 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -682,6 +682,15 @@ config HVC_UDBG
        select HVC_DRIVER
        default n
 
+config HVC_DCC
+       bool "ARM JTAG DCC console"
+       depends on ARM
+       select HVC_DRIVER
+       help
+         This console uses the JTAG DCC on ARM to create a console under the HVC
+	 driver. This console is used through a JTAG only on ARM. If you don't have
+	 a JTAG then you probably don't want this option.
+
 config VIRTIO_CONSOLE
 	tristate "Virtio console"
 	depends on VIRTIO
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index ba53ec9..fa0b824 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
 obj-$(CONFIG_HVC_ISERIES)	+= hvc_iseries.o
 obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
 obj-$(CONFIG_HVC_TILE)		+= hvc_tile.o
+obj-$(CONFIG_HVC_DCC)		+= hvc_dcc.o
 obj-$(CONFIG_HVC_BEAT)		+= hvc_beat.o
 obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
 obj-$(CONFIG_HVC_IRQ)		+= hvc_irq.o
diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
new file mode 100644
index 0000000..6470f63
--- /dev/null
+++ b/drivers/char/hvc_dcc.c
@@ -0,0 +1,133 @@
+/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+
+#include <asm/processor.h>
+
+#include "hvc_console.h"
+
+/* DCC Status Bits */
+#define DCC_STATUS_RX		(1 << 30)
+#define DCC_STATUS_TX		(1 << 29)
+
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+		: "=r" (__ret) : : "cc");
+
+	return __ret;
+}
+
+
+#if defined(CONFIG_CPU_V7)
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("get_wait:	mrc p14, 0, pc, c0, c1, 0                          \n\
+			bne get_wait                                       \n\
+			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#else
+static inline char __dcc_getchar(void)
+{
+	char __c;
+
+	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c));
+
+	return __c;
+}
+#endif
+
+#if defined(CONFIG_CPU_V7)
+static inline void __dcc_putchar(char c)
+{
+	asm("put_wait:	mrc p14, 0, pc, c0, c1, 0                 \n\
+			bcs put_wait                              \n\
+			mcr p14, 0, %0, c0, c5, 0                   "
+	: : "r" (c) : "cc");
+}
+#else
+static inline void __dcc_putchar(char c)
+{
+	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+		: /* no output register */
+		: "r" (c));
+}
+#endif
+
+static int hvc_dcc_put_chars(uint32_t vt, const char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		while (__dcc_getstatus() & DCC_STATUS_TX)
+			cpu_relax();
+
+		__dcc_putchar((char)(buf[i] & 0xFF));
+	}
+
+	return count;
+}
+
+static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i) {
+		int c = -1;
+
+		if (__dcc_getstatus() & DCC_STATUS_RX)
+			c = __dcc_getchar();
+		if (c < 0)
+			break;
+		buf[i] = c;
+	}
+
+	return i;
+}
+
+static const struct hv_ops hvc_dcc_get_put_ops = {
+	.get_chars = hvc_dcc_get_chars,
+	.put_chars = hvc_dcc_put_chars,
+};
+
+static int __init hvc_dcc_console_init(void)
+{
+	hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
+	return 0;
+}
+console_initcall(hvc_dcc_console_init);
+
+static int __init hvc_dcc_init(void)
+{
+	hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
+	return 0;
+}
+device_initcall(hvc_dcc_init);
-- 
1.7.1

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.