linux-kernel - Re: [PATCH] pinctrl: single: Add compatible for ti,am625-padconf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230807080922.t35yvyvrknjcriyr@dhruva>
Date:   Mon, 7 Aug 2023 13:39:22 +0530
From:   Dhruva Gole <d-gole@...com>
To:     Tony Lindgren <tony@...mide.com>
CC:     Nishanth Menon <nm@...com>,
        Linus Walleij <linus.walleij@...aro.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-omap@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH] pinctrl: single: Add compatible for ti,am625-padconf

On Aug 07, 2023 at 10:07:24 +0300, Tony Lindgren wrote:
> * Nishanth Menon <nm@...com> [230805 17:15]:
> > On 10:25-20230805, Dhruva Gole wrote:
> > > From: Tony Lindgren <tony@...mide.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@...com>