[PATCH v3 04/11] thermal: armada: Rationalize register accesses

[miquel.raynal@free-electrons.com (Miquel RAYNAL)]

On Fri, 15 Dec 2017 10:56:22 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miqu?l,
> 
> On Thu, Dec 14, 2017 at 11:30:04AM +0100, Miquel Raynal wrote:
> > Bindings were incomplete for a long time by only exposing one of
> > the two available control registers. To ease the migration to the
> > full bindings (already in use for the Armada 375 SoC), rename the
> > pointers for clarification. This way, it will only be needed to add
> > another pointer to access the other control register when the time
> > comes.
> > 
> > This avoids dangerous situations where the offset 0 of the control
> > area can be either one register or the other depending on the
> > bindings used. After this change, device trees of other SoCs could
> > be migrated to the "full" bindings if they may benefit from
> > features from the unaccessible register, without any change in the
> > driver.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/thermal/armada_thermal.c | 86
> > +++++++++++++++++++++++++--------------- 1 file changed, 55
> > insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 26698f2d3ca7..e5b184cee79b
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -39,12 +39,21 @@
> >  #define A375_HW_RESETn			BIT(8)
> >  #define A380_HW_RESET			BIT(8)
> >  
> > +/* Legacy bindings */
> > +#define LEGACY_CONTROL_MEM_LEN		0x4
> > +
> > +/* Current bindings with the 2 control registers under the same
> > memory area */ +#define LEGACY_CONTROL1_OFFSET		0x0
> > +#define CONTROL0_OFFSET			0x0
> > +#define CONTROL1_OFFSET			0x4
> > +
> >  struct armada_thermal_data;
> >  
> >  /* Marvell EBU Thermal Sensor Dev Structure */
> >  struct armada_thermal_priv {
> > -	void __iomem *sensor;
> > -	void __iomem *control;
> > +	void __iomem *status;
> > +	void __iomem *control0;
> > +	void __iomem *control1;  
> 
> The 'status' -> 'sensor' rename is not mentioned in the commit log.
> I'd say it is a matter for a separate patch.

Sure, I'll split.

> 
> Otherwise, good cleanup.

Thanks!

Miqu?l

> 
> baruch
> 
> >  	struct armada_thermal_data *data;
> >  };
> >  
> > @@ -71,45 +80,45 @@ struct armada_thermal_data {
> >  static void armadaxp_init_sensor(struct platform_device *pdev,
> >  				 struct armada_thermal_priv *priv)
> >  {
> > -	unsigned long reg;
> > +	u32 reg;
> >  
> > -	reg = readl_relaxed(priv->control);
> > +	reg = readl_relaxed(priv->control1);
> >  	reg |= PMU_TDC0_OTF_CAL_MASK;
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	/* Reference calibration value */
> >  	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> >  	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	/* Reset the sensor */
> > -	reg = readl_relaxed(priv->control);
> > -	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > +	reg = readl_relaxed(priv->control1);
> > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
> >  
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	/* Enable the sensor */
> > -	reg = readl_relaxed(priv->sensor);
> > +	reg = readl_relaxed(priv->status);
> >  	reg &= ~PMU_TM_DISABLE_MASK;
> > -	writel(reg, priv->sensor);
> > +	writel(reg, priv->status);
> >  }
> >  
> >  static void armada370_init_sensor(struct platform_device *pdev,
> >  				  struct armada_thermal_priv *priv)
> >  {
> > -	unsigned long reg;
> > +	u32 reg;
> >  
> > -	reg = readl_relaxed(priv->control);
> > +	reg = readl_relaxed(priv->control1);
> >  	reg |= PMU_TDC0_OTF_CAL_MASK;
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	/* Reference calibration value */
> >  	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> >  	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	reg &= ~PMU_TDC0_START_CAL_MASK;
> > -	writel(reg, priv->control);
> > +	writel(reg, priv->control1);
> >  
> >  	msleep(10);
> >  }
> > @@ -117,37 +126,37 @@ static void armada370_init_sensor(struct
> > platform_device *pdev, static void armada375_init_sensor(struct
> > platform_device *pdev, struct armada_thermal_priv *priv)
> >  {
> > -	unsigned long reg;
> > +	u32 reg;
> >  
> > -	reg = readl(priv->control + 4);
> > +	reg = readl(priv->control1);
> >  	reg &= ~(A375_UNIT_CONTROL_MASK <<
> > A375_UNIT_CONTROL_SHIFT); reg &= ~A375_READOUT_INVERT;
> >  	reg &= ~A375_HW_RESETn;
> >  
> > -	writel(reg, priv->control + 4);
> > +	writel(reg, priv->control1);
> >  	msleep(20);
> >  
> >  	reg |= A375_HW_RESETn;
> > -	writel(reg, priv->control + 4);
> > +	writel(reg, priv->control1);
> >  	msleep(50);
> >  }
> >  
> >  static void armada380_init_sensor(struct platform_device *pdev,
> >  				  struct armada_thermal_priv *priv)
> >  {
> > -	unsigned long reg = readl_relaxed(priv->control);
> > +	u32 reg = readl_relaxed(priv->control1);
> >  
> >  	/* Reset hardware once */
> >  	if (!(reg & A380_HW_RESET)) {
> >  		reg |= A380_HW_RESET;
> > -		writel(reg, priv->control);
> > +		writel(reg, priv->control1);
> >  		msleep(10);
> >  	}
> >  }
> >  
> >  static bool armada_is_valid(struct armada_thermal_priv *priv)
> >  {
> > -	unsigned long reg = readl_relaxed(priv->sensor);
> > +	u32 reg = readl_relaxed(priv->status);
> >  
> >  	return reg & priv->data->is_valid_bit;
> >  }
> > @@ -156,7 +165,7 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, int *temp)
> >  {
> >  	struct armada_thermal_priv *priv = thermal->devdata;
> > -	unsigned long reg;
> > +	u32 reg;
> >  	unsigned long m, b, div;
> >  
> >  	/* Valid check */
> > @@ -166,7 +175,7 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, return -EIO;
> >  	}
> >  
> > -	reg = readl_relaxed(priv->sensor);
> > +	reg = readl_relaxed(priv->status);
> >  	reg = (reg >> priv->data->temp_shift) &
> > priv->data->temp_mask; 
> >  	/* Get formula coeficients */
> > @@ -253,6 +262,7 @@ MODULE_DEVICE_TABLE(of,
> > armada_thermal_id_table); 
> >  static int armada_thermal_probe(struct platform_device *pdev)
> >  {
> > +	void __iomem *control = NULL;
> >  	struct thermal_zone_device *thermal;
> >  	const struct of_device_id *match;
> >  	struct armada_thermal_priv *priv;
> > @@ -267,14 +277,28 @@ static int armada_thermal_probe(struct
> > platform_device *pdev) return -ENOMEM;
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> > -	if (IS_ERR(priv->sensor))
> > -		return PTR_ERR(priv->sensor);
> > +	priv->status = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv->status))
> > +		return PTR_ERR(priv->status);
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > -	priv->control = devm_ioremap_resource(&pdev->dev, res);
> > -	if (IS_ERR(priv->control))
> > -		return PTR_ERR(priv->control);
> > +	control = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(control))
> > +		return PTR_ERR(control);
> > +
> > +	/*
> > +	 * Legacy DT bindings only described "control1" register
> > (also referred
> > +	 * as "control MSB" on old documentation). New bindings
> > cover
> > +	 * "control0/control LSB" and "control1/control MSB"
> > registers within
> > +	 * the same resource, which is then of size 8 instead of 4.
> > +	 */
> > +	if ((res->end - res->start) == LEGACY_CONTROL_MEM_LEN) {
> > +		/* ->control0 unavailable in this configuration */
> > +		priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > +	} else {
> > +		priv->control0 = control + CONTROL0_OFFSET;
> > +		priv->control1 = control + CONTROL1_OFFSET;
> > +	}
> >  
> >  	priv->data = (struct armada_thermal_data *)match->data;
> >  	priv->data->init_sensor(pdev, priv);  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com