Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support

[Sowjanya Komatineni]

On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
> 31.07.2019 3:20, Sowjanya Komatineni ÐÐÑÐÑ:
> > This patch implements save and restore context for peripheral fixed
> > clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
> > peripheral clock ops.
> >
> > During system suspend, core power goes off and looses the settings
> > of the Tegra CAR controller registers.
> >
> > So during suspend entry clock and reset state of peripherals is saved
> > and on resume they are restored to have clocks back to same rate and
> > state as before suspend.
> >
> > Acked-by: Thierry Reding <treding@xxxxxxxxxx>
> > Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
> > ---
> >   drivers/clk/tegra/clk-periph-fixed.c | 33 ++++++++++++++++++++++++++++++++
> >   drivers/clk/tegra/clk-periph-gate.c  | 34 +++++++++++++++++++++++++++++++++
> >   drivers/clk/tegra/clk-periph.c       | 37 ++++++++++++++++++++++++++++++++++++
> >   drivers/clk/tegra/clk-sdmmc-mux.c    | 28 +++++++++++++++++++++++++++
> >   drivers/clk/tegra/clk.h              |  6 ++++++
> >   5 files changed, 138 insertions(+)
> >
> > diff --git a/drivers/clk/tegra/clk-periph-fixed.c b/drivers/clk/tegra/clk-periph-fixed.c
> > index c088e7a280df..21b24530fa00 100644
> > --- a/drivers/clk/tegra/clk-periph-fixed.c
> > +++ b/drivers/clk/tegra/clk-periph-fixed.c
> > @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw *hw,
> >   	return (unsigned long)rate;
> >   }
> >   
> > +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
> > +{
> > +	struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
> > +	u32 mask = 1 << (fixed->num % 32);
> This could be BIT(fixed->num % 32).
>
> > +	fixed->enb_ctx = readl_relaxed(fixed->base + fixed->regs->enb_reg) &
> > +			 mask;
> > +	fixed->rst_ctx = readl_relaxed(fixed->base + fixed->regs->rst_reg) &
> > +			 mask;
> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
> here. You're getting away here because bool is an 32bit unsigned int,
> but you shouldn't rely on it and always explicitly convert to a bool.
>
> > +	return 0;
> > +}
> > +
> > +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
> > +{
> > +	struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
> > +	u32 mask = 1 << (fixed->num % 32);
> > +
> > +	if (fixed->enb_ctx)
> > +		writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
> > +	else
> > +		writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
> > +
> > +	udelay(2);
> Will be better to read out and compare the hardware's state with the
> restored one, then bail out if the state is unchanged.
>
> Shouldn't it be fence_udelay()?
>
> > +	if (!fixed->rst_ctx) {
> > +		udelay(5); /* reset propogation delay */
> Why delaying is done before the writing to the reset register?

During SC7 exit, peripheral reset state is set to POR state. So some 
peripherals will already be in reset state and making sure of 
propagation delay before releasing from reset.

It should be rst_clr_reg. will fix in next rev

> > +		writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
> I'm not quite sure what's going on here, this looks wrong.
>
> 1. rst_reg points to RST_DEVICES_x
> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
> each individual device
> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
> device which corresponds to the mask
> 4. The reset is asserted for a single device, while !fixed->rst_ctx
> means that it actually should be deasserted (?)
>
> Apparently you should use rst_set_reg / rst_clr_reg.
Yes, It should be rst_clr_reg. will fix in next rev
> > +	}
> What about the case where rst_ctx=true?

ON SC7 exit, state of RST_DEV will be POR state where most peripherals 
will already be in reset state.

Few of them which are not in reset state in POR values are those that 
need to stay de-asserted across the boot states anyway.

> > +}
> > @@ -517,6 +517,8 @@ struct tegra_clk_periph_gate {
> >   	int			clk_num;
> >   	int			*enable_refcnt;
> >   	const struct tegra_clk_periph_regs *regs;
> > +	bool			clk_state_ctx;
> > +	bool			rst_state_ctx;
> >   };
> >   
> >   #define to_clk_periph_gate(_hw)					\
> > @@ -543,6 +545,8 @@ struct tegra_clk_periph_fixed {
> >   	unsigned int mul;
> >   	unsigned int div;
> >   	unsigned int num;
> > +	bool enb_ctx;
> > +	bool rst_ctx;
> >   };
> I'd expect these to be bool:1.