[PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[Lennert Buytenhek]

For some reason, sata_mv doesn't clear interrupt status during init
when it's running on an SoC host adapter.  If the bootloader has
touched the SATA controller before starting Linux, Linux can end up
enabling the SATA interrupt with events pending, which will cause the
interrupt to be marked as spurious and then be disabled, which then
breaks all further accesses to the controller.

This patch makes the SoC path clear interrupt status on init like in
the non-SoC case.

Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
---
 drivers/ata/sata_mv.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index ad169ff..e829a3a 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, unsigned int board_idx)
 		writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
 	}
 
-	if (!IS_SOC(hpriv)) {
-		/* Clear any currently outstanding host interrupt conditions */
-		writelfl(0, mmio + hpriv->irq_cause_ofs);
+	/* Clear any currently outstanding host interrupt conditions */
+	writelfl(0, mmio + hpriv->irq_cause_ofs);
 
-		/* and unmask interrupt generation for host regs */
-		writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
+	/* and unmask interrupt generation for host regs */
+	writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
+
+	/*
+	 * enable only global host interrupts for now.
+	 * The per-port interrupts get done later as ports are set up.
+	 */
+	mv_set_main_irq_mask(host, 0, PCI_ERR);
 
-		/*
-		 * enable only global host interrupts for now.
-		 * The per-port interrupts get done later as ports are set up.
-		 */
-		mv_set_main_irq_mask(host, 0, PCI_ERR);
-	}
 done:
 	return rc;
 }
-- 
1.5.6.4

Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[Mark Lord]

Lennert Buytenhek wrote:
> For some reason, sata_mv doesn't clear interrupt status during init
> when it's running on an SoC host adapter.  If the bootloader has
> touched the SATA controller before starting Linux, Linux can end up
> enabling the SATA interrupt with events pending, which will cause the
> interrupt to be marked as spurious and then be disabled, which then
> breaks all further accesses to the controller.
> 
> This patch makes the SoC path clear interrupt status on init like in
> the non-SoC case.
> 
> Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
> ---
>  drivers/ata/sata_mv.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index ad169ff..e829a3a 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, unsigned int board_idx)
>  		writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
>  	}
>  
> -	if (!IS_SOC(hpriv)) {
> -		/* Clear any currently outstanding host interrupt conditions */
> -		writelfl(0, mmio + hpriv->irq_cause_ofs);
> +	/* Clear any currently outstanding host interrupt conditions */
> +	writelfl(0, mmio + hpriv->irq_cause_ofs);
>  
> -		/* and unmask interrupt generation for host regs */
> -		writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +	/* and unmask interrupt generation for host regs */
> +	writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +
> +	/*
> +	 * enable only global host interrupts for now.
> +	 * The per-port interrupts get done later as ports are set up.
> +	 */
> +	mv_set_main_irq_mask(host, 0, PCI_ERR);
>  
> -		/*
> -		 * enable only global host interrupts for now.
> -		 * The per-port interrupts get done later as ports are set up.
> -		 */
> -		mv_set_main_irq_mask(host, 0, PCI_ERR);
> -	}
>  done:
>  	return rc;
>  }


Looks fine to me -- Saeed Bishara did the SOC stuff, and Marvell refuses to show
me any documentation for it.  So if it works for you, then let's put it in there.
Or maybe get Saeed's sign-off first.

Cheers

Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[saeed bishara]

