On Mon, Feb 7, 2022 at 6:15 AM Adam Ford <aford...@gmail.com> wrote:
>
> On Mon, Feb 7, 2022 at 5:50 AM Marek Vasut <ma...@denx.de> wrote:
> >
> > On 2/7/22 12:13, Adam Ford wrote:
> > > On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut <ma...@denx.de> wrote:
> > >>
> > >> On 2/7/22 01:51, Adam Ford wrote:
> > >>> On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut <ma...@denx.de> wrote:
> > >>>>
> > >>>> On 2/3/22 22:20, Adam Ford wrote:
> > >>>>> The imx8mm and imx8mn appear compatible with imx7d-usb
> > >>>>> flags in the OTG driver. If the dr_mode is defined as
> > >>>>> host or peripheral, the device appears to operate correctly,
> > >>>>> however the auto host/peripheral detection results in an error.
> > >>>>>
> > >>>>> The solution isn't just adding checks for imx8mm and imx8mn to
> > >>>>> the check for imx7, because the USB clock needs to be running
> > >>>>> to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
> > >>>>>
> > >>>>> Marek requested that I not enable the clocks in ehci_usb_of_to_plat,
> > >>>>> so I modified that function to return an unknown state if the
> > >>>>> device tree does not explicitly state whether it is a host
> > >>>>> or a peripheral.
> > >>>>>
> > >>>>> When the driver probes, it looks to see if it's in the unknown
> > >>>>> state, and only then will it read the register to auto-detect.
> > >>>>>
> > >>>>> Signed-off-by: Adam Ford <aford...@gmail.com>
> > >>>>> Tested-by: Tim Harvey <thar...@gateworks.com>
> > >>>>> ---
> > >>>>> V4: Fix 'err_clk' reference, so it is not encapsulated in
> > >>>>> an ifdef making it available at all times.
> > >>>>> V3: Keep ehci_usb_of_to_plat but add the ability to return
> > >>>>> and unknown state instead of reading the register.
> > >>>>> If the probe determines the states is unknown, it will
> > >>>>> query the register after the clocks have been enabled.
> > >>>>> Because of the slight behavior change, I removed any
> > >>>>> review or tested tags.
> > >>>>>
> > >>>>> V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it
> > >>>>> from the probe after t
> > >>>>>
> > >>>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > >>>>> index 1bd6147c76..060b02accc 100644
> > >>>>> --- a/drivers/usb/host/ehci-mx6.c
> > >>>>> +++ b/drivers/usb/host/ehci-mx6.c
> > >>>>> @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> > >>>>> plat->init_type = USB_INIT_DEVICE;
> > >>>>> else
> > >>>>> plat->init_type = USB_INIT_HOST;
> > >>>>> - } else if (is_mx7()) {
> > >>>>> + } else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
> > >>>>> phy_status = (void __iomem *)(addr +
> > >>>>>
> > >>>>> USBNC_PHY_STATUS_OFFSET);
> > >>>>> val = readl(phy_status);
> > >>>>
> > >>>> Is the USB GPCv2 power domain always ON at this point ? If it isn't,
> > >>>> then this access will lock up the whole SoC.
> > >>>
> > >>> The GPCv2 needs to be enabled if USB is going to be used, but I would
> > >>> expect that to happen even without this patch. I ran into that during
> > >>> my early testing.
> > >>>
> > >>>>
> > >>>> You can check that easily by adding printf() into
> > >>>> drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and
> > >>>> verify that printf() prints something before this ehci_usb_phy_mode()
> > >>>> code is called.
> > >>>
> > >>> I added a printf to show when a power domain is enabled. It does
> > >>> appear that power domains for HSIO, USBOTG1 and USBOTG2 are all
> > >>> enabled before the scan on the respective USB bus happens, so I think
> > >>> we're safe from hanging.
> > >>
> > >> Isn't the ehci_usb_of_to_plat() called way before you even initiate the
> > >> scan, somewhere during U-Boot startup ? So no, you might end up hanging
> > >> the hardware ?
> > >
> > > If you look at what this patch is doing, ehci_usb_of_to_plat is no
> > > longer making any calls to the USB registers. ehci_usb_of_to_plat is
> > > just an exercise in reading the device tree for the desired mode. If
> > > it's not host mode or peripheral mode it returns unknown. The part
> > > that reads the registers now is delayed until after the clocks are
> > > enabled inside the probe function which is called when USB is started
> > > and the power-domains are available.
> >
> > Ah, I missed that part of the change.
> >
> > One more thing, can you double-check that the "if (ctrl->init ==
> > USB_INIT_DEVICE)" checks in drivers/usb/host/ehci-hcd.c still work as
> > they should ? If so, then this patch is fine.
>
> Sure. If I run ums 0 mmc 2, I see the peripheral is enumerating on
> the U-Boot side:
>
> U-Boot 2022.01-00678-ge83818c782-dirty (Feb 06 2022 - 18:55:39 -0600)
>
> CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz
> Reset cause: POR
> Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit
> DRAM: 2 GiB
> Core: 99 devices, 21 uclasses, devicetree: separate
> WDT: Started watchdog@30280000 with servicing (60s timeout)
> MMC: FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... OK
> In: serial@30890000
> Out: serial@30890000
> Err: serial@30890000
> Net: eth0: ethernet@30be0000
> Hit any key to stop autoboot: 0
> u-boot=> ums 0 mmc 2
> UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000
> imx8m_power_domain_on 0
> imx8m_power_domain_on 2
> -
>
> And the power domains enable in the right order.
>
> On the host side, I see:
>
> usb 3-2: new high-speed USB device number 24 using xhci_hcd
> usb 3-2: New USB device found, idVendor=0525, idProduct=a4a5, bcdDevice= 2.21
> usb 3-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> usb 3-2: Product: USB download gadget
> usb 3-2: Manufacturer: U-Boot
> usb-storage 3-2:1.0: USB Mass Storage device detected
> usb-storage 3-2:1.0: Quirks match for vid 0525 pid a4a5: 10000
> scsi host6: usb-storage 3-2:1.0
> scsi 6:0:0:0: Direct-Access Linux UMS disk 0 ffff PQ: 0 ANSI: 2
> sd 6:0:0:0: Attached scsi generic sg2 type 0
> sd 6:0:0:0: [sdb] 61071360 512-byte logical blocks: (31.3 GB/29.1 GiB)
> sd 6:0:0:0: [sdb] Write Protect is off
> sd 6:0:0:0: [sdb] Mode Sense: 0f 00 00 00
> sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't
> support DPO or FUA
> sd 6:0:0:0: [sdb] Attached SCSI removable disk
Marek,
Is this sufficient, or do you need explicit printf's inside the driver
to show the device is in peripheral mode?
adam
