Re: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720

[Pali Rohár]

Hello!

On Monday 28 December 2020 13:11:49 Marek Behun wrote:
> Hi Pali and Miquel,
> 
> On Wed, 23 Dec 2020 17:24:03 +0100
> Pali Rohár <pali@xxxxxxxxxx> wrote:
> 
> >  int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
> >  {
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > +	struct device *dev = hcd->self.controller;
> > +	struct phy *phy;
> > +	int ret;
> >  
> >  	/* Without reset on resume, the HC won't work at all */
> >  	xhci->quirks |= XHCI_RESET_ON_RESUME;
> >  
> > +	/* Old bindings miss the PHY handle */
> > +	phy = of_phy_get(dev->of_node, "usb3-phy");
> > +	if (IS_ERR(phy) && PTR_ERR(phy) == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> > +	else if (IS_ERR(phy))
> > +		goto phy_out;
> > +
> > +	ret = phy_init(phy);
> > +	if (ret)
> > +		goto phy_put;
> > +
> > +	ret = phy_set_mode(phy, PHY_MODE_USB_HOST_SS);
> > +	if (ret)
> > +		goto phy_exit;
> > +
> > +	ret = phy_power_on(phy);
> > +	if (ret == -EOPNOTSUPP) {
> > +		/* Skip initializatin of XHCI PHY when it is unsupported by firmware */
> > +		dev_warn(dev, "PHY unsupported by firmware\n");
> > +		xhci->quirks |= XHCI_SKIP_PHY_INIT;
> > +	}
> > +	if (ret)
> > +		goto phy_exit;
> 
> I am not sure if this is the correct way to check whether PHY_INIT
> should be skipped.

I do not have a better idea how to fix described issue for Armada 3720
boards without touching other usb/core/hdc code. I wanted to put these
Armada 3720 changes only into xhci-mvebu/xhci_mvebu_a3700_init_quirk
code so it does not "pollute" other usb generic code.

> Moreover the subsequent phy_power_off:
> 
> > +
> > +	phy_power_off(phy);
> 
> won't power off the PHY, because the corresponding handler in ATF is
> currently empty. 

This is not an issue as the usb core will later power on PHY during
initialization. So I can remove this line, it has no effect on
functionality.

> I guess the patch needs to be in kernel if users are unwilling to upgrade
> ATF firmware.

Yes. If distributions which are running stable kernels (4.14, 4.19)
start updating their kernels to new stable versions (5.4+) then this
upgrade can break support for USB. Similarly for SATA and PCIe (already
fixed and backported to stable kernels). This is relevant e.g. to Debian
which stable version is on 4.19 and therefore is not affected by this
issue yet.

So my opinion is that such thing is a regression if kernel starts
depending on a new firmware version and cause malfunctions of some
subsystems.

Updating kernel on Espressobin or Turris MOX (both A3720) when using
Debian, OpenWRT (or any similar distribution) is relatively easy as
kernel is stored on SD card on rootfs filesystem. Package manager will
update it easily and can do it automatically (without user interation).

But updating ATF firmware is harder as it is stored in nand and on
Espressobin it is part of another M3 secure firmware image. I do not
know any distribution which can do it automatically, it needs to be done
by user.

> The SMC calls for Marvell's comphy are designed to be generic for
> several Marvell platforms (the constants are the same and so one), but
> we still have different drivers for them anyway.
> 
> Maybe it would be better to just not use the ATF implementation at all,
> and implement the comphy driver for A3720 entirely in kernel...

Globalscaletechnolgies already implemented something in their kernel:
https://github.com/globalscaletechnologies/linux/commit/86fa2470c3a867ca9a78e5521a7af037617f5b3b

I do not like too an idea to depends on external RPC API in kernel.
I think that it is better to have native driver in linux kernel instead
of current RPC driver. Bugs in our kernel drivers can be fixed and
backported to stable kernels. But bugs in firmware we cannot fix and we
have to deal with them (in case we are using it).

I think that in past Linus said that Linux kernel does not use nor
depend on x86 BIOS routines/interrupts for similar reasons.

> Miquel, what do you think?
> 
> Marek