diff mbox

[1/4] ata: ahci_platform: Add ACPI support for AMD Seattle SATA controller

Message ID 1410828446-28502-2-git-send-email-suravee.suthikulpanit@amd.com
State New
Headers show

Commit Message

Suthikulpanit, Suravee Sept. 16, 2014, 12:47 a.m. UTC
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch adds ACPI match table in ahci_platform. The table includes
the acpi_device_id to match AMD Seattle SATA controller with following
asl structure in DSDT:

    Device (SATA0)
    {
      Name(_HID, "AMDI0600")	// Seattle AHSATA
      Name (_CCA, 1)		// Cache-coherent controller
      Name (_CRS, ResourceTemplate ()
      {
        Memory32Fixed (ReadWrite, 0xE0300000, 0x00010000)
        Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 387 }
      })
    }

Also, since ATA driver should not require PCI support for ATA_ACPI,
this patch removes dependency in the driver/ata/Kconfig.
---
 drivers/ata/Kconfig         |  2 +-
 drivers/ata/ahci_platform.c | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Suthikulpanit, Suravee Oct. 1, 2014, 9:19 p.m. UTC | #1
On 9/16/2014 8:26 PM, Matthew Garrett wrote:
> On Mon, Sep 15, 2014 at 07:47:23PM -0500, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> This patch adds ACPI match table in ahci_platform. The table includes
>> the acpi_device_id to match AMD Seattle SATA controller with following
>> asl structure in DSDT:
>>
>>      Device (SATA0)
>>      {
>>        Name(_HID, "AMDI0600")	// Seattle AHSATA
>
> There really ought to be a well-defined PNPID for AHCI, so you can _HID
> to AMD and _CID to something generic. That way we won't have:
>
>> +#ifdef CONFIG_ATA_ACPI
>> +static const struct acpi_device_id ahci_acpi_match[] = {
>> +	{ "AMDI0600", 0 }, /* AMD Seattle AHCI */
>> +	{ },
>> +};
>
> utter sadness here. Really, please don't end up in a situation where we
> need to add device-specific IDs to a generic driver.
>
Matthew,

Currently, there is no _CID defined for generic AHCI. We will work on 
proposing one, and provide update patches for including the new ID.

Thanks,
Suravee

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Oct. 2, 2014, 8:39 a.m. UTC | #2
On Wednesday 01 October 2014 16:19:37 Suravee Suthikulanit wrote:
> On 9/16/2014 8:26 PM, Matthew Garrett wrote:
> > On Mon, Sep 15, 2014 at 07:47:23PM -0500, suravee.suthikulpanit@amd.com wrote:
> >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >>
> >> This patch adds ACPI match table in ahci_platform. The table includes
> >> the acpi_device_id to match AMD Seattle SATA controller with following
> >> asl structure in DSDT:
> >>
> >>      Device (SATA0)
> >>      {
> >>        Name(_HID, "AMDI0600")        // Seattle AHSATA
> >
> > There really ought to be a well-defined PNPID for AHCI, so you can _HID
> > to AMD and _CID to something generic. That way we won't have:
> >
> >> +#ifdef CONFIG_ATA_ACPI
> >> +static const struct acpi_device_id ahci_acpi_match[] = {
> >> +    { "AMDI0600", 0 }, /* AMD Seattle AHCI */
> >> +    { },
> >> +};
> >
> > utter sadness here. Really, please don't end up in a situation where we
> > need to add device-specific IDs to a generic driver.
> >
> Matthew,
> 
> Currently, there is no _CID defined for generic AHCI. We will work on 
> proposing one, and provide update patches for including the new ID.
> 

I think part of the problem is that there is no specification for what
an AHCI device should look like when it's not connected to a PCI
bus, the AHCI document published by Intel just states:

"AHCI is a PCI class device that acts as a data movement engine
between system memory and Serial ATA devices."

It also requires the PCI config space to have the PM capability
registers and (optionally) the MSI capability registers.
There are lots of chips we support in Linux with the ahci-platform
driver, but they are not actually compliant because they cannot
use the pmcap registers but instead typically rely on setting
external clock/phy/regulator/pinctrl registers.

The ARM SBSA document just requires any SATA controller to be AHCI
compliant but does not explain what that means in the case where
it's not a PCI device.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Oct. 6, 2014, 4:31 p.m. UTC | #3
On Thu, Oct 02, 2014 at 10:39:18AM +0200, Arnd Bergmann wrote:

> I think part of the problem is that there is no specification for what
> an AHCI device should look like when it's not connected to a PCI
> bus, the AHCI document published by Intel just states:

One thing that has come out of further discussion on this - ACPI defines 
a _CLS object, which allows ACPI devices to declare compatibility with a 
PCI class device. We don't have support for it at the moment, but it 
would be easy enough to add this to the kernel and then just expose the 
AHCI PCI class via _CLS. That would avoid us having problems with people 
doing similar things with USB hosts.
Arnd Bergmann Oct. 6, 2014, 6:19 p.m. UTC | #4
On Monday 06 October 2014 17:31:47 Matthew Garrett wrote:
> On Thu, Oct 02, 2014 at 10:39:18AM +0200, Arnd Bergmann wrote:
> 
> > I think part of the problem is that there is no specification for what
> > an AHCI device should look like when it's not connected to a PCI
> > bus, the AHCI document published by Intel just states:
> 
> One thing that has come out of further discussion on this - ACPI defines 
> a _CLS object, which allows ACPI devices to declare compatibility with a 
> PCI class device. We don't have support for it at the moment, but it 
> would be easy enough to add this to the kernel and then just expose the 
> AHCI PCI class via _CLS. That would avoid us having problems with people 
> doing similar things with USB hosts.

Interesting. Does this also define a way to get access to registers
that are normally in PCI config space, provided they are accessible at
all?

I'm guessing that it's not extremely important, given that so far all
platforms are doing ok without the power management registers or MSI,
but I guess it would be nice if we could support those.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Oct. 6, 2014, 6:21 p.m. UTC | #5
On Mon, Oct 06, 2014 at 08:19:37PM +0200, Arnd Bergmann wrote:

> Interesting. Does this also define a way to get access to registers
> that are normally in PCI config space, provided they are accessible at
> all?

Unfortunately not. I'd assume that PM registers are expected to be 
accessed via the _PS* methods instead. Does MSI make sense outside the 
context of PCI interrupts?
Arnd Bergmann Oct. 6, 2014, 6:44 p.m. UTC | #6
On Monday 06 October 2014 19:21:53 Matthew Garrett wrote:
> On Mon, Oct 06, 2014 at 08:19:37PM +0200, Arnd Bergmann wrote:
> 
> > Interesting. Does this also define a way to get access to registers
> > that are normally in PCI config space, provided they are accessible at
> > all?
> 
> Unfortunately not. I'd assume that PM registers are expected to be 
> accessed via the _PS* methods instead. Does MSI make sense outside the 
> context of PCI interrupts?

Yes, the ARM GIC has a weird sense of what MSI is used for, and
apparently some SoC vendors have started using MSI by default for
all on-chip peripherals.

A patch series to extend MSI to platform devices is currently
under review.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Oct. 6, 2014, 6:47 p.m. UTC | #7
On Mon, Oct 06, 2014 at 08:44:35PM +0200, Arnd Bergmann wrote:
> On Monday 06 October 2014 19:21:53 Matthew Garrett wrote:
> > Unfortunately not. I'd assume that PM registers are expected to be 
> > accessed via the _PS* methods instead. Does MSI make sense outside the 
> > context of PCI interrupts?
> 
> Yes, the ARM GIC has a weird sense of what MSI is used for, and
> apparently some SoC vendors have started using MSI by default for
> all on-chip peripherals.
> 
> A patch series to extend MSI to platform devices is currently
> under review.

Mm. Yeah, it doesn't seem like there's any ACPI-defined mechanism for 
MSI control. Let's chat about this next week?
diff mbox

Patch

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e1b9278..f2e6c9e 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -48,7 +48,7 @@  config ATA_VERBOSE_ERROR
 
 config ATA_ACPI
 	bool "ATA ACPI Support"
-	depends on ACPI && PCI
+	depends on ACPI
 	default y
 	help
 	  This option adds support for ATA-related ACPI objects.
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index f61ddb9..3499bab 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -20,6 +20,9 @@ 
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
+#ifdef CONFIG_ATA_ACPI
+#include <linux/acpi.h>
+#endif
 #include "ahci.h"
 
 static const struct ata_port_info ahci_port_info = {
@@ -87,6 +90,13 @@  static const struct of_device_id ahci_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
 
+#ifdef CONFIG_ATA_ACPI
+static const struct acpi_device_id ahci_acpi_match[] = {
+	{ "AMDI0600", 0 }, /* AMD Seattle AHCI */
+	{ },
+};
+#endif
+
 static struct platform_driver ahci_driver = {
 	.probe = ahci_probe,
 	.remove = ata_platform_remove_one,
@@ -94,6 +104,9 @@  static struct platform_driver ahci_driver = {
 		.name = "ahci",
 		.owner = THIS_MODULE,
 		.of_match_table = ahci_of_match,
+#ifdef CONFIG_ATA_ACPI
+		.acpi_match_table = ACPI_PTR(ahci_acpi_match),
+#endif
 		.pm = &ahci_pm_ops,
 	},
 };