uchcom(4) did not work

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

uchcom(4) did not work

SASANO Takayoshi
Hello,

Recently I bought Arduino board which uses Nanjing QinHeng (WinChipHead)
CH340T USB-UART bridge via eBay, and I found uchcom(4) did not work.
At misc@, other user reported similar problem. [1]

The uchcom(4) comes from NetBSD and it seems to be worked at that time.

I found that uchcom_set_line_control() breaks the value of
UCHCOM_REG_LCR1(0x18) makes the problem. The UCHCOM_REG_LCR1 register
seems to control parity bit (none/odd/even) but I doubt the role of that
register has changed between old/new CH340T chip.

At least following changes are required to work my CH340T but parity bit
control is no longer supported.

If there is no problem, I want to commit.

[1] http://openbsd.7691.n7.nabble.com/issue-with-WinChipHead-CH340-usb-serial-td99678.html

Index: uchcom.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
retrieving revision 1.19
diff -u -p -r1.19 uchcom.c
--- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
+++ uchcom.c 13 May 2014 19:43:37 -0000
@@ -91,14 +91,6 @@ int uchcomdebug = 0;
 #define UCHCOM_BRK1_MASK 0x01
 #define UCHCOM_BRK2_MASK 0x40
 
-#define UCHCOM_LCR1_MASK 0xAF
-#define UCHCOM_LCR2_MASK 0x07
-#define UCHCOM_LCR1_PARENB 0x80
-#define UCHCOM_LCR2_PAREVEN 0x07
-#define UCHCOM_LCR2_PARODD 0x06
-#define UCHCOM_LCR2_PARMARK 0x05
-#define UCHCOM_LCR2_PARSPACE 0x04
-
 #define UCHCOM_INTR_STAT1 0x02
 #define UCHCOM_INTR_STAT2 0x03
 #define UCHCOM_INTR_LEAST 4
@@ -707,27 +699,10 @@ uchcom_set_dte_rate(struct uchcom_softc
 int
 uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
 {
- usbd_status err;
- uint8_t lcr1 = 0, lcr2 = 0;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
-    &lcr2);
- if (err) {
- printf("%s: cannot get LCR: %s\n",
-       sc->sc_dev.dv_xname, usbd_errstr(err));
- return EIO;
- }
-
- lcr1 &= ~UCHCOM_LCR1_MASK;
- lcr2 &= ~UCHCOM_LCR2_MASK;
-
  /*
  * XXX: it is difficult to handle the line control appropriately:
- *   - CS8, !CSTOPB and any parity mode seems ok, but
- *   - the chip doesn't have the function to calculate parity
- *     in !CS8 mode.
- *   - it is unclear that the chip supports CS5,6 mode.
- *   - it is unclear how to handle stop bits.
+ *   work as chip default - CS8, no parity, !CSTOPB
+ *   other modes are not supported.
  */
 
  switch (ISSET(cflag, CSIZE)) {
@@ -739,21 +714,8 @@ uchcom_set_line_control(struct uchcom_so
  break;
  }
 
- if (ISSET(cflag, PARENB)) {
- lcr1 |= UCHCOM_LCR1_PARENB;
- if (ISSET(cflag, PARODD))
- lcr2 |= UCHCOM_LCR2_PARODD;
- else
- lcr2 |= UCHCOM_LCR2_PAREVEN;
- }
-
- err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
-    lcr2);
- if (err) {
- printf("%s: cannot set LCR: %s\n",
-       sc->sc_dev.dv_xname, usbd_errstr(err));
- return EIO;
- }
+ if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
+ return EINVAL;
 
  return 0;
 }
@@ -778,33 +740,10 @@ int
 uchcom_reset_chip(struct uchcom_softc *sc)
 {
  usbd_status err;
- uint8_t lcr1, lcr2, pre, div, mod;
- uint16_t val=0, idx=0;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
- if (err)
- goto failed;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
-    &div);
- if (err)
- goto failed;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
-    NULL);
- if (err)
- goto failed;
+ uint16_t val, idx;
 
- val |= (uint16_t)(lcr1&0xF0) << 8;
- val |= 0x01;
- val |= (uint16_t)(lcr2&0x0F) << 8;
- val |= 0x02;
- idx |= pre & 0x07;
- val |= 0x04;
- idx |= (uint16_t)div << 8;
- val |= 0x08;
- idx |= mod & 0xF8;
- val |= 0x10;
+ val = 0x501f;
+ idx = 0xd90a;
 
  DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
  sc->sc_dev.dv_xname, val, idx));

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Bryan Steele-2
On Wed, May 14, 2014 at 05:39:38AM +0900, SASANO Takayoshi wrote:

