stm32: Don't perform any side effects in irq_update().

[pfalcon]

The purpose of irq_update() is to cache value of UART IRQ status
register for devices which needs such caching. No other driver
performs any other side effects in this call. For STM32, clearing
TC (tx complete) bit was introduced in 8c079e9
which is otherwise titled as a conversion to STM32Cube HAL. Thus,
there does not seem to be specific reasons why this code was added.
On the other hand, it leads to behaviorial artifacts when dealing
with interrup-driven UART code (specific issue seen was delaying
of transmitting every other character).

Change-Id: Id20bf214b36eeb6c09e29cc2e6bfca4f7221a1a4
Signed-off-by: Paul Sokolovsky [email protected]

---

This change is 

[pfalcon]

Migrated from https://gerrit.zephyrproject.org/r/#/c/12917/ (which has a lot of discussion!)

[pfalcon]

@erwango , @galak , @daniel-thompson

[pfalcon]

@lag-linaro : please review

[lag-linaro]

You require a review from the author of the original snip-it.

[pfalcon]

@lag-linaro : Thanks. If you don't have concerns with this patch and find explanations in the commit messages convincing, (and of course if you have time) can you please approve this PR (either with github's native approval, or by just posting +1 comment)? Without this patch, #49 doesn't work as expected.

[pfalcon]

Btw, with #42 merged, nothing blocks this patch.

[daniel-thompson]

I had understood the underlying problem was that the (broken) test "forced" the STM32 driver into using the latched TC (tx-complete) rather than the level sensitive TE (tx-double-buffer-empty) interrupt as a source of tx interrupt.

That change avoids acking the TC interrupt but still uses TC rather than TE as the source of interrupt.

@erwango: I think this means using the TC interrupt means that the interrupt will always run too late (e.g. after the UART has gone idle) leading to gaps related to interrupt latency between characters. I admit I'm doing this mostly from memory (i.e. without reference manual). Do I have the role of TC and TE correct?

[erwango]

@daniel-thompson: Indeed TC is Transmition Complete, while TE is TXEmpty. I'm not sure about the double buffer though. One possible issue is that in some system using TE can lead to not transmitting the last byte of the data you want to transmit, as you can enter sleep mode just after for instance.
In the other hand, TE will enable faster transmition (only in specific cases I think). But I'm not sure data rate is what we want from UART.

[pfalcon]

I think this means using the TC interrupt means that the interrupt will always run too late (e.g. after the UART has gone idle) leading to gaps related to interrupt latency between characters. I admit I'm doing this mostly from memory (i.e. without reference manual).

So, from my memory either, but the reason I'm after this patch is because with this code in, #49 worked following way: you press a key, nothing output. Then you press second key, both chars appear.

[pfalcon]

but still uses TC rather than TE as the source of interrupt.

Handling that would be subject of the next patch, requires cross-arch research (I remember @daniel-thompson that you did one, I appreciate and trust it, but I'd like to do it formally and fill in this table: https://drive.google.com/open?id=1e4hx9iVmAp5pBSTd2namF_VdsiMBS-OmlJUI4UPBMN4)

That change avoids acking the TC interrupt

So, @erwango reports breakage with #42 in the "arm" branch (details TBC). I think that's good sign: test works and detects that something's wrong ;-). But I start to wonder if we really should standardize on the "completion" interrupt to behave like edge-triggered, i.e. give user UART ISR a chance to handle it, but not spam ISR with it repeatedly. But we'd need to do that smarter that the code being removed in this patch.

From IRC:

danielt: Hi, so in our previous discussions, did you want to say (or said, but myself concentrating on other things) that UART "tx complete" IRQ should be naturally an edge one (so we may need to emulate that edge-ness for platforms where it isn't)?

[pfalcon]

using TE can lead to not transmitting the last byte of the data you want to transmit, as you can enter sleep mode just after for instance.

So, that's the point: to transmit well (especially in the case of real FIFO), you need to use what STM32 calls TE, but to got to sleep, you need to watch for TC first. They're both needed and can't replace one another, and we need to handle them well - for STM32, and in general.

[lag-linaro]

My +1 would be meaningless here, since I don't know if it's the right thing to do.

This is why I suggested that you require either the original author or another SME to +1.

[pfalcon]

Per https://docs.google.com/spreadsheets/d/1e4hx9iVmAp5pBSTd2namF_VdsiMBS-OmlJUI4UPBMN4/edit#gid=2127015340, this patch works as expected on 96b_carbon on the current master, and on current arm branch, being rebased on master, to pick up #42 fix.

[erwango]

Tested ok on nucleo_f411re on both current master and arm branch

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[daniel-thompson]

TC versus TE serve different purposes don't they? TE is used to manage data flow (FIFO or double buffer refill) whilst TC is used to manage power (and perhaps to manage half duplex serial links).

Using TC to simulate TE simply results in risk of needless buffer backlogs emerging (since we cannot transmit as fast as we can receive).