Re: [PATCH 3/4] irqchip: imgpdc: Set sys wake polarities to active high

James Hogan
2017-10-04T13:23:41Z

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

Hi Ed,

On Mon, Oct 02, 2017 at 10:55:58AM +0100, Ed Blake wrote:
>
> Set all sys wake polarities to active high during initial setup. The
> default is active low, which currently causes the 'flow_type' passed
> into the set_type function to be effectively inverted.
>
> Signed-off-by: Ed Blake <ed.blake@sondrel.com>
> ---
> drivers/irqchip/irq-imgpdc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
> index 22d8f8a..d1bcfef 100644
> --- a/drivers/irqchip/irq-imgpdc.c
> +++ b/drivers/irqchip/irq-imgpdc.c
> @@ -49,6 +49,8 @@
> #define PDC_IRQ_ROUTE_EXT_EN_WD 0x00000004
> #define PDC_IRQ_ROUTE_EXT_EN_IR 0x00000002
> #define PDC_IRQ_ROUTE_EXT_EN_RTC 0x00000001
> +#define PDC_SYS_WAKE_POL 0x00000020
> +#define PDC_SYS_WAKE_POL_SHIFT 5

I don't see this bit in the documentation for the PDC in TZ1090, so it
must be new in later versions of the IP. Maybe that can be mentioned in
the commit message or a code comment.

Does it need backporting to any stable branches (presumably v4.1+ for
Pistachio's sake)?

> #define PDC_SYS_WAKE_RESET 0x00000010
> #define PDC_SYS_WAKE_INT_MODE 0x0000000e
> #define PDC_SYS_WAKE_INT_MODE_SHIFT 1
> @@ -63,6 +65,9 @@
> #define PDC_SYS_WAKE_INT_CHANGE 0x6
> #define PDC_SYS_WAKE_INT_NONE 0x4
>
> +#define PDC_SYS_WAKE_ACTIVE_LOW 0x0
> +#define PDC_SYS_WAKE_ACTIVE_HIGH 0x1

Its a pity this HW isn't compatible with the older version which I
presume treated the INT_MODEs as active high but would have reported 0
in this bit. Never mind.

> +
> /**
> * struct pdc_intc_priv - private pdc interrupt data.
> * @nr_perips: Number of peripheral interrupt signals.
> @@ -335,8 +340,9 @@ static void pdc_intc_setup(struct pdc_intc_priv *priv)
> for (i = 0; i < priv->nr_syswakes; ++i) {
> /* set the IRQ mode to none */
> soc_sys_wake_regoff = PDC_SYS_WAKE_BASE + i*PDC_SYS_WAKE_STRIDE;
> - soc_sys_wake = PDC_SYS_WAKE_INT_NONE
> - << PDC_SYS_WAKE_INT_MODE_SHIFT;
> + soc_sys_wake =
> + PDC_SYS_WAKE_ACTIVE_HIGH << PDC_SYS_WAKE_POL_SHIFT |
> + PDC_SYS_WAKE_INT_NONE << PDC_SYS_WAKE_INT_MODE_SHIFT;

Looks reasonable. With a tweaked commit message or comment as mentioned
above:

Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James

> pdc_write(priv, soc_sys_wake_regoff, soc_sys_wake);
> }
> }
> --
> 1.9.1
>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]