[PATCH v3 06/11] thermal: armada: Add support for Armada CP110

[Gregory CLEMENT]

Hi Miquel,
 
 On jeu., déc. 14 2017, Miquel RAYNAL <miquel.raynal at free-electrons.com> wrote:

> On Thu, 14 Dec 2017 12:11:49 +0100
> Gregory CLEMENT <gregory.clement at free-electrons.com> wrote:
>
>> Hi Miquel,
>>  
>>  On jeu., déc. 14 2017, Miquel Raynal
>> <miquel.raynal at free-electrons.com> wrote:
>> 
>> > From: Baruch Siach <baruch at tkos.co.il>
>> >
>> > The CP110 component is integrated in the Armada 8k and 7k lines of
>> > processors.
>> >
>> > Signed-off-by: Baruch Siach <baruch at tkos.co.il>
>> > [<miquel.raynal at free-electrons.com>: renamed the register
>> > pointers]  
>> 
>> Actually you did more thant this see below
>> 
>> 
>> > Signed-off-by: Miquel Raynal <miquel.raynal at free-electrons.com>
>> > ---
>> >  drivers/thermal/armada_thermal.c | 30
>> > ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6
>> > deletions(-)
>> >
>> > diff --git a/drivers/thermal/armada_thermal.c
>> > b/drivers/thermal/armada_thermal.c index 279d01937bb8..f5c911524656
>> > 100644 --- a/drivers/thermal/armada_thermal.c
>> > +++ b/drivers/thermal/armada_thermal.c
>> > @@ -37,7 +37,6 @@
>> >  #define A375_UNIT_CONTROL_MASK		0x7
>> >  #define A375_READOUT_INVERT		BIT(15)
>> >  #define A375_HW_RESETn			BIT(8)
>> > -#define A380_HW_RESET			BIT(8)
>> >  
>> >  /* Legacy bindings */
>> >  #define LEGACY_CONTROL_MEM_LEN		0x4
>> > @@ -52,6 +51,10 @@
>> >  #define CONTROL0_TSEN_RESET		BIT(1)
>> >  #define CONTROL0_TSEN_ENABLE		BIT(2)
>> >  
>> > +/* EXT_TSEN refers to the external temperature sensors, out of the
>> > AP */ +#define CONTROL1_EXT_TSEN_SW_RESET	BIT(7)
>> > +#define CONTROL1_EXT_TSEN_HW_RESETn	BIT(8)  
>> You added or rename these values
>> 
>> > +
>> >  struct armada_thermal_data;
>> >  
>> >  /* Marvell EBU Thermal Sensor Dev Structure */
>> > @@ -153,11 +156,10 @@ static void armada380_init_sensor(struct
>> > platform_device *pdev, u32 reg = readl_relaxed(priv->control1);
>> >  
>> >  	/* Reset hardware once */
>> > -	if (!(reg & A380_HW_RESET)) {
>> > -		reg |= A380_HW_RESET;
>> > -		writel(reg, priv->control1);
>> > -		msleep(10);
>> > -	}
>> > +	reg |= CONTROL1_EXT_TSEN_HW_RESETn;
>> > +	reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
>> > +	writel(reg, priv->control1);  
>> 
>> And here you modified the behavior of this function.
>> Did you checked that it is valid for Armada 38x?
>
> There is nothing about it the documentation and anyway this register
> can be accessed later, so writing it is harmless ayway.
>
>> 
>> Given the comment we had, I thought we should not do anything if
>> CONTROL1_EXT_TSEN_HW_RESETn was not set.
>
> That is the opposite, if it is not set (ie. reset is active), you have
> to set it (reset is then disabled).

Actually I was concerned by the "once" for me it means "only one time",
but maybe it just meant it was useless to reset it again but not
harmful.

Gregory

>
>> 
>> By the way, if the new sequence is valid, this comment should be
>> removed or at least updated.
>
> That's right, I will in v4.
>
> Thanks for reviewing,
> Miquèl
>
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com