Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Aug 2023 13:39:22 +0530
From:   Dhruva Gole <d-gole@ti.com>
To:     Tony Lindgren <tony@atomide.com>
CC:     Nishanth Menon <nm@ti.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-omap@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
        Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [PATCH] pinctrl: single: Add compatible for ti,am625-padconf
Message-ID: <20230807080922.t35yvyvrknjcriyr@dhruva>
References: <20230805045554.786092-1-d-gole@ti.com>
 <20230805171508.schg4xquoa24klk5@october>
 <20230807070724.GN14799@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230807070724.GN14799@atomide.com>
Precedence: bulk

On Aug 07, 2023 at 10:07:24 +0300, Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [230805 17:15]:
> > On 10:25-20230805, Dhruva Gole wrote:
> > > From: Tony Lindgren <tony@atomide.com>
> > > +static const struct pcs_soc_data pinctrl_single_am625 = {
> > > +	.flags = PCS_QUIRK_SHARED_IRQ | PCS_CONTEXT_LOSS_OFF,
> > > +	.irq_enable_mask = (1 << 29),   /* WKUP_EN */
> > > +	.irq_status_mask = (1 << 30),   /* WKUP_EVT */
> > > +};
> > > +
> > 
> > Why cant we set this in the k3-pinctrl.h and set it once?

Do you mean that I set 1 << 29 and 30 as sort of macros in the
k3-pinctrl.h file and then include it in pinctrl-single.c?

Are we okay to #include a header from arch/arm64/boot/dts/ti?

> 
> Good idea to define the bit offsets k3-pinctrl.h instead of magic numbers
> here :)

If I understand what Nishanth is saying correctly, are we expected to
set the wake_en bit on every single K3 SoC's every single padconf reg?

I am a little sceptical with this approach, because what is people
_don't_ want to wakeup from certain pads? What would be the right way to
disable wakeup on those pads then?

> 
> > The event will not be generated until wakeup daisy chain is triggered
> > anyways.

Any voltage level shift can potentially trigger a daisychain and I don't
think that's really such a good idea?

> 
> Yup, and having that happen is enough to show the wake-up reason with
> grep wakeup /proc/interrupts :)
> 
> > Have you looked at all the padconf registers across devices to ensure
> > the WKUP_EN/EVT bits are present? daisy chain feature is used elsewhere
> > as well.

In my limited experience, I have only seen daisychain wakeups being
enabled on AM62x SOC. This is because this is one of the first K3
devices to implement deepsleep, and I think IO daisychain only applies for
wakeups in the case of deepsleep kind of scenarios.

> 
> The lack of bits at least earlier just meant that attempting to use a
> wake-up interrupt would just never trigger. Worth checking though.
> Dhruva, care to check if some padconf register have reserved bits for
> 29 and 30 that might be set high by default?

Sure, I could take a look, but setting wake_en on all pads still
doesn't feel right to me.

> 
> Regards,
> 
> Tony

To summarise, I don't think any other devices are using daisychain
atleast today, and even if there is possibility of using in future I
think the same compatible I have used here can be used to set wake_en
wherever applicable, for eg. whenever AM62A would want to use daisychain
it can use this quirk in it's DT node.

I believe that we shouldn't set every pad as daisychain enabled
otherwise in deepsleep it may result in unintended wakeups. And the way
I thought we can give this choice to the user is using wakeirq chained
interrupt along with this quirk,
compatible = "ti,am6-padconf";

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>