| Message ID | 1410828446-28502-2-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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.
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
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?
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
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 --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, }, };
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(-)