* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-29 8:45 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-29 8:45 UTC (permalink / raw)
To: Marc Kleine-Budde, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Devicetree Discussions
On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>
> Cc'ed Devicetree Discussions
>
> On 06/29/2012 03:43 AM, Richard Zhao wrote:
> > On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> >> This patch allows the device tree to limit the chipidea to host or
> >> peripheral mode only.
> >>
> >> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> ---
> >> .../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(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >> index 5a0ad66..67f97f56 100644
> >> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >> @@ -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.
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 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:
> > - 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:
> > - 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?
>
> >>
> >> Optional properties:
> >> - fsl,usbphy: phandler of usb phy that connects to the only one port
> >> @@ -14,4 +16,5 @@ usb@02184000 { /* USB OTG */
> >> reg = <0x02184000 0x200>;
> >> interrupts = <0 43 0x04>;
> >> fsl,usbphy = <&usbphy1>;
> >> + dr_mode= "otg";
> >> };
> >> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> >> index efae2be..8e926fb 100644
> >> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> >> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> >> @@ -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);
> >> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> >> index 1083585..aa8b1856 100644
> >> --- a/drivers/usb/chipidea/core.c
> >> +++ b/drivers/usb/chipidea/core.c
> >> @@ -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);
>
> > 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.
You may return some common flag.
>
> >> +
> >> 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");
> >> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> >> index 544825d..29ad908 100644
> >> --- a/include/linux/usb/chipidea.h
> >> +++ b/include/linux/usb/chipidea.h
> >> @@ -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.
>
> Okay.
>
> >
> > Thanks
> > Richard
> >> +#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
> >> --
> >> 1.7.10
> >>
> >>
> >
>
> Marc
> --
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-29 9:29 ` Marc Kleine-Budde
0 siblings, 0 replies; 35+ messages in thread
From: Marc Kleine-Budde @ 2012-06-29 9:29 UTC (permalink / raw)
To: linux-arm-kernel
On 06/29/2012 10:45 AM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>
>> Cc'ed Devicetree Discussions
>>
>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>> This patch allows the device tree to limit the chipidea to host or
>>>> peripheral mode only.
>>>>
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>> .../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(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> index 5a0ad66..67f97f56 100644
>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> @@ -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.
> 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?
>> 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:
>>> - 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:
>>> - 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?
Sensible name sounds good. Devicetree Discussions, we need an official
name for the property and its values :)
Marc
--
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>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-29 9:29 ` Marc Kleine-Budde
0 siblings, 0 replies; 35+ messages in thread
From: Marc Kleine-Budde @ 2012-06-29 9:29 UTC (permalink / raw)
To: Richard Zhao
Cc: olof-nZhT3qVonbNeoWH0uzbU5w, grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Devicetree Discussions
[-- Attachment #1: Type: text/plain, Size: 3727 bytes --]
On 06/29/2012 10:45 AM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>
>> Cc'ed Devicetree Discussions
>>
>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>> This patch allows the device tree to limit the chipidea to host or
>>>> peripheral mode only.
>>>>
>>>> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> ---
>>>> .../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(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> index 5a0ad66..67f97f56 100644
>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> @@ -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.
> 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?
>> 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:
>>> - 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:
>>> - 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?
Sensible name sounds good. Devicetree Discussions, we need an official
name for the property and its values :)
Marc
--
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:40 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:40 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 29, 2012 at 11:29:03AM +0200, Marc Kleine-Budde wrote:
> On 06/29/2012 10:45 AM, Richard Zhao wrote:
> > On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >>
> >> Cc'ed Devicetree Discussions
> >>
> >> On 06/29/2012 03:43 AM, Richard Zhao wrote:
> >>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> >>>> This patch allows the device tree to limit the chipidea to host or
> >>>> peripheral mode only.
> >>>>
> >>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >>>> ---
> >>>> .../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(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> index 5a0ad66..67f97f56 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> @@ -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.
> > 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
>
> >> 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:
> >>> - 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:
> >>> - 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?
>
> Sensible name sounds good. Devicetree Discussions, we need an official
> name for the property and its values :)
>
> Marc
> --
> 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 |
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:40 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:40 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Richard Zhao, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Devicetree Discussions
On Fri, Jun 29, 2012 at 11:29:03AM +0200, Marc Kleine-Budde wrote:
> On 06/29/2012 10:45 AM, Richard Zhao wrote:
> > On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >>
> >> Cc'ed Devicetree Discussions
> >>
> >> On 06/29/2012 03:43 AM, Richard Zhao wrote:
> >>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> >>>> This patch allows the device tree to limit the chipidea to host or
> >>>> peripheral mode only.
> >>>>
> >>>> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> ---
> >>>> .../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(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> index 5a0ad66..67f97f56 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> @@ -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.
> > 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
>
> >> 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:
> >>> - 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:
> >>> - 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?
>
> Sensible name sounds good. Devicetree Discussions, we need an official
> name for the property and its values :)
>
> Marc
> --
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-29 15:51 ` Stephen Warren
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Warren @ 2012-06-29 15:51 UTC (permalink / raw)
To: linux-arm-kernel
On 06/29/2012 02:45 AM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>
>> Cc'ed Devicetree Discussions
>>
>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>> This patch allows the device tree to limit the chipidea to host or
>>>> peripheral mode only.
>>>>
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>> .../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(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> index 5a0ad66..67f97f56 100644
>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> @@ -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.
>
> 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 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:
>>> - 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:
>>> - 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?
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.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-29 15:51 ` Stephen Warren
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Warren @ 2012-06-29 15:51 UTC (permalink / raw)
To: Richard Zhao
Cc: Marc Kleine-Budde, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 06/29/2012 02:45 AM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>
>> Cc'ed Devicetree Discussions
>>
>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>> This patch allows the device tree to limit the chipidea to host or
>>>> peripheral mode only.
>>>>
>>>> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> ---
>>>> .../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(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> index 5a0ad66..67f97f56 100644
>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>> @@ -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.
>
> 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 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:
>>> - 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:
>>> - 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?
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:19 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:19 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 29, 2012 at 09:51:49AM -0600, Stephen Warren wrote:
> On 06/29/2012 02:45 AM, Richard Zhao wrote:
> > On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >>
> >> Cc'ed Devicetree Discussions
> >>
> >> On 06/29/2012 03:43 AM, Richard Zhao wrote:
> >>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> >>>> This patch allows the device tree to limit the chipidea to host or
> >>>> peripheral mode only.
> >>>>
> >>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >>>> ---
> >>>> .../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(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> index 5a0ad66..67f97f56 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> @@ -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.
> >
> > 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 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:
> >>> - 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:
> >>> - 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?
>
> 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.
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
> --
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:19 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:19 UTC (permalink / raw)
To: Stephen Warren
Cc: Richard Zhao, Marc Kleine-Budde, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Fri, Jun 29, 2012 at 09:51:49AM -0600, Stephen Warren wrote:
> On 06/29/2012 02:45 AM, Richard Zhao wrote:
> > On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >>
> >> Cc'ed Devicetree Discussions
> >>
> >> On 06/29/2012 03:43 AM, Richard Zhao wrote:
> >>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> >>>> This patch allows the device tree to limit the chipidea to host or
> >>>> peripheral mode only.
> >>>>
> >>>> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> ---
> >>>> .../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(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> index 5a0ad66..67f97f56 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >>>> @@ -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.
> >
> > 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 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:
> >>> - 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:
> >>> - 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?
>
> 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.
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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
2012-06-30 1:19 ` Richard Zhao
@ 2012-07-02 20:04 ` Stephen Warren
-1 siblings, 0 replies; 35+ messages in thread
From: Stephen Warren @ 2012-07-02 20:04 UTC (permalink / raw)
To: linux-arm-kernel
On 06/29/2012 07:19 PM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:51:49AM -0600, Stephen Warren wrote:
>> On 06/29/2012 02:45 AM, Richard Zhao wrote:
>>> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>>>
>>>> Cc'ed Devicetree Discussions
>>>>
>>>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>>>> This patch allows the device tree to limit the chipidea to host or
>>>>>> peripheral mode only.
>>>>>>
>>>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>>>> ---
>>>>>> .../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(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> index 5a0ad66..67f97f56 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> @@ -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.
>>>
>>> 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 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:
>>>>> - 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:
>>>>> - 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?
>>
>> 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.
>
> 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).
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.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-02 20:04 ` Stephen Warren
0 siblings, 0 replies; 35+ messages in thread
From: Stephen Warren @ 2012-07-02 20:04 UTC (permalink / raw)
To: Richard Zhao
Cc: Richard Zhao, Marc Kleine-Budde, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 06/29/2012 07:19 PM, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:51:49AM -0600, Stephen Warren wrote:
>> On 06/29/2012 02:45 AM, Richard Zhao wrote:
>>> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
>>>>
>>>> Cc'ed Devicetree Discussions
>>>>
>>>> On 06/29/2012 03:43 AM, Richard Zhao wrote:
>>>>> On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
>>>>>> This patch allows the device tree to limit the chipidea to host or
>>>>>> peripheral mode only.
>>>>>>
>>>>>> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>>> ---
>>>>>> .../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(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> index 5a0ad66..67f97f56 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>>>>>> @@ -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.
>>>
>>> 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 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:
>>>>> - 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:
>>>>> - 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?
>>
>> 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.
>
> 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).
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 2:22 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2012-07-03 2:22 UTC (permalink / raw)
To: linux-arm-kernel
> 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.
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.
> --
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 2:22 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2012-07-03 2:22 UTC (permalink / raw)
To: Stephen Warren
Cc: Richard Zhao, Richard Zhao, Marc Kleine-Budde,
olof-nZhT3qVonbNeoWH0uzbU5w, grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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.
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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 3:00 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-07-03 3:00 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > 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.
>
> 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.
>
> 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.
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
>
> 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.
>
> > --
> > 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 3:00 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-07-03 3:00 UTC (permalink / raw)
To: Peter Chen
Cc: Stephen Warren, Richard Zhao, Marc Kleine-Budde,
olof-nZhT3qVonbNeoWH0uzbU5w, grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > 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.
>
> 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.
>
> 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.
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
>
> 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.
>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 7:02 ` Lothar Waßmann
0 siblings, 0 replies; 35+ messages in thread
From: Lothar Waßmann @ 2012-07-03 7:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Richard Zhao writes:
> On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > > 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.
> >
> > 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.
> >
> > 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.
> 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
--
___________________________________________________________
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
___________________________________________________________
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 7:02 ` Lothar Waßmann
0 siblings, 0 replies; 35+ messages in thread
From: Lothar Waßmann @ 2012-07-03 7:02 UTC (permalink / raw)
To: Richard Zhao
Cc: alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Marc Kleine-Budde,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Peter Chen, Richard Zhao
Hi,
Richard Zhao writes:
> On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > > 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.
> >
> > 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.
> >
> > 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.
> 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
--
___________________________________________________________
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@karo-electronics.de
___________________________________________________________
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 7:10 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-07-03 7:10 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 03, 2012 at 09:02:40AM +0200, Lothar Wa?mann wrote:
> Hi,
>
> Richard Zhao writes:
> > On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > > > 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.
> > >
> > > 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.
> > >
> > > 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.
> > 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.
Right, it _force_ the controller to a certain mode.
Thanks
Richard
>
>
> Lothar Wa?mann
> --
> ___________________________________________________________
>
> 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
> ___________________________________________________________
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-07-03 7:10 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-07-03 7:10 UTC (permalink / raw)
To: Lothar Waßmann
Cc: alexander.shishkin-VuQAYsv1563Yd54FQh9/CA, Devicetree Discussions,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Marc Kleine-Budde,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Peter Chen, Richard Zhao
On Tue, Jul 03, 2012 at 09:02:40AM +0200, Lothar Waßmann wrote:
> Hi,
>
> Richard Zhao writes:
> > On Tue, Jul 03, 2012 at 10:22:37AM +0800, Peter Chen wrote:
> > > > 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.
> > >
> > > 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.
> > >
> > > 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.
> > 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.
Right, it _force_ the controller to a certain mode.
Thanks
Richard
>
>
> Lothar Waßmann
> --
> ___________________________________________________________
>
> 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-AvR2QvxeiV7DiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org
> ___________________________________________________________
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:33 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:33 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 29, 2012 at 04:45:10PM +0800, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >
> > Cc'ed Devicetree Discussions
> >
> > On 06/29/2012 03:43 AM, Richard Zhao wrote:
> > > On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> > >> This patch allows the device tree to limit the chipidea to host or
> > >> peripheral mode only.
> > >>
> > >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > >> ---
> > >> .../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(-)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> index 5a0ad66..67f97f56 100644
> > >> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> @@ -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.
> 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 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:
> > > - 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:
> > > - 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?
> >
> > >>
> > >> 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";
> > >> };
> > >> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> > >> index efae2be..8e926fb 100644
> > >> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> > >> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > >> @@ -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);
> > >> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > >> index 1083585..aa8b1856 100644
> > >> --- a/drivers/usb/chipidea/core.c
> > >> +++ b/drivers/usb/chipidea/core.c
> > >> @@ -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);
> >
> > > 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.
> 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.
> >
> > >> +
> > >> 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");
> > >> + }
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
> > >>
> > >> if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
> > >> dev_err(dev, "no supported roles\n");
> > >> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> > >> index 544825d..29ad908 100644
> > >> --- a/include/linux/usb/chipidea.h
> > >> +++ b/include/linux/usb/chipidea.h
> > >> @@ -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.
> >
> > Okay.
> >
> > >
> > > Thanks
> > > Richard
> > >> +#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
> > >> --
> > >> 1.7.10
> > >>
> > >>
> > >
> >
> > Marc
> > --
> > 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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/4] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
@ 2012-06-30 1:33 ` Richard Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Richard Zhao @ 2012-06-30 1:33 UTC (permalink / raw)
To: Richard Zhao
Cc: Marc Kleine-Budde, olof-nZhT3qVonbNeoWH0uzbU5w,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
swarren-DDmLM1+adcrQT0dZR+AlfA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexander.shishkin-VuQAYsv1563Yd54FQh9/CA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Devicetree Discussions
On Fri, Jun 29, 2012 at 04:45:10PM +0800, Richard Zhao wrote:
> On Fri, Jun 29, 2012 at 09:47:03AM +0200, Marc Kleine-Budde wrote:
> >
> > Cc'ed Devicetree Discussions
> >
> > On 06/29/2012 03:43 AM, Richard Zhao wrote:
> > > On Thu, Jun 28, 2012 at 03:53:48PM +0200, Marc Kleine-Budde wrote:
> > >> This patch allows the device tree to limit the chipidea to host or
> > >> peripheral mode only.
> > >>
> > >> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > >> ---
> > >> .../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(-)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> index 5a0ad66..67f97f56 100644
> > >> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >> @@ -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.
> 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 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:
> > > - 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:
> > > - 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?
> >
> > >>
> > >> Optional properties:
> > >> - fsl,usbphy: phandler of usb phy that connects to the only one port
> > >> @@ -14,4 +16,5 @@ usb@02184000 { /* USB OTG */
> > >> reg = <0x02184000 0x200>;
> > >> interrupts = <0 43 0x04>;
> > >> fsl,usbphy = <&usbphy1>;
> > >> + dr_mode= "otg";
> > >> };
> > >> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> > >> index efae2be..8e926fb 100644
> > >> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> > >> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > >> @@ -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);
> > >> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > >> index 1083585..aa8b1856 100644
> > >> --- a/drivers/usb/chipidea/core.c
> > >> +++ b/drivers/usb/chipidea/core.c
> > >> @@ -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);
> >
> > > 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.
> 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.
> >
> > >> +
> > >> 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");
> > >> + }
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
> > >>
> > >> if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
> > >> dev_err(dev, "no supported roles\n");
> > >> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> > >> index 544825d..29ad908 100644
> > >> --- a/include/linux/usb/chipidea.h
> > >> +++ b/include/linux/usb/chipidea.h
> > >> @@ -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.
> >
> > Okay.
> >
> > >
> > > Thanks
> > > Richard
> > >> +#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
> > >> --
> > >> 1.7.10
> > >>
> > >>
> > >
> >
> > Marc
> > --
> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread