[PATCH v4 00/10] Enable per-port SATA interrupts and drop a hack in the IRQ subsystem

[PATCH v4 00/10] Enable per-port SATA interrupts and drop a hack in the IRQ subsystem

[Miquel Raynal]

Hello,

Some time ago, when the initial support for Armada CP110 was
contributed, the SATA core was not able to handle per-port
interrupts. Despite the hardware reality, the device tree only
represents one main interrupt for the two ports. Having both SATA
ports enabled at the same time has been achieved by a hack in the ICU
driver(1) that faked the use of the two interrupts, no matter which
SATA port was in use.

Now that the SATA core is ready to handle more than one interrupt,
this series adds support for it in the libahci_platform code. The
CP110 device tree must be updated to reflect the two SATA ports
available and their respective interrupts. To do not break DT backward
compatibility, the ahci_platform driver now embeds a special quirk
which checks if the DT is valid (only for A8k compatible) and, if
needed, creates the two missing sub-nodes, and assign them the
relevant "reg" and "interrupts" properties, before removing the main
SATA node "interrupts" one.


Thanks,
Miquèl

(1) The ICU is an irqchip aggregating the CP110 (south-bridge)
interrupts into MSIs for the AP806 (north-bridge).

Changes in v4
=============
* Rebase on top of v5.2-rc1
* s/ARM64/arm64/ in the DT change commit title.

Changes in v3
=============
* Removed useless locking when acking the SATA host IRQ_STAT bit.
* As spotted by Hans, do not enable the AHCI_HFLAG_MULTI_MSI flag only
  if more than one port is enabled, DTs might declare only one
  interrupt and have multiple ports. Added Hans' Reviewed-by tag.
* Added Rob's Reviewed-by tags.
* Added explicit references to the 'Device Tree' being the culprit
  for the hacks which are being treated in patch "ata: ahci: mvebu:
  Add support for A8k legacy bindings" as suggested by Marc.
* Modified all DTs to avoid enabling/disabling the SATA ports
  independently, which does not work. Either both ports are enabled, or
  none. Tested on MacchiatoBin that all three availabe ports on the
  board are functional.

Changes in v2
=============
* In the AHCI world, the 'irq' is now an '*irqs' array, I ensured it
  is allocated even when not using *_platform drivers.
* Moved the whole logic from the generic ahci_platform.c driver to the
  Marvell's ahci_mvebu.c driver.
* Dropped the whole DT manipulation quirk.
* Instead used a hack to configure both interrupts when using the
  deprecated bindings, this hack is a8k specific but there is a flag
  that is passed to the core during the ahci_platform_get_resources()
  to indicate that the number of ports must be forced to 2 no matter
  the number of child nodes.
* The A8k based Clearfog-GT actually uses the SATA IP (Baruch's info)
  so do not remove the SATA node from the DT. Instead, change the DTS
  to fit the new bindings (the board only uses the second port at
  offset 1).
* Added bindings documentation about the A8k AHCI compatible (existing
  in DTs, missing in the doc).
* SATA Sub-nodes representing ports already are documented, I just
  added a mention that they can also have an interrupts property which
  is mutually exclusive with the root SATA node.


Miquel Raynal (9):
  ata: libahci: Ensure the host interrupt status bits are cleared
  ata: ahci: Support per-port interrupts
  dt-bindings: ata: Update ahci bindings with possible per-port
    interrupts
  ata: ahci: mvebu: Rename a platform data flag
  ata: ahci: mvebu: Add a parameter to a platform data callback
  dt-bindings: ata: Update ahci_mvebu bindings
  ata: ahci: mvebu: Support A8k compatible
  ata: ahci: mvebu: Add support for A8k legacy DT bindings
  irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack

Thomas Petazzoni (1):
  arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

 .../devicetree/bindings/ata/ahci-platform.txt |  7 ++
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 13 +++-
 drivers/ata/acard-ahci.c                      |  2 +-
 drivers/ata/ahci.c                            |  8 ++-
 drivers/ata/ahci.h                            |  3 +-
 drivers/ata/ahci_mvebu.c                      | 61 +++++++++++++---
 drivers/ata/libahci.c                         |  7 +-
 drivers/ata/libahci_platform.c                | 70 ++++++++++++++++---
 drivers/ata/sata_highbank.c                   |  2 +-
 drivers/irqchip/irq-mvebu-icu.c               | 18 -----
 include/linux/ahci_platform.h                 |  1 +
 11 files changed, 148 insertions(+), 44 deletions(-)

-- 
2.19.1


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

[PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Miquel Raynal]

ahci_multi_irqs_intr_hard() is going to be used as interrupt handler
to support SATA per-port interrupts. The current logic is to check and
clear the SATA port interrupt status register only. To avoid spurious
IRQs and interrupt storms, it will be needed to clear the port
interrupt bit in the host interrupt status register as well.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/libahci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 692782dddc0f..9db6f488db59 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap)
 static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
 {
 	struct ata_port *ap = dev_instance;
+	struct ata_host *host = ap->host;
+	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
+	void __iomem *mmio = hpriv->mmio;
 	u32 status;
 
 	VPRINTK("ENTER\n");
@@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
 	ahci_handle_port_interrupt(ap, port_mmio, status);
 	spin_unlock(ap->lock);
 
+	writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
+
 	VPRINTK("EXIT\n");
 
 	return IRQ_HANDLED;
-- 
2.19.1


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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[raymond pang]

Hi Miquel,

This patch adds clearing GHC.IS into hot path, could you explain how
irq storm is generated? thanks
According to AHCI Spec, HBA should not refer to GHC.IS to generate
MSI when applying multiple MSIs.

Best Regards,
Raymond

On Tue, May 21, 2019 at 2:31 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> ahci_multi_irqs_intr_hard() is going to be used as interrupt handler
> to support SATA per-port interrupts. The current logic is to check and
> clear the SATA port interrupt status register only. To avoid spurious
> IRQs and interrupt storms, it will be needed to clear the port
> interrupt bit in the host interrupt status register as well.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/ata/libahci.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 692782dddc0f..9db6f488db59 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap)
>  static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
>  {
>         struct ata_port *ap = dev_instance;
> +       struct ata_host *host = ap->host;
> +       struct ahci_host_priv *hpriv = host->private_data;
>         void __iomem *port_mmio = ahci_port_base(ap);
> +       void __iomem *mmio = hpriv->mmio;
>         u32 status;
>
>         VPRINTK("ENTER\n");
> @@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
>         ahci_handle_port_interrupt(ap, port_mmio, status);
>         spin_unlock(ap->lock);
>
> +       writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
> +
>         VPRINTK("EXIT\n");
>
>         return IRQ_HANDLED;
> --
> 2.19.1
>

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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Marc Zyngier]