> Hello,
>
> Recently I bought Arduino board which uses Nanjing QinHeng (WinChipHead)
> CH340T USB-UART bridge via eBay, and I found uchcom(4) did not work.
> At misc@, other user reported similar problem. [1]
>
> The uchcom(4) comes from NetBSD and it seems to be worked at that time.
>
> I found that uchcom_set_line_control() breaks the value of
> UCHCOM_REG_LCR1(0x18) makes the problem. The UCHCOM_REG_LCR1 register
> seems to control parity bit (none/odd/even) but I doubt the role of that
> register has changed between old/new CH340T chip.
>
> At least following changes are required to work my CH340T but parity bit
> control is no longer supported.
>
> If there is no problem, I want to commit.
>
> [1] http://openbsd.7691.n7.nabble.com/issue-with-WinChipHead-CH340-usb-serial-td99678.html

This fixes a device I first thought was DOA, so, ok by me. :-)

uchcom0 at uhub2 port 1 "QinHeng Electronics CH340 serial/parallel" rev
1.10/2.54 addr 2
uchcom0: CH341
ucom0 at uchcom0

-Bryan.

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Mike Larkin
In reply to this post by SASANO Takayoshi
On Wed, May 14, 2014 at 05:39:38AM +0900, SASANO Takayoshi wrote:

> Hello,
>
> Recently I bought Arduino board which uses Nanjing QinHeng (WinChipHead)
> CH340T USB-UART bridge via eBay, and I found uchcom(4) did not work.
> At misc@, other user reported similar problem. [1]
>
> The uchcom(4) comes from NetBSD and it seems to be worked at that time.
>
> I found that uchcom_set_line_control() breaks the value of
> UCHCOM_REG_LCR1(0x18) makes the problem. The UCHCOM_REG_LCR1 register
> seems to control parity bit (none/odd/even) but I doubt the role of that
> register has changed between old/new CH340T chip.
>
> At least following changes are required to work my CH340T but parity bit
> control is no longer supported.
>
> If there is no problem, I want to commit.
>
> [1] http://openbsd.7691.n7.nabble.com/issue-with-WinChipHead-CH340-usb-serial-td99678.html
>
> Index: uchcom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 uchcom.c
> --- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
> +++ uchcom.c 13 May 2014 19:43:37 -0000
> @@ -91,14 +91,6 @@ int uchcomdebug = 0;
>  #define UCHCOM_BRK1_MASK 0x01
>  #define UCHCOM_BRK2_MASK 0x40
>  
> -#define UCHCOM_LCR1_MASK 0xAF
> -#define UCHCOM_LCR2_MASK 0x07
> -#define UCHCOM_LCR1_PARENB 0x80
> -#define UCHCOM_LCR2_PAREVEN 0x07
> -#define UCHCOM_LCR2_PARODD 0x06
> -#define UCHCOM_LCR2_PARMARK 0x05
> -#define UCHCOM_LCR2_PARSPACE 0x04
> -
>  #define UCHCOM_INTR_STAT1 0x02
>  #define UCHCOM_INTR_STAT2 0x03
>  #define UCHCOM_INTR_LEAST 4
> @@ -707,27 +699,10 @@ uchcom_set_dte_rate(struct uchcom_softc
>  int
>  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
>  {
> - usbd_status err;
> - uint8_t lcr1 = 0, lcr2 = 0;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
> -    &lcr2);
> - if (err) {
> - printf("%s: cannot get LCR: %s\n",
> -       sc->sc_dev.dv_xname, usbd_errstr(err));
> - return EIO;
> - }
> -
> - lcr1 &= ~UCHCOM_LCR1_MASK;
> - lcr2 &= ~UCHCOM_LCR2_MASK;
> -
>   /*
>   * XXX: it is difficult to handle the line control appropriately:
> - *   - CS8, !CSTOPB and any parity mode seems ok, but
> - *   - the chip doesn't have the function to calculate parity
> - *     in !CS8 mode.
> - *   - it is unclear that the chip supports CS5,6 mode.
> - *   - it is unclear how to handle stop bits.
> + *   work as chip default - CS8, no parity, !CSTOPB
> + *   other modes are not supported.
>   */
>  
>   switch (ISSET(cflag, CSIZE)) {
> @@ -739,21 +714,8 @@ uchcom_set_line_control(struct uchcom_so
>   break;
>   }
>  
> - if (ISSET(cflag, PARENB)) {
> - lcr1 |= UCHCOM_LCR1_PARENB;
> - if (ISSET(cflag, PARODD))
> - lcr2 |= UCHCOM_LCR2_PARODD;
> - else
> - lcr2 |= UCHCOM_LCR2_PAREVEN;
> - }
> -
> - err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
> -    lcr2);
> - if (err) {
> - printf("%s: cannot set LCR: %s\n",
> -       sc->sc_dev.dv_xname, usbd_errstr(err));
> - return EIO;
> - }
> + if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> + return EINVAL;
>  
>   return 0;
>  }
> @@ -778,33 +740,10 @@ int
>  uchcom_reset_chip(struct uchcom_softc *sc)
>  {
>   usbd_status err;
> - uint8_t lcr1, lcr2, pre, div, mod;
> - uint16_t val=0, idx=0;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
> - if (err)
> - goto failed;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
> -    &div);
> - if (err)
> - goto failed;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
> -    NULL);
> - if (err)
> - goto failed;
> + uint16_t val, idx;
>  
> - val |= (uint16_t)(lcr1&0xF0) << 8;
> - val |= 0x01;
> - val |= (uint16_t)(lcr2&0x0F) << 8;
> - val |= 0x02;
> - idx |= pre & 0x07;
> - val |= 0x04;
> - idx |= (uint16_t)div << 8;
> - val |= 0x08;
> - idx |= mod & 0xF8;
> - val |= 0x10;
> + val = 0x501f;
> + idx = 0xd90a;
>  

