* [3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"
@ 2018-05-17 12:58 Marc Zyngier
0 siblings, 0 replies; 3+ messages in thread
From: Marc Zyngier @ 2018-05-17 12:58 UTC (permalink / raw)
To: linux-usb
Cc: Mathias Nyman, Christian Brauns, Domenico Andreoli,
Bockholdt Arne, Ard Biesheuvel, Faiz Abbas, Troy Kisky
This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.
Now that we can properly reset the uPD72020x without a hard PCI reset,
let's get rid of the existing quirks.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
drivers/usb/host/pci-quirks.c | 20 --------------------
drivers/usb/host/pci-quirks.h | 1 -
drivers/usb/host/xhci-pci.c | 7 -------
drivers/usb/host/xhci.c | 21 ++++++++++++---------
drivers/usb/host/xhci.h | 2 +-
5 files changed, 13 insertions(+), 38 deletions(-)
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..3625a5c1a41b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
}
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
-
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
-{
- /*
- * Our dear uPD72020{1,2} friend only partially resets when
- * asked to via the XHCI interface, and may end up doing DMA
- * at the wrong addresses, as it keeps the top 32bit of some
- * addresses from its previous programming under obscure
- * circumstances.
- * Give it a good wack at probe time. Unfortunately, this
- * needs to happen before we've had a chance to discover any
- * quirk, or the system will be in a rather bad state.
- */
- if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
- (pdev->device == 0x0014 || pdev->device == 0x0015))
- return true;
-
- return false;
-}
-EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 4ca0d9b7e463..63c633077d9e 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
void sb800_prefetch(struct device *dev, int on);
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
bool usb_amd_pt_check_port(struct device *device, int port);
#else
struct pci_dev;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index e0a0a12871e2..6372edf339d9 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
driver = (struct hc_driver *)id->driver_data;
- /* For some HW implementation, a XHCI reset is just not enough... */
- if (usb_xhci_needs_pci_reset(dev)) {
- dev_info(&dev->dev, "Resetting\n");
- if (pci_reset_function_locked(dev))
- dev_warn(&dev->dev, "Reset failed");
- }
-
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(&dev->dev);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index cd6b48c43007..07272d1ce32a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4923,16 +4923,19 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
/*
* Some Renesas controllers get into a weird state if they are
- * reset while programmed with 64bit addresses (they will
- * preserve the top half of the address in internal, non
- * visible registers). You end up with half the address coming
- * from the kernel, and the other half coming from the
- * firmware. Also, changing the programming leads to extra
- * accesses even if the controller is supposed to be
- * halted. The controller ends up with a fatal fault, and is
- * then ripe for being properly reset.
+ * reset while programmed with 64bit addresses (they will preserve
+ * the top half of the address in internal, non visible
+ * registers). You end up with half the address coming from the
+ * kernel, and the other half coming from the firmware. Also,
+ * changing the programming leads to extra accesses even if the
+ * controller is supposed to be halted. The controller ends up with
+ * a fatal fault, and is then ripe for being properly reset.
+ *
+ * Special care is taken to only apply this if the device is behind
+ * an iommu. Doing anything when there is no iommu is definitely
+ * unsafe...
*/
- if (xhci->quirks & XHCI_ZERO_64B_REGS) {
+ if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
u64 val;
int i;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 5ea0b52b49cc..758864767fd4 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1831,7 +1831,7 @@ struct xhci_hcd {
#define XHCI_HW_LPM_DISABLE (1 << 29)
#define XHCI_SUSPEND_DELAY (1 << 30)
#define XHCI_INTEL_USB_ROLE_SW (1 << 31)
-#define XHCI_ZERO_64B_REGS (1 << 32)
+#define XHCI_ZERO_64B_REGS (1ULL << 32)
unsigned int num_active_eps;
unsigned int limit_active_eps;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"
@ 2018-05-17 13:29 Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2018-05-17 13:29 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-usb, Mathias Nyman, Christian Brauns, Domenico Andreoli,
Bockholdt Arne, Ard Biesheuvel, Faiz Abbas, Troy Kisky
On Thu, May 17, 2018 at 01:58:36PM +0100, Marc Zyngier wrote:
> This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.
>
> Now that we can properly reset the uPD72020x without a hard PCI reset,
> let's get rid of the existing quirks.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> drivers/usb/host/pci-quirks.c | 20 --------------------
> drivers/usb/host/pci-quirks.h | 1 -
> drivers/usb/host/xhci-pci.c | 7 -------
> drivers/usb/host/xhci.c | 21 ++++++++++++---------
> drivers/usb/host/xhci.h | 2 +-
> 5 files changed, 13 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 67ad4bb6919a..3625a5c1a41b 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
> }
> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
> -
> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
> -{
> - /*
> - * Our dear uPD72020{1,2} friend only partially resets when
> - * asked to via the XHCI interface, and may end up doing DMA
> - * at the wrong addresses, as it keeps the top 32bit of some
> - * addresses from its previous programming under obscure
> - * circumstances.
> - * Give it a good wack at probe time. Unfortunately, this
> - * needs to happen before we've had a chance to discover any
> - * quirk, or the system will be in a rather bad state.
> - */
> - if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
> - (pdev->device == 0x0014 || pdev->device == 0x0015))
> - return true;
> -
> - return false;
> -}
> -EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
> index 4ca0d9b7e463..63c633077d9e 100644
> --- a/drivers/usb/host/pci-quirks.h
> +++ b/drivers/usb/host/pci-quirks.h
> @@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
> void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
> void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
> void sb800_prefetch(struct device *dev, int on);
> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
> bool usb_amd_pt_check_port(struct device *device, int port);
> #else
> struct pci_dev;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index e0a0a12871e2..6372edf339d9 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>
> driver = (struct hc_driver *)id->driver_data;
>
> - /* For some HW implementation, a XHCI reset is just not enough... */
> - if (usb_xhci_needs_pci_reset(dev)) {
> - dev_info(&dev->dev, "Resetting\n");
> - if (pci_reset_function_locked(dev))
> - dev_warn(&dev->dev, "Reset failed");
> - }
> -
> /* Prevent runtime suspending between USB-2 and USB-3 initialization */
> pm_runtime_get_noresume(&dev->dev);
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index cd6b48c43007..07272d1ce32a 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4923,16 +4923,19 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>
> /*
> * Some Renesas controllers get into a weird state if they are
> - * reset while programmed with 64bit addresses (they will
> - * preserve the top half of the address in internal, non
> - * visible registers). You end up with half the address coming
> - * from the kernel, and the other half coming from the
> - * firmware. Also, changing the programming leads to extra
> - * accesses even if the controller is supposed to be
> - * halted. The controller ends up with a fatal fault, and is
> - * then ripe for being properly reset.
> + * reset while programmed with 64bit addresses (they will preserve
> + * the top half of the address in internal, non visible
> + * registers). You end up with half the address coming from the
> + * kernel, and the other half coming from the firmware. Also,
> + * changing the programming leads to extra accesses even if the
> + * controller is supposed to be halted. The controller ends up with
> + * a fatal fault, and is then ripe for being properly reset.
> + *
> + * Special care is taken to only apply this if the device is behind
> + * an iommu. Doing anything when there is no iommu is definitely
> + * unsafe...
> */
> - if (xhci->quirks & XHCI_ZERO_64B_REGS) {
> + if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
> u64 val;
> int i;
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 5ea0b52b49cc..758864767fd4 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1831,7 +1831,7 @@ struct xhci_hcd {
> #define XHCI_HW_LPM_DISABLE (1 << 29)
> #define XHCI_SUSPEND_DELAY (1 << 30)
> #define XHCI_INTEL_USB_ROLE_SW (1 << 31)
> -#define XHCI_ZERO_64B_REGS (1 << 32)
> +#define XHCI_ZERO_64B_REGS (1ULL << 32)
Why change this now? Didn't you just add it in the previous patch?
And BIT_ULL() is what you want here, right?
thanks,
greg k-h
---
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"
@ 2018-05-18 16:04 Marc Zyngier
0 siblings, 0 replies; 3+ messages in thread
From: Marc Zyngier @ 2018-05-18 16:04 UTC (permalink / raw)
To: Greg KH
Cc: linux-usb, Mathias Nyman, Christian Brauns, Domenico Andreoli,
Bockholdt Arne, Ard Biesheuvel, Faiz Abbas, Troy Kisky
On 17/05/18 14:29, Greg KH wrote:
> On Thu, May 17, 2018 at 01:58:36PM +0100, Marc Zyngier wrote:
>> This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.
>>
>> Now that we can properly reset the uPD72020x without a hard PCI reset,
>> let's get rid of the existing quirks.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> drivers/usb/host/pci-quirks.c | 20 --------------------
>> drivers/usb/host/pci-quirks.h | 1 -
>> drivers/usb/host/xhci-pci.c | 7 -------
>> drivers/usb/host/xhci.c | 21 ++++++++++++---------
>> drivers/usb/host/xhci.h | 2 +-
>> 5 files changed, 13 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 67ad4bb6919a..3625a5c1a41b 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
>> }
>> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
>> PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
>> -
>> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
>> -{
>> - /*
>> - * Our dear uPD72020{1,2} friend only partially resets when
>> - * asked to via the XHCI interface, and may end up doing DMA
>> - * at the wrong addresses, as it keeps the top 32bit of some
>> - * addresses from its previous programming under obscure
>> - * circumstances.
>> - * Give it a good wack at probe time. Unfortunately, this
>> - * needs to happen before we've had a chance to discover any
>> - * quirk, or the system will be in a rather bad state.
>> - */
>> - if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> - (pdev->device == 0x0014 || pdev->device == 0x0015))
>> - return true;
>> -
>> - return false;
>> -}
>> -EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
>> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
>> index 4ca0d9b7e463..63c633077d9e 100644
>> --- a/drivers/usb/host/pci-quirks.h
>> +++ b/drivers/usb/host/pci-quirks.h
>> @@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
>> void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
>> void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
>> void sb800_prefetch(struct device *dev, int on);
>> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
>> bool usb_amd_pt_check_port(struct device *device, int port);
>> #else
>> struct pci_dev;
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index e0a0a12871e2..6372edf339d9 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>
>> driver = (struct hc_driver *)id->driver_data;
>>
>> - /* For some HW implementation, a XHCI reset is just not enough... */
>> - if (usb_xhci_needs_pci_reset(dev)) {
>> - dev_info(&dev->dev, "Resetting\n");
>> - if (pci_reset_function_locked(dev))
>> - dev_warn(&dev->dev, "Reset failed");
>> - }
>> -
>> /* Prevent runtime suspending between USB-2 and USB-3 initialization */
>> pm_runtime_get_noresume(&dev->dev);
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index cd6b48c43007..07272d1ce32a 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4923,16 +4923,19 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>>
>> /*
>> * Some Renesas controllers get into a weird state if they are
>> - * reset while programmed with 64bit addresses (they will
>> - * preserve the top half of the address in internal, non
>> - * visible registers). You end up with half the address coming
>> - * from the kernel, and the other half coming from the
>> - * firmware. Also, changing the programming leads to extra
>> - * accesses even if the controller is supposed to be
>> - * halted. The controller ends up with a fatal fault, and is
>> - * then ripe for being properly reset.
>> + * reset while programmed with 64bit addresses (they will preserve
>> + * the top half of the address in internal, non visible
>> + * registers). You end up with half the address coming from the
>> + * kernel, and the other half coming from the firmware. Also,
>> + * changing the programming leads to extra accesses even if the
>> + * controller is supposed to be halted. The controller ends up with
>> + * a fatal fault, and is then ripe for being properly reset.
>> + *
>> + * Special care is taken to only apply this if the device is behind
>> + * an iommu. Doing anything when there is no iommu is definitely
>> + * unsafe...
>> */
>> - if (xhci->quirks & XHCI_ZERO_64B_REGS) {
>> + if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
>> u64 val;
>> int i;
>>
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 5ea0b52b49cc..758864767fd4 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1831,7 +1831,7 @@ struct xhci_hcd {
>> #define XHCI_HW_LPM_DISABLE (1 << 29)
>> #define XHCI_SUSPEND_DELAY (1 << 30)
>> #define XHCI_INTEL_USB_ROLE_SW (1 << 31)
>> -#define XHCI_ZERO_64B_REGS (1 << 32)
>> +#define XHCI_ZERO_64B_REGS (1ULL << 32)
>
> Why change this now? Didn't you just add it in the previous patch?
Because I'm a moron, applied the fixup to the wrong patch, and didn't do
any further check...
> And BIT_ULL() is what you want here, right?
Works for me.
Thanks,
M.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-18 16:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-17 13:29 [3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue" Greg KH
-- strict thread matches above, loose matches on Subject: below --
2018-05-18 16:04 Marc Zyngier
2018-05-17 12:58 Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).