[v4,07/10] clk: starfive: Add StarFive JH7110 Video-Output clock driver
Commit Message
Add driver for the StarFive JH7110 Video-Output clock controller.
And these clock controllers should power on and enable the clocks from
SYSCRG first before registering.
Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
drivers/clk/starfive/Kconfig | 11 +
drivers/clk/starfive/Makefile | 1 +
.../clk/starfive/clk-starfive-jh7110-vout.c | 239 ++++++++++++++++++
3 files changed, 251 insertions(+)
create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-vout.c
Comments
Quoting Xingyu Wu (2023-04-11 06:55:55)
> 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..4c6f5ae198cf
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
> @@ -0,0 +1,239 @@
> +// 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/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
Include module.h, device.h, and kernel.h for things like ERR_PTR().
Probably need to include a reset header as well for reset APIs.
> +
> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> +
> +#include "clk-starfive-jh7110.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)
> +
> +/* VOUT domian clocks */
> +struct vout_top_crg {
> + struct clk_bulk_data *top_clks;
> + int top_clks_num;
size_t?
> + void __iomem *base;
> +};
> +
> +static struct clk_bulk_data jh7110_vout_top_clks[] = {
> + { .id = "vout_src" },
> + { .id = "vout_top_ahb" }
> +};
> +
> +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),
> +};
> +
> +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_init(struct jh71x0_clk_priv *priv, struct vout_top_crg *top)
> +{
> + struct reset_control *top_rst;
> + 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)
> + return dev_err_probe(priv->dev, ret, "failed to get top clocks\n");
> +
> + /* The reset should be shared and other Vout modules will use its. */
> + top_rst = devm_reset_control_get_shared(priv->dev, NULL);
> + if (IS_ERR(top_rst))
> + return dev_err_probe(priv->dev, PTR_ERR(top_rst), "failed to get top reset\n");
> +
> + ret = clk_bulk_prepare_enable(top->top_clks_num, top->top_clks);
> + if (ret)
> + return dev_err_probe(priv->dev, ret, "failed to enable top clocks\n");
> +
> + return reset_control_deassert(top_rst);
> +}
> +
> +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);
> +
> + pm_runtime_enable(priv->dev);
Use devm_pm_runtime_enable()?
> + ret = pm_runtime_get_sync(priv->dev);
And use pm_runtime_resume_and_get() here?
> + if (ret < 0)
> + return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
> +
> + ret = jh7110_vout_top_crg_init(priv, top);
> + if (ret)
> + goto err_clk;
> +
> + top->base = priv->base;
> + dev_set_drvdata(priv->dev, (void *)(&top->base));
See comment later about setting this to 'top' instead. Casting away
iomem markings is not good hygiene.
> +
> + 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;
> + const char *fw_name[JH7110_VOUTCLK_EXT_END - JH7110_VOUTCLK_END] = {
> + "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];
Can you use .index instead?
> + }
> +
> + 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, "rst-vout", 4);
> + if (ret)
> + goto err_exit;
> +
> + return 0;
> +
> +err_exit:
> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
> +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);
Why not set the driver data to be vout_top_crg?
> + struct vout_top_crg *top = top_crg_from(base);
And get rid of this top_crg_from() API?
> +
> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
> + 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,
Use remove_new please.
On 2023/4/12 2:33, Stephen Boyd wrote:
> Quoting Xingyu Wu (2023-04-11 06:55:55)
>> 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..4c6f5ae198cf
>> --- /dev/null
>> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
>> @@ -0,0 +1,239 @@
>> +// 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/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>
> Include module.h, device.h, and kernel.h for things like ERR_PTR().
The local headfile 'clk-starfive-jh71x0.h' from the basic JH71x0 clock driver
already includes the device.h.
And I found the module.h is included in device/driver.h file and then it is included
in the device.h file.
The kernel.h is included in the clk.h file.
So do I still need to list them?
> Probably need to include a reset header as well for reset APIs.
The reset APIs like devm_reset_control_get_shared() and reset_control_deassert()
come from the reset.h file and I have included it.
>
>> +
>> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> +
>> +#include "clk-starfive-jh7110.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)
>> +
>> +/* VOUT domian clocks */
>> +struct vout_top_crg {
>> + struct clk_bulk_data *top_clks;
>> + int top_clks_num;
>
> size_t?
Will modify to 'unsigned int'.
>
>> + void __iomem *base;
>> +};
>> +
>> +static struct clk_bulk_data jh7110_vout_top_clks[] = {
>> + { .id = "vout_src" },
>> + { .id = "vout_top_ahb" }
>> +};
>> +
>> +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),
>> +};
>> +
>> +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_init(struct jh71x0_clk_priv *priv, struct vout_top_crg *top)
>> +{
>> + struct reset_control *top_rst;
>> + 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)
>> + return dev_err_probe(priv->dev, ret, "failed to get top clocks\n");
>> +
>> + /* The reset should be shared and other Vout modules will use its. */
>> + top_rst = devm_reset_control_get_shared(priv->dev, NULL);
>> + if (IS_ERR(top_rst))
>> + return dev_err_probe(priv->dev, PTR_ERR(top_rst), "failed to get top reset\n");
>> +
>> + ret = clk_bulk_prepare_enable(top->top_clks_num, top->top_clks);
>> + if (ret)
>> + return dev_err_probe(priv->dev, ret, "failed to enable top clocks\n");
>> +
>> + return reset_control_deassert(top_rst);
>> +}
>> +
>> +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);
>> +
>> + pm_runtime_enable(priv->dev);
>
> Use devm_pm_runtime_enable()?
Will fix.
>
>> + ret = pm_runtime_get_sync(priv->dev);
>
> And use pm_runtime_resume_and_get() here?
Will fix.
>
>> + if (ret < 0)
>> + return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
>> +
>> + ret = jh7110_vout_top_crg_init(priv, top);
>> + if (ret)
>> + goto err_clk;
>> +
>> + top->base = priv->base;
>> + dev_set_drvdata(priv->dev, (void *)(&top->base));
>
> See comment later about setting this to 'top' instead. Casting away
> iomem markings is not good hygiene.
JH7110 resets as the auxiliary device of clocks use the same iomem as the clocks
and the iomem will be got by dev_get_drvdata() in the 7110 reset drivers when registering reset.
So I follow the basic 7110 reset driver and also set the iomem not top_crg struct.
>
>> +
>> + 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;
>> + const char *fw_name[JH7110_VOUTCLK_EXT_END - JH7110_VOUTCLK_END] = {
>> + "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];
>
> Can you use .index instead?
OK, I try to use it instead.
>
>> + }
>> +
>> + 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, "rst-vout", 4);
>> + if (ret)
>> + goto err_exit;
>> +
>> + return 0;
>> +
>> +err_exit:
>> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
>> +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);
>
> Why not set the driver data to be vout_top_crg?
The reason is stated above.
>
>> + struct vout_top_crg *top = top_crg_from(base);
>
> And get rid of this top_crg_from() API?
>
>> +
>> + clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
>> + 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,
>
> Use remove_new please.
Will fix.
Thanks.
Best regards,
Xingyu Wu
Quoting Xingyu Wu (2023-04-11 23:15:26)
> On 2023/4/12 2:33, Stephen Boyd wrote:
> > Quoting Xingyu Wu (2023-04-11 06:55:55)
> >> 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..4c6f5ae198cf
> >> --- /dev/null
> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
> >> @@ -0,0 +1,239 @@
> >> +// 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/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >
> > Include module.h, device.h, and kernel.h for things like ERR_PTR().
>
> The local headfile 'clk-starfive-jh71x0.h' from the basic JH71x0 clock driver
> already includes the device.h.
> And I found the module.h is included in device/driver.h file and then it is included
> in the device.h file.
> The kernel.h is included in the clk.h file.
> So do I still need to list them?
Yes.
>
> > Probably need to include a reset header as well for reset APIs.
>
> The reset APIs like devm_reset_control_get_shared() and reset_control_deassert()
> come from the reset.h file and I have included it.
Cool, I missed it.
>
> >
> >> +
> >> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> >> +
> >> +#include "clk-starfive-jh7110.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)
> >> +
> >> +/* VOUT domian clocks */
> >> +struct vout_top_crg {
> >> + struct clk_bulk_data *top_clks;
> >> + int top_clks_num;
> >
> > size_t?
>
> Will modify to 'unsigned int'.
Why not size_t?
>
> >
> >> + if (ret < 0)
> >> + return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
> >> +
> >> + ret = jh7110_vout_top_crg_init(priv, top);
> >> + if (ret)
> >> + goto err_clk;
> >> +
> >> + top->base = priv->base;
> >> + dev_set_drvdata(priv->dev, (void *)(&top->base));
> >
> > See comment later about setting this to 'top' instead. Casting away
> > iomem markings is not good hygiene.
>
> JH7110 resets as the auxiliary device of clocks use the same iomem as the clocks
> and the iomem will be got by dev_get_drvdata() in the 7110 reset drivers when registering reset.
> So I follow the basic 7110 reset driver and also set the iomem not top_crg struct.
Oh I totally missed that this is how it's been done for the other
starfive driver. It's still not good hygiene to stash the iomem pointer
that way because the iomem marking is lost and has to be recovered. Can
you make a wrapper struct, either for the adev or to pass in struct
device::platform_data?
---8<---
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
index 5ec210644e1d..851b93d0f371 100644
--- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
+++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
@@ -11,6 +11,9 @@
#include <linux/init.h>
#include <linux/io.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <soc/starfive/reset-starfive-jh71x0.h>
#include <dt-bindings/clock/starfive,jh7110-crg.h>
@@ -335,26 +338,32 @@ static void jh7110_reset_unregister_adev(void *_adev)
struct auxiliary_device *adev = _adev;
auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
}
static void jh7110_reset_adev_release(struct device *dev)
{
struct auxiliary_device *adev = to_auxiliary_dev(dev);
+ struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
- auxiliary_device_uninit(adev);
+ kfree(rdev);
}
int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
const char *adev_name,
u32 adev_id)
{
+ struct jh71x0_reset_adev *rdev;
struct auxiliary_device *adev;
int ret;
- adev = devm_kzalloc(priv->dev, sizeof(*adev), GFP_KERNEL);
- if (!adev)
+ rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
+ if (!rdev)
return -ENOMEM;
+ rdev->base = priv->base;
+
+ adev = &rdev->adev;
adev->name = adev_name;
adev->dev.parent = priv->dev;
adev->dev.release = jh7110_reset_adev_release;
diff --git a/drivers/reset/starfive/reset-starfive-jh7110.c b/drivers/reset/starfive/reset-starfive-jh7110.c
index c1b3a490d951..2d26ae95c8cc 100644
--- a/drivers/reset/starfive/reset-starfive-jh7110.c
+++ b/drivers/reset/starfive/reset-starfive-jh7110.c
@@ -7,6 +7,8 @@
#include <linux/auxiliary_bus.h>
+#include <soc/starfive/reset-starfive-jh71x0.h>
+
#include "reset-starfive-jh71x0.h"
#include <dt-bindings/reset/starfive,jh7110-crg.h>
@@ -33,14 +35,15 @@ static int jh7110_reset_probe(struct auxiliary_device *adev,
const struct auxiliary_device_id *id)
{
struct jh7110_reset_info *info = (struct jh7110_reset_info *)(id->driver_data);
- void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
+ struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
+ void __iomem *base = rdev->base;
if (!info || !base)
return -ENODEV;
return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
- *base + info->assert_offset,
- *base + info->status_offset,
+ base + info->assert_offset,
+ base + info->status_offset,
NULL,
info->nr_resets,
NULL);
diff --git a/include/soc/starfive/reset-starfive-jh71x0.h b/include/soc/starfive/reset-starfive-jh71x0.h
new file mode 100644
index 000000000000..47b486ececc5
--- /dev/null
+++ b/include/soc/starfive/reset-starfive-jh71x0.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_STARFIVE_RESET_JH71X0_H
+#define __SOC_STARFIVE_RESET_JH71X0_H
+
+#include <linux/auxiliary_bus.h>
+#include <linux/compiler_types.h>
+#include <linux/container_of.h>
+
+struct jh71x0_reset_adev {
+ void __iomem *base;
+ struct auxiliary_device adev;
+};
+
+#define to_jh71x0_reset_adev(_adev) \
+ container_of((_adev), struct jh71x0_reset_adev, adev)
+
+#endif
On 2023/4/13 12:04, Stephen Boyd wrote:
> Quoting Xingyu Wu (2023-04-11 23:15:26)
>> On 2023/4/12 2:33, Stephen Boyd wrote:
>> > Quoting Xingyu Wu (2023-04-11 06:55:55)
>> >> 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..4c6f5ae198cf
>> >> --- /dev/null
>> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-vout.c
>> >> @@ -0,0 +1,239 @@
>> >> +// 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/platform_device.h>
>> >> +#include <linux/pm_runtime.h>
>> >> +#include <linux/reset.h>
>> >
>> > Include module.h, device.h, and kernel.h for things like ERR_PTR().
>>
>> The local headfile 'clk-starfive-jh71x0.h' from the basic JH71x0 clock driver
>> already includes the device.h.
>> And I found the module.h is included in device/driver.h file and then it is included
>> in the device.h file.
>> The kernel.h is included in the clk.h file.
>> So do I still need to list them?
>
> Yes.
OK, will fix.
>
>>
>> > Probably need to include a reset header as well for reset APIs.
>>
>> The reset APIs like devm_reset_control_get_shared() and reset_control_deassert()
>> come from the reset.h file and I have included it.
>
> Cool, I missed it.
>
>>
>> >
>> >> +
>> >> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> >> +
>> >> +#include "clk-starfive-jh7110.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)
>> >> +
>> >> +/* VOUT domian clocks */
>> >> +struct vout_top_crg {
>> >> + struct clk_bulk_data *top_clks;
>> >> + int top_clks_num;
>> >
>> > size_t?
>>
>> Will modify to 'unsigned int'.
>
> Why not size_t?
OK,I will use size_t.
>
>>
>> >
>> >> + if (ret < 0)
>> >> + return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
>> >> +
>> >> + ret = jh7110_vout_top_crg_init(priv, top);
>> >> + if (ret)
>> >> + goto err_clk;
>> >> +
>> >> + top->base = priv->base;
>> >> + dev_set_drvdata(priv->dev, (void *)(&top->base));
>> >
>> > See comment later about setting this to 'top' instead. Casting away
>> > iomem markings is not good hygiene.
>>
>> JH7110 resets as the auxiliary device of clocks use the same iomem as the clocks
>> and the iomem will be got by dev_get_drvdata() in the 7110 reset drivers when registering reset.
>> So I follow the basic 7110 reset driver and also set the iomem not top_crg struct.
>
> Oh I totally missed that this is how it's been done for the other
> starfive driver. It's still not good hygiene to stash the iomem pointer
> that way because the iomem marking is lost and has to be recovered. Can
> you make a wrapper struct, either for the adev or to pass in struct
> device::platform_data?
>
> ---8<---
> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> index 5ec210644e1d..851b93d0f371 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> @@ -11,6 +11,9 @@
> #include <linux/init.h>
> #include <linux/io.h>
> #include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <soc/starfive/reset-starfive-jh71x0.h>
>
> #include <dt-bindings/clock/starfive,jh7110-crg.h>
>
> @@ -335,26 +338,32 @@ static void jh7110_reset_unregister_adev(void *_adev)
> struct auxiliary_device *adev = _adev;
>
> auxiliary_device_delete(adev);
> + auxiliary_device_uninit(adev);
> }
>
> static void jh7110_reset_adev_release(struct device *dev)
> {
> struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
>
> - auxiliary_device_uninit(adev);
> + kfree(rdev);
> }
>
> int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
> const char *adev_name,
> u32 adev_id)
> {
> + struct jh71x0_reset_adev *rdev;
> struct auxiliary_device *adev;
> int ret;
>
> - adev = devm_kzalloc(priv->dev, sizeof(*adev), GFP_KERNEL);
> - if (!adev)
> + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
Can there use 'devm_kzalloc'? Are you not using this because the struct is public and clock driver
and reset driver both use it. But I think the both clock driver and reset driver are the same
device and can use 'devm_kzalloc'.
> + if (!rdev)
> return -ENOMEM;
>
> + rdev->base = priv->base;
> +
> + adev = &rdev->adev;
> adev->name = adev_name;
> adev->dev.parent = priv->dev;
> adev->dev.release = jh7110_reset_adev_release;
> diff --git a/drivers/reset/starfive/reset-starfive-jh7110.c b/drivers/reset/starfive/reset-starfive-jh7110.c
> index c1b3a490d951..2d26ae95c8cc 100644
> --- a/drivers/reset/starfive/reset-starfive-jh7110.c
> +++ b/drivers/reset/starfive/reset-starfive-jh7110.c
> @@ -7,6 +7,8 @@
>
> #include <linux/auxiliary_bus.h>
>
> +#include <soc/starfive/reset-starfive-jh71x0.h>
> +
> #include "reset-starfive-jh71x0.h"
>
> #include <dt-bindings/reset/starfive,jh7110-crg.h>
> @@ -33,14 +35,15 @@ static int jh7110_reset_probe(struct auxiliary_device *adev,
> const struct auxiliary_device_id *id)
> {
> struct jh7110_reset_info *info = (struct jh7110_reset_info *)(id->driver_data);
> - void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
> + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
> + void __iomem *base = rdev->base;
>
> if (!info || !base)
> return -ENODEV;
>
> return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
> - *base + info->assert_offset,
> - *base + info->status_offset,
> + base + info->assert_offset,
> + base + info->status_offset,
> NULL,
> info->nr_resets,
> NULL);
> diff --git a/include/soc/starfive/reset-starfive-jh71x0.h b/include/soc/starfive/reset-starfive-jh71x0.h
> new file mode 100644
> index 000000000000..47b486ececc5
> --- /dev/null
> +++ b/include/soc/starfive/reset-starfive-jh71x0.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_STARFIVE_RESET_JH71X0_H
> +#define __SOC_STARFIVE_RESET_JH71X0_H
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/compiler_types.h>
> +#include <linux/container_of.h>
> +
> +struct jh71x0_reset_adev {
> + void __iomem *base;
> + struct auxiliary_device adev;
> +};
> +
> +#define to_jh71x0_reset_adev(_adev) \
> + container_of((_adev), struct jh71x0_reset_adev, adev)
> +
> +#endif
That's great. It don't need to set iomem to driver_data and I can set the struct data like 'top_crg'
to driver_data in VOUTCRG and ISPCRG driver. I try to modify it in next patchset.
Thanks for your suggestion.
Best regards,
Xingyu Wu
On Wed, Apr 12, 2023 at 09:04:08PM -0700, Stephen Boyd wrote:
> Quoting Xingyu Wu (2023-04-11 23:15:26)
> > On 2023/4/12 2:33, Stephen Boyd wrote:
> > > Quoting Xingyu Wu (2023-04-11 06:55:55)
> > >> + if (ret < 0)
> > >> + return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
> > >> +
> > >> + ret = jh7110_vout_top_crg_init(priv, top);
> > >> + if (ret)
> > >> + goto err_clk;
> > >> +
> > >> + top->base = priv->base;
> > >> + dev_set_drvdata(priv->dev, (void *)(&top->base));
> > >
> > > See comment later about setting this to 'top' instead. Casting away
> > > iomem markings is not good hygiene.
> >
> > JH7110 resets as the auxiliary device of clocks use the same iomem as the clocks
> > and the iomem will be got by dev_get_drvdata() in the 7110 reset drivers when registering reset.
> > So I follow the basic 7110 reset driver and also set the iomem not top_crg struct.
>
> Oh I totally missed that this is how it's been done for the other
> starfive driver. It's still not good hygiene to stash the iomem pointer
> that way because the iomem marking is lost and has to be recovered. Can
> you make a wrapper struct, either for the adev or to pass in struct
> device::platform_data?
FWIW, this did come up on an earlier version of the other starfive
driver:
https://lore.kernel.org/linux-clk/e0d8f9ba-5bf4-d7dd-5110-20d4196556f9@starfivetech.com/
I probably should've pushed Hal to use a struct, but evidently I didn't
reply to his final message there, so apologies for that!
Quoting Xingyu Wu (2023-04-13 06:31:12)
> On 2023/4/13 12:04, Stephen Boyd wrote:
> > diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> > index 5ec210644e1d..851b93d0f371 100644
> > --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> > +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> > @@ -11,6 +11,9 @@
> > #include <linux/init.h>
> > #include <linux/io.h>
> > #include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include <soc/starfive/reset-starfive-jh71x0.h>
> >
> > #include <dt-bindings/clock/starfive,jh7110-crg.h>
> >
> > @@ -335,26 +338,32 @@ static void jh7110_reset_unregister_adev(void *_adev)
> > struct auxiliary_device *adev = _adev;
> >
> > auxiliary_device_delete(adev);
> > + auxiliary_device_uninit(adev);
> > }
> >
> > static void jh7110_reset_adev_release(struct device *dev)
> > {
> > struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
> >
> > - auxiliary_device_uninit(adev);
> > + kfree(rdev);
> > }
> >
> > int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
> > const char *adev_name,
> > u32 adev_id)
> > {
> > + struct jh71x0_reset_adev *rdev;
> > struct auxiliary_device *adev;
> > int ret;
> >
> > - adev = devm_kzalloc(priv->dev, sizeof(*adev), GFP_KERNEL);
> > - if (!adev)
> > + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>
> Can there use 'devm_kzalloc'? Are you not using this because the struct is public and clock driver
> and reset driver both use it. But I think the both clock driver and reset driver are the same
> device and can use 'devm_kzalloc'.
No. The release function for the auxiliary_device is supposed to free
the memory. It shouldn't be tied to the lifetime of anything like the
lifetime of the clk driver being bound.
On 2023/4/14 2:38, Stephen Boyd wrote:
> Quoting Xingyu Wu (2023-04-13 06:31:12)
>> On 2023/4/13 12:04, Stephen Boyd wrote:
>> > diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> > index 5ec210644e1d..851b93d0f371 100644
>> > --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> > +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> > @@ -11,6 +11,9 @@
>> > #include <linux/init.h>
>> > #include <linux/io.h>
>> > #include <linux/platform_device.h>
>> > +#include <linux/slab.h>
>> > +
>> > +#include <soc/starfive/reset-starfive-jh71x0.h>
>> >
>> > #include <dt-bindings/clock/starfive,jh7110-crg.h>
>> >
>> > @@ -335,26 +338,32 @@ static void jh7110_reset_unregister_adev(void *_adev)
>> > struct auxiliary_device *adev = _adev;
>> >
>> > auxiliary_device_delete(adev);
>> > + auxiliary_device_uninit(adev);
>> > }
>> >
>> > static void jh7110_reset_adev_release(struct device *dev)
>> > {
>> > struct auxiliary_device *adev = to_auxiliary_dev(dev);
>> > + struct jh71x0_reset_adev *rdev = to_jh71x0_reset_adev(adev);
>> >
>> > - auxiliary_device_uninit(adev);
>> > + kfree(rdev);
>> > }
>> >
>> > int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv,
>> > const char *adev_name,
>> > u32 adev_id)
>> > {
>> > + struct jh71x0_reset_adev *rdev;
>> > struct auxiliary_device *adev;
>> > int ret;
>> >
>> > - adev = devm_kzalloc(priv->dev, sizeof(*adev), GFP_KERNEL);
>> > - if (!adev)
>> > + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>>
>> Can there use 'devm_kzalloc'? Are you not using this because the struct is public and clock driver
>> and reset driver both use it. But I think the both clock driver and reset driver are the same
>> device and can use 'devm_kzalloc'.
>
> No. The release function for the auxiliary_device is supposed to free
> the memory. It shouldn't be tied to the lifetime of anything like the
> lifetime of the clk driver being bound.
Get it. Thanks.
Best regards,
Xingyu Wu
@@ -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 m if ARCH_STARFIVE
+ help
+ Say yes here to support the Video-Output clock controller
+ on the StarFive JH7110 SoC.
@@ -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
new file mode 100644
@@ -0,0 +1,239 @@
+// 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/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+#include <dt-bindings/clock/starfive,jh7110-crg.h>
+
+#include "clk-starfive-jh7110.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)
+
+/* VOUT domian clocks */
+struct vout_top_crg {
+ struct clk_bulk_data *top_clks;
+ int top_clks_num;
+ void __iomem *base;
+};
+
+static struct clk_bulk_data jh7110_vout_top_clks[] = {
+ { .id = "vout_src" },
+ { .id = "vout_top_ahb" }
+};
+
+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),
+};
+
+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_init(struct jh71x0_clk_priv *priv, struct vout_top_crg *top)
+{
+ struct reset_control *top_rst;
+ 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)
+ return dev_err_probe(priv->dev, ret, "failed to get top clocks\n");
+
+ /* The reset should be shared and other Vout modules will use its. */
+ top_rst = devm_reset_control_get_shared(priv->dev, NULL);
+ if (IS_ERR(top_rst))
+ return dev_err_probe(priv->dev, PTR_ERR(top_rst), "failed to get top reset\n");
+
+ ret = clk_bulk_prepare_enable(top->top_clks_num, top->top_clks);
+ if (ret)
+ return dev_err_probe(priv->dev, ret, "failed to enable top clocks\n");
+
+ return reset_control_deassert(top_rst);
+}
+
+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);
+
+ pm_runtime_enable(priv->dev);
+ ret = pm_runtime_get_sync(priv->dev);
+ if (ret < 0)
+ return dev_err_probe(priv->dev, ret, "failed to turn on power\n");
+
+ ret = jh7110_vout_top_crg_init(priv, top);
+ if (ret)
+ goto err_clk;
+
+ top->base = priv->base;
+ dev_set_drvdata(priv->dev, (void *)(&top->base));
+
+ 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;
+ const char *fw_name[JH7110_VOUTCLK_EXT_END - JH7110_VOUTCLK_END] = {
+ "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, "rst-vout", 4);
+ if (ret)
+ goto err_exit;
+
+ return 0;
+
+err_exit:
+ clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
+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);
+
+ clk_bulk_disable_unprepare(top->top_clks_num, top->top_clks);
+ 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 <xingyu.wu@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JH7110 Video-Output clock driver");
+MODULE_LICENSE("GPL");