| Message ID | 20241120063701.8194-3-friday.yang@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add SMI clamp in MediaTek SMI driver | expand |
On 20/11/2024 07:36, Friday Yang wrote: > In order to avoid handling glitch signal when MTCMOS on/off, SMI need > clamp and reset operation. Parse power reset settings for LARBs which > need to reset. Register genpd callback for SMI LARBs and apply reset > operations in the callback. > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > --- > drivers/memory/mtk-smi.c | 175 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 171 insertions(+), 4 deletions(-) > > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c > index 2bc034dff691..c7119f655350 100644 > --- a/drivers/memory/mtk-smi.c > +++ b/drivers/memory/mtk-smi.c > @@ -10,15 +10,21 @@ > #include <linux/err.h> > #include <linux/io.h> > #include <linux/iopoll.h> > +#include <linux/mfd/syscon.h> Where do you use it? > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_domain.h> Where do you use it? > #include <linux/pm_runtime.h> > +#include <linux/regmap.h> Where do you use it? > +#include <linux/reset.h> > +#include <linux/reset-controller.h> > #include <linux/soc/mediatek/mtk_sip_svc.h> > #include <soc/mediatek/smi.h> > #include <dt-bindings/memory/mt2701-larb-port.h> > #include <dt-bindings/memory/mtk-memory-port.h> > +#include <dt-bindings/reset/mt8188-resets.h> > ... > > +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb *larb) > +{ > + struct device *dev = larb->dev; > + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; > + struct device_node *smi_node; > + struct of_phandle_args args; > + int ret, index; > + > + /* Only SMI LARBs located in camera and image subsys need to Use Linux coding style. > + * apply clamp and reset operation, others can be skipped. > + */ > + ret = of_parse_phandle_with_fixed_args(dev->of_node, > + "resets", 1, 0, &args); NAK > + if (ret) > + return 0; > + > + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); > + if (!smi_node) > + return -EINVAL; > + > + index = args.args[0]; That's reset, not clamp port. NAK. > + larb->sub_comm_inport = larb_gen->clamp_port[index]; > + larb->sub_comm_syscon = device_node_to_regmap(smi_node); > + of_node_put(smi_node); > + > + if (IS_ERR(larb->sub_comm_syscon) || > + larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) { > + larb->sub_comm_syscon = NULL; > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb *larb) > +{ > + struct device *dev = larb->dev; > + int ret; > + > + /* Only SMI LARBs located in camera and image subsys need to Use Linux coding style. This applies to all your patches and all places in this patch. > + * apply clamp and reset operation, others can be skipped. > + */ > + if (!of_find_property(dev->of_node, "resets", NULL)) That's not how you use reset framework API. Use proper optional API. > + return 0; > + > + larb->rst_con = devm_reset_control_get(dev, "larb"); > + if (IS_ERR(larb->rst_con)) > + return dev_err_probe(dev, PTR_ERR(larb->rst_con), > + "Can not get larb reset controller\n"); > + > + larb->nb.notifier_call = mtk_smi_genpd_callback; > + ret = dev_pm_genpd_add_notifier(dev, &larb->nb); > + if (ret) { > + dev_err(dev, "Failed to add genpd callback %d\n", ret); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int mtk_smi_larb_probe(struct platform_device *pdev) > { > struct mtk_smi_larb *larb; > @@ -538,6 +685,7 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) > if (!larb) > return -ENOMEM; > > + larb->dev = dev; > larb->larb_gen = of_device_get_match_data(dev); > larb->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(larb->base)) > @@ -554,15 +702,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > - pm_runtime_enable(dev); > + /* find sub common to clamp larb for ISP software reset */ > + ret = mtk_smi_larb_parse_clamp_info(larb); > + if (ret) { > + dev_err(dev, "Failed to get clamp setting for larb\n"); > + goto err_link_remove; > + } > + > + ret = mtk_smi_larb_parse_reset_info(larb); > + if (ret) { > + dev_err(dev, "Failed to get power setting for larb\n"); > + goto err_link_remove; > + } > + > platform_set_drvdata(pdev, larb); > ret = component_add(dev, &mtk_smi_larb_component_ops); > if (ret) > - goto err_pm_disable; > + goto err_link_remove; > + > + pm_runtime_enable(dev); > + > return 0; > > -err_pm_disable: > - pm_runtime_disable(dev); > +err_link_remove: > device_link_remove(dev, larb->smi_common_dev); > return ret; > } > @@ -686,6 +848,10 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = { > .init = mtk_smi_common_mt8195_init, > }; > > +static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 = { > + .type = MTK_SMI_GEN2_SUB_COMM, > +}; > + > static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = { > .type = MTK_SMI_GEN2, > .has_gals = true, > @@ -729,6 +895,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = { > {.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186}, > {.compatible = "mediatek,mt8188-smi-common-vdo", .data = &mtk_smi_common_mt8188_vdo}, > {.compatible = "mediatek,mt8188-smi-common-vpp", .data = &mtk_smi_common_mt8188_vpp}, > + {.compatible = "mediatek,mt8188-smi-sub-common", .data = &mtk_smi_sub_common_mt8188}, > {.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192}, > {.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo}, > {.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp}, Best regards, Krzysztof
Il 20/11/24 07:36, Friday Yang ha scritto: > In order to avoid handling glitch signal when MTCMOS on/off, SMI need > clamp and reset operation. Parse power reset settings for LARBs which > need to reset. Register genpd callback for SMI LARBs and apply reset > operations in the callback. > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > --- > drivers/memory/mtk-smi.c | 175 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 171 insertions(+), 4 deletions(-) > > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c > index 2bc034dff691..c7119f655350 100644 > --- a/drivers/memory/mtk-smi.c > +++ b/drivers/memory/mtk-smi.c > @@ -10,15 +10,21 @@ > #include <linux/err.h> > #include <linux/io.h> > #include <linux/iopoll.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > +#include <linux/reset-controller.h> > #include <linux/soc/mediatek/mtk_sip_svc.h> > #include <soc/mediatek/smi.h> > #include <dt-bindings/memory/mt2701-larb-port.h> > #include <dt-bindings/memory/mtk-memory-port.h> > +#include <dt-bindings/reset/mt8188-resets.h> > > /* SMI COMMON */ > #define SMI_L1LEN 0x100 > @@ -36,6 +42,13 @@ > #define SMI_DCM 0x300 > #define SMI_DUMMY 0x444 > > +#define SMI_COMMON_CLAMP_EN 0x3c0 > +#define SMI_COMMON_CLAMP_EN_SET 0x3c4 > +#define SMI_COMMON_CLAMP_EN_CLR 0x3c8 > +#define SMI_COMMON_CLAMP_MASK(inport) BIT(inport) > + > +#define SMI_SUB_COMM_INPORT_NR (8) > + > /* SMI LARB */ > #define SMI_LARB_SLP_CON 0xc > #define SLP_PROT_EN BIT(0) > @@ -134,6 +147,7 @@ struct mtk_smi_larb_gen { > unsigned int larb_direct_to_common_mask; > unsigned int flags_general; > const u8 (*ostd)[SMI_LARB_PORT_NR_MAX]; > + const u8 *clamp_port; > }; > > struct mtk_smi { > @@ -150,6 +164,7 @@ struct mtk_smi { > }; > > struct mtk_smi_larb { /* larb: local arbiter */ > + struct device *dev; > struct mtk_smi smi; > void __iomem *base; > struct device *smi_common_dev; /* common or sub-common dev */ > @@ -157,6 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter */ > int larbid; > u32 *mmu; > unsigned char *bank; > + struct regmap *sub_comm_syscon; > + u8 sub_comm_inport; > + struct notifier_block nb; > + struct reset_control *rst_con; > }; > > static int > @@ -377,6 +396,19 @@ static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = { > [28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,}, > }; > > +static const u8 mtk_smi_larb_clamp_port_mt8188[] = { You can just set these to BIT(x) directly, like: [MT8188_SMI_RST_LARB10] = BIT(0), so that you can check if there's actually a supported/valid clamp port in function mtk_smi_larb_parse_clamp_info() - check below for comments in that function. Note that you are declaring SMI_SUB_COMM_INPORT_NR = 8 and this means that there are a maximum of 8 ports, with each port having its own BIT, starting from BIT 0 and ending at BIT 7 (so [7:0]). This means that we have a maximum of 8 bits here, so even if you assign BIT(x) it all still fits in a u8! :-) > + [MT8188_SMI_RST_LARB10] = 1, > + [MT8188_SMI_RST_LARB11A] = 2, > + [MT8188_SMI_RST_LARB11C] = 3, > + [MT8188_SMI_RST_LARB12] = 0, > + [MT8188_SMI_RST_LARB11B] = 2, > + [MT8188_SMI_RST_LARB15] = 1, > + [MT8188_SMI_RST_LARB16B] = 2, > + [MT8188_SMI_RST_LARB17B] = 3, > + [MT8188_SMI_RST_LARB16A] = 2, > + [MT8188_SMI_RST_LARB17A] = 3, > +}; > + > static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = { > .port_in_larb = { > LARB0_PORT_OFFSET, LARB1_PORT_OFFSET, > @@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = { > .flags_general = MTK_SMI_FLAG_THRT_UPDATE | MTK_SMI_FLAG_SW_FLAG | > MTK_SMI_FLAG_SLEEP_CTL | MTK_SMI_FLAG_CFG_PORT_SEC_CTL, > .ostd = mtk_smi_larb_mt8188_ostd, > + .clamp_port = mtk_smi_larb_clamp_port_mt8188, > }; > > static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = { > @@ -472,6 +505,60 @@ static void mtk_smi_larb_sleep_ctrl_disable(struct mtk_smi_larb *larb) > writel_relaxed(0, larb->base + SMI_LARB_SLP_CON); > } > > +static int mtk_smi_larb_clamp_protect_enable(struct device *dev) > +{ > + struct mtk_smi_larb *larb = dev_get_drvdata(dev); > + int ret; > + > + /* sub_comm_syscon could be NULL if larb directly linked to SMI common */ > + if (!larb->sub_comm_syscon) > + return -EINVAL; > + > + ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_SET, > + SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport)); > + if (ret) > + dev_err(dev, "Unable to enable clamp, inport %d, ret %d\n", > + larb->sub_comm_inport, ret); > + > + return ret; > +} > + > +static int mtk_smi_larb_clamp_protect_disble(struct device *dev) typo: mtk_smi_larb_clamp_protect_disable > +{ > + struct mtk_smi_larb *larb = dev_get_drvdata(dev); > + int ret; > + > + /* sub_comm_syscon could be NULL if larb directly linked to SMI common */ > + if (!larb->sub_comm_syscon) > + return -EINVAL; > + > + ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_CLR, > + SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport)); > + if (ret) > + dev_err(dev, "Unable to disable clamp, inport %d, ret %d\n", > + larb->sub_comm_inport, ret); > + > + return ret; > +} ...but anyway, I would rather do it like that: static int mtk_smi_larb_clamp_protect_enable(struct device *dev, bool enable) { struct mtk_smi_larb *larb = dev_get_drvdata(dev); u32 reg; int ret; /* sub_comm_syscon could be NULL if larb directly linked to SMI common */ if (!larb->sub_comm_syscon) return -EINVAL; reg = enable ? SMI_COMMON_CLAMP_EN_SET : SMI_COMMON_CLAMP_EN_CLR; ret = regmap_write(larb->sub_comm_syscon, reg, SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport)); if (ret) { dev_err(dev, "Unable to %s clamp for input port %d: %d\n", enable ? "enable" : "disable", larb->sub_comm_inport, ret); return ret; } return 0; } ...and then call it like ret = mtk_smi_larb_clamp_protect_enable(dev, true); or ret = mtk_smi_larb_clamp_protect_enable(dev, false); > + > +static int mtk_smi_genpd_callback(struct notifier_block *nb, > + unsigned long flags, void *data) > +{ > + struct mtk_smi_larb *larb = container_of(nb, struct mtk_smi_larb, nb); > + struct device *dev = larb->dev; > + > + if (flags == GENPD_NOTIFY_PRE_ON || flags == GENPD_NOTIFY_PRE_OFF) { > + /* disable related SMI sub-common port */ > + mtk_smi_larb_clamp_protect_enable(dev); > + } else if (flags == GENPD_NOTIFY_ON) { > + /* enable related SMI sub-common port */ > + reset_control_reset(larb->rst_con); > + mtk_smi_larb_clamp_protect_disble(dev); > + } > + > + return NOTIFY_OK; > +} > + > static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev) > { > struct platform_device *smi_com_pdev; > @@ -528,6 +615,66 @@ static int mtk_smi_dts_clk_init(struct device *dev, struct mtk_smi *smi, > return ret; > } > > +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb *larb) > +{ > + struct device *dev = larb->dev; > + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; > + struct device_node *smi_node; > + struct of_phandle_args args; > + int ret, index; > + > + /* Only SMI LARBs located in camera and image subsys need to > + * apply clamp and reset operation, others can be skipped. > + */ > + ret = of_parse_phandle_with_fixed_args(dev->of_node, > + "resets", 1, 0, &args); > + if (ret) > + return 0; > + > + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); > + if (!smi_node) > + return -EINVAL; > + > + index = args.args[0]; > + larb->sub_comm_inport = larb_gen->clamp_port[index]; > + larb->sub_comm_syscon = device_node_to_regmap(smi_node); > + of_node_put(smi_node); If you declare BIT(x) as clamp_ports, or if your clamp_ports start from 1, anything that is not more than 0 is something that was not declared, and this means that you can then error check: if (!larb->sub_comm_inport) return dev_err_probe(dev, -EINVAL, "Unknown clamp port for larb %d\n", index); > + > + if (IS_ERR(larb->sub_comm_syscon) || > + larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) { > + larb->sub_comm_syscon = NULL; > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb *larb) > +{ > + struct device *dev = larb->dev; > + int ret; > + > + /* Only SMI LARBs located in camera and image subsys need to > + * apply clamp and reset operation, others can be skipped. > + */ > + if (!of_find_property(dev->of_node, "resets", NULL)) > + return 0; You don't have to manually check whether 'resets' exists or not - that's already done (devm_)reset_control_get in a way, as that will return -ENOENT if there is no 'reset-names' property. Check the comments down there.... > + > + larb->rst_con = devm_reset_control_get(dev, "larb"); > + if (IS_ERR(larb->rst_con)) > + return dev_err_probe(dev, PTR_ERR(larb->rst_con), > + "Can not get larb reset controller\n"); > + > + larb->nb.notifier_call = mtk_smi_genpd_callback; > + ret = dev_pm_genpd_add_notifier(dev, &larb->nb); > + if (ret) { > + dev_err(dev, "Failed to add genpd callback %d\n", ret); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int mtk_smi_larb_probe(struct platform_device *pdev) > { > struct mtk_smi_larb *larb; > @@ -538,6 +685,7 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) > if (!larb) > return -ENOMEM; > > + larb->dev = dev; > larb->larb_gen = of_device_get_match_data(dev); > larb->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(larb->base)) > @@ -554,15 +702,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > - pm_runtime_enable(dev); ret = mtk_smi_larb_parse_reset_optional(larb); Choose between: if (ret == 0) { ret = mtk_smi_larb_parse_smi_clamp(larb); if (ret) { dev_err_probe(dev, ret, "Failed to get SMI clamp\n"); goto err_link_remove; } } else if (ret != -ENOENT) { dev_err_probe(dev, ret, "Cannot get larb resets\n"); goto err_link_remove; } else { /* * Only SMI LARBs located in camera and image subsys need to apply * clamp and reset operation. For the others, resets are optional. */ ret = 0; } and... if (ret && ret != -ENOENT) { dev_err_probe ..... } else if (ret) { /* only smi larbs .... */ ret = 0; } else { ret = mtk_smi_larb_parse_smi_clamp ..... } whatever you like the most. > + /* find sub common to clamp larb for ISP software reset */ > + ret = mtk_smi_larb_parse_clamp_info(larb); > + if (ret) { > + dev_err(dev, "Failed to get clamp setting for larb\n"); > + goto err_link_remove; > + } > + > + ret = mtk_smi_larb_parse_reset_info(larb); > + if (ret) { > + dev_err(dev, "Failed to get power setting for larb\n"); > + goto err_link_remove; > + } > + > platform_set_drvdata(pdev, larb); > ret = component_add(dev, &mtk_smi_larb_component_ops); > if (ret) > - goto err_pm_disable; > + goto err_link_remove; > + > + pm_runtime_enable(dev); > + > return 0; > > -err_pm_disable: > - pm_runtime_disable(dev); > +err_link_remove: > device_link_remove(dev, larb->smi_common_dev); > return ret; > } > @@ -686,6 +848,10 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = { > .init = mtk_smi_common_mt8195_init, > }; > > +static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 = { > + .type = MTK_SMI_GEN2_SUB_COMM, ...no gals in MT8188?! Cheers, Angelo > +}; > + > static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = { > .type = MTK_SMI_GEN2, > .has_gals = true, > @@ -729,6 +895,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = { > {.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186}, > {.compatible = "mediatek,mt8188-smi-common-vdo", .data = &mtk_smi_common_mt8188_vdo}, > {.compatible = "mediatek,mt8188-smi-common-vpp", .data = &mtk_smi_common_mt8188_vpp}, > + {.compatible = "mediatek,mt8188-smi-sub-common", .data = &mtk_smi_sub_common_mt8188}, > {.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192}, > {.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo}, > {.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp},
On Wed, 2024-11-20 at 12:49 +0100, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 20/11/24 07:36, Friday Yang ha scritto: > > In order to avoid handling glitch signal when MTCMOS on/off, SMI > > need > > clamp and reset operation. Parse power reset settings for LARBs > > which > > need to reset. Register genpd callback for SMI LARBs and apply > > reset > > operations in the callback. > > > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > > --- > > drivers/memory/mtk-smi.c | 175 > > ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 171 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c > > index 2bc034dff691..c7119f655350 100644 > > --- a/drivers/memory/mtk-smi.c > > +++ b/drivers/memory/mtk-smi.c > > @@ -10,15 +10,21 @@ > > #include <linux/err.h> > > #include <linux/io.h> > > #include <linux/iopoll.h> > > +#include <linux/mfd/syscon.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > +#include <linux/pm_domain.h> > > #include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <linux/reset.h> > > +#include <linux/reset-controller.h> > > #include <linux/soc/mediatek/mtk_sip_svc.h> > > #include <soc/mediatek/smi.h> > > #include <dt-bindings/memory/mt2701-larb-port.h> > > #include <dt-bindings/memory/mtk-memory-port.h> > > +#include <dt-bindings/reset/mt8188-resets.h> > > > > /* SMI COMMON */ > > #define SMI_L1LEN 0x100 > > @@ -36,6 +42,13 @@ > > #define SMI_DCM 0x300 > > #define SMI_DUMMY 0x444 > > > > +#define SMI_COMMON_CLAMP_EN 0x3c0 > > +#define SMI_COMMON_CLAMP_EN_SET 0x3c4 > > +#define SMI_COMMON_CLAMP_EN_CLR 0x3c8 > > +#define SMI_COMMON_CLAMP_MASK(inport) BIT(inport) > > + > > +#define SMI_SUB_COMM_INPORT_NR (8) > > + > > /* SMI LARB */ > > #define SMI_LARB_SLP_CON 0xc > > #define SLP_PROT_EN BIT(0) > > @@ -134,6 +147,7 @@ struct mtk_smi_larb_gen { > > unsigned int larb_direct_to_common_mask; > > unsigned int flags_general; > > const > > u8 (*ostd)[SMI_LARB_PORT_NR_MAX]; > > + const u8 *clamp_port; > > }; > > > > struct mtk_smi { > > @@ -150,6 +164,7 @@ struct mtk_smi { > > }; > > > > struct mtk_smi_larb { /* larb: local arbiter */ > > + struct device *dev; > > struct mtk_smi smi; > > void __iomem *base; > > struct device *smi_common_dev; /* common or > > sub-common dev */ > > @@ -157,6 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter > > */ > > int larbid; > > u32 *mmu; > > unsigned char *bank; > > + struct regmap *sub_comm_syscon; > > + u8 sub_comm_inport; > > + struct notifier_block nb; > > + struct reset_control *rst_con; > > }; > > > > static int > > @@ -377,6 +396,19 @@ static const u8 > > mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = { > > [28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,}, > > }; > > > > +static const u8 mtk_smi_larb_clamp_port_mt8188[] = { > > You can just set these to BIT(x) directly, like: > > [MT8188_SMI_RST_LARB10] = BIT(0), > > so that you can check if there's actually a supported/valid clamp > port in function > mtk_smi_larb_parse_clamp_info() - check below for comments in that > function. > > Note that you are declaring SMI_SUB_COMM_INPORT_NR = 8 and this means > that there > are a maximum of 8 ports, with each port having its own BIT, starting > from BIT 0 > and ending at BIT 7 (so [7:0]). > > This means that we have a maximum of 8 bits here, so even if you > assign BIT(x) it > all still fits in a u8! :-) Thanks for your comments, I will modify the definition for mtk_smi_larb_clamp_port_mt8188, and macro 'SMI_COMMON_CLAMP_MASK' could be removed. > > > + [MT8188_SMI_RST_LARB10] = 1, > > + [MT8188_SMI_RST_LARB11A] = 2, > > + [MT8188_SMI_RST_LARB11C] = 3, > > + [MT8188_SMI_RST_LARB12] = 0, > > + [MT8188_SMI_RST_LARB11B] = 2, > > + [MT8188_SMI_RST_LARB15] = 1, > > + [MT8188_SMI_RST_LARB16B] = 2, > > + [MT8188_SMI_RST_LARB17B] = 3, > > + [MT8188_SMI_RST_LARB16A] = 2, > > + [MT8188_SMI_RST_LARB17A] = 3, > > +}; > > + > > static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = { > > .port_in_larb = { > > LARB0_PORT_OFFSET, LARB1_PORT_OFFSET, > > @@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen > > mtk_smi_larb_mt8188 = { > > .flags_general = MTK_SMI_FLAG_THRT_UPDATE | > > MTK_SMI_FLAG_SW_FLAG | > > MTK_SMI_FLAG_SLEEP_CTL | > > MTK_SMI_FLAG_CFG_PORT_SEC_CTL, > > .ostd = mtk_smi_larb_mt8188_ostd, > > + .clamp_port = mtk_smi_larb_clamp_port_mt8188, > > }; > > > > static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = { > > @@ -472,6 +505,60 @@ static void > > mtk_smi_larb_sleep_ctrl_disable(struct mtk_smi_larb *larb) > > writel_relaxed(0, larb->base + SMI_LARB_SLP_CON); > > } > > > > +static int mtk_smi_larb_clamp_protect_enable(struct device *dev) > > +{ > > + struct mtk_smi_larb *larb = dev_get_drvdata(dev); > > + int ret; > > + > > + /* sub_comm_syscon could be NULL if larb directly linked to > > SMI common */ > > + if (!larb->sub_comm_syscon) > > + return -EINVAL; > > + > > + ret = regmap_write(larb->sub_comm_syscon, > > SMI_COMMON_CLAMP_EN_SET, > > + SMI_COMMON_CLAMP_MASK(larb- > > >sub_comm_inport)); > > + if (ret) > > + dev_err(dev, "Unable to enable clamp, inport %d, ret > > %d\n", > > + larb->sub_comm_inport, ret); > > + > > + return ret; > > +} > > + > > +static int mtk_smi_larb_clamp_protect_disble(struct device *dev) > > typo: mtk_smi_larb_clamp_protect_disable > > > +{ > > + struct mtk_smi_larb *larb = dev_get_drvdata(dev); > > + int ret; > > + > > + /* sub_comm_syscon could be NULL if larb directly linked to > > SMI common */ > > + if (!larb->sub_comm_syscon) > > + return -EINVAL; > > + > > + ret = regmap_write(larb->sub_comm_syscon, > > SMI_COMMON_CLAMP_EN_CLR, > > + SMI_COMMON_CLAMP_MASK(larb- > > >sub_comm_inport)); > > + if (ret) > > + dev_err(dev, "Unable to disable clamp, inport %d, ret > > %d\n", > > + larb->sub_comm_inport, ret); > > + > > + return ret; > > +} > > ...but anyway, I would rather do it like that: > > static int mtk_smi_larb_clamp_protect_enable(struct device *dev, bool > enable) > { > struct mtk_smi_larb *larb = dev_get_drvdata(dev); > u32 reg; > int ret; > > /* sub_comm_syscon could be NULL if larb directly linked to > SMI common */ > if (!larb->sub_comm_syscon) > return -EINVAL; > > reg = enable ? SMI_COMMON_CLAMP_EN_SET : > SMI_COMMON_CLAMP_EN_CLR; > > ret = regmap_write(larb->sub_comm_syscon, reg, > SMI_COMMON_CLAMP_MASK(larb- > >sub_comm_inport)); > if (ret) { > dev_err(dev, "Unable to %s clamp for input port %d: > %d\n", > enable ? "enable" : "disable", > larb->sub_comm_inport, ret); > return ret; > } > > return 0; > } > > ...and then call it like > ret = mtk_smi_larb_clamp_protect_enable(dev, true); > > or > > ret = mtk_smi_larb_clamp_protect_enable(dev, false); > Thanks for your comments, I will merge these two functions into one. This looks better. > > + > > +static int mtk_smi_genpd_callback(struct notifier_block *nb, > > + unsigned long flags, void *data) > > +{ > > + struct mtk_smi_larb *larb = container_of(nb, struct > > mtk_smi_larb, nb); > > + struct device *dev = larb->dev; > > + > > + if (flags == GENPD_NOTIFY_PRE_ON || flags == > > GENPD_NOTIFY_PRE_OFF) { > > + /* disable related SMI sub-common port */ > > + mtk_smi_larb_clamp_protect_enable(dev); > > + } else if (flags == GENPD_NOTIFY_ON) { > > + /* enable related SMI sub-common port */ > > + reset_control_reset(larb->rst_con); > > + mtk_smi_larb_clamp_protect_disble(dev); > > + } > > + > > + return NOTIFY_OK; > > +} > > + > > static int mtk_smi_device_link_common(struct device *dev, struct > > device **com_dev) > > { > > struct platform_device *smi_com_pdev; > > @@ -528,6 +615,66 @@ static int mtk_smi_dts_clk_init(struct device > > *dev, struct mtk_smi *smi, > > return ret; > > } > > > > +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb > > *larb) > > +{ > > + struct device *dev = larb->dev; > > + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; > > + struct device_node *smi_node; > > + struct of_phandle_args args; > > + int ret, index; > > + > > + /* Only SMI LARBs located in camera and image subsys need to > > + * apply clamp and reset operation, others can be skipped. > > + */ > > + ret = of_parse_phandle_with_fixed_args(dev->of_node, > > + "resets", 1, 0, > > &args); > > + if (ret) > > + return 0; > > + > > + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); > > + if (!smi_node) > > + return -EINVAL; > > + > > + index = args.args[0]; > > + larb->sub_comm_inport = larb_gen->clamp_port[index]; > > + larb->sub_comm_syscon = device_node_to_regmap(smi_node); > > + of_node_put(smi_node); > > If you declare BIT(x) as clamp_ports, or if your clamp_ports start > from 1, > anything that is not more than 0 is something that was not declared, > and > this means that you can then error check: > > if (!larb->sub_comm_inport) > return dev_err_probe(dev, -EINVAL, "Unknown clamp port for > larb %d\n", index); > Yes, you are right. I will fix it as this way. > > + > > + if (IS_ERR(larb->sub_comm_syscon) || > > + larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) { > > + larb->sub_comm_syscon = NULL; > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb > > *larb) > > +{ > > + struct device *dev = larb->dev; > > + int ret; > > + > > + /* Only SMI LARBs located in camera and image subsys need to > > + * apply clamp and reset operation, others can be skipped. > > + */ > > + if (!of_find_property(dev->of_node, "resets", NULL)) > > + return 0; > > You don't have to manually check whether 'resets' exists or not - > that's already > done (devm_)reset_control_get in a way, as that will return -ENOENT > if there is > no 'reset-names' property. Check the comments down there.... > > > + > > + larb->rst_con = devm_reset_control_get(dev, "larb"); > > + if (IS_ERR(larb->rst_con)) > > + return dev_err_probe(dev, PTR_ERR(larb->rst_con), > > + "Can not get larb reset > > controller\n"); > > + > > + larb->nb.notifier_call = mtk_smi_genpd_callback; > > + ret = dev_pm_genpd_add_notifier(dev, &larb->nb); > > + if (ret) { > > + dev_err(dev, "Failed to add genpd callback %d\n", > > ret); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + OK, I will just check the return value of devm_reset_control_get instead of parsing 'resets' here. This looks better. > > static int mtk_smi_larb_probe(struct platform_device *pdev) > > { > > struct mtk_smi_larb *larb; > > @@ -538,6 +685,7 @@ static int mtk_smi_larb_probe(struct > > platform_device *pdev) > > if (!larb) > > return -ENOMEM; > > > > + larb->dev = dev; > > larb->larb_gen = of_device_get_match_data(dev); > > larb->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(larb->base)) > > @@ -554,15 +702,29 @@ static int mtk_smi_larb_probe(struct > > platform_device *pdev) > > if (ret < 0) > > return ret; > > > > - pm_runtime_enable(dev); > > ret = mtk_smi_larb_parse_reset_optional(larb); > > Choose between: > > if (ret == 0) { > ret = mtk_smi_larb_parse_smi_clamp(larb); > if (ret) { > dev_err_probe(dev, ret, "Failed to get SMI > clamp\n"); > goto err_link_remove; > } > } else if (ret != -ENOENT) { > dev_err_probe(dev, ret, "Cannot get larb resets\n"); > goto err_link_remove; > } else { > /* > * Only SMI LARBs located in camera and image subsys > need to apply > * clamp and reset operation. For the others, resets > are optional. > */ > ret = 0; > } > > and... > > if (ret && ret != -ENOENT) { > dev_err_probe ..... > } else if (ret) { > /* only smi larbs .... */ > ret = 0; > } else { > ret = mtk_smi_larb_parse_smi_clamp ..... > } > > whatever you like the most. Thanks for your comments, I will check the return value as this way. > > > + /* find sub common to clamp larb for ISP software reset */ > > + ret = mtk_smi_larb_parse_clamp_info(larb); > > + if (ret) { > > + dev_err(dev, "Failed to get clamp setting for > > larb\n"); > > + goto err_link_remove; > > + } > > + > > + ret = mtk_smi_larb_parse_reset_info(larb); > > + if (ret) { > > + dev_err(dev, "Failed to get power setting for > > larb\n"); > > + goto err_link_remove; > > + } > > + > > platform_set_drvdata(pdev, larb); > > ret = component_add(dev, &mtk_smi_larb_component_ops); > > if (ret) > > - goto err_pm_disable; > > + goto err_link_remove; > > + > > + pm_runtime_enable(dev); > > + > > return 0; > > > > -err_pm_disable: > > - pm_runtime_disable(dev); > > +err_link_remove: > > device_link_remove(dev, larb->smi_common_dev); > > return ret; > > } > > @@ -686,6 +848,10 @@ static const struct mtk_smi_common_plat > > mtk_smi_common_mt8188_vpp = { > > .init = mtk_smi_common_mt8195_init, > > }; > > > > +static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 > > = { > > + .type = MTK_SMI_GEN2_SUB_COMM, > > ...no gals in MT8188?! > Yes, there is gals in MT8188 hardware design. And the 'gals' clock may be the same as the 'apb' and 'smi' clock. I will add this in the next version, thanks for your comments. > Cheers, > Angelo > > > +}; > > + > > static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = { > > .type = MTK_SMI_GEN2, > > .has_gals = true, > > @@ -729,6 +895,7 @@ static const struct of_device_id > > mtk_smi_common_of_ids[] = { > > {.compatible = "mediatek,mt8186-smi-common", .data = > > &mtk_smi_common_mt8186}, > > {.compatible = "mediatek,mt8188-smi-common-vdo", .data = > > &mtk_smi_common_mt8188_vdo}, > > {.compatible = "mediatek,mt8188-smi-common-vpp", .data = > > &mtk_smi_common_mt8188_vpp}, > > + {.compatible = "mediatek,mt8188-smi-sub-common", .data = > > &mtk_smi_sub_common_mt8188}, > > {.compatible = "mediatek,mt8192-smi-common", .data = > > &mtk_smi_common_mt8192}, > > {.compatible = "mediatek,mt8195-smi-common-vdo", .data = > > &mtk_smi_common_mt8195_vdo}, > > {.compatible = "mediatek,mt8195-smi-common-vpp", .data = > > &mtk_smi_common_mt8195_vpp}, > >
On Wed, 2024-11-20 at 08:49 +0100, Krzysztof Kozlowski wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On 20/11/2024 07:36, Friday Yang wrote: > > In order to avoid handling glitch signal when MTCMOS on/off, SMI > > need > > clamp and reset operation. Parse power reset settings for LARBs > > which > > need to reset. Register genpd callback for SMI LARBs and apply > > reset > > operations in the callback. > > > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > > --- > > drivers/memory/mtk-smi.c | 175 > > ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 171 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c > > index 2bc034dff691..c7119f655350 100644 > > --- a/drivers/memory/mtk-smi.c > > +++ b/drivers/memory/mtk-smi.c > > @@ -10,15 +10,21 @@ > > #include <linux/err.h> > > #include <linux/io.h> > > #include <linux/iopoll.h> > > +#include <linux/mfd/syscon.h> > > Where do you use it? device_node_to_regmap need this header file. > > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > +#include <linux/pm_domain.h> > > Where do you use it? dev_pm_genpd_add_notifier need this header file. > > > #include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > Where do you use it? regmap_write need this header file. > > > +#include <linux/reset.h> > > +#include <linux/reset-controller.h> > > #include <linux/soc/mediatek/mtk_sip_svc.h> > > #include <soc/mediatek/smi.h> > > #include <dt-bindings/memory/mt2701-larb-port.h> > > #include <dt-bindings/memory/mtk-memory-port.h> > > +#include <dt-bindings/reset/mt8188-resets.h> > > > > ... reset_control_reset/devm_reset_control_get need reset.h But reset-controller.h could be removed. MT8188_SMI_RST_LARB10 and other index need mt8188-resets.h > > > > > +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb > > *larb) > > +{ > > + struct device *dev = larb->dev; > > + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; > > + struct device_node *smi_node; > > + struct of_phandle_args args; > > + int ret, index; > > + > > + /* Only SMI LARBs located in camera and image subsys need to > > Use Linux coding style. Sorry for the mistake, I will fix it. > > > + * apply clamp and reset operation, others can be skipped. > > + */ > > + ret = of_parse_phandle_with_fixed_args(dev->of_node, > > + "resets", 1, 0, > > &args); > > NAK > > > + if (ret) > > + return 0; > > + > > + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); > > + if (!smi_node) > > + return -EINVAL; > > + > > + index = args.args[0]; > > That's reset, not clamp port. NAK. I could change from 'clamp_port' to 'reset_port'. This index is used for getting the port id from the array 'mtk_smi_larb_clamp_port_mt8188 ' in SMI driver. It could also be used for getting the reset signal in SMI reset controller driver. > > > + larb->sub_comm_inport = larb_gen->clamp_port[index]; > > + larb->sub_comm_syscon = device_node_to_regmap(smi_node); > > + of_node_put(smi_node); > > + > > + if (IS_ERR(larb->sub_comm_syscon) || > > + larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) { > > + larb->sub_comm_syscon = NULL; > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb > > *larb) > > +{ > > + struct device *dev = larb->dev; > > + int ret; > > + > > + /* Only SMI LARBs located in camera and image subsys need to > > Use Linux coding style. This applies to all your patches and all > places > in this patch. OK, I will fix it. > > > + * apply clamp and reset operation, others can be skipped. > > + */ > > + if (!of_find_property(dev->of_node, "resets", NULL)) > > That's not how you use reset framework API. Use proper optional API. Thanks, I will check the return value for devm_reset_control_get instead of parsing 'resets' here. > > > + return 0; > > + > > + larb->rst_con = devm_reset_control_get(dev, "larb"); > > + if (IS_ERR(larb->rst_con)) > > + return dev_err_probe(dev, PTR_ERR(larb->rst_con), > > + "Can not get larb reset > > controller\n"); > > + > > + larb->nb.notifier_call = mtk_smi_genpd_callback; > > + ret = dev_pm_genpd_add_notifier(dev, &larb->nb); > > + if (ret) { > > + dev_err(dev, "Failed to add genpd callback %d\n", > > ret); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > static int mtk_smi_larb_probe(struct platform_device *pdev) > > { > > struct mtk_smi_larb *larb; > > @@ -538,6 +685,7 @@ static int mtk_smi_larb_probe(struct > > platform_device *pdev) > > if (!larb) > > return -ENOMEM; > > > > + larb->dev = dev; > > larb->larb_gen = of_device_get_match_data(dev); > > larb->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(larb->base)) > > @@ -554,15 +702,29 @@ static int mtk_smi_larb_probe(struct > > platform_device *pdev) > > if (ret < 0) > > return ret; > > > > - pm_runtime_enable(dev); > > + /* find sub common to clamp larb for ISP software reset */ > > + ret = mtk_smi_larb_parse_clamp_info(larb); > > + if (ret) { > > + dev_err(dev, "Failed to get clamp setting for > > larb\n"); > > + goto err_link_remove; > > + } > > + > > + ret = mtk_smi_larb_parse_reset_info(larb); > > + if (ret) { > > + dev_err(dev, "Failed to get power setting for > > larb\n"); > > + goto err_link_remove; > > + } > > + > > platform_set_drvdata(pdev, larb); > > ret = component_add(dev, &mtk_smi_larb_component_ops); > > if (ret) > > - goto err_pm_disable; > > + goto err_link_remove; > > + > > + pm_runtime_enable(dev); > > + > > return 0; > > > > -err_pm_disable: > > - pm_runtime_disable(dev); > > +err_link_remove: > > device_link_remove(dev, larb->smi_common_dev); > > return ret; > > } > > @@ -686,6 +848,10 @@ static const struct mtk_smi_common_plat > > mtk_smi_common_mt8188_vpp = { > > .init = mtk_smi_common_mt8195_init, > > }; > > > > +static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 > > = { > > + .type = MTK_SMI_GEN2_SUB_COMM, > > +}; > > + > > static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = { > > .type = MTK_SMI_GEN2, > > .has_gals = true, > > @@ -729,6 +895,7 @@ static const struct of_device_id > > mtk_smi_common_of_ids[] = { > > {.compatible = "mediatek,mt8186-smi-common", .data = > > &mtk_smi_common_mt8186}, > > {.compatible = "mediatek,mt8188-smi-common-vdo", .data = > > &mtk_smi_common_mt8188_vdo}, > > {.compatible = "mediatek,mt8188-smi-common-vpp", .data = > > &mtk_smi_common_mt8188_vpp}, > > + {.compatible = "mediatek,mt8188-smi-sub-common", .data = > > &mtk_smi_sub_common_mt8188}, > > {.compatible = "mediatek,mt8192-smi-common", .data = > > &mtk_smi_common_mt8192}, > > {.compatible = "mediatek,mt8195-smi-common-vdo", .data = > > &mtk_smi_common_mt8195_vdo}, > > {.compatible = "mediatek,mt8195-smi-common-vpp", .data = > > &mtk_smi_common_mt8195_vpp}, > > > Best regards, > Krzysztof
On 22/11/2024 10:41, Friday Yang (杨阳) wrote: > On Wed, 2024-11-20 at 08:49 +0100, Krzysztof Kozlowski wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> On 20/11/2024 07:36, Friday Yang wrote: >>> In order to avoid handling glitch signal when MTCMOS on/off, SMI >>> need >>> clamp and reset operation. Parse power reset settings for LARBs >>> which >>> need to reset. Register genpd callback for SMI LARBs and apply >>> reset >>> operations in the callback. >>> >>> Signed-off-by: Friday Yang <friday.yang@mediatek.com> >>> --- >>> drivers/memory/mtk-smi.c | 175 >>> ++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 171 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c >>> index 2bc034dff691..c7119f655350 100644 >>> --- a/drivers/memory/mtk-smi.c >>> +++ b/drivers/memory/mtk-smi.c >>> @@ -10,15 +10,21 @@ >>> #include <linux/err.h> >>> #include <linux/io.h> >>> #include <linux/iopoll.h> >>> +#include <linux/mfd/syscon.h> >> >> Where do you use it? > > device_node_to_regmap need this header file. Ah, indeed, but then I wonder why you parse phandle instead of using standard syscon API: syscon_regmap_lookup_by_phandle(). > >> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/of_platform.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_domain.h> >> >> Where do you use it? > > dev_pm_genpd_add_notifier need this header file. ack > >> >>> #include <linux/pm_runtime.h> >>> +#include <linux/regmap.h> >> >> Where do you use it? > > regmap_write need this header file. ack > >> >>> +#include <linux/reset.h> >>> +#include <linux/reset-controller.h> >>> #include <linux/soc/mediatek/mtk_sip_svc.h> >>> #include <soc/mediatek/smi.h> >>> #include <dt-bindings/memory/mt2701-larb-port.h> >>> #include <dt-bindings/memory/mtk-memory-port.h> >>> +#include <dt-bindings/reset/mt8188-resets.h> >>> >> >> ... > > reset_control_reset/devm_reset_control_get need reset.h > But reset-controller.h could be removed. > MT8188_SMI_RST_LARB10 and other index need mt8188-resets.h > >> >>> >>> +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb >>> *larb) >>> +{ >>> + struct device *dev = larb->dev; >>> + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; >>> + struct device_node *smi_node; >>> + struct of_phandle_args args; >>> + int ret, index; >>> + >>> + /* Only SMI LARBs located in camera and image subsys need to >> >> Use Linux coding style. > > Sorry for the mistake, I will fix it. > >> >>> + * apply clamp and reset operation, others can be skipped. >>> + */ >>> + ret = of_parse_phandle_with_fixed_args(dev->of_node, >>> + "resets", 1, 0, >>> &args); >> >> NAK >> >>> + if (ret) >>> + return 0; >>> + >>> + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); >>> + if (!smi_node) >>> + return -EINVAL; >>> + >>> + index = args.args[0]; >> >> That's reset, not clamp port. NAK. > > I could change from 'clamp_port' to 'reset_port'. > This index is used for getting the port id from the array > 'mtk_smi_larb_clamp_port_mt8188 ' in SMI driver. Index is for reset, not for port ID. > It could also be used for getting the reset signal in > SMI reset controller driver. > Look at my comments from previous version - they were not under reset property. The argument for reset is for reset provider, not this reset consumer. BTW, when you link to previous versions of patchset, link to lore, not patchwork. Or just use b4. Best regards, Krzysztof
diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c index 2bc034dff691..c7119f655350 100644 --- a/drivers/memory/mtk-smi.c +++ b/drivers/memory/mtk-smi.c @@ -10,15 +10,21 @@ #include <linux/err.h> #include <linux/io.h> #include <linux/iopoll.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/reset.h> +#include <linux/reset-controller.h> #include <linux/soc/mediatek/mtk_sip_svc.h> #include <soc/mediatek/smi.h> #include <dt-bindings/memory/mt2701-larb-port.h> #include <dt-bindings/memory/mtk-memory-port.h> +#include <dt-bindings/reset/mt8188-resets.h> /* SMI COMMON */ #define SMI_L1LEN 0x100 @@ -36,6 +42,13 @@ #define SMI_DCM 0x300 #define SMI_DUMMY 0x444 +#define SMI_COMMON_CLAMP_EN 0x3c0 +#define SMI_COMMON_CLAMP_EN_SET 0x3c4 +#define SMI_COMMON_CLAMP_EN_CLR 0x3c8 +#define SMI_COMMON_CLAMP_MASK(inport) BIT(inport) + +#define SMI_SUB_COMM_INPORT_NR (8) + /* SMI LARB */ #define SMI_LARB_SLP_CON 0xc #define SLP_PROT_EN BIT(0) @@ -134,6 +147,7 @@ struct mtk_smi_larb_gen { unsigned int larb_direct_to_common_mask; unsigned int flags_general; const u8 (*ostd)[SMI_LARB_PORT_NR_MAX]; + const u8 *clamp_port; }; struct mtk_smi { @@ -150,6 +164,7 @@ struct mtk_smi { }; struct mtk_smi_larb { /* larb: local arbiter */ + struct device *dev; struct mtk_smi smi; void __iomem *base; struct device *smi_common_dev; /* common or sub-common dev */ @@ -157,6 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter */ int larbid; u32 *mmu; unsigned char *bank; + struct regmap *sub_comm_syscon; + u8 sub_comm_inport; + struct notifier_block nb; + struct reset_control *rst_con; }; static int @@ -377,6 +396,19 @@ static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = { [28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,}, }; +static const u8 mtk_smi_larb_clamp_port_mt8188[] = { + [MT8188_SMI_RST_LARB10] = 1, + [MT8188_SMI_RST_LARB11A] = 2, + [MT8188_SMI_RST_LARB11C] = 3, + [MT8188_SMI_RST_LARB12] = 0, + [MT8188_SMI_RST_LARB11B] = 2, + [MT8188_SMI_RST_LARB15] = 1, + [MT8188_SMI_RST_LARB16B] = 2, + [MT8188_SMI_RST_LARB17B] = 3, + [MT8188_SMI_RST_LARB16A] = 2, + [MT8188_SMI_RST_LARB17A] = 3, +}; + static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = { .port_in_larb = { LARB0_PORT_OFFSET, LARB1_PORT_OFFSET, @@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = { .flags_general = MTK_SMI_FLAG_THRT_UPDATE | MTK_SMI_FLAG_SW_FLAG | MTK_SMI_FLAG_SLEEP_CTL | MTK_SMI_FLAG_CFG_PORT_SEC_CTL, .ostd = mtk_smi_larb_mt8188_ostd, + .clamp_port = mtk_smi_larb_clamp_port_mt8188, }; static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = { @@ -472,6 +505,60 @@ static void mtk_smi_larb_sleep_ctrl_disable(struct mtk_smi_larb *larb) writel_relaxed(0, larb->base + SMI_LARB_SLP_CON); } +static int mtk_smi_larb_clamp_protect_enable(struct device *dev) +{ + struct mtk_smi_larb *larb = dev_get_drvdata(dev); + int ret; + + /* sub_comm_syscon could be NULL if larb directly linked to SMI common */ + if (!larb->sub_comm_syscon) + return -EINVAL; + + ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_SET, + SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport)); + if (ret) + dev_err(dev, "Unable to enable clamp, inport %d, ret %d\n", + larb->sub_comm_inport, ret); + + return ret; +} + +static int mtk_smi_larb_clamp_protect_disble(struct device *dev) +{ + struct mtk_smi_larb *larb = dev_get_drvdata(dev); + int ret; + + /* sub_comm_syscon could be NULL if larb directly linked to SMI common */ + if (!larb->sub_comm_syscon) + return -EINVAL; + + ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_CLR, + SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport)); + if (ret) + dev_err(dev, "Unable to disable clamp, inport %d, ret %d\n", + larb->sub_comm_inport, ret); + + return ret; +} + +static int mtk_smi_genpd_callback(struct notifier_block *nb, + unsigned long flags, void *data) +{ + struct mtk_smi_larb *larb = container_of(nb, struct mtk_smi_larb, nb); + struct device *dev = larb->dev; + + if (flags == GENPD_NOTIFY_PRE_ON || flags == GENPD_NOTIFY_PRE_OFF) { + /* disable related SMI sub-common port */ + mtk_smi_larb_clamp_protect_enable(dev); + } else if (flags == GENPD_NOTIFY_ON) { + /* enable related SMI sub-common port */ + reset_control_reset(larb->rst_con); + mtk_smi_larb_clamp_protect_disble(dev); + } + + return NOTIFY_OK; +} + static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev) { struct platform_device *smi_com_pdev; @@ -528,6 +615,66 @@ static int mtk_smi_dts_clk_init(struct device *dev, struct mtk_smi *smi, return ret; } +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb *larb) +{ + struct device *dev = larb->dev; + const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen; + struct device_node *smi_node; + struct of_phandle_args args; + int ret, index; + + /* Only SMI LARBs located in camera and image subsys need to + * apply clamp and reset operation, others can be skipped. + */ + ret = of_parse_phandle_with_fixed_args(dev->of_node, + "resets", 1, 0, &args); + if (ret) + return 0; + + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); + if (!smi_node) + return -EINVAL; + + index = args.args[0]; + larb->sub_comm_inport = larb_gen->clamp_port[index]; + larb->sub_comm_syscon = device_node_to_regmap(smi_node); + of_node_put(smi_node); + + if (IS_ERR(larb->sub_comm_syscon) || + larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) { + larb->sub_comm_syscon = NULL; + return -EINVAL; + } + + return 0; +} + +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb *larb) +{ + struct device *dev = larb->dev; + int ret; + + /* Only SMI LARBs located in camera and image subsys need to + * apply clamp and reset operation, others can be skipped. + */ + if (!of_find_property(dev->of_node, "resets", NULL)) + return 0; + + larb->rst_con = devm_reset_control_get(dev, "larb"); + if (IS_ERR(larb->rst_con)) + return dev_err_probe(dev, PTR_ERR(larb->rst_con), + "Can not get larb reset controller\n"); + + larb->nb.notifier_call = mtk_smi_genpd_callback; + ret = dev_pm_genpd_add_notifier(dev, &larb->nb); + if (ret) { + dev_err(dev, "Failed to add genpd callback %d\n", ret); + return -EINVAL; + } + + return 0; +} + static int mtk_smi_larb_probe(struct platform_device *pdev) { struct mtk_smi_larb *larb; @@ -538,6 +685,7 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) if (!larb) return -ENOMEM; + larb->dev = dev; larb->larb_gen = of_device_get_match_data(dev); larb->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(larb->base)) @@ -554,15 +702,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev) if (ret < 0) return ret; - pm_runtime_enable(dev); + /* find sub common to clamp larb for ISP software reset */ + ret = mtk_smi_larb_parse_clamp_info(larb); + if (ret) { + dev_err(dev, "Failed to get clamp setting for larb\n"); + goto err_link_remove; + } + + ret = mtk_smi_larb_parse_reset_info(larb); + if (ret) { + dev_err(dev, "Failed to get power setting for larb\n"); + goto err_link_remove; + } + platform_set_drvdata(pdev, larb); ret = component_add(dev, &mtk_smi_larb_component_ops); if (ret) - goto err_pm_disable; + goto err_link_remove; + + pm_runtime_enable(dev); + return 0; -err_pm_disable: - pm_runtime_disable(dev); +err_link_remove: device_link_remove(dev, larb->smi_common_dev); return ret; } @@ -686,6 +848,10 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = { .init = mtk_smi_common_mt8195_init, }; +static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 = { + .type = MTK_SMI_GEN2_SUB_COMM, +}; + static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = { .type = MTK_SMI_GEN2, .has_gals = true, @@ -729,6 +895,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = { {.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186}, {.compatible = "mediatek,mt8188-smi-common-vdo", .data = &mtk_smi_common_mt8188_vdo}, {.compatible = "mediatek,mt8188-smi-common-vpp", .data = &mtk_smi_common_mt8188_vpp}, + {.compatible = "mediatek,mt8188-smi-sub-common", .data = &mtk_smi_sub_common_mt8188}, {.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192}, {.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo}, {.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp},
In order to avoid handling glitch signal when MTCMOS on/off, SMI need clamp and reset operation. Parse power reset settings for LARBs which need to reset. Register genpd callback for SMI LARBs and apply reset operations in the callback. Signed-off-by: Friday Yang <friday.yang@mediatek.com> --- drivers/memory/mtk-smi.c | 175 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 171 insertions(+), 4 deletions(-)