Re: [PATCH V3 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana <akashast@codeaurora.org>]

Hi Matthias,

On 4/7/2020 10:56 PM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Tue, Apr 07, 2020 at 05:04:17PM +0530, Akash Asthana wrote:
>> Hi Matthias,
>>
>>
>>>>    static int geni_se_probe(struct platform_device *pdev)
>>>>    {
>>>>    	struct device *dev = &pdev->dev;
>>>> @@ -845,6 +868,34 @@ static int geni_se_probe(struct platform_device *pdev)
>>>>    		}
>>>>    	}
>>>> +#ifdef CONFIG_SERIAL_EARLYCON
>>>> +	wrapper->to_core.path = devm_of_icc_get(dev, "qup-core");
>>>> +	if (IS_ERR(wrapper->to_core.path))
>>>> +		return PTR_ERR(wrapper->to_core.path);
>>>> +	/*
>>>> +	 * Put minmal BW request on core clocks on behalf of early console.
>>>> +	 * The vote will be removed earlycon exit function.
>>>> +	 *
>>>> +	 * Note: We are putting vote on each QUP wrapper instead only to which
>>>> +	 * earlycon is connected because QUP core clock of different wrapper
>>>> +	 * share same voltage domain. If core1 is put to 0, then core2 will
>>>> +	 * also run at 0, if not voted. Default ICC vote will be removed ASA
>>>> +	 * we touch any of the core clock.
>>>> +	 * core1 = core2 = max(core1, core2)
>>>> +	 */
>>> I don't really understand this part. According to the comment if we vote
>>> (let's say) for core2 but not for core1 then:
>>>
>>> core1: 0
>>> core2: GENI_DEFAULT_BW
>>>
>>> core1 = core2 = max(core1, core2)
>>>     or
>>> core1 = core2 = max(0, GENI_DEFAULT_BW)
>>>
>>> hence
>>>
>>> core1 = core2 = GENI_DEFAULT_BW
>>>
>>> What am I missing, why is it necessary to vote for both/all?
>> say core1 is for earlycon usecase
>>
>> There is common switch to control both the QUP core clock. I guess most
>> appropriate description would be     switch = max(vote_on_core1,
>> vote_on_core2) + default_vote.
>>
>> During early bootup, vote_on_core1 = 0, vote_on_core2 = 0;
>>
>> As earlycon was working even without voting it's core need because there was
>> some default vote present on the core switch by ICC during bootup.
>>
>> So if any child(say SPI) of other QUP wrapper resumed and suspended before
>> earlycon wrapper comes up. This will make core clock to run at zero and will
>> cause NOC issue because vote_on_core1 = 0, vote_on_core2 = 0; and it seems
>> default votes from core switch is removed  ASA it's voted on any core.
> Thanks for the explication!
>
> You are probably totally right, but for some reason my brain still resists
> to get it ...
>
> With the above my current interpretation is (assuming earlycon only votes on
> core1):
>
>                        core1   core2  default  switch
> early boot              0       0        1       1
> SPI resume (core2)      0       1        0       1
> SPI suspend (core2)     0       0        0       0
> earlycon init 		1	0        0       1
>
>
> What is wrong in the above table?
>
> Thanks for bearing with me :)
NP :)

I guess you meant QUP WRAPPER 1 probe by "earlycon init".

                       core1   core2  default  switch	Time
early boot              0       0        1       1	0s
SPI resume (core2)      0       1        0       1	3.2s
SPI suspend (core2)     0       0        0       0	3.3s
QUP WRAPPER 1(probe)	1	0        0       1	5s (say)

So switch is at 0 in time interval [3.3, 5] that will make core clock to run at 0.
If we use earlycon during this time interval it will reset the board.

Did above answered the query?

Regards,
Akash

>
> Matthias

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project