What are these magic numbers?

>   DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
>   sc->sc_dev.dv_xname, val, idx));
>

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

SASANO Takayoshi
Hi, Mike.

>> + val = 0x501f;
>> + idx = 0xd90a;
>
> What are these magic numbers?

These numbers come from Linux driver (ch341.c). I don't know what they mean.
Maybe these numbers represent default character length and baudrate divisor.

<a href="http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217">http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217

With my CH340T, uchcom(4) generates val=0x401f/idx=0xb2d2.

uchcom(4) reads CH340 registers to make val/idx value but I think this
method is improper for initialize. Because none can take the initial state
when the chip's default value has changed.

It seems that the content/function of UCHCOM_REG_LCR1 has been changed,
and generated results are doubtful. So I use Linux's magic numbers.

Regards,
--
SASANO Takayoshi <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Mike Larkin
On Wed, May 14, 2014 at 10:01:28AM +0900, SASANO Takayoshi wrote:

> Hi, Mike.
>
> >> + val = 0x501f;
> >> + idx = 0xd90a;
> >
> > What are these magic numbers?
>
> These numbers come from Linux driver (ch341.c). I don't know what they mean.
> Maybe these numbers represent default character length and baudrate divisor.
>
> <a href="http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217">http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217
>
> With my CH340T, uchcom(4) generates val=0x401f/idx=0xb2d2.
>
> uchcom(4) reads CH340 registers to make val/idx value but I think this
> method is improper for initialize. Because none can take the initial state
> when the chip's default value has changed.
>
> It seems that the content/function of UCHCOM_REG_LCR1 has been changed,
> and generated results are doubtful. So I use Linux's magic numbers.
>
> Regards,
> --
> SASANO Takayoshi <[hidden email]>
>

I don't think we should be committing things that use magic numbers unless
we completely understand what they mean. And in that case, such magic numbers would
likely be constructed from some #defines. Just because Linux uses those numbers
doesn't mean we should, IMO.

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Kenneth Westerback
On 13 May 2014 21:09, Mike Larkin <[hidden email]> wrote:

> On Wed, May 14, 2014 at 10:01:28AM +0900, SASANO Takayoshi wrote:
>> Hi, Mike.
>>
>> >> +  val = 0x501f;
>> >> +  idx = 0xd90a;
>> >
>> > What are these magic numbers?
>>
>> These numbers come from Linux driver (ch341.c). I don't know what they mean.
>> Maybe these numbers represent default character length and baudrate divisor.
>>
>> <a href="http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217">http://lxr.cpsc.ucalgary.ca/lxr/#linux+v2.6.31/drivers/usb/serial/ch341.c#L217
>>
>> With my CH340T, uchcom(4) generates val=0x401f/idx=0xb2d2.
>>
>> uchcom(4) reads CH340 registers to make val/idx value but I think this
>> method is improper for initialize. Because none can take the initial state
>> when the chip's default value has changed.
>>
>> It seems that the content/function of UCHCOM_REG_LCR1 has been changed,
>> and generated results are doubtful. So I use Linux's magic numbers.
>>
>> Regards,
>> --
>> SASANO Takayoshi <[hidden email]>
>>
>
> I don't think we should be committing things that use magic numbers unless
> we completely understand what they mean. And in that case, such magic numbers would
> likely be constructed from some #defines. Just because Linux uses those numbers
> doesn't mean we should, IMO.
>

+1

.... Ken

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

SASANO Takayoshi
Hi,

