From: Andre Przywara <andre.przywara@arm.com> To: Samuel Holland <samuel@sholland.org> Cc: Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Rob Herring <robh@kernel.org>, Icenowy Zheng <icenowy@aosc.io>, Ondrej Jirman <megous@megous.com>, linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Alessandro Zummo <a.zummo@towertech.it>, Alexandre Belloni <alexandre.belloni@bootlin.com>, linux-rtc@vger.kernel.org, linux-clk@vger.kernel.org Subject: Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string Date: Tue, 15 Jun 2021 13:24:40 +0100 [thread overview] Message-ID: <20210615132440.55793ec5@slackpad.fritz.box> (raw) In-Reply-To: <56ad752b-b1c2-cb05-be8b-71c29f271ec9@sholland.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Andre Przywara <andre.przywara@arm.com> To: Samuel Holland <samuel@sholland.org> Cc: Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Rob Herring <robh@kernel.org>, Icenowy Zheng <icenowy@aosc.io>, Ondrej Jirman <megous@megous.com>, linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Alessandro Zummo <a.zummo@towertech.it>, Alexandre Belloni <alexandre.belloni@bootlin.com>, linux-rtc@vger.kernel.org, linux-clk@vger.kernel.org Subject: Re: [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string Date: Tue, 15 Jun 2021 13:24:40 +0100 [thread overview] Message-ID: <20210615132440.55793ec5@slackpad.fritz.box> (raw) In-Reply-To: <56ad752b-b1c2-cb05-be8b-71c29f271ec9@sholland.org> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-15 12:25 UTC|newest] Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-19 10:41 [PATCH v6 00/17] arm64: sunxi: Initial Allwinner H616 SoC support Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 01/17] dt-bindings: mfd: axp20x: Add AXP305 compatible (plus optional IRQ) Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-21 1:39 ` Rob Herring 2021-05-21 1:39 ` Rob Herring 2021-05-22 14:46 ` Samuel Holland 2021-05-22 14:46 ` Samuel Holland 2021-05-23 0:01 ` Andre Przywara 2021-05-23 0:01 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 02/17] mfd: axp20x: Allow AXP 806 chips without interrupt lines Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 15:01 ` Lee Jones 2021-05-19 15:01 ` Lee Jones 2021-05-19 10:41 ` [PATCH v6 03/17] dt-bindings: rtc: sun6i: Add H616 compatible string Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-21 1:39 ` Rob Herring 2021-05-21 1:39 ` Rob Herring 2021-05-21 2:37 ` Samuel Holland 2021-05-21 2:37 ` Samuel Holland 2021-06-07 12:59 ` Andre Przywara 2021-06-07 12:59 ` Andre Przywara 2021-06-08 4:23 ` Samuel Holland 2021-06-08 4:23 ` Samuel Holland 2021-06-15 12:24 ` Andre Przywara [this message] 2021-06-15 12:24 ` Andre Przywara 2021-06-16 9:07 ` Maxime Ripard 2021-06-16 9:07 ` Maxime Ripard 2021-06-16 11:28 ` Andre Przywara 2021-06-16 11:28 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 04/17] rtc: sun6i: Add support for linear day storage Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-22 7:26 ` Jernej Škrabec 2021-05-22 7:26 ` Jernej Škrabec 2021-05-19 10:41 ` [PATCH v6 05/17] rtc: sun6i: Add Allwinner H616 support Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-22 7:29 ` Jernej Škrabec 2021-05-22 7:29 ` Jernej Škrabec 2021-05-23 0:06 ` Andre Przywara 2021-05-23 0:06 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 06/17] dt-bindings: net: sun8i-emac: Add H616 compatible string Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-21 1:40 ` Rob Herring 2021-05-21 1:40 ` Rob Herring 2021-05-19 10:41 ` [PATCH v6 07/17] net: stmmac: dwmac-sun8i: Prepare for second EMAC clock register Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 08/17] dt-bindings: usb: Add H616 compatible string Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-21 1:40 ` Rob Herring 2021-05-21 1:40 ` Rob Herring 2021-05-21 1:40 ` Rob Herring 2021-05-19 10:41 ` [PATCH v6 09/17] dt-bindings: usb: sunxi-musb: " Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-21 1:40 ` Rob Herring 2021-05-21 1:40 ` Rob Herring 2021-05-19 10:41 ` [PATCH v6 10/17] phy: sun4i-usb: Rework HCI PHY (aka. "pmu_unk1") handling Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 11/17] phy: sun4i-usb: Allow reset line to be shared Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-24 11:59 ` Maxime Ripard 2021-05-24 11:59 ` Maxime Ripard 2021-05-24 11:59 ` Maxime Ripard 2021-05-24 12:51 ` Jernej Škrabec 2021-05-24 12:51 ` Jernej Škrabec 2021-05-24 12:51 ` Jernej Škrabec 2021-05-25 11:29 ` Andre Przywara 2021-05-25 11:29 ` Andre Przywara 2021-05-25 11:29 ` Andre Przywara 2021-06-07 13:22 ` Maxime Ripard 2021-06-07 13:22 ` Maxime Ripard 2021-06-07 13:22 ` Maxime Ripard 2021-06-07 14:17 ` Andre Przywara 2021-06-07 14:17 ` Andre Przywara 2021-06-07 14:17 ` Andre Przywara 2021-06-07 14:26 ` [linux-sunxi] " Chen-Yu Tsai 2021-06-07 14:26 ` Chen-Yu Tsai 2021-06-07 14:26 ` Chen-Yu Tsai 2021-06-07 14:26 ` Chen-Yu Tsai 2021-06-14 0:20 ` Andre Przywara 2021-06-14 0:20 ` Andre Przywara 2021-06-14 0:20 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 13/17] phy: sun4i-usb: Add support for the H616 USB PHY Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 14/17] arm64: dts: allwinner: Add Allwinner H616 .dtsi file Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-24 12:02 ` Maxime Ripard 2021-05-24 12:02 ` Maxime Ripard 2021-06-07 12:59 ` Andre Przywara 2021-06-07 12:59 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 15/17] dt-bindings: arm: sunxi: Add OrangePi Zero 2 binding Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 16/17] arm64: dts: allwinner: h616: Add OrangePi Zero 2 board support Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-19 10:41 ` [PATCH v6 17/17] arm64: dts: allwinner: h616: Add X96 Mate TV box support Andre Przywara 2021-05-19 10:41 ` Andre Przywara 2021-05-22 7:32 ` Jernej Škrabec 2021-05-22 7:32 ` Jernej Škrabec
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210615132440.55793ec5@slackpad.fritz.box \ --to=andre.przywara@arm.com \ --cc=a.zummo@towertech.it \ --cc=alexandre.belloni@bootlin.com \ --cc=devicetree@vger.kernel.org \ --cc=icenowy@aosc.io \ --cc=jernej.skrabec@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rtc@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=linux-sunxi@lists.linux.dev \ --cc=megous@megous.com \ --cc=mripard@kernel.org \ --cc=robh@kernel.org \ --cc=samuel@sholland.org \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.