| Message ID | 20230805045554.786092-1-d-gole@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: single: Add compatible for ti,am625-padconf | expand |
On 10:25-20230805, Dhruva Gole wrote: > 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(+) > > 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 */ > +}; > + Why cant we set this in the k3-pinctrl.h and set it once? The event will not be generated until wakeup daisy chain is triggered anyways. 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. > 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 }, > -- > 2.34.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
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";
* 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 --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 },