Simply magic values are rewrited with #define.
If these values need to be disassembled, please take a while...

----
SASANO Takayoshi <[hidden email]>

Index: uchcom.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
retrieving revision 1.19
diff -u -p -r1.19 uchcom.c
--- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
+++ uchcom.c 14 May 2014 01:43:34 -0000
@@ -91,18 +91,14 @@ int uchcomdebug = 0;
 #define UCHCOM_BRK1_MASK 0x01
 #define UCHCOM_BRK2_MASK 0x40
 
-#define UCHCOM_LCR1_MASK 0xAF
-#define UCHCOM_LCR2_MASK 0x07
-#define UCHCOM_LCR1_PARENB 0x80
-#define UCHCOM_LCR2_PAREVEN 0x07
-#define UCHCOM_LCR2_PARODD 0x06
-#define UCHCOM_LCR2_PARMARK 0x05
-#define UCHCOM_LCR2_PARSPACE 0x04
-
 #define UCHCOM_INTR_STAT1 0x02
 #define UCHCOM_INTR_STAT2 0x03
 #define UCHCOM_INTR_LEAST 4
 
+/* these values come from Linux (drivers/usb/serial/ch341.c) */
+#define UCHCOM_RESET_VALUE 0x501F /* XXX default line mode? */
+#define UCHCOM_RESET_INDEX 0xD90A /* XXX default baud rate? */
+
 #define UCHCOMIBUFSIZE 256
 #define UCHCOMOBUFSIZE 256
 
@@ -707,27 +703,10 @@ uchcom_set_dte_rate(struct uchcom_softc
 int
 uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
 {
- usbd_status err;
- uint8_t lcr1 = 0, lcr2 = 0;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
-    &lcr2);
- if (err) {
- printf("%s: cannot get LCR: %s\n",
-       sc->sc_dev.dv_xname, usbd_errstr(err));
- return EIO;
- }
-
- lcr1 &= ~UCHCOM_LCR1_MASK;
- lcr2 &= ~UCHCOM_LCR2_MASK;
-
  /*
  * XXX: it is difficult to handle the line control appropriately:
- *   - CS8, !CSTOPB and any parity mode seems ok, but
- *   - the chip doesn't have the function to calculate parity
- *     in !CS8 mode.
- *   - it is unclear that the chip supports CS5,6 mode.
- *   - it is unclear how to handle stop bits.
+ *   work as chip default - CS8, no parity, !CSTOPB
+ *   other modes are not supported.
  */
 
  switch (ISSET(cflag, CSIZE)) {
@@ -739,21 +718,8 @@ uchcom_set_line_control(struct uchcom_so
  break;
  }
 
- if (ISSET(cflag, PARENB)) {
- lcr1 |= UCHCOM_LCR1_PARENB;
- if (ISSET(cflag, PARODD))
- lcr2 |= UCHCOM_LCR2_PARODD;
- else
- lcr2 |= UCHCOM_LCR2_PAREVEN;
- }
-
- err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
-    lcr2);
- if (err) {
- printf("%s: cannot set LCR: %s\n",
-       sc->sc_dev.dv_xname, usbd_errstr(err));
- return EIO;
- }
+ if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
+ return EINVAL;
 
  return 0;
 }
@@ -778,38 +744,12 @@ int
 uchcom_reset_chip(struct uchcom_softc *sc)
 {
  usbd_status err;
- uint8_t lcr1, lcr2, pre, div, mod;
- uint16_t val=0, idx=0;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
- if (err)
- goto failed;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
-    &div);
- if (err)
- goto failed;
-
- err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
-    NULL);
- if (err)
- goto failed;
-
- val |= (uint16_t)(lcr1&0xF0) << 8;
- val |= 0x01;
- val |= (uint16_t)(lcr2&0x0F) << 8;
- val |= 0x02;
- idx |= pre & 0x07;
- val |= 0x04;
- idx |= (uint16_t)div << 8;
- val |= 0x08;
- idx |= mod & 0xF8;
- val |= 0x10;
 
- DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
- sc->sc_dev.dv_xname, val, idx));
+ DPRINTF(("%s: reset\n", sc->sc_dev.dv_xname));
 
- err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET, val, idx);
+ err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET,
+ UCHCOM_RESET_VALUE,
+ UCHCOM_RESET_INDEX);
  if (err)
  goto failed;

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Mike Larkin
On Wed, May 14, 2014 at 11:02:49AM +0900, SASANO Takayoshi wrote:
> Hi,
>
> Simply magic values are rewrited with #define.
> If these values need to be disassembled, please take a while...
>

I think we need to understand what those values mean. When I mentioned
#defines, I meant something like:

