Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
Hi,
On Tue, 6 Jul 2021 at 15:17, Marek Vasut wrote:
>
> On 7/6/21 6:11 PM, Tim Harvey wrote:
> > On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut wrote:
> >>
> >> On 7/4/21 5:35 PM, Fabio Estevam wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut wrote:
> >>>
> > Retrieving the USB base addresses from DT would be preferred, yes, but
> > the current code does not do that.
>
> I implemented exactly that in mx6_parse_dt_addrs() , see:
> 4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
> >>>
> >>> We are talking about USB_BASE_ADDR, right?
> >>
> >> All of the addresses used by the driver, I am trying hard to get rid of
> >> all that hard-coding in the driver. They should be parsed out of DT.
> >>
> >
> > Marek,
> >
> > I agree with trying to get rid of all the hard-coding but not all of
> > us are using SPL_DM_USB. I'm not sure how to best tell who is not
> > using DM in the SPL.
>
> You should be able to use PLATDATA in SPL.
>
> The current situation in the driver is total chaos, that must be fixed.
> So, I would say in SPL -> PLATDATA , U-Boot -> DM/DT .
>
> > I know I ran into issues as I'm supporting a
> > family of boards with the same U-Boot and could not find a way to
> > switch dt's early in the SPL after I've detected via EEPROM what board
> > I have.
>
> Right, the platdata should help here. Then you don't need DT.
I suggest using of-platdata, so you can still use DT but it gets
compiled into the C code.
Regards,
Simon
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
On 7/6/21 6:11 PM, Tim Harvey wrote:
On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut wrote:
On 7/4/21 5:35 PM, Fabio Estevam wrote:
Hi Marek,
Hi,
On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut wrote:
Retrieving the USB base addresses from DT would be preferred, yes, but
the current code does not do that.
I implemented exactly that in mx6_parse_dt_addrs() , see:
4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
We are talking about USB_BASE_ADDR, right?
All of the addresses used by the driver, I am trying hard to get rid of
all that hard-coding in the driver. They should be parsed out of DT.
Marek,
I agree with trying to get rid of all the hard-coding but not all of
us are using SPL_DM_USB. I'm not sure how to best tell who is not
using DM in the SPL.
You should be able to use PLATDATA in SPL.
The current situation in the driver is total chaos, that must be fixed.
So, I would say in SPL -> PLATDATA , U-Boot -> DM/DT .
I know I ran into issues as I'm supporting a
family of boards with the same U-Boot and could not find a way to
switch dt's early in the SPL after I've detected via EEPROM what board
I have.
Right, the platdata should help here. Then you don't need DT.
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut wrote:
>
> On 7/4/21 5:35 PM, Fabio Estevam wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut wrote:
> >
> >>> Retrieving the USB base addresses from DT would be preferred, yes, but
> >>> the current code does not do that.
> >>
> >> I implemented exactly that in mx6_parse_dt_addrs() , see:
> >> 4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
> >
> > We are talking about USB_BASE_ADDR, right?
>
> All of the addresses used by the driver, I am trying hard to get rid of
> all that hard-coding in the driver. They should be parsed out of DT.
>
Marek,
I agree with trying to get rid of all the hard-coding but not all of
us are using SPL_DM_USB. I'm not sure how to best tell who is not
using DM in the SPL. I know I ran into issues as I'm supporting a
family of boards with the same U-Boot and could not find a way to
switch dt's early in the SPL after I've detected via EEPROM what board
I have.
Tim
> > imx6/imx7/imxrt provide the USB_BASE_ADDR as define.
>
> That's only because they still need to be fully converted, someone needs
> to write the PHY driver for those. For MX8M, the NOP PHY driver is used.
>
> > If we remove the imx6 definition from
> > arch/arm/include/asm/arch-mx6/imx-regs.h
> > the ehci-mx6: driver fails to build.
> >
> > I didn't want to change ehci-mx6 in this aspect, so that's why I used
> > Frieder's patch that passes
> > USB_BASE_ADDR via define for i.MX8MM too.
> >
> > Is this an acceptable solution?
>
> No, let's not do that, because that exactly un-does the attempt to get
> rid of these hard-coded addresses. Please parse the address out of DT.
> And if you run into something which might still need hard-coded
> addresses, please fix it.
>
> The ehci-mx6 is bad, let's not make it worse, lets fix it instead.
>
> >>> Without providing these defines:
> >>>
> >>> drivers/usb/host/ehci-mx6.c:254:62: error: ‘USB_BASE_ADDR’ undeclared
> >>> (first use in this function); did you mean ‘SRC_BASE_ADDR’?
> >>> 254 | struct usbnc_regs *usbnc = (struct usbnc_regs
> >>> *)(uintptr_t)(USB_BASE_ADDR +
> >>
> >> I suspect you need CONFIG_PHY for mx8m .
> >
> > CONFIG_PHY is already selected by imx8mm_evk_defconfig.
>
> USBNC is iMX7 specific , so you cannot hit that error on iMX8M, there is
> #elif defined(CONFIG_MX7) around it.
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
On 7/4/21 5:35 PM, Fabio Estevam wrote:
Hi Marek,
Hi,
On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut wrote:
Retrieving the USB base addresses from DT would be preferred, yes, but
the current code does not do that.
I implemented exactly that in mx6_parse_dt_addrs() , see:
4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
We are talking about USB_BASE_ADDR, right?
All of the addresses used by the driver, I am trying hard to get rid of
all that hard-coding in the driver. They should be parsed out of DT.
imx6/imx7/imxrt provide the USB_BASE_ADDR as define.
That's only because they still need to be fully converted, someone needs
to write the PHY driver for those. For MX8M, the NOP PHY driver is used.
If we remove the imx6 definition from arch/arm/include/asm/arch-mx6/imx-regs.h
the ehci-mx6: driver fails to build.
I didn't want to change ehci-mx6 in this aspect, so that's why I used
Frieder's patch that passes
USB_BASE_ADDR via define for i.MX8MM too.
Is this an acceptable solution?
No, let's not do that, because that exactly un-does the attempt to get
rid of these hard-coded addresses. Please parse the address out of DT.
And if you run into something which might still need hard-coded
addresses, please fix it.
The ehci-mx6 is bad, let's not make it worse, lets fix it instead.
Without providing these defines:
drivers/usb/host/ehci-mx6.c:254:62: error: ‘USB_BASE_ADDR’ undeclared
(first use in this function); did you mean ‘SRC_BASE_ADDR’?
254 | struct usbnc_regs *usbnc = (struct usbnc_regs
*)(uintptr_t)(USB_BASE_ADDR +
I suspect you need CONFIG_PHY for mx8m .
CONFIG_PHY is already selected by imx8mm_evk_defconfig.
USBNC is iMX7 specific , so you cannot hit that error on iMX8M, there is
#elif defined(CONFIG_MX7) around it.
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
Hi Marek,
On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut wrote:
> > Retrieving the USB base addresses from DT would be preferred, yes, but
> > the current code does not do that.
>
> I implemented exactly that in mx6_parse_dt_addrs() , see:
> 4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
We are talking about USB_BASE_ADDR, right?
imx6/imx7/imxrt provide the USB_BASE_ADDR as define.
If we remove the imx6 definition from arch/arm/include/asm/arch-mx6/imx-regs.h
the ehci-mx6: driver fails to build.
I didn't want to change ehci-mx6 in this aspect, so that's why I used
Frieder's patch that passes
USB_BASE_ADDR via define for i.MX8MM too.
Is this an acceptable solution?
> > Without providing these defines:
> >
> > drivers/usb/host/ehci-mx6.c:254:62: error: ‘USB_BASE_ADDR’ undeclared
> > (first use in this function); did you mean ‘SRC_BASE_ADDR’?
> >254 | struct usbnc_regs *usbnc = (struct usbnc_regs
> > *)(uintptr_t)(USB_BASE_ADDR +
>
> I suspect you need CONFIG_PHY for mx8m .
CONFIG_PHY is already selected by imx8mm_evk_defconfig.
Thanks,
Fabio Estevam
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
On 7/3/21 11:38 PM, Fabio Estevam wrote:
[...]
+#ifdef CONFIG_IMX8MM
+#define USB1_BASE_ADDR 0x32E4
+#define USB2_BASE_ADDR 0x32E5
+#else
+#define USB1_BASE_ADDR 0x3810
+#define USB2_BASE_ADDR 0x3820
+#endif
+#define USB_BASE_ADDRUSB1_BASE_ADDR
+#define USB1_PHY_BASE_ADDR 0x381F
+#define USB2_PHY_BASE_ADDR 0x382F
All of this should come from DT, no ?
Retrieving the USB base addresses from DT would be preferred, yes, but
the current code does not do that.
I implemented exactly that in mx6_parse_dt_addrs() , see:
4dcfa3bcbcb ("usb: ehci-mx6: Parse USB PHY and MISC offsets from DT")
Without providing these defines:
drivers/usb/host/ehci-mx6.c:254:62: error: ‘USB_BASE_ADDR’ undeclared
(first use in this function); did you mean ‘SRC_BASE_ADDR’?
254 | struct usbnc_regs *usbnc = (struct usbnc_regs
*)(uintptr_t)(USB_BASE_ADDR +
I suspect you need CONFIG_PHY for mx8m .
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
Hi Marek, On Sat, Jul 3, 2021 at 5:57 PM Marek Vasut wrote: > > On 7/3/21 9:58 PM, Fabio Estevam wrote: > > From: Frieder Schrempf > > > > The i.MX8MM register addresses differ from i.MX8M in many ways. One > > thing to fix is the USB addresses. > > > > Signed-off-by: Frieder Schrempf > > Signed-off-by: Fabio Estevam > > --- > > arch/arm/include/asm/arch-imx8m/imx-regs.h | 11 +++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm/include/asm/arch-imx8m/imx-regs.h > > b/arch/arm/include/asm/arch-imx8m/imx-regs.h > > index b800da13a1e4..de01e9969626 100644 > > --- a/arch/arm/include/asm/arch-imx8m/imx-regs.h > > +++ b/arch/arm/include/asm/arch-imx8m/imx-regs.h > > @@ -51,6 +51,17 @@ > > > > #define TZASC_BASE_ADDR 0x32F8 > > > > +#ifdef CONFIG_IMX8MM > > +#define USB1_BASE_ADDR 0x32E4 > > +#define USB2_BASE_ADDR 0x32E5 > > +#else > > +#define USB1_BASE_ADDR 0x3810 > > +#define USB2_BASE_ADDR 0x3820 > > +#endif > > +#define USB_BASE_ADDRUSB1_BASE_ADDR > > +#define USB1_PHY_BASE_ADDR 0x381F > > +#define USB2_PHY_BASE_ADDR 0x382F > > All of this should come from DT, no ? Retrieving the USB base addresses from DT would be preferred, yes, but the current code does not do that. Without providing these defines: drivers/usb/host/ehci-mx6.c:254:62: error: ‘USB_BASE_ADDR’ undeclared (first use in this function); did you mean ‘SRC_BASE_ADDR’? 254 | struct usbnc_regs *usbnc = (struct usbnc_regs *)(uintptr_t)(USB_BASE_ADDR +
Re: [PATCH 1/8] imx8mm: Fix USB reg addresses for i.MX8MM
On 7/3/21 9:58 PM, Fabio Estevam wrote: From: Frieder Schrempf The i.MX8MM register addresses differ from i.MX8M in many ways. One thing to fix is the USB addresses. Signed-off-by: Frieder Schrempf Signed-off-by: Fabio Estevam --- arch/arm/include/asm/arch-imx8m/imx-regs.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/include/asm/arch-imx8m/imx-regs.h b/arch/arm/include/asm/arch-imx8m/imx-regs.h index b800da13a1e4..de01e9969626 100644 --- a/arch/arm/include/asm/arch-imx8m/imx-regs.h +++ b/arch/arm/include/asm/arch-imx8m/imx-regs.h @@ -51,6 +51,17 @@ #define TZASC_BASE_ADDR 0x32F8 +#ifdef CONFIG_IMX8MM +#define USB1_BASE_ADDR 0x32E4 +#define USB2_BASE_ADDR 0x32E5 +#else +#define USB1_BASE_ADDR 0x3810 +#define USB2_BASE_ADDR 0x3820 +#endif +#define USB_BASE_ADDR USB1_BASE_ADDR +#define USB1_PHY_BASE_ADDR 0x381F +#define USB2_PHY_BASE_ADDR 0x382F All of this should come from DT, no ?
