Re: [PATCH] soc/tegra: pmc: Use register definitions instead of magic values

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

On Thu, Jun 09, 2016 at 09:20:42AM +0100, Jon Hunter wrote:
> 
> On 08/06/16 18:15, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Use register definitions for the main SoC reset operation instead of
> > hard-coding magic values. Note that the PMC_RST_STATUS register isn't
> > actually accessed, but since it is mentioned in a comment the
> > definitions are added for completeness.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/soc/tegra/pmc.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 852f8470d6e7..70acad7ceca0 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -51,6 +51,7 @@
> >  #define  PMC_CNTRL_CPU_PWRREQ_POLARITY	(1 << 15)  /* CPU pwr req polarity */
> >  #define  PMC_CNTRL_CPU_PWRREQ_OE	(1 << 16)  /* CPU pwr req enable */
> >  #define  PMC_CNTRL_INTR_POLARITY	(1 << 17)  /* inverts INTR polarity */
> > +#define  PMC_CNTRL_MAIN_RST		(1 <<  4)
> >  
> >  #define DPD_SAMPLE			0x020
> >  #define  DPD_SAMPLE_ENABLE		(1 << 0)
> > @@ -80,6 +81,14 @@
> >  #define PMC_SENSOR_CTRL_SCRATCH_WRITE	(1 << 2)
> >  #define PMC_SENSOR_CTRL_ENABLE_RST	(1 << 1)
> >  
> > +#define PMC_RST_STATUS			0x1b4
> > +#define  PMC_RST_STATUS_AOTAG		(1 << 5)
> > +#define  PMC_RST_STATUS_LP0		(1 << 4)
> > +#define  PMC_RST_STATUS_SW_MAIN		(1 << 3)
> > +#define  PMC_RST_STATUS_SENSOR		(1 << 2)
> > +#define  PMC_RST_STATUS_WATCHDOG	(1 << 1)
> > +#define  PMC_RST_STATUS_POR		(1 << 0)
> 
> Looking at the TRM, the PMC_RST_STATUS register only has one field
> called "RST_SOURCE" and the above are possible values for this field.
> However, these are integer values and not actual bits (as it is a 3-bit
> field). I believe that they should just be ...
> 
> #define PMC_RST_STATUS			0x1b4
> #define  PMC_RST_STATUS_AOTAG		5
> #define  PMC_RST_STATUS_LP0		4
> #define  PMC_RST_STATUS_SW_MAIN		3
> #define  PMC_RST_STATUS_SENSOR		2
> #define  PMC_RST_STATUS_WATCHDOG	1
> #define  PMC_RST_STATUS_POR		0

Ugh... indeed. No idea how I came up with this.

> >  #define IO_DPD_REQ			0x1b8
> >  #define  IO_DPD_REQ_CODE_IDLE		(0 << 30)
> >  #define  IO_DPD_REQ_CODE_OFF		(1 << 30)
> > @@ -638,9 +647,10 @@ static int tegra_pmc_restart_notify(struct notifier_block *this,
> >  
> >  	tegra_pmc_writel(value, PMC_SCRATCH0);
> >  
> > -	value = tegra_pmc_readl(0);
> > -	value |= 0x10;
> > -	tegra_pmc_writel(value, 0);
> > +	/* reset everything but PMC_SCRATCH0 and PMC_RST_STATUS */
> 
> Not sure I understand the comment about PMC_RST_STATUS because this is
> just a status register and it will log source of the reset. The TRM does
> show that it is a R/W register. Are you just saying that source of the
> reset will be preserved by the PMC_RST_STATUS register?

Yes, PMC_SCRATCH0 and PMC_RST_STATUS are the only two registers that are
preserved over the reset, whereas all other registers will be reset.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]