#define UCHCOM_SOME_FLAG 0x1234
#define UCHCOM_SOME_OTHER_FLAG 0x5678
...
...
#define UCHCOM_RESET_VALUE (UCHCOM_SOME_FLAG | UCHCOM_SOME_OTHER_FLAG)

That way we know what the values do. If the value we're setting is not
a flag, we should understand what 0x501F and 0xD90A actually mean.

If we don't do it this way, the next person to come along and try to
work on this code won't have any idea what to do.

-ml

> ----
> SASANO Takayoshi <[hidden email]>
>
> Index: uchcom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 uchcom.c
> --- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
> +++ uchcom.c 14 May 2014 01:43:34 -0000
> @@ -91,18 +91,14 @@ int uchcomdebug = 0;
>  #define UCHCOM_BRK1_MASK 0x01
>  #define UCHCOM_BRK2_MASK 0x40
>  
> -#define UCHCOM_LCR1_MASK 0xAF
> -#define UCHCOM_LCR2_MASK 0x07
> -#define UCHCOM_LCR1_PARENB 0x80
> -#define UCHCOM_LCR2_PAREVEN 0x07
> -#define UCHCOM_LCR2_PARODD 0x06
> -#define UCHCOM_LCR2_PARMARK 0x05
> -#define UCHCOM_LCR2_PARSPACE 0x04
> -
>  #define UCHCOM_INTR_STAT1 0x02
>  #define UCHCOM_INTR_STAT2 0x03
>  #define UCHCOM_INTR_LEAST 4
>  
> +/* these values come from Linux (drivers/usb/serial/ch341.c) */
> +#define UCHCOM_RESET_VALUE 0x501F /* XXX default line mode? */
> +#define UCHCOM_RESET_INDEX 0xD90A /* XXX default baud rate? */
> +
>  #define UCHCOMIBUFSIZE 256
>  #define UCHCOMOBUFSIZE 256
>  
> @@ -707,27 +703,10 @@ uchcom_set_dte_rate(struct uchcom_softc
>  int
>  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
>  {
> - usbd_status err;
> - uint8_t lcr1 = 0, lcr2 = 0;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
> -    &lcr2);
> - if (err) {
> - printf("%s: cannot get LCR: %s\n",
> -       sc->sc_dev.dv_xname, usbd_errstr(err));
> - return EIO;
> - }
> -
> - lcr1 &= ~UCHCOM_LCR1_MASK;
> - lcr2 &= ~UCHCOM_LCR2_MASK;
> -
>   /*
>   * XXX: it is difficult to handle the line control appropriately:
> - *   - CS8, !CSTOPB and any parity mode seems ok, but
> - *   - the chip doesn't have the function to calculate parity
> - *     in !CS8 mode.
> - *   - it is unclear that the chip supports CS5,6 mode.
> - *   - it is unclear how to handle stop bits.
> + *   work as chip default - CS8, no parity, !CSTOPB
> + *   other modes are not supported.
>   */
>  
>   switch (ISSET(cflag, CSIZE)) {
> @@ -739,21 +718,8 @@ uchcom_set_line_control(struct uchcom_so
>   break;
>   }
>  
> - if (ISSET(cflag, PARENB)) {
> - lcr1 |= UCHCOM_LCR1_PARENB;
> - if (ISSET(cflag, PARODD))
> - lcr2 |= UCHCOM_LCR2_PARODD;
> - else
> - lcr2 |= UCHCOM_LCR2_PAREVEN;
> - }
> -
> - err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
> -    lcr2);
> - if (err) {
> - printf("%s: cannot set LCR: %s\n",
> -       sc->sc_dev.dv_xname, usbd_errstr(err));
> - return EIO;
> - }
> + if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> + return EINVAL;
>  
>   return 0;
>  }
> @@ -778,38 +744,12 @@ int
>  uchcom_reset_chip(struct uchcom_softc *sc)
>  {
>   usbd_status err;
> - uint8_t lcr1, lcr2, pre, div, mod;
> - uint16_t val=0, idx=0;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
> - if (err)
> - goto failed;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
> -    &div);
> - if (err)
> - goto failed;
> -
> - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
> -    NULL);
> - if (err)
> - goto failed;
> -
> - val |= (uint16_t)(lcr1&0xF0) << 8;
> - val |= 0x01;
> - val |= (uint16_t)(lcr2&0x0F) << 8;
> - val |= 0x02;
> - idx |= pre & 0x07;
> - val |= 0x04;
> - idx |= (uint16_t)div << 8;
> - val |= 0x08;
> - idx |= mod & 0xF8;
> - val |= 0x10;
>  
> - DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
> - sc->sc_dev.dv_xname, val, idx));
> + DPRINTF(("%s: reset\n", sc->sc_dev.dv_xname));
>  
> - err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET, val, idx);
> + err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET,
> + UCHCOM_RESET_VALUE,
> + UCHCOM_RESET_INDEX);
>   if (err)
>   goto failed;
>

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Martin Pieuchot-2
On 13/05/14(Tue) 21:24, Mike Larkin wrote:

> On Wed, May 14, 2014 at 11:02:49AM +0900, SASANO Takayoshi wrote:
> > Hi,
> >
> > Simply magic values are rewrited with #define.
> > If these values need to be disassembled, please take a while...
> >
>
> I think we need to understand what those values mean. When I mentioned
> #defines, I meant something like:
>
> #define UCHCOM_SOME_FLAG 0x1234
> #define UCHCOM_SOME_OTHER_FLAG 0x5678
> ...
> ...
> #define UCHCOM_RESET_VALUE (UCHCOM_SOME_FLAG | UCHCOM_SOME_OTHER_FLAG)
>
> That way we know what the values do. If the value we're setting is not
> a flag, we should understand what 0x501F and 0xD90A actually mean.

Unfortunately a lot of USB drivers are written without spec. and some
values are taken by analysing the traffic generated by drivers on other
OS.

> If we don't do it this way, the next person to come along and try to
> work on this code won't have any idea what to do.

I also really don't like magic values, but I don't see how we could do
it differently.  Maybe somebody can send an email to the author of the
linux driver and ask him what these values are.  But I'd bet he doesn't
no neither.

> > ----
> > SASANO Takayoshi <[hidden email]>
> >
> > Index: uchcom.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> > retrieving revision 1.19
> > diff -u -p -r1.19 uchcom.c
> > --- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
> > +++ uchcom.c 14 May 2014 01:43:34 -0000
> > @@ -91,18 +91,14 @@ int uchcomdebug = 0;
> >  #define UCHCOM_BRK1_MASK 0x01
> >  #define UCHCOM_BRK2_MASK 0x40
> >  
> > -#define UCHCOM_LCR1_MASK 0xAF
> > -#define UCHCOM_LCR2_MASK 0x07
> > -#define UCHCOM_LCR1_PARENB 0x80
> > -#define UCHCOM_LCR2_PAREVEN 0x07
> > -#define UCHCOM_LCR2_PARODD 0x06
> > -#define UCHCOM_LCR2_PARMARK 0x05
> > -#define UCHCOM_LCR2_PARSPACE 0x04
> > -
> >  #define UCHCOM_INTR_STAT1 0x02
> >  #define UCHCOM_INTR_STAT2 0x03
> >  #define UCHCOM_INTR_LEAST 4
> >  
> > +/* these values come from Linux (drivers/usb/serial/ch341.c) */
> > +#define UCHCOM_RESET_VALUE 0x501F /* XXX default line mode? */
> > +#define UCHCOM_RESET_INDEX 0xD90A /* XXX default baud rate? */
> > +
> >  #define UCHCOMIBUFSIZE 256
> >  #define UCHCOMOBUFSIZE 256
> >  
> > @@ -707,27 +703,10 @@ uchcom_set_dte_rate(struct uchcom_softc
> >  int
> >  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
> >  {
> > - usbd_status err;
> > - uint8_t lcr1 = 0, lcr2 = 0;
> > -
> > - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
> > -    &lcr2);
> > - if (err) {
> > - printf("%s: cannot get LCR: %s\n",
> > -       sc->sc_dev.dv_xname, usbd_errstr(err));
> > - return EIO;
> > - }
> > -
> > - lcr1 &= ~UCHCOM_LCR1_MASK;
> > - lcr2 &= ~UCHCOM_LCR2_MASK;
> > -
> >   /*
> >   * XXX: it is difficult to handle the line control appropriately:
> > - *   - CS8, !CSTOPB and any parity mode seems ok, but
> > - *   - the chip doesn't have the function to calculate parity
> > - *     in !CS8 mode.
> > - *   - it is unclear that the chip supports CS5,6 mode.
> > - *   - it is unclear how to handle stop bits.
> > + *   work as chip default - CS8, no parity, !CSTOPB
> > + *   other modes are not supported.
> >   */
> >  
> >   switch (ISSET(cflag, CSIZE)) {
> > @@ -739,21 +718,8 @@ uchcom_set_line_control(struct uchcom_so
> >   break;
> >   }
> >  
> > - if (ISSET(cflag, PARENB)) {
> > - lcr1 |= UCHCOM_LCR1_PARENB;
> > - if (ISSET(cflag, PARODD))
> > - lcr2 |= UCHCOM_LCR2_PARODD;
> > - else
> > - lcr2 |= UCHCOM_LCR2_PAREVEN;
> > - }
> > -
> > - err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
> > -    lcr2);
> > - if (err) {
> > - printf("%s: cannot set LCR: %s\n",
> > -       sc->sc_dev.dv_xname, usbd_errstr(err));
> > - return EIO;
> > - }
> > + if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> > + return EINVAL;
> >  
> >   return 0;
> >  }
> > @@ -778,38 +744,12 @@ int
> >  uchcom_reset_chip(struct uchcom_softc *sc)
> >  {
> >   usbd_status err;
> > - uint8_t lcr1, lcr2, pre, div, mod;
> > - uint16_t val=0, idx=0;
> > -
> > - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
> > - if (err)
> > - goto failed;
> > -
> > - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
> > -    &div);
> > - if (err)
> > - goto failed;
> > -
> > - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
> > -    NULL);
> > - if (err)
> > - goto failed;
> > -
> > - val |= (uint16_t)(lcr1&0xF0) << 8;
> > - val |= 0x01;
> > - val |= (uint16_t)(lcr2&0x0F) << 8;
> > - val |= 0x02;
> > - idx |= pre & 0x07;
> > - val |= 0x04;
> > - idx |= (uint16_t)div << 8;
> > - val |= 0x08;
> > - idx |= mod & 0xF8;
> > - val |= 0x10;
> >  
> > - DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
> > - sc->sc_dev.dv_xname, val, idx));
> > + DPRINTF(("%s: reset\n", sc->sc_dev.dv_xname));
> >  
> > - err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET, val, idx);
> > + err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET,
> > + UCHCOM_RESET_VALUE,
> > + UCHCOM_RESET_INDEX);
> >   if (err)
> >   goto failed;
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Mark Kettenis
> Date: Wed, 14 May 2014 11:04:56 +0200
> From: Martin Pieuchot <[hidden email]>
>
> On 13/05/14(Tue) 21:24, Mike Larkin wrote:
> > On Wed, May 14, 2014 at 11:02:49AM +0900, SASANO Takayoshi wrote:
> > > Hi,
> > >
> > > Simply magic values are rewrited with #define.
> > > If these values need to be disassembled, please take a while...
> > >
> >
> > I think we need to understand what those values mean. When I mentioned
> > #defines, I meant something like:
> >
> > #define UCHCOM_SOME_FLAG 0x1234
> > #define UCHCOM_SOME_OTHER_FLAG 0x5678
> > ...
> > ...
> > #define UCHCOM_RESET_VALUE (UCHCOM_SOME_FLAG | UCHCOM_SOME_OTHER_FLAG)
> >
> > That way we know what the values do. If the value we're setting is not
> > a flag, we should understand what 0x501F and 0xD90A actually mean.
>
> Unfortunately a lot of USB drivers are written without spec. and some
> values are taken by analysing the traffic generated by drivers on other
> OS.
>
> > If we don't do it this way, the next person to come along and try to
> > work on this code won't have any idea what to do.
>
> I also really don't like magic values, but I don't see how we could do
> it differently.  Maybe somebody can send an email to the author of the
> linux driver and ask him what these values are.  But I'd bet he doesn't
> no neither.