On 23/05/2019 04:11, raymond pang wrote:
> Hi Miquel,
> 
> This patch adds clearing GHC.IS into hot path, could you explain how
> irq storm is generated? thanks
> According to AHCI Spec, HBA should not refer to GHC.IS to generate
> MSI when applying multiple MSIs.

Well spotted.

I have the ugly feeling that this is because the Marvell AHCI
implementation is not using MSIs at all, but instead a pair of wired
interrupts (which are level triggered instead of edge, hence the
screaming interrupts).

The changes in the following patches abuse the rest of the driver by
pretending this is a a multi-MSI setup, while it clearly doesn't match
the expectation of the AHCI spec for MSIs.

It looks like this shouldn't be imposed on other unsuspecting
implementations which correctly use edge-triggered MSIs and do not
require such an MMIO access.

Thanks,

	M.

> 
> Best Regards,
> Raymond
> 
> On Tue, May 21, 2019 at 2:31 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> ahci_multi_irqs_intr_hard() is going to be used as interrupt handler
>> to support SATA per-port interrupts. The current logic is to check and
>> clear the SATA port interrupt status register only. To avoid spurious
>> IRQs and interrupt storms, it will be needed to clear the port
>> interrupt bit in the host interrupt status register as well.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/ata/libahci.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index 692782dddc0f..9db6f488db59 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap)
>>  static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
>>  {
>>         struct ata_port *ap = dev_instance;
>> +       struct ata_host *host = ap->host;
>> +       struct ahci_host_priv *hpriv = host->private_data;
>>         void __iomem *port_mmio = ahci_port_base(ap);
>> +       void __iomem *mmio = hpriv->mmio;
>>         u32 status;
>>
>>         VPRINTK("ENTER\n");
>> @@ -1924,6 +1927,8 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
>>         ahci_handle_port_interrupt(ap, port_mmio, status);
>>         spin_unlock(ap->lock);
>>
>> +       writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
>> +
>>         VPRINTK("EXIT\n");
>>
>>         return IRQ_HANDLED;
>> --
>> 2.19.1
>>


-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Miquel Raynal]

Hi Marc & Raymond,

Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01
+0100:

> On 23/05/2019 04:11, raymond pang wrote:
> > Hi Miquel,
> > 
> > This patch adds clearing GHC.IS into hot path, could you explain how
> > irq storm is generated? thanks
> > According to AHCI Spec, HBA should not refer to GHC.IS to generate
> > MSI when applying multiple MSIs.  
> 
> Well spotted.
> 
> I have the ugly feeling that this is because the Marvell AHCI
> implementation is not using MSIs at all, but instead a pair of wired
> interrupts (which are level triggered instead of edge, hence the
> screaming interrupts).
> 
> The changes in the following patches abuse the rest of the driver by
> pretending this is a a multi-MSI setup, while it clearly doesn't match
> the expectation of the AHCI spec for MSIs.
> 
> It looks like this shouldn't be imposed on other unsuspecting
> implementations which correctly use edge-triggered MSIs and do not
> require such an MMIO access.

I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in
hpriv->flags which will be used by the mvebu_ahci.c driver to request
for this MMIO access. This way, the hot path remains the same.


Thanks,
Miquèl

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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Marc Zyngier]

On 29/05/2019 11:08, Miquel Raynal wrote:
> Hi Marc & Raymond,
> 
> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01
> +0100:
> 
>> On 23/05/2019 04:11, raymond pang wrote:
>>> Hi Miquel,
>>>
>>> This patch adds clearing GHC.IS into hot path, could you explain how
>>> irq storm is generated? thanks
>>> According to AHCI Spec, HBA should not refer to GHC.IS to generate
>>> MSI when applying multiple MSIs.  
>>
>> Well spotted.
>>
>> I have the ugly feeling that this is because the Marvell AHCI
>> implementation is not using MSIs at all, but instead a pair of wired
>> interrupts (which are level triggered instead of edge, hence the
>> screaming interrupts).
>>
>> The changes in the following patches abuse the rest of the driver by
>> pretending this is a a multi-MSI setup, while it clearly doesn't match
>> the expectation of the AHCI spec for MSIs.
>>
>> It looks like this shouldn't be imposed on other unsuspecting
>> implementations which correctly use edge-triggered MSIs and do not
>> require such an MMIO access.
> 
> I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in
> hpriv->flags which will be used by the mvebu_ahci.c driver to request
> for this MMIO access. This way, the hot path remains the same.

I'm not convinced that's a good idea, if only because from the PoV of
the AHCI device itself, these are not MSIs at all, but wired interrupts.
The fact that there is some glue logic in the middle that turns it into
a message (and then back into a wire) is a regrettable implementation
detail.

I'd rather you stick to the normal interrupt handler, or provide your
own, which would solve most problems.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Miquel Raynal]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 29 May 2019 11:37:58
+0100:

