Re: [U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
On 10/10/19 2:55 PM, Igor Opaniuk wrote: > On Thu, Oct 10, 2019 at 3:43 PM Marek Vasut wrote: >> >> On 10/10/19 2:29 PM, Igor Opaniuk wrote: >>> Hi Marek >> >> Hi Igor, >> >>> On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote: On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...] >* from which it derives offsets in the PHY and ANATOP register > sets. >* >* Here we attempt to calculate these indexes from DT information as > - * well as we can. The USB controllers on all existing iMX6/iMX7 > SoCs > - * are placed next to each other, at addresses incremented by 0x200. > - * Thus, the index is derived from the multiple of 0x200 offset from > - * the first controller address. > + * well as we can. The USB controllers on all existing iMX6 SoCs > + * are placed next to each other, at addresses incremented by 0x200, > + * and iMX7 their addresses are shifted by 0x1000. > + * Thus, the index is derived from the multiple of 0x200 (0x1000 for > + * iMX7) offset from the first controller address. >* >* However, to complete conversion of this driver to DT probing, the >* following has to be done: > @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) >* With these changes in place, the ad-hoc indexing goes away and >* the driver is fully converted to DT probing. >*/ > - fdt_size_t size; > - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, ); > +#if defined(CONFIG_MX6) > + u32 controller_spacing = 0x200; > +#elif defined(CONFIG_MX7) > + u32 controller_spacing = 0x1; > +#endif > + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL); This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match. >>> >>> So in that case existing ehci_hcd_init() implementation >>> won't work either, as it does the same. >> >> Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM >> code and probes from information in DT (or platdata), hence no such >> ifdeffery is allowed. >> >>> So if we want to address this case (anyway, is it the real case when both >>> CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement >>> other parts of the driver. >>> >>> But taking into account that it's a temporary workaround and should be fully >>> reimplemented,why not to proceed with a pretty simple solution for now >>> as what is done in ehci-vf.c : >> >> Because this does not work if ehci0 is not your first controller, then >> the derivation of ANATOP bit offsets breaks down because the controller >> indexes are off. That's what the original patch was trying to fix and >> what is described in that long explanation text in the driver too. I was >> trying to make sure this problem is documented. >> >> I think ehci-vf and ehci-mx5 have the same problem and need to be fixed >> too. But the real fix is to finish converting the driver to DM proper >> and fix that ANATOP bit offset derivation properly. Or even pull it out >> of DT, which would be the best. > Thanks for the explanation! > >> >> Anyway, what about this: >> >> u32 controller_spacing = is_mx7() ? 0x1000 : 0x200; >> fdt_addr_t addr = devfdt_get_addr_index(dev, 0); >> >> This gets rid of the ifdeffery and works on both mx6 and mx7, no ? > Makes sense, will change it in v2. Great, thanks! ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
On Thu, Oct 10, 2019 at 3:43 PM Marek Vasut wrote:
>
> On 10/10/19 2:29 PM, Igor Opaniuk wrote:
> > Hi Marek
>
> Hi Igor,
>
> > On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
> >>
> >> On 10/10/19 1:25 PM, Igor Opaniuk wrote:
> >> [...]
> >>>* from which it derives offsets in the PHY and ANATOP register
> >>> sets.
> >>>*
> >>>* Here we attempt to calculate these indexes from DT information as
> >>> - * well as we can. The USB controllers on all existing iMX6/iMX7
> >>> SoCs
> >>> - * are placed next to each other, at addresses incremented by 0x200.
> >>> - * Thus, the index is derived from the multiple of 0x200 offset from
> >>> - * the first controller address.
> >>> + * well as we can. The USB controllers on all existing iMX6 SoCs
> >>> + * are placed next to each other, at addresses incremented by 0x200,
> >>> + * and iMX7 their addresses are shifted by 0x1000.
> >>> + * Thus, the index is derived from the multiple of 0x200 (0x1000 for
> >>> + * iMX7) offset from the first controller address.
> >>>*
> >>>* However, to complete conversion of this driver to DT probing, the
> >>>* following has to be done:
> >>> @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev)
> >>>* With these changes in place, the ad-hoc indexing goes away and
> >>>* the driver is fully converted to DT probing.
> >>>*/
> >>> - fdt_size_t size;
> >>> - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, );
> >>> +#if defined(CONFIG_MX6)
> >>> + u32 controller_spacing = 0x200;
> >>> +#elif defined(CONFIG_MX7)
> >>> + u32 controller_spacing = 0x1;
> >>> +#endif
> >>> + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);
> >>
> >> This won't work with U-Boot that's compiled for both platforms, so you
> >> need some other way to discern those two things. Either something like
> >> cpu_is_...() or some DT match.
> >
> > So in that case existing ehci_hcd_init() implementation
> > won't work either, as it does the same.
>
> Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM
> code and probes from information in DT (or platdata), hence no such
> ifdeffery is allowed.
>
> > So if we want to address this case (anyway, is it the real case when both
> > CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement
> > other parts of the driver.
> >
> > But taking into account that it's a temporary workaround and should be fully
> > reimplemented,why not to proceed with a pretty simple solution for now
> > as what is done in ehci-vf.c :
>
> Because this does not work if ehci0 is not your first controller, then
> the derivation of ANATOP bit offsets breaks down because the controller
> indexes are off. That's what the original patch was trying to fix and
> what is described in that long explanation text in the driver too. I was
> trying to make sure this problem is documented.
>
> I think ehci-vf and ehci-mx5 have the same problem and need to be fixed
> too. But the real fix is to finish converting the driver to DM proper
> and fix that ANATOP bit offset derivation properly. Or even pull it out
> of DT, which would be the best.
Thanks for the explanation!
>
> Anyway, what about this:
>
> u32 controller_spacing = is_mx7() ? 0x1000 : 0x200;
> fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>
> This gets rid of the ifdeffery and works on both mx6 and mx7, no ?
Makes sense, will change it in v2.
>
> > 295 static int vf_usb_bind(struct udevice *dev)
> > 296 {
> > 297 static int num_controllers;
> > 298
> > 299 /*
> > 300 * Without this hack, if we return ENODEV for USB
> > Controller 0, on
> > 301 * probe for the next controller, USB Controller 1 will be
> > given a
> > 302 * sequence number of 0. This conflicts with our
> > requirement of
> > 303 * sequence numbers while initialising the peripherals.
> > 304 */
> > 305 dev->req_seq = num_controllers;
> > 306 num_controllers++;
> > 307
> > 308 return 0;
> > 309 }
> >
> > Thanks
> >
>
>
> --
> Best regards,
> Marek Vasut
Thanks
--
Best regards - Freundliche Grüsse - Meilleures salutations
Igor Opaniuk
mailto: igor.opan...@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
On 10/10/19 2:29 PM, Igor Opaniuk wrote:
> Hi Marek
Hi Igor,
> On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
>>
>> On 10/10/19 1:25 PM, Igor Opaniuk wrote:
>> [...]
>>>* from which it derives offsets in the PHY and ANATOP register sets.
>>>*
>>>* Here we attempt to calculate these indexes from DT information as
>>> - * well as we can. The USB controllers on all existing iMX6/iMX7 SoCs
>>> - * are placed next to each other, at addresses incremented by 0x200.
>>> - * Thus, the index is derived from the multiple of 0x200 offset from
>>> - * the first controller address.
>>> + * well as we can. The USB controllers on all existing iMX6 SoCs
>>> + * are placed next to each other, at addresses incremented by 0x200,
>>> + * and iMX7 their addresses are shifted by 0x1000.
>>> + * Thus, the index is derived from the multiple of 0x200 (0x1000 for
>>> + * iMX7) offset from the first controller address.
>>>*
>>>* However, to complete conversion of this driver to DT probing, the
>>>* following has to be done:
>>> @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev)
>>>* With these changes in place, the ad-hoc indexing goes away and
>>>* the driver is fully converted to DT probing.
>>>*/
>>> - fdt_size_t size;
>>> - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, );
>>> +#if defined(CONFIG_MX6)
>>> + u32 controller_spacing = 0x200;
>>> +#elif defined(CONFIG_MX7)
>>> + u32 controller_spacing = 0x1;
>>> +#endif
>>> + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);
>>
>> This won't work with U-Boot that's compiled for both platforms, so you
>> need some other way to discern those two things. Either something like
>> cpu_is_...() or some DT match.
>
> So in that case existing ehci_hcd_init() implementation
> won't work either, as it does the same.
Only the non-DM_USB one though , right ? The ehci_usb_bind() is a DM
code and probes from information in DT (or platdata), hence no such
ifdeffery is allowed.
> So if we want to address this case (anyway, is it the real case when both
> CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement
> other parts of the driver.
>
> But taking into account that it's a temporary workaround and should be fully
> reimplemented,why not to proceed with a pretty simple solution for now
> as what is done in ehci-vf.c :
Because this does not work if ehci0 is not your first controller, then
the derivation of ANATOP bit offsets breaks down because the controller
indexes are off. That's what the original patch was trying to fix and
what is described in that long explanation text in the driver too. I was
trying to make sure this problem is documented.
I think ehci-vf and ehci-mx5 have the same problem and need to be fixed
too. But the real fix is to finish converting the driver to DM proper
and fix that ANATOP bit offset derivation properly. Or even pull it out
of DT, which would be the best.
Anyway, what about this:
u32 controller_spacing = is_mx7() ? 0x1000 : 0x200;
fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
This gets rid of the ifdeffery and works on both mx6 and mx7, no ?
> 295 static int vf_usb_bind(struct udevice *dev)
> 296 {
> 297 static int num_controllers;
> 298
> 299 /*
> 300 * Without this hack, if we return ENODEV for USB
> Controller 0, on
> 301 * probe for the next controller, USB Controller 1 will be
> given a
> 302 * sequence number of 0. This conflicts with our
> requirement of
> 303 * sequence numbers while initialising the peripherals.
> 304 */
> 305 dev->req_seq = num_controllers;
> 306 num_controllers++;
> 307
> 308 return 0;
> 309 }
>
> Thanks
>
--
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
Hi Marek
On Thu, Oct 10, 2019 at 2:47 PM Marek Vasut wrote:
>
> On 10/10/19 1:25 PM, Igor Opaniuk wrote:
> [...]
> >* from which it derives offsets in the PHY and ANATOP register sets.
> >*
> >* Here we attempt to calculate these indexes from DT information as
> > - * well as we can. The USB controllers on all existing iMX6/iMX7 SoCs
> > - * are placed next to each other, at addresses incremented by 0x200.
> > - * Thus, the index is derived from the multiple of 0x200 offset from
> > - * the first controller address.
> > + * well as we can. The USB controllers on all existing iMX6 SoCs
> > + * are placed next to each other, at addresses incremented by 0x200,
> > + * and iMX7 their addresses are shifted by 0x1000.
> > + * Thus, the index is derived from the multiple of 0x200 (0x1000 for
> > + * iMX7) offset from the first controller address.
> >*
> >* However, to complete conversion of this driver to DT probing, the
> >* following has to be done:
> > @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev)
> >* With these changes in place, the ad-hoc indexing goes away and
> >* the driver is fully converted to DT probing.
> >*/
> > - fdt_size_t size;
> > - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, );
> > +#if defined(CONFIG_MX6)
> > + u32 controller_spacing = 0x200;
> > +#elif defined(CONFIG_MX7)
> > + u32 controller_spacing = 0x1;
> > +#endif
> > + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL);
>
> This won't work with U-Boot that's compiled for both platforms, so you
> need some other way to discern those two things. Either something like
> cpu_is_...() or some DT match.
So in that case existing ehci_hcd_init() implementation
won't work either, as it does the same.
So if we want to address this case (anyway, is it the real case when both
CONFIG_MX6 and CONFIG_MX7 are defined?), we will have to reimplement
other parts of the driver.
But taking into account that it's a temporary workaround and should be fully
reimplemented,why not to proceed with a pretty simple solution for now
as what is done in ehci-vf.c :
295 static int vf_usb_bind(struct udevice *dev)
296 {
297 static int num_controllers;
298
299 /*
300 * Without this hack, if we return ENODEV for USB
Controller 0, on
301 * probe for the next controller, USB Controller 1 will be
given a
302 * sequence number of 0. This conflicts with our
requirement of
303 * sequence numbers while initialising the peripherals.
304 */
305 dev->req_seq = num_controllers;
306 num_controllers++;
307
308 return 0;
309 }
Thanks
--
Best regards - Freundliche Grüsse - Meilleures salutations
Igor Opaniuk
mailto: igor.opan...@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] usb: ehci-mx6: Fix bus enumeration for iMX7 SoCs
On 10/10/19 1:25 PM, Igor Opaniuk wrote: [...] >* from which it derives offsets in the PHY and ANATOP register sets. >* >* Here we attempt to calculate these indexes from DT information as > - * well as we can. The USB controllers on all existing iMX6/iMX7 SoCs > - * are placed next to each other, at addresses incremented by 0x200. > - * Thus, the index is derived from the multiple of 0x200 offset from > - * the first controller address. > + * well as we can. The USB controllers on all existing iMX6 SoCs > + * are placed next to each other, at addresses incremented by 0x200, > + * and iMX7 their addresses are shifted by 0x1000. > + * Thus, the index is derived from the multiple of 0x200 (0x1000 for > + * iMX7) offset from the first controller address. >* >* However, to complete conversion of this driver to DT probing, the >* following has to be done: > @@ -531,10 +532,14 @@ static int ehci_usb_bind(struct udevice *dev) >* With these changes in place, the ad-hoc indexing goes away and >* the driver is fully converted to DT probing. >*/ > - fdt_size_t size; > - fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, ); > +#if defined(CONFIG_MX6) > + u32 controller_spacing = 0x200; > +#elif defined(CONFIG_MX7) > + u32 controller_spacing = 0x1; > +#endif > + fdt_addr_t addr = devfdt_get_addr_size_index(dev, 0, NULL); This won't work with U-Boot that's compiled for both platforms, so you need some other way to discern those two things. Either something like cpu_is_...() or some DT match. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
