| Message ID | 20180511022751.9096-1-samuel@sholland.org |
|---|---|
| Headers | show |
| Series | Allwinner A64 timer workaround | expand |
Hi, On 11/05/18 03:27, Samuel Holland wrote: > Hello, > > Several people (including me) have experienced extremely large system > clock jumps on their A64-based devices, apparently due to the > architectural timer going backward, which is interpreted by Linux as > the timer wrapping around after 2^56 cycles. So I experienced this before, though I didn't see actual clock jumps, only that subsequent reads of CNTVCT_EL0, both directly via mrs and indirectly via CLOCK_MONOTONIC, from userland (triggered by a directed test) *sometimes* went backwards, with a number of '1's in the lower bits. But that didn't happen on every boot, and I was suspecting that some timer setup was missing on the hardware/firmware side. And later on I failed to reproduce it anymore. So do you see it on every boot, with recent U-Boot/ATF? Cheers, Andre. > Investigation led to discovery of some obvious problems with this SoC's > architectural timer, and this patch series introduces what I believe is > the simplest workaround. More details are in the commit message for > patch 1. Patch 2 simply enables the workaround in the device tree. > > Thanks, > Samuel > > Samuel Holland (2): > arm64: arch_timer: Workaround for Allwinner A64 timer instability > arm64: dts: allwinner: a64: Enable A64 timer workaround > > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 1 + > drivers/clocksource/Kconfig | 11 ++++++++ > drivers/clocksource/arm_arch_timer.c | 39 +++++++++++++++++++++++++++ > 3 files changed, 51 insertions(+) > > -- > 2.16.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 11/05/18 03:27, Samuel Holland wrote: > Hello, > > Several people (including me) have experienced extremely large system > clock jumps on their A64-based devices, apparently due to the > architectural timer going backward, which is interpreted by Linux as > the timer wrapping around after 2^56 cycles. > > Investigation led to discovery of some obvious problems with this SoC's > architectural timer, and this patch series introduces what I believe is > the simplest workaround. More details are in the commit message for > patch 1. Patch 2 simply enables the workaround in the device tree. What's the deal with this series? There was a couple of nits to address, and I was more or less expecting a v2. Thanks, M.
On 07/03/18 10:09, Marc Zyngier wrote: > On 11/05/18 03:27, Samuel Holland wrote: >> Hello, >> >> Several people (including me) have experienced extremely large system >> clock jumps on their A64-based devices, apparently due to the architectural >> timer going backward, which is interpreted by Linux as the timer wrapping >> around after 2^56 cycles. >> >> Investigation led to discovery of some obvious problems with this SoC's >> architectural timer, and this patch series introduces what I believe is >> the simplest workaround. More details are in the commit message for patch >> 1. Patch 2 simply enables the workaround in the device tree. > > What's the deal with this series? There was a couple of nits to address, and > I was more or less expecting a v2. I got reports that people were still occasionally having clock jumps after applying this series, so I wanted to attempt a more complete fix, but I haven't had time to do any deeper investigation. I think this series is still beneficial even if it's not a complete solution, so I'll come back with another patch on top of this if/once I get it fully fixed. I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz timer) ≈ 150 should be a conservative iteration limit? Also, does this make sense to CC to stable? Thanks, Samuel
On 03/07/18 19:42, Samuel Holland wrote: > On 07/03/18 10:09, Marc Zyngier wrote: >> On 11/05/18 03:27, Samuel Holland wrote: >>> Hello, >>> >>> Several people (including me) have experienced extremely large system >>> clock jumps on their A64-based devices, apparently due to the architectural >>> timer going backward, which is interpreted by Linux as the timer wrapping >>> around after 2^56 cycles. >>> >>> Investigation led to discovery of some obvious problems with this SoC's >>> architectural timer, and this patch series introduces what I believe is >>> the simplest workaround. More details are in the commit message for patch >>> 1. Patch 2 simply enables the workaround in the device tree. >> >> What's the deal with this series? There was a couple of nits to address, and >> I was more or less expecting a v2. > > I got reports that people were still occasionally having clock jumps after > applying this series, so I wanted to attempt a more complete fix, but I haven't > had time to do any deeper investigation. I think this series is still beneficial > even if it's not a complete solution, so I'll come back with another patch on > top of this if/once I get it fully fixed. > > I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz > timer) ≈ 150 should be a conservative iteration limit? Should be OK. Maxime: How do you want to deal with the documentation aspect? We need an erratum number, but AFAIU the concept hasn't made it into the silicom vendor's brain yet. Any chance you could come up with something that uniquely identifies this? > Also, does this make sense to CC to stable? Probably not, as the HW never worked, so it is not a regression. Thanks, M.
On Wed, Jul 4, 2018 at 4:16 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 03/07/18 19:42, Samuel Holland wrote: >> On 07/03/18 10:09, Marc Zyngier wrote: >>> On 11/05/18 03:27, Samuel Holland wrote: >>>> Hello, >>>> >>>> Several people (including me) have experienced extremely large system >>>> clock jumps on their A64-based devices, apparently due to the architectural >>>> timer going backward, which is interpreted by Linux as the timer wrapping >>>> around after 2^56 cycles. >>>> >>>> Investigation led to discovery of some obvious problems with this SoC's >>>> architectural timer, and this patch series introduces what I believe is >>>> the simplest workaround. More details are in the commit message for patch >>>> 1. Patch 2 simply enables the workaround in the device tree. >>> >>> What's the deal with this series? There was a couple of nits to address, and >>> I was more or less expecting a v2. >> >> I got reports that people were still occasionally having clock jumps after >> applying this series, so I wanted to attempt a more complete fix, but I haven't >> had time to do any deeper investigation. I think this series is still beneficial >> even if it's not a complete solution, so I'll come back with another patch on >> top of this if/once I get it fully fixed. >> >> I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz >> timer) ≈ 150 should be a conservative iteration limit? > > Should be OK. > > Maxime: How do you want to deal with the documentation aspect? We need > an erratum number, but AFAIU the concept hasn't made it into the silicom > vendor's brain yet. Any chance you could come up with something that > uniquely identifies this? > >> Also, does this make sense to CC to stable? > > Probably not, as the HW never worked, so it is not a regression. A64 support has been in for a few releases now. So while this is not fixing a regression, people will still benefit from it being in stable. ChenYu
On 04/07/2018 10:16, Marc Zyngier wrote: > On 03/07/18 19:42, Samuel Holland wrote: >> On 07/03/18 10:09, Marc Zyngier wrote: >>> On 11/05/18 03:27, Samuel Holland wrote: >>>> Hello, >>>> >>>> Several people (including me) have experienced extremely large system >>>> clock jumps on their A64-based devices, apparently due to the architectural >>>> timer going backward, which is interpreted by Linux as the timer wrapping >>>> around after 2^56 cycles. >>>> >>>> Investigation led to discovery of some obvious problems with this SoC's >>>> architectural timer, and this patch series introduces what I believe is >>>> the simplest workaround. More details are in the commit message for patch >>>> 1. Patch 2 simply enables the workaround in the device tree. >>> >>> What's the deal with this series? There was a couple of nits to address, and >>> I was more or less expecting a v2. >> >> I got reports that people were still occasionally having clock jumps after >> applying this series, so I wanted to attempt a more complete fix, but I haven't >> had time to do any deeper investigation. I think this series is still beneficial >> even if it's not a complete solution, so I'll come back with another patch on >> top of this if/once I get it fully fixed. >> >> I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz >> timer) ≈ 150 should be a conservative iteration limit? > > Should be OK. > > Maxime: How do you want to deal with the documentation aspect? We need > an erratum number, but AFAIU the concept hasn't made it into the silicom > vendor's brain yet. Any chance you could come up with something that > uniquely identifies this? > >> Also, does this make sense to CC to stable? > > Probably not, as the HW never worked, so it is not a regression. If the patches fix a bug which already exist, it makes sense to propagated the fix back to the stable versions.
On 04/07/18 09:23, Daniel Lezcano wrote: > On 04/07/2018 10:16, Marc Zyngier wrote: >> On 03/07/18 19:42, Samuel Holland wrote: >>> On 07/03/18 10:09, Marc Zyngier wrote: >>>> On 11/05/18 03:27, Samuel Holland wrote: >>>>> Hello, >>>>> >>>>> Several people (including me) have experienced extremely large system >>>>> clock jumps on their A64-based devices, apparently due to the architectural >>>>> timer going backward, which is interpreted by Linux as the timer wrapping >>>>> around after 2^56 cycles. >>>>> >>>>> Investigation led to discovery of some obvious problems with this SoC's >>>>> architectural timer, and this patch series introduces what I believe is >>>>> the simplest workaround. More details are in the commit message for patch >>>>> 1. Patch 2 simply enables the workaround in the device tree. >>>> >>>> What's the deal with this series? There was a couple of nits to address, and >>>> I was more or less expecting a v2. >>> >>> I got reports that people were still occasionally having clock jumps after >>> applying this series, so I wanted to attempt a more complete fix, but I haven't >>> had time to do any deeper investigation. I think this series is still beneficial >>> even if it's not a complete solution, so I'll come back with another patch on >>> top of this if/once I get it fully fixed. >>> >>> I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz >>> timer) ≈ 150 should be a conservative iteration limit? >> >> Should be OK. >> >> Maxime: How do you want to deal with the documentation aspect? We need >> an erratum number, but AFAIU the concept hasn't made it into the silicom >> vendor's brain yet. Any chance you could come up with something that >> uniquely identifies this? >> >>> Also, does this make sense to CC to stable? >> >> Probably not, as the HW never worked, so it is not a regression. > > If the patches fix a bug which already exist, it makes sense to > propagated the fix back to the stable versions. That's your call, but I'm not supportive of that decision, specially as we have information from the person developing the workaround that this doesn't fully address the issue. M.
On 04/07/2018 10:16, Marc Zyngier wrote: > On 03/07/18 19:42, Samuel Holland wrote: >> On 07/03/18 10:09, Marc Zyngier wrote: >>> On 11/05/18 03:27, Samuel Holland wrote: >>>> Hello, >>>> >>>> Several people (including me) have experienced extremely large system >>>> clock jumps on their A64-based devices, apparently due to the architectural >>>> timer going backward, which is interpreted by Linux as the timer wrapping >>>> around after 2^56 cycles. >>>> >>>> Investigation led to discovery of some obvious problems with this SoC's >>>> architectural timer, and this patch series introduces what I believe is >>>> the simplest workaround. More details are in the commit message for patch >>>> 1. Patch 2 simply enables the workaround in the device tree. >>> >>> What's the deal with this series? There was a couple of nits to address, and >>> I was more or less expecting a v2. >> >> I got reports that people were still occasionally having clock jumps after >> applying this series, so I wanted to attempt a more complete fix, but I haven't >> had time to do any deeper investigation. I think this series is still beneficial >> even if it's not a complete solution, so I'll come back with another patch on >> top of this if/once I get it fully fixed. >> >> I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz >> timer) ≈ 150 should be a conservative iteration limit? > > Should be OK. > > Maxime: How do you want to deal with the documentation aspect? We need > an erratum number, but AFAIU the concept hasn't made it into the silicom > vendor's brain yet. Any chance you could come up with something that > uniquely identifies this? I went through the different pointers provided in the description but I did not find a clear statement that is a hardware issue or may be I missed it. Are we sure there isn't another subsystem responsible on this instability ? (eg PM or something else) >> Also, does this make sense to CC to stable? > > Probably not, as the HW never worked, so it is not a regression. > > Thanks, > > M. >
On 03/07/2018 20:42, Samuel Holland wrote: > On 07/03/18 10:09, Marc Zyngier wrote: >> On 11/05/18 03:27, Samuel Holland wrote: >>> Hello, >>> >>> Several people (including me) have experienced extremely large system >>> clock jumps on their A64-based devices, apparently due to the architectural >>> timer going backward, which is interpreted by Linux as the timer wrapping >>> around after 2^56 cycles. >>> >>> Investigation led to discovery of some obvious problems with this SoC's >>> architectural timer, and this patch series introduces what I believe is >>> the simplest workaround. More details are in the commit message for patch >>> 1. Patch 2 simply enables the workaround in the device tree. >> >> What's the deal with this series? There was a couple of nits to address, and >> I was more or less expecting a v2. > > I got reports that people were still occasionally having clock jumps after > applying this series, so I wanted to attempt a more complete fix, but I haven't > had time to do any deeper investigation. I think this series is still beneficial > even if it's not a complete solution, so I'll come back with another patch on > top of this if/once I get it fully fixed. > > I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz > timer) ≈ 150 should be a conservative iteration limit? > > Also, does this make sense to CC to stable? I understand a partial fix is better than nothing but if you can narrow down the issue and provide patches to fix it in one shot that would be awesome.
On Wed, Jul 04, 2018 at 09:16:32AM +0100, Marc Zyngier wrote: > On 03/07/18 19:42, Samuel Holland wrote: > > On 07/03/18 10:09, Marc Zyngier wrote: > >> On 11/05/18 03:27, Samuel Holland wrote: > >>> Hello, > >>> > >>> Several people (including me) have experienced extremely large system > >>> clock jumps on their A64-based devices, apparently due to the architectural > >>> timer going backward, which is interpreted by Linux as the timer wrapping > >>> around after 2^56 cycles. > >>> > >>> Investigation led to discovery of some obvious problems with this SoC's > >>> architectural timer, and this patch series introduces what I believe is > >>> the simplest workaround. More details are in the commit message for patch > >>> 1. Patch 2 simply enables the workaround in the device tree. > >> > >> What's the deal with this series? There was a couple of nits to address, and > >> I was more or less expecting a v2. > > > > I got reports that people were still occasionally having clock jumps after > > applying this series, so I wanted to attempt a more complete fix, but I haven't > > had time to do any deeper investigation. I think this series is still beneficial > > even if it's not a complete solution, so I'll come back with another patch on > > top of this if/once I get it fully fixed. > > > > I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz > > timer) ≈ 150 should be a conservative iteration limit? > > Should be OK. > > Maxime: How do you want to deal with the documentation aspect? We need > an erratum number, but AFAIU the concept hasn't made it into the silicom > vendor's brain yet. Any chance you could come up with something that > uniquely identifies this? Yeah, I don't know how we can address that unfortunately. Or maybe we can call it timer-broken-1 ? It's as good as an ID than any other ID :) Maxime
On Wed, 4 Jul 2018, Marc Zyngier wrote: > On 04/07/18 09:23, Daniel Lezcano wrote: > > > > If the patches fix a bug which already exist, it makes sense to > > propagated the fix back to the stable versions. > > That's your call, but I'm not supportive of that decision, specially as > we have information from the person developing the workaround that this > doesn't fully address the issue. The patches should not be applied at all. Simply because they don't fix the issue completely. >From a quick glance at various links and information about this, this very much smells like the FSL_ERRATUM_A008585. Has that been tried? It looks way more robust than the magic 11 bit crystal ball logic. Thanks, tglx
Hi, On 03/07/18 19:42, Samuel Holland wrote: > On 07/03/18 10:09, Marc Zyngier wrote: >> On 11/05/18 03:27, Samuel Holland wrote: >>> Hello, >>> >>> Several people (including me) have experienced extremely large system >>> clock jumps on their A64-based devices, apparently due to the architectural >>> timer going backward, which is interpreted by Linux as the timer wrapping >>> around after 2^56 cycles. >>> >>> Investigation led to discovery of some obvious problems with this SoC's >>> architectural timer, and this patch series introduces what I believe is >>> the simplest workaround. More details are in the commit message for patch >>> 1. Patch 2 simply enables the workaround in the device tree. >> >> What's the deal with this series? There was a couple of nits to address, and >> I was more or less expecting a v2. > > I got reports that people were still occasionally having clock jumps after > applying this series, so I wanted to attempt a more complete fix, but I haven't > had time to do any deeper investigation. Looking at the FSL workaround, I see that they cover TVAL reads as well. Not sure entirely why, but maybe it's worth to follow this lead? Cheers, Andre. > I think this series is still beneficial > even if it's not a complete solution, so I'll come back with another patch on > top of this if/once I get it fully fixed. > > I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz > timer) ≈ 150 should be a conservative iteration limit? > > Also, does this make sense to CC to stable? > > Thanks, > Samuel >
Hi, On 04/07/18 11:00, Thomas Gleixner wrote: > On Wed, 4 Jul 2018, Marc Zyngier wrote: >> On 04/07/18 09:23, Daniel Lezcano wrote: >>> >>> If the patches fix a bug which already exist, it makes sense to >>> propagated the fix back to the stable versions. >> >> That's your call, but I'm not supportive of that decision, specially as >> we have information from the person developing the workaround that this >> doesn't fully address the issue. > > The patches should not be applied at all. Simply because they don't fix the > issue completely. > > From a quick glance at various links and information about this, this very > much smells like the FSL_ERRATUM_A008585. > Has that been tried? It looks way more robust than the magic 11 bit > crystal ball logic. The Freescale erratum is similar, but not identical [1]. It seems like the A64 is less variable, so we can use a cheaper workaround, which gets away with normally just one sysreg read. But then again the newer error reports may actually suggest otherwise ... And as it currently stands, the Freescale erratum has the drawback of relying on the CPU running much faster than the timer. The A64 can run at 24 MHz (for power savings, or possibly during DVFS transitions), which is the timer frequency. So subsequent counter reads will never return the same value and the workaround times out. Cheers, Andre. [1] https://lists.denx.de/pipermail/u-boot/2016-November/271836.html
On Wed, 4 Jul 2018, Andre Przywara wrote: > On 04/07/18 11:00, Thomas Gleixner wrote: > > On Wed, 4 Jul 2018, Marc Zyngier wrote: > >> On 04/07/18 09:23, Daniel Lezcano wrote: > >>> > >>> If the patches fix a bug which already exist, it makes sense to > >>> propagated the fix back to the stable versions. > >> > >> That's your call, but I'm not supportive of that decision, specially as > >> we have information from the person developing the workaround that this > >> doesn't fully address the issue. > > > > The patches should not be applied at all. Simply because they don't fix the > > issue completely. > > > > From a quick glance at various links and information about this, this very > > much smells like the FSL_ERRATUM_A008585. > > Has that been tried? It looks way more robust than the magic 11 bit > > crystal ball logic. > > The Freescale erratum is similar, but not identical [1]. > It seems like the A64 is less variable, so we can use a cheaper > workaround, which gets away with normally just one sysreg read. But then > again the newer error reports may actually suggest otherwise ... > > And as it currently stands, the Freescale erratum has the drawback of > relying on the CPU running much faster than the timer. The A64 can run > at 24 MHz (for power savings, or possibly during DVFS transitions), > which is the timer frequency. So subsequent counter reads will never > return the same value and the workaround times out. If that's the case then you need to find a different functional timer for time keeping. Having an erratic behaving timer for time keeping is not an option at all. Thanks, tglx
Hi, On 04/07/18 15:31, Thomas Gleixner wrote: > On Wed, 4 Jul 2018, Andre Przywara wrote: >> On 04/07/18 11:00, Thomas Gleixner wrote: >>> On Wed, 4 Jul 2018, Marc Zyngier wrote: >>>> On 04/07/18 09:23, Daniel Lezcano wrote: >>>>> >>>>> If the patches fix a bug which already exist, it makes sense to >>>>> propagated the fix back to the stable versions. >>>> >>>> That's your call, but I'm not supportive of that decision, specially as >>>> we have information from the person developing the workaround that this >>>> doesn't fully address the issue. >>> >>> The patches should not be applied at all. Simply because they don't fix the >>> issue completely. >>> >>> From a quick glance at various links and information about this, this very >>> much smells like the FSL_ERRATUM_A008585. >>> Has that been tried? It looks way more robust than the magic 11 bit >>> crystal ball logic. >> >> The Freescale erratum is similar, but not identical [1]. >> It seems like the A64 is less variable, so we can use a cheaper >> workaround, which gets away with normally just one sysreg read. But then >> again the newer error reports may actually suggest otherwise ... >> >> And as it currently stands, the Freescale erratum has the drawback of >> relying on the CPU running much faster than the timer. The A64 can run >> at 24 MHz (for power savings, or possibly during DVFS transitions), >> which is the timer frequency. So subsequent counter reads will never >> return the same value and the workaround times out. > > If that's the case then you need to find a different functional timer for > time keeping. Having an erratic behaving timer for time keeping is not an > option at all. That's not an option on arm64. There are other usable time sources in the SoC, but the arch timer is somewhat mandatory for all practical purposes on arm64. We rely on it in some many places that it's not feasible to run without it. That's why we call it "architected" timer after all ;-) But I am quite confident that we can find a correct workaround. Maybe it's really the TVAL (the downcounter) write which is the culprit here, since the hardware actually writes "now() + TVAL" into the CVAL (upcounter) register. This internal counter access may be flawed as well. Cheers, Andre.
On Wed, 04 Jul 2018 15:44:36 +0100, Andre Przywara <andre.przywara@arm.com> wrote: > > Hi, > > On 04/07/18 15:31, Thomas Gleixner wrote: > > On Wed, 4 Jul 2018, Andre Przywara wrote: > >> On 04/07/18 11:00, Thomas Gleixner wrote: > >>> On Wed, 4 Jul 2018, Marc Zyngier wrote: > >>>> On 04/07/18 09:23, Daniel Lezcano wrote: > >>>>> > >>>>> If the patches fix a bug which already exist, it makes sense to > >>>>> propagated the fix back to the stable versions. > >>>> > >>>> That's your call, but I'm not supportive of that decision, specially as > >>>> we have information from the person developing the workaround that this > >>>> doesn't fully address the issue. > >>> > >>> The patches should not be applied at all. Simply because they don't fix the > >>> issue completely. > >>> > >>> From a quick glance at various links and information about this, this very > >>> much smells like the FSL_ERRATUM_A008585. > >>> Has that been tried? It looks way more robust than the magic 11 bit > >>> crystal ball logic. > >> > >> The Freescale erratum is similar, but not identical [1]. > >> It seems like the A64 is less variable, so we can use a cheaper > >> workaround, which gets away with normally just one sysreg read. But then > >> again the newer error reports may actually suggest otherwise ... > >> > >> And as it currently stands, the Freescale erratum has the drawback of > >> relying on the CPU running much faster than the timer. The A64 can run > >> at 24 MHz (for power savings, or possibly during DVFS transitions), > >> which is the timer frequency. So subsequent counter reads will never > >> return the same value and the workaround times out. > > > > If that's the case then you need to find a different functional timer for > > time keeping. Having an erratic behaving timer for time keeping is not an > > option at all. > > That's not an option on arm64. There are other usable time sources in > the SoC, but the arch timer is somewhat mandatory for all practical > purposes on arm64. We rely on it in some many places that it's not > feasible to run without it. That's why we call it "architected" timer > after all ;-) > But I am quite confident that we can find a correct workaround. Maybe > it's really the TVAL (the downcounter) write which is the culprit here, > since the hardware actually writes "now() + TVAL" into the CVAL > (upcounter) register. This internal counter access may be flawed as well. You got it backward: CVAL is not a counter at all. It is a Comparator. And TVAL has an implicit read from the counter, as it is defined as "CVAL - CNT" (i.e. the number of ticks until the timer expires). So it might be worth trying to handle TVAL entirely in SW. But this relies on being able to read the timer and get a number of correct values out of it. One possibility would be to sacrifice precision and always ignore some of the bottom bits, but this is always going to suck terribly. The alternative is burn that thing, and pretend it never existed. M.
On Wed, 4 Jul 2018, Andre Przywara wrote: > On 04/07/18 15:31, Thomas Gleixner wrote: > > If that's the case then you need to find a different functional timer for > > time keeping. Having an erratic behaving timer for time keeping is not an > > option at all. > > That's not an option on arm64. There are other usable time sources in > the SoC, but the arch timer is somewhat mandatory for all practical > purposes on arm64. We rely on it in some many places that it's not > feasible to run without it. That's why we call it "architected" timer > after all ;-) The argument that it has to be used just because someone defined it as 'architected' is bullshit and doesn't change the fact that it's broken and not usable for timekeeping. There is no wiggle room. Either it works or not, but works mostly is not an option. > But I am quite confident that we can find a correct workaround. Maybe > it's really the TVAL (the downcounter) write which is the culprit here, > since the hardware actually writes "now() + TVAL" into the CVAL > (upcounter) register. This internal counter access may be flawed as well. If the write to the event device is wreckaging the counter which provides time, then there is something seriously wrong either in the design or in that particular piece of silicon. Yet another proof for the theory that timers are implemented by janitors and that silicon/IP vendors have a competition running who can create the most subtly broken timers. Intel surely had a head start with that, but ARM is definitely catching up. Thanks, tglx
Hi, On 04/07/18 16:01, Marc Zyngier wrote: > On Wed, 04 Jul 2018 15:44:36 +0100, > Andre Przywara <andre.przywara@arm.com> wrote: >> >> Hi, >> >> On 04/07/18 15:31, Thomas Gleixner wrote: >>> On Wed, 4 Jul 2018, Andre Przywara wrote: >>>> On 04/07/18 11:00, Thomas Gleixner wrote: >>>>> On Wed, 4 Jul 2018, Marc Zyngier wrote: >>>>>> On 04/07/18 09:23, Daniel Lezcano wrote: >>>>>>> >>>>>>> If the patches fix a bug which already exist, it makes sense to >>>>>>> propagated the fix back to the stable versions. >>>>>> >>>>>> That's your call, but I'm not supportive of that decision, specially as >>>>>> we have information from the person developing the workaround that this >>>>>> doesn't fully address the issue. >>>>> >>>>> The patches should not be applied at all. Simply because they don't fix the >>>>> issue completely. >>>>> >>>>> From a quick glance at various links and information about this, this very >>>>> much smells like the FSL_ERRATUM_A008585. >>>>> Has that been tried? It looks way more robust than the magic 11 bit >>>>> crystal ball logic. >>>> >>>> The Freescale erratum is similar, but not identical [1]. >>>> It seems like the A64 is less variable, so we can use a cheaper >>>> workaround, which gets away with normally just one sysreg read. But then >>>> again the newer error reports may actually suggest otherwise ... >>>> >>>> And as it currently stands, the Freescale erratum has the drawback of >>>> relying on the CPU running much faster than the timer. The A64 can run >>>> at 24 MHz (for power savings, or possibly during DVFS transitions), >>>> which is the timer frequency. So subsequent counter reads will never >>>> return the same value and the workaround times out. >>> >>> If that's the case then you need to find a different functional timer for >>> time keeping. Having an erratic behaving timer for time keeping is not an >>> option at all. >> >> That's not an option on arm64. There are other usable time sources in >> the SoC, but the arch timer is somewhat mandatory for all practical >> purposes on arm64. We rely on it in some many places that it's not >> feasible to run without it. That's why we call it "architected" timer >> after all ;-) >> But I am quite confident that we can find a correct workaround. Maybe >> it's really the TVAL (the downcounter) write which is the culprit here, >> since the hardware actually writes "now() + TVAL" into the CVAL >> (upcounter) register. This internal counter access may be flawed as well. > > You got it backward: CVAL is not a counter at all. It is a > Comparator. And TVAL has an implicit read from the counter, as it is > defined as "CVAL - CNT" (i.e. the number of ticks until the timer > expires). Yes, that's what I meant actually, sorry for the lousy wording. What I am actually more concerned about than reading (do we actually read TVAL?), is writing TVAL. The original BSP errata hack hints at this being a problem: https://github.com/longsleep/linux-pine64/blob/5b10a45ae8b0/drivers/clocksource/arm_arch_timer.c#L231-L244 > So it might be worth trying to handle TVAL entirely in SW. > > But this relies on being able to read the timer and get a number of > correct values out of it. One possibility would be to sacrifice > precision and always ignore some of the bottom bits, but this is > always going to suck terribly. > > The alternative is burn that thing, and pretend it never existed. Yes, that crossed my mind multiple times. Cheers, Andre.
On 07/04/18 10:01, Marc Zyngier wrote: > On Wed, 04 Jul 2018 15:44:36, Andre Przywara <andre.przywara@arm.com> wrote: >> On 04/07/18 15:31, Thomas Gleixner wrote: >>> On Wed, 4 Jul 2018, Andre Przywara wrote: >>>> On 04/07/18 11:00, Thomas Gleixner wrote: >>>>> On Wed, 4 Jul 2018, Marc Zyngier wrote: >>>>>> On 04/07/18 09:23, Daniel Lezcano wrote: >>>>>>> >>>>>>> If the patches fix a bug which already exist, it makes sense to >>>>>>> propagated the fix back to the stable versions. >>>>>> >>>>>> That's your call, but I'm not supportive of that decision, >>>>>> specially as we have information from the person developing the >>>>>> workaround that this doesn't fully address the issue. >>>>> >>>>> The patches should not be applied at all. Simply because they don't >>>>> fix the issue completely. >>>>> >>>>> From a quick glance at various links and information about this, >>>>> this very much smells like the FSL_ERRATUM_A008585. Has that been >>>>> tried? It looks way more robust than the magic 11 bit crystal ball >>>>> logic. >>>> >>>> The Freescale erratum is similar, but not identical [1]. It seems like >>>> the A64 is less variable, so we can use a cheaper workaround, which >>>> gets away with normally just one sysreg read. But then again the newer >>>> error reports may actually suggest otherwise ... >>>> >>>> And as it currently stands, the Freescale erratum has the drawback of >>>> relying on the CPU running much faster than the timer. The A64 can run >>>> at 24 MHz (for power savings, or possibly during DVFS transitions), >>>> which is the timer frequency. So subsequent counter reads will never >>>> return the same value and the workaround times out. >>> >>> If that's the case then you need to find a different functional timer for >>> time keeping. Having an erratic behaving timer for time keeping is not an >>> option at all. >> >> That's not an option on arm64. There are other usable time sources in the >> SoC, but the arch timer is somewhat mandatory for all practical purposes >> on arm64. We rely on it in some many places that it's not feasible to run >> without it. That's why we call it "architected" timer after all ;-) But I >> am quite confident that we can find a correct workaround. Maybe it's >> really the TVAL (the downcounter) write which is the culprit here, since >> the hardware actually writes "now() + TVAL" into the CVAL (upcounter) >> register. This internal counter access may be flawed as well. > > You got it backward: CVAL is not a counter at all. It is a Comparator. And > TVAL has an implicit read from the counter, as it is defined as "CVAL - CNT" > (i.e. the number of ticks until the timer expires). > > So it might be worth trying to handle TVAL entirely in SW. > > But this relies on being able to read the timer and get a number of correct > values out of it. One possibility would be to sacrifice precision and always > ignore some of the bottom bits, but this is always going to suck terribly. > > The alternative is burn that thing, and pretend it never existed. >From the testing I have done, this patch series fully stabilizes reading CNTPCT and CNTVCT. So with the workaround, the timer *can* accurately count time. So merging this would be an improvement on the current situation. The system clock jumps might be explained by interaction with CVAL/TVAL, and that's the part I haven't investigated yet. As I mentioned before, and Andre just mentioned again, the BSP provided by the vendor has another workaround for writing the TVAL register. Hopefully, that's the missing piece which will fix the clock jumps. Thanks, Samuel
On 04/07/18 16:15, Andre Przywara wrote: > Hi, > > On 04/07/18 16:01, Marc Zyngier wrote: >> On Wed, 04 Jul 2018 15:44:36 +0100, >> Andre Przywara <andre.przywara@arm.com> wrote: >>> >>> Hi, >>> >>> On 04/07/18 15:31, Thomas Gleixner wrote: >>>> On Wed, 4 Jul 2018, Andre Przywara wrote: >>>>> On 04/07/18 11:00, Thomas Gleixner wrote: >>>>>> On Wed, 4 Jul 2018, Marc Zyngier wrote: >>>>>>> On 04/07/18 09:23, Daniel Lezcano wrote: >>>>>>>> >>>>>>>> If the patches fix a bug which already exist, it makes sense to >>>>>>>> propagated the fix back to the stable versions. >>>>>>> >>>>>>> That's your call, but I'm not supportive of that decision, specially as >>>>>>> we have information from the person developing the workaround that this >>>>>>> doesn't fully address the issue. >>>>>> >>>>>> The patches should not be applied at all. Simply because they don't fix the >>>>>> issue completely. >>>>>> >>>>>> From a quick glance at various links and information about this, this very >>>>>> much smells like the FSL_ERRATUM_A008585. >>>>>> Has that been tried? It looks way more robust than the magic 11 bit >>>>>> crystal ball logic. >>>>> >>>>> The Freescale erratum is similar, but not identical [1]. >>>>> It seems like the A64 is less variable, so we can use a cheaper >>>>> workaround, which gets away with normally just one sysreg read. But then >>>>> again the newer error reports may actually suggest otherwise ... >>>>> >>>>> And as it currently stands, the Freescale erratum has the drawback of >>>>> relying on the CPU running much faster than the timer. The A64 can run >>>>> at 24 MHz (for power savings, or possibly during DVFS transitions), >>>>> which is the timer frequency. So subsequent counter reads will never >>>>> return the same value and the workaround times out. >>>> >>>> If that's the case then you need to find a different functional timer for >>>> time keeping. Having an erratic behaving timer for time keeping is not an >>>> option at all. >>> >>> That's not an option on arm64. There are other usable time sources in >>> the SoC, but the arch timer is somewhat mandatory for all practical >>> purposes on arm64. We rely on it in some many places that it's not >>> feasible to run without it. That's why we call it "architected" timer >>> after all ;-) >>> But I am quite confident that we can find a correct workaround. Maybe >>> it's really the TVAL (the downcounter) write which is the culprit here, >>> since the hardware actually writes "now() + TVAL" into the CVAL >>> (upcounter) register. This internal counter access may be flawed as well. >> >> You got it backward: CVAL is not a counter at all. It is a >> Comparator. And TVAL has an implicit read from the counter, as it is >> defined as "CVAL - CNT" (i.e. the number of ticks until the timer >> expires). > > Yes, that's what I meant actually, sorry for the lousy wording. > > What I am actually more concerned about than reading (do we actually > read TVAL?), is writing TVAL. The original BSP errata hack hints at this > being a problem: > https://github.com/longsleep/linux-pine64/blob/5b10a45ae8b0/drivers/clocksource/arm_arch_timer.c#L231-L244 Right, and they only address the comparator, ignoring the counter. Braindead. I specially enjoy the "we should try to fix this" comment. >> So it might be worth trying to handle TVAL entirely in SW. Given the above, I think the above makes sense: - write TVAL: read CNT until stable, add the delta, write CVAL instead - read TVAL: read CNT until stable, substract CVAL, return the delta The low frequency problem remains. If it can't be solved, drop the arch timer from the DT (it is dead), and use a separate timer/counter. Simply not fit for purpose. M.
Hi, On 04/07/18 16:14, Thomas Gleixner wrote: > On Wed, 4 Jul 2018, Andre Przywara wrote: >> On 04/07/18 15:31, Thomas Gleixner wrote: >>> If that's the case then you need to find a different functional timer for >>> time keeping. Having an erratic behaving timer for time keeping is not an >>> option at all. >> >> That's not an option on arm64. There are other usable time sources in >> the SoC, but the arch timer is somewhat mandatory for all practical >> purposes on arm64. We rely on it in some many places that it's not >> feasible to run without it. That's why we call it "architected" timer >> after all ;-) > > The argument that it has to be used just because someone defined it as > 'architected' is bullshit and doesn't change the fact that it's broken and > not usable for timekeeping. There is no wiggle room. Either it works or > not, but works mostly is not an option. The "architected" part of the arch timer is fine, it's just that eventually someone has to implement that at some point. And as you mention below, this is where Murphy's law is kicking in ;-) Especially for such seemingly simple tasks as connecting a counter in the "uncore" part of the chip (Allwinner SoC) to the counter register interface in the core (ARM Cortex-A53) [1]. Apparently the propagation is not really atomic for all bits here ... >> But I am quite confident that we can find a correct workaround. Maybe >> it's really the TVAL (the downcounter) write which is the culprit here, >> since the hardware actually writes "now() + TVAL" into the CVAL >> (upcounter) register. This internal counter access may be flawed as well. > > If the write to the event device is wreckaging the counter which provides > time, then there is something seriously wrong either in the design or in > that particular piece of silicon. Apologies, that was my lousy wording: There is one 64-bit comparison register (CVAL), which signals when the counter (an independent register) is greater or equal. TVAL is just a different *view* of that same relation. So this part is fine, it's really that the "strictly monotonic counter" nature of CNTPCT is not really observed by the chip. > Yet another proof for the theory that timers are implemented by janitors > and that silicon/IP vendors have a competition running who can create the > most subtly broken timers. Intel surely had a head start with that, but ARM > is definitely catching up. ARM is trying really hard to be actually better ;-) Cheers, Andre. [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABIGHII.html
On Wed, 4 Jul 2018, Andre Przywara wrote: > On 04/07/18 16:14, Thomas Gleixner wrote: > > On Wed, 4 Jul 2018, Andre Przywara wrote: > >> On 04/07/18 15:31, Thomas Gleixner wrote: > >>> If that's the case then you need to find a different functional timer for > >>> time keeping. Having an erratic behaving timer for time keeping is not an > >>> option at all. > >> > >> That's not an option on arm64. There are other usable time sources in > >> the SoC, but the arch timer is somewhat mandatory for all practical > >> purposes on arm64. We rely on it in some many places that it's not > >> feasible to run without it. That's why we call it "architected" timer > >> after all ;-) > > > > The argument that it has to be used just because someone defined it as > > 'architected' is bullshit and doesn't change the fact that it's broken and > > not usable for timekeeping. There is no wiggle room. Either it works or > > not, but works mostly is not an option. > > The "architected" part of the arch timer is fine, it's just that > eventually someone has to implement that at some point. And as you > mention below, this is where Murphy's law is kicking in ;-) Especially > for such seemingly simple tasks as connecting a counter in the "uncore" > part of the chip (Allwinner SoC) to the counter register interface in > the core (ARM Cortex-A53) [1]. Apparently the propagation is not really > atomic for all bits here ... I've immediately spotted the fail in that document: The Cortex-A53 processor does not include the system counter. This resides in the SoC. > >> But I am quite confident that we can find a correct workaround. Maybe > >> it's really the TVAL (the downcounter) write which is the culprit here, > >> since the hardware actually writes "now() + TVAL" into the CVAL > >> (upcounter) register. This internal counter access may be flawed as well. > > > > If the write to the event device is wreckaging the counter which provides > > time, then there is something seriously wrong either in the design or in > > that particular piece of silicon. > > Apologies, that was my lousy wording: There is one 64-bit comparison > register (CVAL), which signals when the counter (an independent > register) is greater or equal. TVAL is just a different *view* of that > same relation. So this part is fine, it's really that the "strictly > monotonic counter" nature of CNTPCT is not really observed by the chip. > > > Yet another proof for the theory that timers are implemented by janitors > > and that silicon/IP vendors have a competition running who can create the > > most subtly broken timers. Intel surely had a head start with that, but ARM > > is definitely catching up. > > ARM is trying really hard to be actually better ;-) Better in terms of subtle brokenness? I surely can do consulting for that. I've seen most of it in all colours, but I surely can come up with new even subtler ways to wreckage them. You know how to reach me. Thanks, tglx
On 07/04/18 03:41, Daniel Lezcano wrote: > On 04/07/2018 10:16, Marc Zyngier wrote: >> On 03/07/18 19:42, Samuel Holland wrote: >>> On 07/03/18 10:09, Marc Zyngier wrote: >>>> On 11/05/18 03:27, Samuel Holland wrote: >>>>> Hello, >>>>> >>>>> Several people (including me) have experienced extremely large system >>>>> clock jumps on their A64-based devices, apparently due to the architectural >>>>> timer going backward, which is interpreted by Linux as the timer wrapping >>>>> around after 2^56 cycles. >>>>> >>>>> Investigation led to discovery of some obvious problems with this SoC's >>>>> architectural timer, and this patch series introduces what I believe is >>>>> the simplest workaround. More details are in the commit message for patch >>>>> 1. Patch 2 simply enables the workaround in the device tree. >>>> >>>> What's the deal with this series? There was a couple of nits to address, and >>>> I was more or less expecting a v2. >>> >>> I got reports that people were still occasionally having clock jumps after >>> applying this series, so I wanted to attempt a more complete fix, but I haven't >>> had time to do any deeper investigation. I think this series is still beneficial >>> even if it's not a complete solution, so I'll come back with another patch on >>> top of this if/once I get it fully fixed. >>> >>> I'll prepare a v2 with a bounded loop. Presumably, 3 * (max CPU Hz) / (24MHz >>> timer) ≈ 150 should be a conservative iteration limit? >> >> Should be OK. >> >> Maxime: How do you want to deal with the documentation aspect? We need >> an erratum number, but AFAIU the concept hasn't made it into the silicom >> vendor's brain yet. Any chance you could come up with something that >> uniquely identifies this? > > I went through the different pointers provided in the description but I > did not find a clear statement that is a hardware issue or may be I > missed it. > > Are we sure there isn't another subsystem responsible on this > instability ? (eg PM or something else) This issue has been observed on kernels with and without DVFS, across several Linux, U-Boot, and Trusted Firmware versions. It has not been observed on any other Allwinner SoC, including the A64's twin, the H5. In fact, this workaround was recently successfully used in U-Boot [1] to fix issues with an MMC driver that needed reliable numbers from CNTVCT. So while the vendor hasn't confirmed it (and I wouldn't count on that happening), everything I've seen points to it being a silicon bug, not a software issue. [1]: https://git.denx.de/?p=u-boot.git;a=commit;h=be0d217952222b2bd3ed071de9bb0c66d8cc80d9 >>> Also, does this make sense to CC to stable? >> >> Probably not, as the HW never worked, so it is not a regression. >> >> Thanks, >> >> M. >> > >