[PATCH 0/2] ata: Introduce the AHCI_HFLAG_EDGE_IRQ_BROKEN in libahci and enabling it for ahci_xgene driver.

[PATCH 0/2] ata: Introduce the AHCI_HFLAG_EDGE_IRQ_BROKEN in libahci and enabling it for ahci_xgene driver.

[Suman Tripathi]

Happy new  year !!

Any comments on this patchset ?

On Thu, Dec 24, 2015 at 1:01 PM, Suman Tripathi <stripathi@apm.com> wrote:
> This patch set implements a workaround for an errate in the APM
> X-Gene SATA host controller with edge interrupt. The HOST_IRQ_STAT
> misses the edge interrupt from the PORT_IRQ_STAT when clearing the
> HOST_IRQ_STAT and reporting the PORT_IRQ_STAT happens in same clock
> cycle.
>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
>
> Suman Tripathi (2):
>   libahci: Implement the workaround to fix the missing of edge
>     interrupt for HOST_IRQ_STAT
>   ahci_xgene: Enable AHCI_HFLAG_EDGE_IRQ_BROKEN for APM X-Gen2 SATA
>     host controller
>
>  drivers/ata/ahci.h       |    2 ++
>  drivers/ata/ahci_xgene.c |    3 ++-
>  drivers/ata/libahci.c    |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 1 deletions(-)
>



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH 0/2] ata: Introduce the AHCI_HFLAG_EDGE_IRQ_BROKEN in libahci and enabling it for ahci_xgene driver.

[Tejun Heo]

On Mon, Jan 11, 2016 at 03:36:01PM +0530, Suman Tripathi wrote:
> This patch set implements a workaround for an errate in the APM
> X-Gene SATA host controller with edge interrupt. The HOST_IRQ_STAT
> misses the edge interrupt from the PORT_IRQ_STAT when clearing the
> HOST_IRQ_STAT and reporting the PORT_IRQ_STAT happens in same clock
> cycle.

Heh, I don't know why but I had to pull this posting out of spam too.
For some reason, gmail thought these are spams.

-- 
tejun

[PATCH 1/2] libahci: Implement the workaround to fix the missing of edge interrupt for HOST_IRQ_STAT

[Tejun Heo]

Hello,

On Mon, Jan 11, 2016 at 03:36:02PM +0530, Suman Tripathi wrote:
> Due to H/W errata, the HOST_IRQ_STAT register misses the edge interrupt
> when clearing the HOST_IRQ_STAT register and hardware reporting the
> PORT_IRQ_STAT register at the same clock cycle. As such, the
> algorithm below outlines the workaround.
> 
> 1. Read HOST_IRQ_STAT register and save the state.
> 2. Clear the HOST_IRQ_STAT register.
> 3. Read back the HOST_IRQ_STAT register.
> 4. If HOST_IRQ_STAT register equals to zero, then
>    traverse the rest of port's PORT_IRQ_STAT register
>    to check if an interrupt is triggered at that point else
>    go to step 6.
> 5. If PORT_IRQ_STAT register of rest ports is not equal to zero
>    then update the state of HOST_IRQ_STAT saved in step 1.
> 6. Handle port interrupts.
> 7. Exit
...
> +static void ahci_handle_broken_edge_irq(struct ata_host *host,
> +					u32 *irq_masked)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +	void __iomem *mmio = hpriv->mmio;
> +	unsigned int i, temp_irq_masked;
> +	struct ata_port *next_ap;
> +	void __iomem *port_mmio;
> +	int j;
> +
> +	if (!readl(mmio + HOST_IRQ_STAT)) {
> +		temp_irq_masked = *irq_masked;
> +
> +		for (i = 0; i < __sw_hweight32(hpriv->port_map);
> +		     i++) {
                     ^^^^^^
		     Doesn't this fit on the same line?

> +			if (*irq_masked & (1 << i)) {
> +				for (j = 0;
> +				     j < __sw_hweight32(hpriv->port_map);
> +				     j++) {

Heh, if (!COND) continue; can be your friend.

> +					if (i == j)
> +						continue;
> +
> +					next_ap = host->ports[j];
> +					port_mmio = ahci_port_base(next_ap);
> +					if (readl(port_mmio + PORT_IRQ_STAT))
> +						temp_irq_masked |= (1 << j);
> +				}
> +			}
> +		}
> +		*irq_masked = temp_irq_masked;
> +	}
> +
> +}

So, this is really specific to the controller.

>  static u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
>  {
> +	struct ahci_host_priv *hpriv = host->private_data;
>  	unsigned int i, handled = 0;
> 
> +	/*
> +	 * For hardware with broken edge trigger latch
> +	 * the HOST_IRQ_STAT register misses the edge interrupt
> +	 * when clearing of HOST_IRQ_STAT register and hardware
> +	 * reporting the PORT_IRQ_STAT register at the
> +	 * same clock cycle.
> +	 */
> +	if (hpriv->flags & AHCI_HFLAG_EDGE_IRQ_BROKEN)
> +		ahci_handle_broken_edge_irq(host, &irq_masked);
> +

And all it needs is doing some extra processing at the beginning of
port interrupt handling.  I think it'd be better to implement
ahci_xgene specific IRQ handler which does the special processing and
call the generic ahci_handle_port_intr().

Thanks.

-- 
tejun