On Sun, Aug 24, 2008 at 6:04 AM, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
> For some reason, sata_mv doesn't clear interrupt status during init
> when it's running on an SoC host adapter.  If the bootloader has
> touched the SATA controller before starting Linux, Linux can end up
> enabling the SATA interrupt with events pending, which will cause the
> interrupt to be marked as spurious and then be disabled, which then
> breaks all further accesses to the controller.
>
> This patch makes the SoC path clear interrupt status on init like in
> the non-SoC case.
>
> Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
> ---
>  drivers/ata/sata_mv.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index ad169ff..e829a3a 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, unsigned int board_idx)
>                writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
>        }
>
> -       if (!IS_SOC(hpriv)) {
> -               /* Clear any currently outstanding host interrupt conditions */
> -               writelfl(0, mmio + hpriv->irq_cause_ofs);
> +       /* Clear any currently outstanding host interrupt conditions */
> +       writelfl(0, mmio + hpriv->irq_cause_ofs);
>
> -               /* and unmask interrupt generation for host regs */
> -               writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +       /* and unmask interrupt generation for host regs */
> +       writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +
> +       /*
> +        * enable only global host interrupts for now.
> +        * The per-port interrupts get done later as ports are set up.
> +        */
> +       mv_set_main_irq_mask(host, 0, PCI_ERR);
>
> -               /*
> -                * enable only global host interrupts for now.
> -                * The per-port interrupts get done later as ports are set up.
> -                */
> -               mv_set_main_irq_mask(host, 0, PCI_ERR);
> -       }
>  done:
>        return rc;
>  }
The patch looks fine.
Mark, I think that all the code that clears the cause registers should
be moved from mv_init_host to the reset_hc functions, which already
clears some of those register. for example, register 0x14 (host
controller cause register) get cleared twice, the first time at
mv5_reset_one_hc. also, the reset_hc functions better use the register
define instead of a hardcoded number when possible.


saeed

Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[Lennert Buytenhek]

On Tue, Aug 26, 2008 at 09:50:23AM -0400, Mark Lord wrote:

> >For some reason, sata_mv doesn't clear interrupt status during init
> >when it's running on an SoC host adapter.  If the bootloader has
> >touched the SATA controller before starting Linux, Linux can end up
> >enabling the SATA interrupt with events pending, which will cause the
> >interrupt to be marked as spurious and then be disabled, which then
> >breaks all further accesses to the controller.
> >
> >This patch makes the SoC path clear interrupt status on init like in
> >the non-SoC case.
> >
> >Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
> >---
> > drivers/ata/sata_mv.c |   21 ++++++++++-----------
> > 1 files changed, 10 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> >index ad169ff..e829a3a 100644
> >--- a/drivers/ata/sata_mv.c
> >+++ b/drivers/ata/sata_mv.c
> >@@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, 
> >unsigned int board_idx)
> > 		writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
> > 	}
> > 
> >-	if (!IS_SOC(hpriv)) {
> >-		/* Clear any currently outstanding host interrupt conditions 
> >*/
> >-		writelfl(0, mmio + hpriv->irq_cause_ofs);
> >+	/* Clear any currently outstanding host interrupt conditions */
> >+	writelfl(0, mmio + hpriv->irq_cause_ofs);
> > 
> >-		/* and unmask interrupt generation for host regs */
> >-		writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> >+	/* and unmask interrupt generation for host regs */
> >+	writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> >+
> >+	/*
> >+	 * enable only global host interrupts for now.
> >+	 * The per-port interrupts get done later as ports are set up.
> >+	 */
> >+	mv_set_main_irq_mask(host, 0, PCI_ERR);
> > 
> >-		/*
> >-		 * enable only global host interrupts for now.
> >-		 * The per-port interrupts get done later as ports are set 
> >up.
> >-		 */
> >-		mv_set_main_irq_mask(host, 0, PCI_ERR);
> >-	}
> > done:
> > 	return rc;
> > }
> 
> Looks fine to me -- Saeed Bishara did the SOC stuff, and Marvell
> refuses to show me any documentation for it.

Yeah, docs are often a problem (even internally).  However, this
particular bit has been opened up, and you can get a copy of the SoC
docs here:

	http://www.marvell.com/products/media/index.jsp

(Get the docs for the 5182, which is the only 5xxx series SoC that has
SATA built-in.)

Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[Martin Michlmayr]

* Lennert Buytenhek <buytenh@wantstofly.org> [2008-08-24 05:04]:
> For some reason, sata_mv doesn't clear interrupt status during init
> when it's running on an SoC host adapter.  If the bootloader has
> touched the SATA controller before starting Linux, Linux can end up
> enabling the SATA interrupt with events pending, which will cause the
> interrupt to be marked as spurious and then be disabled, which then
> breaks all further accesses to the controller.
> 
> This patch makes the SoC path clear interrupt status on init like in
> the non-SoC case.

