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

[James Hogan <james.hogan@imgtec.com>]

[-- 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 --]