Re: [PATCH 2/6] clk: qcom: gdsc: Add support for gdscs with gds hw controller

[Rajendra Nayak]

On 12/01/2015 07:52 AM, Stephen Boyd wrote:
> On 11/26, Rajendra Nayak wrote:
>> @@ -58,30 +58,34 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en)
>>  {
>>  	int ret;
>>  	u32 val = en ? 0 : SW_COLLAPSE_MASK;
>> -	u32 check = en ? PWR_ON_MASK : 0;
>>  	unsigned long timeout;
>> +	unsigned int status_reg = sc->gdscr;
>>  
>>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
>>  	if (ret)
>>  		return ret;
>>  
>>  	timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
>> -	do {
>> -		ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> -		if (ret)
>> -			return ret;
>>  
>> -		if ((val & PWR_ON_MASK) == check)
>> +	if (sc->gds_hw_ctrl) {
>> +		status_reg = sc->gds_hw_ctrl;
>> +	/*
>> +	 * The gds hw controller asserts/de-asserts the status bit soon
>> +	 * after it receives a power on/off request from a master.
>> +	 * The controller then takes around 8 xo cycles to start its internal
>> +	 * state machine and update the status bit. During this time, the
>> +	 * status bit does not reflect the true status of the core.
>> +	 * Add a delay of 1 us between writing to the SW_COLLAPSE bit and
>> +	 * polling the status bit
>> +	 */
> 
> Please indent this correctly to the udelay indent level.

will do.

> 
>> +		udelay(1);
>> +	}
>> +
>> +	do {
>> +		if (gdsc_is_enabled(sc, status_reg) == en)
>>  			return 0;
>>  	} while (time_before(jiffies, timeout));
>>  
>> -	ret = regmap_read(sc->regmap, sc->gdscr, &val);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if ((val & PWR_ON_MASK) == check)
>> -		return 0;
>> -
> 
> This opens a bug where we timeout and then the status bit changes
> after the timeout. One more check is good and should stay. We
> could also change this to ktime instead of jiffies. That would be
> a good improvement.

If the status bit does change after timeout maybe the timeout isn't
really enough and needs to be increased?

> 
>>  	return -ETIMEDOUT;
>>  }
>>  
>> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc)
>>  {
>>  	u32 mask, val;
>>  	int on, ret;
>> +	unsigned int reg;
>>  
>>  	/*
>>  	 * Disable HW trigger: collapse/restore occur based on registers writes.
>> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc)
>>  			return ret;
>>  	}
>>  
>> -	on = gdsc_is_enabled(sc);
>> +	reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr;
>> +	on = gdsc_is_enabled(sc, reg);
> 
> If the gdsc is voteable, then we need to make sure that the vote
> is from us when we boot up. Otherwise the kernel may think that
> the gdsc is enabled, but it gets turned off by some other master
> later on. I don't know if this causes some sort of problem for
> the power domain framework, but we can't rely on the status bit
> unless we're sure that we've actually set the register to enable
> it. In the normal enable/disable path we'll always know we set
> the register, so this really only matters once when we boot up.

right, thanks for catching this. However if we vote for a votable
GDSC just because its ON at boot (due to someone else having voted)
we won't ever remove the vote keeping it always enabled.

I think a safe way would be to consider all votable gdscs for which
*we* haven't voted explicitly to be disabled at boot?

> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/