| Message ID | 20190829154913.2049-1-thomas.schaefer@kontron.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: imx7: fix USB controller 'size' parameter | expand |
Hi Thomas, [Adding Peter Chen] On Thu, Aug 29, 2019 at 12:49 PM Thomas Schaefer <thomas.schaefer@kontron.com> wrote: > > Currently the size parameter in the reg property of usbotg and > usbh nodes in imx7s and imx7d dts includes is set to 0x200 which > is wrong for the i.MX7 CPU. According to reference manual, spacing > of USB controller registers is 0x10000 instead. > > This patch will fix this and set the 'size' to 0x10000. > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> Thanks for the patch. I would suggest adding one more sentence in the commit log explaining that this size mismatch caused real world problems in U-Boot. This way it makes clear that this change was not done merely upon inspection, but it fixes a real bug instead. Thanks, Fabio Estevam
On 19-08-29 12:58, Fabio Estevam wrote: > Hi Thomas, > > [Adding Peter Chen] > > On Thu, Aug 29, 2019 at 12:49 PM Thomas Schaefer > <thomas.schaefer@kontron.com> wrote: > > > > Currently the size parameter in the reg property of usbotg and > > usbh nodes in imx7s and imx7d dts includes is set to 0x200 which > > is wrong for the i.MX7 CPU. According to reference manual, spacing > > of USB controller registers is 0x10000 instead. > > > > This patch will fix this and set the 'size' to 0x10000. > > > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> > > Thanks for the patch. > > I would suggest adding one more sentence in the commit log explaining > that this size mismatch caused real world problems in U-Boot. > > This way it makes clear that this change was not done merely upon > inspection, but it fixes a real bug instead. Do we need a fixes tag too? Regards, Marco > Thanks, > > Fabio Estevam > >
On 19-08-29 17:49:13, Thomas Schaefer wrote: > Currently the size parameter in the reg property of usbotg and > usbh nodes in imx7s and imx7d dts includes is set to 0x200 which > is wrong for the i.MX7 CPU. According to reference manual, spacing > of USB controller registers is 0x10000 instead. > > This patch will fix this and set the 'size' to 0x10000. > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> > --- > arch/arm/boot/dts/imx7d.dtsi | 2 +- > arch/arm/boot/dts/imx7s.dtsi | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi > index 42528d2812a2..f1b098d28b6e 100644 > --- a/arch/arm/boot/dts/imx7d.dtsi > +++ b/arch/arm/boot/dts/imx7d.dtsi > @@ -117,7 +117,7 @@ > &aips3 { > usbotg2: usb@30b20000 { > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > - reg = <0x30b20000 0x200>; > + reg = <0x30b20000 0x10000>; > interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clks IMX7D_USB_CTRL_CLK>; > fsl,usbphy = <&usbphynop2>; > diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi > index c1a4fff5ceda..9e25fccf33f0 100644 > --- a/arch/arm/boot/dts/imx7s.dtsi > +++ b/arch/arm/boot/dts/imx7s.dtsi > @@ -1088,7 +1088,7 @@ > > usbotg1: usb@30b10000 { > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > - reg = <0x30b10000 0x200>; > + reg = <0x30b10000 0x10000>; > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clks IMX7D_USB_CTRL_CLK>; > fsl,usbphy = <&usbphynop1>; > @@ -1099,7 +1099,7 @@ > > usbh: usb@30b30000 { > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > - reg = <0x30b30000 0x200>; > + reg = <0x30b30000 0x10000>; > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clks IMX7D_USB_CTRL_CLK>; > fsl,usbphy = <&usbphynop3>; Hi Thomos, The core controller range is 0x200, from offset 0x200, it is non-core register, which is used by usbmisc. Fabio said you met problem at u-boot for this size, what's the problem about it? Thanks, Peter
-----Ursprüngliche Nachricht----- Von: Peter Chen <peter.chen@nxp.com> Gesendet: Freitag, 30. August 2019 04:26 > On 19-08-29 17:49:13, Thomas Schaefer wrote: > > Currently the size parameter in the reg property of usbotg and usbh > > nodes in imx7s and imx7d dts includes is set to 0x200 which is wrong > > for the i.MX7 CPU. According to reference manual, spacing of USB > > controller registers is 0x10000 instead. > > > > This patch will fix this and set the 'size' to 0x10000. > > > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> > > --- > > arch/arm/boot/dts/imx7d.dtsi | 2 +- > > arch/arm/boot/dts/imx7s.dtsi | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/boot/dts/imx7d.dtsi > > b/arch/arm/boot/dts/imx7d.dtsi index 42528d2812a2..f1b098d28b6e 100644 > > --- a/arch/arm/boot/dts/imx7d.dtsi > > +++ b/arch/arm/boot/dts/imx7d.dtsi > > @@ -117,7 +117,7 @@ > > &aips3 { > > usbotg2: usb@30b20000 { > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > - reg = <0x30b20000 0x200>; > > + reg = <0x30b20000 0x10000>; > > interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > fsl,usbphy = <&usbphynop2>; > > diff --git a/arch/arm/boot/dts/imx7s.dtsi > > b/arch/arm/boot/dts/imx7s.dtsi index c1a4fff5ceda..9e25fccf33f0 100644 > > --- a/arch/arm/boot/dts/imx7s.dtsi > > +++ b/arch/arm/boot/dts/imx7s.dtsi > > @@ -1088,7 +1088,7 @@ > > > > usbotg1: usb@30b10000 { > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > - reg = <0x30b10000 0x200>; > > + reg = <0x30b10000 0x10000>; > > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > fsl,usbphy = <&usbphynop1>; > > @@ -1099,7 +1099,7 @@ > > > > usbh: usb@30b30000 { > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > - reg = <0x30b30000 0x200>; > > + reg = <0x30b30000 0x10000>; > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > fsl,usbphy = <&usbphynop3>; > > Hi Thomos, > > The core controller range is 0x200, from offset 0x200, it is non-core register, which is used by usbmisc. Fabio said you met problem at u-boot for this size, what's the problem about it? > > Thanks, > Peter Hi Peter, When porting one of our i.MX7 based modules to u-boot v2019.07, I found that scanning USB with 'usb start' crashes when trying to scan the _second_ controller enabled in the device tree (the first controller was detected properly). After some investigation I found that the problem was introduced with Marek Vasuts 'usb: ehci-mx6: Fix bus enumeration for DM case' (u-boot commit 501547cec1f7f0438cae388a104ff60f18576c01). This patch uses the 'reg' property in the usbotg and usbh nodes to calculate the device index number for the driver. Actually, controller range on i.MX6 is 0x200, thus the calculation is correct for i.MX6, but on i.MX7 the base addresses of the controller registers differ by 0x10000 and calculation will fail if more than one USB controller is enabled in the device tree. This is why I suggested to fix this problem in the imx7s and imx7d device tree include files. Added Marek to this thread. Best regards, Thomas
On 19-08-30 07:11:42, Thomas Schaefer wrote: > -----Ursprüngliche Nachricht----- > Von: Peter Chen <peter.chen@nxp.com> > Gesendet: Freitag, 30. August 2019 04:26 > > On 19-08-29 17:49:13, Thomas Schaefer wrote: > > > Currently the size parameter in the reg property of usbotg and usbh > > > nodes in imx7s and imx7d dts includes is set to 0x200 which is wrong > > > for the i.MX7 CPU. According to reference manual, spacing of USB > > > controller registers is 0x10000 instead. > > > > > > This patch will fix this and set the 'size' to 0x10000. > > > > > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> > > > --- > > > arch/arm/boot/dts/imx7d.dtsi | 2 +- > > > arch/arm/boot/dts/imx7s.dtsi | 4 ++-- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm/boot/dts/imx7d.dtsi > > > b/arch/arm/boot/dts/imx7d.dtsi index 42528d2812a2..f1b098d28b6e 100644 > > > --- a/arch/arm/boot/dts/imx7d.dtsi > > > +++ b/arch/arm/boot/dts/imx7d.dtsi > > > @@ -117,7 +117,7 @@ > > > &aips3 { > > > usbotg2: usb@30b20000 { > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > - reg = <0x30b20000 0x200>; > > > + reg = <0x30b20000 0x10000>; > > > interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > fsl,usbphy = <&usbphynop2>; > > > diff --git a/arch/arm/boot/dts/imx7s.dtsi > > > b/arch/arm/boot/dts/imx7s.dtsi index c1a4fff5ceda..9e25fccf33f0 100644 > > > --- a/arch/arm/boot/dts/imx7s.dtsi > > > +++ b/arch/arm/boot/dts/imx7s.dtsi > > > @@ -1088,7 +1088,7 @@ > > > > > > usbotg1: usb@30b10000 { > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > - reg = <0x30b10000 0x200>; > > > + reg = <0x30b10000 0x10000>; > > > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > fsl,usbphy = <&usbphynop1>; > > > @@ -1099,7 +1099,7 @@ > > > > > > usbh: usb@30b30000 { > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > - reg = <0x30b30000 0x200>; > > > + reg = <0x30b30000 0x10000>; > > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > fsl,usbphy = <&usbphynop3>; > > > > Hi Thomos, > > > > The core controller range is 0x200, from offset 0x200, it is non-core register, which is used by usbmisc. Fabio said you met problem at u-boot for this size, what's the problem about it? > > > > Thanks, > > Peter > > Hi Peter, > > When porting one of our i.MX7 based modules to u-boot v2019.07, I found that scanning USB with 'usb start' crashes when trying to scan the _second_ controller enabled in the device tree (the first controller was detected properly). After some investigation I found that the problem was introduced with Marek Vasuts 'usb: ehci-mx6: Fix bus enumeration for DM case' (u-boot commit 501547cec1f7f0438cae388a104ff60f18576c01). This patch uses the 'reg' property in the usbotg and usbh nodes to calculate the device index number for the driver. > > Actually, controller range on i.MX6 is 0x200, thus the calculation is correct for i.MX6, but on i.MX7 the base addresses of the controller registers differ by 0x10000 and calculation will fail if more than one USB controller is enabled in the device tree. This is why I suggested to fix this problem in the imx7s and imx7d device tree include files. > At dts, each USB controller has its reg property to describe its base address, we do not need additional calculation. For controller index, it is only needed for imx6 since we share non-core register from 0x800 among controllers. From imx7, we have dedicate non-core region for each controller, it can be described at dts well. At dts, we use usbmisc to describe the non core part, and the related Linux code is at drivers/usb/chipidea/usbmisc_imx.c
-----Ursprüngliche Nachricht----- Von: Peter Chen <peter.chen@nxp.com> Gesendet: Freitag, 30. August 2019 11:34 > On 19-08-30 07:11:42, Thomas Schaefer wrote: > > -----Ursprüngliche Nachricht----- > > Von: Peter Chen <peter.chen@nxp.com> > > Gesendet: Freitag, 30. August 2019 04:26 > > > On 19-08-29 17:49:13, Thomas Schaefer wrote: > > > > Currently the size parameter in the reg property of usbotg and > > > > usbh nodes in imx7s and imx7d dts includes is set to 0x200 which > > > > is wrong for the i.MX7 CPU. According to reference manual, spacing > > > > of USB controller registers is 0x10000 instead. > > > > > > > > This patch will fix this and set the 'size' to 0x10000. > > > > > > > > Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> > > > > --- > > > > arch/arm/boot/dts/imx7d.dtsi | 2 +- arch/arm/boot/dts/imx7s.dtsi > > > > | 4 ++-- > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/arm/boot/dts/imx7d.dtsi > > > > b/arch/arm/boot/dts/imx7d.dtsi index 42528d2812a2..f1b098d28b6e > > > > 100644 > > > > --- a/arch/arm/boot/dts/imx7d.dtsi > > > > +++ b/arch/arm/boot/dts/imx7d.dtsi > > > > @@ -117,7 +117,7 @@ > > > > &aips3 { > > > > usbotg2: usb@30b20000 { > > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > > - reg = <0x30b20000 0x200>; > > > > + reg = <0x30b20000 0x10000>; > > > > interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > > fsl,usbphy = <&usbphynop2>; > > > > diff --git a/arch/arm/boot/dts/imx7s.dtsi > > > > b/arch/arm/boot/dts/imx7s.dtsi index c1a4fff5ceda..9e25fccf33f0 > > > > 100644 > > > > --- a/arch/arm/boot/dts/imx7s.dtsi > > > > +++ b/arch/arm/boot/dts/imx7s.dtsi > > > > @@ -1088,7 +1088,7 @@ > > > > > > > > usbotg1: usb@30b10000 { > > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > > - reg = <0x30b10000 0x200>; > > > > + reg = <0x30b10000 0x10000>; > > > > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > > fsl,usbphy = <&usbphynop1>; > > > > @@ -1099,7 +1099,7 @@ > > > > > > > > usbh: usb@30b30000 { > > > > compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; > > > > - reg = <0x30b30000 0x200>; > > > > + reg = <0x30b30000 0x10000>; > > > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > > > clocks = <&clks IMX7D_USB_CTRL_CLK>; > > > > fsl,usbphy = <&usbphynop3>; > > > > > > Hi Thomos, > > > > > > The core controller range is 0x200, from offset 0x200, it is non-core register, which is used by usbmisc. Fabio said you met problem at u-boot for this size, what's the problem about it? > > > > > > Thanks, > > > Peter > > > > Hi Peter, > > > > When porting one of our i.MX7 based modules to u-boot v2019.07, I found that scanning USB with 'usb start' crashes when trying to scan the _second_ controller enabled in the device tree (the first controller was detected properly). After some investigation I found that the problem was introduced with Marek Vasuts 'usb: ehci-mx6: Fix bus enumeration for DM case' (u-boot commit 501547cec1f7f0438cae388a104ff60f18576c01). This patch uses the 'reg' property in the usbotg and usbh nodes to calculate the device index number for the driver. > > > > Actually, controller range on i.MX6 is 0x200, thus the calculation is correct for i.MX6, but on i.MX7 the base addresses of the controller registers differ by 0x10000 and calculation will fail if more than one USB controller is enabled in the device tree. This is why I suggested to fix this problem in the imx7s and imx7d device tree include files. > > > > At dts, each USB controller has its reg property to describe its base address, we do not need additional calculation. > > For controller index, it is only needed for imx6 since we share non-core register from 0x800 among controllers. From imx7, we have dedicate non-core region for each controller, it can be described at dts well. > > At dts, we use usbmisc to describe the non core part, and the related Linux code is at drivers/usb/chipidea/usbmisc_imx.c > > -- > > Thanks, > Peter Chen > OK. Understood. This is not the right place to fix this problem in u-boot. Thomas
diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi index 42528d2812a2..f1b098d28b6e 100644 --- a/arch/arm/boot/dts/imx7d.dtsi +++ b/arch/arm/boot/dts/imx7d.dtsi @@ -117,7 +117,7 @@ &aips3 { usbotg2: usb@30b20000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; - reg = <0x30b20000 0x200>; + reg = <0x30b20000 0x10000>; interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX7D_USB_CTRL_CLK>; fsl,usbphy = <&usbphynop2>; diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi index c1a4fff5ceda..9e25fccf33f0 100644 --- a/arch/arm/boot/dts/imx7s.dtsi +++ b/arch/arm/boot/dts/imx7s.dtsi @@ -1088,7 +1088,7 @@ usbotg1: usb@30b10000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; - reg = <0x30b10000 0x200>; + reg = <0x30b10000 0x10000>; interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX7D_USB_CTRL_CLK>; fsl,usbphy = <&usbphynop1>; @@ -1099,7 +1099,7 @@ usbh: usb@30b30000 { compatible = "fsl,imx7d-usb", "fsl,imx27-usb"; - reg = <0x30b30000 0x200>; + reg = <0x30b30000 0x10000>; interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX7D_USB_CTRL_CLK>; fsl,usbphy = <&usbphynop3>;
Currently the size parameter in the reg property of usbotg and usbh nodes in imx7s and imx7d dts includes is set to 0x200 which is wrong for the i.MX7 CPU. According to reference manual, spacing of USB controller registers is 0x10000 instead. This patch will fix this and set the 'size' to 0x10000. Signed-off-by: Thomas Schaefer <thomas.schaefer@kontron.com> --- arch/arm/boot/dts/imx7d.dtsi | 2 +- arch/arm/boot/dts/imx7s.dtsi | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)