Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[gregkh@xxxxxxxxxxxxxxxxxxx]

On Fri, Oct 28, 2022 at 03:11:31AM +0000, Jim Lin wrote:
> On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote:
> > > Program USB2 pad PD controls during port connect/disconnect, port
> > > suspend/resume, and test mode, to reduce power consumption on
> > > disconnect or suspend.
> > > 
> > > Signed-off-by: Petlozu Pravareshwar <petlozup@xxxxxxxxxx>
> > > Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx>
> > > Signed-off-by: Jim Lin <jilin@xxxxxxxxxx>
> > 
> > Who is the author here?  These do not seem to be in the correct order
> > if
> > you are the author, right?
> > > This is an old patch. Each time went with some small modification.
> 
> 
> Petlozu is author for local Kernel 3.18
> 
> Then JC for local Kernel 4.4
> Now my turn for Kernel 5.xx

Then you all need to figure out how to proper document this (hint, read
the documentation for how to do that...)

> > > ---
> > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124
> > > v3: No change on copyright
> > > v4: Remove hcd_to_tegra_xusb() function which is used only once.
> > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c)
> > >     Invoke xhci_hub_control() directly (xhci-tegra.c)
> > > 
> > >  drivers/usb/host/xhci-tegra.c | 131
> > > +++++++++++++++++++++++++++++++++-
> > >  1 file changed, 130 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-
> > > tegra.c
> > > index c8af2cd2216d..f685bb7459ba 100644
> > > --- a/drivers/usb/host/xhci-tegra.c
> > > +++ b/drivers/usb/host/xhci-tegra.c
> > > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc {
> > >       } fpci;
> > >  };
> > > 
> > > +enum tegra_xhci_phy_type {
> > > +     USB3_PHY,
> > > +     USB2_PHY,
> > > +     HSIC_PHY,
> > > +     MAX_PHY_TYPES,
> > > +};
> > > +
> > >  struct tegra_xusb_soc {
> > >       const char *firmware;
> > >       const char * const *supply_names;
> > > @@ -274,6 +281,7 @@ struct tegra_xusb {
> > > 
> > >       bool suspended;
> > >       struct tegra_xusb_context context;
> > > +     u32 enable_utmi_pad_after_lp0_exit;
> > 
> > This is a bitfield, how do we know it will fit in a u32?  What is the
> > range you are putting in here?
> > 
> > thanks,
> > 
> > greg k-h
> static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb
> *tegra)
> {
> 	unsigned int i;
> 
> 	for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) {
> 		if (!is_host_mode_phy(tegra, USB2_PHY, i))
> 			continue;
> 		/* USB2 */
> 		if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i))
> :
> How many bits to be used is based on tegra->soc-
> >phy_types[USB2_PHY].num which is defined like
> 
> static const struct tegra_xusb_phy_type tegra210_phy_types[] = {
> :
> 	{ .name = "usb2", .num = 4, },
> :
> };
> 
> static const struct tegra_xusb_phy_type tegra194_phy_types[] = {
> :
> 	{ .name = "usb2", .num = 4, },
> };
> , so far at most 4.
> 
> Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough.

I am sorry, I do not understand.  If you are needing to treat this as a
bitfield, then properly define it that way so it is obvious what is
happening.

As it is, this is very confusing and I do not understand what is going
on at all.  What is the relationship to "num" and the bit being set?
Why set a bit at all?

thanks,

greg k-h