Message-ID: <20210901072123.gqfua52rupsrmtks@gilmour>
Date:   Wed, 1 Sep 2021 09:21:23 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Rob Herring <robh@...nel.org>, Icenowy Zheng <icenowy@...c.io>,
        Samuel Holland <samuel@...lland.org>,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...glegroups.com,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Ondrej Jirman <megous@...ous.com>, devicetree@...r.kernel.org,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        linux-rtc@...r.kernel.org
Subject: Re: [PATCH v8 02/11] dt-bindings: rtc: sun6i: Add H616 compatible
 string

On Wed, Aug 18, 2021 at 10:04:07AM +0100, Andre Przywara wrote:
> On Tue, 17 Aug 2021 09:38:10 +0200
> Maxime Ripard <maxime@...no.tech> wrote:
> 
> Hi Maxime,
> 
> > On Mon, Aug 02, 2021 at 01:39:38AM +0100, Andre Przywara wrote:
> > > On Mon, 26 Jul 2021 16:41:37 +0200
> > > Maxime Ripard <maxime@...no.tech> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > On Fri, Jul 23, 2021 at 04:38:29PM +0100, Andre Przywara wrote:  
> > > > > 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.
> > > > > Also the clock part is quite different, as there is no external 32K LOSC
> > > > > oscillator input.
> > > > > 
> > > > > Signed-off-by: Andre Przywara <andre.przywara@....com>
> > > > >
> > > > > ---
> > > > >  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml      | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> > > > > index beeb90e55727..d8a6500e5840 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
> > > > > @@ -104,6 +105,19 @@ allOf:
> > > > >            minItems: 3
> > > > >            maxItems: 3
> > > > >  
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            const: allwinner,sun50i-h616-rtc
> > > > > +
> > > > > +    then:
> > > > > +      properties:
> > > > > +        clock-output-names:
> > > > > +          minItems: 3
> > > > > +          maxItems: 3    
> > > > 
> > > > You don't need both of them when they are equal
> > > >   
> > > > > +        clocks: false
> > > > > +    
> > > > 
> > > > It's not entirely clear to me what those clocks are about though. If we
> > > > look at the clock output in the user manual, it looks like there's only
> > > > two clocks that are actually being output: the 32k "fanout" clock and
> > > > the losc. What are the 3 you're talking about?]  
> > > 
> > > I see three: the raw SYSTEM "CLK32K_LOSC", the RTC input + debounce
> > > clock (/32), and the multiplexed PAD.  
> > 
> > But the input and debounce clock is only for the RTC itself right? So it
> > should be local to the driver and doesn't need to be made available to
> > the other drivers
> 
> I understood "debounce" as being the clock used for the pinctrl
> debouncer. What would it debounce otherwise? Do you think that this
> "debounce circuit" is something internal to the RTC and is totally
> irrelevant for us?

I don't think that's it.

The Debounce circuit is after the 32 divider, so we have a clock rate of
1kHz (Figure 3-35, page 275)

The PIO Interrupt debouncing can use either a 32kHz or 24MHz clock, so
the rates don't match, and given the naming would rather be clocked from
CLK32K_LOSC.

The DCXO_CTRL_REG (Section 3.13.6.13) hints at something different
though, it says:

"
CLK16M_RC_EN
1: Enable
0: Disable
The related register configuration is necessary to ensure the reset debounce
circuit has a stable clock source.
The first time SoC starts up, by default, the reset debounce circuit of SoC
uses 32K divided by RC16M. In power-off, software reads the related bit to
ensure whether EXT32K is working normally, if it is normal, first switch the
clock source of debounce circuit to EXT32K, then close RC16M.
Without EXT32K scenario or external RTC scenario, software confirms firstly
whether EXT32K is working normally before switching, or software does not
close RC16M.
"

I'm not sure why it would be useful for though

> But in general I looked at how many *different* clocks this diagram
> describes, and I count: one unaltered ("SYSTEM"), one "div by
> 32" (RTC/debounce), and one multiplexed. My aim was to avoid
> DT binding changes when we later discover we do need one of them for
> something (as happened in the past). So three seemed to be the safe
> choice here, to avoid surprises. In the worst case we just will never
> reference one of them.

My concern is the pretty much the opposite: if we ever need to remove it
for whatever reason, if it's in the DT, we can't. While we can totally
add it if we need it.

> > Either way, what this list is must be documented.
> 
> You mean to overwrite the "description" stanza for clock-output-names?

Yes

> And can this be done in the per-SoC parts in the later part of the
> binding, keeping the existing description?

Sure

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists