Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uart: Add a "txdone" register, set compat to "sifive,uart1" #90

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@palmer-dabbelt
Copy link

commented Oct 19, 2018

The UART is a pretty small device, so its clock tends to be coupled to
at least some other part of the SOC. As a result, users may want to
change the UART's clock frequency in close proximity to transmitting a
character out of the UART. The UART currently does not actually expose
anything that software can read in order to check to see if transmission
is done -- users can check to see if the FIFO is empty, but at that
point there can still be up to one UART word being transmitted.

Even in the fastest modes, UART word transmission happens at about 10KHz
which is sufficiently slower than most processor/bus speeds. As a
result, it is possible for the processor to see that the TX FIFO is
empty before the last word has transmitted and then change the UART's
clock, which corrupts that word. The window is small, but this issue
manifests during Zephyr boot on the HiFive1 so it's not just
theoretical.

My fix is to plumb out the busy bit from the TX module to a register
that software can access, called "txdone". I added this as a whole new
register rather than sticking it into txctrl because there's a
variable-length field at the end of txctrl and I didn't want to have a
complicated memory map. This bit informs software that the TX queue is
empty and the last word has actually been transmitted. In order to
inform software of the new register I've bumped the device tree's compat
string.

The semantics of the bit are intended to allow software to ensure that
no word is currently transmitting from the UART so the clock can safely
be changed before resuming transmission. This bit is designed to allow
for two modes:

  • Software wants to change the clock ASAP. In this case TX is disabled,
    which causes the TX FIFO to stop firing. As a result, when TX is
    disabled it doesn't matter if there are words in the FIFO so the done
    bit is just directly the busy bit.
  • Software wants to change the clock when nothing is busy. In this
    case TX is left enabled, so the TX FIFO must be empty before the clock
    can be changed. As a result, when TX is enabled the done bit can only
    be set when the FIFO is empty.

As usual, I've given this patch absolutely no testing: in this case I
haven't even compiled it. I wrote most of this before 9am, so I'm sure
it's broken :)

@palmer-dabbelt

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

@nategraff-sifive informs me that this should really have the device tree compat string set to 'compat = "sifive,uart1", "sifive,uart0";' because this is backwards-compatible. I've updated the PR accordingly.

@mwachs5
Copy link
Contributor

left a comment

This is a read-only register field, right?


UARTCtrlRegs.txdone -> Seq(
RegField(1, txm.io.done,
RegFieldDesc("txdone","Transmit finished",reset=Some(1))))

This comment has been minimized.

Copy link
@mwachs5

mwachs5 Oct 19, 2018

Contributor

this is a read-only register, so you should use RegField.r(...). Also, should set volatile=true inside the RegFieldDesc because this may change between reads to it. (note that the RegField.r(...) will automatically apply the Read-Only attribute to the description).

This comment has been minimized.

Copy link
@palmer-dabbelt

palmer-dabbelt Oct 22, 2018

Author

Thanks, I believe these should be fixed.

@mwachs5

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Also, 👍 for this feature

@paul-walmsley-sifive
Copy link

left a comment

First: great commit message.

Second: the idea behind the change looks good, and we should definitely merge this fix or one like it once the hardware folks are happy with it. But we should also pursue a fix on the SoC clock integration side. A better overall approach to solve the specific problem of rate changes on the UART source clock is simply not to change the UART source clock at all (or SPI, I2C, I2S, etc.). Otherwise the UART receive side is either non-functional or might receive garbage during the PLL change, and that's not acceptable for many customer use-cases.

There are two usual ways that other SoCs have dealt with this:

  1. Clock the UART shift register and FIFO from the external (fixed-rate) HF clk crystal/resonator. UART line protocol is async so this should be OK from a jitter point of view. This option is also nice from a chip bringup point of view since it completely isolates the debug UART from any PLL-related problems.

  2. Clock the UART shift register and FIFO from a dedicated peripherals PLL, which would also act as a source clock to I2C, SPI, etc.

Ideally both options would be available via a clock mux.

@mwachs5

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

@paul-walmsley-sifive this code already supports the ability to run the UART off a constant/more dedicated clock source. Agree that is the right way to clock the UART but I think this feature is still useful, even if you never wanted to change the clock it would be nice to know when the transmission is complete.

@paul-walmsley-sifive

This comment has been minimized.

Copy link

commented Oct 19, 2018

@mwachs5 Yep, just to clarify, I think the idea behind this patch is great, and endorse it. I personally can't comment on the specific implementation since I'm not familiar enough with Chisel yet. So yeah, I'm in favor of it, no matter what happens to the SoC integration.

Regarding the integration: initial thought here was to separate the UART TX/RX logic from the UART register target, and synchronize between the two Sounds like you were thinking of placing the CDC between the interconnect and the entire UART IP. Any opinions on pros/cons of those two approaches? Would naively assume that the former case would be more efficient from the CPU and interconnect perspective, although it seems more complex to implement in Chisel.

@mwachs5

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Sounds like you were thinking of placing the CDC between the interconnect and the entire UART IP.

Yes, this is already supported by this codebase

uart: Add a "txdone" register, set compat to "sifive,uart1"
The UART is a pretty small device, so its clock tends to be coupled to
at least some other part of the SOC.  As a result, users may want to
change the UART's clock frequency in close proximity to transmitting a
character out of the UART.  The UART currently does not actually expose
anything that software can read in order to check to see if transmission
is done -- users can check to see if the FIFO is empty, but at that
point there can still be up to one UART word being transmitted.

Even in the fastest modes, UART word transmission happens at about 10KHz
which is sufficiently slower than most processor/bus speeds.  As a
result, it is possible for the processor to see that the TX FIFO is
empty before the last word has transmitted and then change the UART's
clock, which corrupts that word.  The window is small, but this issue
manifests during Zephyr boot on the HiFive1 so it's not just
theoretical.

My fix is to plumb out the busy bit from the TX module to a register
that software can access, called "txdone".  I added this as a whole new
register rather than sticking it into txctrl because there's a
variable-length field at the end of txctrl and I didn't want to have a
complicated memory map.  This bit informs software that the TX queue is
empty and the last word has actually been transmitted.  In order to
inform software of the new register I've bumped the device tree's compat
string.

The semantics of the bit are intended to allow software to ensure that
no word is currently transmitting from the UART so the clock can safely
be changed before resuming transmission.  This bit is designed to allow
for two modes:

* Software wants to change the clock ASAP.  In this case TX is disabled,
  which causes the TX FIFO to stop firing.  As a result, when TX is
  disabled it doesn't matter if there are words in the FIFO so the done
  bit is just directly the busy bit.
* Software wants to change the clock when nothing is busy.  In this
  case TX is left enabled, so the TX FIFO must be empty before the clock
  can be changed.  As a result, when TX is enabled the done bit can only
  be set when the FIFO is empty.

In order to allow software to detect the presence of this new bit, I've
added the "sifive,uart1" compat string.  Since this device is backwards
compatible it retains the "sifive,uart0" string.

As usual, I've given this patch absolutely no testing: in this case I
haven't even compiled it.  I wrote most of this before 9am, so I'm sure
it's broken :)
@palmer-dabbelt

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

@paul-walmsley-sifive, @mwachs5 I believe I've updated the PR to take your review into account.

Thanks!

@palmer-dabbelt

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

ping

@paul-walmsley-sifive

This comment has been minimized.

Copy link

commented Jul 9, 2019

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.