| Message ID | 20231018070144.8512-1-zhangqing@rock-chips.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rockchip: add GATE_LINK | expand |
Hi, On Wed, Oct 18, 2023 at 03:01:40PM +0800, Elaine Zhang wrote: > Recent Rockchip SoCs have a new hardware block called Native Interface > Unit (NIU), which gates clocks to devices behind them. These effectively > need two parent clocks. > Use GATE_LINK to handle this. > > change in v4: > [PATCH v4 1/4]: No change > [PATCH v4 2/4]: No change > [PATCH v4 3/4]: dropping CLK_NR_CLKS,reword commit message > [PATCH v4 4/4]: No change > > change in V3: > [PATCH v3 1/4]: new, export clk_gate_endisable for PATCH2. > [PATCH v3 2/4]: reuse clk_gate_endisable and clk_gate_is_enabled. > add prepare and unprepare ops. > [PATCH v3 3/4]: No change > [PATCH v3 4/4]: reword commit message > > change in V2: > [PATCH v2 1/3]: fix reported warnings > [PATCH v2 2/3]: Bindings submit independent patches > [PATCH v2 3/3]: fix reported warnings > > Elaine Zhang (4): > clk: gate: export clk_gate_endisable > clk: rockchip: add support for gate link > dt-bindings: clock: rk3588: export PCLK_VO1GRF clk id > clk: rockchip: rk3588: Adjust the GATE_LINK parameter > > drivers/clk/clk-gate.c | 3 +- > drivers/clk/rockchip/Makefile | 1 + > drivers/clk/rockchip/clk-gate-link.c | 120 ++++++++++++++++++ > drivers/clk/rockchip/clk-rk3588.c | 110 ++++++++-------- > drivers/clk/rockchip/clk.c | 7 + > drivers/clk/rockchip/clk.h | 22 ++++ > .../dt-bindings/clock/rockchip,rk3588-cru.h | 2 +- > include/linux/clk-provider.h | 1 + > 8 files changed, 213 insertions(+), 53 deletions(-) > create mode 100644 drivers/clk/rockchip/clk-gate-link.c I get this with the patchset applied: Oct 20 18:25:04 rk3588-evb1 kernel: clk: Disabling unused clocks Oct 20 18:25:04 rk3588-evb1 kernel: ------------[ cut here ]------------ Oct 20 18:25:04 rk3588-evb1 kernel: hclk_rkvenc0 already disabled Oct 20 18:25:04 rk3588-evb1 kernel: WARNING: CPU: 4 PID: 1 at drivers/clk/clk.c:1090 clk_core_disable+0x1b0/0x1e0 Oct 20 18:25:04 rk3588-evb1 kernel: Modules linked in: Oct 20 18:25:04 rk3588-evb1 kernel: CPU: 4 PID: 1 Comm: swapper/0 Not tainted 6.6.0-rc6-00044-g31c3dbd16ca1 #1120 Oct 20 18:25:04 rk3588-evb1 kernel: Hardware name: Rockchip RK3588 EVB1 V10 Board (DT) Oct 20 18:25:04 rk3588-evb1 kernel: pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) Oct 20 18:25:04 rk3588-evb1 kernel: pc : clk_core_disable+0x1b0/0x1e0 Oct 20 18:25:04 rk3588-evb1 kernel: lr : clk_core_disable+0x1b0/0x1e0 Oct 20 18:25:04 rk3588-evb1 kernel: sp : ffff8000846fbbf0 Oct 20 18:25:04 rk3588-evb1 kernel: x29: ffff8000846fbbf0 x28: ffff8000833a0008 x27: ffff8000830e0130 Oct 20 18:25:04 rk3588-evb1 kernel: x26: ffff800082ffcff0 x25: ffff8000830b9be8 x24: ffff800083236100 Oct 20 18:25:04 rk3588-evb1 kernel: x23: 000000000000104a x22: 0000000000000000 x21: ffff800084669000 Oct 20 18:25:04 rk3588-evb1 kernel: x20: ffff00040087ca00 x19: ffff00040087ca00 x18: ffffffffffffffff Oct 20 18:25:04 rk3588-evb1 kernel: x17: ffff80008435d050 x16: ffff80008435cfe0 x15: 0720072007200720 Oct 20 18:25:04 rk3588-evb1 kernel: x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720 Oct 20 18:25:04 rk3588-evb1 kernel: x11: 0720072007200720 x10: ffff800084079890 x9 : ffff800080128a78 Oct 20 18:25:04 rk3588-evb1 kernel: x8 : 00000000ffffefff x7 : ffff800084079890 x6 : 80000000fffff000 Oct 20 18:25:04 rk3588-evb1 kernel: x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 Oct 20 18:25:04 rk3588-evb1 kernel: x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000400a08000 Oct 20 18:25:04 rk3588-evb1 kernel: Call trace: Oct 20 18:25:04 rk3588-evb1 kernel: clk_core_disable+0x1b0/0x1e0 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable+0x38/0x60 Oct 20 18:25:04 rk3588-evb1 kernel: clk_gate_link_disable+0x2c/0x48 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x104/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x34/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x34/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x34/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x34/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused_subtree+0x34/0x270 Oct 20 18:25:04 rk3588-evb1 kernel: clk_disable_unused+0x54/0x148 Oct 20 18:25:04 rk3588-evb1 kernel: do_one_initcall+0x48/0x2a8 Oct 20 18:25:04 rk3588-evb1 kernel: kernel_init_freeable+0x1ec/0x3d8 Oct 20 18:25:04 rk3588-evb1 kernel: kernel_init+0x2c/0x1f8 Oct 20 18:25:04 rk3588-evb1 kernel: ret_from_fork+0x10/0x20 Oct 20 18:25:04 rk3588-evb1 kernel: ---[ end trace 0000000000000000 ]--- Oct 20 18:25:04 rk3588-evb1 kernel: ------------[ cut here ]------------ ... (more clocks follow) I managed to fix this by introducing some flags, that clk_disable / clk_unprepare is only called on the linked clock if there was a prior clk_enable/clk_prepare for it. Apart from that this does not remove the existing GATE clocks for pclk_vo0grf and pclk_vo1grf resulting in the following errors: Oct 20 19:22:37 rk3588-evb1 kernel: pclk_vo0grf clk_register field Oct 20 19:22:37 rk3588-evb1 kernel: rockchip_clk_register_branches: failed to register clock pclk_vo0grf: -17 Oct 20 19:22:37 rk3588-evb1 kernel: pclk_vo1grf clk_register field Oct 20 19:22:37 rk3588-evb1 kernel: rockchip_clk_register_branches: failed to register clock pclk_vo1grf: -17 Last but not least I think it's better to restructure the patches a bit: Patch 1: Add CLK ID for PCLK_VO1GRF in the DT binding (add Fixes tag for 4f5ca304f202) Patch 2: Fix clock"pclk_vo0grf" and "pclk_vo1grf" (add Fixes tag for f1c506d152ff) Patch 3: Adjust GATE_LINK parameter from string to constant, but keep local GATE_LINK() define Patch 4: Export clk_gate_endisable Patch 5: Add gate-link support and remove local GATE_LINK() define from rk3588 Patch 6: Remove RK3588_LINKED_CLK Greetings, -- Sebastian
Quoting Elaine Zhang (2023-10-18 00:01:40) > Recent Rockchip SoCs have a new hardware block called Native Interface > Unit (NIU), which gates clocks to devices behind them. These effectively > need two parent clocks. > Use GATE_LINK to handle this. Why can't pm clks be used here? The qcom clk driver has been doing that for some time now. $ git grep pm_clk_add -- drivers/clk/qcom/
Hello Stephen, On Mon, Oct 23, 2023 at 06:47:17PM -0700, Stephen Boyd wrote: > Quoting Elaine Zhang (2023-10-18 00:01:40) > > Recent Rockchip SoCs have a new hardware block called Native Interface > > Unit (NIU), which gates clocks to devices behind them. These effectively > > need two parent clocks. > > Use GATE_LINK to handle this. > > Why can't pm clks be used here? The qcom clk driver has been doing that > for some time now. > > $ git grep pm_clk_add -- drivers/clk/qcom/ Maybe I'm mistaken, but as far as I can tell this is adding the dependency on controller level and only works because Qualcomm has multiple separate clock controllers. In the Rockchip design there is only one platform device. Note, that the original downstream code from Rockchip actually used pm_clk infrastructure by moving these clocks to separate platform devices. I changed this when upstreaming the code, since that leaks into DT and from DT point of view there should be only one clock controller. Greetings, -- Sebastian
Quoting Sebastian Reichel (2023-10-25 12:48:49) > Hello Stephen, > > On Mon, Oct 23, 2023 at 06:47:17PM -0700, Stephen Boyd wrote: > > Quoting Elaine Zhang (2023-10-18 00:01:40) > > > Recent Rockchip SoCs have a new hardware block called Native Interface > > > Unit (NIU), which gates clocks to devices behind them. These effectively > > > need two parent clocks. > > > Use GATE_LINK to handle this. > > > > Why can't pm clks be used here? The qcom clk driver has been doing that > > for some time now. > > > > $ git grep pm_clk_add -- drivers/clk/qcom/ > > Maybe I'm mistaken, but as far as I can tell this is adding the > dependency on controller level and only works because Qualcomm > has multiple separate clock controllers. In the Rockchip design > there is only one platform device. > > Note, that the original downstream code from Rockchip actually used > pm_clk infrastructure by moving these clocks to separate platform > devices. I changed this when upstreaming the code, since that leaks > into DT and from DT point of view there should be only one clock > controller. > Why can't the rockchip driver bind to a single device node and make sub-devices for each clk domain and register clks for those? Maybe it can use the auxiliary driver infrastructure to do that?
在 2023/10/26 5:40, Stephen Boyd 写道: > Quoting Sebastian Reichel (2023-10-25 12:48:49) >> Hello Stephen, >> >> On Mon, Oct 23, 2023 at 06:47:17PM -0700, Stephen Boyd wrote: >>> Quoting Elaine Zhang (2023-10-18 00:01:40) >>>> Recent Rockchip SoCs have a new hardware block called Native Interface >>>> Unit (NIU), which gates clocks to devices behind them. These effectively >>>> need two parent clocks. >>>> Use GATE_LINK to handle this. >>> Why can't pm clks be used here? The qcom clk driver has been doing that >>> for some time now. >>> >>> $ git grep pm_clk_add -- drivers/clk/qcom/ >> Maybe I'm mistaken, but as far as I can tell this is adding the >> dependency on controller level and only works because Qualcomm >> has multiple separate clock controllers. In the Rockchip design >> there is only one platform device. >> >> Note, that the original downstream code from Rockchip actually used >> pm_clk infrastructure by moving these clocks to separate platform >> devices. I changed this when upstreaming the code, since that leaks >> into DT and from DT point of view there should be only one clock >> controller. >> > Why can't the rockchip driver bind to a single device node and make > sub-devices for each clk domain and register clks for those? Maybe it > can use the auxiliary driver infrastructure to do that? Option 1: Use the current patch to adapt the GATE_LINK type upstream. The real function of GATE_LINK is implemented。 Just to improve and adapt the existing features on upstream. Option 2: What we use on our internal branches are: drivers/clk/rockchip/clk-link.c static int rockchip_clk_link_probe(struct platform_device *pdev) { struct rockchip_link_clk *priv; struct device_node *node = pdev->dev.of_node; const struct of_device_id *match; const char *clk_name; const struct rockchip_link_info *link_info; int ret; match = of_match_node(rockchip_clk_link_of_match, node); if (!match) return -ENXIO; priv = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_link_clk), GFP_KERNEL); if (!priv) return -ENOMEM; priv->link = match->data; spin_lock_init(&priv->lock); platform_set_drvdata(pdev, priv); priv->base = of_iomap(node, 0); if (IS_ERR(priv->base)) return PTR_ERR(priv->base); if (of_property_read_string(node, "clock-output-names", &clk_name)) priv->name = node->name; else priv->name = clk_name; link_info = rockchip_get_link_infos(priv->link, priv->name); priv->shift = link_info->shift; priv->pname = link_info->pname; pm_runtime_enable(&pdev->dev); ret = pm_clk_create(&pdev->dev); if (ret) goto disable_pm_runtime; ret = pm_clk_add(&pdev->dev, "link"); if (ret) goto destroy_pm_clk; ret = register_clocks(priv, &pdev->dev); if (ret) goto destroy_pm_clk; return 0; destroy_pm_clk: pm_clk_destroy(&pdev->dev); disable_pm_runtime: pm_runtime_disable(&pdev->dev); return ret; } Both of these methods are OK. Whichever one Upstream prefers, I can submit it as required.
Quoting zhangqing (2023-10-25 19:25:43) > > 在 2023/10/26 5:40, Stephen Boyd 写道: > > Quoting Sebastian Reichel (2023-10-25 12:48:49) > >> Hello Stephen, > >> > >> On Mon, Oct 23, 2023 at 06:47:17PM -0700, Stephen Boyd wrote: > >>> Quoting Elaine Zhang (2023-10-18 00:01:40) > >>>> Recent Rockchip SoCs have a new hardware block called Native Interface > >>>> Unit (NIU), which gates clocks to devices behind them. These effectively > >>>> need two parent clocks. > >>>> Use GATE_LINK to handle this. > >>> Why can't pm clks be used here? The qcom clk driver has been doing that > >>> for some time now. > >>> > >>> $ git grep pm_clk_add -- drivers/clk/qcom/ > >> Maybe I'm mistaken, but as far as I can tell this is adding the > >> dependency on controller level and only works because Qualcomm > >> has multiple separate clock controllers. In the Rockchip design > >> there is only one platform device. > >> > >> Note, that the original downstream code from Rockchip actually used > >> pm_clk infrastructure by moving these clocks to separate platform > >> devices. I changed this when upstreaming the code, since that leaks > >> into DT and from DT point of view there should be only one clock > >> controller. > >> > > Why can't the rockchip driver bind to a single device node and make > > sub-devices for each clk domain and register clks for those? Maybe it > > can use the auxiliary driver infrastructure to do that? > > Option 1: > > Use the current patch to adapt the GATE_LINK type upstream. > > The real function of GATE_LINK is implemented。 > > Just to improve and adapt the existing features on upstream. > > > Option 2: > > What we use on our internal branches are: Does this require DT changes? If so, it's a non-starter. Why can't auxiliary devices be used?
Recent Rockchip SoCs have a new hardware block called Native Interface Unit (NIU), which gates clocks to devices behind them. These effectively need two parent clocks. Use GATE_LINK to handle this. change in v4: [PATCH v4 1/4]: No change [PATCH v4 2/4]: No change [PATCH v4 3/4]: dropping CLK_NR_CLKS,reword commit message [PATCH v4 4/4]: No change change in V3: [PATCH v3 1/4]: new, export clk_gate_endisable for PATCH2. [PATCH v3 2/4]: reuse clk_gate_endisable and clk_gate_is_enabled. add prepare and unprepare ops. [PATCH v3 3/4]: No change [PATCH v3 4/4]: reword commit message change in V2: [PATCH v2 1/3]: fix reported warnings [PATCH v2 2/3]: Bindings submit independent patches [PATCH v2 3/3]: fix reported warnings Elaine Zhang (4): clk: gate: export clk_gate_endisable clk: rockchip: add support for gate link dt-bindings: clock: rk3588: export PCLK_VO1GRF clk id clk: rockchip: rk3588: Adjust the GATE_LINK parameter drivers/clk/clk-gate.c | 3 +- drivers/clk/rockchip/Makefile | 1 + drivers/clk/rockchip/clk-gate-link.c | 120 ++++++++++++++++++ drivers/clk/rockchip/clk-rk3588.c | 110 ++++++++-------- drivers/clk/rockchip/clk.c | 7 + drivers/clk/rockchip/clk.h | 22 ++++ .../dt-bindings/clock/rockchip,rk3588-cru.h | 2 +- include/linux/clk-provider.h | 1 + 8 files changed, 213 insertions(+), 53 deletions(-) create mode 100644 drivers/clk/rockchip/clk-gate-link.c