ohci-platform: Add support for controllers with big-endian regs / descriptors
Subsystems (4)
PATCH 1/2 ohci-platform: Add support for controllers with big-endian regs / descriptors
Note this commit uses the same devicetree booleans for this as the ones already existing in the usb-ehci bindings.
--- Documentation/devicetree/bindings/usb/usb-ohci.txt | 3 +++ drivers/usb/host/Kconfig | 4 ++++ drivers/usb/host/ohci-platform.c | 27 ++++++++++++++++++++++ 3 files changed, 34 insertions(+)
Documentation/devicetree/bindings/usb/usb-ohci.txt +3 -0
@@ -6,6 +6,9 @@ Required properties: - interrupts : ohci controller interrupt Optional properties: +- big-endian-regs : boolean, set this for hcds with big-endian registers +- big-endian-desc : boolean, set this for hcds with big-endian descriptors +- big-endian : boolean, for hcds with big-endian-regs + big-endian-desc - clocks : a list of phandle + clock specifier pairs - phys : phandle + phy specifier pair - phy-names : "usb"
drivers/usb/host/Kconfig +4 -0
@@ -512,6 +512,10 @@ config USB_CNS3XXX_OHCI config USB_OHCI_HCD_PLATFORM tristate "Generic OHCI driver for a platform device" + # Always support LE, support BE on architectures which have readl_be + select USB_OHCI_LITTLE_ENDIAN + select USB_OHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_OHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default n ---help--- Adds an OHCI host driver for a generic platform device, which
drivers/usb/host/ohci-platform.c +27 -0
@@ -128,6 +128,7 @@ static int ohci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ohci_pdata *pdata =dev_get_platdata (&dev->dev);struct ohci_platform_priv *priv; +struct ohci_hcd *ohci;int err, irq, clk =0 ;if (usb_disabled ())
@@ -164,8 +165,34 @@ static int ohci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ohci_priv (hcd); + ohci =hcd_to_ohci (hcd);if (pdata == &ohci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ohci->flags |=OHCI_QUIRK_BE_MMIO ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ohci->flags |=OHCI_QUIRK_BE_DESC ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ohci->flags |=OHCI_QUIRK_BE_MMIO |OHCI_QUIRK_BE_DESC ; + +#ifndef CONFIG_USB_OHCI_BIG_ENDIAN_MMIO +if (ohci->flags &OHCI_QUIRK_BE_MMIO ) { +dev_err (&dev->dev, +"Error big-endian-regs not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd; + } +#endif +#ifndef CONFIG_USB_OHCI_BIG_ENDIAN_DESC +if (ohci->flags &OHCI_QUIRK_BE_DESC ) { +dev_err (&dev->dev, +"Error big-endian-desc not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd; + } +#endif priv->phy =devm_phy_get (&dev->dev,"usb" );if (IS_ERR (priv->phy)) { err =PTR_ERR (priv->phy);
Signature · 1 line
1.8.5.3
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
This uses the already documented devicetree booleans for this.
--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)
drivers/usb/host/Kconfig +3 -0
@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default n ---help--- Adds an EHCI host driver for a generic platform device, which
drivers/usb/host/ehci-platform.c +31 -2
@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);
@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())
@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ; + +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO +if (ehci->big_endian_mmio) { +dev_err (&dev->dev, +"Error big-endian-regs not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd; + } +#endif +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_DESC +if (ehci->big_endian_desc) { +dev_err (&dev->dev, +"Error big-endian-desc not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd; + } +#endif priv->phy =devm_phy_get (&dev->dev,"usb" );if (IS_ERR (priv->phy)) { err =PTR_ERR (priv->phy);
Signature · 1 line
1.8.5.3
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
2014/1/21 Hans de Goede <[email protected]>:
Quoted 1 line
This uses the already documented devicetree booleans for this.
(I would greatly appreciate if you could CC people who gave you feedback on this before)
A more informative commit message would be welcome, along with a reference to which Device Tree binding documentation you are referring to.
Quoted 12 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)drivers/usb/host/Kconfig +0
-0
drivers/usb/host/Kconfig config USB_EHCI_ATH79 · line 261
Quoted 6 lines
drivers/usb/host/Kconfig +3
-0@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)
I do not think this is that simple nor correct for at least Microblaze and MIPS since they can run in either BE or LE mode, and those specific platforms should already do the proper select at the board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64, although I believe some specific PPC64 boards can run in little-endian mode like the P-series, SPARC might too.
It seems to me that you should not touch this and keep the existing selects in place, if it turns out that the selects are missing the error messages you added below are catching those misuses.
Quoted 33 lines
drivers/usb/host/Kconfig +0
-0default n ---help--- Adds an EHCI host driver for a generic platform device, whichdrivers/usb/host/ehci-platform.c +6
-2@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 193
Quoted 10 lines
drivers/usb/host/ehci-platform.c +8
-0if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ;
Ok, so I am confused now, should you update pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly modify ehci->big_endian_{desc,mmio}, is not there any risk to undo what is done in ehci_platform_reset(), or is ehci_platform_reset() only called for non-DT cases?
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 198
Quoted 5 lines
drivers/usb/host/ehci-platform.c +5
-0+ +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO +if (ehci->big_endian_mmio) { +dev_err (&dev->dev, +"Error big-endian-regs not compiled in\n" );
I do not think using the Device Tree property name would be very informative since this is supposed to guard against misconfigurations for both DT and non-DT enabled platforms, how about something like the following:
"support for big-endian MMIO registers not enabled".
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 208
Quoted 10 lines
drivers/usb/host/ehci-platform.c +10
-0+ err = -EINVAL ; + goto err_put_hcd; + } +#endif +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_DESC +if (ehci->big_endian_desc) { +dev_err (&dev->dev, +"Error big-endian-desc not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd;
And here "support for big-endian descriptors not enabled".
Quoted 13 lines
drivers/usb/host/ehci-platform.c +2
-1+ } +#endif priv->phy =devm_phy_get (&dev->dev,"usb" );if (IS_ERR (priv->phy)) { err =PTR_ERR (priv->phy); --1.8.5.3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 1 line
Florian
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Hi,
Quoted 5 lines · Florian Fainelli
2014/1/21 Hans de Goede <[email protected]>:
Quoted 1 line
This uses the already documented devicetree booleans for this.
(I would greatly appreciate if you could CC people who gave you feedback on this before)
Will do.
Quoted 3 lines
A more informative commit message would be welcome, along with a reference to which Device Tree binding documentation you are referring to.
I've added a reference to the bindings doc in the commit msg for my next version.
Quoted 25 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)drivers/usb/host/Kconfig +0
-0drivers/usb/host/Kconfig config USB_EHCI_ATH79 · line 261
Quoted 6 lines
drivers/usb/host/Kconfig +3
-0@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)I do not think this is that simple nor correct for at least Microblaze and MIPS since they can run in either BE or LE mode, and those specific platforms should already do the proper select at the board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64, although I believe some specific PPC64 boards can run in little-endian mode like the P-series, SPARC might too.
Quoted 3 lines
It seems to me that you should not touch this and keep the existing selects in place, if it turns out that the selects are missing the error messages you added below are catching those misuses.
As discussed with Alan, I will drop these lines from my next version.
Quoted 34 lines
Quoted 34 lines
drivers/usb/host/Kconfig +0
-0default n ---help--- Adds an EHCI host driver for a generic platform device, whichdrivers/usb/host/ehci-platform.c +6
-2@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);
Quoted 15 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 193
Quoted 9 lines
drivers/usb/host/ehci-platform.c +8
-0if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ;Ok, so I am confused now, should you update pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly modify ehci->big_endian_{desc,mmio}, is not there any risk to undo what is done in
ehci_platform_reset(), or isehci_platform_reset()only called for non-DT cases?
Both the pdata checks in ehci_platform_reset() and the dt checks here only ever set these flags, neither code path clears them. And in the dt case pdata will be NULL and vice versa.
Quoted 10 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 198
Quoted 6 lines
drivers/usb/host/ehci-platform.c +5
-0+ +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO +if (ehci->big_endian_mmio) { +dev_err (&dev->dev, +"Error big-endian-regs not compiled in\n" );I do not think using the Device Tree property name would be very informative since this is supposed to guard against misconfigurations for both DT and non-DT enabled platforms
Nope this is in a dt only code path.
Quoted 4 lines
Quoted 4 lines
drivers/usb/host/ehci-platform.c +4
-0+ err = -EINVAL ; + goto err_put_hcd; + } +#endif
Quoted 14 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 208
Quoted 6 lines
drivers/usb/host/ehci-platform.c +6
-0+#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_DESC +if (ehci->big_endian_desc) { +dev_err (&dev->dev, +"Error big-endian-desc not compiled in\n" ); + err = -EINVAL ; + goto err_put_hcd;And here "support for big-endian descriptors not enabled".
Quoted 5 lines
drivers/usb/host/ehci-platform.c +2
-0+ } +#endif priv->phy =devm_phy_get (&dev->dev,"usb" );if (IS_ERR (priv->phy)) { err =PTR_ERR (priv->phy);
Regards,
Hans
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Hi,
Quoted 98 lines · On Wed, 22 Jan 2014 20:28:26 +0100 Hans de Goede <[email protected]> wrote:
Hi,
Quoted 5 lines · Florian Fainelli
2014/1/21 Hans de Goede <[email protected]>:
Quoted 1 line
This uses the already documented devicetree booleans for this.
(I would greatly appreciate if you could CC people who gave you feedback on this before)
Will do.
Quoted 3 lines
A more informative commit message would be welcome, along with a reference to which Device Tree binding documentation you are referring to.
I've added a reference to the bindings doc in the commit msg for my next version.
Quoted 25 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)drivers/usb/host/Kconfig +0
-0drivers/usb/host/Kconfig config USB_EHCI_ATH79 · line 261
Quoted 6 lines
drivers/usb/host/Kconfig +3
-0@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)I do not think this is that simple nor correct for at least Microblaze and MIPS since they can run in either BE or LE mode, and those specific platforms should already do the proper select at the board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64, although I believe some specific PPC64 boards can run in little-endian mode like the P-series, SPARC might too.
Quoted 3 lines
It seems to me that you should not touch this and keep the existing selects in place, if it turns out that the selects are missing the error messages you added below are catching those misuses.
As discussed with Alan, I will drop these lines from my next version.
Quoted 34 lines
Quoted 34 lines
drivers/usb/host/Kconfig +0
-0default n ---help--- Adds an EHCI host driver for a generic platform device, whichdrivers/usb/host/ehci-platform.c +6
-2@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);Quoted 15 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 193
Quoted 9 lines
drivers/usb/host/ehci-platform.c +8
-0if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ;Ok, so I am confused now, should you update pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly modify ehci->big_endian_{desc,mmio}, is not there any risk to undo what is done in
ehci_platform_reset(), or isehci_platform_reset()only called for non-DT cases?
Quoted 3 lines
Both the pdata checks in
ehci_platform_reset()and the dt checks here only ever set these flags, neither code path clears them. And in the dt case pdata will be NULL and vice versa.
If it's safe to set ehci->big_endian_{desc,mmio} from the _probe() routine, then maybe the pdata sets in _reset() should be moved into here instead of adding extra cludges/checks into _reset().
Quoted 12 lines
Quoted 11 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 198
Quoted 7 lines
drivers/usb/host/ehci-platform.c +5
-0+ +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO +if (ehci->big_endian_mmio) { +dev_err (&dev->dev, +"Error big-endian-regs not compiled in\n" );I do not think using the Device Tree property name would be very informative since this is supposed to guard against misconfigurations for both DT and non-DT enabled platforms
Quoted 1 line
Nope this is in a dt only code path.
Then these built-in checks could be done for both paths, as !DT also has the issue, just doesn't warn/error out with it.
I also agree with Florian that the error message could be better worded, or should be at least have a ":" after "Error". When seeing the error message the fist time, I would wonder what "error big-endian-regs" are, and why I would want to have them ;)
Finally, IS_ENABLED()* allows you to drop those ugly #ifndefs:
if (ehci->big_endian_mmio &&
!IS_ENABLED(CONFIG_USB_EHCI_BIG_ENDIAN_MMIO)) {
dev_err(&dev->dev,
"Error big-endian-regs not compiled in\n");looks much nicer IMHO ;)
Regards
Jonas
*Yes, I know IS_BUILTIN() would be more formal correct, but IS_ENABLED() reads IMHO nicer, and I doubt these config symbols will ever go tristate ;)
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Hi,
Quoted 102 lines · Jonas Gorski
Hi,
Quoted 98 lines · On Wed, 22 Jan 2014 20:28:26 +0100 Hans de Goede <[email protected]> wrote:
Hi,
Quoted 5 lines · Florian Fainelli
2014/1/21 Hans de Goede <[email protected]>:
Quoted 1 line
This uses the already documented devicetree booleans for this.
(I would greatly appreciate if you could CC people who gave you feedback on this before)
Will do.
Quoted 3 lines
A more informative commit message would be welcome, along with a reference to which Device Tree binding documentation you are referring to.
I've added a reference to the bindings doc in the commit msg for my next version.
Quoted 25 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)drivers/usb/host/Kconfig +0
-0drivers/usb/host/Kconfig config USB_EHCI_ATH79 · line 261
Quoted 6 lines
drivers/usb/host/Kconfig +3
-0@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)I do not think this is that simple nor correct for at least Microblaze and MIPS since they can run in either BE or LE mode, and those specific platforms should already do the proper select at the board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64, although I believe some specific PPC64 boards can run in little-endian mode like the P-series, SPARC might too.
Quoted 3 lines
It seems to me that you should not touch this and keep the existing selects in place, if it turns out that the selects are missing the error messages you added below are catching those misuses.
As discussed with Alan, I will drop these lines from my next version.
Quoted 49 lines
Quoted 33 lines
drivers/usb/host/Kconfig +0
-0default n ---help--- Adds an EHCI host driver for a generic platform device, whichdrivers/usb/host/ehci-platform.c +6
-2@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 193
Quoted 10 lines
drivers/usb/host/ehci-platform.c +8
-0if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ;Ok, so I am confused now, should you update pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly modify ehci->big_endian_{desc,mmio}, is not there any risk to undo what is done in
ehci_platform_reset(), or isehci_platform_reset()only called for non-DT cases?
Quoted 7 lines
Quoted 3 lines
Both the pdata checks in
ehci_platform_reset()and the dt checks here only ever set these flags, neither code path clears them. And in the dt case pdata will be NULL and vice versa.If it's safe to set ehci->big_endian_{desc,mmio} from the
_probe()routine, then maybe the pdata sets in_reset()should be moved into here instead of adding extra cludges/checks into_reset().
That seems like an entire separate patch / improvement on top of adding support for big-endian controllers to the dt code.
If people won't to improve on thus further and / or clean things up further I'm sure the usb-subsys maintainers would welcome patches.
<snip more suggestions best done in a separate patch written by someone else>
Regards,
Hans
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Hi,
Quoted 113 lines · Hans de Goede
Hi,
Quoted 102 lines · Jonas Gorski
Hi,
Quoted 98 lines · On Wed, 22 Jan 2014 20:28:26 +0100 Hans de Goede <[email protected]> wrote:
Hi,
Quoted 5 lines · Florian Fainelli
2014/1/21 Hans de Goede <[email protected]>:
Quoted 1 line
This uses the already documented devicetree booleans for this.
(I would greatly appreciate if you could CC people who gave you feedback on this before)
Will do.
Quoted 3 lines
A more informative commit message would be welcome, along with a reference to which Device Tree binding documentation you are referring to.
I've added a reference to the bindings doc in the commit msg for my next version.
Quoted 25 lines
Quoted 11 lines
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/Kconfig | 3 +++ drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)drivers/usb/host/Kconfig +0
-0drivers/usb/host/Kconfig config USB_EHCI_ATH79 · line 261
Quoted 6 lines
drivers/usb/host/Kconfig +3
-0@@ -256,6 +256,9 @@ config USB_EHCI_ATH79 config USB_EHCI_HCD_PLATFORM tristate "Generic EHCI driver for a platform device" depends on !PPC_OF + # Support BE on architectures which have readl_be + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)I do not think this is that simple nor correct for at least Microblaze and MIPS since they can run in either BE or LE mode, and those specific platforms should already do the proper select at the board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64, although I believe some specific PPC64 boards can run in little-endian mode like the P-series, SPARC might too.
Quoted 3 lines
It seems to me that you should not touch this and keep the existing selects in place, if it turns out that the selects are missing the error messages you added below are catching those misuses.
As discussed with Alan, I will drop these lines from my next version.
Quoted 49 lines
Quoted 33 lines
drivers/usb/host/Kconfig +0
-0default n ---help--- Adds an EHCI host driver for a generic platform device, whichdrivers/usb/host/ehci-platform.c +6
-2@@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd) hcd->has_tt = pdata->has_tt; ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug; - ehci->big_endian_desc = pdata->big_endian_desc; - ehci->big_endian_mmio = pdata->big_endian_mmio; +if (pdata->big_endian_desc) + ehci->big_endian_desc =1 ; +if (pdata->big_endian_mmio) + ehci->big_endian_mmio =1 ;if (pdata->pre_setup) { retval = pdata->pre_setup (hcd);@@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev);struct ehci_platform_priv *priv; +struct ehci_hcd *ehci;int err, irq, clk =0 ;if (usb_disabled ())@@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)platform_set_drvdata (dev, hcd); dev->dev.platform_data = pdata; priv =hcd_to_ehci_priv (hcd); + ehci =hcd_to_ehci (hcd);drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 193
Quoted 10 lines
drivers/usb/host/ehci-platform.c +8
-0if (pdata == &ehci_platform_defaults && dev->dev.of_node) { +if (of_property_read_bool (dev->dev.of_node,"big-endian-regs" )) + ehci->big_endian_mmio =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian-desc" )) + ehci->big_endian_desc =1 ; + +if (of_property_read_bool (dev->dev.of_node,"big-endian" )) + ehci->big_endian_mmio = ehci->big_endian_desc =1 ;Ok, so I am confused now, should you update pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly modify ehci->big_endian_{desc,mmio}, is not there any risk to undo what is done in
ehci_platform_reset(), or isehci_platform_reset()only called for non-DT cases?Quoted 7 lines
Quoted 3 lines
Both the pdata checks in
ehci_platform_reset()and the dt checks here only ever set these flags, neither code path clears them. And in the dt case pdata will be NULL and vice versa.If it's safe to set ehci->big_endian_{desc,mmio} from the
_probe()routine, then maybe the pdata sets in_reset()should be moved into here instead of adding extra cludges/checks into_reset().
Quoted 5 lines
That seems like an entire separate patch / improvement on top of adding support for big-endian controllers to the dt code.
If people won't to improve on thus further and / or clean things up further I'm sure the usb-subsys maintainers would welcome patches.
s/won't to improve on thus/want to improve this/
Quoted 6 lines
<snip more suggestions best done in a separate patch written by someone else>
Regards,
Hans
Regards,
Hans
Alan Stern
maintainerPATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
In reply to Jonas GorskiQuoted 3 lines · Jonas Gorski
If it's safe to set ehci->big_endian_{desc,mmio} from the
_probe()routine, then maybe the pdata sets in_reset()should be moved into here instead of adding extra cludges/checks into_reset().
Why? What difference would it make?
Alan Stern
PATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Quoted 7 lines · On Wed, 22 Jan 2014 16:17:42 -0500 (EST) Alan Stern…
Quoted 3 lines · Jonas Gorski
If it's safe to set ehci->big_endian_{desc,mmio} from the
_probe()routine, then maybe the pdata sets in_reset()should be moved into here instead of adding extra cludges/checks into_reset().Why? What difference would it make?
Effectivewise none, but to me it seems to be cleaner to set them once in probe() instead of everytime reset() is called.
I admit I don't know the code flow good enough if reset() is called more than once in the lifetime of a hcd device.
And as I said, it would allow doing the checks the patch adds for both DT and !DT, not just DT only.
Regards
Jonas
Alan Stern
maintainerPATCH 2/2 ehci-platform: Add support for controllers with big-endian regs / descriptors
Quoted 9 lines · Jonas Gorski
Quoted 6 lines · On Wed, 22 Jan 2014 16:17:42 -0500 (EST) Alan Stern…
Quoted 3 lines · Jonas Gorski
If it's safe to set ehci->big_endian_{desc,mmio} from the
_probe()routine, then maybe the pdata sets in_reset()should be moved into here instead of adding extra cludges/checks into_reset().
Quoted 7 lines
Quoted 1 line
Why? What difference would it make?
Effectivewise none, but to me it seems to be cleaner to set them once in
probe()instead of everytimereset()is called.I admit I don't know the code flow good enough if
reset()is called more than once in the lifetime of a hcd device.
Only once.
Quoted 2 lines
And as I said, it would allow doing the checks the patch adds for both DT and !DT, not just DT only.
That's true. Or the checks could be moved into the probe routine.
Alan Stern
Alan Stern
maintainerPATCH 1/2 ohci-platform: Add support for controllers with big-endian regs / descriptors
In reply to Hans de GoedeQuoted 8 lines · Hans de Goede
Note this commit uses the same devicetree booleans for this as the ones already existing in the usb-ehci bindings.
Signed-off-by Hans de Goede <[email protected]>
drivers/usb/host/Kconfig config USB_CNS3XXX_OHCI · line 519
Quoted 8 lines
drivers/usb/host/Kconfig +4
-0@@ -512,6 +512,10 @@ config USB_CNS3XXX_OHCI config USB_OHCI_HCD_PLATFORM tristate "Generic OHCI driver for a platform device" + # Always support LE, support BE on architectures which have readl_be + select USB_OHCI_LITTLE_ENDIAN + select USB_OHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_OHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default n
The comment line above is slightly misleading. USB_OHCI_LITTLE_ENDIAN doesn't exactly mean to include support for little-endian controllers; it means include mixed-endian support if either USB_OHCI_BIG_ENDIAN_DESC or USB_OHCI_BIG_ENDIAN_MMIO is set. That is, the driver determines at runtime whether a particular controller is big-endian or little-endian, rather than choosing to support one or the other at compile time.
In any case, the style we have adopted is that these select lines go in the arch-specific defconfig, not here. For example, check out the occurrences of EHCI in arch/mips/Kconfig. Also, I'm not sure how you came up with that list of architectures for the two selects; it's hard to say if they are right. For instance, why did you include AVR32?
The changes to the driver itself look fine.
Similar comments apply to the ehci-platform patch.
Alan Stern
PATCH 1/2 ohci-platform: Add support for controllers with big-endian regs / descriptors
Hi,
Quoted 11 lines · Alan Stern
Quoted 9 lines · Hans de Goede
Note this commit uses the same devicetree booleans for this as the ones already existing in the usb-ehci bindings.
Signed-off-by Hans de Goede <[email protected]>drivers/usb/host/Kconfig +0
-0@@ -512,6 +512,10 @@ config USB_CNS3XXX_OHCI
Quoted 15 lines
drivers/usb/host/Kconfig config USB_CNS3XXX_OHCI · line 519
Quoted 7 lines
drivers/usb/host/Kconfig +4
-0config USB_OHCI_HCD_PLATFORM tristate "Generic OHCI driver for a platform device" + # Always support LE, support BE on architectures which have readl_be + select USB_OHCI_LITTLE_ENDIAN + select USB_OHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_OHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default nThe comment line above is slightly misleading.
USB_OHCI_LITTLE_ENDIANdoesn't exactly mean to include support for little-endian controllers; it means include mixed-endian support if eitherUSB_OHCI_BIG_ENDIAN_DESCorUSB_OHCI_BIG_ENDIAN_MMIOis set. That is, the driver determines at runtime whether a particular controller is big-endian or little-endian, rather than choosing to support one or the other at compile time.
Right I already knew that, that is what the "Always" tried to summarize.
Quoted 2 lines
In any case, the style we have adopted is that these select lines go in the arch-specific defconfig, not here.
Ok, so I should drop the Kconfig parts of both patches ?
Quoted 4 lines
For example, check out the
occurrences of EHCI inarch/mips/Kconfig. Also, I'm not sure how you came up with that list of architectures for the two selects; it's hard to say if they are right. For instance, why did you include AVR32?
I included all archs which defines readl_be in asm/io.h
Regards,
Hans
Alan Stern
maintainerPATCH 1/2 ohci-platform: Add support for controllers with big-endian regs / descriptors
Quoted 25 lines · Hans de Goede
Hi,
Quoted 11 lines · Alan Stern
Quoted 9 lines · Hans de Goede
Note this commit uses the same devicetree booleans for this as the ones already existing in the usb-ehci bindings.
Signed-off-by Hans de Goede <[email protected]>drivers/usb/host/Kconfig +0
-0@@ -512,6 +512,10 @@ config USB_CNS3XXX_OHCIQuoted 10 lines
drivers/usb/host/Kconfig config USB_CNS3XXX_OHCI · line 519
Quoted 7 lines
drivers/usb/host/Kconfig +4
-0config USB_OHCI_HCD_PLATFORM tristate "Generic OHCI driver for a platform device" + # Always support LE, support BE on architectures which have readl_be + select USB_OHCI_LITTLE_ENDIAN + select USB_OHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_OHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default nIn any case, the style we have adopted is that these select lines go in the arch-specific defconfig, not here.
Quoted 1 line
Ok, so I should drop the
Kconfigparts of both patches ?
That's rigt. They are likely to cause trouble, and if the selects were needed then they should already be present somewhere else.
Alan Stern
PATCH 1/2 ohci-platform: Add support for controllers with big-endian regs / descriptors
2014/1/21 Alan Stern <[email protected]>:
Quoted 27 lines · Hans de Goede
Quoted 26 lines
Hi,
Quoted 21 lines · Alan Stern
Quoted 8 lines · Hans de Goede
Note this commit uses the same devicetree booleans for this as the ones already existing in the usb-ehci bindings.
Signed-off-by Hans de Goede <[email protected]>drivers/usb/host/Kconfig config USB_CNS3XXX_OHCI · line 519
Quoted 8 lines
drivers/usb/host/Kconfig +4
-0@@ -512,6 +512,10 @@ config USB_CNS3XXX_OHCI config USB_OHCI_HCD_PLATFORM tristate "Generic OHCI driver for a platform device" + # Always support LE, support BE on architectures which have readl_be + select USB_OHCI_LITTLE_ENDIAN + select USB_OHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) + select USB_OHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64) default nIn any case, the style we have adopted is that these select lines go in the arch-specific defconfig, not here.
Quoted 4 lines
Quoted 1 line
Ok, so I should drop the
Kconfigparts of both patches ?That's rigt. They are likely to cause trouble, and if the selects were needed then they should already be present somewhere else.
Absolutely, they will actually break platforms. Since you added some guards for when these properties are set, but proper support in the kernel is not enabled, this is already catching misuses, and as such is already an improvement.
Signature · 1 line
Florian