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

[Matthias Kaehlcke <mka@chromium.org>]

On Wed, Apr 08, 2020 at 05:08:01PM +0530, Akash Asthana wrote:
> 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?

now it finally made 'click', thanks :)