[PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

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:
-- 
2.17.5

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Rob Herring]

On Wed, 19 May 2021 11:41:38 +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.
> 
> 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(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Samuel Holland]

Andre,

On 5/19/21 5:41 AM, 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.
> 
> 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.

It is also missing a clock reference to the RTC register gate (and that
clock is in turn missing from the r_ccu driver).

Regards,
Samuel

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

On Thu, 20 May 2021 21:37:34 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> On 5/19/21 5:41 AM, 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.
> > 
> > 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.
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?

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?

> It is also missing a clock reference to the RTC register gate (and that
> clock is in turn missing from the r_ccu driver).

Do you mean a gate bit somewhere in the PRCM? Do you have any
pointer/documentation for that?

Cheers,
Andre

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Samuel Holland]

On 6/7/21 7:59 AM, Andre Przywara wrote:
> On Thu, 20 May 2021 21:37:34 -0500
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi,
> 
>> On 5/19/21 5:41 AM, 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.
>>>
>>> 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.

> 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.

> 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.

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.

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.

>> It is also missing a clock reference to the RTC register gate (and that
>> clock is in turn missing from the r_ccu driver).
> 
> Do you mean a gate bit somewhere in the PRCM? Do you have any
> pointer/documentation for that?

Yes, it's bit 0 of PRCM+0x20c, documented in the BSP[1], used in
mainline[2], and verified by experiment.

[1]:
https://github.com/orangepi-xunlong/linux-orangepi/blob/orange-pi-4.9-sun50iw9/drivers/clk/sunxi/clk-sun50iw9.h#L169
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/sunxi-ng/ccu-sun50i-a100-r.c#n129

> Cheers,
> Andre

Regards,
Samuel

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

On Mon, 7 Jun 2021 23:23:04 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> On 6/7/21 7:59 AM, Andre Przywara wrote:
> > On Thu, 20 May 2021 21:37:34 -0500
> > Samuel Holland <samuel@sholland.org> wrote:
> > 
> > Hi,
> >   
> >> On 5/19/21 5:41 AM, 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.
> >>>
> >>> 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.

> > 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?

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.
 
> > 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.
 
> 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.

> 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.

> >> It is also missing a clock reference to the RTC register gate (and that
> >> clock is in turn missing from the r_ccu driver).  
> > 
> > Do you mean a gate bit somewhere in the PRCM? Do you have any
> > pointer/documentation for that?  
> 
> Yes, it's bit 0 of PRCM+0x20c, documented in the BSP[1], used in
> mainline[2], and verified by experiment.

I can confirm this, also by experimentation. And the H6 seems to have
the same bit.
But what purpose would this bit solve? I don't see a good reason to
describe this in the DT, it's more like a turn-off bit for firmware?

Cheers,
Andre

 
> [1]:
> https://github.com/orangepi-xunlong/linux-orangepi/blob/orange-pi-4.9-sun50iw9/drivers/clk/sunxi/clk-sun50iw9.h#L169
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/sunxi-ng/ccu-sun50i-a100-r.c#n129
> 
> > Cheers,
> > Andre  
> 
> Regards,
> Samuel

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Maxime Ripard]

Hi,

On Tue, Jun 15, 2021 at 01:24:40PM +0100, Andre Przywara wrote:
> > On 6/7/21 7:59 AM, Andre Przywara wrote:
> > > On Thu, 20 May 2021 21:37:34 -0500
> > > Samuel Holland <samuel@sholland.org> wrote:
> > > 
> > > Hi,
> > >   
> > >> On 5/19/21 5:41 AM, 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.
> > >>>
> > >>> 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.

> > > 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.
>  
> > > 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?

> > 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.

> > 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

Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

On Wed, 16 Jun 2021 11:07:00 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

Hi,

> On Tue, Jun 15, 2021 at 01:24:40PM +0100, Andre Przywara wrote:
> > > On 6/7/21 7:59 AM, Andre Przywara wrote:  
> > > > On Thu, 20 May 2021 21:37:34 -0500
> > > > Samuel Holland <samuel@sholland.org> wrote:
> > > > 
> > > > Hi,
> > > >     
> > > >> On 5/19/21 5:41 AM, 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.
> > > >>>
> > > >>> 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.

I see your point, but that's why I made it a no-clock choice: we can
add clocks any time later, older kernels finding them in newer DTs will
ignore them, older DTs on newer kernels wouldn't instantiate them in
the first place.

> 
> > > > 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.

That would possibly be an option, yes. What makes this further
complicated though is that there are several LOSC outputs: one going
to the RTC, one going to other peripherals, one going to the pad. And
they can have different sources, at least on the H616: the RTC and
system clock can't be driven by the PLL or divided down HOSC, just by
the RC oscillator. But all three of them can supply the clock to the
pad. I guess another reason to separate clock and actual RTC.

> > 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.
> >    
> > > > 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?

