[PATCH v3 2/2] ata: ahci_xgene: Add AHCI Support for 2nd HW version of APM X-Gene SoC AHCI SATA Host controller.

[PATCH v3 2/2] ata: ahci_xgene: Add AHCI Support for 2nd HW version of APM X-Gene SoC AHCI SATA Host controller.

[Julian Calaby]

Hi Suman,

On Mon, May 4, 2015 at 1:33 AM, Suman Tripathi <stripathi@apm.com> wrote:
> This patch enables full AHCI feature support for APM X-Gene SoC SATA
> host controller. The following errata's are removed:
>
> 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the
>                  IDENTIFY DEVICE PIO mode command")
> 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the
>                  ATA_CMD_SMART PIO mode command")
> 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to
>                  support PMP")
> 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP
>                  enumeration and discovery")
> 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the
>                  ATA_CMD_PACKET PIO mode command")
> 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene
>                 SoC AHCI SATA Host Controller driver")

Won't this mean that the 1st HW version won't work properly?

Thanks,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH v3 2/2] ata: ahci_xgene: Add AHCI Support for 2nd HW version of APM X-Gene SoC AHCI SATA Host controller.

[Suman Tripathi]

Hi Julian.

On Mon, May 4, 2015 at 5:56 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Suman,
>
> On Mon, May 4, 2015 at 1:33 AM, Suman Tripathi <stripathi@apm.com> wrote:
>> This patch enables full AHCI feature support for APM X-Gene SoC SATA
>> host controller. The following errata's are removed:
>>
>> 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the
>>                  IDENTIFY DEVICE PIO mode command")
>> 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the
>>                  ATA_CMD_SMART PIO mode command")
>> 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to
>>                  support PMP")
>> 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP
>>                  enumeration and discovery")
>> 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the
>>                  ATA_CMD_PACKET PIO mode command")
>> 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene
>>                 SoC AHCI SATA Host Controller driver")
>
> Won't this mean that the 1st HW version won't work properly?

No it doesn't mean that, It means whatever workaround we had in 1st HW
version has been
fixed in 2nd HW version.

>
> Thanks,
>
> --
> Julian Calaby
>
> Email: julian.calaby at gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH v3 2/2] ata: ahci_xgene: Add AHCI Support for 2nd HW version of APM X-Gene SoC AHCI SATA Host controller.

[Julian Calaby]

Hi Suman,

On Mon, May 4, 2015 at 12:51 PM, Suman Tripathi <stripathi@apm.com> wrote:
> Hi Julian.
>
> On Mon, May 4, 2015 at 5:56 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Suman,
>>
>> On Mon, May 4, 2015 at 1:33 AM, Suman Tripathi <stripathi@apm.com> wrote:
>>> This patch enables full AHCI feature support for APM X-Gene SoC SATA
>>> host controller. The following errata's are removed:
>>>
>>> 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the
>>>                  IDENTIFY DEVICE PIO mode command")
>>> 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the
>>>                  ATA_CMD_SMART PIO mode command")
>>> 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to
>>>                  support PMP")
>>> 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP
>>>                  enumeration and discovery")
>>> 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the
>>>                  ATA_CMD_PACKET PIO mode command")
>>> 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene
>>>                 SoC AHCI SATA Host Controller driver")
>>
>> Won't this mean that the 1st HW version won't work properly?
>
> No it doesn't mean that, It means whatever workaround we had in 1st HW
> version has been
> fixed in 2nd HW version.

I'm sorry, I read the description and assumed that these fixes were
being removed from the driver, hence breaking support for the 1st
hardware version.

Sorry for the confusion.

Thanks,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH v3 2/2] ata: ahci_xgene: Add AHCI Support for 2nd HW version of APM X-Gene SoC AHCI SATA Host controller.

[Suman Tripathi]

Hi Julian,

On Mon, May 4, 2015 at 8:52 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Suman,
>
> On Mon, May 4, 2015 at 12:51 PM, Suman Tripathi <stripathi@apm.com> wrote:
>> Hi Julian.
>>
>> On Mon, May 4, 2015 at 5:56 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>>> Hi Suman,
>>>
>>> On Mon, May 4, 2015 at 1:33 AM, Suman Tripathi <stripathi@apm.com> wrote:
>>>> This patch enables full AHCI feature support for APM X-Gene SoC SATA
>>>> host controller. The following errata's are removed:
>>>>
>>>> 1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the
>>>>                  IDENTIFY DEVICE PIO mode command")
>>>> 2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the
>>>>                  ATA_CMD_SMART PIO mode command")
>>>> 3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to
>>>>                  support PMP")
>>>> 4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP
>>>>                  enumeration and discovery")
>>>> 5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the
>>>>                  ATA_CMD_PACKET PIO mode command")
>>>> 6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene
>>>>                 SoC AHCI SATA Host Controller driver")
>>>
>>> Won't this mean that the 1st HW version won't work properly?
>>
>> No it doesn't mean that, It means whatever workaround we had in 1st HW
>> version has been
>> fixed in 2nd HW version.
>
> I'm sorry, I read the description and assumed that these fixes were
> being removed from the driver, hence breaking support for the 1st
> hardware version.
>
> Sorry for the confusion.

No probs ..

>
> Thanks,
>
> --
> Julian Calaby
>
> Email: julian.calaby at gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Sergei Shtylyov]

Hello.

On 5/3/2015 6:33 PM, Suman Tripathi wrote:

> This patch adds the support to handle HOST_IRQ_STAT as edge trigger
> latch.

> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>   drivers/ata/ahci.h    |  2 ++
>   drivers/ata/libahci.c | 19 +++++++++++++++++++
>   2 files changed, 21 insertions(+)

> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 71262e0..2df2237 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
[...]
> +++ b/drivers/ata/libahci.c
> @@ -1879,6 +1879,25 @@ static irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance)
>   	 */
>   	writel(irq_stat, mmio + HOST_IRQ_STAT);
>
> +	/*
> +	 * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT
> +	 * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts

    s/loose/lose/.

MBR, Sergei

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Tejun Heo]

On Sun, May 03, 2015 at 09:03:39PM +0530, Suman Tripathi wrote:
> This patch adds the support to handle HOST_IRQ_STAT as edge trigger
> latch.
...
> +	/*
> +	 * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT
> +	 * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts
> +	 * when interrupts are triggered from both ports. So handling of
> +	 * the residual interrupt is required.
> +	 */
> +	if (hpriv->flags & AHCI_HFLAG_EDGE_TRIG_IRQ) {
> +		for (i = 0; i < host->n_ports; i++) {
> +			struct ata_port *ap;
> +
> +			ap = host->ports[i];
> +			if (ap) {
> +				ahci_port_intr(ap);
> +				VPRINTK("Residual irq from port %u\n", i);
> +			}
> +			handled = 1;
> +		}
> +	}

This is kinda gross.  The right thing do is clearing irq stat
registers before handling the events, right?  That shouldn't be too
difficult to implement.  Create a separate set of irq functions which
clear irqs before processing rather than after.

Thanks.

-- 
tejun

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Suman Tripathi]

On Mon, May 4, 2015 at 9:17 PM, Tejun Heo <tj@kernel.org> wrote:
> On Sun, May 03, 2015 at 09:03:39PM +0530, Suman Tripathi wrote:
>> This patch adds the support to handle HOST_IRQ_STAT as edge trigger
>> latch.
> ...
>> +     /*
>> +      * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT
>> +      * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts
>> +      * when interrupts are triggered from both ports. So handling of
>> +      * the residual interrupt is required.
>> +      */
>> +     if (hpriv->flags & AHCI_HFLAG_EDGE_TRIG_IRQ) {
>> +             for (i = 0; i < host->n_ports; i++) {
>> +                     struct ata_port *ap;
>> +
>> +                     ap = host->ports[i];
>> +                     if (ap) {
>> +                             ahci_port_intr(ap);
>> +                             VPRINTK("Residual irq from port %u\n", i);
>> +                     }
>> +                     handled = 1;
>> +             }
>> +     }
>
> This is kinda gross.  The right thing do is clearing irq stat
> registers before handling the events, right?  That shouldn't be too
> difficult to implement.  Create a separate set of irq functions which
> clear irqs before processing rather than after.

AFAIK clearing host_irq_stat means we have handled port interrupts .
Now for our case we still have interrupts left because it didn't get
detected on
first ahci_port_intr.  So you  mean to handle that residual irq in the
next cycle (i mean next call intr handler ) ??

>
> Thanks.
>
> --
> tejun



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Tejun Heo]

Hello,

On Mon, May 04, 2015 at 10:13:11PM +0530, Suman Tripathi wrote:
> AFAIK clearing host_irq_stat means we have handled port interrupts .
> Now for our case we still have interrupts left because it didn't get
> detected on
> first ahci_port_intr.  So you  mean to handle that residual irq in the
> next cycle (i mean next call intr handler ) ??

Heh, I think we're talking past each other.  For level triggered IRQs,
the latched IRQ bits should be cleared after handling the events;
otherwise, the latched bits are gonna get set immediately as the
events are still pending.  Also, this doesn't lose any events as
they're level triggered latches - if any event is pending, the IRQ is
gonna be raised again.

Edge triggered latches are the other way around.  You should clear the
latches before actually handling and clearing the events.  The pending
events won't trigger the latches again as it's edge-triggered and the
events which happens after this irq handling starts won't get lost as
they'll be latched for the next round.

Thanks.

-- 
tejun

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Suman Tripathi]

Hi Tejun,

On Mon, May 4, 2015 at 11:56 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Mon, May 04, 2015 at 10:13:11PM +0530, Suman Tripathi wrote:
>> AFAIK clearing host_irq_stat means we have handled port interrupts .
>> Now for our case we still have interrupts left because it didn't get
>> detected on
>> first ahci_port_intr.  So you  mean to handle that residual irq in the
>> next cycle (i mean next call intr handler ) ??
>
> Heh, I think we're talking past each other.  For level triggered IRQs,
> the latched IRQ bits should be cleared after handling the events;
> otherwise, the latched bits are gonna get set immediately as the
> events are still pending.  Also, this doesn't lose any events as
> they're level triggered latches - if any event is pending, the IRQ is
> gonna be raised again.
>
> Edge triggered latches are the other way around.  You should clear the
> latches before actually handling and clearing the events.  The pending
> events won't trigger the latches again as it's edge-triggered and the
> events which happens after this irq handling starts won't get lost as
> they'll be latched for the next round.

Yeah it's very simple. The clearing of irq_stat needs to move up.
That's it. tried and it worked. Thanks a lot !!
Just a quick question : Why edge trigger handling is not yet present
in libahci yet? I mean no one has used it yet ??

>
> Thanks.
>
> --
> tejun



-- 
Thanks,
with regards,
Suman Tripathi

[PATCH v3 1/2] libahci: Add support to handle HOST_IRQ_STAT as edge trigger latch.

[Tejun Heo]

On Tue, May 05, 2015 at 11:25:14AM +0530, Suman Tripathi wrote:
> Yeah it's very simple. The clearing of irq_stat needs to move up.
> That's it. tried and it worked. Thanks a lot !!
> Just a quick question : Why edge trigger handling is not yet present
> in libahci yet? I mean no one has used it yet ??

Yeah, no other ahci controller uses edge triggered latches.  I think
ahci spec says irq is supposed to be level triggered.

Thanks.

-- 
tejun