> On 29/05/2019 11:08, Miquel Raynal wrote:
> > Hi Marc & Raymond,
> > 
> > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01
> > +0100:
> >   
> >> On 23/05/2019 04:11, raymond pang wrote:  
> >>> Hi Miquel,
> >>>
> >>> This patch adds clearing GHC.IS into hot path, could you explain how
> >>> irq storm is generated? thanks
> >>> According to AHCI Spec, HBA should not refer to GHC.IS to generate
> >>> MSI when applying multiple MSIs.    
> >>
> >> Well spotted.
> >>
> >> I have the ugly feeling that this is because the Marvell AHCI
> >> implementation is not using MSIs at all, but instead a pair of wired
> >> interrupts (which are level triggered instead of edge, hence the
> >> screaming interrupts).
> >>
> >> The changes in the following patches abuse the rest of the driver by
> >> pretending this is a a multi-MSI setup, while it clearly doesn't match
> >> the expectation of the AHCI spec for MSIs.
> >>
> >> It looks like this shouldn't be imposed on other unsuspecting
> >> implementations which correctly use edge-triggered MSIs and do not
> >> require such an MMIO access.  
> > 
> > I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in
> > hpriv->flags which will be used by the mvebu_ahci.c driver to request
> > for this MMIO access. This way, the hot path remains the same.  
> 
> I'm not convinced that's a good idea, if only because from the PoV of
> the AHCI device itself, these are not MSIs at all, but wired interrupts.
> The fact that there is some glue logic in the middle that turns it into
> a message (and then back into a wire) is a regrettable implementation
> detail.
> 
> I'd rather you stick to the normal interrupt handler, or provide your
> own, which would solve most problems.

Unless I don't understand your proposal, "stick to the normal interrupt
handler" is not possible as I need this register write to happen at
this time, not at any other moment.

However, on the possibility of having a separate interrupt handler, I
may use the new AHCI_HFALG_LEVEL_MSI flag to change the
devm_request_irq call here [1] and use my own at this moment. The
handler will be in libahci.c though.

Would this be a better approach?


Thanks,
Miquèl

[1] https://elixir.bootlin.com/linux/v5.2-rc2/source/drivers/ata/libahci.c#L2557

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

Re: [PATCH v4 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Marc Zyngier]

On 29/05/2019 13:13, Miquel Raynal wrote:
> Hi Marc,
> 
> Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 29 May 2019 11:37:58
> +0100:
> 
>> On 29/05/2019 11:08, Miquel Raynal wrote:
>>> Hi Marc & Raymond,
>>>
>>> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 23 May 2019 10:26:01
>>> +0100:
>>>   
>>>> On 23/05/2019 04:11, raymond pang wrote:  
>>>>> Hi Miquel,
>>>>>
>>>>> This patch adds clearing GHC.IS into hot path, could you explain how
>>>>> irq storm is generated? thanks
>>>>> According to AHCI Spec, HBA should not refer to GHC.IS to generate
>>>>> MSI when applying multiple MSIs.    
>>>>
>>>> Well spotted.
>>>>
>>>> I have the ugly feeling that this is because the Marvell AHCI
>>>> implementation is not using MSIs at all, but instead a pair of wired
>>>> interrupts (which are level triggered instead of edge, hence the
>>>> screaming interrupts).
>>>>
>>>> The changes in the following patches abuse the rest of the driver by
>>>> pretending this is a a multi-MSI setup, while it clearly doesn't match
>>>> the expectation of the AHCI spec for MSIs.
>>>>
>>>> It looks like this shouldn't be imposed on other unsuspecting
>>>> implementations which correctly use edge-triggered MSIs and do not
>>>> require such an MMIO access.  
>>>
>>> I understand your concern, let me add a AHCI_HFLAG_LEVEL_MSI in
>>> hpriv->flags which will be used by the mvebu_ahci.c driver to request
>>> for this MMIO access. This way, the hot path remains the same.  
>>
>> I'm not convinced that's a good idea, if only because from the PoV of
>> the AHCI device itself, these are not MSIs at all, but wired interrupts.
>> The fact that there is some glue logic in the middle that turns it into
>> a message (and then back into a wire) is a regrettable implementation
>> detail.
>>
>> I'd rather you stick to the normal interrupt handler, or provide your
>> own, which would solve most problems.
> 
> Unless I don't understand your proposal, "stick to the normal interrupt
> handler" is not possible as I need this register write to happen at
> this time, not at any other moment.
> 
> However, on the possibility of having a separate interrupt handler, I
> may use the new AHCI_HFALG_LEVEL_MSI flag to change the
> devm_request_irq call here [1] and use my own at this moment. The
> handler will be in libahci.c though.
> 
> Would this be a better approach?

Again, level-triggered MSIs are not a property of the AHCI block. If
anything, that's a property of the ICU block. Please do not conflate the
two.

What you have here is a set of wired interrupts, one per port. I'd
suggest you implement it a separate interrupt handler keyed on a
particular flag if you cannot detect this case by other means.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[PATCH v4 02/10] ata: ahci: Support per-port interrupts

[Miquel Raynal]

Right now the ATA core only allows IPs to use a single interrupt. Some
of them (for instance the Armada-CP110 one) actually has one interrupt
per port. Add some logic to support such situation.

We consider that either there is one single interrupt declared in the
main IP node, or there are per-port interrupts, each of them being
declared in the port sub-nodes.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/acard-ahci.c       |  2 +-
 drivers/ata/ahci.c             |  8 +++-
 drivers/ata/ahci.h             |  3 +-
 drivers/ata/libahci.c          |  2 +-
 drivers/ata/libahci_platform.c | 67 ++++++++++++++++++++++++++++------
 drivers/ata/sata_highbank.c    |  2 +-
 6 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 583e366be7e2..9414b81e994c 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
 	if (!hpriv)
 		return -ENOMEM;
 