This would be a way out, at the cost of making newer DTs not work on
this kernel (the H616 RTC compatible wouldn't be recognised). I have to
check how fatal this is, IIRC pinctrl and CCU still work somehow (it's
only needed for debounce, which is optional?)
But if this is the price to pay to get it into 5.14 ... 
 
> > > 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.

Yes, this was not meant to be an universal statement, but as you
mention above, we get pretty far with ignoring the RTC completely, even.
 
> > > 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?

I don't mean changing the binding between SoCs, this is of course not
very reasonable, especially if dealing with Allwinner, who apparently
have little regard to something like "compatibility" and like to spread
new bits around various peripherals that happens to have free space.

I was referring to changing existing bindings for one particular SoC,
like we did in the past when adding the <&rtc 2> clock output.
And I am just afraid this will happen again if we start to support the
RTC "properly" now, for instance for the H6.

Cheers,
Andre

[PATCH v6 04/17] rtc: sun6i: Add support for linear day storage

[Andre Przywara]

Newer versions of the Allwinner RTC, as for instance found in the H616
SoC, no longer store a broken-down day/month/year representation in the
RTC_DAY_REG, but just a linear day number.
The user manual does not give any indication about the expected epoch
time of this day count, but the BSP kernel uses the UNIX epoch, which
allows easy support due to existing conversion functions in the kernel.

Allow tagging a compatible string with a flag, and use that to mark
those new RTCs. Then convert between a UNIX day number (converted into
seconds) and the broken-down day representation using mktime64() and
time64_to_tm() in the set_time/get_time functions.

That enables support for the RTC in those new chips.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 58 +++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index adec1b14a8de..0228e9dfd969 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -133,12 +133,15 @@ struct sun6i_rtc_clk_data {
 	unsigned int has_auto_swt : 1;
 };
 
+#define RTC_LINEAR_DAY	BIT(0)
+
 struct sun6i_rtc_dev {
 	struct rtc_device *rtc;
 	const struct sun6i_rtc_clk_data *data;
 	void __iomem *base;
 	int irq;
 	unsigned long alarm;
+	unsigned long flags;
 
 	struct clk_hw hw;
 	struct clk_hw *int_osc;
@@ -471,17 +474,30 @@ static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	rtc_tm->tm_min  = SUN6I_TIME_GET_MIN_VALUE(time);
 	rtc_tm->tm_hour = SUN6I_TIME_GET_HOUR_VALUE(time);
 
-	rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
-	rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date);
-	rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
-
-	rtc_tm->tm_mon  -= 1;
-
-	/*
-	 * switch from (data_year->min)-relative offset to
-	 * a (1900)-relative one
-	 */
-	rtc_tm->tm_year += SUN6I_YEAR_OFF;
+	if (chip->flags & RTC_LINEAR_DAY) {
+		struct tm tm;
+
+		/*
+		 * Newer chips store a linear day number, the manual
+		 * does not mandate any epoch base. The BSP driver uses
+		 * the UNIX epoch, let's just copy that, as it's the
+		 * easiest anyway.
+		 */
+		time64_to_tm((date & 0xffff) * 3600ULL * 24, 0, &tm);
+		rtc_tm->tm_mday = tm.tm_mday;
+		rtc_tm->tm_mon  = tm.tm_mon;
+		rtc_tm->tm_year = tm.tm_year;
+	} else {
+		rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
+		rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date) - 1;
+		rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
+
+		/*
+		 * switch from (data_year->min)-relative offset to
+		 * a (1900)-relative one
+		 */
+		rtc_tm->tm_year += SUN6I_YEAR_OFF;
+	}
 
 	return 0;
 }
@@ -571,15 +587,21 @@ static int sun6i_rtc_settime(struct device *dev, struct rtc_time *rtc_tm)
 	u32 date = 0;
 	u32 time = 0;
 
-	rtc_tm->tm_year -= SUN6I_YEAR_OFF;
 	rtc_tm->tm_mon += 1;
 
-	date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
-		SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
-		SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
+	if (chip->flags & RTC_LINEAR_DAY) {
+		date = mktime64(rtc_tm->tm_year + 1900, rtc_tm->tm_mon,
+				rtc_tm->tm_mday, 0, 0, 0) / (3600ULL * 24);
+	} else {
+		rtc_tm->tm_year -= SUN6I_YEAR_OFF;
+
+		date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
+			SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
+			SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
 
-	if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
-		date |= SUN6I_LEAP_SET_VALUE(1);
+		if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
+			date |= SUN6I_LEAP_SET_VALUE(1);
+	}
 
 	time = SUN6I_TIME_SET_SEC_VALUE(rtc_tm->tm_sec)  |
 		SUN6I_TIME_SET_MIN_VALUE(rtc_tm->tm_min)  |
@@ -678,6 +700,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, chip);
 
+	chip->flags = (unsigned long)of_device_get_match_data(&pdev->dev);
+
 	chip->irq = platform_get_irq(pdev, 0);
 	if (chip->irq < 0)
 		return chip->irq;
-- 
2.17.5

Re: [PATCH v6 04/17] rtc: sun6i: Add support for linear day storage

[Jernej Škrabec]

Hi Andre!

