Re: [PATCH 2/3] usb: xhci-mtk: fix UAS issue by XHCI_BROKEN_STREAMS quirk

[Rob Herring]

On Thu, Dec 17, 2020 at 02:23:50PM +0800, Chunfeng Yun wrote:
> On Thu, 2020-12-17 at 11:32 +0800, Nicolas Boichat wrote:
> > On Thu, Dec 17, 2020 at 10:19 AM Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 2020-12-16 at 20:28 +0800, Nicolas Boichat wrote:
> > > > On Wed, Dec 16, 2020 at 7:53 PM Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > The 0.96 xHCI controller on some platforms does not support
> > > > > bulk stream even HCCPARAMS says supporting, due to MaxPSASize
> > > > > is set a non-zero default value by mistake, here use
> > > > > XHCI_BROKEN_STREAMS quirk to fix it.
> > > > >
> > > > > Fixes: 94a631d91ad3 ("usb: xhci-mtk: check hcc_params after adding primary hcd")
> > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/usb/host/xhci-mtk.c | 7 ++++++-
> > > > >  drivers/usb/host/xhci-mtk.h | 1 +
> > > > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > > > > index 8f321f39ab96..08dab974d847 100644
> > > > > --- a/drivers/usb/host/xhci-mtk.c
> > > > > +++ b/drivers/usb/host/xhci-mtk.c
> > > > > @@ -395,6 +395,9 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
> > > > >         xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
> > > > >         if (mtk->lpm_support)
> > > > >                 xhci->quirks |= XHCI_LPM_SUPPORT;
> > > > > +
> > > > > +       if (mtk->broken_streams)
> > > > > +               xhci->quirks |= XHCI_BROKEN_STREAMS;
> > > > >  }
> > > > >
> > > > >  /* called during probe() after chip reset completes */
> > > > > @@ -460,6 +463,8 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > > > >                 return ret;
> > > > >
> > > > >         mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
> > > > > +       mtk->broken_streams =
> > > > > +               of_property_read_bool(node, "mediatek,broken_streams_quirk");
> > > >
> > > > Would it be better to add a data field to struct of_device_id
> > > > mtk_xhci_of_match, and enable this quirk on mediatek,mt8173-xhci only?
> > > This is the common issue for all SoCs (before 2016.06) with 0.96 xHCI
> > > when the controller don't support bulk stream. If enable this quirk only
> > > for mt8173, then for other SoCs, the compatible need include
> > > "mediatek,mt8173-xhci" in dts, this may be not flexible for some cases,
> > > e.g. a new SoC has the broken stream as mt8173, but also has another
> > > different quirk, the way you suggested will not handle it.
> > 
> > It can, we do this regularly for many other components. One example:
> > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-mt65xx.c#L402
> > 
> Got it. Indeed works when add compatible private data.
> 
> Due to many SoCs supports USB and not upstream, I'd prefer to avoid
> adding new compatible in driver when support new SoCs, and leave the
> code as simple as possible.

No. The problem is adding new fixes requires updating the DT. That would 
be fine if you knew all possible issues and quirks up front. You may for 
some, but you won't catch them all.

Save DT properties for per board quirks/features, not per SoC ones.

> 
> > > And I plan to remove "mediatek,mt8173-xhci" in mtk_xhci_of_match after
> > > converting the binding to YMAL.
> > >
> > > >
> > > > (IMHO usb3-lpm-capable detection should also be done in the same way)

I tend to agree, but am more tolerable of standard USB features than 
specific IP block quirks.

Rob