On Wed, Feb 13, 2019 at 12:34:39PM +0530, Nagarjuna Kristam wrote:
> Hi Thierry,
>
> On 04-02-2019 17:57, Thierry Reding wrote:
> > On Thu, Jan 03, 2019 at 03:34:52PM +0530, Nagarjuna Kristam wrote:
> >> Add support for XUSB device mode controller on Tegra210.
> >> Update PADCTL driver to set port cap based on DT config.
> >> Add code to handle property "nvidia,usb3-port-fake"
> >> Provide API's to control vbus override and utmi pad power control
> >
> > You essentially list three features additions here, so it makes sense to
> > provide each of them in a separate patch. That helps reviewers
> > concentrate on one feature at a time without having to load context for
> > three different things in parallel.
> >
> > Also, for each feature please provide a bit more background information
> > as well as reasons for why they are added.
> >
>
> For XUSB device control Feature support, we needed all three and hence
> added them as part of one change. If we can have group of individuals CL's
> for providing one feature support, will split them accordingly.
> Doing above will mark current changes as a different patch series, considering
> new patch series will be made, is it fine to move USB gadget driver changes to
> a new series ?
> Please share your inputs
Yes, it's generally fine to rearrange patches in a series or add new
ones as the need arises. One thing to consider is to keep the subject of
the cover letter intact and use proper versioning of patches. Both of
these should help track the series as it evolves.
When splitting up patches, though, make sure that all patches can be
successfully built in the order that you send them. This is important in
order to be able to run git bisections later on. You can easily verify
this by doing something like:
$ git rebase -i --exec '/bin/sh'
once the series is ready. That will automatically drop you into a shell
after each commit and you can run your build commands to verify that
each commit builds. You can also replace "/bin/sh" with the build
command directly to fully automate this process. However you have to
make sure that your build command returns proper error codes on failure
for this to work.
[...]
> >> + reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> >> + reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD;
> >> + padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> >> +
> >> + reg = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
> >> + reg |= XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR;
> >> + padctl_writel(padctl, reg, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
> >> +
> >> + udelay(2);
> >> +
> >> + tegra210_utmi_bias_pad_power_off(padctl, pad);
> >> + usb2->powered_on = false;
> >> +
> >> + mutex_unlock(&padctl->lock);
> >> +}
> >
> > I'm not sure I understand this correctly. This seems to implement some
> > reference counting of its own. Why is that? Is it because it is shared
> > between multiple pads? But then, you're writing to indexed registers,
> > which indicates to me that these are in fact separate pads and not a
> > shared resource. So why the reference counting?
> >
> > I'm also not sure why these functions need to be accessible outside this
> > file. They are not used outside, as far as I can tell.
> >
>
> Lock was added, since the API area called from external context(XUDC driver)
> and to protect against on/off, mutex_lock was done.
> Based on later suggestion in the review, will move utmi pad and utmi bias pad
> power on functions to phy_power_on, with that, mutex lock is not needed any more.
Sounds great!
[...]
> >> @@ -546,9 +546,26 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
> >> {
> >> struct tegra_xusb_port *port = &usb2->base;
> >> struct device_node *np = port->dev.of_node;
> >> + const char *prop_string;
> >> + u32 value;
> >>
> >> usb2->internal = of_property_read_bool(np, "nvidia,internal");
> >>
> >> + usb2->port_cap = USB_PORT_DISABLED; /* default */
> >> + if (!of_property_read_string(np, "mode", &prop_string)) {
> >> + if (!strcmp("host", prop_string))
> >> + usb2->port_cap = USB_HOST_CAP;
> >> + else if (!strcmp("device", prop_string))
> >> + usb2->port_cap = USB_DEVICE_CAP;
> >> + else if (!strcmp("otg", prop_string))
> >> + usb2->port_cap = USB_OTG_CAP;
> >> + }
> >
> > This seems to fulfill the same purpose as this:
> >
> > http://patchwork.ozlabs.org/patch/1031014/
> >
> > Although, looking more closely at that, we can probably just the
> > usb_get_dr_mode() helper for this. There's also a standard property for
> > the dual-role mode, named "dr_mode", albeit the name does not seem to
> > match standard best practices. Seems like it would still make sense to
> > move to that standard property name.
> >
>
> Yes, this part of change is what below patch is doing
> http://patchwork.ozlabs.org/patch/1031014/
> w.r.t using usb_get_dr_mode(), it parses dr_mode string if present in fwnode
> But in our case, we have mode part of port setting and is not inline with
> expectation of API usb_get_dr_mode(). I believe we need to stay with implementation
> as in above link.
Yeah, I realized the same thing when I tried to convert the above patch
to use usb_get_dr_mode() instead. I agree, I think we need to stick with
the current implementation.
> Considering current patch and changes in patch/1031014/ are same,
> please suggest, if changes w.r.t mode reading will be taken as part of
> patch/1031014/ or shall i continue in current patch series
I would suggest that you either stick with this patch or replace it with
the above for now. If the above series goes in first, you can rebase
your series on top of it and drop the duplicate patch. If your series
goes in first, then the above series will need to be rebased and drop
the duplicate patch.
> >> +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
> >> +{
> >> + struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> >> + struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> >> +
> >> + if (padctl->soc->ops->utmi_pad_power_on)
> >> + padctl->soc->ops->utmi_pad_power_on(phy);
> >> +}
> >> +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_on);
> >> +
> >> +void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
> >> +{
> >> + struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> >> + struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> >> +
> >> + if (padctl->soc->ops->utmi_pad_power_down)
> >> + padctl->soc->ops->utmi_pad_power_down(phy);
> >> +}
> >> +EXPORT_SYMBOL_GPL(tegra_phy_xusb_utmi_pad_power_down);
> >
> > I already briefly touched on this earlier, bu why do these have to be
> > exported functions? Can't these simply be abstracted behind the PHY
> > abstraction?
> >
>
> yes, they can be part of phy_power_xxx(). Will remove
> tegra_phy_xusb_utmi_pad_power_xx export functions
Excellent!
[...]
> >> diff --git a/include/linux/phy/tegra/xusb.h b/include/linux/phy/tegra/xusb.h
> >> index 8e1a57a..143af44 100644
> >> --- a/include/linux/phy/tegra/xusb.h
> >> +++ b/include/linux/phy/tegra/xusb.h
> >> @@ -1,5 +1,5 @@
> >> /*
> >> - * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
> >> + * Copyright (c) 2016-2018, NVIDIA CORPORATION. All rights reserved.
> >> *
> >> * This program is free software; you can redistribute it and/or modify it
> >> * under the terms and conditions of the GNU General Public License,
> >> @@ -26,5 +26,10 @@ int tegra_xusb_padctl_hsic_set_idle(struct tegra_xusb_padctl *padctl,
> >> unsigned int port, bool idle);
> >> int tegra_xusb_padctl_usb3_set_lfps_detect(struct tegra_xusb_padctl *padctl,
> >> unsigned int port, bool enable);
> >> +int tegra_xusb_padctl_set_vbus_override(struct tegra_xusb_padctl *padctl);
> >> +int tegra_xusb_padctl_clear_vbus_override(struct tegra_xusb_padctl *padctl);
> >>
> >> +void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy);
> >> +void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy);
> >> +int tegra_phy_xusb_utmi_port_reset_quirk(struct phy *phy);
> >
> > We already have more custom APIs here than I'd like, so I really want to
> > make sure that we're not adding to these if not absolutely necessary.
> >
> > Are there any reasons why we can't somehow hide these behind the
> > existing PHY framework?
> >
> > tegra_phy_xusb_utmi_port_reset_quirk() seems like something that we may
> > be able to implement using the phy_ops->reset() callback. The power_on()
> > and power_off() for the UTMI pad seem like they should be just part of
> > the regular pad ->power_on() and ->power_off() callback implementations
> > (perhaps this means that we need to introduce another pad for this?).
> >
>
> As mentioned earlier, will move utmi pad and utmi bias pad power function
> can be to phy power functions.
>
> tegra_phy_xusb_utmi_port_reset_quirk() does a vbus override only on a specific
> condition, so it cannot be direct replacement of phy_ops->reset.
If XUDC is the only user of the PHY, then it can call phy_reset() on the
PHY at any time that it wants, so it could be a replacement for the
custom API if we choose to.
That said, it seems like this actually operates on the "port" registers
rather than the "pad" registers, so phy_reset() might not be the best
candidate after all, since the PHYs really only deal with pads.
> > Set/clear VBUS override seems like maybe something that could be hidden
> > behind phy_ops->configure(). This is a fairly recent addition, but
> > sounds like we could also push things like HSIC idle and USB3 LPFS
> > detection into that.
> >
> AFAIU, we can not use phy_ops->configure(), since the same has enum argument
> phy_configure_opts_mipi_dphy, which is not inline with needs of vbus override.
> Also, API is not present in current kernel.
The API is part of linux-next, so you could rebase the series on that
and use it. Also, what I was suggesting is that we could extend the new
configure hook with options that can model the VBUS override
functionality. But I also realize that it may be a bit of a stretch to
consider the VBUS override configuration. Again, it's also dealing with
a property that's more in the domain of the port rather than the PHY, so
phy_configure() may not be a good candidate after all.
> So i beleive we need to stay with vbus override API, which instead of 2, will
> export 1(and take argument as set/clear) and other port_reset_quirk
Sounds good. Although I'm wondering if perhaps the name could be made a
bit more generic. What if we called the VBUS override APIs something
like:
int tegra_xusb_padctl_set_otg_mode(struct tegra_xusb_padctl *padctl,
enum usb_dr_mode mode);
That way we avoid having a boolean argument. I find those somewhat
annoying because you can't immediately see from the callsite what they
mean. It's fairly clear in this case, but:
tegra_xusb_padctl_set_otg_mode(padctl, USB_DR_MODE_PERIPHERAL);
is even clearer. And it has the added benefit that we can add more code
to it if we need to without making the name ambiguous.
Again, this isn't a strong objection, so if you prefer to keep the
tegra_xusb_padctl_set_vbus_override(padctl, true/false), feel free to do
so.
Thierry
Attachment:
signature.asc
Description: PGP signature
