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 <[email protected]>
> Signed-off-by: JC Kuo <[email protected]>
> Signed-off-by: Jim Lin <[email protected]>
Who is the author here? These do not seem to be in the correct order if
you are the author, right?
>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: JC Kuo <[email protected]>
> > Signed-off-by: Jim Lin <[email protected]>
>
> 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
>
> >
> > ---
> > 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.
--nvpublic
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 <[email protected]>
> > > Signed-off-by: JC Kuo <[email protected]>
> > > Signed-off-by: Jim Lin <[email protected]>
> >
> > 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