* Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters
2008-08-24 3:04 [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters Lennert Buytenhek
@ 2008-08-26 13:50 ` Mark Lord
2008-08-28 20:04 ` Lennert Buytenhek
2008-08-28 10:28 ` saeed bishara
2009-01-10 18:41 ` Martin Michlmayr
2 siblings, 1 reply; 6+ messages in thread
From: Mark Lord @ 2008-08-26 13:50 UTC (permalink / raw)
To: Lennert Buytenhek; +Cc: Saeed Bishara, linux-ide, Jeff Garzik
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters
2008-08-26 13:50 ` Mark Lord
@ 2008-08-28 20:04 ` Lennert Buytenhek
0 siblings, 0 replies; 6+ messages in thread
From: Lennert Buytenhek @ 2008-08-28 20:04 UTC (permalink / raw)
To: Mark Lord; +Cc: Saeed Bishara, linux-ide, Jeff Garzik
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.)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters
2008-08-24 3:04 [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters Lennert Buytenhek
2008-08-26 13:50 ` Mark Lord
@ 2008-08-28 10:28 ` saeed bishara
2009-01-10 18:41 ` Martin Michlmayr
2 siblings, 0 replies; 6+ messages in thread
From: saeed bishara @ 2008-08-28 10:28 UTC (permalink / raw)
To: Lennert Buytenhek, Mark Lord; +Cc: Saeed Bishara, linux-ide, Jeff Garzik
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters
2008-08-24 3:04 [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters Lennert Buytenhek
2008-08-26 13:50 ` Mark Lord
2008-08-28 10:28 ` saeed bishara
@ 2009-01-10 18:41 ` Martin Michlmayr
2009-01-12 21:27 ` Grant Grundler
2 siblings, 1 reply; 6+ messages in thread
From: Martin Michlmayr @ 2009-01-10 18:41 UTC (permalink / raw)
To: Lennert Buytenhek; +Cc: Saeed Bishara, linux-ide, Mark Lord, Jeff Garzik
* 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/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH,RFC] sata_mv: don't avoid clearing interrupt status on SoC host adapters
2009-01-10 18:41 ` Martin Michlmayr
@ 2009-01-12 21:27 ` Grant Grundler
0 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2009-01-12 21:27 UTC (permalink / raw)
To: Martin Michlmayr
Cc: Lennert Buytenhek, Saeed Bishara, linux-ide, Mark Lord,
Jeff Garzik
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread