Re: [PATCH 2/6] soc/tegra: pmc: Fix early initialisation of PMC

[Thierry Reding]

On Wed, Jun 29, 2016 at 05:17:09PM +0100, Jon Hunter wrote:
> 
> On 28/06/16 11:38, Jon Hunter wrote:
> > During early initialisation, the available power partitions for a given
> > device is configured as well as the polarity of the PMC interrupt. Both
> > of which should only be configured if there is a valid device node for
> > the PMC device. This is because the soc data used for configuring the
> > power partitions is only available if a device node for the PMC is found
> > and the code to configure the interrupt polarity uses the device node
> > pointer directly.
> > 
> > Some early device-tree images may not have this device node and so fix
> > this by ensuring the device node pointer is valid when configuring these
> > items.
> > 
> > Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> > ---
> >  drivers/soc/tegra/pmc.c | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 52a9e9703668..2e031c4ad547 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -1550,27 +1550,29 @@ static int __init tegra_pmc_early_init(void)
> >  		return -ENXIO;
> >  	}
> >  
> > -	/* Create a bit-map of the available and valid partitions */
> > -	for (i = 0; i < pmc->soc->num_powergates; i++)
> > -		if (pmc->soc->powergates[i])
> > -			set_bit(i, pmc->powergates_available);
> > -
> >  	mutex_init(&pmc->powergates_lock);
> >  
> > -	/*
> > -	 * Invert the interrupt polarity if a PMC device tree node exists and
> > -	 * contains the nvidia,invert-interrupt property.
> > -	 */
> > -	invert = of_property_read_bool(np, "nvidia,invert-interrupt");
> > +	if (np) {
> > +		/* Create a bit-map of the available and valid partitions */
> > +		for (i = 0; i < pmc->soc->num_powergates; i++)
> > +			if (pmc->soc->powergates[i])
> > +				set_bit(i, pmc->powergates_available);
> >  
> > -	value = tegra_pmc_readl(PMC_CNTRL);
> > +		/*
> > +		 * Invert the interrupt polarity if a PMC device tree node
> > +		 * exists and contains the nvidia,invert-interrupt property.
> > +		 */
> > +		invert = of_property_read_bool(np, "nvidia,invert-interrupt");
> >  
> > -	if (invert)
> > -		value |= PMC_CNTRL_INTR_POLARITY;
> > -	else
> > -		value &= ~PMC_CNTRL_INTR_POLARITY;
> > +		value = tegra_pmc_readl(PMC_CNTRL);
> >  
> > -	tegra_pmc_writel(value, PMC_CNTRL);
> > +		if (invert)
> > +			value |= PMC_CNTRL_INTR_POLARITY;
> > +		else
> > +			value &= ~PMC_CNTRL_INTR_POLARITY;
> > +
> > +		tegra_pmc_writel(value, PMC_CNTRL);
> > +	}
> >  
> >  	return 0;
> >  }
> 
> By the way, the more I think about this, if there is no valid 'pmc'
> node in the device-tree blob, then I wonder if we should even bother
> mapping the pmc address space at all? The reason being, if there is
> no 'pmc' node then we cannot look-up the SoC data and so all the
> public PMC APIs are pretty useless AFAICT. I wonder if we should do
> something like ...

I think it's fine as-is. The PMC driver does more than just powergates
and it'd be useful to have the rest continue to work even on old DTBs.
Everything should already be properly guarded against this exception.

Thierry

Attachment: signature.asc
Description: PGP signature