On Thu, Aug 08, 2013 at 12:38:59PM +0200, Sascha Hauer wrote:
> On Thu, Aug 08, 2013 at 03:33:01PM +0800, Peter Chen wrote:
> > Since ci_hdrc_imx and usbmisc_imx has relationship between each other,
> > they can't be existed as two modules. We change the code, and make
> > the usbmisc_imx has no longer a driver.
> >
> > Due to above reason, we introduce non core register phandle to know
> > the non core register, and delete the binding doc from usbmisc_imx as well.
> >
> > Signed-off-by: Peter Chen <peter.c...@freescale.com>
> > ---
> > .../devicetree/bindings/usb/ci_hdrc_imx.txt | 12 ++++++++----
> > .../devicetree/bindings/usb/usbmisc-imx.txt | 14 --------------
> > arch/arm/boot/dts/imx25.dtsi | 14 +++++---------
> > arch/arm/boot/dts/imx51.dtsi | 16 +++++++---------
> > arch/arm/boot/dts/imx53.dtsi | 16 +++++++---------
> > arch/arm/boot/dts/imx6qdl.dtsi | 16 +++++++---------
> > 6 files changed, 34 insertions(+), 54 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci_hdrc_imx.txt
> > b/Documentation/devicetree/bindings/usb/ci_hdrc_imx.txt
> > index b4b5b79..56d94cb 100644
> > --- a/Documentation/devicetree/bindings/usb/ci_hdrc_imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci_hdrc_imx.txt
> > @@ -1,4 +1,4 @@
> > -* Freescale i.MX ci13xxx usb controllers
> > +* Freescale i.MX chipidea usb controllers
> >
> > Required properties:
> > - compatible: Should be "fsl,imx27-usb"
> > @@ -13,8 +13,7 @@ Recommended properies:
> >
> > Optional properties:
> > - fsl,usbphy: phandler of usb phy that connects to the only one port
> > -- fsl,usbmisc: phandler of non-core register device, with one argument
> > - that indicate usb controller index
> > +- ci,noncore: phandler of non-core register node
> > - vbus-supply: regulator for vbus
> > - disable-over-current: disable over current detect
> > - external-vbus-divider: enables off-chip resistor divider for Vbus
> > @@ -25,7 +24,12 @@ usb@02184000 { /* USB OTG */
> > reg = <0x02184000 0x200>;
> > interrupts = <0 43 0x04>;
> > fsl,usbphy = <&usbphy1>;
> > - fsl,usbmisc = <&usbmisc 0>;
> > + ci,noncore = <&noncore>;
> > disable-over-current;
> > external-vbus-divider;
> > };
> > +
> > +noncore: usb-non-core@02184800 {
> > + compatible = "fsl,imx-usb-non-core", "syscon";
> > + reg = <0x02184800 0x200>;
> > +};
> > diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > deleted file mode 100644
> > index 97ce94e..0000000
> > --- a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> > +++ /dev/null
> > @@ -1,14 +0,0 @@
> > -* Freescale i.MX non-core registers
> > -
> > -Required properties:
> > -- #index-cells: Cells used to descibe usb controller index. Should be <1>
> > -- compatible: Should be one of below:
> > - "fsl,imx6q-usbmisc" for imx6q
> > -- reg: Should contain registers location and length
> > -
> > -Examples:
> > -usbmisc@02184800 {
> > - #index-cells = <1>;
> > - compatible = "fsl,imx6q-usbmisc";
> > - reg = <0x02184800 0x200>;
> > -};
> > diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
> > index 7011539..a09251b 100644
> > --- a/arch/arm/boot/dts/imx25.dtsi
> > +++ b/arch/arm/boot/dts/imx25.dtsi
> > @@ -460,7 +460,7 @@
> > interrupts = <37>;
> > clocks = <&clks 9>, <&clks 70>, <&clks 8>;
> > clock-names = "ipg", "ahb", "per";
> > - fsl,usbmisc = <&usbmisc 0>;
> > + ci,noncore = <&noncore>;
> > status = "disabled";
> > };
> >
> > @@ -470,17 +470,13 @@
> > interrupts = <35>;
> > clocks = <&clks 9>, <&clks 70>, <&clks 8>;
> > clock-names = "ipg", "ahb", "per";
> > - fsl,usbmisc = <&usbmisc 1>;
> > + ci,noncore = <&noncore>;
> > status = "disabled";
> > };
> >
> > - usbmisc: usbmisc@53ff4600 {
> > - #index-cells = <1>;
> > - compatible = "fsl,imx25-usbmisc";
> > - clocks = <&clks 9>, <&clks 70>, <&clks 8>;
> > - clock-names = "ipg", "ahb", "per";
> > - reg = <0x53ff4600 0x00f>;
> > - status = "disabled";
> > + noncore: usb-non-core@53ff4600 {
> > + compatible = "fsl,imx-usb-non-core", "syscon";
> > + reg = <0x53ff4600 0xf>;
> > };
>
> This is bullshit for multiple reasons.
>
> It's the usbmisc unit that changes between different SoCs, not the
> chipidea core. So pretending the usbmisc unit is generic by removing the
> SoC specific compatible and instead leaking in the SoC type from the
> chipidea device nodes is just crazy.
>
Please do not call usbmisc as "unit", it should not be a driver, it is
a "lib", it is "usbmisc-lib" to implement functions for controller
driver.
At current design, there are two drivers (ci_hdrc_imx & usbmisc_imx) to
control one hardware (one imx usb controller). You will need to control
the same clocks at two drivers, it is not a good idea. Besides, there
are dependencies between two drivers, the usbmisc will be unloaded
first, how usbmisc can supply the functions after it is unloaded?
Remember, it is one hardware, why one driver can forbid another to use
controller's function?
> What you are doing here is to model the DT after how the Linux driver is
> programmed. This really is a bad idea. Remember that the bindings have
> to be OS agnostic. They can't be changed just because you want to
> resolve your module probe dependencies.
I am a beginner for DT, how DT binding can't be changed if the driver
has re-designed? If I understand correctly, you say "bindings have to
be OS agnostic", you mean the same DT can be used at all late Linux
versions after the binding has finished, is it correct?
>
> Also the existing binding with referencing the usbmisc instance with
> <&usbmisc number> is good and convenient. Instead depending on the alias
> number of the chipidea node is quite non-obvious.
>
Again, binding is for driver, if it is not suitable as a driver, what
the meaningful for the existent of binding?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
