From: Conor Dooley <conor@kernel.org> To: Hal Feng <hal.feng@linux.starfivetech.com>, Stephen Boyd <sboyd@kernel.org> Cc: Emil Renner Berthing <emil.renner.berthing@canonical.com>, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <maz@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>, Michael Turquette <mturquette@baylibre.com>, Linus Walleij <linus.walleij@linaro.org>, Emil Renner Berthing <kernel@esmil.dk>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 15/30] clk: starfive: Use regmap APIs to operate registers Date: Sun, 23 Oct 2022 11:25:26 +0100 [thread overview] Message-ID: <4AF0F174-CB35-447A-9F22-7D300B225011@kernel.org> (raw) In-Reply-To: <07B628ED6CABEF1D+932737cc-7d4b-4071-531e-82f88d89a872@linux.starfivetech.com> On 23 October 2022 05:11:41 IST, Hal Feng <hal.feng@linux.starfivetech.com> wrote: >On Wed, 12 Oct 2022 16:05:23 -0700, Stephen Boyd wrote: >> Quoting Emil Renner Berthing (2022-10-05 06:14:44) >> > > > @@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev) >> > > > if (!priv) >> > > > return -ENOMEM; >> > > > >> > > > - spin_lock_init(&priv->rmw_lock); >> > > > priv->dev = &pdev->dev; >> > > > - priv->base = devm_platform_ioremap_resource(pdev, 0); >> > > > - if (IS_ERR(priv->base)) >> > > > - return PTR_ERR(priv->base); >> > > > + priv->regmap = device_node_to_regmap(priv->dev->of_node); >> > > >> > > This is sad. Why do we need to make a syscon? Can we instead use the >> > > auxiliary bus to make a reset device that either gets a regmap made here >> > > in this driver or uses a void __iomem * mapped with ioremap >> > > (priv->base)? >> > >> > In my original code the clock driver just registers the resets too >> > similar to other combined clock and reset drivers. I wonder what you >> > think about that approach: >> > https://github.com/esmil/linux/commit/36f15e1b827b02d7f493dc5fce31060b21976e68 >> > and >> > https://github.com/esmil/linux/commit/4ccafadb72968480aa3dd28c227fcccae411c13b#diff-ffec81f902f810cb210012c25e8d88217ea5b4021419a4206d1fd4dd19edfce8R471 >> >> I think we should use auxiliary bus and split the driver logically into >> a reset driver in drivers/reset and a clk driver in drivers/clk. That >> way the appropriate maintainers can review the code. There is only one >> platform device with a single reg property and node in DT, but there are >> two drivers. > >Yes, I agree that the reset driver and the clock driver should be split. >However, I think using auxiliary bus is a little bit complicated in this >case, because the reset is not a part of functionality of the clock in >JH7110. They just share a common register base address. I think it is >better to use ioremap for the same address, and the dt will be like > >syscrg_clk: clock-controller@13020000 { > compatible = "starfive,jh7110-clkgen-sys"; > reg = <0x0 0x13020000 0x0 0x10000>; > ... >}; >syscrg_rst: reset-controller@13020000 { > compatible = "starfive,jh7110-reset-sys"; > reg = <0x0 0x13020000 0x0 0x10000>; > ... >}; > >What do you think of this approach? I would appreciate your suggestions. No, the dtb checks will all start warning for this. Aux bus is not that difficult, you can likely copy much of what I did recently in clk-mpfs.c > >Best regards, >Hal
WARNING: multiple messages have this Message-ID (diff)
From: Conor Dooley <conor@kernel.org> To: Hal Feng <hal.feng@linux.starfivetech.com>, Stephen Boyd <sboyd@kernel.org> Cc: Emil Renner Berthing <emil.renner.berthing@canonical.com>, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <maz@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>, Michael Turquette <mturquette@baylibre.com>, Linus Walleij <linus.walleij@linaro.org>, Emil Renner Berthing <kernel@esmil.dk>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 15/30] clk: starfive: Use regmap APIs to operate registers Date: Sun, 23 Oct 2022 11:25:26 +0100 [thread overview] Message-ID: <4AF0F174-CB35-447A-9F22-7D300B225011@kernel.org> (raw) In-Reply-To: <07B628ED6CABEF1D+932737cc-7d4b-4071-531e-82f88d89a872@linux.starfivetech.com> On 23 October 2022 05:11:41 IST, Hal Feng <hal.feng@linux.starfivetech.com> wrote: >On Wed, 12 Oct 2022 16:05:23 -0700, Stephen Boyd wrote: >> Quoting Emil Renner Berthing (2022-10-05 06:14:44) >> > > > @@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev) >> > > > if (!priv) >> > > > return -ENOMEM; >> > > > >> > > > - spin_lock_init(&priv->rmw_lock); >> > > > priv->dev = &pdev->dev; >> > > > - priv->base = devm_platform_ioremap_resource(pdev, 0); >> > > > - if (IS_ERR(priv->base)) >> > > > - return PTR_ERR(priv->base); >> > > > + priv->regmap = device_node_to_regmap(priv->dev->of_node); >> > > >> > > This is sad. Why do we need to make a syscon? Can we instead use the >> > > auxiliary bus to make a reset device that either gets a regmap made here >> > > in this driver or uses a void __iomem * mapped with ioremap >> > > (priv->base)? >> > >> > In my original code the clock driver just registers the resets too >> > similar to other combined clock and reset drivers. I wonder what you >> > think about that approach: >> > https://github.com/esmil/linux/commit/36f15e1b827b02d7f493dc5fce31060b21976e68 >> > and >> > https://github.com/esmil/linux/commit/4ccafadb72968480aa3dd28c227fcccae411c13b#diff-ffec81f902f810cb210012c25e8d88217ea5b4021419a4206d1fd4dd19edfce8R471 >> >> I think we should use auxiliary bus and split the driver logically into >> a reset driver in drivers/reset and a clk driver in drivers/clk. That >> way the appropriate maintainers can review the code. There is only one >> platform device with a single reg property and node in DT, but there are >> two drivers. > >Yes, I agree that the reset driver and the clock driver should be split. >However, I think using auxiliary bus is a little bit complicated in this >case, because the reset is not a part of functionality of the clock in >JH7110. They just share a common register base address. I think it is >better to use ioremap for the same address, and the dt will be like > >syscrg_clk: clock-controller@13020000 { > compatible = "starfive,jh7110-clkgen-sys"; > reg = <0x0 0x13020000 0x0 0x10000>; > ... >}; >syscrg_rst: reset-controller@13020000 { > compatible = "starfive,jh7110-reset-sys"; > reg = <0x0 0x13020000 0x0 0x10000>; > ... >}; > >What do you think of this approach? I would appreciate your suggestions. No, the dtb checks will all start warning for this. Aux bus is not that difficult, you can likely copy much of what I did recently in clk-mpfs.c > >Best regards, >Hal _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2022-10-23 10:25 UTC|newest] Thread overview: 210+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-29 14:31 [PATCH v1 00/30] Basic StarFive JH7110 RISC-V SoC support Hal Feng 2022-09-29 14:31 ` Hal Feng 2022-09-29 14:31 ` [PATCH v1 01/30] dt-bindings: riscv: Add StarFive JH7110 bindings Hal Feng 2022-09-29 14:31 ` Hal Feng 2022-09-29 14:34 ` Krzysztof Kozlowski 2022-09-29 14:34 ` Krzysztof Kozlowski 2022-10-08 3:44 ` Hal Feng 2022-10-08 3:44 ` Hal Feng 2022-09-29 14:31 ` [PATCH v1 02/30] dt-bindings: timer: Add StarFive JH7110 clint Hal Feng 2022-09-29 14:31 ` Hal Feng 2022-09-29 14:34 ` Krzysztof Kozlowski 2022-09-29 14:34 ` Krzysztof Kozlowski 2022-09-29 14:31 ` [PATCH v1 03/30] dt-bindings: interrupt-controller: Add StarFive JH7110 plic Hal Feng 2022-09-29 14:31 ` Hal Feng 2022-09-29 14:35 ` Krzysztof Kozlowski 2022-09-29 14:35 ` Krzysztof Kozlowski 2022-09-29 14:31 ` [PATCH v1 04/30] dt-bindings: sifive-l2-cache: Support StarFive JH71x0 SoCs Hal Feng 2022-09-29 14:31 ` Hal Feng 2022-09-29 14:36 ` Krzysztof Kozlowski 2022-09-29 14:36 ` Krzysztof Kozlowski 2022-09-29 15:33 ` Conor Dooley 2022-09-29 15:33 ` Conor Dooley 2022-10-03 9:26 ` Ben Dooks 2022-10-03 9:26 ` Ben Dooks 2022-10-08 18:54 ` Hal Feng 2022-10-08 18:54 ` Hal Feng 2022-09-29 14:32 ` [PATCH v1 05/30] soc: sifive: l2 cache: Convert to platform driver Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-29 15:32 ` Conor Dooley 2022-09-29 15:32 ` Conor Dooley 2022-09-29 17:57 ` Ben Dooks 2022-09-29 17:57 ` Ben Dooks 2022-10-05 13:44 ` Emil Renner Berthing 2022-10-05 13:44 ` Emil Renner Berthing 2022-10-05 13:48 ` Ben Dooks 2022-10-05 13:48 ` Ben Dooks 2022-10-05 13:55 ` Emil Renner Berthing 2022-10-05 13:55 ` Emil Renner Berthing 2022-10-05 14:05 ` Conor Dooley 2022-10-05 14:05 ` Conor Dooley 2022-10-08 18:07 ` Hal Feng 2022-10-08 18:07 ` Hal Feng 2022-09-29 14:32 ` [PATCH v1 06/30] soc: sifive: l2 cache: Add StarFive JH71x0 support Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-29 14:32 ` [PATCH v1 07/30] reset: starfive: jh7100: Use 32bit I/O on 32bit registers Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-29 14:32 ` [PATCH v1 08/30] reset: starfive: jh7100: Use regmap APIs to operate registers Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-29 14:32 ` [PATCH v1 09/30] reset: starfive: jh7100: Move necessary properties to device tree Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-30 20:49 ` Rob Herring 2022-09-30 20:49 ` Rob Herring 2022-10-05 13:20 ` Emil Renner Berthing 2022-10-05 13:20 ` Emil Renner Berthing 2022-09-29 14:32 ` [PATCH v1 10/30] reset: starfive: Rename 'reset-starfive-jh7100.c' to 'reset-starfive.c' Hal Feng 2022-09-29 14:32 ` Hal Feng 2022-09-29 14:45 ` [PATCH v1 00/30] Basic StarFive JH7110 RISC-V SoC support Krzysztof Kozlowski 2022-09-29 14:45 ` Krzysztof Kozlowski 2022-09-29 17:59 ` Conor Dooley 2022-09-29 17:59 ` Conor Dooley 2022-10-01 1:13 ` hal.feng 2022-10-01 1:13 ` hal.feng 2022-09-29 16:35 ` [PATCH v1 11/30] dt-bindings: reset: Add StarFive JH7110 reset definitions Hal Feng 2022-09-29 16:35 ` Hal Feng 2022-09-29 17:51 ` [PATCH v1 12/30] dt-bindings: reset: Add starfive,jh7110-reset bindings Hal Feng 2022-09-29 17:51 ` Hal Feng 2022-09-29 18:21 ` Rob Herring 2022-09-29 18:21 ` Rob Herring 2022-09-29 18:40 ` Rob Herring 2022-09-29 18:40 ` Rob Herring 2022-09-29 18:43 ` Rob Herring 2022-09-29 18:43 ` Rob Herring 2022-10-11 15:30 ` Hal Feng 2022-10-11 15:30 ` Hal Feng 2022-10-11 16:36 ` Krzysztof Kozlowski 2022-10-11 16:36 ` Krzysztof Kozlowski 2022-10-12 13:16 ` Hal Feng 2022-10-12 13:16 ` Hal Feng 2022-10-12 13:33 ` Krzysztof Kozlowski 2022-10-12 13:33 ` Krzysztof Kozlowski 2022-10-12 14:05 ` Conor Dooley 2022-10-12 14:05 ` Conor Dooley 2022-10-12 15:21 ` Hal Feng 2022-10-12 15:21 ` Hal Feng 2022-10-12 14:53 ` Hal Feng 2022-10-12 14:53 ` Hal Feng 2022-10-12 8:01 ` Emil Renner Berthing 2022-10-12 8:01 ` Emil Renner Berthing 2022-09-29 17:53 ` [PATCH v1 13/30] reset: starfive: Add StarFive JH7110 SoC support Hal Feng 2022-09-29 17:53 ` Hal Feng 2022-09-29 17:54 ` [PATCH v1 14/30] clk: starfive: Factor out common clock driver code Hal Feng 2022-09-29 17:54 ` Hal Feng 2022-09-30 21:43 ` Stephen Boyd 2022-09-30 21:43 ` Stephen Boyd 2022-09-29 17:56 ` [PATCH v1 15/30] clk: starfive: Use regmap APIs to operate registers Hal Feng 2022-09-29 17:56 ` Hal Feng 2022-09-30 21:48 ` Stephen Boyd 2022-09-30 21:48 ` Stephen Boyd 2022-10-05 13:14 ` Emil Renner Berthing 2022-10-05 13:14 ` Emil Renner Berthing 2022-10-12 23:05 ` Stephen Boyd 2022-10-12 23:05 ` Stephen Boyd 2022-10-23 4:11 ` Hal Feng 2022-10-23 4:11 ` Hal Feng 2022-10-23 10:25 ` Conor Dooley [this message] 2022-10-23 10:25 ` Conor Dooley 2022-10-28 3:16 ` Hal Feng 2022-10-28 3:16 ` Hal Feng 2022-10-27 1:26 ` Stephen Boyd 2022-10-27 1:26 ` Stephen Boyd 2022-10-28 2:46 ` Hal Feng 2022-10-28 2:46 ` Hal Feng 2022-09-29 17:56 ` [PATCH v1 16/30] dt-bindings: clock: Add StarFive JH7110 system clock definitions Hal Feng 2022-09-29 17:56 ` Hal Feng 2022-09-29 22:26 ` [PATCH v1 17/30] dt-bindings: clock: Add starfive,jh7110-clkgen-sys bindings Hal Feng 2022-09-29 22:26 ` Hal Feng 2022-09-30 1:55 ` Rob Herring 2022-09-30 1:55 ` Rob Herring 2022-09-30 10:58 ` Krzysztof Kozlowski 2022-09-30 10:58 ` Krzysztof Kozlowski 2022-10-11 17:52 ` Hal Feng 2022-10-11 17:52 ` Hal Feng 2022-09-30 1:50 ` [PATCH v1 18/30] clk: starfive: Add StarFive JH7110 system clock driver Hal Feng 2022-09-30 1:50 ` Hal Feng 2022-09-30 5:49 ` [PATCH v1 19/30] dt-bindings: clock: Add StarFive JH7110 always-on definitions Hal Feng 2022-09-30 5:49 ` Hal Feng 2022-09-30 5:56 ` [PATCH v1 20/30] dt-bindings: clock: Add starfive,jh7110-clkgen-aon bindings Hal Feng 2022-09-30 5:56 ` Hal Feng 2022-09-30 10:59 ` Krzysztof Kozlowski 2022-09-30 10:59 ` Krzysztof Kozlowski 2022-10-11 18:01 ` Hal Feng 2022-10-11 18:01 ` Hal Feng 2022-09-30 12:51 ` Rob Herring 2022-09-30 12:51 ` Rob Herring 2022-09-30 6:03 ` [PATCH v1 21/30] clk: starfive: Add StarFive JH7110 always-on clock driver Hal Feng 2022-09-30 6:03 ` Hal Feng 2022-09-30 6:08 ` [PATCH v1 22/30] pinctrl: Create subdirectory for StarFive drivers Hal Feng 2022-09-30 6:08 ` Hal Feng 2022-10-04 8:43 ` Linus Walleij 2022-10-04 8:43 ` Linus Walleij 2022-09-30 6:14 ` [PATCH v1 23/30] pinctrl: starfive: Rename "pinctrl-starfive" to "pinctrl-starfive-jh7100" Hal Feng 2022-09-30 6:14 ` Hal Feng 2022-09-30 21:28 ` Rob Herring 2022-09-30 21:28 ` Rob Herring 2022-10-04 8:48 ` Linus Walleij 2022-10-04 8:48 ` Linus Walleij 2022-10-04 8:58 ` Conor Dooley 2022-10-04 8:58 ` Conor Dooley 2022-10-04 9:13 ` Linus Walleij 2022-10-04 9:13 ` Linus Walleij 2022-10-04 9:21 ` Conor Dooley 2022-10-04 9:21 ` Conor Dooley 2022-10-04 9:24 ` Conor Dooley 2022-10-04 9:24 ` Conor Dooley 2022-10-06 9:07 ` Geert Uytterhoeven 2022-10-06 9:07 ` Geert Uytterhoeven 2022-09-30 7:33 ` [PATCH v1 24/30] dt-bindings: pinctrl: Add StarFive JH7110 pinctrl definitions Hal Feng 2022-09-30 7:33 ` Hal Feng 2022-09-30 11:00 ` Krzysztof Kozlowski 2022-09-30 11:00 ` Krzysztof Kozlowski 2022-09-30 7:38 ` [PATCH v1 25/30] dt-bindings: pinctrl: Add StarFive JH7110 pinctrl bindings Hal Feng 2022-09-30 7:38 ` Hal Feng 2022-09-30 11:05 ` Krzysztof Kozlowski 2022-09-30 11:05 ` Krzysztof Kozlowski 2022-09-30 12:16 ` Rob Herring 2022-09-30 12:16 ` Rob Herring 2022-10-20 7:28 ` Icenowy Zheng 2022-10-20 7:28 ` Icenowy Zheng 2022-09-30 7:43 ` [PATCH v1 26/30] pinctrl: starfive: Add StarFive JH7110 driver Hal Feng 2022-09-30 7:43 ` Hal Feng 2022-10-01 14:35 ` kernel test robot 2022-10-01 14:35 ` kernel test robot 2022-10-04 8:56 ` Linus Walleij 2022-10-04 8:56 ` Linus Walleij 2022-10-05 13:31 ` Emil Renner Berthing 2022-10-05 13:31 ` Emil Renner Berthing 2022-10-14 2:05 ` Hal Feng 2022-10-14 2:05 ` Hal Feng 2022-09-30 7:49 ` [PATCH v1 27/30] RISC-V: Add initial StarFive JH7110 device tree Hal Feng 2022-09-30 7:49 ` Hal Feng 2022-10-01 10:52 ` Conor Dooley 2022-10-01 10:52 ` Conor Dooley 2022-10-03 7:45 ` Krzysztof Kozlowski 2022-10-03 7:45 ` Krzysztof Kozlowski 2022-10-14 9:41 ` Hal Feng 2022-10-14 9:41 ` Hal Feng 2022-09-30 7:53 ` [PATCH v1 28/30] RISC-V: Add StarFive JH7110 VisionFive2 board " Hal Feng 2022-09-30 7:53 ` Hal Feng 2022-10-01 11:14 ` Conor Dooley 2022-10-01 11:14 ` Conor Dooley 2022-10-29 8:18 ` Hal Feng 2022-10-29 8:18 ` Hal Feng 2022-09-30 9:06 ` [PATCH v1 29/30] RISC-V: defconfig: Enable CONFIG_SERIAL_8250_DW Hal Feng 2022-09-30 9:06 ` Hal Feng 2022-09-30 20:54 ` Ben Dooks 2022-09-30 20:54 ` Ben Dooks 2022-09-30 21:41 ` Conor Dooley 2022-09-30 21:41 ` Conor Dooley 2022-10-14 3:24 ` Hal Feng 2022-10-14 3:24 ` Hal Feng 2022-09-30 12:23 ` [PATCH v1 30/30] RISC-V: Add StarFive JH7100 and JH7110 SoC Kconfig options Hal Feng 2022-09-30 12:23 ` Hal Feng 2022-09-30 12:37 ` Conor Dooley 2022-09-30 12:37 ` Conor Dooley 2022-10-11 18:32 ` Hal Feng 2022-10-11 18:32 ` Hal Feng 2022-10-05 13:05 ` [PATCH v1 00/30] Basic StarFive JH7110 RISC-V SoC support Emil Renner Berthing 2022-10-05 13:05 ` Emil Renner Berthing 2022-10-08 3:18 ` Hal Feng 2022-10-08 3:18 ` Hal Feng
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=4AF0F174-CB35-447A-9F22-7D300B225011@kernel.org \ --to=conor@kernel.org \ --cc=aou@eecs.berkeley.edu \ --cc=daniel.lezcano@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=emil.renner.berthing@canonical.com \ --cc=hal.feng@linux.starfivetech.com \ --cc=kernel@esmil.dk \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=maz@kernel.org \ --cc=mturquette@baylibre.com \ --cc=p.zabel@pengutronix.de \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=robh+dt@kernel.org \ --cc=sboyd@kernel.org \ --cc=tglx@linutronix.de \ /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.