And really, the old code had magic numbers as well.

> > > Index: uchcom.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> > > retrieving revision 1.19
> > > diff -u -p -r1.19 uchcom.c
> > > --- uchcom.c 15 Nov 2013 10:17:39 -0000 1.19
> > > +++ uchcom.c 14 May 2014 01:43:34 -0000
> > > @@ -91,18 +91,14 @@ int uchcomdebug = 0;
> > >  #define UCHCOM_BRK1_MASK 0x01
> > >  #define UCHCOM_BRK2_MASK 0x40
> > >  
> > > -#define UCHCOM_LCR1_MASK 0xAF
> > > -#define UCHCOM_LCR2_MASK 0x07
> > > -#define UCHCOM_LCR1_PARENB 0x80
> > > -#define UCHCOM_LCR2_PAREVEN 0x07
> > > -#define UCHCOM_LCR2_PARODD 0x06
> > > -#define UCHCOM_LCR2_PARMARK 0x05
> > > -#define UCHCOM_LCR2_PARSPACE 0x04
> > > -
> > >  #define UCHCOM_INTR_STAT1 0x02
> > >  #define UCHCOM_INTR_STAT2 0x03
> > >  #define UCHCOM_INTR_LEAST 4
> > >  
> > > +/* these values come from Linux (drivers/usb/serial/ch341.c) */
> > > +#define UCHCOM_RESET_VALUE 0x501F /* XXX default line mode? */
> > > +#define UCHCOM_RESET_INDEX 0xD90A /* XXX default baud rate? */
> > > +
> > >  #define UCHCOMIBUFSIZE 256
> > >  #define UCHCOMOBUFSIZE 256
> > >  
> > > @@ -707,27 +703,10 @@ uchcom_set_dte_rate(struct uchcom_softc
> > >  int
> > >  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
> > >  {
> > > - usbd_status err;
> > > - uint8_t lcr1 = 0, lcr2 = 0;
> > > -
> > > - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
> > > -    &lcr2);
> > > - if (err) {
> > > - printf("%s: cannot get LCR: %s\n",
> > > -       sc->sc_dev.dv_xname, usbd_errstr(err));
> > > - return EIO;
> > > - }
> > > -
> > > - lcr1 &= ~UCHCOM_LCR1_MASK;
> > > - lcr2 &= ~UCHCOM_LCR2_MASK;
> > > -
> > >   /*
> > >   * XXX: it is difficult to handle the line control appropriately:
> > > - *   - CS8, !CSTOPB and any parity mode seems ok, but
> > > - *   - the chip doesn't have the function to calculate parity
> > > - *     in !CS8 mode.
> > > - *   - it is unclear that the chip supports CS5,6 mode.
> > > - *   - it is unclear how to handle stop bits.
> > > + *   work as chip default - CS8, no parity, !CSTOPB
> > > + *   other modes are not supported.
> > >   */
> > >  
> > >   switch (ISSET(cflag, CSIZE)) {
> > > @@ -739,21 +718,8 @@ uchcom_set_line_control(struct uchcom_so
> > >   break;
> > >   }
> > >  
> > > - if (ISSET(cflag, PARENB)) {
> > > - lcr1 |= UCHCOM_LCR1_PARENB;
> > > - if (ISSET(cflag, PARODD))
> > > - lcr2 |= UCHCOM_LCR2_PARODD;
> > > - else
> > > - lcr2 |= UCHCOM_LCR2_PAREVEN;
> > > - }
> > > -
> > > - err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
> > > -    lcr2);
> > > - if (err) {
> > > - printf("%s: cannot set LCR: %s\n",
> > > -       sc->sc_dev.dv_xname, usbd_errstr(err));
> > > - return EIO;
> > > - }
> > > + if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> > > + return EINVAL;
> > >  
> > >   return 0;
> > >  }
> > > @@ -778,38 +744,12 @@ int
> > >  uchcom_reset_chip(struct uchcom_softc *sc)
> > >  {
> > >   usbd_status err;
> > > - uint8_t lcr1, lcr2, pre, div, mod;
> > > - uint16_t val=0, idx=0;
> > > -
> > > - err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, &lcr2);
> > > - if (err)
> > > - goto failed;
> > > -
> > > - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
> > > -    &div);
> > > - if (err)
> > > - goto failed;
> > > -
> > > - err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
> > > -    NULL);
> > > - if (err)
> > > - goto failed;
> > > -
> > > - val |= (uint16_t)(lcr1&0xF0) << 8;
> > > - val |= 0x01;
> > > - val |= (uint16_t)(lcr2&0x0F) << 8;
> > > - val |= 0x02;
> > > - idx |= pre & 0x07;
> > > - val |= 0x04;
> > > - idx |= (uint16_t)div << 8;
> > > - val |= 0x08;
> > > - idx |= mod & 0xF8;
> > > - val |= 0x10;
> > >  
> > > - DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
> > > - sc->sc_dev.dv_xname, val, idx));
> > > + DPRINTF(("%s: reset\n", sc->sc_dev.dv_xname));
> > >  
> > > - err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET, val, idx);
> > > + err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET,
> > > + UCHCOM_RESET_VALUE,
> > > + UCHCOM_RESET_INDEX);
> > >   if (err)
> > >   goto failed;
> > >
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: uchcom(4) did not work

Creamy-2
> > > >   /*
> > > >   * XXX: it is difficult to handle the line control appropriately:
> > > > - *   - CS8, !CSTOPB and any parity mode seems ok, but
> > > > - *   - the chip doesn't have the function to calculate parity
> > > > - *     in !CS8 mode.
> > > > - *   - it is unclear that the chip supports CS5,6 mode.
> > > > - *   - it is unclear how to handle stop bits.
> > > > + *   work as chip default - CS8, no parity, !CSTOPB
> > > > + *   other modes are not supported.
> > > >   */

Given that 8N1 is supported, can we not support other modes entirely in software?
7E1, and 7O1 don't need explicit hardware support.

If you need 7N1, 7M1 will simulate 7N2, which the majority of hardware will work
with, (an extra stop bit).

If 8N1 works, and there is confusion over support for other modes, just bit bang
in 8N1 mode.

--
Creamy! <3