| Message ID | 20230405195927.13487-1-ddrokosov@sberdevices.ru |
|---|---|
| Headers | show |
| Series | add Amlogic A1 clock controller drivers | expand |
On Wed, 05 Apr 2023 22:59:26 +0300, Dmitry Rokosov wrote: > Add the documentation for Amlogic A1 Peripherals clock driver, > and A1 Peripherals clock controller bindings. > A1 PLL clock controller has references to A1 Peripherals clock > controller objects, so reflect them in the schema. > > Signed-off-by: Jian Hu <jian.hu@amlogic.com> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > .../bindings/clock/amlogic,a1-clkc.yaml | 73 +++++++++++ > .../bindings/clock/amlogic,a1-pll-clkc.yaml | 5 +- > include/dt-bindings/clock/amlogic,a1-clkc.h | 114 ++++++++++++++++++ > 3 files changed, 190 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml > create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h > Reviewed-by: Rob Herring <robh@kernel.org>
Hello Dmitry, currently Jerome is busy so I am trying to continue where he left off. I have followed the previous iterations a bit but may have missed some details. So apologies if I'm repeating some questions that Jerome previously asked. On Wed, Apr 5, 2023 at 9:59 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote: [...] > +config COMMON_CLK_A1_PLL > + tristate "Meson A1 SoC PLL controller support" Should this be "Amlogic A1 SoC PLL controller support"? My understanding is that the "meson" name was dropped for this generation of SoCs. [...] > +static const struct of_device_id a1_pll_clkc_match_table[] = { > + { .compatible = "amlogic,a1-pll-clkc", }, > + {}, nit-pick: please drop the comma after {} This empty entry is a sentinel, no other entries are supposed to come after this - so a trailing comma is not necessary. [...] > +/* PLL register offset */ > +#define ANACTRL_FIXPLL_CTRL0 0x0 > +#define ANACTRL_FIXPLL_CTRL1 0x4 > +#define ANACTRL_FIXPLL_STS 0x14 > +#define ANACTRL_HIFIPLL_CTRL0 0xc0 > +#define ANACTRL_HIFIPLL_CTRL1 0xc4 > +#define ANACTRL_HIFIPLL_CTRL2 0xc8 > +#define ANACTRL_HIFIPLL_CTRL3 0xcc > +#define ANACTRL_HIFIPLL_CTRL4 0xd0 > +#define ANACTRL_HIFIPLL_STS 0xd4 Here I have a question that will potentially affect patch 3/6 ("dt-bindings: clock: meson: add A1 PLL clock controller bindings"). In the cover-letter you mentioned that quite a few clocks have been omitted. Any dt-bindings that we create need to be stable going forward. That means: the dt-bindings will always need to describe what the hardware is capable of, not what the driver implements. So my question is: do we have all needed inputs described in the dt-bindings (even though we're omitting quite a few registers here that will only be added/used in the future)? Older SoCs require (temporarily) using the XTAL clock for CPU clock tree changes. To make a long story short: I'm wondering if - at least - the XTAL clock input is missing. PS: I don't have an A1 datasheet nor a vendor kernel source (and even less a board for testing). So I can't verify any of this myself and I'm asking questions instead. Best regards, Martin
Hello Dmitry, (I'm aware you already posted a v14 - but I'm still replying here to continue the discussion on one question I had to keep the context) On Tue, Apr 25, 2023 at 2:33 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote: [...] > > > +/* PLL register offset */ > > > +#define ANACTRL_FIXPLL_CTRL0 0x0 > > > +#define ANACTRL_FIXPLL_CTRL1 0x4 > > > +#define ANACTRL_FIXPLL_STS 0x14 > > > +#define ANACTRL_HIFIPLL_CTRL0 0xc0 > > > +#define ANACTRL_HIFIPLL_CTRL1 0xc4 > > > +#define ANACTRL_HIFIPLL_CTRL2 0xc8 > > > +#define ANACTRL_HIFIPLL_CTRL3 0xcc > > > +#define ANACTRL_HIFIPLL_CTRL4 0xd0 > > > +#define ANACTRL_HIFIPLL_STS 0xd4 > > Here I have a question that will potentially affect patch 3/6 > > ("dt-bindings: clock: meson: add A1 PLL clock controller bindings"). > > In the cover-letter you mentioned that quite a few clocks have been omitted. > > Any dt-bindings that we create need to be stable going forward. That > > means: the dt-bindings will always need to describe what the hardware > > is capable of, not what the driver implements. > > So my question is: do we have all needed inputs described in the > > dt-bindings (even though we're omitting quite a few registers here > > that will only be added/used in the future)? > > Older SoCs require (temporarily) using the XTAL clock for CPU clock > > tree changes. To make a long story short: I'm wondering if - at least > > - the XTAL clock input is missing. > > The Amlogic A1 clock engine comprises four clock controllers for > peripherals, PLL, CPU, and audio. While the first two have been > introduced in the current patch series, the last two will be sent in the > next iteration. I (think that I) understand this part. > Presently, the PLL controller driver includes all the required bindings, > and the peripherals controller driver has all bindings except for the > CPU-related clock. Let's stick to the PLL controller bindings for the next part. My understanding is that the PLL clock controller registers (ANACTRL_*) are managing the following clocks: - fixed_pll - sys_pll - hifi_pll - whatever "AUDDDS" is - and some miscellaneous registers like ANACTRL_POR_CNTL and ANACTRL_MISCTOP_CTRL0 I *think* you got the dt-bindings correct: Even though the driver part does not support the hifi_pll yet, this IP block seems to have a "hifipll_in" clock input. Since the dt-bindings describes the hardware it may describe (for example) clock inputs that are not used by the driver yet. If you agree with my statement from above I'll be able to make my original question more specific: Since we know that we have all the required inputs for fixed_pll, sys_pll and hifi_pll - do you know what AUDDDS is and whether it requires any specific clock inputs (other than "fixpll_in" and "hifipll_in")? > However, I do not believe this to be a significant issue. The clock DT > bindings are organized to simplify the process of introducing new bindings, > whether public or private. For instance, we may add new bindings to > include/dt-bindings at the end of the list and increase the overall number, > without disrupting the DT bindings ABI (the old numbers will remain > unchanged). Yep, this part is clear to me. I should have been more specific that I was asking about the inputs that are described in the .yaml file, not the clock IDs. Best regards, Martin
Hello Martin, I apologize for the delayed response as I was on vacation without email access. I hope this is not a problem. On Mon, May 01, 2023 at 08:39:20PM +0200, Martin Blumenstingl wrote: > Hello Dmitry, > > (I'm aware you already posted a v14 - but I'm still replying here to > continue the discussion on one question I had to keep the context) > Sure, please find, my thoughts below. > On Tue, Apr 25, 2023 at 2:33 PM Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote: > [...] > > > > +/* PLL register offset */ > > > > +#define ANACTRL_FIXPLL_CTRL0 0x0 > > > > +#define ANACTRL_FIXPLL_CTRL1 0x4 > > > > +#define ANACTRL_FIXPLL_STS 0x14 > > > > +#define ANACTRL_HIFIPLL_CTRL0 0xc0 > > > > +#define ANACTRL_HIFIPLL_CTRL1 0xc4 > > > > +#define ANACTRL_HIFIPLL_CTRL2 0xc8 > > > > +#define ANACTRL_HIFIPLL_CTRL3 0xcc > > > > +#define ANACTRL_HIFIPLL_CTRL4 0xd0 > > > > +#define ANACTRL_HIFIPLL_STS 0xd4 > > > Here I have a question that will potentially affect patch 3/6 > > > ("dt-bindings: clock: meson: add A1 PLL clock controller bindings"). > > > In the cover-letter you mentioned that quite a few clocks have been omitted. > > > Any dt-bindings that we create need to be stable going forward. That > > > means: the dt-bindings will always need to describe what the hardware > > > is capable of, not what the driver implements. > > > So my question is: do we have all needed inputs described in the > > > dt-bindings (even though we're omitting quite a few registers here > > > that will only be added/used in the future)? > > > Older SoCs require (temporarily) using the XTAL clock for CPU clock > > > tree changes. To make a long story short: I'm wondering if - at least > > > - the XTAL clock input is missing. > > > > The Amlogic A1 clock engine comprises four clock controllers for > > peripherals, PLL, CPU, and audio. While the first two have been > > introduced in the current patch series, the last two will be sent in the > > next iteration. > I (think that I) understand this part. > > > Presently, the PLL controller driver includes all the required bindings, > > and the peripherals controller driver has all bindings except for the > > CPU-related clock. > Let's stick to the PLL controller bindings for the next part. > My understanding is that the PLL clock controller registers > (ANACTRL_*) are managing the following clocks: > - fixed_pll > - sys_pll > - hifi_pll > - whatever "AUDDDS" is > - and some miscellaneous registers like ANACTRL_POR_CNTL and > ANACTRL_MISCTOP_CTRL0 > > I *think* you got the dt-bindings correct: > Even though the driver part does not support the hifi_pll yet, this IP > block seems to have a "hifipll_in" clock input. > Since the dt-bindings describes the hardware it may describe (for > example) clock inputs that are not used by the driver yet. > > If you agree with my statement from above I'll be able to make my > original question more specific: > Since we know that we have all the required inputs for fixed_pll, > sys_pll and hifi_pll - do you know what AUDDDS is and whether it > requires any specific clock inputs (other than "fixpll_in" and > "hifipll_in")? > To be honest, I have prepared A1 peripherals and A1 PLL drivers based on very poor Amlogic datasheets and custom 4.19-based vendor drivers. The vendor driver has an AUDDDS clock in the PLL clock part, but it is not used anywhere. Unfortunately, as usual, the datasheet doesn't provide any information or explanation about what it is. However, the driver has a few lines of comments that indicate: /* * aud dds clock is not pll clock, not divider clock, * No clock model can describe it. * So we regard it as a gate, and the gate ops * should realize lonely. */ Additionally, the vendor driver states that AUDDDS has a 49Mhz clock, but I do not see any relationship with other clocks (including the exported GENCLK). Jian did not include it in the first version of the PLL driver, and I have decided not to change it either. I also noticed a few lines of AUDDDS initialization sequences in the vendor driver, which may affect CPU clock objects (from my point of view). However, they are currently under development, and I will try to figure it out with Amlogic support. > > However, I do not believe this to be a significant issue. The clock DT > > bindings are organized to simplify the process of introducing new bindings, > > whether public or private. For instance, we may add new bindings to > > include/dt-bindings at the end of the list and increase the overall number, > > without disrupting the DT bindings ABI (the old numbers will remain > > unchanged). > Yep, this part is clear to me. I should have been more specific that I > was asking about the inputs that are described in the .yaml file, not > the clock IDs. Actually, AUDDDS has an xtal2dds parent clock, and if we need to have the AUDDDS clock in the PLL driver, we should add one more link between peripherals and PLL drivers. Let me know if you have any questions.
A1 SoC has four clock controllers on the board: PLL, Peripherals, CPU, and Audio. The audio clock controller is different from others, but the rest are very similar from a functional and regmap point of view. This patch series add support for Amlogic A1 PLL and Peripherals clock drivers. It blocks all A1 peripherals mainline support and a couple of patch series, which were already reviewed and acked, but weren't merged due to pending clock controller drivers series, e.g. https://lore.kernel.org/linux-amlogic/7hd09cw9oh.fsf@baylibre.com/ TODO: CPU and Audio clock controllers are not included in this patch series, it will be sent later. The following clks from these controllers are not supported for now: * Audio clks - vad, mclk_vad, mclk_d, resample_a, locker_in, mclk_b, pdmdclk, pdmsysclk, eqdrc, spdifin, mclk_a, audio2_toaudiotop, audio2_tovad, audio2_toddr_vad, audio2_tdmin_vad, audio2_pdm, audio2_ddr_arb, audio_audiolocker, audio_eqdrc, audio_resamplea, audio_spdifin, audio_toddrb, audio_toddra, audio_frddrb, audio_frddra, audio_tdmoutb, audio_tdmouta, audio_loopbacka, audio_tdminlb, audio_tdminb, audio_tdmina, audio_ddr_arb, mclk_c * CPU clks: cpu_fixed_source_sel0, cpu_fixed_source_div0, cpu_fixed_source_sel1, cpu_fixed_source_div1, cpu_clk Validation: * to double check all clk flags run below helper script: pushd /sys/kernel/debug/clk for f in *; do if [[ -f "$f/clk_flags" ]]; then flags="$(cat $f/clk_flags | awk '{$1=$1};1' | sed ':a;N;$!ba;s/\n/ | /g')" echo -e "$f: $flags" fi done popd * to trace current clks state use '/sys/kernel/debug/clk/clk_dump' node with jq post-processing: $ cat /sys/kernel/debug/clk/clk_dump | jq '.' > clk_dump.json * to debug clk rate propagation, compile kernel with the following definition: $ sed -i "s/undef CLOCK_ALLOW_WRITE_DEBUGFS/define CLOCK_ALLOW_WRITE_DEBUGFS/g" drivers/clk/clk.c after that, clk_rate debug node for each clock will be available for write operation Changes v13 since v12 at [13]: - make the clock object registration order from the roots to the leaves - rearrange the clkids following above rule Changes v12 since v11 at [12]: - split the DT bindings patchset into two patches: one for the PLL clock controller driver, and one for the peripherals clock controller driver - to satisfy the DT binding checker, use fake references to the peripherals clock controller in the PLL DT bindings schema, and then replace them with real references when the peripherals bindings become available. - remove the public/private clocks concept from both controllers, and instead use a linear clkid list with both exposed and internal objects, all of which are registered in the clock provider - combine all comments about RTC children with the flag 'CLK_SET_RATE_NO_REPARENT' into a single item with multiple references to it Changes v11 since v10 at [11]: - change include/dt-bindings license to proper value required for bindings files: 'GPL-2.0-only OR BSD-2-Clause' - pll and peripherals clocks are split into public and private parts; public clocks are available for external consumers through the DT layer, private clocks include internal muxes and dividers of composite clocks, they are placed inside clock controller driver without external access - make public clks CLKID bindings continuous - mark the following clock muxes as NO_REPARENT and add them to public clocks list: GEN_SEL, DSPA_A_SEL, DSPA_B_SEL, DSPB_A_SEL, DSP_B_B_SEL, PWM_A_SEL, PWM_B_SEL, PWM_C_SEL, PWM_D_SEL, PWM_E_SEL, PWM_F_SEL, CECA_32K_SEL, CECB_32K_SEL; each of them can be inherited from more accurate RTC clock and sometimes it's required to forbid reparenting in such situation; also GEN_SEL can be connected to external PAD and should not change parent automatically due to rate propagation. For such clocks user must setup parents on the DT side Changes v10 since v9 at [10]: - split general clk-pll changes into two different patchsets: optional rst usage and new power sequence - squash dt bindings patchsets to avoid chicken-or-the-egg problem during run dt binding check routines - add vendor prefix to PLL and Peripherals clkcs bindings filenames - clear managed hifi_pll fields from initial poke table - move DSPA_SEL, DSPB_SEL and SARADC_SEL to private clkid table, because it should not be opened for direct usage - pwm_a clk used for voltage regulation is not critical anymore, it must be included to the proper cpu voltage regulation setup (will be available in the next patch series) - as discussed with Jerome, dspX clks are simple clocks and it should be enabled/disabled/ignored/anything else from appropriate DSP driver, so remove CLK_IGNORE_UNUSED tags - provide more understandable comments and remove irrelevant (I hope so) - remove CONFIG_OF usage, because it's redundant - fix license issue, it's GPL-2.0+ only in the current version - some commit msgs rewording Changes v9 since v8 at [9]: - remove common a1-clkc driver for the first version of a1 clock controllers as Jerome suggested (it will be discussed after s4 and a1 clks landed, hope so) - replace inherited a1-pll clk_pll_ops with common ops and introduce custom A1 PLL logic under MESON_PARM_APPLICABLE() conditions - rename xtal depended clocks in PLL and Peripherals domains - remove 'a1_' prefix for all clocks, because they are already inside A1 driver, it's redundant - change udelay() to usleep_range() as preferred for small msec amount - purge all double quotes from the yaml schemas - use proper dt node names following kernel guidelines https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation - use devm_platform_ioremap_resource() instead of simple devm_ioremap_resource() - mark all dspX clocks as CLK_IGNORE_UNUSED, because we do not want to touch these clocks during CCF initialization due to possible workload execution on it started from bootloader; in this case bootloader already made all initialization stuff for dspX - also mark all dspX with NO_REPARENT tag, because from dspX clocks we want to select proper clk source from device tree Changes v8 since v7 at [8]: - introduced a1-clkc common driver for all A1 clock controllers - exported meson_clk_pll_wait_lock symbol - supported meson-a1-clkc common driver in the a1-pll and a1 clkc - inherited a1-pll from the base clk-pll driver, implemented own version of init/enable/disable/enabled routines; rate calculating logic is fully the same - aligned CLKID-related definitions with CLKID list from order perspective to remove holes and permutations - corrected Kconfig dependencies and types - provided correct MODULE_AUTHORs() - optimized and fixed up some clock relationships - removed unused register offset definitions - fixed up A1 PLL and Peripherals clkc dtb_check errors - fixed clk_summary kernel panic due to missing a1_pad_ctrl clk_regmap definition - included PLL and Peripherals clk controllers to the base a1 dts - The previous v7 version [8] had several logic and style problems, all of them are resolved in this version. Original Jian Hu v7 patches are not touched, and all additional fixes are implemented in separate patches. Patch "clk: meson: add support for A1 PLL clock ops" is removed, because a1-pll clk driver inherits all stuff from clk-pll base driver, just implements custom init/enable/disable/is_enabled callbacks. Changes v7 since v6 at [7]: - fix 'dt_binding_check' compiling error - add acked-by Changes v6 since v5 at [6]: - fix yaml file - add rst/current_en/l_detect parm detection - remove 'meson_eeclkc_data' in a1.c and a1-pll.c Changes v5 since v4 at [5]: - change yaml GPL - drop meson-eeclk.c patch, add probe function in each driver - add CLK_IS_CRITICAL for sys_clk clock, drop the flag for sys_a and sys_b - add new parm for pll, add protection for rst parm - drop flag for a1_fixed_pll - remove the same comment for fclk_div, add "refer to" - add critical flag for a1_sys_clk - remove rtc table - rename a1_dspa_en_dspa and a1_dspb_en_dspb - remove useless comment Changes v4 since v3 at [3]: - fix reparenting orphan failed, it depends on jerome's patch [4] - fix changelist in v3 about reparenting orphan - remove the dts patch Changes v3 since v2 at [2]: - add probe function for A1 - separate the clock driver into two patch - change some clock flags and ops - add support for a1 PLL ops - add A1 clock node - fix reparenting orphan clock failed, registering xtal_fixpll and xtal_hifipll after the provider registration, it is not a best way. Changes v2 since v1 at [1]: - place A1 config alphabetically - add actual reason for RO ops, CLK_IS_CRITICAL, CLK_IGNORE_UNUSED - separate the driver into two driver: peripheral and pll driver - delete CLK_IGNORE_UNUSED flag for pwm b/c/d/e/f clock, dsp clock - delete the change in Kconfig.platforms, address to Kevin alone - remove the useless comments - modify the meson pll driver to support A1 PLLs Links: [1] https://lkml.kernel.org/r/1569411888-98116-1-git-send-email-jian.hu@amlogic.com [2] https://lkml.kernel.org/r/1571382865-41978-1-git-send-email-jian.hu@amlogic.com [3] https://lkml.kernel.org/r/20191129144605.182774-1-jian.hu@amlogic.com [4] https://lkml.kernel.org/r/20191203080805.104628-1-jbrunet@baylibre.com [5] https://lkml.kernel.org/r/20191206074052.15557-1-jian.hu@amlogic.com [6] https://lkml.kernel.org/r/20191227094606.143637-1-jian.hu@amlogic.com [7] https://lkml.kernel.org/r/20200116080440.118679-1-jian.hu@amlogic.com [8] https://lore.kernel.org/linux-amlogic/20200120034937.128600-1-jian.hu@amlogic.com/ [9] https://lore.kernel.org/linux-amlogic/20221201225703.6507-1-ddrokosov@sberdevices.ru/ [10] https://lore.kernel.org/all/20230301183759.16163-1-ddrokosov@sberdevices.ru/ [11] https://lore.kernel.org/all/20230313201259.19998-1-ddrokosov@sberdevices.ru/ [12] https://lore.kernel.org/all/20230321193014.26349-1-ddrokosov@sberdevices.ru/ [13] https://lore.kernel.org/all/20230404155332.9571-1-ddrokosov@sberdevices.ru/ Dmitry Rokosov (6): clk: meson: make pll rst bit as optional clk: meson: introduce new pll power-on sequence for A1 SoC family dt-bindings: clock: meson: add A1 PLL clock controller bindings clk: meson: a1: add Amlogic A1 PLL clock controller driver dt-bindings: clock: meson: add A1 Peripherals clock controller bindings clk: meson: a1: add Amlogic A1 Peripherals clock controller driver .../bindings/clock/amlogic,a1-clkc.yaml | 73 + .../bindings/clock/amlogic,a1-pll-clkc.yaml | 59 + MAINTAINERS | 1 + drivers/clk/meson/Kconfig | 20 + drivers/clk/meson/Makefile | 2 + drivers/clk/meson/a1-pll.c | 356 +++ drivers/clk/meson/a1-pll.h | 47 + drivers/clk/meson/a1.c | 2273 +++++++++++++++++ drivers/clk/meson/a1.h | 114 + drivers/clk/meson/clk-pll.c | 47 +- drivers/clk/meson/clk-pll.h | 2 + include/dt-bindings/clock/amlogic,a1-clkc.h | 114 + .../dt-bindings/clock/amlogic,a1-pll-clkc.h | 20 + 13 files changed, 3121 insertions(+), 7 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml create mode 100644 drivers/clk/meson/a1-pll.c create mode 100644 drivers/clk/meson/a1-pll.h create mode 100644 drivers/clk/meson/a1.c create mode 100644 drivers/clk/meson/a1.h create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h