-	hpriv->irq = pdev->irq;
+	hpriv->irqs[0] = pdev->irq;
 	hpriv->flags |= (unsigned long)pi.private_data;
 
 	if (!(hpriv->flags & AHCI_HFLAG_NO_MSI))
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 021ce46e2e57..bc37a34fa043 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1817,7 +1817,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		/* legacy intx interrupts */
 		pci_intx(pdev, 1);
 	}
-	hpriv->irq = pci_irq_vector(pdev, 0);
+
+	hpriv->irqs = devm_kzalloc(dev, sizeof(*hpriv->irqs) * n_ports,
+				   GFP_KERNEL);
+	if (!hpriv->irqs)
+		return -ENOMEM;
+
+	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
 
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 8810475f307a..f569f6a0d9f5 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -363,7 +363,7 @@ struct ahci_host_priv {
 	struct phy		**phys;
 	unsigned		nports;		/* Number of ports */
 	void			*plat_data;	/* Other platform data */
-	unsigned int		irq;		/* interrupt line */
+	unsigned int		*irqs;		/* interrupt line(s) */
 	/*
 	 * Optional ahci_start_engine override, if not set this gets set to the
 	 * default ahci_start_engine during ahci_save_initial_config, this can
@@ -434,6 +434,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);
+int ahci_get_per_port_irq_vector(struct ata_host *host, int port);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 9db6f488db59..9d275ad4e14c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2600,7 +2600,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
-	int irq = hpriv->irq;
+	int irq = hpriv->irqs[0];
 	int rc;
 
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 81b1a3332ed6..347ff14aedd2 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -24,6 +24,7 @@
 #include <linux/ahci_platform.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/reset.h>
 #include "ahci.h"
@@ -95,6 +96,14 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	}
 }
 
+int ahci_get_per_port_irq_vector(struct ata_host *host, int port)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	return hpriv->irqs[port];
+}
+EXPORT_SYMBOL_GPL(ahci_get_per_port_irq_vector);
+
 /**
  * ahci_platform_enable_clks - Enable platform clocks
  * @hpriv: host private area to store config values
@@ -385,6 +394,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
  *    or for non devicetree enabled platforms a single clock
  * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
  * 5) phys (optional)
+ * 6) interrupt(s)
  *
  * RETURNS:
  * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
@@ -396,7 +406,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
+	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes, ctrl_irq;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		goto err_out;
 	}
 
+	hpriv->irqs = devm_kzalloc(dev, sizeof(*hpriv->irqs) * hpriv->nports,
+				   GFP_KERNEL);
+	if (!hpriv->irqs) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	ctrl_irq = platform_get_irq(pdev, 0);
+	if (ctrl_irq < 0) {
+		if (ctrl_irq == -EPROBE_DEFER) {
+			rc = ctrl_irq;
+			goto err_out;
+		}
+		ctrl_irq = 0;
+	}
+
+	if (ctrl_irq > 0)
+		hpriv->irqs[0] = ctrl_irq;
+
 	if (child_nodes) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
 			struct platform_device *port_dev __maybe_unused;
+			int port_irq;
 
 			if (!of_device_is_available(child))
 				continue;
@@ -521,6 +551,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 			}
 #endif
 
+			if (!ctrl_irq) {
+				port_irq = of_irq_get(child, 0);
+				if (!port_irq)
+					port_irq = -EINVAL;
+				if (port_irq < 0) {
+					rc = port_irq;
+					goto err_out;
+				}
+
+				hpriv->irqs[port] = port_irq;
+			}
+
 			rc = ahci_platform_get_phy(hpriv, port, dev, child);
 			if (rc)
 				goto err_out;
@@ -548,6 +590,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		if (rc == -EPROBE_DEFER)
 			goto err_out;
 	}
+
+	if (!ctrl_irq && !enabled_ports) {
+		dev_err(&pdev->dev, "No IRQ defined\n");
+		rc = -ENODEV;
+		goto err_out;
+	}
+
+	if (!ctrl_irq && enabled_ports > 1) {
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
@@ -584,16 +638,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	struct ata_port_info pi = *pi_template;
 	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
-	int i, irq, n_ports, rc;
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		if (irq != -EPROBE_DEFER)
-			dev_err(dev, "no irq\n");
-		return irq;
-	}
-
-	hpriv->irq = irq;
+	int i, n_ports, rc;
 
 	/* prepare host */
 	pi.private_data = (void *)(unsigned long)hpriv->flags;
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index c8fc9280d6e4..dcfdab20021b 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -496,7 +496,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	hpriv->irq = irq;
+	hpriv->irqs[0] = irq;
 	hpriv->flags |= (unsigned long)pi.private_data;
 
 	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
-- 
2.19.1


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

Re: [PATCH v4 02/10] ata: ahci: Support per-port interrupts

[Christoph Hellwig]

> --- a/drivers/ata/acard-ahci.c
> +++ b/drivers/ata/acard-ahci.c
> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>  	if (!hpriv)
>  		return -ENOMEM;
>  
> -	hpriv->irq = pdev->irq;
> +	hpriv->irqs[0] = pdev->irq;
>  	hpriv->flags |= (unsigned long)pi.private_data;

Who allocates ->irqs for the non-ahci.c case?

> @@ -95,6 +96,14 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
>  	}
>  }
>  
> +int ahci_get_per_port_irq_vector(struct ata_host *host, int port)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +
> +	return hpriv->irqs[port];
> +}
> +EXPORT_SYMBOL_GPL(ahci_get_per_port_irq_vector);

This function seems a little misnamed.  The only multi-irq support in
the AHCI spec itself is PCIe MSI/MSI-X, which is not handled by this
function, but instead in the PCI layer using pci_irq_vector and the
ahci_get_irq_vector callback in ahci.c.

In fact it seems like this scheme particular to your device config,
why don't we just add a specific libahci-using subdriver for it?  That
would also get rid of the whole ->irq to ->irqs change.

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

[PATCH v4 03/10] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[Miquel Raynal]

Update bindings to reflect the fact that a SATA IP can either have:
- only one interrupt: in this case an 'interrupts' property is
  declared at the root of the node;
or
- each SATA port can have their own interrupt: in this case there is
  one 'interrupts' property per port/sub-node and none at the root.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index e30fd106df4f..80261e2845b0 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -53,6 +53,12 @@ And at least one of the following properties:
 - phys		    : reference to the SATA PHY node
 - target-supply     : regulator for SATA target power
 
+Sub-nodes optional properties:
+- interrupts        : <interrupt mapping for SATA ports IRQ>, please
+                      note that either the root SATA node has the
+                      interrupts property, or there is one per SATA
+                      port, but not both at the same time.
+
 Examples:
         sata@ffe08000 {
 		compatible = "snps,spear-ahci";
-- 
2.19.1


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

[PATCH v4 04/10] ata: ahci: mvebu: Rename a platform data flag

[Miquel Raynal]

Before adding more entries in the platform data structure, rename the
flags entry to be more precise and name it host_flags.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index d4bba3ace45d..43bb2db59698 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -30,7 +30,7 @@
 
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv);
-	unsigned int flags;
+	unsigned int host_flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -196,7 +196,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
-	hpriv->flags |= pdata->flags;
+	hpriv->flags |= pdata->host_flags;
 	hpriv->plat_data = (void *)pdata;
 
 	rc = ahci_platform_enable_resources(hpriv);
@@ -227,7 +227,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_380_plat_data = {
 
 static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 	.plat_config = ahci_mvebu_armada_3700_config,
-	.flags = AHCI_HFLAG_SUSPEND_PHYS,
+	.host_flags = AHCI_HFLAG_SUSPEND_PHYS,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
-- 
2.19.1


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

Re: [PATCH v4 04/10] ata: ahci: mvebu: Rename a platform data flag

[Marc Zyngier]

On 21/05/2019 15:30, Miquel Raynal wrote:
> Before adding more entries in the platform data structure, rename the
> flags entry to be more precise and name it host_flags.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/ata/ahci_mvebu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
> index d4bba3ace45d..43bb2db59698 100644
> --- a/drivers/ata/ahci_mvebu.c
> +++ b/drivers/ata/ahci_mvebu.c
> @@ -30,7 +30,7 @@
>  
>  struct ahci_mvebu_plat_data {
>  	int (*plat_config)(struct ahci_host_priv *hpriv);
> -	unsigned int flags;
> +	unsigned int host_flags;
>  };
>  
>  static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
> @@ -196,7 +196,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
>  	if (IS_ERR(hpriv))
>  		return PTR_ERR(hpriv);
>  
> -	hpriv->flags |= pdata->flags;
> +	hpriv->flags |= pdata->host_flags;
>  	hpriv->plat_data = (void *)pdata;
>  
>  	rc = ahci_platform_enable_resources(hpriv);
> @@ -227,7 +227,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_380_plat_data = {
>  
>  static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
>  	.plat_config = ahci_mvebu_armada_3700_config,
> -	.flags = AHCI_HFLAG_SUSPEND_PHYS,
> +	.host_flags = AHCI_HFLAG_SUSPEND_PHYS,
>  };
>  
>  static const struct of_device_id ahci_mvebu_of_match[] = {
> 

If, as I suspect, you don't need resource_flags in patch 8, then this
patch becomes a bit pointless.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[PATCH v4 05/10] ata: ahci: mvebu: Add a parameter to a platform data callback

[Miquel Raynal]

Before using the ahci_mvebu.c driver with Armada 8k hardware (right
now it is using the ahci_platform.c generic driver), let's add a
'struct device' pointer to the argument list of the
->plat_config() callback. This parameter will be used by the A8k's
callback.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 43bb2db59698..507ee7c5437c 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -29,7 +29,7 @@
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
 struct ahci_mvebu_plat_data {
-	int (*plat_config)(struct ahci_host_priv *hpriv);
+	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
 	unsigned int host_flags;
 };
 
@@ -67,7 +67,8 @@ static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv)
 	writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
 }
 
-static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv,
+					struct device *dev)
 {
 	const struct mbus_dram_target_info *dram;
 	int rc = 0;
@@ -83,7 +84,8 @@ static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
 	return rc;
 }
 
-static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
+					 struct device *dev)
 {
 	u32 reg;
 
@@ -162,7 +164,7 @@ static int ahci_mvebu_resume(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	const struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
 
-	pdata->plat_config(hpriv);
+	pdata->plat_config(hpriv, &pdev->dev);
 
 	return ahci_platform_resume_host(&pdev->dev);
 }
@@ -205,7 +207,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 
 	hpriv->stop_engine = ahci_mvebu_stop_engine;
 
-	rc = pdata->plat_config(hpriv);
+	rc = pdata->plat_config(hpriv, &pdev->dev);
 	if (rc)
 		goto disable_resources;
 
-- 
2.19.1


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

[PATCH v4 06/10] dt-bindings: ata: Update ahci_mvebu bindings

[Miquel Raynal]

Update bindings with the already in use Armada 8k compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 80261e2845b0..a1e6a3a27ee3 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -17,6 +17,7 @@ Required properties:
   - "ibm,476gtr-ahci"
   - "marvell,armada-380-ahci"
   - "marvell,armada-3700-ahci"
+  - "marvell,armada-8k-ahci"
   - "snps,dwc-ahci"
   - "snps,spear-ahci"
   - "generic-ahci"
-- 
2.19.1


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

[PATCH v4 07/10] ata: ahci: mvebu: Support A8k compatible

[Miquel Raynal]

The ahci_platform.c driver was historically the one bound to the A8k
AHCI compatible string, but before adding a quirk for this compatible,
it is probably cleaner to put all Marvell EBU code in one place: the
ahci_mvebu.c driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 507ee7c5437c..8671aa8179fa 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -98,6 +98,12 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
 	return 0;
 }
 
+static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
+				       struct device *dev)
+{
+	return 0;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -232,6 +238,10 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 	.host_flags = AHCI_HFLAG_SUSPEND_PHYS,
 };
 
+static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
+	.plat_config = ahci_mvebu_armada_8k_config,
+};
+
 static const struct of_device_id ahci_mvebu_of_match[] = {
 	{
 		.compatible = "marvell,armada-380-ahci",
@@ -241,6 +251,10 @@ static const struct of_device_id ahci_mvebu_of_match[] = {
 		.compatible = "marvell,armada-3700-ahci",
 		.data = &ahci_mvebu_armada_3700_plat_data,
 	},
+	{
+		.compatible = "marvell,armada-8k-ahci",
+		.data = &ahci_mvebu_armada_8k_plat_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ahci_mvebu_of_match);
-- 
2.19.1


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

[PATCH v4 08/10] ata: ahci: mvebu: Add support for A8k legacy DT bindings

[Miquel Raynal]

The CP110 SATA unit has 2 ports, and a dedicated ICU entry per
port. In the past, the AHCI SATA driver only supported one interrupt
per SATA unit. To solve this conflict, the 2 SATA wired interrupts in
the South-Bridge got configured as 1 GIC interrupt in the
North-Bridge, regardless of the number of SATA ports actually
enabled/in use, and the DT bindings only referenced the interrupt of
one port.

Since then, this limitation has been addressed and this patch ensures
backward compatibility with old DTs not describing SATA ports
correctly directly from the AHCI MVEBU driver. This way, we will be
able to drop the hack from the ICU driver. IOW, when the A8k
compatible string is used and there is no sub-nodes in the DT, we
fake the creation and mapping of the second (missing) interrupt.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c       | 29 ++++++++++++++++++++++++++++-
 drivers/ata/libahci_platform.c |  3 +++
 include/linux/ahci_platform.h  |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 8671aa8179fa..8c981b8d6e4d 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -16,6 +16,7 @@
 #include <linux/mbus.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include "ahci.h"
 
@@ -28,9 +29,13 @@
 #define AHCI_WINDOW_BASE(win)	(0x64 + ((win) << 4))
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
+#define ICU_SATA0_ICU_ID 109
+#define ICU_SATA1_ICU_ID 107
+
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
 	unsigned int host_flags;
+	unsigned int resource_flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -101,6 +106,27 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
 static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
 				       struct device *dev)
 {
+	struct device_node *np = of_irq_find_parent(dev->of_node);
+	struct irq_data *irqd = irq_get_irq_data(hpriv->irqs[0]);
+	int host_irq = irqd ? irqd_to_hwirq(irqd) : 0;
+	int missing_irq = (host_irq == ICU_SATA1_ICU_ID) ?
+		ICU_SATA0_ICU_ID : ICU_SATA1_ICU_ID;
+	struct irq_fwspec fwspec = {
+		.fwnode = of_node_to_fwnode(np),
+		.param_count = 2,
+		.param = {missing_irq, IRQ_TYPE_LEVEL_HIGH},
+	};
+
+	if (of_get_child_count(dev->of_node))
+		return 0;
+
+	hpriv->irqs[1] = irq_create_fwspec_mapping(&fwspec);
+	if (hpriv->irqs[1]) {
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
+		hpriv->mask_port_map = GENMASK(1, 0);
+	}
+
 	return 0;
 }
 
@@ -200,7 +226,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!pdata)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -240,6 +266,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 
 static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
 	.plat_config = ahci_mvebu_armada_8k_config,
+	.resource_flags = AHCI_PLATFORM_A8K_QUIRK,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 347ff14aedd2..620e5f27fdd7 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -484,6 +484,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 	if (!child_nodes)
 		hpriv->nports = 1;
 