Dne sreda, 19. maj 2021 ob 12:41:39 CEST je Andre Przywara napisal(a):
> Newer versions of the Allwinner RTC, as for instance found in the H616
> SoC, no longer store a broken-down day/month/year representation in the
> RTC_DAY_REG, but just a linear day number.
> The user manual does not give any indication about the expected epoch
> time of this day count, but the BSP kernel uses the UNIX epoch, which
> allows easy support due to existing conversion functions in the kernel.
> 
> Allow tagging a compatible string with a flag, and use that to mark
> those new RTCs. Then convert between a UNIX day number (converted into
> seconds) and the broken-down day representation using mktime64() and
> time64_to_tm() in the set_time/get_time functions.
> 
> That enables support for the RTC in those new chips.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Change ^ to Signed-of-by. After that, you can add:
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

[PATCH v6 05/17] rtc: sun6i: Add Allwinner H616 support

[Andre Przywara]

The H616 RTC changes its day storage to the newly introduced linear day
scheme, so pair the new compatible string with this feature flag.
So far the clock parts seem to be the same as the H6, so combine the
compatible string with the existing H6 support bits.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 0228e9dfd969..ec0cd0ee539a 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -382,6 +382,8 @@ static void __init sun50i_h6_rtc_clk_init(struct device_node *node)
 }
 CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
 		      sun50i_h6_rtc_clk_init);
+CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_clk, "allwinner,sun50i-h616-rtc",
+		      sun50i_h6_rtc_clk_init);
 
 /*
  * The R40 user manual is self-conflicting on whether the prescaler is
@@ -773,6 +775,8 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun8i-v3-rtc" },
 	{ .compatible = "allwinner,sun50i-h5-rtc" },
 	{ .compatible = "allwinner,sun50i-h6-rtc" },
+	{ .compatible = "allwinner,sun50i-h616-rtc",
+		.data = (void *)RTC_LINEAR_DAY },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
-- 
2.17.5

Re: [PATCH v6 05/17] rtc: sun6i: Add Allwinner H616 support

[Jernej Škrabec]

Hi Andre!

Dne sreda, 19. maj 2021 ob 12:41:40 CEST je Andre Przywara napisal(a):
> The H616 RTC changes its day storage to the newly introduced linear day
> scheme, so pair the new compatible string with this feature flag.
> So far the clock parts seem to be the same as the H6, so combine the
> compatible string with the existing H6 support bits.

There is one more difference - H616 alarm value is now broken down to days, 
hours, minutes and seconds.

Best regards,
Jernej

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/rtc/rtc-sun6i.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 0228e9dfd969..ec0cd0ee539a 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -382,6 +382,8 @@ static void __init sun50i_h6_rtc_clk_init(struct 
device_node *node)
>  }
>  CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
>  		      sun50i_h6_rtc_clk_init);
> +CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_clk, "allwinner,sun50i-h616-rtc",
> +		      sun50i_h6_rtc_clk_init);
>  
>  /*
>   * The R40 user manual is self-conflicting on whether the prescaler is
> @@ -773,6 +775,8 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
>  	{ .compatible = "allwinner,sun8i-v3-rtc" },
>  	{ .compatible = "allwinner,sun50i-h5-rtc" },
>  	{ .compatible = "allwinner,sun50i-h6-rtc" },
> +	{ .compatible = "allwinner,sun50i-h616-rtc",
> +		.data = (void *)RTC_LINEAR_DAY },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> -- 
> 2.17.5
> 
> 

Re: [PATCH v6 05/17] rtc: sun6i: Add Allwinner H616 support

[Andre Przywara]

On Sat, 22 May 2021 09:29:26 +0200
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

Hi,

> Dne sreda, 19. maj 2021 ob 12:41:40 CEST je Andre Przywara napisal(a):
> > The H616 RTC changes its day storage to the newly introduced linear day
> > scheme, so pair the new compatible string with this feature flag.
> > So far the clock parts seem to be the same as the H6, so combine the
> > compatible string with the existing H6 support bits.  
> 
> There is one more difference - H616 alarm value is now broken down to days, 
> hours, minutes and seconds.

That's a good point, that actually requires adjusting the driver in
this respect as well. And contrary to what the manual says ("Counter
Register will down count to zero"), and the previous RTCs do, those alarm
registers now need to be set to the actual wakeup time, not the time
left before wakeup.
Will fix the driver accordingly.

Thanks for the heads up!

Cheers,
Andre

> 
> Best regards,
> Jernej
> 
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/rtc/rtc-sun6i.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index 0228e9dfd969..ec0cd0ee539a 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -382,6 +382,8 @@ static void __init sun50i_h6_rtc_clk_init(struct   
> device_node *node)
> >  }
> >  CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
> >  		      sun50i_h6_rtc_clk_init);
> > +CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_clk, "allwinner,sun50i-h616-rtc",
> > +		      sun50i_h6_rtc_clk_init);
> >  
> >  /*
> >   * The R40 user manual is self-conflicting on whether the prescaler is
> > @@ -773,6 +775,8 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
> >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> >  	{ .compatible = "allwinner,sun50i-h6-rtc" },
> > +	{ .compatible = "allwinner,sun50i-h616-rtc",
> > +		.data = (void *)RTC_LINEAR_DAY },
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> > -- 
> > 2.17.5
> > 
> >   
> 
>