| Message ID | 20221208093704.1151928-1-Basavaraj.Natikar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: amd: Add Z-state wake control bits | expand |
Hi Basavaraj, thanks for your patch! On Thu, Dec 8, 2022 at 10:37 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > GPIO registers include Bit 27 for WakeCntrlZ used to enable wake in > Z state. Hence add Z-state wake control bits to debugfs output to > debug and analyze Z-states problems. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Tested-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> The patch is overall fine, but as debug-only patch hardly urgent so it will wait until kernel v6.3. What I want to ask is how this bit: +#define WAKECNTRL_Z_OFF 27 Relates to this: static int amd_gpio_irq_set_wake(struct irq_data *d, unsigned int on) { u32 pin_reg; unsigned long flags; struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct amd_gpio *gpio_dev = gpiochip_get_data(gc); u32 wake_mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3); int err; (...) if this is some wake up control that is unrelated to GPIO, such that this can wake up on say I2C traffic on that pin or similar, is this something we actually need a new define for in include/linux/pinctrl/pinconf-generic.h so that you can also implement methods to manipulate it for real, like setting this from a pin control state or so? Yours, Linus Walleij
On 12/9/2022 3:51 AM, Linus Walleij wrote: > Hi Basavaraj, > > thanks for your patch! > > On Thu, Dec 8, 2022 at 10:37 AM Basavaraj Natikar > <Basavaraj.Natikar@amd.com> wrote: > >> GPIO registers include Bit 27 for WakeCntrlZ used to enable wake in >> Z state. Hence add Z-state wake control bits to debugfs output to >> debug and analyze Z-states problems. >> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >> Tested-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> > The patch is overall fine, but as debug-only patch hardly urgent > so it will wait until kernel v6.3. Sure > > What I want to ask is how this bit: > > +#define WAKECNTRL_Z_OFF 27 > > Relates to this: > > static int amd_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > { > u32 pin_reg; > unsigned long flags; > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct amd_gpio *gpio_dev = gpiochip_get_data(gc); > u32 wake_mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3); > int err; > (...) > > if this is some wake up control that is unrelated to GPIO, such > that this can wake up on say I2C traffic on that pin or similar, > is this something we actually need a new define for in > include/linux/pinctrl/pinconf-generic.h > so that you can also implement methods to manipulate it > for real, like setting this from a pin control state or so? we are not going to manipulate using pinconf_to_config_param explicitly, setting will be taken care by BIOS. We need only for debugging purpose to check bit status Thanks, -- Basavaraj
On Thu, Dec 8, 2022 at 10:37 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > GPIO registers include Bit 27 for WakeCntrlZ used to enable wake in > Z state. Hence add Z-state wake control bits to debugfs output to > debug and analyze Z-states problems. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Tested-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> Patch applied! Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index 6be896871718..e944003bb84a 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -218,6 +218,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) char *orientation; char debounce_value[40]; char *debounce_enable; + char *wake_cntrlz; for (bank = 0; bank < gpio_dev->hwbank_num; bank++) { unsigned int time = 0; @@ -305,6 +306,12 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc) wake_cntrl2 = " ∅"; seq_printf(s, "S4/S5 %s| ", wake_cntrl2); + if (pin_reg & BIT(WAKECNTRL_Z_OFF)) + wake_cntrlz = "⏰"; + else + wake_cntrlz = " ∅"; + seq_printf(s, "Z %s| ", wake_cntrlz); + if (pin_reg & BIT(PULL_UP_ENABLE_OFF)) { pull_up_enable = "+"; if (pin_reg & BIT(PULL_UP_SEL_OFF)) diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h index c8635998465d..81ae8319a1f0 100644 --- a/drivers/pinctrl/pinctrl-amd.h +++ b/drivers/pinctrl/pinctrl-amd.h @@ -42,6 +42,7 @@ #define OUTPUT_ENABLE_OFF 23 #define SW_CNTRL_IN_OFF 24 #define SW_CNTRL_EN_OFF 25 +#define WAKECNTRL_Z_OFF 27 #define INTERRUPT_STS_OFF 28 #define WAKE_STS_OFF 29