Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

["Friday Yang (杨阳)" <Friday.Yang@mediatek.com>]

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@mediatek.com>
> > ---
> >  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.
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");
> 
> 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:
    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'.

> >  return ret;
> >  }
> > @@ -686,6 +825,10 @@ static const struct mtk_smi_common_plat
> mtk_smi_common_mt8188_vpp = {
> >  .init     = mtk_smi_common_mt8195_init,
> >  };
> 
> Best regards,
> Krzysztof
>