What's the status of this patch?  Saeed said that the patch is correct [1]
and Mark asked for his Signed-off-by.  Saeed, can you add your
Signed-off-by, so this can go in?

[1] http://www.spinics.net/lists/linux-ide/msg25686.html

> Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
> ---
>  drivers/ata/sata_mv.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index ad169ff..e829a3a 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, unsigned int board_idx)
>  		writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
>  	}
>  
> -	if (!IS_SOC(hpriv)) {
> -		/* Clear any currently outstanding host interrupt conditions */
> -		writelfl(0, mmio + hpriv->irq_cause_ofs);
> +	/* Clear any currently outstanding host interrupt conditions */
> +	writelfl(0, mmio + hpriv->irq_cause_ofs);
>  
> -		/* and unmask interrupt generation for host regs */
> -		writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +	/* and unmask interrupt generation for host regs */
> +	writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
> +
> +	/*
> +	 * enable only global host interrupts for now.
> +	 * The per-port interrupts get done later as ports are set up.
> +	 */
> +	mv_set_main_irq_mask(host, 0, PCI_ERR);
>  
> -		/*
> -		 * enable only global host interrupts for now.
> -		 * The per-port interrupts get done later as ports are set up.
> -		 */
> -		mv_set_main_irq_mask(host, 0, PCI_ERR);
> -	}
>  done:
>  	return rc;
>  }
> -- 
> 1.5.6.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters

[Grant Grundler]

On Sat, Jan 10, 2009 at 10:41 AM, Martin Michlmayr <tbm@cyrius.com> wrote:
> * Lennert Buytenhek <buytenh@wantstofly.org> [2008-08-24 05:04]:
>> For some reason, sata_mv doesn't clear interrupt status during init
>> when it's running on an SoC host adapter.  If the bootloader has
>> touched the SATA controller before starting Linux, Linux can end up
>> enabling the SATA interrupt with events pending, which will cause the
>> interrupt to be marked as spurious and then be disabled, which then
>> breaks all further accesses to the controller.
>>
>> This patch makes the SoC path clear interrupt status on init like in
>> the non-SoC case.
>
> What's the status of this patch?  Saeed said that the patch is correct [1]
> and Mark asked for his Signed-off-by.  Saeed, can you add your
> Signed-off-by, so this can go in?

"Acked-by:" is already implied with Saeed's previous email:
    "The patch looks fine."

That should be sufficient.
This patch can be applied by jgarzik to libata tree.

hth,
grant

>
> [1] http://www.spinics.net/lists/linux-ide/msg25686.html
>
>> Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
>> ---
>>  drivers/ata/sata_mv.c |   21 ++++++++++-----------
>>  1 files changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>> index ad169ff..e829a3a 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -3131,19 +3131,18 @@ static int mv_init_host(struct ata_host *host, unsigned int board_idx)
>>               writelfl(0, hc_mmio + HC_IRQ_CAUSE_OFS);
>>       }
>>
>> -     if (!IS_SOC(hpriv)) {
>> -             /* Clear any currently outstanding host interrupt conditions */
>> -             writelfl(0, mmio + hpriv->irq_cause_ofs);
>> +     /* Clear any currently outstanding host interrupt conditions */
>> +     writelfl(0, mmio + hpriv->irq_cause_ofs);
>>
>> -             /* and unmask interrupt generation for host regs */
>> -             writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
>> +     /* and unmask interrupt generation for host regs */
>> +     writelfl(hpriv->unmask_all_irqs, mmio + hpriv->irq_mask_ofs);
>> +
>> +     /*
>> +      * enable only global host interrupts for now.
>> +      * The per-port interrupts get done later as ports are set up.
>> +      */
>> +     mv_set_main_irq_mask(host, 0, PCI_ERR);
>>
>> -             /*
>> -              * enable only global host interrupts for now.
>> -              * The per-port interrupts get done later as ports are set up.
>> -              */
>> -             mv_set_main_irq_mask(host, 0, PCI_ERR);
>> -     }
>>  done:
>>       return rc;
>>  }
>> --
>> 1.5.6.4
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Martin Michlmayr
> http://www.cyrius.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>