+	if (!child_nodes && flags & AHCI_PLATFORM_A8K_QUIRK)
+		hpriv->nports = 2;
+
 	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
 	if (!hpriv->phys) {
 		rc = -ENOMEM;
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index eaedca5fe6fc..57465ba6bb15 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -44,5 +44,6 @@ int ahci_platform_suspend(struct device *dev);
 int ahci_platform_resume(struct device *dev);
 
 #define AHCI_PLATFORM_GET_RESETS	0x01
+#define AHCI_PLATFORM_A8K_QUIRK		0x02
 
 #endif /* _AHCI_PLATFORM_H */
-- 
2.19.1


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

Re: [PATCH v4 08/10] ata: ahci: mvebu: Add support for A8k legacy DT bindings

[Marc Zyngier]

On 21/05/2019 15:30, Miquel Raynal wrote:
> The CP110 SATA unit has 2 ports, and a dedicated ICU entry per
> port. In the past, the AHCI SATA driver only supported one interrupt
> per SATA unit. To solve this conflict, the 2 SATA wired interrupts in
> the South-Bridge got configured as 1 GIC interrupt in the
> North-Bridge, regardless of the number of SATA ports actually
> enabled/in use, and the DT bindings only referenced the interrupt of
> one port.
> 
> Since then, this limitation has been addressed and this patch ensures
> backward compatibility with old DTs not describing SATA ports
> correctly directly from the AHCI MVEBU driver. This way, we will be
> able to drop the hack from the ICU driver. IOW, when the A8k
> compatible string is used and there is no sub-nodes in the DT, we
> fake the creation and mapping of the second (missing) interrupt.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/ata/ahci_mvebu.c       | 29 ++++++++++++++++++++++++++++-
>  drivers/ata/libahci_platform.c |  3 +++
>  include/linux/ahci_platform.h  |  1 +
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
> index 8671aa8179fa..8c981b8d6e4d 100644
> --- a/drivers/ata/ahci_mvebu.c
> +++ b/drivers/ata/ahci_mvebu.c
> @@ -16,6 +16,7 @@
>  #include <linux/mbus.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include "ahci.h"
>  
> @@ -28,9 +29,13 @@
>  #define AHCI_WINDOW_BASE(win)	(0x64 + ((win) << 4))
>  #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
>  
> +#define ICU_SATA0_ICU_ID 109
> +#define ICU_SATA1_ICU_ID 107
> +
>  struct ahci_mvebu_plat_data {
>  	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
>  	unsigned int host_flags;
> +	unsigned int resource_flags;
>  };
>  
>  static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
> @@ -101,6 +106,27 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
>  static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
>  				       struct device *dev)
>  {
> +	struct device_node *np = of_irq_find_parent(dev->of_node);
> +	struct irq_data *irqd = irq_get_irq_data(hpriv->irqs[0]);
> +	int host_irq = irqd ? irqd_to_hwirq(irqd) : 0;
> +	int missing_irq = (host_irq == ICU_SATA1_ICU_ID) ?
> +		ICU_SATA0_ICU_ID : ICU_SATA1_ICU_ID;
> +	struct irq_fwspec fwspec = {
> +		.fwnode = of_node_to_fwnode(np),
> +		.param_count = 2,
> +		.param = {missing_irq, IRQ_TYPE_LEVEL_HIGH},
> +	};
> +
> +	if (of_get_child_count(dev->of_node))
> +		return 0;
> +
> +	hpriv->irqs[1] = irq_create_fwspec_mapping(&fwspec);
> +	if (hpriv->irqs[1]) {
> +		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
> +		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
> +		hpriv->mask_port_map = GENMASK(1, 0);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -200,7 +226,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
>  	if (!pdata)
>  		return -EINVAL;
>  
> -	hpriv = ahci_platform_get_resources(pdev, 0);
> +	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);

This seems to be the only use of resource_flags in the whole series. Why
can't you just pass AHCI_PLATFORM_A8K_QUIRK as a parameter and not
allocate an extra field in the platform data structure?

>  	if (IS_ERR(hpriv))
>  		return PTR_ERR(hpriv);
>  
> @@ -240,6 +266,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
>  
>  static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
>  	.plat_config = ahci_mvebu_armada_8k_config,
> +	.resource_flags = AHCI_PLATFORM_A8K_QUIRK,
>  };
>  
>  static const struct of_device_id ahci_mvebu_of_match[] = {
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 347ff14aedd2..620e5f27fdd7 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -484,6 +484,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  	if (!child_nodes)
>  		hpriv->nports = 1;
>  
> +	if (!child_nodes && flags & AHCI_PLATFORM_A8K_QUIRK)

Consider using braces around the bitwise operators.

> +		hpriv->nports = 2;
> +
>  	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>  	if (!hpriv->phys) {
>  		rc = -ENOMEM;
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index eaedca5fe6fc..57465ba6bb15 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -44,5 +44,6 @@ int ahci_platform_suspend(struct device *dev);
>  int ahci_platform_resume(struct device *dev);
>  
>  #define AHCI_PLATFORM_GET_RESETS	0x01
> +#define AHCI_PLATFORM_A8K_QUIRK		0x02
>  
>  #endif /* _AHCI_PLATFORM_H */
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v4 08/10] ata: ahci: mvebu: Add support for A8k legacy DT bindings

[Miquel Raynal]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 May 2019 16:46:01
+0100:

> On 21/05/2019 15:30, Miquel Raynal wrote:
> > The CP110 SATA unit has 2 ports, and a dedicated ICU entry per
> > port. In the past, the AHCI SATA driver only supported one interrupt
> > per SATA unit. To solve this conflict, the 2 SATA wired interrupts in
> > the South-Bridge got configured as 1 GIC interrupt in the
> > North-Bridge, regardless of the number of SATA ports actually
> > enabled/in use, and the DT bindings only referenced the interrupt of
> > one port.
> > 
> > Since then, this limitation has been addressed and this patch ensures
> > backward compatibility with old DTs not describing SATA ports
> > correctly directly from the AHCI MVEBU driver. This way, we will be
> > able to drop the hack from the ICU driver. IOW, when the A8k
> > compatible string is used and there is no sub-nodes in the DT, we
> > fake the creation and mapping of the second (missing) interrupt.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/ata/ahci_mvebu.c       | 29 ++++++++++++++++++++++++++++-
> >  drivers/ata/libahci_platform.c |  3 +++
> >  include/linux/ahci_platform.h  |  1 +
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
> > index 8671aa8179fa..8c981b8d6e4d 100644
> > --- a/drivers/ata/ahci_mvebu.c
> > +++ b/drivers/ata/ahci_mvebu.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/mbus.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/platform_device.h>
> >  #include "ahci.h"
> >  
> > @@ -28,9 +29,13 @@
> >  #define AHCI_WINDOW_BASE(win)	(0x64 + ((win) << 4))
> >  #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
> >  
> > +#define ICU_SATA0_ICU_ID 109
> > +#define ICU_SATA1_ICU_ID 107
> > +
> >  struct ahci_mvebu_plat_data {
> >  	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
> >  	unsigned int host_flags;
> > +	unsigned int resource_flags;
> >  };
> >  
> >  static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
> > @@ -101,6 +106,27 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
> >  static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
> >  				       struct device *dev)
> >  {
> > +	struct device_node *np = of_irq_find_parent(dev->of_node);
> > +	struct irq_data *irqd = irq_get_irq_data(hpriv->irqs[0]);
> > +	int host_irq = irqd ? irqd_to_hwirq(irqd) : 0;
> > +	int missing_irq = (host_irq == ICU_SATA1_ICU_ID) ?
> > +		ICU_SATA0_ICU_ID : ICU_SATA1_ICU_ID;
> > +	struct irq_fwspec fwspec = {
> > +		.fwnode = of_node_to_fwnode(np),
> > +		.param_count = 2,
> > +		.param = {missing_irq, IRQ_TYPE_LEVEL_HIGH},
> > +	};
> > +
> > +	if (of_get_child_count(dev->of_node))
> > +		return 0;
> > +
> > +	hpriv->irqs[1] = irq_create_fwspec_mapping(&fwspec);
> > +	if (hpriv->irqs[1]) {
> > +		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
> > +		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
> > +		hpriv->mask_port_map = GENMASK(1, 0);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -200,7 +226,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
> >  	if (!pdata)
> >  		return -EINVAL;
> >  
> > -	hpriv = ahci_platform_get_resources(pdev, 0);
> > +	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);  
> 
> This seems to be the only use of resource_flags in the whole series. Why
> can't you just pass AHCI_PLATFORM_A8K_QUIRK as a parameter and not
> allocate an extra field in the platform data structure?

Because this driver supports 3 different compatibles and the a8k
compatible is the only one needing this flag. Another solution would
be to declare on the stack a 'resource_flags' variable and set it to
AHCI_PLATFORM_A8K_QUIRK only when using the right compatible. I
suppose in this case, using an entry of a platform data structure is
the way to go (cleaner, more readable, at the cost of an extra 4
bytes). Unless you insist on this point I will keep this organization.

> 
> >  	if (IS_ERR(hpriv))
> >  		return PTR_ERR(hpriv);
> >  
> > @@ -240,6 +266,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
> >  
> >  static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
> >  	.plat_config = ahci_mvebu_armada_8k_config,
> > +	.resource_flags = AHCI_PLATFORM_A8K_QUIRK,
> >  };
> >  
> >  static const struct of_device_id ahci_mvebu_of_match[] = {
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 347ff14aedd2..620e5f27fdd7 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -484,6 +484,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >  	if (!child_nodes)
> >  		hpriv->nports = 1;
> >  
> > +	if (!child_nodes && flags & AHCI_PLATFORM_A8K_QUIRK)  
> 
> Consider using braces around the bitwise operators.

Sure!

> 
> > +		hpriv->nports = 2;
> > +
> >  	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
> >  	if (!hpriv->phys) {
> >  		rc = -ENOMEM;
> > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> > index eaedca5fe6fc..57465ba6bb15 100644
> > --- a/include/linux/ahci_platform.h
> > +++ b/include/linux/ahci_platform.h
> > @@ -44,5 +44,6 @@ int ahci_platform_suspend(struct device *dev);
> >  int ahci_platform_resume(struct device *dev);
> >  
> >  #define AHCI_PLATFORM_GET_RESETS	0x01
> > +#define AHCI_PLATFORM_A8K_QUIRK		0x02
> >  
> >  #endif /* _AHCI_PLATFORM_H */
> >   
> 
> Thanks,
> 
> 	M.


Thanks,
Miquèl

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

[PATCH v4 09/10] irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack

[Miquel Raynal]

When writing the driver, a hack was introduced to configure both SATA
interrupts regardless of the port in use to overcome a limitation in
the SATA core. Now that this limitation has been addressed and the
hack moved in the (historically) responsible SATA driver,
ahci_{platform,mvebu}.c, let's clean this driver section.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/irqchip/irq-mvebu-icu.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
index 547045d89c4b..f3b43f63fe2e 100644
--- a/drivers/irqchip/irq-mvebu-icu.c
+++ b/drivers/irqchip/irq-mvebu-icu.c
@@ -38,8 +38,6 @@
 
 /* ICU definitions */
 #define ICU_MAX_IRQS		207
-#define ICU_SATA0_ICU_ID	109
-#define ICU_SATA1_ICU_ID	107
 
 struct mvebu_icu_subset_data {
 	unsigned int icu_group;
@@ -111,22 +109,6 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 
 	writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq));
-
-	/*
-	 * The SATA unit has 2 ports, and a dedicated ICU entry per
-	 * port. The ahci sata driver supports only one irq interrupt
-	 * per SATA unit. To solve this conflict, we configure the 2
-	 * SATA wired interrupts in the south bridge into 1 GIC
-	 * interrupt in the north bridge. Even if only a single port
-	 * is enabled, if sata node is enabled, both interrupts are
-	 * configured (regardless of which port is actually in use).
-	 */
-	if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) {
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID));
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID));
-	}
 }
 
 static struct irq_chip mvebu_icu_nsr_chip = {
-- 
2.19.1


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

Re: [PATCH v4 09/10] irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack

[Marc Zyngier]

On 21/05/2019 15:30, Miquel Raynal wrote:
> When writing the driver, a hack was introduced to configure both SATA
> interrupts regardless of the port in use to overcome a limitation in
> the SATA core. Now that this limitation has been addressed and the
> hack moved in the (historically) responsible SATA driver,
> ahci_{platform,mvebu}.c, let's clean this driver section.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

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

[PATCH v4 10/10] arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

[Miquel Raynal]

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

There are two SATA ports per CP110. Each of them has a dedicated
interrupt. Describe the real hardware by adding two SATA ports to the
CP110 SATA node.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
index 4d6e4a097f72..f9fd89961785 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
@@ -301,10 +301,21 @@
 			"generic-ahci";
 			reg = <0x540000 0x30000>;
 			dma-coherent;
-			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&CP110_LABEL(clk) 1 15>,
 				 <&CP110_LABEL(clk) 1 16>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 			status = "disabled";
+
+			sata-port@0 {
+				reg = <0>;
+				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
+			};
+
+			sata-port@1 {
+				reg = <1>;
+				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		CP110_LABEL(xor0): xor@6a0000 {
-- 
2.19.1


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