| Message ID | 20220420130527.23200-3-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Cleanup MediaTek clk reset drivers and support MT8192/MT8195 | expand |
On Wed, Apr 20, 2022 at 9:05 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote: > > There are two version for clock reset register control of MediaTek SoCs. > The reset operations before MT8183 can use simple reset to cover. I would go slightly into more detail, i.e. The old hardware is one bit per reset control, and does not have separate registers for bit set, clear and read-back operations. This matches the scheme supported by the simple reset driver. ... > Therefore, we replace mtk_reset_ops with reset_simple_ops. ... to remove redundant code. The "why" is more important than "what" in commit logs. "What" you did is already visible in the diff. > In addition, we also rename mtk_register_reset_controller to > mtk_register_reset_controller_simple. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/clk/mediatek/Kconfig | 1 + > drivers/clk/mediatek/clk-mt2701-eth.c | 2 +- > drivers/clk/mediatek/clk-mt2701-g3d.c | 2 +- > drivers/clk/mediatek/clk-mt2701-hif.c | 2 +- > drivers/clk/mediatek/clk-mt2701.c | 4 +-- > drivers/clk/mediatek/clk-mt2712.c | 4 +-- > drivers/clk/mediatek/clk-mt7622-eth.c | 2 +- > drivers/clk/mediatek/clk-mt7622-hif.c | 4 +-- > drivers/clk/mediatek/clk-mt7622.c | 4 +-- > drivers/clk/mediatek/clk-mt7629-eth.c | 2 +- > drivers/clk/mediatek/clk-mt7629-hif.c | 4 +-- > drivers/clk/mediatek/clk-mt8135.c | 4 +-- > drivers/clk/mediatek/clk-mt8173.c | 4 +-- > drivers/clk/mediatek/clk-mtk.h | 6 ++-- > drivers/clk/mediatek/reset.c | 43 +++------------------------ > 15 files changed, 27 insertions(+), 61 deletions(-) [...] > void mtk_register_reset_controller_set_clr(struct device_node *np, > unsigned int num_regs, int regofs) > { > mtk_register_reset_controller_common(np, num_regs, regofs, > - &mtk_reset_ops_set_clr); > + &mtk_reset_ops_set_clr); This change is unrelated and should not be included. ChenYu > } > > MODULE_LICENSE("GPL"); > -- > 2.18.0 >
Il 20/04/22 15:05, Rex-BC Chen ha scritto: > There are two version for clock reset register control of MediaTek SoCs. > The reset operations before MT8183 can use simple reset to cover. > Therefore, we replace mtk_reset_ops with reset_simple_ops. > In addition, we also rename mtk_register_reset_controller to > mtk_register_reset_controller_simple. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Hello Rex, have you tested this? It won't work. reset-simple is not using regmap, and it requires you to pass a struct reset_simple_data through drvdata. Besides, I like that we are using regmap, while reset_simple simply uses readl/writel... so if you want to use that driver, which is good to reduce duplication, you should also implement support for regmap in the form of reset_simple_regmap_ops. Regards, Angelo
Hello Cehn-Yu, On Thu, 2022-04-21 at 15:52 +0800, Chen-Yu Tsai wrote: > On Wed, Apr 20, 2022 at 9:05 PM Rex-BC Chen <rex-bc.chen@mediatek.com > > wrote: > > > > There are two version for clock reset register control of MediaTek > > SoCs. > > The reset operations before MT8183 can use simple reset to cover. > > I would go slightly into more detail, i.e. > > The old hardware is one bit per reset control, and does not have > separate registers for bit set, clear and read-back operations. > This > matches the scheme supported by the simple reset driver. ... > > > Therefore, we replace mtk_reset_ops with reset_simple_ops. > > ... to remove redundant code. > > The "why" is more important than "what" in commit logs. "What" you > did > is already visible in the diff. > Got it, I will modify this in next version. > > In addition, we also rename mtk_register_reset_controller to > > mtk_register_reset_controller_simple. > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/clk/mediatek/Kconfig | 1 + > > drivers/clk/mediatek/clk-mt2701-eth.c | 2 +- > > drivers/clk/mediatek/clk-mt2701-g3d.c | 2 +- > > drivers/clk/mediatek/clk-mt2701-hif.c | 2 +- > > drivers/clk/mediatek/clk-mt2701.c | 4 +-- > > drivers/clk/mediatek/clk-mt2712.c | 4 +-- > > drivers/clk/mediatek/clk-mt7622-eth.c | 2 +- > > drivers/clk/mediatek/clk-mt7622-hif.c | 4 +-- > > drivers/clk/mediatek/clk-mt7622.c | 4 +-- > > drivers/clk/mediatek/clk-mt7629-eth.c | 2 +- > > drivers/clk/mediatek/clk-mt7629-hif.c | 4 +-- > > drivers/clk/mediatek/clk-mt8135.c | 4 +-- > > drivers/clk/mediatek/clk-mt8173.c | 4 +-- > > drivers/clk/mediatek/clk-mtk.h | 6 ++-- > > drivers/clk/mediatek/reset.c | 43 +++------------------ > > ------ > > 15 files changed, 27 insertions(+), 61 deletions(-) > > [...] > > > void mtk_register_reset_controller_set_clr(struct device_node *np, > > unsigned int num_regs, int regofs) > > { > > mtk_register_reset_controller_common(np, num_regs, regofs, > > - &mtk_reset_ops_set_clr); > > + &mtk_reset_ops_set_clr > > ); > > This change is unrelated and should not be included. > > ChenYu > I add a refinement patch in next version, and I will move this to that patch. BRs, Rex > > > } > > > > MODULE_LICENSE("GPL"); > > -- > > 2.18.0 > >
On Thu, 2022-04-21 at 11:08 +0200, AngeloGioacchino Del Regno wrote: > Il 20/04/22 15:05, Rex-BC Chen ha scritto: > > There are two version for clock reset register control of MediaTek > > SoCs. > > The reset operations before MT8183 can use simple reset to cover. > > Therefore, we replace mtk_reset_ops with reset_simple_ops. > > In addition, we also rename mtk_register_reset_controller to > > mtk_register_reset_controller_simple. > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Hello Rex, > have you tested this? It won't work. > > reset-simple is not using regmap, and it requires you to pass a > struct > reset_simple_data through drvdata. > > Besides, I like that we are using regmap, while reset_simple simply > uses > readl/writel... so if you want to use that driver, which is good to > reduce > duplication, you should also implement support for regmap in the form > of > reset_simple_regmap_ops. > > Regards, > Angelo Hello Angelo, Yes, I also notice the error.. The data we used is not reset_simple_data. I will drop using reset_simple_ops and remain original functions in next version. BRs, Rex
diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig index 01ef02c54725..df2cdaa975e4 100644 --- a/drivers/clk/mediatek/Kconfig +++ b/drivers/clk/mediatek/Kconfig @@ -8,6 +8,7 @@ menu "Clock driver for MediaTek SoC" config COMMON_CLK_MEDIATEK tristate select RESET_CONTROLLER + select RESET_SIMPLE help MediaTek SoCs' clock support. diff --git a/drivers/clk/mediatek/clk-mt2701-eth.c b/drivers/clk/mediatek/clk-mt2701-eth.c index 100ff6ca609e..1a6318fbcb32 100644 --- a/drivers/clk/mediatek/clk-mt2701-eth.c +++ b/drivers/clk/mediatek/clk-mt2701-eth.c @@ -58,7 +58,7 @@ static int clk_mt2701_eth_probe(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } diff --git a/drivers/clk/mediatek/clk-mt2701-g3d.c b/drivers/clk/mediatek/clk-mt2701-g3d.c index 1328c112a38f..0cd6b57657b3 100644 --- a/drivers/clk/mediatek/clk-mt2701-g3d.c +++ b/drivers/clk/mediatek/clk-mt2701-g3d.c @@ -52,7 +52,7 @@ static int clk_mt2701_g3dsys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0xc); + mtk_register_reset_controller_simple(node, 1, 0xc); return r; } diff --git a/drivers/clk/mediatek/clk-mt2701-hif.c b/drivers/clk/mediatek/clk-mt2701-hif.c index 61444881c539..883a23bb024d 100644 --- a/drivers/clk/mediatek/clk-mt2701-hif.c +++ b/drivers/clk/mediatek/clk-mt2701-hif.c @@ -57,7 +57,7 @@ static int clk_mt2701_hif_probe(struct platform_device *pdev) return r; } - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return 0; } diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c index 1eb3e4563c3f..3f6508ff8e7f 100644 --- a/drivers/clk/mediatek/clk-mt2701.c +++ b/drivers/clk/mediatek/clk-mt2701.c @@ -785,7 +785,7 @@ static int mtk_infrasys_init(struct platform_device *pdev) if (r) return r; - mtk_register_reset_controller(node, 2, 0x30); + mtk_register_reset_controller_simple(node, 2, 0x30); return 0; } @@ -908,7 +908,7 @@ static int mtk_pericfg_init(struct platform_device *pdev) if (r) return r; - mtk_register_reset_controller(node, 2, 0x0); + mtk_register_reset_controller_simple(node, 2, 0x0); return 0; } diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c index ff72b9ab945b..9b4470ac7be7 100644 --- a/drivers/clk/mediatek/clk-mt2712.c +++ b/drivers/clk/mediatek/clk-mt2712.c @@ -1361,7 +1361,7 @@ static int clk_mt2712_infra_probe(struct platform_device *pdev) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0x30); + mtk_register_reset_controller_simple(node, 2, 0x30); return r; } @@ -1383,7 +1383,7 @@ static int clk_mt2712_peri_probe(struct platform_device *pdev) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0); + mtk_register_reset_controller_simple(node, 2, 0); return r; } diff --git a/drivers/clk/mediatek/clk-mt7622-eth.c b/drivers/clk/mediatek/clk-mt7622-eth.c index c9947dc7ba5a..647bf752a8af 100644 --- a/drivers/clk/mediatek/clk-mt7622-eth.c +++ b/drivers/clk/mediatek/clk-mt7622-eth.c @@ -82,7 +82,7 @@ static int clk_mt7622_ethsys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } diff --git a/drivers/clk/mediatek/clk-mt7622-hif.c b/drivers/clk/mediatek/clk-mt7622-hif.c index 628be0c9f888..1287db1e3cc2 100644 --- a/drivers/clk/mediatek/clk-mt7622-hif.c +++ b/drivers/clk/mediatek/clk-mt7622-hif.c @@ -93,7 +93,7 @@ static int clk_mt7622_ssusbsys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } @@ -115,7 +115,7 @@ static int clk_mt7622_pciesys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } diff --git a/drivers/clk/mediatek/clk-mt7622.c b/drivers/clk/mediatek/clk-mt7622.c index 0e1fb30a1e98..2b744afd9233 100644 --- a/drivers/clk/mediatek/clk-mt7622.c +++ b/drivers/clk/mediatek/clk-mt7622.c @@ -663,7 +663,7 @@ static int mtk_infrasys_init(struct platform_device *pdev) if (r) return r; - mtk_register_reset_controller(node, 1, 0x30); + mtk_register_reset_controller_simple(node, 1, 0x30); return 0; } @@ -714,7 +714,7 @@ static int mtk_pericfg_init(struct platform_device *pdev) clk_prepare_enable(clk_data->clks[CLK_PERI_UART0_PD]); - mtk_register_reset_controller(node, 2, 0x0); + mtk_register_reset_controller_simple(node, 2, 0x0); return 0; } diff --git a/drivers/clk/mediatek/clk-mt7629-eth.c b/drivers/clk/mediatek/clk-mt7629-eth.c index 88279d0ea1a7..0fb5780ae048 100644 --- a/drivers/clk/mediatek/clk-mt7629-eth.c +++ b/drivers/clk/mediatek/clk-mt7629-eth.c @@ -92,7 +92,7 @@ static int clk_mt7629_ethsys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } diff --git a/drivers/clk/mediatek/clk-mt7629-hif.c b/drivers/clk/mediatek/clk-mt7629-hif.c index 5c5b37207afb..6f7d013814ac 100644 --- a/drivers/clk/mediatek/clk-mt7629-hif.c +++ b/drivers/clk/mediatek/clk-mt7629-hif.c @@ -88,7 +88,7 @@ static int clk_mt7629_ssusbsys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } @@ -110,7 +110,7 @@ static int clk_mt7629_pciesys_init(struct platform_device *pdev) "could not register clock provider: %s: %d\n", pdev->name, r); - mtk_register_reset_controller(node, 1, 0x34); + mtk_register_reset_controller_simple(node, 1, 0x34); return r; } diff --git a/drivers/clk/mediatek/clk-mt8135.c b/drivers/clk/mediatek/clk-mt8135.c index 09ad272d51f1..476c6fb5fc5d 100644 --- a/drivers/clk/mediatek/clk-mt8135.c +++ b/drivers/clk/mediatek/clk-mt8135.c @@ -559,7 +559,7 @@ static void __init mtk_infrasys_init(struct device_node *node) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0x30); + mtk_register_reset_controller_simple(node, 2, 0x30); } CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8135-infracfg", mtk_infrasys_init); @@ -587,7 +587,7 @@ static void __init mtk_pericfg_init(struct device_node *node) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0); + mtk_register_reset_controller_simple(node, 2, 0); } CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8135-pericfg", mtk_pericfg_init); diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c index 46b7655feeaa..92beb45de8a0 100644 --- a/drivers/clk/mediatek/clk-mt8173.c +++ b/drivers/clk/mediatek/clk-mt8173.c @@ -882,7 +882,7 @@ static void __init mtk_infrasys_init(struct device_node *node) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0x30); + mtk_register_reset_controller_simple(node, 2, 0x30); } CLK_OF_DECLARE(mtk_infrasys, "mediatek,mt8173-infracfg", mtk_infrasys_init); @@ -910,7 +910,7 @@ static void __init mtk_pericfg_init(struct device_node *node) pr_err("%s(): could not register clock provider: %d\n", __func__, r); - mtk_register_reset_controller(node, 2, 0); + mtk_register_reset_controller_simple(node, 2, 0); } CLK_OF_DECLARE(mtk_pericfg, "mediatek,mt8173-pericfg", mtk_pericfg_init); diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h index bf6565aa7319..f767c9585d8c 100644 --- a/drivers/clk/mediatek/clk-mtk.h +++ b/drivers/clk/mediatek/clk-mtk.h @@ -190,11 +190,11 @@ void mtk_free_clk_data(struct clk_onecell_data *clk_data); struct clk *mtk_clk_register_ref2usb_tx(const char *name, const char *parent_name, void __iomem *reg); -void mtk_register_reset_controller(struct device_node *np, - unsigned int num_regs, int regofs); +void mtk_register_reset_controller_simple(struct device_node *np, + unsigned int num_regs, int regofs); void mtk_register_reset_controller_set_clr(struct device_node *np, - unsigned int num_regs, int regofs); + unsigned int num_regs, int regofs); struct mtk_clk_desc { const struct mtk_gate *clks; diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c index 834d26e9bdfd..9110d0b4229f 100644 --- a/drivers/clk/mediatek/reset.c +++ b/drivers/clk/mediatek/reset.c @@ -9,6 +9,7 @@ #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/reset-controller.h> +#include <linux/reset/reset-simple.h> #include <linux/slab.h> #include "clk-mtk.h" @@ -37,36 +38,6 @@ static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev, return regmap_write(data->regmap, reg, BIT(id % 32)); } -static int mtk_reset_assert(struct reset_controller_dev *rcdev, - unsigned long id) -{ - struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev); - - return regmap_update_bits(data->regmap, data->regofs + ((id / 32) << 2), - BIT(id % 32), ~0); -} - -static int mtk_reset_deassert(struct reset_controller_dev *rcdev, - unsigned long id) -{ - struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev); - - return regmap_update_bits(data->regmap, data->regofs + ((id / 32) << 2), - BIT(id % 32), 0); -} - -static int mtk_reset(struct reset_controller_dev *rcdev, - unsigned long id) -{ - int ret; - - ret = mtk_reset_assert(rcdev, id); - if (ret) - return ret; - - return mtk_reset_deassert(rcdev, id); -} - static int mtk_reset_set_clr(struct reset_controller_dev *rcdev, unsigned long id) { @@ -78,12 +49,6 @@ static int mtk_reset_set_clr(struct reset_controller_dev *rcdev, return mtk_reset_deassert_set_clr(rcdev, id); } -static const struct reset_control_ops mtk_reset_ops = { - .assert = mtk_reset_assert, - .deassert = mtk_reset_deassert, - .reset = mtk_reset, -}; - static const struct reset_control_ops mtk_reset_ops_set_clr = { .assert = mtk_reset_assert_set_clr, .deassert = mtk_reset_deassert_set_clr, @@ -123,18 +88,18 @@ static void mtk_register_reset_controller_common(struct device_node *np, } } -void mtk_register_reset_controller(struct device_node *np, +void mtk_register_reset_controller_simple(struct device_node *np, unsigned int num_regs, int regofs) { mtk_register_reset_controller_common(np, num_regs, regofs, - &mtk_reset_ops); + &reset_simple_ops); } void mtk_register_reset_controller_set_clr(struct device_node *np, unsigned int num_regs, int regofs) { mtk_register_reset_controller_common(np, num_regs, regofs, - &mtk_reset_ops_set_clr); + &mtk_reset_ops_set_clr); } MODULE_LICENSE("GPL");
There are two version for clock reset register control of MediaTek SoCs. The reset operations before MT8183 can use simple reset to cover. Therefore, we replace mtk_reset_ops with reset_simple_ops. In addition, we also rename mtk_register_reset_controller to mtk_register_reset_controller_simple. Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> --- drivers/clk/mediatek/Kconfig | 1 + drivers/clk/mediatek/clk-mt2701-eth.c | 2 +- drivers/clk/mediatek/clk-mt2701-g3d.c | 2 +- drivers/clk/mediatek/clk-mt2701-hif.c | 2 +- drivers/clk/mediatek/clk-mt2701.c | 4 +-- drivers/clk/mediatek/clk-mt2712.c | 4 +-- drivers/clk/mediatek/clk-mt7622-eth.c | 2 +- drivers/clk/mediatek/clk-mt7622-hif.c | 4 +-- drivers/clk/mediatek/clk-mt7622.c | 4 +-- drivers/clk/mediatek/clk-mt7629-eth.c | 2 +- drivers/clk/mediatek/clk-mt7629-hif.c | 4 +-- drivers/clk/mediatek/clk-mt8135.c | 4 +-- drivers/clk/mediatek/clk-mt8173.c | 4 +-- drivers/clk/mediatek/clk-mtk.h | 6 ++-- drivers/clk/mediatek/reset.c | 43 +++------------------------ 15 files changed, 27 insertions(+), 61 deletions(-)