diff mbox

[V2] AHCI: Workaround for ThunderX Errata#22536

Message ID 1455319230-30201-1-git-send-email-tchalamarla@caviumnetworks.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

tchalamarla@caviumnetworks.com Feb. 12, 2016, 11:20 p.m. UTC
From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>

Due to Errata in ThunderX, HOST_IRQ_STAT should be
cleared before leaving the interrupt handler.
The patch attempts to satisfy the need.

Changes from V1:
	- Rebased on top of libata/for-4.6
        - Moved ThunderX intr handler to new file

Signed-off-by: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
---
 drivers/ata/Makefile        |  2 +-
 drivers/ata/ahci.c          |  3 ++
 drivers/ata/ahci.h          |  1 +
 drivers/ata/ahci_thunderx.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ata/ahci_thunderx.c

Comments

Sergei Shtylyov Feb. 13, 2016, 4:54 p.m. UTC | #1
Hello.

On 2/13/2016 2:20 AM, tchalamarla@caviumnetworks.com wrote:

> From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
>
> Due to Errata in ThunderX, HOST_IRQ_STAT should be
> cleared before leaving the interrupt handler.
> The patch attempts to satisfy the need.
>
> Changes from V1:
> 	- Rebased on top of libata/for-4.6
>          - Moved ThunderX intr handler to new file
>
> Signed-off-by: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
> ---
>   drivers/ata/Makefile        |  2 +-
>   drivers/ata/ahci.c          |  3 ++
>   drivers/ata/ahci.h          |  1 +
>   drivers/ata/ahci_thunderx.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 78 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/ata/ahci_thunderx.c
>
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 1857952..a36e70d 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -2,7 +2,7 @@
>   obj-$(CONFIG_ATA)		+= libata.o
>
>   # non-SFF interface
> -obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
> +obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o ahci_thunderx.o
>   obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
>   obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
>   obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 546a369..76e310e 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1560,6 +1560,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (ahci_broken_devslp(pdev))
>   		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
>
> +	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
> +		ahci_thunderx_init(&pdev->dev, hpriv);
> +
>   	/* save initial config */
>   	ahci_pci_save_initial_config(pdev, hpriv);
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 167ba7e..77ae20d 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -425,6 +425,7 @@ void ahci_print_info(struct ata_host *host, const char *scc_s);
>   int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht);
>   void ahci_error_handler(struct ata_port *ap);
>   u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
> +void ahci_thunderx_init(struct device *dev, struct ahci_host_priv *hpriv);
>
>   static inline void __iomem *__ahci_port_base(struct ata_host *host,
>   					     unsigned int port_no)
> diff --git a/drivers/ata/ahci_thunderx.c b/drivers/ata/ahci_thunderx.c
> new file mode 100644
> index 0000000..223e170
> --- /dev/null
> +++ b/drivers/ata/ahci_thunderx.c
> @@ -0,0 +1,73 @@
> +/*
> + * SATA glue for Cavium Thunder SOCs.
> + *
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2010-2016 Cavium Networks
> + *
> + */
> +
> +#include <linux/module.h>
> +#include "ahci.h"
> +#include "libata.h"
> +
> +static irqreturn_t ahci_thunderx_irq_intr(int irq, void *dev_instance)

    *_irq_intr() is a bit tautological.

> +{
> +	struct ata_host *host = dev_instance;
> +	struct ahci_host_priv *hpriv;
> +	unsigned int rc = 0;
> +	void __iomem *mmio;
> +	u32 irq_stat, irq_masked;
> +	unsigned int handled = 1;
> +
> +	VPRINTK("ENTER\n");
> +
> +	hpriv = host->private_data;
> +	mmio = hpriv->mmio;

    Why not do this in the initializers?

> +
> +	/* sigh.  0xffffffff is a valid return from h/w */
> +	irq_stat = readl(mmio + HOST_IRQ_STAT);
> +	if (!irq_stat)
> +		return IRQ_NONE;
> +redo:

    Closer to the next statement, please.

> +
> +	irq_masked = irq_stat & hpriv->port_map;
> +
> +	spin_lock(&host->lock);
> +
> +	rc = ahci_handle_port_intr(host, irq_masked);
> +

    No need for empty line here.

> +	if (!rc)
> +		handled = 0;

    Doesn't this override the valid result in case of looping?

> +
> +	writel(irq_stat, mmio + HOST_IRQ_STAT);
> +
> +	/* Due to ERRATA#22536, ThunderX need to handle
> +	 * HOST_IRQ_STAT differently.
> +	 * Work around is to make sure all pending IRQs
> +	 * are served before leaving handler
> +	 */
> +	irq_stat = readl(mmio + HOST_IRQ_STAT);
> +
> +	spin_unlock(&host->lock);
> +
> +	if (irq_stat)
> +		goto redo;

    You can have a *do*/*while* loop, no need for *goto*.

> +
> +	VPRINTK("EXIT\n");
> +
> +	return IRQ_RETVAL(handled);
> +}
> +
> +void ahci_thunderx_init(struct device *dev, struct ahci_host_priv *hpriv)
> +{
> +	hpriv->irq_handler = ahci_thunderx_irq_intr;
> +}
> +EXPORT_SYMBOL_GPL(ahci_thunderx_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cavium, Inc. <support@cavium.com>");
> +MODULE_DESCRIPTION("Cavium Inc. ThunderX sata config.");

MBR, Sergei

--
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
Tejun Heo Feb. 14, 2016, 5:01 p.m. UTC | #2
Hello,

On Fri, Feb 12, 2016 at 03:20:30PM -0800, tchalamarla@caviumnetworks.com wrote:
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 546a369..76e310e 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1560,6 +1560,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ahci_broken_devslp(pdev))
>  		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
>  
> +	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
> +		ahci_thunderx_init(&pdev->dev, hpriv);

So, this would make ahci fail to build if thunderx is not configured.
Maybe we should add an additional callback to update hpriv.

>  	/* save initial config */
>  	ahci_pci_save_initial_config(pdev, hpriv);
>  
> diff --git a/drivers/ata/ahci_thunderx.c b/drivers/ata/ahci_thunderx.c

This driver isn't upstream yet, right?  Maybe just fold this change
together?

Thanks.
tchalamarla@caviumnetworks.com Feb. 15, 2016, 3:36 a.m. UTC | #3
On 02/14/2016 09:01 AM, Tejun Heo wrote:
> Hello,
>
> On Fri, Feb 12, 2016 at 03:20:30PM -0800, tchalamarla@caviumnetworks.com wrote:
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 546a369..76e310e 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1560,6 +1560,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   	if (ahci_broken_devslp(pdev))
>>   		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
>>
>> +	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
>> +		ahci_thunderx_init(&pdev->dev, hpriv);
>
> So, this would make ahci fail to build if thunderx is not configured.
> Maybe we should add an additional callback to update hpriv.
>
>>   	/* save initial config */
>>   	ahci_pci_save_initial_config(pdev, hpriv);
>>
>> diff --git a/drivers/ata/ahci_thunderx.c b/drivers/ata/ahci_thunderx.c
>
> This driver isn't upstream yet, right?  Maybe just fold this change
> together?
>
There is no need for special Driver, AHCI is sufficient for ThunderX, 
the file only contains this interrupt handler,
is it preferable if this interrupt handler in libahci.c with others, 
instead of separate file?

Thanks,
Tirumalesh.

> Thanks.
>
--
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
Tejun Heo Feb. 15, 2016, 6:30 p.m. UTC | #4
Hello, Tirumalesh.

On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
> There is no need for special Driver, AHCI is sufficient for ThunderX, the
> file only contains this interrupt handler,
> is it preferable if this interrupt handler in libahci.c with others, instead
> of separate file?

Yeap, just fold it in ahci.c with surrounding #ifdef guard.

Thanks.
Robert Richter Feb. 16, 2016, 2:42 p.m. UTC | #5
On 15.02.16 13:30:41, Tejun Heo wrote:
> On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
> > There is no need for special Driver, AHCI is sufficient for ThunderX, the
> > file only contains this interrupt handler,
> > is it preferable if this interrupt handler in libahci.c with others, instead
> > of separate file?
> 
> Yeap, just fold it in ahci.c with surrounding #ifdef guard.

Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
kconfig entry for this to arch/arm64/Kconfig.

-Robert
--
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
tchalamarla@caviumnetworks.com Feb. 16, 2016, 7:14 p.m. UTC | #6
On 02/16/2016 06:42 AM, Robert Richter wrote:
> On 15.02.16 13:30:41, Tejun Heo wrote:
>> On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
>>> There is no need for special Driver, AHCI is sufficient for ThunderX, the
>>> file only contains this interrupt handler,
>>> is it preferable if this interrupt handler in libahci.c with others, instead
>>> of separate file?
>>
>> Yeap, just fold it in ahci.c with surrounding #ifdef guard.
>
> Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
> kconfig entry for this to arch/arm64/Kconfig.
>
Are you sure, this is not a workaround that is based on alternative 
framework rather on pci device and vendor

do you think CONFIG_ARCH_THUNDER a good alternative?



> -Robert
>
--
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
David Daney Feb. 16, 2016, 7:38 p.m. UTC | #7
On 02/16/2016 11:14 AM, Tirumalesh Chalamarla wrote:
>
>
> On 02/16/2016 06:42 AM, Robert Richter wrote:
>> On 15.02.16 13:30:41, Tejun Heo wrote:
>>> On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
>>>> There is no need for special Driver, AHCI is sufficient for
>>>> ThunderX, the
>>>> file only contains this interrupt handler,
>>>> is it preferable if this interrupt handler in libahci.c with others,
>>>> instead
>>>> of separate file?
>>>
>>> Yeap, just fold it in ahci.c with surrounding #ifdef guard.
>>
>> Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
>> kconfig entry for this to arch/arm64/Kconfig.
>>
> Are you sure, this is not a workaround that is based on alternative
> framework rather on pci device and vendor
>
> do you think CONFIG_ARCH_THUNDER a good alternative?

No.  CONFIG_ARCH_THUNDER should be removed all together.

Grouping a bunch of unrelated features under a single config variable 
creates a very brittle system.  What are you going to do when a new 
hardware revision is released?  Create CONFIG_ARCH_THUNDER2?  Which one 
of these two would you select if building a kernel?  It is a choice that 
we don't want to force users (kernel builders) to have to waste mental 
energy on.

Instead, let's try to make things work out of the box without having to 
set a bunch of random config variables.

If a generic arm64 kernel won't get too bloated, I would suggest just 
enabling the compilation of the code unconditionally (at least for 
arm64).  The use of the code would still be gated by the PCI version 
probe that is part of the patch.

David Daney

--
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
tchalamarla@caviumnetworks.com Feb. 16, 2016, 7:50 p.m. UTC | #8
On 02/16/2016 11:38 AM, David Daney wrote:
> On 02/16/2016 11:14 AM, Tirumalesh Chalamarla wrote:
>>
>>
>> On 02/16/2016 06:42 AM, Robert Richter wrote:
>>> On 15.02.16 13:30:41, Tejun Heo wrote:
>>>> On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
>>>>> There is no need for special Driver, AHCI is sufficient for
>>>>> ThunderX, the
>>>>> file only contains this interrupt handler,
>>>>> is it preferable if this interrupt handler in libahci.c with others,
>>>>> instead
>>>>> of separate file?
>>>>
>>>> Yeap, just fold it in ahci.c with surrounding #ifdef guard.
>>>
>>> Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
>>> kconfig entry for this to arch/arm64/Kconfig.
>>>
>> Are you sure, this is not a workaround that is based on alternative
>> framework rather on pci device and vendor
>>
>> do you think CONFIG_ARCH_THUNDER a good alternative?
>
> No.  CONFIG_ARCH_THUNDER should be removed all together.
>
> Grouping a bunch of unrelated features under a single config variable
> creates a very brittle system.  What are you going to do when a new
> hardware revision is released?  Create CONFIG_ARCH_THUNDER2?  Which one
> of these two would you select if building a kernel?  It is a choice that
> we don't want to force users (kernel builders) to have to waste mental
> energy on.
>
> Instead, let's try to make things work out of the box without having to
> set a bunch of random config variables.
>
> If a generic arm64 kernel won't get too bloated, I would suggest just
> enabling the compilation of the code unconditionally (at least for
> arm64).  The use of the code would still be gated by the PCI version
> probe that is part of the patch.
>

exactly, that is my initial choice with v1, and only depends on vendor 
and device id.

but it seems a config is needed. how about ARCH_ARM64 then?

Thanks,
Tirumalesh.

> David Daney
>
--
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
Robert Richter Feb. 16, 2016, 9:14 p.m. UTC | #9
On 16.02.16 11:50:44, Tirumalesh Chalamarla wrote:
> 
> 
> On 02/16/2016 11:38 AM, David Daney wrote:
> >On 02/16/2016 11:14 AM, Tirumalesh Chalamarla wrote:
> >>
> >>
> >>On 02/16/2016 06:42 AM, Robert Richter wrote:
> >>>On 15.02.16 13:30:41, Tejun Heo wrote:
> >>>>On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
> >>>>>There is no need for special Driver, AHCI is sufficient for
> >>>>>ThunderX, the
> >>>>>file only contains this interrupt handler,
> >>>>>is it preferable if this interrupt handler in libahci.c with others,
> >>>>>instead
> >>>>>of separate file?
> >>>>
> >>>>Yeap, just fold it in ahci.c with surrounding #ifdef guard.
> >>>
> >>>Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
> >>>kconfig entry for this to arch/arm64/Kconfig.
> >>>
> >>Are you sure, this is not a workaround that is based on alternative
> >>framework rather on pci device and vendor
> >>
> >>do you think CONFIG_ARCH_THUNDER a good alternative?
> >
> >No.  CONFIG_ARCH_THUNDER should be removed all together.
> >
> >Grouping a bunch of unrelated features under a single config variable
> >creates a very brittle system.  What are you going to do when a new
> >hardware revision is released?  Create CONFIG_ARCH_THUNDER2?  Which one
> >of these two would you select if building a kernel?  It is a choice that
> >we don't want to force users (kernel builders) to have to waste mental
> >energy on.
> >
> >Instead, let's try to make things work out of the box without having to
> >set a bunch of random config variables.
> >
> >If a generic arm64 kernel won't get too bloated, I would suggest just
> >enabling the compilation of the code unconditionally (at least for
> >arm64).  The use of the code would still be gated by the PCI version
> >probe that is part of the patch.
> >
> 
> exactly, that is my initial choice with v1, and only depends on vendor and
> device id.
> 
> but it seems a config is needed. how about ARCH_ARM64 then?

CONFIG_CAVIUM_ERRATUM_22536 is exactly that you need. It is not only
used for core interrupts, e.g. also for gicv3 devices (and now also
for ahci). Non-core errata (e.g. CONFIG_CAVIUM_ERRATUM_23144) are not
enabled in the arm64 cpu errata framework (not handled in
arch/arm64/kernel/cpu_errata.c).

Thus,

#ifdef CONFIG_CAVIUM_ERRATUM_22536
	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
		ahci_thunderx_init(&pdev->dev, hpriv);
#endif

is the correct enablement of the workaround by device id.

And, CAVIUM_ERRATUM_* is very easy to handle, enable and document.

-Robert
--
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
tchalamarla@caviumnetworks.com Feb. 16, 2016, 11:13 p.m. UTC | #10
On 02/16/2016 01:14 PM, Robert Richter wrote:
> On 16.02.16 11:50:44, Tirumalesh Chalamarla wrote:
>>
>>
>> On 02/16/2016 11:38 AM, David Daney wrote:
>>> On 02/16/2016 11:14 AM, Tirumalesh Chalamarla wrote:
>>>>
>>>>
>>>> On 02/16/2016 06:42 AM, Robert Richter wrote:
>>>>> On 15.02.16 13:30:41, Tejun Heo wrote:
>>>>>> On Sun, Feb 14, 2016 at 07:36:18PM -0800, Tirumalesh Chalamarla wrote:
>>>>>>> There is no need for special Driver, AHCI is sufficient for
>>>>>>> ThunderX, the
>>>>>>> file only contains this interrupt handler,
>>>>>>> is it preferable if this interrupt handler in libahci.c with others,
>>>>>>> instead
>>>>>>> of separate file?
>>>>>>
>>>>>> Yeap, just fold it in ahci.c with surrounding #ifdef guard.
>>>>>
>>>>> Yes, please use #ifdef CONFIG_CAVIUM_ERRATUM_22536 ... and add a
>>>>> kconfig entry for this to arch/arm64/Kconfig.
>>>>>
>>>> Are you sure, this is not a workaround that is based on alternative
>>>> framework rather on pci device and vendor
>>>>
>>>> do you think CONFIG_ARCH_THUNDER a good alternative?
>>>
>>> No.  CONFIG_ARCH_THUNDER should be removed all together.
>>>
>>> Grouping a bunch of unrelated features under a single config variable
>>> creates a very brittle system.  What are you going to do when a new
>>> hardware revision is released?  Create CONFIG_ARCH_THUNDER2?  Which one
>>> of these two would you select if building a kernel?  It is a choice that
>>> we don't want to force users (kernel builders) to have to waste mental
>>> energy on.
>>>
>>> Instead, let's try to make things work out of the box without having to
>>> set a bunch of random config variables.
>>>
>>> If a generic arm64 kernel won't get too bloated, I would suggest just
>>> enabling the compilation of the code unconditionally (at least for
>>> arm64).  The use of the code would still be gated by the PCI version
>>> probe that is part of the patch.
>>>
>>
>> exactly, that is my initial choice with v1, and only depends on vendor and
>> device id.
>>
>> but it seems a config is needed. how about ARCH_ARM64 then?
>
> CONFIG_CAVIUM_ERRATUM_22536 is exactly that you need. It is not only
> used for core interrupts, e.g. also for gicv3 devices (and now also
> for ahci). Non-core errata (e.g. CONFIG_CAVIUM_ERRATUM_23144) are not
> enabled in the arm64 cpu errata framework (not handled in
> arch/arm64/kernel/cpu_errata.c).
>
> Thus,
>
> #ifdef CONFIG_CAVIUM_ERRATUM_22536
> 	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
> 		ahci_thunderx_init(&pdev->dev, hpriv);
> #endif
>
> is the correct enablement of the workaround by device id.
>
> And, CAVIUM_ERRATUM_* is very easy to handle, enable and document.
>
The code will only run for Thunder and AHCI, becuase its PCI.

> -Robert
>
--
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
Will Deacon Feb. 17, 2016, 11:29 a.m. UTC | #11
On Tue, Feb 16, 2016 at 03:13:32PM -0800, Tirumalesh Chalamarla wrote:
> On 02/16/2016 01:14 PM, Robert Richter wrote:
> >CONFIG_CAVIUM_ERRATUM_22536 is exactly that you need. It is not only
> >used for core interrupts, e.g. also for gicv3 devices (and now also
> >for ahci). Non-core errata (e.g. CONFIG_CAVIUM_ERRATUM_23144) are not
> >enabled in the arm64 cpu errata framework (not handled in
> >arch/arm64/kernel/cpu_errata.c).
> >
> >Thus,
> >
> >#ifdef CONFIG_CAVIUM_ERRATUM_22536
> >	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
> >		ahci_thunderx_init(&pdev->dev, hpriv);
> >#endif
> >
> >is the correct enablement of the workaround by device id.
> >
> >And, CAVIUM_ERRATUM_* is very easy to handle, enable and document.
> >
> The code will only run for Thunder and AHCI, becuase its PCI.

Well, the guards also serve as documentation for exactly why we're having
to do something special here, so I'd say go ahead and add
CONFIG_CAVIUM_ERRATUM_22536 and update
Documentation/arm64/silicon-errata.txt accordingly.



Will
--
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
David Daney Feb. 17, 2016, 6:57 p.m. UTC | #12
On 02/17/2016 03:29 AM, Will Deacon wrote:
> On Tue, Feb 16, 2016 at 03:13:32PM -0800, Tirumalesh Chalamarla wrote:
>> On 02/16/2016 01:14 PM, Robert Richter wrote:
>>> CONFIG_CAVIUM_ERRATUM_22536 is exactly that you need. It is not only
>>> used for core interrupts, e.g. also for gicv3 devices (and now also
>>> for ahci). Non-core errata (e.g. CONFIG_CAVIUM_ERRATUM_23144) are not
>>> enabled in the arm64 cpu errata framework (not handled in
>>> arch/arm64/kernel/cpu_errata.c).
>>>
>>> Thus,
>>>
>>> #ifdef CONFIG_CAVIUM_ERRATUM_22536
>>> 	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
>>> 		ahci_thunderx_init(&pdev->dev, hpriv);
>>> #endif
>>>
>>> is the correct enablement of the workaround by device id.
>>>
>>> And, CAVIUM_ERRATUM_* is very easy to handle, enable and document.
>>>
>> The code will only run for Thunder and AHCI, becuase its PCI.
>
> Well, the guards also serve as documentation for exactly why we're having
> to do something special here, so I'd say go ahead and add
> CONFIG_CAVIUM_ERRATUM_22536 and update
> Documentation/arm64/silicon-errata.txt accordingly.
>

If the ahci/ata maintainers insist, we can add that.

<sarcasm>
Should we also add some CONFIG_ variables for:

   ahci_mcp89_apple_enable()
   ahci_sb600_enable_64bit()
   ahci_p5wdh_workaround()
   .
   .
   .

Surely we would wish to give people the flexibility to trim that code 
out of the driver if they know that it is not needed.

And don't even get me started on drivers/pci/quirks.c ...

</sarcasm>


>
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

--
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
Tejun Heo Feb. 17, 2016, 8 p.m. UTC | #13
Hello, David.

On Wed, Feb 17, 2016 at 10:57:50AM -0800, David Daney wrote:
> If the ahci/ata maintainers insist, we can add that.

I don't mind whether it is CONFIG_ARM64 or something more specific but
I think it's a good idea to gate it some way.

> <sarcasm>

If at all possible, please write plain arguments.

> Should we also add some CONFIG_ variables for:
> 
>   ahci_mcp89_apple_enable()
>   ahci_sb600_enable_64bit()
>   ahci_p5wdh_workaround()

And I think these should be gated behind CONFIG_X86.  The lack of
gating is historic, not intentional.

Thanks.
tchalamarla@caviumnetworks.com Feb. 17, 2016, 9:46 p.m. UTC | #14
On 02/17/2016 12:00 PM, Tejun Heo wrote:
> Hello, David.
>
> On Wed, Feb 17, 2016 at 10:57:50AM -0800, David Daney wrote:
>> If the ahci/ata maintainers insist, we can add that.
>
> I don't mind whether it is CONFIG_ARM64 or something more specific but
> I think it's a good idea to gate it some way.
>
Already posted a V3, could you please comment on that.

Thanks,
Tirumalesh.

>> <sarcasm>
>
> If at all possible, please write plain arguments.
>
>> Should we also add some CONFIG_ variables for:
>>
>>    ahci_mcp89_apple_enable()
>>    ahci_sb600_enable_64bit()
>>    ahci_p5wdh_workaround()
>
> And I think these should be gated behind CONFIG_X86.  The lack of
> gating is historic, not intentional.
>
> Thanks.
>
--
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
diff mbox

Patch

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 1857952..a36e70d 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -2,7 +2,7 @@ 
 obj-$(CONFIG_ATA)		+= libata.o
 
 # non-SFF interface
-obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
+obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o ahci_thunderx.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 546a369..76e310e 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1560,6 +1560,9 @@  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ahci_broken_devslp(pdev))
 		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
 
+	if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
+		ahci_thunderx_init(&pdev->dev, hpriv);
+
 	/* save initial config */
 	ahci_pci_save_initial_config(pdev, hpriv);
 
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 167ba7e..77ae20d 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -425,6 +425,7 @@  void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht);
 void ahci_error_handler(struct ata_port *ap);
 u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
+void ahci_thunderx_init(struct device *dev, struct ahci_host_priv *hpriv);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
diff --git a/drivers/ata/ahci_thunderx.c b/drivers/ata/ahci_thunderx.c
new file mode 100644
index 0000000..223e170
--- /dev/null
+++ b/drivers/ata/ahci_thunderx.c
@@ -0,0 +1,73 @@ 
+/*
+ * SATA glue for Cavium Thunder SOCs.
+ *
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2010-2016 Cavium Networks
+ *
+ */
+
+#include <linux/module.h>
+#include "ahci.h"
+#include "libata.h"
+
+static irqreturn_t ahci_thunderx_irq_intr(int irq, void *dev_instance)
+{
+	struct ata_host *host = dev_instance;
+	struct ahci_host_priv *hpriv;
+	unsigned int rc = 0;
+	void __iomem *mmio;
+	u32 irq_stat, irq_masked;
+	unsigned int handled = 1;
+
+	VPRINTK("ENTER\n");
+
+	hpriv = host->private_data;
+	mmio = hpriv->mmio;
+
+	/* sigh.  0xffffffff is a valid return from h/w */
+	irq_stat = readl(mmio + HOST_IRQ_STAT);
+	if (!irq_stat)
+		return IRQ_NONE;
+redo:
+
+	irq_masked = irq_stat & hpriv->port_map;
+
+	spin_lock(&host->lock);
+
+	rc = ahci_handle_port_intr(host, irq_masked);
+
+	if (!rc)
+		handled = 0;
+
+	writel(irq_stat, mmio + HOST_IRQ_STAT);
+
+	/* Due to ERRATA#22536, ThunderX need to handle
+	 * HOST_IRQ_STAT differently.
+	 * Work around is to make sure all pending IRQs
+	 * are served before leaving handler
+	 */
+	irq_stat = readl(mmio + HOST_IRQ_STAT);
+
+	spin_unlock(&host->lock);
+
+	if (irq_stat)
+		goto redo;
+
+	VPRINTK("EXIT\n");
+
+	return IRQ_RETVAL(handled);
+}
+
+void ahci_thunderx_init(struct device *dev, struct ahci_host_priv *hpriv)
+{
+	hpriv->irq_handler = ahci_thunderx_irq_intr;
+}
+EXPORT_SYMBOL_GPL(ahci_thunderx_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cavium, Inc. <support@cavium.com>");
+MODULE_DESCRIPTION("Cavium Inc. ThunderX sata config.");