usb: chipidea: fix error handling and add dr_role property
Subsystems (4)
v2 cover usb: chipidea: fix error handling and add dr_role property
while hacking on the chipidea driver, I fixed the error path in the udc part of the chipidea driver. Further I added the possibility to limit the driver to host or device only mode. It's possible via platformdata and device tree. For dt I selected the dr_role property which is already used for other dual role usb drivers.
Changes since v1:
- rebase to Richard's Tree
https://github.com/riczhao/kernel-imx/commits/topics/usb-driver
- remove redundant wording from devicetree binding documentation
- move dtsi changes into seperate patch
- also change imx23.dtsi
- tested on compile time non OF tree platforms (amd64) of.h provides proper stubs.
regards, Marc
PATCH v2 1/4 usb: chipidea: pci: make platformdata static
--- drivers/usb/chipidea/ci13xxx_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/usb/chipidea/ci13xxx_pci.c +3 -3
@@ -23,17 +23,17 @@/****************************************************************************** * PCI block *****************************************************************************/ -struct ci13xxx_platform_data pci_platdata = { +static struct ci13xxx_platform_data pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =DEF_CAPOFFSET , }; -struct ci13xxx_platform_data langwell_pci_platdata = { +static struct ci13xxx_platform_data langwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , }; -struct ci13xxx_platform_data penwell_pci_platdata = { +static struct ci13xxx_platform_data penwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , .power_budget =200 ,
Signature · 1 line
1.7.10
PATCH v2 1/4 usb: chipidea: pci: make platformdata static
Quoted 10 lines · Marc Kleine-Budde
Signed-off-by Marc Kleine-Budde <[email protected]>--- drivers/usb/chipidea/ci13xxx_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/usb/chipidea/ci13xxx_pci.c +0
-0
drivers/usb/chipidea/ci13xxx_pci.c line 26
Quoted 5 lines
drivers/usb/chipidea/ci13xxx_pci.c +1
-1@@ -23,17 +23,17 @@/****************************************************************************** * PCI block *****************************************************************************/ -struct ci13xxx_platform_data pci_platdata = { +static struct ci13xxx_platform_data pci_platdata = {
what about adding __devinitdata?
Thanks
Richard
Quoted 22 lines
drivers/usb/chipidea/ci13xxx_pci.c +2
-2.name = UDC_DRIVER_NAME, .capoffset = DEF_CAPOFFSET, }; -struct ci13xxx_platform_data langwell_pci_platdata = { +static struct ci13xxx_platform_data langwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , }; -struct ci13xxx_platform_data penwell_pci_platdata = { +static struct ci13xxx_platform_data penwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , .power_budget =200 ,Signature · 6 lines
1.7.10 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/4 usb: chipidea: pci: make platformdata static
Quoted 6 lines · Richard Zhao
Quoted 5 lines · Marc Kleine-Budde
Signed-off-by Marc Kleine-Budde <[email protected]>--- drivers/usb/chipidea/ci13xxx_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 11 lines
drivers/usb/chipidea/ci13xxx_pci.c line 26
Quoted 10 lines
drivers/usb/chipidea/ci13xxx_pci.c +1
-1@@ -23,17 +23,17 @@/****************************************************************************** * PCI block *****************************************************************************/ -struct ci13xxx_platform_data pci_platdata = { +static struct ci13xxx_platform_data pci_platdata = {what about adding
__devinitdata?
Marc, what do you think?
Thanks
Richard
Quoted 25 lines
Thanks
RichardQuoted 22 lines
drivers/usb/chipidea/ci13xxx_pci.c +2
-3.name = UDC_DRIVER_NAME, .capoffset = DEF_CAPOFFSET, }; -struct ci13xxx_platform_data langwell_pci_platdata = { +static struct ci13xxx_platform_data langwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , }; -struct ci13xxx_platform_data penwell_pci_platdata = { +static struct ci13xxx_platform_data penwell_pci_platdata = { .name =UDC_DRIVER_NAME , .capoffset =0 , .power_budget =200 , --1.7.10
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/4 usb: chipidea: pci: make platformdata static
Quoted 18 lines · Richard Zhao
Quoted 6 lines · Richard Zhao
Quoted 5 lines · Marc Kleine-Budde
Signed-off-by Marc Kleine-Budde <[email protected]>--- drivers/usb/chipidea/ci13xxx_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)Quoted 11 lines
drivers/usb/chipidea/ci13xxx_pci.c line 26
Quoted 10 lines
drivers/usb/chipidea/ci13xxx_pci.c +1
-1@@ -23,17 +23,17 @@/****************************************************************************** * PCI block *****************************************************************************/ -struct ci13xxx_platform_data pci_platdata = { +static struct ci13xxx_platform_data pci_platdata = {what about adding
__devinitdata?
Quoted 1 line
Marc, what do you think?
Not on statically declared platform data. That's always a mistake.
PATCH v2 1/4 usb: chipidea: pci: make platformdata static
Quoted 19 lines · Russell King - ARM Linux
Quoted 18 lines · Richard Zhao
Quoted 17 lines · Richard Zhao
Quoted 10 lines · Marc Kleine-Budde
Signed-off-by Marc Kleine-Budde <[email protected]>--- drivers/usb/chipidea/ci13xxx_pci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/usb/chipidea/ci13xxx_pci.c +0
-0drivers/usb/chipidea/ci13xxx_pci.c line 26
Quoted 5 lines
drivers/usb/chipidea/ci13xxx_pci.c +1
-1@@ -23,17 +23,17 @@/****************************************************************************** * PCI block *****************************************************************************/ -struct ci13xxx_platform_data pci_platdata = { +static struct ci13xxx_platform_data pci_platdata = {what about adding
__devinitdata?
Quoted 4 lines
Quoted 1 line
Marc, what do you think?
Not on statically declared platform data. That's always a mistake.
Could you please explain more?
platdata in this driver is duplicated when call ci13xxx_add_device in probe.
Thanks
Richard
PATCH v2 2/4 usb: chipidea: udc: fix error path in udc_start()
In reply to Marc Kleine-BuddeThis patch fixes the error path of udc_start(). Now NULL is used to unset the peripheral with otg_set_peripheral().
--- drivers/usb/chipidea/udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/chipidea/udc.c +1 -1
@@ -1739,7 +1739,7 @@ static int udc_start(struct ci13xxx *ci) remove_trans:if (ci->transceiver) { -otg_set_peripheral (ci->transceiver->otg, &ci->gadget); +otg_set_peripheral (ci->transceiver->otg,NULL );if (ci->global_phy)usb_put_transceiver (ci->transceiver); }
Signature · 1 line
1.7.10
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
In reply to Marc Kleine-BuddeThis patch allows the device tree to limit the chipidea to host or peripheral mode only.
--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +3 -0
@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined. Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port
@@ -14,4 +16,5 @@ usb at 02184000 { /* USB OTG */ reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; + dr_mode= "otg"; };
drivers/usb/chipidea/ci13xxx_imx.c +3 -0
@@ -120,6 +120,9 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) *pdev->dev.dma_mask =DMA_BIT_MASK (32 );dma_set_coherent_mask (&pdev->dev, *pdev->dev.dma_mask); } + +ci13xxx_get_dr_mode (pdev->dev.of_node, &ci13xxx_imx_platdata); + plat_ci =ci13xxx_add_device (&pdev->dev, pdev->resource, pdev->num_resources, &ci13xxx_imx_platdata);
drivers/usb/chipidea/core.c +35 -6
60 lines not on this page · raw message
include/linux/usb/chipidea.h +9 -0
@@ -19,6 +19,12 @@ struct ci13xxx_platform_data {#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)#define CI13XXX_PULLUP_ON_VBUS BIT(2)#define CI13XXX_DISABLE_STREAMING BIT(3) +#define CI13XXX_DR_MODE_HOST BIT(4) +#define CI13XXX_DR_MODE_PERIPHERAL BIT(5) +#define CI13XXX_DR_MODE_OTG BIT(6) +#define CI13XXX_DR_MODE_MASK \ + (CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL | \ + CI13XXX_DR_MODE_OTG)#define CI13XXX_CONTROLLER_RESET_EVENT 0#define CI13XXX_CONTROLLER_STOPPED_EVENT 1
@@ -35,4 +41,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,/* Remove ci13xxx device */ void ci13xxx_remove_device (struct platform_device *pdev); +/* Parse of-tree "dr_mode" property */ +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata); +#endif
Signature · 1 line
1.7.10
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 16 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +0
-0
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 5 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.
By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 46 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port@@ -14,4 +16,5 @@ usb at 02184000 { /* USB OTG */ reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; + dr_mode= "otg"; };drivers/usb/chipidea/ci13xxx_imx.c +3
-0@@ -120,6 +120,9 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) *pdev->dev.dma_mask =DMA_BIT_MASK (32 );dma_set_coherent_mask (&pdev->dev, *pdev->dev.dma_mask); } + +ci13xxx_get_dr_mode (pdev->dev.of_node, &ci13xxx_imx_platdata); + plat_ci =ci13xxx_add_device (&pdev->dev, pdev->resource, pdev->num_resources, &ci13xxx_imx_platdata);drivers/usb/chipidea/core.c +8
-0@@ -63,6 +63,8 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> @@ -386,6 +388,23 @@ void ci13xxx_remove_device(struct platform_device *pdev) }EXPORT_SYMBOL_GPL (ci13xxx_remove_device); +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata) +{ +const unsigned char *dr_mode; + + dr_mode =of_get_property (of_node,"dr_mode" ,NULL ); +if (!dr_mode)
drivers/usb/chipidea/core.c void ci13xxx_remove_device(struct platform_device *pdev) · line 406
Quoted 10 lines
drivers/usb/chipidea/core.c +10
-0+return ; + +if (!strcmp (dr_mode,"host" )) + pdata->flags |=CI13XXX_DR_MODE_HOST ; +else if (!strcmp (dr_mode,"peripheral" )) + pdata->flags |=CI13XXX_DR_MODE_PERIPHERAL ; +else if (!strcmp (dr_mode,"otg" )) + pdata->flags |=CI13XXX_DR_MODE_OTG ; +} +EXPORT_SYMBOL_GPL (ci13xxx_get_dr_mode);
If you make the property name generic, this function is not specific to chipidea. IMHO, the function is simple, if we do it in individual drivers, it's ok too.
Quoted 39 lines
drivers/usb/chipidea/core.c +17
-6+static int __devinit ci_hdrc_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;@@ -446,13 +465,23 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev) }/* initialize role(s) before the interrupt is requested */ - ret =ci_hdrc_host_init (ci); -if (ret) -dev_info (dev,"doesn't support host\n" ); +/* default to otg */ +if (!(ci->platdata->flags &CI13XXX_DR_MODE_MASK )) + ci->platdata->flags |=CI13XXX_DR_MODE_OTG ; + +if (ci->platdata->flags & + (CI13XXX_DR_MODE_HOST |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_host_init (ci); +if (ret) +dev_info (dev,"doesn't support host\n" ); + } - ret =ci_hdrc_gadget_init (ci); -if (ret) -dev_info (dev,"doesn't support gadget\n" ); +if (ci->platdata->flags & + (CI13XXX_DR_MODE_PERIPHERAL |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_gadget_init (ci); +if (ret) +dev_info (dev,"doesn't support gadget\n" ); + }if (!ci->roles[CI_ROLE_HOST ] && !ci->roles[CI_ROLE_GADGET ]) {dev_err (dev,"no supported roles\n" );include/linux/usb/chipidea.h +0
-0
include/linux/usb/chipidea.h struct ci13xxx_platform_data { · line 24
Quoted 6 lines
include/linux/usb/chipidea.h +3
-0@@ -19,6 +19,12 @@ struct ci13xxx_platform_data {#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)#define CI13XXX_PULLUP_ON_VBUS BIT(2)#define CI13XXX_DISABLE_STREAMING BIT(3) +#define CI13XXX_DR_MODE_HOST BIT(4) +#define CI13XXX_DR_MODE_PERIPHERAL BIT(5) +#define CI13XXX_DR_MODE_OTG BIT(6)
All we need is CI13XXX_DR_FORCE_HOST and CI13XXX_DR_FORCE_DEVICE.
Thanks
Richard
Quoted 18 lines
include/linux/usb/chipidea.h +6
-0+#define CI13XXX_DR_MODE_MASK \ + (CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL | \ + CI13XXX_DR_MODE_OTG)#define CI13XXX_CONTROLLER_RESET_EVENT 0#define CI13XXX_CONTROLLER_STOPPED_EVENT 1@@ -35,4 +41,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,/* Remove ci13xxx device */ void ci13xxx_remove_device (struct platform_device *pdev); +/* Parse of-tree "dr_mode" property */ +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata); +#endif Signature · 1 line
1.7.10
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)
Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
The property isn't prefixed with "fsl,", it's just "dr_mode".
Why not "force-xxx"? I had a look at Documentation/devicetree/bindings/usb:
tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")
fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.
So why invent something new, if there seems to be a pattern?
Quoted 52 lines
Quoted 52 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port@@ -14,4 +16,5 @@ usb at 02184000 { /* USB OTG */ reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; + dr_mode= "otg"; };drivers/usb/chipidea/ci13xxx_imx.c +3
-0@@ -120,6 +120,9 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) *pdev->dev.dma_mask =DMA_BIT_MASK (32 );dma_set_coherent_mask (&pdev->dev, *pdev->dev.dma_mask); } + +ci13xxx_get_dr_mode (pdev->dev.of_node, &ci13xxx_imx_platdata); + plat_ci =ci13xxx_add_device (&pdev->dev, pdev->resource, pdev->num_resources, &ci13xxx_imx_platdata);drivers/usb/chipidea/core.c +14
-0@@ -63,6 +63,8 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> @@ -386,6 +388,23 @@ void ci13xxx_remove_device(struct platform_device *pdev) }EXPORT_SYMBOL_GPL (ci13xxx_remove_device); +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata) +{ +const unsigned char *dr_mode; + + dr_mode =of_get_property (of_node,"dr_mode" ,NULL ); +if (!dr_mode) +return ; + +if (!strcmp (dr_mode,"host" )) + pdata->flags |=CI13XXX_DR_MODE_HOST ; +else if (!strcmp (dr_mode,"peripheral" )) + pdata->flags |=CI13XXX_DR_MODE_PERIPHERAL ;
Quoted 8 lines
drivers/usb/chipidea/core.c void ci13xxx_remove_device(struct platform_device *pdev) · line 406
Quoted 4 lines
drivers/usb/chipidea/core.c +4
-0+else if (!strcmp (dr_mode,"otg" )) + pdata->flags |=CI13XXX_DR_MODE_OTG ; +} +EXPORT_SYMBOL_GPL (ci13xxx_get_dr_mode);If you make the property name generic, this function is not specific to chipidea. IMHO, the function is simple, if we do it in individual drivers, it's ok too.
The property name is generic, it's just "dr_mode", but the function is chipidea specific, because it's working on the ci13xxx_platform_data.
Quoted 32 lines
Quoted 32 lines
drivers/usb/chipidea/core.c +17
-6+static int __devinit ci_hdrc_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;@@ -446,13 +465,23 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev) }/* initialize role(s) before the interrupt is requested */ - ret =ci_hdrc_host_init (ci); -if (ret) -dev_info (dev,"doesn't support host\n" ); +/* default to otg */ +if (!(ci->platdata->flags &CI13XXX_DR_MODE_MASK )) + ci->platdata->flags |=CI13XXX_DR_MODE_OTG ; + +if (ci->platdata->flags & + (CI13XXX_DR_MODE_HOST |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_host_init (ci); +if (ret) +dev_info (dev,"doesn't support host\n" ); + } - ret =ci_hdrc_gadget_init (ci); -if (ret) -dev_info (dev,"doesn't support gadget\n" ); +if (ci->platdata->flags & + (CI13XXX_DR_MODE_PERIPHERAL |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_gadget_init (ci); +if (ret) +dev_info (dev,"doesn't support gadget\n" ); + }
Quoted 14 lines
include/linux/usb/chipidea.h struct ci13xxx_platform_data { · line 24
Quoted 13 lines
drivers/usb/chipidea/core.c +0
-0if (!ci->roles[CI_ROLE_HOST ] && !ci->roles[CI_ROLE_GADGET ]) {dev_err (dev,"no supported roles\n" );include/linux/usb/chipidea.h +3
-0@@ -19,6 +19,12 @@ struct ci13xxx_platform_data {#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)#define CI13XXX_PULLUP_ON_VBUS BIT(2)#define CI13XXX_DISABLE_STREAMING BIT(3) +#define CI13XXX_DR_MODE_HOST BIT(4) +#define CI13XXX_DR_MODE_PERIPHERAL BIT(5) +#define CI13XXX_DR_MODE_OTG BIT(6)All we need is
CI13XXX_DR_FORCE_HOSTandCI13XXX_DR_FORCE_DEVICE.
Okay.
Quoted 13 lines
Thanks
RichardQuoted 10 lines
include/linux/usb/chipidea.h +3
-0+#define CI13XXX_DR_MODE_MASK \ + (CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL | \ + CI13XXX_DR_MODE_OTG)#define CI13XXX_CONTROLLER_RESET_EVENT 0#define CI13XXX_CONTROLLER_STOPPED_EVENT 1@@ -35,4 +41,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,/* Remove ci13xxx device */ void ci13xxx_remove_device (struct platform_device *pdev);
Quoted 9 lines
Quoted 8 lines
include/linux/usb/chipidea.h +3
-0+/* Parse of-tree "dr_mode" property */ +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata); +#endif Signature · 1 line
1.7.10
Marc
Signature · 12 lines
Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120629/8b9e5f5c/attachment.sig>
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
Quoted 13 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")
Quoted 7 lines
fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Quoted 62 lines
Quoted 53 lines
Quoted 53 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port@@ -14,4 +16,5 @@ usb at 02184000 { /* USB OTG */ reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; + dr_mode= "otg"; };drivers/usb/chipidea/ci13xxx_imx.c +3
-0@@ -120,6 +120,9 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) *pdev->dev.dma_mask =DMA_BIT_MASK (32 );dma_set_coherent_mask (&pdev->dev, *pdev->dev.dma_mask); } + +ci13xxx_get_dr_mode (pdev->dev.of_node, &ci13xxx_imx_platdata); + plat_ci =ci13xxx_add_device (&pdev->dev, pdev->resource, pdev->num_resources, &ci13xxx_imx_platdata);drivers/usb/chipidea/core.c +14
-0@@ -63,6 +63,8 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> @@ -386,6 +388,23 @@ void ci13xxx_remove_device(struct platform_device *pdev) }EXPORT_SYMBOL_GPL (ci13xxx_remove_device); +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata) +{ +const unsigned char *dr_mode; + + dr_mode =of_get_property (of_node,"dr_mode" ,NULL ); +if (!dr_mode) +return ; + +if (!strcmp (dr_mode,"host" )) + pdata->flags |=CI13XXX_DR_MODE_HOST ; +else if (!strcmp (dr_mode,"peripheral" )) + pdata->flags |=CI13XXX_DR_MODE_PERIPHERAL ;Quoted 8 lines
drivers/usb/chipidea/core.c void ci13xxx_remove_device(struct platform_device *pdev) · line 406
Quoted 4 lines
drivers/usb/chipidea/core.c +4
-0+else if (!strcmp (dr_mode,"otg" )) + pdata->flags |=CI13XXX_DR_MODE_OTG ; +} +EXPORT_SYMBOL_GPL (ci13xxx_get_dr_mode);If you make the property name generic, this function is not specific to chipidea. IMHO, the function is simple, if we do it in individual drivers, it's ok too.
Quoted 2 lines
The property name is generic, it's just "
dr_mode", but the function is chipidea specific, because it's working on theci13xxx_platform_data.
You may return some common flag.
Quoted 80 lines
Quoted 33 lines
Quoted 33 lines
drivers/usb/chipidea/core.c +17
-6+static int __devinit ci_hdrc_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;@@ -446,13 +465,23 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev) }/* initialize role(s) before the interrupt is requested */ - ret =ci_hdrc_host_init (ci); -if (ret) -dev_info (dev,"doesn't support host\n" ); +/* default to otg */ +if (!(ci->platdata->flags &CI13XXX_DR_MODE_MASK )) + ci->platdata->flags |=CI13XXX_DR_MODE_OTG ; + +if (ci->platdata->flags & + (CI13XXX_DR_MODE_HOST |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_host_init (ci); +if (ret) +dev_info (dev,"doesn't support host\n" ); + } - ret =ci_hdrc_gadget_init (ci); -if (ret) -dev_info (dev,"doesn't support gadget\n" ); +if (ci->platdata->flags & + (CI13XXX_DR_MODE_PERIPHERAL |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_gadget_init (ci); +if (ret) +dev_info (dev,"doesn't support gadget\n" ); + }Quoted 14 lines
include/linux/usb/chipidea.h struct ci13xxx_platform_data { · line 24
Quoted 13 lines
drivers/usb/chipidea/core.c +0
-0if (!ci->roles[CI_ROLE_HOST ] && !ci->roles[CI_ROLE_GADGET ]) {dev_err (dev,"no supported roles\n" );include/linux/usb/chipidea.h +3
-0@@ -19,6 +19,12 @@ struct ci13xxx_platform_data {#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)#define CI13XXX_PULLUP_ON_VBUS BIT(2)#define CI13XXX_DISABLE_STREAMING BIT(3) +#define CI13XXX_DR_MODE_HOST BIT(4) +#define CI13XXX_DR_MODE_PERIPHERAL BIT(5) +#define CI13XXX_DR_MODE_OTG BIT(6)All we need is
CI13XXX_DR_FORCE_HOSTandCI13XXX_DR_FORCE_DEVICE.Okay.
Quoted 13 lines
Thanks
RichardQuoted 10 lines
include/linux/usb/chipidea.h +3
-0+#define CI13XXX_DR_MODE_MASK \ + (CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL | \ + CI13XXX_DR_MODE_OTG)#define CI13XXX_CONTROLLER_RESET_EVENT 0#define CI13XXX_CONTROLLER_STOPPED_EVENT 1@@ -35,4 +41,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,/* Remove ci13xxx device */ void ci13xxx_remove_device (struct platform_device *pdev);Quoted 9 lines
Quoted 8 lines
include/linux/usb/chipidea.h +3
-0+/* Parse of-tree "dr_mode" property */ +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata); +#endif Signature · 1 line
1.7.10Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 32 lines · Richard Zhao
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 26 lines · Richard Zhao
Quoted 16 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +0
-0Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 5 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 6 lines
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
The device tree is used to describe the hardware. How would you describe a system, which has just a USB device connector? And the hardware guys used the not needed id-pin for a LED?
Quoted 18 lines
Quoted 18 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.
Quoted 4 lines
Quoted 1 line
So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Sensible name sounds good. Devicetree Discussions, we need an official name for the property and its values :)
Marc
Signature · 12 lines
Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 262 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120629/eeee92d3/attachment.sig>
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 35 lines · Marc Kleine-Budde
Quoted 34 lines · Richard Zhao
Quoted 33 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
Quoted 8 lines
Quoted 4 lines
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
The device tree is used to describe the hardware. How would you describe a system, which has just a USB device connector? And the hardware guys used the not needed id-pin for a LED?
I see your case. Ideally, hw should pull-up or pull-down id pin. We may not take it as a common case. As I mentioned in another mail, we may interpret property in imx driver.
Thanks
Richard
Quoted 34 lines
Quoted 19 lines
Quoted 19 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.Quoted 4 lines
Quoted 1 line
So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Sensible name sounds good. Devicetree Discussions, we need an official name for the property and its values :)
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
In reply to Richard ZhaoQuoted 58 lines · Richard Zhao
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
Quoted 19 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.
Quoted 5 lines
Quoted 1 line
So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
I'm not quite sure what the question is I'm being asked here.
I certainly think that new bindings should follow existing precedent where possible for representing the same data. dr_mode is that precedent for a USB host's operating mode. Tegra chose to use that because of precedent in fsl-usb.txt IIRC.
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 65 lines · Stephen Warren
Quoted 58 lines · Richard Zhao
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
Quoted 19 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.Quoted 5 lines
Quoted 1 line
So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Quoted 6 lines
I'm not quite sure what the question is I'm being asked here.
I certainly think that new bindings should follow existing precedent where possible for representing the same data.
dr_modeis that precedent for a USB host's operating mode. Tegra chose to use that because of precedent in fsl-usb.txt IIRC.
For imx, in most cases, we don't use dr_mode. The role is decided by CAP_DCCPARAMS and ID pin. For tegra and fsl-usb, it looks like the role is totally decided by dt property. So I suggested use force-xxx to let everyone know it does not follow the default action. I don't quite insist on the naming, because dr_mode is similar (not same).
Thanks
Richard
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 73 lines · Richard Zhao
Quoted 65 lines · Stephen Warren
Quoted 58 lines · Richard Zhao
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
Quoted 19 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.Quoted 5 lines
Quoted 1 line
So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Quoted 6 lines
I'm not quite sure what the question is I'm being asked here.
I certainly think that new bindings should follow existing precedent where possible for representing the same data.
dr_modeis that precedent for a USB host's operating mode. Tegra chose to use that because of precedent in fsl-usb.txt IIRC.
Quoted 5 lines
For imx, in most cases, we don't use
dr_mode. The role is decided byCAP_DCCPARAMSand ID pin. For tegra and fsl-usb, it looks like the role is totally decided by dt property. So I suggested use force-xxx to let everyone know it does not follow the default action. I don't quite insist on the naming, becausedr_modeis similar (not same).
Hmm. I think it'd be reasonable to use dr_mode like the other bindings, and have the default case be decided by the ID pin when dr_mode isn't specified. Having different defaults for different bindings seems pretty reasonable to me.
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 4 lines
Hmm. I think it'd be reasonable to use
dr_modelike the other bindings, and have the default case be decided by the ID pin whendr_modeisn't specified. Having different defaults for different bindings seems pretty reasonable to me.
As far as I know, there is no USB Spec says the ID value should be used at device or host mode. At USB device and host driver, there should be NO ID related things, that is to say device or host driver should not care ID is grounded or floated, as ID may be grounded at device mode, and high at host mode for different board design. ID interrupt should only be enabled when dr_mode = otg;
Board design and customized usage (user may want to use only device mode for otg capable port) makes what USB role will be used
at driver, so dr_mode at dts (or pdata->dr_mode at non-dt solution) can make the decision for driver how USB role will be used.
Marc's patch is almost OK, but better use below logic at core.c:
If there is no dr_mode, it is better to indicate an error message and quit probe.
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 11 lines · Peter Chen
Quoted 4 lines
Hmm. I think it'd be reasonable to use
dr_modelike the other bindings, and have the default case be decided by the ID pin whendr_modeisn't specified. Having different defaults for different bindings seems pretty reasonable to me.As far as I know, there is no USB Spec says the ID value should be used at device or host mode. At USB device and host driver, there should be NO ID related things, that is to say device or host driver should not care ID is grounded or floated, as ID may be grounded at device mode, and high at host mode for different board design. ID interrupt should only be enabled when
dr_mode= otg;
I don't quite understand your point. All the discussions are based on otg controllers.
Quoted 5 lines
Board design and customized usage (user may want to use only device mode for otg capable port) makes what USB role will be used
at driver, sodr_modeat dts (orpdata->dr_modeat non-dt solution) can make the decision for driver how USB role will be used.
That's the case we need the property. But for fine otg hw design, we don't need the property, right?
And to decide whether it's otg controller or not, the driver read the capability registers.
Thanks
Richard
Quoted 10 lines
Marc's patch is almost OK, but better use below logic at
core.c:
If there is nodr_mode, it is better to indicate an error message and quit probe.Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Hi,
Quoted 19 lines · Richard Zhao
Quoted 11 lines · Peter Chen
Quoted 4 lines
Hmm. I think it'd be reasonable to use
dr_modelike the other bindings, and have the default case be decided by the ID pin whendr_modeisn't specified. Having different defaults for different bindings seems pretty reasonable to me.As far as I know, there is no USB Spec says the ID value should be used at device or host mode. At USB device and host driver, there should be NO ID related things, that is to say device or host driver should not care ID is grounded or floated, as ID may be grounded at device mode, and high at host mode for different board design. ID interrupt should only be enabled when
dr_mode= otg;I don't quite understand your point. All the discussions are based on otg controllers.
Quoted 5 lines
Board design and customized usage (user may want to use only device mode for otg capable port) makes what USB role will be used
at driver, sodr_modeat dts (orpdata->dr_modeat non-dt solution) can make the decision for driver how USB role will be used.
Quoted 6 lines
That's the case we need the property. But for fine otg hw design, we don't need the property, right?
And to decide whether it's otg controller or not, the driver read the capability registers.
A user may wish to restrict even a 'fine otg hw design' to be used in pure host or device mode. That's where the dr_mode property jumps in.
Lothar Wa?mann
Signature · 9 lines
___________________________________________________________ Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Gesch?ftsf?hrer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | info at karo-electronics.de ___________________________________________________________
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Quoted 22 lines · Lothar Wa?mann
Hi,
Quoted 19 lines · Richard Zhao
Quoted 11 lines · Peter Chen
Quoted 4 lines
Hmm. I think it'd be reasonable to use
dr_modelike the other bindings, and have the default case be decided by the ID pin whendr_modeisn't specified. Having different defaults for different bindings seems pretty reasonable to me.As far as I know, there is no USB Spec says the ID value should be used at device or host mode. At USB device and host driver, there should be NO ID related things, that is to say device or host driver should not care ID is grounded or floated, as ID may be grounded at device mode, and high at host mode for different board design. ID interrupt should only be enabled when
dr_mode= otg;I don't quite understand your point. All the discussions are based on otg controllers.
Quoted 5 lines
Board design and customized usage (user may want to use only device mode for otg capable port) makes what USB role will be used
at driver, sodr_modeat dts (orpdata->dr_modeat non-dt solution) can make the decision for driver how USB role will be used.
Quoted 8 lines
Quoted 6 lines
That's the case we need the property. But for fine otg hw design, we don't need the property, right?
And to decide whether it's otg controller or not, the driver read the capability registers.
A user may wish to restrict even a 'fine otg hw design' to be used in pure host or device mode. That's where the
dr_modeproperty jumps in.
Right, it _force_ the controller to a certain mode.
Thanks
Richard
Quoted 14 lines
Lothar Wa?mann
Signature · 9 lines
___________________________________________________________ Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Gesch?ftsf?hrer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | info at karo-electronics.de ___________________________________________________________
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
In reply to Richard ZhaoQuoted 119 lines · Richard Zhao
Quoted 31 lines · Marc Kleine-Budde
Cc'ed Devicetree Discussions
Quoted 12 lines · Richard Zhao
Quoted 11 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde <[email protected]>--- .../devicetree/bindings/usb/ci13xxx-imx.txt | 3 ++ drivers/usb/chipidea/ci13xxx_imx.c | 3 ++ drivers/usb/chipidea/core.c | 41 +++++++++++++++++--- include/linux/usb/chipidea.h | 9 +++++ 4 files changed, 50 insertions(+), 6 deletions(-)Quoted 14 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 8
Quoted 10 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +2
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can + be "host", "peripheral", or "otg". Defaults to "otg" if not defined.By default, it should be decided by capability registers. Only bad hw design needs such settings. So, why not name it as force-xxx? If it's not specific to imx, it doesn't needs to has prefix "fsl,".
Quoted 2 lines
It's not a bad hardware design if you don't route or enable all ports a soc offers. In modern socs you cannot enable all ports anyway.
I'm not sure about your case, but generally, it's not about ports. It's about ID pin. If ID pin is not connect correctly, we may need to force it to host or device working mode. The 'force" here means it won't follow the capability registers and ID pin.
Quoted 13 lines
The property isn't prefixed with "fsl,", it's just "
dr_mode".Why not "force-xxx"? I had a look at
Documentation/devicetree/bindings/usb:tegra-usb.txt:
Quoted 6 lines
-
dr_mode: dual role mode. Indicates the working mode for nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility. host means this is a host controller
peripheral means it is device controller
otg means it can operate as either ("on the go")Quoted 7 lines
fsl-usb.txt:
Quoted 4 lines
-
dr_mode: indicates the working mode for "fsl-usb2-dr" compatible controllers. Can be "host", "peripheral", or "otg". Default to "host" if not defined for backward compatibility.So why invent something new, if there seems to be a pattern?
I'm not sure they mean the same things, because the default value is different. Event if they're same, why not make them all with sensible name?
Quoted 58 lines
Quoted 57 lines
Quoted 57 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port@@ -14,4 +16,5 @@ usb at 02184000 { /* USB OTG */ reg = <0x02184000 0x200>; interrupts = <0 43 0x04>; fsl,usbphy = <&usbphy1>; + dr_mode= "otg"; };drivers/usb/chipidea/ci13xxx_imx.c +3
-0@@ -120,6 +120,9 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) *pdev->dev.dma_mask =DMA_BIT_MASK (32 );dma_set_coherent_mask (&pdev->dev, *pdev->dev.dma_mask); } + +ci13xxx_get_dr_mode (pdev->dev.of_node, &ci13xxx_imx_platdata); + plat_ci =ci13xxx_add_device (&pdev->dev, pdev->resource, pdev->num_resources, &ci13xxx_imx_platdata);drivers/usb/chipidea/core.c +18
-0@@ -63,6 +63,8 @@#include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> @@ -386,6 +388,23 @@ void ci13xxx_remove_device(struct platform_device *pdev) }EXPORT_SYMBOL_GPL (ci13xxx_remove_device); +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata) +{ +const unsigned char *dr_mode; + + dr_mode =of_get_property (of_node,"dr_mode" ,NULL ); +if (!dr_mode) +return ; + +if (!strcmp (dr_mode,"host" )) + pdata->flags |=CI13XXX_DR_MODE_HOST ; +else if (!strcmp (dr_mode,"peripheral" )) + pdata->flags |=CI13XXX_DR_MODE_PERIPHERAL ; +else if (!strcmp (dr_mode,"otg" )) + pdata->flags |=CI13XXX_DR_MODE_OTG ; +} +EXPORT_SYMBOL_GPL (ci13xxx_get_dr_mode);
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
If you make the property name generic, this function is not specific to chipidea. IMHO, the function is simple, if we do it in individual drivers, it's ok too.
The property name is generic, it's just "
dr_mode", but the function is chipidea specific, because it's working on theci13xxx_platform_data.You may return some common flag.
hmm.. in my opinion, it's better put it in ci13xxx_imx.c, if we don't have of_chipidea.c . Reason:
- it may not so widely used.
- it cause binary increase for non-dt platform.
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
drivers/usb/chipidea/core.c +11
-3+static int __devinit ci_hdrc_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;@@ -446,13 +465,23 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev) }/* initialize role(s) before the interrupt is requested */ - ret =ci_hdrc_host_init (ci); -if (ret) -dev_info (dev,"doesn't support host\n" ); +/* default to otg */ +if (!(ci->platdata->flags &CI13XXX_DR_MODE_MASK )) + ci->platdata->flags |=CI13XXX_DR_MODE_OTG ; + +if (ci->platdata->flags & + (CI13XXX_DR_MODE_HOST |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_host_init (ci); +if (ret) +dev_info (dev,"doesn't support host\n" ); + }
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/usb/chipidea/core.c static int __devinit ci_hdrc_probe(struct platform_device *pdev) · line 484
Quoted 9 lines
drivers/usb/chipidea/core.c +6
-3- ret =ci_hdrc_gadget_init (ci); -if (ret) -dev_info (dev,"doesn't support gadget\n" ); +if (ci->platdata->flags & + (CI13XXX_DR_MODE_PERIPHERAL |CI13XXX_DR_MODE_OTG )) { + ret =ci_hdrc_gadget_init (ci); +if (ret) +dev_info (dev,"doesn't support gadget\n" ); + }
I was just thinking, why don't you just modify function ci_otg_role? if (flag & CI13XXX_DR_FORCE_HOST)
return host-role;
else if (flag & CI13XXX_DR_FORCE_GADGET)
return gadget-role.
Thanks
Richard
Quoted 55 lines
Quoted 41 lines
Quoted 15 lines
Quoted 8 lines
drivers/usb/chipidea/core.c +0
-0if (!ci->roles[CI_ROLE_HOST ] && !ci->roles[CI_ROLE_GADGET ]) {dev_err (dev,"no supported roles\n" );include/linux/usb/chipidea.h +0
-0include/linux/usb/chipidea.h struct ci13xxx_platform_data { · line 24
Quoted 6 lines
include/linux/usb/chipidea.h +3
-0@@ -19,6 +19,12 @@ struct ci13xxx_platform_data {#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)#define CI13XXX_PULLUP_ON_VBUS BIT(2)#define CI13XXX_DISABLE_STREAMING BIT(3) +#define CI13XXX_DR_MODE_HOST BIT(4) +#define CI13XXX_DR_MODE_PERIPHERAL BIT(5) +#define CI13XXX_DR_MODE_OTG BIT(6)All we need is
CI13XXX_DR_FORCE_HOSTandCI13XXX_DR_FORCE_DEVICE.Okay.
Quoted 13 lines
Thanks
RichardQuoted 10 lines
include/linux/usb/chipidea.h +3
-0+#define CI13XXX_DR_MODE_MASK \ + (CI13XXX_DR_MODE_HOST | CI13XXX_DR_MODE_PERIPHERAL | \ + CI13XXX_DR_MODE_OTG)#define CI13XXX_CONTROLLER_RESET_EVENT 0#define CI13XXX_CONTROLLER_STOPPED_EVENT 1@@ -35,4 +41,7 @@ struct platform_device *ci13xxx_add_device(struct device *dev,/* Remove ci13xxx device */ void ci13xxx_remove_device (struct platform_device *pdev);Quoted 9 lines
Quoted 8 lines
include/linux/usb/chipidea.h +3
-0+/* Parse of-tree "dr_mode" property */ +void ci13xxx_get_dr_mode (struct device_node *of_node,struct ci13xxx_platform_data *pdata); +#endif Signature · 1 line
1.7.10Quoted 7 lines
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 3/4 usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
In reply to Marc Kleine-BuddeHello.
Quoted 4 lines · Marc Kleine-Budde
This patch allows the device tree to limit the chipidea to host or peripheral mode only.
Signed-off-by Marc Kleine-Budde<[email protected]>
[...]
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 7
Quoted 9 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0@@ -4,6 +4,8 @@ Required properties: - compatible: Should be "fsl,imx27-usb" - reg: Should contain registers location and length - interrupts: Should contain controller interrupt +- dr_mode: indicates the working mode for compatible controllers. Can
Hyphens are preferred to underscores in the device tree property names.
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt Required properties: · line 11
Quoted 4 lines
Documentation/devicetree/bindings/usb/ci13xxx-imx.txt +1
-0+ be "host", "peripheral", or "otg". Defaults to "otg" if not defined. Optional properties: - fsl,usbphy: phandler of usb phy that connects to the only one port
WBR, Sergei
PATCH v2 4/4 ARM: dts: imx23,28: limit usb to host role
In reply to Marc Kleine-BuddeThe driver just supports host for now.
--- arch/arm/boot/dts/imx23.dtsi | 1 + arch/arm/boot/dts/imx28.dtsi | 2 ++ 2 files changed, 3 insertions(+)
arch/arm/boot/dts/imx23.dtsi +1 -0
@@ -289,6 +289,7 @@ usbctrl at 80080000 { reg = <0x80080000 0x10000>; + dr_mode = "host"; status = "disabled"; }; };
arch/arm/boot/dts/imx28.dtsi +2 -0
@@ -461,11 +461,13 @@ usbctrl0: usbctrl at 80080000 { reg = <0x80080000 0x10000>; + dr_mode = "host"; status = "disabled"; }; usbctrl1: usbctrl at 80090000 { reg = <0x80090000 0x10000>; + dr_mode = "host"; status = "disabled"; };
Signature · 1 line
1.7.10