Re: [PATCH 3/4] memory: mtk-smi: Add sleep ctrl function

[Yong Wu <yong.wu@mediatek.com>]

On Sat, 2021-12-04 at 12:48 +0100, Krzysztof Kozlowski wrote:
> On 03/12/2021 07:40, Yong Wu wrote:
> > sleep control means that when the larb go to sleep, we should wait
> > a bit
> 
> s/go/goes/
> 
> > until all the current commands are finished. thus, when the larb
> > runtime
> 
> Please start every sentence with a capital letter.
> 
> > suspend, we need enable this function to wait until all the existed
> 
> s/suspend/suspends/
> s/we need enable/we need to enable/
> 
> > command are finished. when the larb resume, just disable this
> > function.
> 
> s/command/commands/
> s/resume/resumes/
> 
> > This function only improve the safe of bus. Add a new flag for this
> 
> s/improve/improves/
> s/the safe/the safety/
> 
> > function. Prepare for mt8186.
> 
> In total it is hard to parse, really.

Will fix them in next version.

Thanks for reviewing so detailedly. Sorry. I didn't pay attention to
the grammar before.

> 
> > 
> > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> >  drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++-
> > ---
> >  1 file changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index b883dcc0bbfa..4b59b28e4d73 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> > @@ -32,6 +33,10 @@
> >  #define SMI_DUMMY			0x444
> >  
> >  /* SMI LARB */
> > +#define SMI_LARB_SLP_CON                0x00c
> > +#define SLP_PROT_EN                     BIT(0)
> > +#define SLP_PROT_RDY                    BIT(16)
> > +
> >  #define SMI_LARB_CMD_THRT_CON		0x24
> >  #define SMI_LARB_THRT_RD_NU_LMT_MSK	GENMASK(7, 4)
> >  #define SMI_LARB_THRT_RD_NU_LMT		(5 << 4)
> > @@ -81,6 +86,7 @@
> >  
> >  #define MTK_SMI_FLAG_THRT_UPDATE	BIT(0)
> >  #define MTK_SMI_FLAG_SW_FLAG		BIT(1)
> > +#define MTK_SMI_FLAG_SLEEP_CTL		BIT(2)
> >  #define MTK_SMI_CAPS(flags, _x)		(!!((flags) & (_x)))
> >  
> >  struct mtk_smi_reg_pair {
> > @@ -371,6 +377,24 @@ static const struct of_device_id
> > mtk_smi_larb_of_ids[] = {
> >  	{}
> >  };
> >  
> > +static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool
> > to_sleep)
> > +{
> 
> Make two functions instead. There is no single code reuse (shared)
> between sleep and resume. In the same time bool arguments are
> confusing
> when looking at caller and one never knows whether true means to
> resume
> or to sleep. Having two functions is obvious. Obvious code is easier
> to
> read and maintain.

Make sense. Thanks for this suggestion.

> 
> > +	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > +	int ret = 0;
> > +	u32 tmp;
> > +
> > +	if (to_sleep) {
> > +		writel_relaxed(SLP_PROT_EN, larb->base +
> > SMI_LARB_SLP_CON);
> > +		ret = readl_poll_timeout_atomic(larb->base +
> > SMI_LARB_SLP_CON,
> > +						tmp, !!(tmp &
> > SLP_PROT_RDY), 10, 1000);
> > +		if (ret)
> > +			dev_warn(dev, "sleep ctrl is not
> > ready(0x%x).\n", tmp);
> > +	} else {
> > +		writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
> > +	}
> > +	return ret;
> > +}
> > +
> >  static int mtk_smi_device_link_common(struct device *dev, struct
> > device **com_dev)
> >  {
> >  	struct platform_device *smi_com_pdev;
> > @@ -477,24 +501,31 @@ static int __maybe_unused
> > mtk_smi_larb_resume(struct device *dev)
> >  {
> >  	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> >  	const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > -	int ret;
> > +	int ret = 0;
> 
> This line does not have a sense.

Yes. This is unhelpful. Will remove this.

> 
> >  
> >  	ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb-
> > >smi.clks);
> > -	if (ret < 0)
> > +	if (ret)
> 
> Why changing this?

The successful return value should be 0. I will use a independent patch
for this.

> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek