Hi,

On Tue, Jun 15, 2021 at 01:24:40PM +0100, Andre Przywara wrote:

quoted

On 6/7/21 7:59 AM, Andre Przywara wrote:

quoted

On Thu, 20 May 2021 21:37:34 -0500
Samuel Holland [off-list ref] wrote:

Hi,
  

quoted

On 5/19/21 5:41 AM, Andre Przywara wrote:  

quoted

Add the obvious compatible name to the existing RTC binding.
The actual RTC part of the device uses a different day/month/year
storage scheme, so it's not compatible with the previous devices.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml     | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
index b1b0ee769b71..178c955f88bf 100644
--- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml

@@ -26,6 +26,7 @@ properties:
           - const: allwinner,sun50i-a64-rtc
           - const: allwinner,sun8i-h3-rtc
       - const: allwinner,sun50i-h6-rtc
+      - const: allwinner,sun50i-h616-rtc
 
   reg:
     maxItems: 1

@@ -97,7 +98,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: allwinner,sun50i-h6-rtc
+            enum:
+              - allwinner,sun50i-h6-rtc
+              - allwinner,sun50i-h616-rtc
 
     then:
       properties:
    

This binding is missing a clock reference for the pll-periph0-2x input
to the 32kHz clock fanout.  

Right. So do I get this correctly that we don't model the OSC24M input
explicitly so far in the DT? I only see one possible input clock, which
is for an optional 32K crystal oscillator.
And this means we need to change some code also? Because at the moment
a clock specified is assumed to be the 32K OSC, and having this clock
means we switch to the external 32K OSC.  

Right. The code would need updates to follow the binding.

I changed the binding for now to not allow any clock, and the code to
ignore any clocks when the H616 compatible is used. This way we can
extend this later without breaking anything.

I'm not really a fan of this: it just creates one more special case that
we'll have to take into account later on, complicating further the logic
that is already way too complicated.

quoted

quoted

And who would decide which clock source to use? What would be the
reason to use PLL_PERIPH(2x) over the RC16M based clock or the
divided down 24MHz?  

Because it would be more accurate. 24MHz/750 == 32000 Hz, while the RTC
expects 32768 Hz.

I thought about this as well, but this means there is no reason to not
use the PLL? At least not for Linux (normal operation with PLLs
running anyway)? This situation is different for the other SoCs, because
boards *might* have a separate and more precise 32K crystal.
So we could code this similar to the other SoCs: If we have a clock
property defined, we assume it's pointing to the PLL and switch to use
it?

We have another option though: list all the clocks that could be
available for a 32khz source, call clk_get_accuracy on them, and then
use the clock with the best accuracy. We already have the accuracy
requirements in the datasheet for each crystal, so it shouldn't be too
hard to support.

But, looking at the diagram in the manual (and assuming it's
correct), the PLL based clock can only be routed to the pad, but cannot
be used for the RTC. That seems to be also the case for the T5, which
has an external LOSC pin.
 

quoted

quoted

So shall we ignore the PLL based input clock for now, put "0 input
clocks" in the current binding, then later on extend this to allow
choosing the PLL? And have it that way that having the PLL reference
means we use it?  

No, the device tree represents the hardware, not whatever happens to be
used by Linux drivers at the time. It should be in the binding
regardless of what the driver does with it.

I understand that very well, but was just looking for a solution where
we can go ahead with an easier solution *now*. I am afraid implementing
this annoying RTC special snowflake properly will just delay the whole
series.
In the long run your "D1 & friends" extra RTC clock driver looks the
right way out, but it will probably take some more time to get this
merged.

To be honest, I'm not entirely sure why we need the rtc in the first
place. If your plan is to figure it out later anyway, why not just model
the 32kHz clock as a fixed clock, and change it later once it's been
entirely figured out?

quoted

Though the circular dependency between the clock providers does cause
problems. We cannot get a clk_hw for the PLL-based clock, so we would
have to hardcode a global name for it, which means we aren't really
using the device tree.

I start to wonder how much business Linux really has in controlling all
those RTC details. The current code happens to work, because everything
is setup correctly already, on reset.

That's not true for all the SoCs.

quoted

We already see this "not really using the binding" with the other CCUs:
the H616 CCU hardcodes the name "osc24M", while the A100 CCU hardcodes
"dcxo24M" for the same clock. So moving that clock into the RTC clock
provider would require using both names in one clk_hw simultaneously (or
rather fixing the CCU drivers to get a clk_hw from the DT instead of
referencing by name).

And trying to deal with optional clocks by index is only going to get
more painful over time. For example, with the R329 and D1, the RTC has
the following inputs:
 * DCXO24M (unless you model it inside the RTC)
 * External OSC32k (optional!)
 * The RTC bus gate/reset from the PRCM
 * R-AHB from the PRCM for the RTC SPI clock domain

So it seems time to start using clock-names in the RTC binding.

Yes, that sounds reasonable. It's just a shame that we keep changing
the RTC bindings, and so creating a lot of incompatibility on the way.

I mean, we keep changing it because the hardware keeps changing. The RTC
on the A20 had no clock at all. The A31 later on got only a single clock
input, and a single output. If your point is that we should have known
better 9 years ago what the current SoCs would look like, that's a bit
absurd, don't you think?

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel