by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v2 11/11] riscv: dts: starfive: jh7110: Add STGCRG/ISPCRG/VOUTCRG nodes

On Tue, Feb 21, 2023 at 04:33:23PM +0800, Xingyu Wu wrote:
> Add STGCRG/ISPCRG/VOUTCRG new node to support JH7110
> System-Top-Group, Image-Signal-Process and Video-Output
> clock and reset drivers for the JH7110 RISC-V SoC.
>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 59 ++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index a5e6fb3ad188..697ab59191a1 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -6,6 +6,7 @@
>
> /dts-v1/;
> #include <dt-bindings/clock/starfive,jh7110-crg.h>
> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
> #include <dt-bindings/reset/starfive,jh7110-crg.h>

Please keep these sorted alphabetically, otherwise this *looks* fine to
me.
Reviewed-by: Conor Dooley <[email protected]>

Thanks,
Conor.

Attachments:

(No filename) (988.00 B)
signature.asc (228.00 B)
Download all attachments

2023-02-27 19:56:24

On 2023/3/3 17:23, Emil Renner Berthing wrote:
> On Fri, 3 Mar 2023 at 04:37, Xingyu Wu <[email protected]> wrote:
>>
>> On 2023/3/2 23:48, Emil Renner Berthing wrote:
>> > On Tue, 21 Feb 2023 at 09:40, Xingyu Wu <[email protected]> wrote:
>> >>
>> >> Add driver for the StarFive JH7110 Video-Output clock controller.
>> >>
>> >> Signed-off-by: Xingyu Wu <[email protected]>
>> >> ---
>> >> drivers/clk/starfive/Kconfig | 11 +
>> >> drivers/clk/starfive/Makefile | 1 +
>> >> .../clk/starfive/clk-starfive-jh7110-vout.c | 261 ++++++++++++++++++
>> >> 3 files changed, 273 insertions(+)
>> >> create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-vout.c
>> >>
>> >> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
>> >> index 59499acb95f7..5ebf1ed08627 100644
>> >> --- a/drivers/clk/starfive/Kconfig
>> >> +++ b/drivers/clk/starfive/Kconfig
>> >> @@ -64,3 +64,14 @@ config CLK_STARFIVE_JH7110_ISP
>> >> help
>> >> Say yes here to support the Image-Signal-Process clock controller
>> >> on the StarFive JH7110 SoC.
>> >> +
>> >> +config CLK_STARFIVE_JH7110_VOUT
>> >> + tristate "StarFive JH7110 Video-Output clock support"
>> >> + depends on CLK_STARFIVE_JH7110_SYS && JH71XX_PMU
>> >> + select AUXILIARY_BUS
>> >> + select CLK_STARFIVE_JH71X0
>> >> + select RESET_STARFIVE_JH7110
>> >> + default CLK_STARFIVE_JH7110_SYS
>> >
>> > default m if ARCH_STARFIVE
>>
>> Will modify it.
>>
>> >
>> >> + help
>> >> + Say yes here to support the Video-Output clock controller
>> >> + on the StarFive JH7110 SoC.
>> >> diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
>> >> index 76fb9f8d628b..841377e45bb6 100644
>> >> --- a/drivers/clk/starfive/Makefile
>> >> +++ b/drivers/clk/starfive/Makefile
>> >> @@ -8,3 +8,4 @@ obj-$(CONFIG_CLK_STARFIVE_JH7110_SYS) += clk-starfive-jh7110-sys.o
>> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_AON) += clk-starfive-jh7110-aon.o
>> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_STG) += clk-starfive-jh7110-stg.o
>> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_ISP) += clk-starfive-jh7110-isp.o
>> >> +obj-$(CONFIG_CLK_STARFIVE_JH7110_VOUT) += clk-starfive-jh7110-vout.o
>> >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-vout.c b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
>> >> new file mode 100644
>> >> index 000000000000..d786537563a4
>> >> --- /dev/null
>> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
>> >> @@ -0,0 +1,261 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * StarFive JH7110 Video-Output Clock Driver
>> >> + *
>> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> >> + */
>> >> +
>> >> +#include <linux/clk.h>
>> >> +#include <linux/clk-provider.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/of.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/pm_runtime.h>
>> >> +#include <linux/reset.h>
>> >> +
>> >> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> >> +
>> >> +#include "clk-starfive-jh71x0.h"
>> >> +
>> >> +/* external clocks */
>> >> +#define JH7110_VOUTCLK_VOUT_SRC (JH7110_VOUTCLK_END + 0)
>> >> +#define JH7110_VOUTCLK_VOUT_TOP_AHB (JH7110_VOUTCLK_END + 1)
>> >> +#define JH7110_VOUTCLK_VOUT_TOP_AXI (JH7110_VOUTCLK_END + 2)
>> >> +#define JH7110_VOUTCLK_VOUT_TOP_HDMITX0_MCLK (JH7110_VOUTCLK_END + 3)
>> >> +#define JH7110_VOUTCLK_I2STX0_BCLK (JH7110_VOUTCLK_END + 4)
>> >> +#define JH7110_VOUTCLK_HDMITX0_PIXELCLK (JH7110_VOUTCLK_END + 5)
>> >> +#define JH7110_VOUTCLK_EXT_END (JH7110_VOUTCLK_END + 6)
>> >> +
>> >> +static const struct jh71x0_clk_data jh7110_voutclk_data[] = {
>> >> + /* divider */
>> >> + JH71X0__DIV(JH7110_VOUTCLK_APB, "apb", 8, JH7110_VOUTCLK_VOUT_TOP_AHB),
>> >> + JH71X0__DIV(JH7110_VOUTCLK_DC8200_PIX, "dc8200_pix", 63, JH7110_VOUTCLK_VOUT_SRC),
>> >> + JH71X0__DIV(JH7110_VOUTCLK_DSI_SYS, "dsi_sys", 31, JH7110_VOUTCLK_VOUT_SRC),
>> >> + JH71X0__DIV(JH7110_VOUTCLK_TX_ESC, "tx_esc", 31, JH7110_VOUTCLK_VOUT_TOP_AHB),
>> >> + /* dc8200 */
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_AXI, "dc8200_axi", 0, JH7110_VOUTCLK_VOUT_TOP_AXI),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_CORE, "dc8200_core", 0, JH7110_VOUTCLK_VOUT_TOP_AXI),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_AHB, "dc8200_ahb", 0, JH7110_VOUTCLK_VOUT_TOP_AHB),
>> >> + JH71X0_GMUX(JH7110_VOUTCLK_DC8200_PIX0, "dc8200_pix0", 0, 2,
>> >> + JH7110_VOUTCLK_DC8200_PIX,
>> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
>> >> + JH71X0_GMUX(JH7110_VOUTCLK_DC8200_PIX1, "dc8200_pix1", 0, 2,
>> >> + JH7110_VOUTCLK_DC8200_PIX,
>> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
>> >> + /* LCD */
>> >> + JH71X0_GMUX(JH7110_VOUTCLK_DOM_VOUT_TOP_LCD, "dom_vout_top_lcd", 0, 2,
>> >> + JH7110_VOUTCLK_DC8200_PIX0,
>> >> + JH7110_VOUTCLK_DC8200_PIX1),
>> >> + /* dsiTx */
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_APB, "dsiTx_apb", 0, JH7110_VOUTCLK_DSI_SYS),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_SYS, "dsiTx_sys", 0, JH7110_VOUTCLK_DSI_SYS),
>> >> + JH71X0_GMUX(JH7110_VOUTCLK_DSITX_DPI, "dsiTx_dpi", 0, 2,
>> >> + JH7110_VOUTCLK_DC8200_PIX,
>> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_TXESC, "dsiTx_txesc", 0, JH7110_VOUTCLK_TX_ESC),
>> >> + /* mipitx DPHY */
>> >> + JH71X0_GATE(JH7110_VOUTCLK_MIPITX_DPHY_TXESC, "mipitx_dphy_txesc", 0,
>> >> + JH7110_VOUTCLK_TX_ESC),
>> >> + /* hdmi */
>> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_MCLK, "hdmi_tx_mclk", 0,
>> >> + JH7110_VOUTCLK_VOUT_TOP_HDMITX0_MCLK),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_BCLK, "hdmi_tx_bclk", 0,
>> >> + JH7110_VOUTCLK_I2STX0_BCLK),
>> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_SYS, "hdmi_tx_sys", 0, JH7110_VOUTCLK_APB),
>> >> +};
>> >> +
>> >> +struct vout_top_crg {
>> >> + struct clk_bulk_data *top_clks;
>> >> + struct reset_control *top_rst;
>> >> + int top_clks_num;
>> >> + void __iomem *base;
>> >> +};
>> >> +
>> >> +static struct clk_bulk_data jh7110_vout_top_clks[] = {
>> >> + { .id = "vout_src" },
>> >> + { .id = "vout_top_ahb" }
>> >> +};
>> >> +
>> >> +static struct vout_top_crg *top_crg_from(void __iomem **base)
>> >> +{
>> >> + return container_of(base, struct vout_top_crg, base);
>> >> +}
>> >> +
>> >> +static int jh7110_vout_top_crg_get(struct jh71x0_clk_priv *priv, struct vout_top_crg *top)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + top->top_clks = jh7110_vout_top_clks;
>> >> + top->top_clks_num = ARRAY_SIZE(jh7110_vout_top_clks);
>> >> + ret = devm_clk_bulk_get(priv->dev, top->top_clks_num, top->top_clks);
>> >> + if (ret) {
>> >> + dev_err(priv->dev, "top clks get failed: %d\n", ret);
>> >> + return ret;
>> >> + }
>> >> +
>> >> + /* The reset should be shared and other Vout modules will use its. */
>> >> + top->top_rst = devm_reset_control_get_shared(priv->dev, NULL);
>> >> + if (IS_ERR(top->top_rst)) {
>> >> + dev_err(priv->dev, "top rst get failed\n");
>> >> + return PTR_ERR(top->top_rst);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int jh7110_vout_top_crg_enable(struct vout_top_crg *top)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + ret = clk_bulk_prepare_enable(top->top_clks_num, top->top_clks);
>> >> + if (ret)
>> >> + return ret;
>> >
>> > Hmm.. do all the clocks used as input really need to be enabled for
>> > any one of the vout clocks to work?
>> >
>> > In other words: suppose you just need a single clock in the VOUTCRG
>> > domain. Do we really need to turn on all the input clocks for the
>> > VOUTCRG for that one clock to work? Normally I'd expect the clock
>> > framework to make sure all parents of that clock are enabled.
>>
>> It must enable core clock and ahb clock before reading or writing VOUTCRG registers
>> otherwise it would be failed to read and write. And it is even crash if it don't
>> enable core clock before reading or writing ISPCRG registers.
>> This serious problem was found when debugging earlier.
>
> Ah sorry. I see now that you're only claiming the core and AHB bus.
> That makes sense.
>
> So now the question is if there are similar clocks that need to be
> claimed by the AON, STG and ISP CRGs. They're most likely already
> turned on by u-boot or by default which is why we don't see errors,
> but Linux should still claim them if the driver needs them.

Oh, just VOUTCRG and ISPCRG should actively enable the clock. The clocks like bus clock
are enabled by default about AONCRG and STGCRG. And these clocks already uses CLK_IS_CRITICAL
in SYSCRG to make sure it would not be disabled.

>
>> >
>> >> +
>> >> + return reset_control_deassert(top->top_rst);
>> >> +}
>> >> +
>> >> +static void jh7110_vout_top_crg_disable(struct vout_top_crg *top)
>> >> +{
>> >> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
>> >> +}
>> >> +
>> >> +static struct clk_hw *jh7110_voutclk_get(struct of_phandle_args *clkspec, void *data)
>> >> +{
>> >> + struct jh71x0_clk_priv *priv = data;
>> >> + unsigned int idx = clkspec->args[0];
>> >> +
>> >> + if (idx < JH7110_VOUTCLK_END)
>> >> + return &priv->reg[idx].hw;
>> >> +
>> >> + return ERR_PTR(-EINVAL);
>> >> +}
>> >> +
>> >> +static int jh7110_voutcrg_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct jh71x0_clk_priv *priv;
>> >> + struct vout_top_crg *top;
>> >> + unsigned int idx;
>> >> + int ret;
>> >> +
>> >> + priv = devm_kzalloc(&pdev->dev,
>> >> + struct_size(priv, reg, JH7110_VOUTCLK_END),
>> >> + GFP_KERNEL);
>> >> + if (!priv)
>> >> + return -ENOMEM;
>> >> +
>> >> + top = devm_kzalloc(&pdev->dev, sizeof(*top), GFP_KERNEL);
>> >> + if (!top)
>> >> + 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);
>> >> +
>> >> + top->base = priv->base;
>> >> + dev_set_drvdata(priv->dev, (void *)(&top->base));
>> >> +
>> >> + pm_runtime_enable(priv->dev);
>> >> + ret = pm_runtime_get_sync(priv->dev);
>> >> + if (ret < 0) {
>> >> + dev_err(priv->dev, "failed to turn power: %d\n", ret);
>> >> + return ret;
>> >> + }
>> >> +
>> >> + ret = jh7110_vout_top_crg_get(priv, top);
>> >> + if (ret)
>> >> + goto err_clk;
>> >> +
>> >> + ret = jh7110_vout_top_crg_enable(top);
>> >> + if (ret)
>> >> + goto err_clk;
>> >> +
>> >> + for (idx = 0; idx < JH7110_VOUTCLK_END; idx++) {
>> >> + u32 max = jh7110_voutclk_data[idx].max;
>> >> + struct clk_parent_data parents[4] = {};
>> >> + struct clk_init_data init = {
>> >> + .name = jh7110_voutclk_data[idx].name,
>> >> + .ops = starfive_jh71x0_clk_ops(max),
>> >> + .parent_data = parents,
>> >> + .num_parents =
>> >> + ((max & JH71X0_CLK_MUX_MASK) >> JH71X0_CLK_MUX_SHIFT) + 1,
>> >> + .flags = jh7110_voutclk_data[idx].flags,
>> >> + };
>> >> + struct jh71x0_clk *clk = &priv->reg[idx];
>> >> + unsigned int i;
>> >> + char *fw_name[JH7110_VOUTCLK_EXT_END - JH7110_VOUTCLK_END] = {
>> >
>> > this can be const char *const fw_name[...] right?
>> >
>> >> + "vout_src",
>> >> + "vout_top_ahb",
>> >> + "vout_top_axi",
>> >> + "vout_top_hdmitx0_mclk",
>> >> + "i2stx0_bclk",
>> >> + "hdmitx0_pixelclk"
>> >> + };
>> >> +
>> >> + for (i = 0; i < init.num_parents; i++) {
>> >> + unsigned int pidx = jh7110_voutclk_data[idx].parents[i];
>> >> +
>> >> + if (pidx < JH7110_VOUTCLK_END)
>> >> + parents[i].hw = &priv->reg[pidx].hw;
>> >> + else if (pidx < JH7110_VOUTCLK_EXT_END)
>> >> + parents[i].fw_name = fw_name[pidx - JH7110_VOUTCLK_END];
>> >> + }
>> >> +
>> >> + clk->hw.init = &init;
>> >> + clk->idx = idx;
>> >> + clk->max_div = max & JH71X0_CLK_DIV_MASK;
>> >> +
>> >> + ret = devm_clk_hw_register(&pdev->dev, &clk->hw);
>> >> + if (ret)
>> >> + goto err_exit;
>> >> + }
>> >> +
>> >> + ret = devm_of_clk_add_hw_provider(&pdev->dev, jh7110_voutclk_get, priv);
>> >> + if (ret)
>> >> + goto err_exit;
>> >> +
>> >> + ret = jh7110_reset_controller_register(priv, "reset-vout", 4);
>> >> + if (ret)
>> >> + goto err_exit;
>> >> +
>> >> + return 0;
>> >> +
>> >> +err_exit:
>> >> + jh7110_vout_top_crg_disable(top);
>> >> +err_clk:
>> >> + pm_runtime_put_sync(priv->dev);
>> >> + pm_runtime_disable(priv->dev);
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int jh7110_voutcrg_remove(struct platform_device *pdev)
>> >> +{
>> >> + void __iomem **base = dev_get_drvdata(&pdev->dev);
>> >> + struct vout_top_crg *top = top_crg_from(base);
>> >> +
>> >> + jh7110_vout_top_crg_disable(top);
>> >> + pm_runtime_disable(&pdev->dev);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static const struct of_device_id jh7110_voutcrg_match[] = {
>> >> + { .compatible = "starfive,jh7110-voutcrg" },
>> >> + { /* sentinel */ }
>> >> +};
>> >> +MODULE_DEVICE_TABLE(of, jh7110_voutcrg_match);
>> >> +
>> >> +static struct platform_driver jh7110_voutcrg_driver = {
>> >> + .probe = jh7110_voutcrg_probe,
>> >> + .remove = jh7110_voutcrg_remove,
>> >> + .driver = {
>> >> + .name = "clk-starfive-jh7110-vout",
>> >> + .of_match_table = jh7110_voutcrg_match,
>> >> + },
>> >> +};
>> >> +module_platform_driver(jh7110_voutcrg_driver);
>> >> +
>> >> +MODULE_AUTHOR("Xingyu Wu <[email protected]>");
>> >> +MODULE_DESCRIPTION("StarFive JH7110 Video-Output clock driver");
>> >> +MODULE_LICENSE("GPL");
>> >> --
>> >> 2.25.1
>> >>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> [email protected]
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv

Best regards,
Xingyu Wu

2023-03-03 09:52:36

by Emil Renner Berthing

[permalink] [raw]

Subject: Re: [PATCH v2 09/11] clk: starfive: Add StarFive JH7110 Video-Output clock driver

On Fri, 3 Mar 2023 at 10:44, Xingyu Wu <[email protected]> wrote:
> On 2023/3/3 17:23, Emil Renner Berthing wrote:
> > On Fri, 3 Mar 2023 at 04:37, Xingyu Wu <[email protected]> wrote:
> >>
> >> On 2023/3/2 23:48, Emil Renner Berthing wrote:
> >> > On Tue, 21 Feb 2023 at 09:40, Xingyu Wu <[email protected]> wrote:
> >> >>
> >> >> Add driver for the StarFive JH7110 Video-Output clock controller.
> >> >>
> >> >> Signed-off-by: Xingyu Wu <[email protected]>
> >> >> ---
> >> >> drivers/clk/starfive/Kconfig | 11 +
> >> >> drivers/clk/starfive/Makefile | 1 +
> >> >> .../clk/starfive/clk-starfive-jh7110-vout.c | 261 ++++++++++++++++++
> >> >> 3 files changed, 273 insertions(+)
> >> >> create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-vout.c
> >> >>
> >> >> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
> >> >> index 59499acb95f7..5ebf1ed08627 100644
> >> >> --- a/drivers/clk/starfive/Kconfig
> >> >> +++ b/drivers/clk/starfive/Kconfig
> >> >> @@ -64,3 +64,14 @@ config CLK_STARFIVE_JH7110_ISP
> >> >> help
> >> >> Say yes here to support the Image-Signal-Process clock controller
> >> >> on the StarFive JH7110 SoC.
> >> >> +
> >> >> +config CLK_STARFIVE_JH7110_VOUT
> >> >> + tristate "StarFive JH7110 Video-Output clock support"
> >> >> + depends on CLK_STARFIVE_JH7110_SYS && JH71XX_PMU
> >> >> + select AUXILIARY_BUS
> >> >> + select CLK_STARFIVE_JH71X0
> >> >> + select RESET_STARFIVE_JH7110
> >> >> + default CLK_STARFIVE_JH7110_SYS
> >> >
> >> > default m if ARCH_STARFIVE
> >>
> >> Will modify it.
> >>
> >> >
> >> >> + help
> >> >> + Say yes here to support the Video-Output clock controller
> >> >> + on the StarFive JH7110 SoC.
> >> >> diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
> >> >> index 76fb9f8d628b..841377e45bb6 100644
> >> >> --- a/drivers/clk/starfive/Makefile
> >> >> +++ b/drivers/clk/starfive/Makefile
> >> >> @@ -8,3 +8,4 @@ obj-$(CONFIG_CLK_STARFIVE_JH7110_SYS) += clk-starfive-jh7110-sys.o
> >> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_AON) += clk-starfive-jh7110-aon.o
> >> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_STG) += clk-starfive-jh7110-stg.o
> >> >> obj-$(CONFIG_CLK_STARFIVE_JH7110_ISP) += clk-starfive-jh7110-isp.o
> >> >> +obj-$(CONFIG_CLK_STARFIVE_JH7110_VOUT) += clk-starfive-jh7110-vout.o
> >> >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-vout.c b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
> >> >> new file mode 100644
> >> >> index 000000000000..d786537563a4
> >> >> --- /dev/null
> >> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
> >> >> @@ -0,0 +1,261 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +/*
> >> >> + * StarFive JH7110 Video-Output Clock Driver
> >> >> + *
> >> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> >> + */
> >> >> +
> >> >> +#include <linux/clk.h>
> >> >> +#include <linux/clk-provider.h>
> >> >> +#include <linux/io.h>
> >> >> +#include <linux/of.h>
> >> >> +#include <linux/platform_device.h>
> >> >> +#include <linux/pm_runtime.h>
> >> >> +#include <linux/reset.h>
> >> >> +
> >> >> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> >> >> +
> >> >> +#include "clk-starfive-jh71x0.h"
> >> >> +
> >> >> +/* external clocks */
> >> >> +#define JH7110_VOUTCLK_VOUT_SRC (JH7110_VOUTCLK_END + 0)
> >> >> +#define JH7110_VOUTCLK_VOUT_TOP_AHB (JH7110_VOUTCLK_END + 1)
> >> >> +#define JH7110_VOUTCLK_VOUT_TOP_AXI (JH7110_VOUTCLK_END + 2)
> >> >> +#define JH7110_VOUTCLK_VOUT_TOP_HDMITX0_MCLK (JH7110_VOUTCLK_END + 3)
> >> >> +#define JH7110_VOUTCLK_I2STX0_BCLK (JH7110_VOUTCLK_END + 4)
> >> >> +#define JH7110_VOUTCLK_HDMITX0_PIXELCLK (JH7110_VOUTCLK_END + 5)
> >> >> +#define JH7110_VOUTCLK_EXT_END (JH7110_VOUTCLK_END + 6)
> >> >> +
> >> >> +static const struct jh71x0_clk_data jh7110_voutclk_data[] = {
> >> >> + /* divider */
> >> >> + JH71X0__DIV(JH7110_VOUTCLK_APB, "apb", 8, JH7110_VOUTCLK_VOUT_TOP_AHB),
> >> >> + JH71X0__DIV(JH7110_VOUTCLK_DC8200_PIX, "dc8200_pix", 63, JH7110_VOUTCLK_VOUT_SRC),
> >> >> + JH71X0__DIV(JH7110_VOUTCLK_DSI_SYS, "dsi_sys", 31, JH7110_VOUTCLK_VOUT_SRC),
> >> >> + JH71X0__DIV(JH7110_VOUTCLK_TX_ESC, "tx_esc", 31, JH7110_VOUTCLK_VOUT_TOP_AHB),
> >> >> + /* dc8200 */
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_AXI, "dc8200_axi", 0, JH7110_VOUTCLK_VOUT_TOP_AXI),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_CORE, "dc8200_core", 0, JH7110_VOUTCLK_VOUT_TOP_AXI),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DC8200_AHB, "dc8200_ahb", 0, JH7110_VOUTCLK_VOUT_TOP_AHB),
> >> >> + JH71X0_GMUX(JH7110_VOUTCLK_DC8200_PIX0, "dc8200_pix0", 0, 2,
> >> >> + JH7110_VOUTCLK_DC8200_PIX,
> >> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
> >> >> + JH71X0_GMUX(JH7110_VOUTCLK_DC8200_PIX1, "dc8200_pix1", 0, 2,
> >> >> + JH7110_VOUTCLK_DC8200_PIX,
> >> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
> >> >> + /* LCD */
> >> >> + JH71X0_GMUX(JH7110_VOUTCLK_DOM_VOUT_TOP_LCD, "dom_vout_top_lcd", 0, 2,
> >> >> + JH7110_VOUTCLK_DC8200_PIX0,
> >> >> + JH7110_VOUTCLK_DC8200_PIX1),
> >> >> + /* dsiTx */
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_APB, "dsiTx_apb", 0, JH7110_VOUTCLK_DSI_SYS),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_SYS, "dsiTx_sys", 0, JH7110_VOUTCLK_DSI_SYS),
> >> >> + JH71X0_GMUX(JH7110_VOUTCLK_DSITX_DPI, "dsiTx_dpi", 0, 2,
> >> >> + JH7110_VOUTCLK_DC8200_PIX,
> >> >> + JH7110_VOUTCLK_HDMITX0_PIXELCLK),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_DSITX_TXESC, "dsiTx_txesc", 0, JH7110_VOUTCLK_TX_ESC),
> >> >> + /* mipitx DPHY */
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_MIPITX_DPHY_TXESC, "mipitx_dphy_txesc", 0,
> >> >> + JH7110_VOUTCLK_TX_ESC),
> >> >> + /* hdmi */
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_MCLK, "hdmi_tx_mclk", 0,
> >> >> + JH7110_VOUTCLK_VOUT_TOP_HDMITX0_MCLK),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_BCLK, "hdmi_tx_bclk", 0,
> >> >> + JH7110_VOUTCLK_I2STX0_BCLK),
> >> >> + JH71X0_GATE(JH7110_VOUTCLK_HDMI_TX_SYS, "hdmi_tx_sys", 0, JH7110_VOUTCLK_APB),
> >> >> +};
> >> >> +
> >> >> +struct vout_top_crg {
> >> >> + struct clk_bulk_data *top_clks;
> >> >> + struct reset_control *top_rst;
> >> >> + int top_clks_num;
> >> >> + void __iomem *base;
> >> >> +};
> >> >> +
> >> >> +static struct clk_bulk_data jh7110_vout_top_clks[] = {
> >> >> + { .id = "vout_src" },
> >> >> + { .id = "vout_top_ahb" }
> >> >> +};
> >> >> +
> >> >> +static struct vout_top_crg *top_crg_from(void __iomem **base)
> >> >> +{
> >> >> + return container_of(base, struct vout_top_crg, base);
> >> >> +}
> >> >> +
> >> >> +static int jh7110_vout_top_crg_get(struct jh71x0_clk_priv *priv, struct vout_top_crg *top)
> >> >> +{
> >> >> + int ret;
> >> >> +
> >> >> + top->top_clks = jh7110_vout_top_clks;
> >> >> + top->top_clks_num = ARRAY_SIZE(jh7110_vout_top_clks);
> >> >> + ret = devm_clk_bulk_get(priv->dev, top->top_clks_num, top->top_clks);
> >> >> + if (ret) {
> >> >> + dev_err(priv->dev, "top clks get failed: %d\n", ret);
> >> >> + return ret;
> >> >> + }
> >> >> +
> >> >> + /* The reset should be shared and other Vout modules will use its. */
> >> >> + top->top_rst = devm_reset_control_get_shared(priv->dev, NULL);
> >> >> + if (IS_ERR(top->top_rst)) {
> >> >> + dev_err(priv->dev, "top rst get failed\n");
> >> >> + return PTR_ERR(top->top_rst);
> >> >> + }
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static int jh7110_vout_top_crg_enable(struct vout_top_crg *top)
> >> >> +{
> >> >> + int ret;
> >> >> +
> >> >> + ret = clk_bulk_prepare_enable(top->top_clks_num, top->top_clks);
> >> >> + if (ret)
> >> >> + return ret;
> >> >
> >> > Hmm.. do all the clocks used as input really need to be enabled for
> >> > any one of the vout clocks to work?
> >> >
> >> > In other words: suppose you just need a single clock in the VOUTCRG
> >> > domain. Do we really need to turn on all the input clocks for the
> >> > VOUTCRG for that one clock to work? Normally I'd expect the clock
> >> > framework to make sure all parents of that clock are enabled.
> >>
> >> It must enable core clock and ahb clock before reading or writing VOUTCRG registers
> >> otherwise it would be failed to read and write. And it is even crash if it don't
> >> enable core clock before reading or writing ISPCRG registers.
> >> This serious problem was found when debugging earlier.
> >
> > Ah sorry. I see now that you're only claiming the core and AHB bus.
> > That makes sense.
> >
> > So now the question is if there are similar clocks that need to be
> > claimed by the AON, STG and ISP CRGs. They're most likely already
> > turned on by u-boot or by default which is why we don't see errors,
> > but Linux should still claim them if the driver needs them.
>
> Oh, just VOUTCRG and ISPCRG should actively enable the clock. The clocks like bus clock
> are enabled by default about AONCRG and STGCRG. And these clocks already uses CLK_IS_CRITICAL
> in SYSCRG to make sure it would not be disabled.

Oh, so maybe those clocks are not critical, but just need to be
claimed and turned on by the AONCRG and STGCRG drivers before use?

> >
> >> >
> >> >> +
> >> >> + return reset_control_deassert(top->top_rst);
> >> >> +}
> >> >> +
> >> >> +static void jh7110_vout_top_crg_disable(struct vout_top_crg *top)
> >> >> +{
> >> >> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
> >> >> +}
> >> >> +
> >> >> +static struct clk_hw *jh7110_voutclk_get(struct of_phandle_args *clkspec, void *data)
> >> >> +{
> >> >> + struct jh71x0_clk_priv *priv = data;
> >> >> + unsigned int idx = clkspec->args[0];
> >> >> +
> >> >> + if (idx < JH7110_VOUTCLK_END)
> >> >> + return &priv->reg[idx].hw;
> >> >> +
> >> >> + return ERR_PTR(-EINVAL);
> >> >> +}
> >> >> +
> >> >> +static int jh7110_voutcrg_probe(struct platform_device *pdev)
> >> >> +{
> >> >> + struct jh71x0_clk_priv *priv;
> >> >> + struct vout_top_crg *top;
> >> >> + unsigned int idx;
> >> >> + int ret;
> >> >> +
> >> >> + priv = devm_kzalloc(&pdev->dev,
> >> >> + struct_size(priv, reg, JH7110_VOUTCLK_END),
> >> >> + GFP_KERNEL);
> >> >> + if (!priv)
> >> >> + return -ENOMEM;
> >> >> +
> >> >> + top = devm_kzalloc(&pdev->dev, sizeof(*top), GFP_KERNEL);
> >> >> + if (!top)
> >> >> + 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);
> >> >> +
> >> >> + top->base = priv->base;
> >> >> + dev_set_drvdata(priv->dev, (void *)(&top->base));
> >> >> +
> >> >> + pm_runtime_enable(priv->dev);
> >> >> + ret = pm_runtime_get_sync(priv->dev);
> >> >> + if (ret < 0) {
> >> >> + dev_err(priv->dev, "failed to turn power: %d\n", ret);
> >> >> + return ret;
> >> >> + }
> >> >> +
> >> >> + ret = jh7110_vout_top_crg_get(priv, top);
> >> >> + if (ret)
> >> >> + goto err_clk;
> >> >> +
> >> >> + ret = jh7110_vout_top_crg_enable(top);
> >> >> + if (ret)
> >> >> + goto err_clk;
> >> >> +
> >> >> + for (idx = 0; idx < JH7110_VOUTCLK_END; idx++) {
> >> >> + u32 max = jh7110_voutclk_data[idx].max;
> >> >> + struct clk_parent_data parents[4] = {};
> >> >> + struct clk_init_data init = {
> >> >> + .name = jh7110_voutclk_data[idx].name,
> >> >> + .ops = starfive_jh71x0_clk_ops(max),
> >> >> + .parent_data = parents,
> >> >> + .num_parents =
> >> >> + ((max & JH71X0_CLK_MUX_MASK) >> JH71X0_CLK_MUX_SHIFT) + 1,
> >> >> + .flags = jh7110_voutclk_data[idx].flags,
> >> >> + };
> >> >> + struct jh71x0_clk *clk = &priv->reg[idx];
> >> >> + unsigned int i;
> >> >> + char *fw_name[JH7110_VOUTCLK_EXT_END - JH7110_VOUTCLK_END] = {
> >> >
> >> > this can be const char *const fw_name[...] right?
> >> >
> >> >> + "vout_src",
> >> >> + "vout_top_ahb",
> >> >> + "vout_top_axi",
> >> >> + "vout_top_hdmitx0_mclk",
> >> >> + "i2stx0_bclk",
> >> >> + "hdmitx0_pixelclk"
> >> >> + };
> >> >> +
> >> >> + for (i = 0; i < init.num_parents; i++) {
> >> >> + unsigned int pidx = jh7110_voutclk_data[idx].parents[i];
> >> >> +
> >> >> + if (pidx < JH7110_VOUTCLK_END)
> >> >> + parents[i].hw = &priv->reg[pidx].hw;
> >> >> + else if (pidx < JH7110_VOUTCLK_EXT_END)
> >> >> + parents[i].fw_name = fw_name[pidx - JH7110_VOUTCLK_END];
> >> >> + }
> >> >> +
> >> >> + clk->hw.init = &init;
> >> >> + clk->idx = idx;
> >> >> + clk->max_div = max & JH71X0_CLK_DIV_MASK;
> >> >> +
> >> >> + ret = devm_clk_hw_register(&pdev->dev, &clk->hw);
> >> >> + if (ret)
> >> >> + goto err_exit;
> >> >> + }
> >> >> +
> >> >> + ret = devm_of_clk_add_hw_provider(&pdev->dev, jh7110_voutclk_get, priv);
> >> >> + if (ret)
> >> >> + goto err_exit;
> >> >> +
> >> >> + ret = jh7110_reset_controller_register(priv, "reset-vout", 4);
> >> >> + if (ret)
> >> >> + goto err_exit;
> >> >> +
> >> >> + return 0;
> >> >> +
> >> >> +err_exit:
> >> >> + jh7110_vout_top_crg_disable(top);
> >> >> +err_clk:
> >> >> + pm_runtime_put_sync(priv->dev);
> >> >> + pm_runtime_disable(priv->dev);
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> +static int jh7110_voutcrg_remove(struct platform_device *pdev)
> >> >> +{
> >> >> + void __iomem **base = dev_get_drvdata(&pdev->dev);
> >> >> + struct vout_top_crg *top = top_crg_from(base);
> >> >> +
> >> >> + jh7110_vout_top_crg_disable(top);
> >> >> + pm_runtime_disable(&pdev->dev);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static const struct of_device_id jh7110_voutcrg_match[] = {
> >> >> + { .compatible = "starfive,jh7110-voutcrg" },
> >> >> + { /* sentinel */ }
> >> >> +};
> >> >> +MODULE_DEVICE_TABLE(of, jh7110_voutcrg_match);
> >> >> +
> >> >> +static struct platform_driver jh7110_voutcrg_driver = {
> >> >> + .probe = jh7110_voutcrg_probe,
> >> >> + .remove = jh7110_voutcrg_remove,
> >> >> + .driver = {
> >> >> + .name = "clk-starfive-jh7110-vout",
> >> >> + .of_match_table = jh7110_voutcrg_match,
> >> >> + },
> >> >> +};
> >> >> +module_platform_driver(jh7110_voutcrg_driver);
> >> >> +
> >> >> +MODULE_AUTHOR("Xingyu Wu <[email protected]>");
> >> >> +MODULE_DESCRIPTION("StarFive JH7110 Video-Output clock driver");
> >> >> +MODULE_LICENSE("GPL");
> >> >> --
> >> >> 2.25.1
> >> >>
> >> >>
> >> >> _______________________________________________
> >> >> linux-riscv mailing list
> >> >> [email protected]
> >> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Best regards,
> Xingyu Wu
>