| Message ID | 20210703195837.28153-2-festevam@denx.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | imx8mm: Add serial download support | expand |
On 7/3/21 9:58 PM, Fabio Estevam wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > 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 <frieder.schrempf@kontron.de> > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > 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 0x32F80000 > > +#ifdef CONFIG_IMX8MM > +#define USB1_BASE_ADDR 0x32E40000 > +#define USB2_BASE_ADDR 0x32E50000 > +#else > +#define USB1_BASE_ADDR 0x38100000 > +#define USB2_BASE_ADDR 0x38200000 > +#endif > +#define USB_BASE_ADDR USB1_BASE_ADDR > +#define USB1_PHY_BASE_ADDR 0x381F0000 > +#define USB2_PHY_BASE_ADDR 0x382F0000 All of this should come from DT, no ?
Hi Marek, On Sat, Jul 3, 2021 at 5:57 PM Marek Vasut <marex@denx.de> wrote: > > On 7/3/21 9:58 PM, Fabio Estevam wrote: > > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > > > 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 <frieder.schrempf@kontron.de> > > Signed-off-by: Fabio Estevam <festevam@denx.de> > > --- > > 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 0x32F80000 > > > > +#ifdef CONFIG_IMX8MM > > +#define USB1_BASE_ADDR 0x32E40000 > > +#define USB2_BASE_ADDR 0x32E50000 > > +#else > > +#define USB1_BASE_ADDR 0x38100000 > > +#define USB2_BASE_ADDR 0x38200000 > > +#endif > > +#define USB_BASE_ADDR USB1_BASE_ADDR > > +#define USB1_PHY_BASE_ADDR 0x381F0000 > > +#define USB2_PHY_BASE_ADDR 0x382F0000 > > 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 +
On 7/3/21 11:38 PM, Fabio Estevam wrote: [...] >>> +#ifdef CONFIG_IMX8MM >>> +#define USB1_BASE_ADDR 0x32E40000 >>> +#define USB2_BASE_ADDR 0x32E50000 >>> +#else >>> +#define USB1_BASE_ADDR 0x38100000 >>> +#define USB2_BASE_ADDR 0x38200000 >>> +#endif >>> +#define USB_BASE_ADDR USB1_BASE_ADDR >>> +#define USB1_PHY_BASE_ADDR 0x381F0000 >>> +#define USB2_PHY_BASE_ADDR 0x382F0000 >> >> 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 .
Hi Marek, On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut <marex@denx.de> 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
On 7/4/21 5:35 PM, Fabio Estevam wrote: > Hi Marek, Hi, > On Sat, Jul 3, 2021 at 10:04 PM Marek Vasut <marex@denx.de> 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.
On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut <marex@denx.de> 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 <marex@denx.de> 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.
On 7/6/21 6:11 PM, Tim Harvey wrote: > On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut <marex@denx.de> 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 <marex@denx.de> 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.
Hi, On Tue, 6 Jul 2021 at 15:17, Marek Vasut <marex@denx.de> wrote: > > On 7/6/21 6:11 PM, Tim Harvey wrote: > > On Sun, Jul 4, 2021 at 12:25 PM Marek Vasut <marex@denx.de> 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 <marex@denx.de> 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
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 0x32F80000 +#ifdef CONFIG_IMX8MM +#define USB1_BASE_ADDR 0x32E40000 +#define USB2_BASE_ADDR 0x32E50000 +#else +#define USB1_BASE_ADDR 0x38100000 +#define USB2_BASE_ADDR 0x38200000 +#endif +#define USB_BASE_ADDR USB1_BASE_ADDR +#define USB1_PHY_BASE_ADDR 0x381F0000 +#define USB2_PHY_BASE_ADDR 0x382F0000 + #define MXS_LCDIF_BASE IS_ENABLED(CONFIG_IMX8MQ) ? \ 0x30320000 : 0x32e00000