Re: [PATCH] pinctrl: qcom: Add egpio feature support

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 30 Sep 22:34 PDT 2021, Rajendra Nayak wrote:

> 
> On 9/30/2021 8:25 PM, Bjorn Andersson wrote:
> > On Thu 30 Sep 02:46 PDT 2021, Rajendra Nayak wrote:
> > 
> > > 
> > > 
> > > On 9/21/2021 9:56 PM, Bjorn Andersson wrote:
> > > > On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > On 9/20/2021 6:14 AM, Bjorn Andersson wrote:
> > > > > > On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote:
> > > > > > 
> > > > > > > From: Prasad Sodagudi <psodagud@codeaurora.org>
> > > > > > > 
> > > > > > > egpio is a scheme which allows special power Island Domain IOs
> > > > > > > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
> > > > > > > TLMM functions with Island Domain functions.
> > > > > > > With this scheme, an IO can be controlled both by the cpu running
> > > > > > > linux and the Island processor. This provides great flexibility to
> > > > > > > re-purpose the Island IOs for regular TLMM usecases.
> > > > > > > 
> > > > > > > 2 new bits are added to ctl_reg, egpio_present is a read only bit
> > > > > > > which shows if egpio feature is available or not on a given gpio.
> > > > > > > egpio_enable is the read/write bit and only effective if egpio_present
> > > > > > > is 1. Once its set, the Island IO is controlled from Chip TLMM.
> > > > > > > egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
> > > > > > > 
> > > > > > > The support exists on most recent qcom SoCs, and we add support
> > > > > > > for sm8150/sm8250/sm8350 and sc7280 as part of this patch.
> > > > > > > 
> > > > > > 
> > > > > > I was under the impression that this feature would allow you to
> > > > > > repurpose pins for use either by the remote island or by apps.
> > > > > 
> > > > > thats right, you can repurpose the pins for usage by apps by setting
> > > > > the egpio_enable to 1, when set to 0 its owned by the island processor.
> > > > 
> > > > Good.
> > > > 
> > > > > > 
> > > > > > But if I understand your proposal, you check to see if the pin is
> > > > > > "egpio capable" for a pin and if so just sets the bit - muxing it to
> > > > > > apps (or the island?).
> > > > > 
> > > > > Right, so if there is a request for a egpio-capable pin, the driver
> > > > > flips the ownership. Are you suggesting having some kind of checks to determine
> > > > > who should own it?
> > > > > 
> > > > 
> > > > I see, I missed that nuance. So Linux will steal any pins that are
> > > > mentioned in DT. But that would mean that you're relying on someone else
> > > > to ensure that this bit is cleared for the other pins and you would not
> > > > be able to explicitly flip the state back to island mode in runtime.
> > > > 
> > > > I would prefer that this was more explicit.
> > > > 
> > > > > > It seems reasonable that this would be another pinmux state for these
> > > > > > pins, rather than just flipping them all in one or the other direction.
> > > > > 
> > > > > hmm, I don't understand. This is not a pinmux state, its a switch to decide
> > > > > the ownership.
> > > > 
> > > > But does it mux the pin to the island, or does it state that the island
> > > > is now in charge of the associated TLMM registers?
> > > 
> > > The island processor does not access the APPS TLMM register space, it has its
> > > own TLMM register space that it configures. APPS TLMM registers control its
> > > mux/conf settings and Island TLMM registers controls its mux/conf. So essentially
> > > there are 2 sets of registers to control the same pin.
> > > This bit is more like a top level mux which decides what register settings
> > > take affect.
> > > 
> > 
> > "One mux to rule them all" :)
> > 
> > When we switch this mux towards the Island TLMM, do we need to configure
> > the APPS TLMM in a particular way, or does the state of that not matter?
> 
> No APPS TLMM settings should be needed, the state of that does not matter.
> 
> > Would it be reasonable to say that when muxed towards the island the
> > apps should always be in gpio mux with some predetermined properties, to
> > save power?
> 
> No, the the register settings in APPS TLMM are nop/dont care when egpio_enable is 0.
> 

That's good.

> > To reiterate, as proposed, mentioning a egpio-capable pin in the apps
> > DTS will cause it to be muxed to the APSS TLMM. But I'm not convinced
> > that we don't have scenarios where one might want to dynamically mux the
> > pin between island and apss tlmm.
> > 
> > My suggestion is that even that it's two independent muxes controlled in
> > the apps tlmm, we'd express them in the same pinmux, i.e. we'd have
> > something like:
> > 
> > some-local-state {
> > 	pins = "gpio1";
> > 	function = "gpio";
> > 	output-high;
> > };
> 
> so this would set the function to gpio in the APPS TLMM and set the egpio_enable = 1?
> which was also what the original $SUBJECT patch did.
> 

Right, the effect for any of the today defined functions would be the
same as the patch you proposed.

> > some-remote-state {
> > 	pins = "gpio1";
> > 	function = "island"; /* or just egpio... ? */
> > };
> 
> Here we add a new function to the pin and that's used to set the egpio_enable to 0?
> 

Yes.

> > One case I imaging where this could be useful is to allow Linux to
> > configure a known state of pins when the island isn't running, from the
> > remoteproc driver and then flip it over to island mode before booting
> > the remote.
> 
> So we save power during boot up until the island processor comes up?
> So fwik when we boot linux its either configured to boot the island processor
> or not. Are you talking about some scenario where the island processor comes
> up on demand and goes down when not used?
> 

Perhaps I'm just paranoid, but I do like the idea of being able to
explicitly describe the "island configuration" in DT, rather than
relying on an implicit "if I don't define the node it's probably in
island mode".

I was in particular thinking about the case where the remote stops, but
I guess that should only happen very shortly before the remote is
restarted again.

The other thing that your proposed patch rely on is that all
egpio-capable pins are left with egpio_enable = 0 by the bootloader,
because the only way to flip a egpio_enable = 1 to a 0 is to recompile
and re-sign the bootloader.

Regards,
Bjorn