[PATCH 01/19] ARM: OMAP4: PM: save/restore all DPLL settings in OFF mode

[Tony Lindgren]

* Jon Hunter <jon-hunter at ti.com> [120425 08:16]:
> Hi Tero,
> 
> On 04/25/2012 02:33 AM, Tero Kristo wrote:
> > On Mon, 2012-04-23 at 11:09 -0500, Jon Hunter wrote:
> >> Hi Tero,
> >>
> >> On 04/20/2012 04:33 AM, Tero Kristo wrote:
> >>
> >> [...]
> >>
> >>> +/**
> >>> + * omap4_dpll_print_reg - dump out a single DPLL register value
> >>> + * @dpll_reg: register to dump
> >>> + * @name: name of the register
> >>> + * @tuple: content of the register
> >>> + *
> >>> + * Helper dump function to print out a DPLL register value in case
> >>> + * of restore failures.
> >>> + */
> >>> +static void omap4_dpll_print_reg(struct omap4_dpll_regs *dpll_reg, char *name,
> >>> +				 struct dpll_reg *tuple)
> >>> +{
> >>> +	if (tuple->offset)
> >>> +		pr_warn("%s - Address offset = 0x%08x, value=0x%08x\n", name,
> >>> +			tuple->offset, tuple->val);
> >>
> >> Minor nit-pick here ...
> >>
> >> 1. Offset is 16-bits and so we can just have "offset = 0x%04x"
> >> 2. Consider dropping "Address" from "Address offset"
> >> 3. Can we be consistent in our spaces for "offset = " and "value="?
> >>
> >>> +}
> >>> +
> >>> +/*
> >>> + * omap4_dpll_dump_regs - dump out DPLL registers
> >>> + * @dpll_reg: DPLL to dump
> >>> + *
> >>> + * Dump out the contents of the registers for a DPLL. Called if a
> >>> + * restore for DPLL fails to lock.
> >>> + */
> >>> +static void omap4_dpll_dump_regs(struct omap4_dpll_regs *dpll_reg)
> >>> +{
> >>> +	pr_warn("%s: Unable to lock dpll %s[part=%x inst=%x]:\n",
> >>> +		__func__, dpll_reg->name, dpll_reg->mod_partition,
> >>> +		dpll_reg->mod_inst);
> >>> +	omap4_dpll_print_reg(dpll_reg, "clksel", &dpll_reg->clksel);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m2", &dpll_reg->div_m2);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m3", &dpll_reg->div_m3);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m4", &dpll_reg->div_m4);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m5", &dpll_reg->div_m5);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m6", &dpll_reg->div_m6);
> >>> +	omap4_dpll_print_reg(dpll_reg, "div_m7", &dpll_reg->div_m7);
> >>> +	omap4_dpll_print_reg(dpll_reg, "clkdcoldo", &dpll_reg->clkdcoldo);
> >>> +	omap4_dpll_print_reg(dpll_reg, "clkmode", &dpll_reg->clkmode);
> >>> +	omap4_dpll_print_reg(dpll_reg, "autoidle", &dpll_reg->autoidle);
> >>> +	if (dpll_reg->idlest.offset)
> >>> +		pr_warn("idlest - Address offset = 0x%08x, before val=0x%08x"
> >>> +			" after = 0x%08x\n", dpll_reg->idlest.offset,
> >>> +			dpll_reg->idlest.val,
> >>> +			omap4_dpll_read_reg(dpll_reg, &dpll_reg->idlest));
> >>
> >> Nit-pick ...
> >>
> >> 1. Same comments as above
> >> 2. Consider dropping "Address offset" altogether here as we print
> >> "idlest" the offset should be implied.
> >> 3. Also can we be consistent in our naming of "before val and "after"?
> >> For example, "val before =" and "val now = ".
> > 
> > Okay, I'll modify both prints slightly. Question though, do we want
> > these prints in the kernel in the first place? At least Tony has been
> > frowning upon register dumps in the kernel and this falls into that
> > category. What we could do is just to print the warning but drop the
> > register dumps out.
> 
> Thanks. Good question. If we are not seeing failures often, and I would
> hope not, probably ok to remove. However, I will let Tony comment here  ...

Well if the register dumps really help trace the issue and don't happen
continuously I'm fine with them.

Tony