diff mbox series

pinctrl: single: Add compatible for ti,am625-padconf

Message ID 20230805045554.786092-1-d-gole@ti.com
State Superseded
Headers show
Series pinctrl: single: Add compatible for ti,am625-padconf | expand

Commit Message

Dhruva Gole Aug. 5, 2023, 4:55 a.m. UTC 
From: Tony Lindgren <tony@atomide.com>

Add compatible for ti,am625-padconf to enable the use of wake-up events.

Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Dhruva Gole <d-gole@ti.com>
---

Base:
*****
tag: next-20230731 + below "depends on" patches

Depends on:
***********
[0] Update pinctrl-single to use yaml
[1] dt-binding: pinctrl-single: add am625 compatible

Links:
******
[0] https://lore.kernel.org/linux-omap/20230731061908.GG5194@atomide.com/T/
[1] https://lore.kernel.org/all/20230804050737.635186-1-d-gole@ti.com/

Cc: Nishanth Menon <nm@ti.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>

Link to this patch:
******************
https://lore.kernel.org/all/20230805045554.786092-1-d-gole@ti.com

 drivers/pinctrl/pinctrl-single.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Tony Lindgren Aug. 7, 2023, 7:07 a.m. UTC | #1 
* 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?

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

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

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.

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?

Regards,

Tony
Dhruva Gole Aug. 7, 2023, 8:09 a.m. UTC | #2 
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";
Tony Lindgren Aug. 7, 2023, 10:59 a.m. UTC | #3 
* Dhruva Gole <d-gole@ti.com> [230807 08:09]:
> 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?

Yes, but SoC specific defines needs to start with a SoC specific
prefix as multiple files may be included for various SoCs.

> 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 wake_en only gets set when some driver does request_irq() on
the wakeirq. No need to set them all.

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

No need to set all wake_en pads, just checking that if request_irq()
is done for some pin that does not have wake_en capability does not
cause eternal interrupts if a reserved bit is high all the time :)

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index f056923ecc98..3a2a9910f2ec 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1954,6 +1954,12 @@  static const struct pcs_soc_data pinctrl_single_am437x = {
 	.irq_status_mask = (1 << 30),   /* OMAP_WAKEUP_EVENT */
 };
 
+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 */
+};
+
 static const struct pcs_soc_data pinctrl_single = {
 };
 
@@ -1962,6 +1968,7 @@  static const struct pcs_soc_data pinconf_single = {
 };
 
 static const struct of_device_id pcs_of_match[] = {
+	{ .compatible = "ti,am625-padconf", .data = &pinctrl_single_am625 },
 	{ .compatible = "ti,omap3-padconf", .data = &pinctrl_single_omap_wkup },
 	{ .compatible = "ti,omap4-padconf", .data = &pinctrl_single_omap_wkup },
 	{ .compatible = "ti,omap5-padconf", .data = &pinctrl_single_omap_wkup },