From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:39:59
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
Samuel Holland (7):
dt-bindings: rtc: sun6i: Add H616 and R329 compatibles
clk: sunxi-ng: div: Add macro using CLK_HW_INIT_FW_NAME
clk: sunxi-ng: mux: Add macro using CLK_HW_INIT_PARENTS_DATA
clk: sunxi-ng: mux: Allow muxes to have keys
clk: sunxi-ng: Add support for the sun50i RTC clocks
[DO NOT MERGE] clk: sunxi-ng: Add support for H6
[DO NOT MERGE] clk: sunxi-ng: Add support for T5
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 ++-
drivers/clk/sunxi-ng/Kconfig | 6 +
drivers/clk/sunxi-ng/Makefile | 1 +
drivers/clk/sunxi-ng/ccu_common.h | 1 +
drivers/clk/sunxi-ng/ccu_div.h | 14 +
drivers/clk/sunxi-ng/ccu_mux.c | 7 +
drivers/clk/sunxi-ng/ccu_mux.h | 28 ++
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 433 ++++++++++++++++++
drivers/clk/sunxi-ng/sun50i-rtc-ccu.h | 15 +
drivers/rtc/rtc-sun6i.c | 17 -
include/dt-bindings/clock/sun50i-rtc.h | 12 +
11 files changed, 566 insertions(+), 23 deletions(-)
create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
--
2.31.1
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:06
To use the external clock references from the device tree, instead of
hardcoded global names, parents should be referenced with .fw_name. Add
a variant of the SUNXI_CCU_M_WITH_GATE initializer which does this.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/ccu_div.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:07
Some muxes need the flexibility to specify a combination of internal
parents (using .hw) and external parents (using .fw_name). Support
this with a version of the SUNXI_CCU_MUX_WITH_GATE macro that uses
CLK_HW_INIT_PARENTS_DATA to provide the parent information.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/ccu_mux.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:08
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
@@ -37,7 +39,24 @@ properties:-description:RTC Alarm 1clocks:-maxItems:1+minItems:1+maxItems:4++clock-names:+minItems:1+maxItems:4+items:+-anyOf:+-const:ahb+description:AHB parent for SPI bus clock+-const:bus+description:AHB/APB bus clock for register access+-const:ext-osc32k+description:External 32768 Hz oscillator input+-const:hosc+description:24 MHz oscillator input+-const:pll-32k+description:32 kHz clock divided from a PLLclock-output-names:minItems:1
@@ -96,13 +118,35 @@ allOf:properties:compatible:contains:-const:allwinner,sun50i-h6-rtc+enum:+-allwinner,sun50i-h616-rtc+-allwinner,sun50i-r329-rtcthen:+clocks:+minItems:3# bus, hosc, and (pll-32k [H616] or ahb [R329])++clock-names:+minItems:3++required:+-clock-names++else:+required:+-clock-output-names++-if:+properties:clock-names++then:+required:+-clocks# hosc is required++else:properties:-clock-output-names:-minItems:3-maxItems:3+clocks:+maxItems:1# only ext-osc32k is allowed-if:properties:
From: Rob Herring <robh@kernel.org> Date: 2021-09-01 12:06:25
On Wed, 01 Sep 2021 00:39:45 -0500, Samuel Holland wrote:
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Traceback (most recent call last):
File "/usr/local/bin/dt-doc-validate", line 67, in <module>
ret = check_doc(f)
File "/usr/local/bin/dt-doc-validate", line 33, in check_doc
for error in sorted(dtschema.DTValidator.iter_schema_errors(testtree), key=lambda e: e.linecol):
File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 723, in iter_schema_errors
cls(meta_schema).annotate_error(error, meta_schema, error.schema_path)
File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 706, in annotate_error
schema = schema[p]
KeyError: 'properties'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml: ignoring, error in schema: allOf: 5: if: properties
warning: no schema found in file: ./Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.example.dt.yaml:0:0: /example-0/rtc@1f00000: failed to match any schema with compatible: ['allwinner,sun6i-a31-rtc']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1522863
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
From: Rob Herring <robh@kernel.org> Date: 2021-09-02 15:27:59
On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
quoted hunk
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-03 15:36:53
On 9/2/21 10:27 AM, Rob Herring wrote:
On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
quoted
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
This says the first entry is any of these. What about the rest of them?
Oh, right. The list below is the list of all possible clocks.
quoted
+ - const: ahb
+ description: AHB parent for SPI bus clock
The description should go in 'clocks'.
Will do for v2.
The order should be defined as well with the first clock being the
one that existed previously.
The only way I know how to further refine the list is with
minItems/maxItems. My problem is that 1) some clocks are only valid for
certain SoCs, and 2) some clocks are optional, depending on how the
board is wired. So there is no single order where the "valid"
combinations are prefixes of the "possible" combinations of clocks.
Or in other words, how can I say "clocks #1 and #2 from this list are
required, and #4 is optional, but #3 is not allowed"?
Some concrete examples, with the always-required clocks moved to the
beginning:
H6:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: optional
- pll-32k: not allowed
H616:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: not allowed
- pll-32k: required
R329:
- bus: required
- hosc: required
- ahb: required
- ext-osc32k: optional
- pll-32k: not allowed
Should I just move the entire clocks/clock-items properties to if/then
blocks based on the compatible?
quoted
+ - const: bus
+ description: AHB/APB bus clock for register access
+ - const: ext-osc32k
+ description: External 32768 Hz oscillator input
+ - const: hosc
+ description: 24 MHz oscillator input
+ - const: pll-32k
+ description: 32 kHz clock divided from a PLL
clock-output-names:
minItems: 1
From: Rob Herring <robh@kernel.org> Date: 2021-09-07 14:45:00
On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland [off-list ref] wrote:
On 9/2/21 10:27 AM, Rob Herring wrote:
quoted
On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
quoted
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
This says the first entry is any of these. What about the rest of them?
Oh, right. The list below is the list of all possible clocks.
quoted
quoted
+ - const: ahb
+ description: AHB parent for SPI bus clock
The description should go in 'clocks'.
Will do for v2.
quoted
The order should be defined as well with the first clock being the
one that existed previously.
The only way I know how to further refine the list is with
minItems/maxItems. My problem is that 1) some clocks are only valid for
certain SoCs, and 2) some clocks are optional, depending on how the
board is wired. So there is no single order where the "valid"
combinations are prefixes of the "possible" combinations of clocks.
Or in other words, how can I say "clocks #1 and #2 from this list are
required, and #4 is optional, but #3 is not allowed"?
This says you have up to 4 clocks, but only defines the 1st 2:
maxItems: 4
items:
- description: 1st clock
- description: 2nd clock
But I think you will be better off with just defining the range
(minItems/maxItems) at the top level and then use if/then schemas.
Some concrete examples, with the always-required clocks moved to the
beginning:
H6:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: optional
- pll-32k: not allowed
Is this really 2 different 32k clock inputs to the h/w block? Doesn't
seem like it given both are never valid.
H616:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: not allowed
- pll-32k: required
R329:
- bus: required
- hosc: required
- ahb: required
- ext-osc32k: optional
- pll-32k: not allowed
Should I just move the entire clocks/clock-items properties to if/then
blocks based on the compatible?
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-08 02:26:28
On 9/7/21 9:44 AM, Rob Herring wrote:
On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland [off-list ref] wrote:
quoted
On 9/2/21 10:27 AM, Rob Herring wrote:
quoted
On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
quoted
For these new SoCs, start requiring a complete list of input clocks.
For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.
I'm not sure how to best represent this in the binding...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
.../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
include/dt-bindings/clock/sun50i-rtc.h | 12 ++++
2 files changed, 61 insertions(+), 6 deletions(-)
create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
This says the first entry is any of these. What about the rest of them?
Oh, right. The list below is the list of all possible clocks.
quoted
quoted
+ - const: ahb
+ description: AHB parent for SPI bus clock
The description should go in 'clocks'.
Will do for v2.
quoted
The order should be defined as well with the first clock being the
one that existed previously.
The only way I know how to further refine the list is with
minItems/maxItems. My problem is that 1) some clocks are only valid for
certain SoCs, and 2) some clocks are optional, depending on how the
board is wired. So there is no single order where the "valid"
combinations are prefixes of the "possible" combinations of clocks.
Or in other words, how can I say "clocks #1 and #2 from this list are
required, and #4 is optional, but #3 is not allowed"?
This says you have up to 4 clocks, but only defines the 1st 2:
maxItems: 4
items:
- description: 1st clock
- description: 2nd clock
But I think you will be better off with just defining the range
(minItems/maxItems) at the top level and then use if/then schemas.
Ah, thanks for the suggestions.
quoted
Some concrete examples, with the always-required clocks moved to the
beginning:
H6:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: optional
- pll-32k: not allowed
Is this really 2 different 32k clock inputs to the h/w block? Doesn't
seem like it given both are never valid.
Yes, there are two separate 32k inputs. Both are valid at the same time
on some SoCs like T5 (patch 7), but not on any of those I listed here.
Regards,
Samuel
quoted
H616:
- bus: required
- hosc: required
- ahb: not allowed
- ext-osc32k: not allowed
- pll-32k: required
R329:
- bus: required
- hosc: required
- ahb: required
- ext-osc32k: optional
- pll-32k: not allowed
Should I just move the entire clocks/clock-items properties to if/then
blocks based on the compatible?
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:11
The RTC power domain in sun50i SoCs manages the 16 MHz RC oscillator
(called "IOSC" or "osc16M") and the optional 32 kHz crystal oscillator
(called "LOSC" or "osc32k"). Starting with the H6, this power domain
handles the 24 MHz DCXO (called "HOSC", "dcxo24M", or "osc24M") as well.
The H6 also introduces a calibration circuit for IOSC.
Later SoCs introduce further variations on the design:
- H616 adds an additional mux for the 32 kHz fanout source.
- R329 adds an additional mux for the RTC timekeeping clock, a clock
for the SPI bus between power domains inside the RTC, and removes the
IOSC calibration functionality.
Take advantage of the CCU framework to handle this increased complexity.
The CCU framework provides pre-made widgets for the mux/gate/divider
combinations. And it allows plugging in different clocks for the same
specifiers based on the compatible string.
This driver is intended to be a drop-in replacement for the existing RTC
clock driver. So some runtime adjustment of the clock parents is needed,
both to handle hardware differences, and to support the old binding
which omitted some of the input clocks.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/Kconfig | 6 +
drivers/clk/sunxi-ng/Makefile | 1 +
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 344 ++++++++++++++++++++++++++
drivers/clk/sunxi-ng/sun50i-rtc-ccu.h | 15 ++
4 files changed, 366 insertions(+)
create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h
@@ -42,6 +42,12 @@ config SUN50I_H6_R_CCUdefaultARM64&&ARCH_SUNXIdepends on(ARM64&&ARCH_SUNXI)||COMPILE_TEST+configSUN50I_RTC_CCU+bool"Support for the Allwinner H6/H616/R329 RTC CCU"+defaultARM64&&ARCH_SUNXI+depends on(ARM64&&ARCH_SUNXI)||COMPILE_TEST+depends onSUNXI_CCU=y+configSUN4I_A10_CCUtristate"Support for the Allwinner A10/A20 CCU"defaultMACH_SUN4I
@@ -0,0 +1,344 @@+// SPDX-License-Identifier: GPL-2.0-only++#include<linux/clk-provider.h>+#include<linux/of_address.h>++#include"ccu_common.h"++#include"ccu_div.h"+#include"ccu_gate.h"+#include"ccu_mux.h"++#include"sun50i-rtc-ccu.h"++#define IOSC_ACCURACY 300000000 /* 30% */+#define IOSC_RATE 16000000++#define LOSC_RATE 32768+#define LOSC_RATE_SHIFT 15++#define LOSC_CTRL_KEY 0x16aa0000++#define IOSC_32K_CLK_DIV_REG 0x8+#define IOSC_32K_CLK_DIV GENMASK(4, 0)+#define IOSC_32K_PRE_DIV 32++#define IOSC_CLK_CALI_REG 0xc+#define IOSC_CLK_CALI_DIV_ONES 22+#define IOSC_CLK_CALI_EN BIT(1)+#define IOSC_CLK_CALI_SRC_SEL BIT(0)++#define DCXO_CTRL_REG 0x160+#define DCXO_CTRL_CLK16M_RC_EN BIT(0)++staticboolhave_iosc_calib;++staticintccu_iosc_enable(structclk_hw*hw)+{+structccu_common*cm=hw_to_ccu_common(hw);++returnccu_gate_helper_enable(cm,DCXO_CTRL_CLK16M_RC_EN);+}++staticvoidccu_iosc_disable(structclk_hw*hw)+{+structccu_common*cm=hw_to_ccu_common(hw);++returnccu_gate_helper_disable(cm,DCXO_CTRL_CLK16M_RC_EN);+}++staticintccu_iosc_is_enabled(structclk_hw*hw)+{+structccu_common*cm=hw_to_ccu_common(hw);++returnccu_gate_helper_is_enabled(cm,DCXO_CTRL_CLK16M_RC_EN);+}++staticunsignedlongccu_iosc_recalc_rate(structclk_hw*hw,+unsignedlongparent_rate)+{+structccu_common*cm=hw_to_ccu_common(hw);++if(have_iosc_calib){+u32reg=readl(cm->base+IOSC_CLK_CALI_REG);++/*+*RecovertheIOSCfrequencybyshiftingtheonesplaceof+*(fixed-pointdivider*32768)intobitzero.+*/+if(reg&IOSC_CLK_CALI_EN)+returnreg>>(IOSC_CLK_CALI_DIV_ONES-LOSC_RATE_SHIFT);+}++returnIOSC_RATE;+}++staticunsignedlongccu_iosc_recalc_accuracy(structclk_hw*hw,+unsignedlongparent_accuracy)+{+returnIOSC_ACCURACY;+}++staticconststructclk_opsccu_iosc_ops={+.enable=ccu_iosc_enable,+.disable=ccu_iosc_disable,+.is_enabled=ccu_iosc_is_enabled,+.recalc_rate=ccu_iosc_recalc_rate,+.recalc_accuracy=ccu_iosc_recalc_accuracy,+};++staticstructccu_commoniosc_clk={+.reg=DCXO_CTRL_REG,+.hw.init=CLK_HW_INIT_NO_PARENT("iosc",&ccu_iosc_ops,+CLK_GET_RATE_NOCACHE),+};++staticintccu_iosc_32k_enable(structclk_hw*hw)+{+structccu_common*cm=hw_to_ccu_common(hw);+unsignedlongflags;+u32reg;++if(!have_iosc_calib)+return0;++spin_lock_irqsave(cm->lock,flags);++reg=readl(cm->base+IOSC_CLK_CALI_REG);+writel(reg|IOSC_CLK_CALI_EN|IOSC_CLK_CALI_SRC_SEL,+cm->base+IOSC_CLK_CALI_REG);++spin_unlock_irqrestore(cm->lock,flags);++return0;+}++staticvoidccu_iosc_32k_disable(structclk_hw*hw)+{+structccu_common*cm=hw_to_ccu_common(hw);+unsignedlongflags;+u32reg;++if(!have_iosc_calib)+return;++spin_lock_irqsave(cm->lock,flags);++reg=readl(cm->base+IOSC_CLK_CALI_REG);+writel(reg&~(IOSC_CLK_CALI_EN|IOSC_CLK_CALI_SRC_SEL),+cm->base+IOSC_CLK_CALI_REG);++spin_unlock_irqrestore(cm->lock,flags);+}++staticunsignedlongccu_iosc_32k_recalc_rate(structclk_hw*hw,+unsignedlongparent_rate)+{+structccu_common*cm=hw_to_ccu_common(hw);+u32reg;++if(have_iosc_calib){+reg=readl(cm->base+IOSC_CLK_CALI_REG);++/* Assume the calibrated 32k clock is accurate. */+if(reg&IOSC_CLK_CALI_SRC_SEL)+returnLOSC_RATE;+}++reg=readl(cm->base+IOSC_32K_CLK_DIV_REG)&IOSC_32K_CLK_DIV;++returnparent_rate/IOSC_32K_PRE_DIV/(reg+1);+}++staticunsignedlongccu_iosc_32k_recalc_accuracy(structclk_hw*hw,+unsignedlongparent_accuracy)+{+structccu_common*cm=hw_to_ccu_common(hw);+u32reg;++if(have_iosc_calib){+reg=readl(cm->base+IOSC_CLK_CALI_REG);++/* Assume the calibrated 32k clock is accurate. */+if(reg&IOSC_CLK_CALI_SRC_SEL)+return0;+}++returnparent_accuracy;+}++staticconststructclk_opsccu_iosc_32k_ops={+.enable=ccu_iosc_32k_enable,+.disable=ccu_iosc_32k_disable,+.recalc_rate=ccu_iosc_32k_recalc_rate,+.recalc_accuracy=ccu_iosc_32k_recalc_accuracy,+};++staticstructccu_commoniosc_32k_clk={+.hw.init=CLK_HW_INIT_HW("iosc-32k",&iosc_clk.hw,+&ccu_iosc_32k_ops,0),+};++/* The old binding did not use clock-names, so fw_name may get cleared out. */+staticstructclk_parent_dataext_osc32k[]={+{.fw_name="ext-osc32k",.index=0}+};+staticSUNXI_CCU_GATE_DATA(ext_osc32k_gate_clk,"ext-osc32k-gate",+ext_osc32k,0x0,BIT(4),0);++staticconststructclk_hw*osc32k_parents[]={&iosc_32k_clk.hw,+&ext_osc32k_gate_clk.common.hw};+staticSUNXI_CCU_MUX_HW_WITH_KEY(osc32k_clk,"osc32k",osc32k_parents,+0x0,0,1,0);++/* Fall back to the global name for RTC nodes without an osc24M reference. */+staticstructclk_parent_dataosc24M[]={+{.fw_name="hosc",.name="osc24M"}+};+staticstructccu_gateosc24M_32k_clk={+.enable=BIT(16),+.common={+.reg=0x60,+.prediv=750,+.features=CCU_FEATURE_ALL_PREDIV,+.hw.init=CLK_HW_INIT_PARENTS_DATA("osc24M-32k",osc24M,+&ccu_gate_ops,0),+},+};++staticCLK_FIXED_FACTOR_HW(rtc_32k_fixed_clk,"rtc-32k",+&osc32k_clk.common.hw,1,1,0);++staticconststructclk_hw*rtc_32k_parents[]={&osc32k_clk.common.hw,+&osc24M_32k_clk.common.hw};+staticSUNXI_CCU_MUX_HW_WITH_KEY(rtc_32k_mux_clk,"rtc-32k",rtc_32k_parents,+0x0,1,1,0);++staticstructclk_parent_dataosc32k_fanout_parents[]={+{.hw=&osc32k_clk.common.hw},+/* Parent is modified depending on the hardware variant. */+{.fw_name="pll-32k"},+{.hw=&osc24M_32k_clk.common.hw},+};+staticSUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk,"rtc-32k-fanout",+osc32k_fanout_parents,+0x60,1,2,BIT(0),0);++staticSUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk,"rtc-spi","ahb",+0x310,0,5,BIT(31),0);++staticstructccu_common*sun50i_h616_rtc_ccu_clks[]={+&iosc_clk,+&iosc_32k_clk,+&osc32k_clk.common,+&osc24M_32k_clk.common,+&osc32k_fanout_clk.common,+};++staticstructccu_common*sun50i_r329_rtc_ccu_clks[]={+&iosc_clk,+&iosc_32k_clk,+&ext_osc32k_gate_clk.common,+&osc32k_clk.common,+&osc24M_32k_clk.common,+&rtc_32k_mux_clk.common,+&osc32k_fanout_clk.common,+&rtc_spi_clk.common,+};++staticstructclk_hw_onecell_datasun50i_h616_rtc_ccu_hw_clks={+.num=CLK_NUMBER,+.hws={+[CLK_OSC32K]=&osc32k_clk.common.hw,+[CLK_OSC32K_FANOUT]=&osc32k_fanout_clk.common.hw,+[CLK_IOSC]=&iosc_clk.hw,++[CLK_IOSC_32K]=&iosc_32k_clk.hw,+[CLK_EXT_OSC32K_GATE]=NULL,+[CLK_OSC24M_32K]=&osc24M_32k_clk.common.hw,+[CLK_RTC_32K]=&rtc_32k_fixed_clk.hw,+[CLK_RTC_SPI]=NULL,+},+};++staticstructclk_hw_onecell_datasun50i_r329_rtc_ccu_hw_clks={+.num=CLK_NUMBER,+.hws={+[CLK_OSC32K]=&osc32k_clk.common.hw,+[CLK_OSC32K_FANOUT]=&osc32k_fanout_clk.common.hw,+[CLK_IOSC]=&iosc_clk.hw,++[CLK_IOSC_32K]=&iosc_32k_clk.hw,+[CLK_EXT_OSC32K_GATE]=&ext_osc32k_gate_clk.common.hw,+[CLK_OSC24M_32K]=&osc24M_32k_clk.common.hw,+[CLK_RTC_32K]=&rtc_32k_mux_clk.common.hw,+[CLK_RTC_SPI]=&rtc_spi_clk.common.hw,+},+};++staticconststructsunxi_ccu_descsun50i_h616_rtc_ccu_desc={+.ccu_clks=sun50i_h616_rtc_ccu_clks,+.num_ccu_clks=ARRAY_SIZE(sun50i_h616_rtc_ccu_clks),++.hw_clks=&sun50i_h616_rtc_ccu_hw_clks,+};++staticconststructsunxi_ccu_descsun50i_r329_rtc_ccu_desc={+.ccu_clks=sun50i_r329_rtc_ccu_clks,+.num_ccu_clks=ARRAY_SIZE(sun50i_r329_rtc_ccu_clks),++.hw_clks=&sun50i_r329_rtc_ccu_hw_clks,+};++staticvoid__initsunxi_rtc_ccu_init(structdevice_node*node,+conststructsunxi_ccu_desc*desc)+{+void__iomem*reg;+inti;++reg=of_iomap(node,0);+if(IS_ERR(reg)){+pr_err("%pOF: Failed to map registers\n",node);+return;+}++/* ext-osc32k was the only input clock in the old binding. */+if(!of_property_read_bool(node,"clock-names"))+ext_osc32k[0].fw_name=NULL;++/* Rename the first 3 clocks to respect clock-output-names. */+for(i=0;i<3;++i){+structclk_init_data*init=(structclk_init_data*)+desc->hw_clks->hws[i]->init;++of_property_read_string_index(node,"clock-output-names",i,+&init->name);+}++of_sunxi_ccu_probe(node,reg,desc);+}++staticvoid__initsun50i_h616_rtc_ccu_setup(structdevice_node*node)+{+have_iosc_calib=1;++/* Remove the second parent as external osc32k is not supported. */+osc32k_parents[1]=&iosc_32k_clk.hw;++sunxi_rtc_ccu_init(node,&sun50i_h616_rtc_ccu_desc);+}+CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_ccu,"allwinner,sun50i-h616-rtc",+sun50i_h616_rtc_ccu_setup);++staticvoid__initsun50i_r329_rtc_ccu_setup(structdevice_node*node)+{+have_iosc_calib=0;++osc32k_fanout_parents[1]=(structclk_parent_data){+.hw=&ext_osc32k_gate_clk.common.hw+};++sunxi_rtc_ccu_init(node,&sun50i_r329_rtc_ccu_desc);+}+CLK_OF_DECLARE_DRIVER(sun50i_r329_rtc_ccu,"allwinner,sun50i-r329-rtc",+sun50i_r329_rtc_ccu_setup);
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:17
The T5 RTC is similar to the H616 RTC (no rtc-32k mux, pll-32k as the
second fanout input), except that it adds the ext-osc32k input.
It also isn't a "sun50i" SoC, so it creates a bit of a naming problem...
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 40 +++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:21
H6 has IOSC calibration and an ext-osc32k input.
H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
drivers/rtc/rtc-sun6i.c | 17 ----------
2 files changed, 49 insertions(+), 17 deletions(-)
@@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,of_sunxi_ccu_probe(node,reg,desc);}+staticvoid__initsun50i_h6_rtc_ccu_setup(structdevice_node*node)+{+structclk_init_data*init;++have_iosc_calib=1;++/* Casting away the const from a pointer to a non-const anonymous object... */+init=(structclk_init_data*)osc32k_fanout_clk.common.hw.init;++/* Fanout only has one parent: osc32k. */+init->num_parents=1;++sunxi_rtc_ccu_init(node,&sun50i_h6_rtc_ccu_desc);+}+CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_ccu,"allwinner,sun50i-h6-rtc",+sun50i_h6_rtc_ccu_setup);+staticvoid__initsun50i_h616_rtc_ccu_setup(structdevice_node*node){have_iosc_calib=1;
From: Maxime Ripard <hidden> Date: 2021-09-03 14:51:25
On Wed, Sep 01, 2021 at 12:39:50AM -0500, Samuel Holland wrote:
quoted hunk
H6 has IOSC calibration and an ext-osc32k input.
H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
drivers/rtc/rtc-sun6i.c | 17 ----------
2 files changed, 49 insertions(+), 17 deletions(-)
@@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,of_sunxi_ccu_probe(node,reg,desc);}+staticvoid__initsun50i_h6_rtc_ccu_setup(structdevice_node*node)+{+structclk_init_data*init;++have_iosc_calib=1;++/* Casting away the const from a pointer to a non-const anonymous object... */+init=(structclk_init_data*)osc32k_fanout_clk.common.hw.init;++/* Fanout only has one parent: osc32k. */+init->num_parents=1;++sunxi_rtc_ccu_init(node,&sun50i_h6_rtc_ccu_desc);+}
Indeed, that's not great.
Maybe we should just duplicate the sun50i_h6_rtc_ccu_desc (and
osc32k_fanout_clk) to cover both cases?
Maxime
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-03 15:07:22
On 9/3/21 9:51 AM, Maxime Ripard wrote:
On Wed, Sep 01, 2021 at 12:39:50AM -0500, Samuel Holland wrote:
quoted
H6 has IOSC calibration and an ext-osc32k input.
H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
drivers/rtc/rtc-sun6i.c | 17 ----------
2 files changed, 49 insertions(+), 17 deletions(-)
@@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,of_sunxi_ccu_probe(node,reg,desc);}+staticvoid__initsun50i_h6_rtc_ccu_setup(structdevice_node*node)+{+structclk_init_data*init;++have_iosc_calib=1;++/* Casting away the const from a pointer to a non-const anonymous object... */+init=(structclk_init_data*)osc32k_fanout_clk.common.hw.init;++/* Fanout only has one parent: osc32k. */+init->num_parents=1;++sunxi_rtc_ccu_init(node,&sun50i_h6_rtc_ccu_desc);+}
Indeed, that's not great.
Maybe we should just duplicate the sun50i_h6_rtc_ccu_desc (and
osc32k_fanout_clk) to cover both cases?
Right, I could split osc32k_fanout_clk like with rtc_32k_fixed_clk and
rtc_32k_mux_clk. Then H6 would use osc32k_fanout_fixed_clk.
Regards,
Samuel
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-01 05:40:23
The muxes in the RTC can only be updated when setting a key field to a
specific value. Add a feature flag to denote muxes with this property.
Since so far the key value is always the same, it does not need to be
provided separately for each mux.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/clk/sunxi-ng/ccu_common.h | 1 +
drivers/clk/sunxi-ng/ccu_mux.c | 7 +++++++
drivers/clk/sunxi-ng/ccu_mux.h | 14 ++++++++++++++
3 files changed, 22 insertions(+)
@@ -188,6 +190,11 @@ int ccu_mux_helper_set_parent(struct ccu_common *common,spin_lock_irqsave(common->lock,flags);reg=readl(common->base+common->reg);++/* The key field always reads as zero. */+if(common->features&CCU_FEATURE_KEY_FIELD)+reg|=CCU_MUX_KEY_VALUE;+reg&=~GENMASK(cm->width+cm->shift-1,cm->shift);writel(reg|(index<<cm->shift),common->base+common->reg);
From: Maxime Ripard <hidden> Date: 2021-09-03 14:50:22
Hi,
On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.
My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Maxime
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-03 15:21:19
On 9/3/21 9:50 AM, Maxime Ripard wrote:
Hi,
On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
quoted
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.
My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.
That's a good point. Originally, I had this RTC CCU providing osc24M, so
it did need to be an early provider. But with the current version, we
could have the RTC platform driver call devm_sunxi_ccu_probe. That does
seem cleaner.
(Since it wasn't immediately obvious to me why this works: the only
early provider remaining is the sun5i CCU, and it doesn't use the sun6i
RTC driver.)
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
Regards,
Samuel
From: Maxime Ripard <hidden> Date: 2021-09-09 08:45:46
On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
On 9/3/21 9:50 AM, Maxime Ripard wrote:
quoted
Hi,
On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
quoted
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.
My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.
That's a good point. Originally, I had this RTC CCU providing osc24M, so
it did need to be an early provider. But with the current version, we
could have the RTC platform driver call devm_sunxi_ccu_probe. That does
seem cleaner.
(Since it wasn't immediately obvious to me why this works: the only
early provider remaining is the sun5i CCU, and it doesn't use the sun6i
RTC driver.)
quoted
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Ah, right...
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.
I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).
And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.
Maxime
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-28 07:46:44
On 9/9/21 3:45 AM, Maxime Ripard wrote:
On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
quoted
On 9/3/21 9:50 AM, Maxime Ripard wrote:
quoted
Hi,
On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
quoted
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.
My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.
That's a good point. Originally, I had this RTC CCU providing osc24M, so
it did need to be an early provider. But with the current version, we
could have the RTC platform driver call devm_sunxi_ccu_probe. That does
seem cleaner.
(Since it wasn't immediately obvious to me why this works: the only
early provider remaining is the sun5i CCU, and it doesn't use the sun6i
RTC driver.)
quoted
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Ah, right...
quoted
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.
I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).
I don't think there can be two "struct device"s for a single OF node. So
if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe" function
would have to be called from the RTC driver. Since there has to be
cooperation anyway, I don't think there would be any ordering problems.
And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.
While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.
Regards,
Samuel
From: Maxime Ripard <hidden> Date: 2021-09-28 09:06:38
Hi,
On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
On 9/9/21 3:45 AM, Maxime Ripard wrote:
quoted
On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
quoted
On 9/3/21 9:50 AM, Maxime Ripard wrote:
quoted
Hi,
On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
quoted
This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.
The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.
A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.
It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.
This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.
I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.
My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.
That's a good point. Originally, I had this RTC CCU providing osc24M, so
it did need to be an early provider. But with the current version, we
could have the RTC platform driver call devm_sunxi_ccu_probe. That does
seem cleaner.
(Since it wasn't immediately obvious to me why this works: the only
early provider remaining is the sun5i CCU, and it doesn't use the sun6i
RTC driver.)
quoted
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Ah, right...
quoted
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.
I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).
I don't think there can be two "struct device"s for a single OF node.
That's not what I meant, there's indeed a single of_node for a single
struct device. If we dig a bit into the core framework, the most likely
scenario is that we would register both the RTC and clock driver at
module_init, and with the device already created with its of_node set
during the initial DT parsing.
We register our platform driver using module_platform_driver, which
expands to calling driver_register() at module_init(), setting the
driver bus to the platform_bus in the process (in
__platform_driver_register()).
After some sanity check, driver_register() calls bus_add_driver(), which
will call driver_attach() if drivers_autoprobe is set (which is the
default, set into bus_register()).
driver_attach() will, for each device on the platform bus, call
__driver_attach(). If there's a match between that device and our driver
(which is evaluated by platform_match() in our case), we'll call our
driver probe with that device through driver_probe_device(),
__driver_probe_device() and finally really_probe().
However, at no point in time there's any check about whether that device
has already been bound to a driver, nor does it create a new device for
each driver. So this means that, if you have two drivers that match the
same device (like our clock and RTC drivers), you'll have both probe
being called with the same device, and the probe order will be defined
by the link order. Worse, they would share the same driver_data, with
each driver not being aware of the other. This is incredibly fragile,
and hard to notice since it goes against the usual expectations.
So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
function would have to be called from the RTC driver.
No, it would be called by the core directly if there's a compatible to
match.
Since there has to be cooperation anyway, I don't think there would be
any ordering problems.
My initial point was that, with a direct function call, it's both
deterministic and obvious.
quoted
And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.
While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.
I know, but this was more of a social problem than a technical one. Each
contributor and reviewer in the future will have to know or remember
that it's there, and make sure that it's still the case after any change
they make or review.
This is again a fairly fragile assumption.
Maxime
From: Samuel Holland <samuel@sholland.org> Date: 2021-09-29 03:54:37
Hi Maxime,
Thanks for your reply.
On 9/28/21 4:06 AM, Maxime Ripard wrote:
On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
quoted
On 9/9/21 3:45 AM, Maxime Ripard wrote:
quoted
On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
quoted
On 9/3/21 9:50 AM, Maxime Ripard wrote:
quoted
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Ah, right...
quoted
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.
I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).
I don't think there can be two "struct device"s for a single OF node.
That's not what I meant, there's indeed a single of_node for a single
struct device. If we dig a bit into the core framework, the most likely
scenario is that we would register both the RTC and clock driver at
module_init, and with the device already created with its of_node set
during the initial DT parsing.
We register our platform driver using module_platform_driver, which
expands to calling driver_register() at module_init(), setting the
driver bus to the platform_bus in the process (in
__platform_driver_register()).
After some sanity check, driver_register() calls bus_add_driver(), which
will call driver_attach() if drivers_autoprobe is set (which is the
default, set into bus_register()).
driver_attach() will, for each device on the platform bus, call
__driver_attach(). If there's a match between that device and our driver
(which is evaluated by platform_match() in our case), we'll call our
driver probe with that device through driver_probe_device(),
__driver_probe_device() and finally really_probe().
However, at no point in time there's any check about whether that device
has already been bound to a driver, nor does it create a new device for
each driver.
I would expect this to hit the:
if (dev->driver)
return -EBUSY;
in __driver_probe_device(), or fail the "if (!dev->driver)" check in
__driver_attach() for the async case, once the first driver is bound.
So this means that, if you have two drivers that match the
same device (like our clock and RTC drivers), you'll have both probe
being called with the same device, and the probe order will be defined
by the link order. Worse, they would share the same driver_data, with
each driver not being aware of the other. This is incredibly fragile,
and hard to notice since it goes against the usual expectations.
quoted
So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
function would have to be called from the RTC driver.
No, it would be called by the core directly if there's a compatible to
match.
quoted
Since there has to be cooperation anyway, I don't think there would be
any ordering problems.
My initial point was that, with a direct function call, it's both
deterministic and obvious.
I believe I did what you are suggesting for v2. From patch 7:
@@ -683,6 +684,10 @@ static int sun6i_rtc_probe(struct platform_device
*pdev)
chip->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(chip->base))
return PTR_ERR(chip->base);
+
+ ret = sun6i_rtc_ccu_probe(&pdev->dev, chip->base);
+ if (ret)
+ return ret;
}
platform_set_drvdata(pdev, chip);
quoted
quoted
And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.
While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.
I know, but this was more of a social problem than a technical one. Each
contributor and reviewer in the future will have to know or remember
that it's there, and make sure that it's still the case after any change
they make or review.
This is again a fairly fragile assumption.
Yeah, I agree that having a lock that is only sometimes safe to use with
certain registers is quite fragile.
Would splitting the spinlock in rtc-sun6i.c into "losc_lock" (for the
clock provider) and "alarm_lock" (for the RTC driver) make this
distinction clear enough?
Eventually, I want to split up the struct between the clock provider and
RTC driver so it's clear which members belong to whom, and there's no
ugly global pointer use. Maybe I should do this first?
Regards,
Samuel
From: Maxime Ripard <hidden> Date: 2021-10-25 15:54:38
On Tue, Sep 28, 2021 at 10:54:26PM -0500, Samuel Holland wrote:
Hi Maxime,
Thanks for your reply.
On 9/28/21 4:06 AM, Maxime Ripard wrote:
quoted
On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
quoted
On 9/9/21 3:45 AM, Maxime Ripard wrote:
quoted
On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
quoted
On 9/3/21 9:50 AM, Maxime Ripard wrote:
quoted
And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h
Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.
Ah, right...
quoted
Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)
I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.
I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).
I don't think there can be two "struct device"s for a single OF node.
That's not what I meant, there's indeed a single of_node for a single
struct device. If we dig a bit into the core framework, the most likely
scenario is that we would register both the RTC and clock driver at
module_init, and with the device already created with its of_node set
during the initial DT parsing.
We register our platform driver using module_platform_driver, which
expands to calling driver_register() at module_init(), setting the
driver bus to the platform_bus in the process (in
__platform_driver_register()).
After some sanity check, driver_register() calls bus_add_driver(), which
will call driver_attach() if drivers_autoprobe is set (which is the
default, set into bus_register()).
driver_attach() will, for each device on the platform bus, call
__driver_attach(). If there's a match between that device and our driver
(which is evaluated by platform_match() in our case), we'll call our
driver probe with that device through driver_probe_device(),
__driver_probe_device() and finally really_probe().
However, at no point in time there's any check about whether that device
has already been bound to a driver, nor does it create a new device for
each driver.
I would expect this to hit the:
if (dev->driver)
return -EBUSY;
in __driver_probe_device(), or fail the "if (!dev->driver)" check in
__driver_attach() for the async case, once the first driver is bound.
Hmmm, it might. I know we "leveraged" this some time ago for another
platform, but it might not be working anymore indeed.
quoted hunk
quoted
So this means that, if you have two drivers that match the
same device (like our clock and RTC drivers), you'll have both probe
being called with the same device, and the probe order will be defined
by the link order. Worse, they would share the same driver_data, with
each driver not being aware of the other. This is incredibly fragile,
and hard to notice since it goes against the usual expectations.
quoted
So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
function would have to be called from the RTC driver.
No, it would be called by the core directly if there's a compatible to
match.
quoted
Since there has to be cooperation anyway, I don't think there would be
any ordering problems.
My initial point was that, with a direct function call, it's both
deterministic and obvious.
I believe I did what you are suggesting for v2. From patch 7:
@@ -683,6 +684,10 @@ static int sun6i_rtc_probe(struct platform_device
*pdev)
chip->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(chip->base))
return PTR_ERR(chip->base);
+
+ ret = sun6i_rtc_ccu_probe(&pdev->dev, chip->base);
+ if (ret)
+ return ret;
}
Ah, sorry, I entirely missed it. Yes, that totally fine by me then. I'd
prefer to have the spinlock passed as an argument as well, but it can be
done in a follow-up patch.
platform_set_drvdata(pdev, chip);
quoted
quoted
quoted
And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.
While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.
I know, but this was more of a social problem than a technical one. Each
contributor and reviewer in the future will have to know or remember
that it's there, and make sure that it's still the case after any change
they make or review.
This is again a fairly fragile assumption.
Yeah, I agree that having a lock that is only sometimes safe to use with
certain registers is quite fragile.
Would splitting the spinlock in rtc-sun6i.c into "losc_lock" (for the
clock provider) and "alarm_lock" (for the RTC driver) make this
distinction clear enough?
Eventually, I want to split up the struct between the clock provider and
RTC driver so it's clear which members belong to whom, and there's no
ugly global pointer use. Maybe I should do this first?