Il 24/10/24 03:29, Friday Yang (杨阳) ha scritto:
On Wed, 2024-08-21 at 11:00 +0200, Krzysztof Kozlowski wrote:
External email : Please do not click links or open attachments until
you have verified the sender or the content.
On 21/08/2024 10:26, 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@xxxxxxxxxxxx>
---
drivers/memory/mtk-smi.c | 148
++++++++++++++++++++++++++++++++++++++-
1 file changed, 146 insertions(+), 2 deletions(-)
...
+
+static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb
*larb)
+{
+struct device_node *reset_node;
+struct device *dev = larb->dev;
+int ret;
+
+/* only larb with "resets" need to get reset setting */
+reset_node = of_parse_phandle(dev->of_node, "resets", 0);
Nope, you do not parse rasets.
1.If I need to use the Linux reset control framework, 'resets' is the
required property.
Leave that to the reset API, don't manually parse the resets phandle here,
that's what Krzysztof was meaning.
2.'reset-names' give the list of reset signal name strings sorted in
the same order as the 'resets' property. SMI driver will use 'reset-
names' to match reset signal names with reset specifiers.
3.Not all SMI larbs need to apply reset operations, only SMI larbs
which may have bus glitch issues need this. Just to confirm if this
larb support reset function.
+if (!reset_node)
+return 0;
+of_node_put(reset_node);
+
+larb->rst_con = devm_reset_control_get(dev, "larb_rst");
"larb" is just fine as a name: it's clear that this is a reset, as
it's specified as `reset-names = "name"`.
Where are the bindings? Why do you add undocumented properties? How
possible this passes dtbs_check???
This is not the new added property in SMI larb DT node.
It is the reset signal name which is inclued in 'reset-names'.
Just like this:
resets = <&imgsys1_dip_nr_rst MT8188_SIM_RST_LARB15>;
reset-name = 'larb_rst';
Maybe I can add this name to the 'reset-name' property binding
description, like this, is this clear for you?
reset-name:
It's "reset-names" btw.
const: larb_rst
description: the name of reset signal. SMI driver need to obtain
the reset controller based on this.
+if (IS_ERR(larb->rst_con))
+return dev_err_probe(dev, PTR_ERR(larb->rst_con),
+ "cannot 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 +662,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 +679,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_pm_disable;
+}
+
+ret = mtk_smi_larb_parse_reset_info(larb);
+if (ret) {
+dev_err(dev, "Failed to get power setting for larb\n");
+goto err_pm_disable;
+}
+
platform_set_drvdata(pdev, larb);
ret = component_add(dev, &mtk_smi_larb_component_ops);
if (ret)
goto err_pm_disable;
+
+pm_runtime_enable(dev);
+
return 0;
err_pm_disable:
-pm_runtime_disable(dev);
device_link_remove(dev, larb->smi_common_dev);
Label asls pm disable. Where is the pm disable?
Thanks, I will fix it to 'err_link_remove'.
...or you can just use devm_pm_runtime_enable() instead, and not worry
about disabling it on your own.
Regards,
Angelo
