Pete,
You're absolutely right. My apology again, I re-checked values of MSR masks used to update icount counters and they all belong to upper MSR nibble, so they indeed track changes (not states). I took out counter increments from mct_u232_msr_to_state function and encapsulated them in the newly defined mct_u232_msr_to_icount function, effectively leaving the previous implementation of mct_u232_msr_to_state function intact. mct_u232_msr_to_icount uses delta nibble to track state changes. RM-60 testing showed the same levels as measured by PDM-2 in close proximity to RM-60. Please let me know if anything else needs to be corrected to roll this out,
Thank you again for your expertise,
Vadim.
--- On Mon, 12/27/10, Pete Zaitcev <[email protected]> wrote:
> From: Pete Zaitcev <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions
> To: "Tsozik" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected]
> Date: Monday, December 27, 2010, 12:08 PM
> On Sun, 26 Dec 2010 22:34:23 -0800
> (PST)
> Tsozik <[email protected]>
> wrote:
>
> > Usually MSR contains information about both states
> which are raised and
> > states which are changed. It looks like we use upper
> MSR nibble which
> > contains information about 4 states which were raised,
> not lower MSR
> > nibble which contains information about 4 states which
> were changed.
>
> Sure, I know how typical UART works. Does the MCT device
> deliver the
> change states? If it does and you want to use those, go
> ahead.
>
> > I also researched implementations under usb/serial and
> saw that all of
> > them are rely on the current states (not change
> states) to increment
> > the respective counter variables.
>
> Here's drivers/usb/serial/io_edgeport.c:
>
> static void handle_new_msr(struct edgeport_port *edge_port,
> __u8 newMsr)
> {
> ??? struct? async_icount *icount;
>
> ??? dbg("%s %02x", __func__, newMsr);
>
> ??? if (newMsr & (EDGEPORT_MSR_DELTA_CTS
> | EDGEPORT_MSR_DELTA_DSR |
> ??? ??? ???
> EDGEPORT_MSR_DELTA_RI | EDGEPORT_MSR_DELTA_CD)) {
> ??? ??? icount =
> &edge_port->icount;
>
> ??? ??? /* update input line
> counters */
> ??? ??? if (newMsr &
> EDGEPORT_MSR_DELTA_CTS)
> ??? ??? ???
> icount->cts++;
> ??? ??? if (newMsr &
> EDGEPORT_MSR_DELTA_DSR)
> ??? ??? ???
> icount->dsr++;
> ??? ??? if (newMsr &
> EDGEPORT_MSR_DELTA_CD)
> ??? ??? ???
> icount->dcd++;
> ??? ??? if (newMsr &
> EDGEPORT_MSR_DELTA_RI)
> ??? ??? ???
> icount->rng++;
> ??? ???
> wake_up_interruptible(&edge_port->delta_msr_wait);
> ??? }
>
> Looks like it counts changes to me. And in any case, the
> gold standard
> is in drivers/serial/8250.c:
>
> static unsigned int check_modem_status(struct
> uart_8250_port *up)
> {
> ??? unsigned int status = serial_in(up,
> UART_MSR);
>
> ??? status |= up->msr_saved_flags;
> ??? up->msr_saved_flags = 0;
> ??? if (status & UART_MSR_ANY_DELTA
> && up->ier & UART_IER_MSI &&
> ??? ? ? up->port.state != NULL)
> {
> ??? ??? if (status &
> UART_MSR_TERI)
> ??? ??? ???
> up->port.icount.rng++;
> ??? ??? if (status &
> UART_MSR_DDSR)
> ??? ??? ???
> up->port.icount.dsr++;
> ??? ??? if (status &
> UART_MSR_DDCD)
> ??? ??? ???
> uart_handle_dcd_change(&up->port, status &
> UART_MSR_DCD);
> ??? ??? if (status &
> UART_MSR_DCTS)
> ??? ??? ???
> uart_handle_cts_change(&up->port, status &
> UART_MSR_CTS);
>
> ??? ???
> wake_up_interruptible(&up->port.state->port.delta_msr_wait);
> ??? }
>
> I only proposed accomplishing the same result by comparing
> states
> in place of relying on hardware change reports like both of
> the
> above do.
>
> -- Pete
>
On Mon, 27 Dec 2010 12:55:38 -0800 (PST)
Tsozik <[email protected]> wrote:
> +static void mct_u232_msr_to_icount(struct async_icount *icount,
> + unsigned char msr)
> +{
> + /* Translate Control Line states */
> + if (msr & MCT_U232_MSR_DDSR)
> + icount->dsr++;
> + if (msr & MCT_U232_MSR_DCTS)
> + icount->cts++;
> + if (msr & MCT_U232_MSR_DRI)
> + icount->rng++;
> + if (msr & MCT_U232_MSR_DCD)
> + icount->dcd++;
> +} /* mct_u232_msr_to_icount */
This looks good to me, but since it's a new hardware facility that
we did not use previously, I'd like to make sure this works for you.
> mct_u232_msr_to_icount uses delta nibble to track state changes.
> RM-60 testing showed the same levels as measured by PDM-2 in close
> proximity to RM-60.
Right, this is good. I have just one request for you: could you find
somewhere a computer with a built-in 8250-compatible serial port,
attach RM-60 to it, and run your counting application there?
I think your patch looks just fine, but I would like to make sure
that we (e.g. mct_u232+patch) are truly compatible now.
-- Pete
Pete,
Many thanks for your reply again. I actually still own an antique HP a230n (disclaimer: I got it from/with my wife and would never buy it myself). According to dmesg it's got built-in 8250-compliant serial:
[vtsozik@f15 serial]$ dmesg | grep -i serial | grep 8250
Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
[vtsozik@f15 serial]$
So I ran geiger counter against /dev/ttyS0 device for 20 minutes and acquired 20 measurements. Then I compared last average with last 20 minute measurement average acquired via mct_u232 on the laptop placed nearby. The error was ~4% (rounded up). Taking in consideration that measurements were not acquired synchronously (i.e. in 2 different time slots) this is a pretty good match.
Thank you again,
Vadim.
--- On Mon, 12/27/10, Pete Zaitcev <[email protected]> wrote:
> From: Pete Zaitcev <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions
> To: "Tsozik" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected], [email protected]
> Date: Monday, December 27, 2010, 5:12 PM
> On Mon, 27 Dec 2010 12:55:38 -0800
> (PST)
> Tsozik <[email protected]>
> wrote:
>
> > +static void mct_u232_msr_to_icount(struct
> async_icount *icount,
> > +??? ???
> ??? ??? ???
> ??? unsigned char msr)
> > +{
> > +??? /* Translate Control Line states
> */
> > +??? if (msr & MCT_U232_MSR_DDSR)
> > +??? ???
> icount->dsr++;
> > +??? if (msr & MCT_U232_MSR_DCTS)
> > +??? ???
> icount->cts++;
> > +??? if (msr & MCT_U232_MSR_DRI)
> > +??? ???
> icount->rng++;
> > +??? if (msr & MCT_U232_MSR_DCD)
> > +??? ???
> icount->dcd++;
> > +} /* mct_u232_msr_to_icount */
>
> This looks good to me, but since it's a new hardware
> facility that
> we did not use previously, I'd like to make sure this works
> for you.
>
> > mct_u232_msr_to_icount uses delta nibble to track
> state changes.
> > RM-60 testing showed the same levels as measured by
> PDM-2 in close
> > proximity to RM-60.
>
> Right, this is good. I have just one request for you: could
> you find
> somewhere a computer with a built-in 8250-compatible serial
> port,
> attach RM-60 to it, and run your counting application
> there?
> I think your patch looks just fine, but I would like to
> make sure
> that we (e.g. mct_u232+patch) are truly compatible now.
>
> -- Pete
>
On Mon, 27 Dec 2010 20:04:51 -0800 (PST)
Tsozik <[email protected]> wrote:
> So I ran geiger counter against /dev/ttyS0 device for 20 minutes and
> acquired 20 measurements. Then I compared last average with last 20
> minute measurement average acquired via mct_u232 on the laptop placed
> nearby. The error was ~4% (rounded up).
Great, I'm ready to ack.
There's just one thing that is bugging me... I think it would be best
if Alan Cox or Greg Kroah commented on it. The edgeport does the
following, which we copied:
schedule();
........
if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
return -EIO; /* no change => error */
if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
return 0;
}
So, if there was a status report, but no change to bits, the ioctl
TIOCMIWAIT would return with -EIO. In serial_core.c, that serves
conventional non-USB UARTs, nothing like this occurs. I am not quite
sure what the point of doing this -EIO check is.
Oh and BTW, I'm wondering what is going to happen if the device is
disconnected while an application is blocked waiting for the status
change. The patch is not particularly bad here, it just copies
an existing code from elsewhere.
-- Pete
Greg,
I'm sorry to bother you again, but I'm wondering if you could comment on Pete's concern below.
Thank you in advance for your expertise on the matter,
Vadim.
--- On Tue, 12/28/10, Pete Zaitcev <[email protected]> wrote:
> From: Pete Zaitcev <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions
> To: "Tsozik" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected], [email protected]
> Date: Tuesday, December 28, 2010, 1:40 AM
> On Mon, 27 Dec 2010 20:04:51 -0800
> (PST)
> Tsozik <[email protected]>
> wrote:
>
> > So I ran geiger counter against /dev/ttyS0 device for
> 20 minutes and
> > acquired 20 measurements. Then I compared last average
> with last 20
> > minute measurement average acquired via mct_u232 on
> the laptop placed
> > nearby. The error was ~4% (rounded up).
>
> Great, I'm ready to ack.
>
> There's just one thing that is bugging me... I think it
> would be best
> if Alan Cox or Greg Kroah commented on it. The edgeport
> does the
> following, which we copied:
>
>
> ??? ??? schedule();
> ??? ??? ........
> ??? ??? if (cnow.rng ==
> cprev.rng && cnow.dsr == cprev.dsr &&
> ??? ??? ? ?
> cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
> ??? ??? ???
> return -EIO; /* no change => error */
> ??? ??? if (((arg &
> TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
> ??? ??? ? ? ((arg
> & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
> ??? ??? ? ? ((arg
> & TIOCM_CD)? && (cnow.dcd != cprev.dcd))
> ||
> ??? ??? ? ? ((arg
> & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
> ??? ??? ???
> return 0;
> ??? ??? }
>
> So, if there was a status report, but no change to bits,
> the ioctl
> TIOCMIWAIT would return with -EIO. In serial_core.c, that
> serves
> conventional non-USB UARTs, nothing like this occurs. I am
> not quite
> sure what the point of doing this -EIO check is.
>
> Oh and BTW, I'm wondering what is going to happen if the
> device is
> disconnected while an application is blocked waiting for
> the status
> change. The patch is not particularly bad here, it just
> copies
> an existing code from elsewhere.
>
> -- Pete
>