| Message ID | 1415119833-9135-2-git-send-email-mlangsdo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: > Provide the methods to let ACPI identify the need to use > xhci-platform. Change the Kconfig files so the > xhci-plat.o file is selectable during kernel config. > > Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com> > --- > Changes from v1 > Renamed from "add support for APM X-Gene to xhci-platform" > Removed changes to arm64/Kconfig > Made CONFIG_USB_XHCI_PLATFORM a user selectable config option > > drivers/usb/host/Kconfig | 7 ++++++- > drivers/usb/host/xhci-plat.c | 11 +++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 82800a7..060a2361 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -27,7 +27,12 @@ config USB_XHCI_HCD > if USB_XHCI_HCD > > config USB_XHCI_PLATFORM > - tristate > + tristate "xHCI platform driver support" > + --help-- > + Say 'Y' to enable the support for the xHCI host controller > + as a platform device. Many ARM SoCs provide USB this way. > + > + If unsure, say 'Y'. You really want a 'default Y' response here? That's not good at all, what happens if I select this on a system without such hardware? > > config USB_XHCI_MVEBU > tristate "xHCI support for Marvell Armada 375/38x" > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 91c7557..3db47ea 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/slab.h> > #include <linux/usb/xhci_pdriver.h> > +#include <linux/acpi.h> > > #include "xhci.h" > #include "xhci-mvebu.h" > @@ -287,6 +288,15 @@ static const struct of_device_id usb_xhci_of_match[] = { > MODULE_DEVICE_TABLE(of, usb_xhci_of_match); > #endif > > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id usb_xhci_acpi_match[] = { > + /* APM X-Gene USB Controller */ > + { "PNP0D10", }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); > +#endif That looks like a very "generic" PNP value, are you sure it is assigned only to this specific device? > + > static struct platform_driver usb_xhci_driver = { > .probe = xhci_plat_probe, > .remove = xhci_plat_remove, > @@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { > .name = "xhci-hcd", > .pm = DEV_PM_OPS, > .of_match_table = of_match_ptr(usb_xhci_of_match), > + .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), Shouldn't the reworked driver core code handle this differently with the ability to handle either OF or ACPI in the same driver? thanks, greg k-h
On 11/04/2014 11:12 AM, Greg KH wrote: > On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: >> Provide the methods to let ACPI identify the need to use >> xhci-platform. Change the Kconfig files so the >> xhci-plat.o file is selectable during kernel config. >> >> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com> >> --- >> Changes from v1 >> Renamed from "add support for APM X-Gene to xhci-platform" >> Removed changes to arm64/Kconfig >> Made CONFIG_USB_XHCI_PLATFORM a user selectable config option >> >> drivers/usb/host/Kconfig | 7 ++++++- >> drivers/usb/host/xhci-plat.c | 11 +++++++++++ >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig >> index 82800a7..060a2361 100644 >> --- a/drivers/usb/host/Kconfig >> +++ b/drivers/usb/host/Kconfig >> @@ -27,7 +27,12 @@ config USB_XHCI_HCD >> if USB_XHCI_HCD >> >> config USB_XHCI_PLATFORM >> - tristate >> + tristate "xHCI platform driver support" >> + --help-- >> + Say 'Y' to enable the support for the xHCI host controller >> + as a platform device. Many ARM SoCs provide USB this way. >> + >> + If unsure, say 'Y'. > > You really want a 'default Y' response here? > > That's not good at all, what happens if I select this on a system > without such hardware? Based on testing with my 2 x86 systems, nothing bad, but I'll make it 'M' because that's correct. >> config USB_XHCI_MVEBU >> tristate "xHCI support for Marvell Armada 375/38x" >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index 91c7557..3db47ea 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -18,6 +18,7 @@ >> #include <linux/platform_device.h> >> #include <linux/slab.h> >> #include <linux/usb/xhci_pdriver.h> >> +#include <linux/acpi.h> >> >> #include "xhci.h" >> #include "xhci-mvebu.h" >> @@ -287,6 +288,15 @@ static const struct of_device_id usb_xhci_of_match[] = { >> MODULE_DEVICE_TABLE(of, usb_xhci_of_match); >> #endif >> >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id usb_xhci_acpi_match[] = { >> + /* APM X-Gene USB Controller */ >> + { "PNP0D10", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); >> +#endif > > That looks like a very "generic" PNP value, are you sure it is assigned > only to this specific device? I'll adjust the comment. It is a generic PNP value and a lot of other SoCs will use this controller. >> + >> static struct platform_driver usb_xhci_driver = { >> .probe = xhci_plat_probe, >> .remove = xhci_plat_remove, >> @@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { >> .name = "xhci-hcd", >> .pm = DEV_PM_OPS, >> .of_match_table = of_match_ptr(usb_xhci_of_match), >> + .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), > > Shouldn't the reworked driver core code handle this differently with the > ability to handle either OF or ACPI in the same driver? I'm not sure I understand the question. With these changes, the driver handles both ACPI and DTB/OF. It's the same style of code as used in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. Why do you think this code isn't correct? --Mark Langsdorf
On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote: > >> static struct platform_driver usb_xhci_driver = { > >> .probe = xhci_plat_probe, > >> .remove = xhci_plat_remove, > >>@@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { > >> .name = "xhci-hcd", > >> .pm = DEV_PM_OPS, > >> .of_match_table = of_match_ptr(usb_xhci_of_match), > >>+ .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), > > > >Shouldn't the reworked driver core code handle this differently with the > >ability to handle either OF or ACPI in the same driver? > > I'm not sure I understand the question. With these changes, the driver > handles both ACPI and DTB/OF. It's the same style of code as used > in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. > Why do you think this code isn't correct? There is a new framework in the kernel that keeps a driver from having to query both of and acpi to get the needed resources, it just does one query and depending on the platform, everything "just works". Shouldn't that be used here as well? thanks, greg k-h
On 11/05/2014 01:11 PM, Greg KH wrote: > On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote: >>>> static struct platform_driver usb_xhci_driver = { >>>> .probe = xhci_plat_probe, >>>> .remove = xhci_plat_remove, >>>> @@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { >>>> .name = "xhci-hcd", >>>> .pm = DEV_PM_OPS, >>>> .of_match_table = of_match_ptr(usb_xhci_of_match), >>>> + .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), >>> >>> Shouldn't the reworked driver core code handle this differently with the >>> ability to handle either OF or ACPI in the same driver? >> >> I'm not sure I understand the question. With these changes, the driver >> handles both ACPI and DTB/OF. It's the same style of code as used >> in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. >> Why do you think this code isn't correct? > > There is a new framework in the kernel that keeps a driver from having > to query both of and acpi to get the needed resources, it just does one > query and depending on the platform, everything "just works". Shouldn't > that be used here as well? Would you send me a pointer to a driver that's using this new framework? I can't find any references to it and all the other drivers that support ACPI and OF are doing it the way I'm doing it. --Mark Langsdorf
On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote: > On 11/05/2014 01:11 PM, Greg KH wrote: > >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote: > >>>> static struct platform_driver usb_xhci_driver = { > >>>> .probe = xhci_plat_probe, > >>>> .remove = xhci_plat_remove, > >>>>@@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { > >>>> .name = "xhci-hcd", > >>>> .pm = DEV_PM_OPS, > >>>> .of_match_table = of_match_ptr(usb_xhci_of_match), > >>>>+ .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), > >>> > >>>Shouldn't the reworked driver core code handle this differently with the > >>>ability to handle either OF or ACPI in the same driver? > >> > >>I'm not sure I understand the question. With these changes, the driver > >>handles both ACPI and DTB/OF. It's the same style of code as used > >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. > >>Why do you think this code isn't correct? > > > >There is a new framework in the kernel that keeps a driver from having > >to query both of and acpi to get the needed resources, it just does one > >query and depending on the platform, everything "just works". Shouldn't > >that be used here as well? > > Would you send me a pointer to a driver that's using this new > framework? I can't find any references to it and all the other > drivers that support ACPI and OF are doing it the way I'm doing > it. See the email on lkml: Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support for the latest patch series. thanks, greg k-h
On Wednesday 05 November 2014 11:55:07 Greg KH wrote: > On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote: > > On 11/05/2014 01:11 PM, Greg KH wrote: > > >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote: > > >>>> static struct platform_driver usb_xhci_driver = { > > >>>> .probe = xhci_plat_probe, > > >>>> .remove = xhci_plat_remove, > > >>>>@@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { > > >>>> .name = "xhci-hcd", > > >>>> .pm = DEV_PM_OPS, > > >>>> .of_match_table = of_match_ptr(usb_xhci_of_match), > > >>>>+ .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), > > >>> > > >>>Shouldn't the reworked driver core code handle this differently with the > > >>>ability to handle either OF or ACPI in the same driver? > > >> > > >>I'm not sure I understand the question. With these changes, the driver > > >>handles both ACPI and DTB/OF. It's the same style of code as used > > >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. > > >>Why do you think this code isn't correct? > > > > > >There is a new framework in the kernel that keeps a driver from having > > >to query both of and acpi to get the needed resources, it just does one > > >query and depending on the platform, everything "just works". Shouldn't > > >that be used here as well? > > > > Would you send me a pointer to a driver that's using this new > > framework? I can't find any references to it and all the other > > drivers that support ACPI and OF are doing it the way I'm doing > > it. > > See the email on lkml: > Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support > > for the latest patch series. > The _DSD approach is for devices that do not follow the ACPI specification but do have a DT binding. Those will work without the .acpi_match_table entry when the firmware uses the compatible value in the new properties. In this case, the device does have an official ACPI ID "PNP0D10", so we should use that for compatibility with other operating systems and with BIOS versions that provide the standard IDs. Arnd
On Wed, Nov 05, 2014 at 09:41:23PM +0100, Arnd Bergmann wrote: > On Wednesday 05 November 2014 11:55:07 Greg KH wrote: > > On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote: > > > On 11/05/2014 01:11 PM, Greg KH wrote: > > > >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote: > > > >>>> static struct platform_driver usb_xhci_driver = { > > > >>>> .probe = xhci_plat_probe, > > > >>>> .remove = xhci_plat_remove, > > > >>>>@@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { > > > >>>> .name = "xhci-hcd", > > > >>>> .pm = DEV_PM_OPS, > > > >>>> .of_match_table = of_match_ptr(usb_xhci_of_match), > > > >>>>+ .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), > > > >>> > > > >>>Shouldn't the reworked driver core code handle this differently with the > > > >>>ability to handle either OF or ACPI in the same driver? > > > >> > > > >>I'm not sure I understand the question. With these changes, the driver > > > >>handles both ACPI and DTB/OF. It's the same style of code as used > > > >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF. > > > >>Why do you think this code isn't correct? > > > > > > > >There is a new framework in the kernel that keeps a driver from having > > > >to query both of and acpi to get the needed resources, it just does one > > > >query and depending on the platform, everything "just works". Shouldn't > > > >that be used here as well? > > > > > > Would you send me a pointer to a driver that's using this new > > > framework? I can't find any references to it and all the other > > > drivers that support ACPI and OF are doing it the way I'm doing > > > it. > > > > See the email on lkml: > > Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support > > > > for the latest patch series. > > > > The _DSD approach is for devices that do not follow the ACPI specification > but do have a DT binding. Those will work without the .acpi_match_table > entry when the firmware uses the compatible value in the new properties. > > In this case, the device does have an official ACPI ID "PNP0D10", so we should > use that for compatibility with other operating systems and with BIOS > versions that provide the standard IDs. Ah, ok, nevermind then, sorry for the noise, I wasn't aware of this. greg k-h
On 11/04/2014 11:12 AM, Greg KH wrote: > On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: #endif >> >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id usb_xhci_acpi_match[] = { >> + /* APM X-Gene USB Controller */ >> + { "PNP0D10", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); >> +#endif > > That looks like a very "generic" PNP value, are you sure it is assigned > only to this specific device? Although this is a generic PNP device, the specific implementation I'm testing has issues with USB3. Is there a flag or function call that will disable the USB3 host while keeping the USB2 host?? My naive attempts in finding one mostly hung the machine. --Mark Langsdorf
On Thu, Nov 13, 2014 at 12:36:09PM -0600, Mark Langsdorf wrote: > On 11/04/2014 11:12 AM, Greg KH wrote: > >On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: > #endif > >> > >>+#ifdef CONFIG_ACPI > >>+static const struct acpi_device_id usb_xhci_acpi_match[] = { > >>+ /* APM X-Gene USB Controller */ > >>+ { "PNP0D10", }, > >>+ { } > >>+}; > >>+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); > >>+#endif > > > >That looks like a very "generic" PNP value, are you sure it is assigned > >only to this specific device? > > Although this is a generic PNP device, the specific implementation > I'm testing has issues with USB3. Is there a flag or function > call that will disable the USB3 host while keeping the USB2 > host?? My naive attempts in finding one mostly hung the machine. If this is an xhci chip, there is no stand-along USB 2 controller anymore, sorry. greg k-h
On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com> wrote: > On 11/04/2014 11:12 AM, Greg KH wrote: >> >> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: > > #endif >>> >>> >>> +#ifdef CONFIG_ACPI >>> +static const struct acpi_device_id usb_xhci_acpi_match[] = { >>> + /* APM X-Gene USB Controller */ >>> + { "PNP0D10", }, Mark, would it be better to use PRP0001 instead as in this patch? https://lkml.org/lkml/2014/9/16/230 >>> + { } >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); >>> +#endif >> >> >> That looks like a very "generic" PNP value, are you sure it is assigned >> only to this specific device? > > > Although this is a generic PNP device, the specific implementation > I'm testing has issues with USB3. Is there a flag or function > call that will disable the USB3 host while keeping the USB2 > host?? My naive attempts in finding one mostly hung the machine. > > --Mark Langsdorf > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/18/2014 02:05 PM, Feng Kan wrote: > On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com> wrote: >> On 11/04/2014 11:12 AM, Greg KH wrote: >>> >>> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: >> >> #endif >>>> >>>> >>>> +#ifdef CONFIG_ACPI >>>> +static const struct acpi_device_id usb_xhci_acpi_match[] = { >>>> + /* APM X-Gene USB Controller */ >>>> + { "PNP0D10", }, > > Mark, would it be better to use PRP0001 instead as in this patch? > https://lkml.org/lkml/2014/9/16/230 Quoting Arnd, "In this case, the device does have an official ACPI ID "PNP0D10", so we should use that for compatibility with other operating systems and with BIOS versions that provide the standard IDs." --Mark Langsdorf
On Tue, Nov 18, 2014 at 12:33 PM, Mark Langsdorf <mlangsdo@redhat.com> wrote: > On 11/18/2014 02:05 PM, Feng Kan wrote: >> >> On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com> >> wrote: >>> >>> On 11/04/2014 11:12 AM, Greg KH wrote: >>>> >>>> >>>> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote: >>> >>> >>> #endif >>>>> >>>>> >>>>> >>>>> +#ifdef CONFIG_ACPI >>>>> +static const struct acpi_device_id usb_xhci_acpi_match[] = { >>>>> + /* APM X-Gene USB Controller */ >>>>> + { "PNP0D10", }, >> >> >> Mark, would it be better to use PRP0001 instead as in this patch? >> https://lkml.org/lkml/2014/9/16/230 > > > Quoting Arnd, > > "In this case, the device does have an official ACPI ID "PNP0D10", > so we should use that for compatibility with other operating > systems and with BIOS versions that provide the standard IDs." > Yes, thanks. I missed that part, sorry for the spam then. > --Mark Langsdorf
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 82800a7..060a2361 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -27,7 +27,12 @@ config USB_XHCI_HCD if USB_XHCI_HCD config USB_XHCI_PLATFORM - tristate + tristate "xHCI platform driver support" + --help-- + Say 'Y' to enable the support for the xHCI host controller + as a platform device. Many ARM SoCs provide USB this way. + + If unsure, say 'Y'. config USB_XHCI_MVEBU tristate "xHCI support for Marvell Armada 375/38x" diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 91c7557..3db47ea 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -18,6 +18,7 @@ #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/usb/xhci_pdriver.h> +#include <linux/acpi.h> #include "xhci.h" #include "xhci-mvebu.h" @@ -287,6 +288,15 @@ static const struct of_device_id usb_xhci_of_match[] = { MODULE_DEVICE_TABLE(of, usb_xhci_of_match); #endif +#ifdef CONFIG_ACPI +static const struct acpi_device_id usb_xhci_acpi_match[] = { + /* APM X-Gene USB Controller */ + { "PNP0D10", }, + { } +}; +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); +#endif + static struct platform_driver usb_xhci_driver = { .probe = xhci_plat_probe, .remove = xhci_plat_remove, @@ -294,6 +304,7 @@ static struct platform_driver usb_xhci_driver = { .name = "xhci-hcd", .pm = DEV_PM_OPS, .of_match_table = of_match_ptr(usb_xhci_of_match), + .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match), }, }; MODULE_ALIAS("platform:xhci-hcd");
Provide the methods to let ACPI identify the need to use xhci-platform. Change the Kconfig files so the xhci-plat.o file is selectable during kernel config. Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com> --- Changes from v1 Renamed from "add support for APM X-Gene to xhci-platform" Removed changes to arm64/Kconfig Made CONFIG_USB_XHCI_PLATFORM a user selectable config option drivers/usb/host/Kconfig | 7 ++++++- drivers/usb/host/xhci-plat.c | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-)