Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control

[Philipp Zabel]

On Wed, 2018-04-04 at 10:25 +0900, Masahiro Yamada wrote:
> 2018-04-03 19:35 GMT+09:00 Vivek Gautam <vivek.gau...@codeaurora.org>:
> > 
> > 
> > On 4/3/2018 3:49 PM, Masahiro Yamada wrote:
> > > 
> > > 2018-04-03 17:46 GMT+09:00 Philipp Zabel <p.za...@pengutronix.de>:
> > > > 
> > > > On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
> > > > > 
> > > > > 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.za...@pengutronix.de>:
> > > > > > 
> > > > > > On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
> > > > > > > 
> > > > > > > This driver handles the reset control in a common manner; deassert
> > > > > > > resets before use, assert them after use.  There is no good reason
> > > > > > > why it should be exclusive.
> > > > > > 
> > > > > > Is this preemptive cleanup, or do you have hardware on the horizon 
> > > > > > that
> > > > > > shares these reset lines with other peripherals?
> > > > > 
> > > > > This patch is necessary for Socionext SoCs.
> > > > > 
> > > > > The same reset lines are shared between
> > > > > this dwc3-of_simple and other glue circuits.
> > > > 
> > > > Thanks, this is helpful information.
> > > > 
> > > > > > > Also, use devm_ for clean-up.
> > > > > > > 
> > > > > > > Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > CCing Philipp Zabel.
> > > > > > > I see his sob in commit 06c47e6286d5.
> > > > > > 
> > > > > > At the time I was concerned with the reset_control_array addition 
> > > > > > and
> > > > > > didn't look closely at the exclusive vs shared issue.
> > > > > > > 
> > > > > > >   drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
> > > > > > >   1 file changed, 2 insertions(+), 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> > > > > > > b/drivers/usb/dwc3/dwc3-of-simple.c
> > > > > > > index e54c362..bd6ab65 100644
> > > > > > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > > > > > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > > > > > > @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > >platform_set_drvdata(pdev, simple);
> > > > > > >simple->dev = dev;
> > > > > > > 
> > > > > > > - simple->resets =
> > > > > > > of_reset_control_array_get_optional_exclusive(np);
> > > > > > > + simple->resets =
> > > > > > > devm_reset_control_array_get_optional_shared(dev);
> > > > > > 
> > > > > >  From the usage in the driver, it does indeed look like _shared 
> > > > > > reset
> > > > > > usage is appropriate. I assume that the hardware has no need for the
> > > > > > reset to be asserted right before probe or after remove, it just
> > > > > > requires that the reset line is kept deasserted while the driver is
> > > > > > probed.
> > > > > > 
> > > > > > >if (IS_ERR(simple->resets)) {
> > > > > > >ret = PTR_ERR(simple->resets);
> > > > > > >dev_err(dev, "failed to get device resets, 
> > > > > > > err=%d\n",
> > > > > > > ret);
> > > > > > > @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > > 
> > > > > > >ret = reset_control_deassert(simple->resets);
> > > > > > >if (ret)
> > > > > > > - goto err_resetc_put;
> > > > > > > + return ret;
> > > > > > > 
> > > > > > >ret = dwc3_of_simple_clk_init(simple,
> > > > > > > of_count_phandle_with_args(np,
> > > > > > &g

Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control

[Philipp Zabel]

On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
> 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.za...@pengutronix.de>:
> > On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
> > > This driver handles the reset control in a common manner; deassert
> > > resets before use, assert them after use.  There is no good reason
> > > why it should be exclusive.
> > 
> > Is this preemptive cleanup, or do you have hardware on the horizon that
> > shares these reset lines with other peripherals?
> 
> This patch is necessary for Socionext SoCs.
> 
> The same reset lines are shared between
> this dwc3-of_simple and other glue circuits.

Thanks, this is helpful information.

> > > Also, use devm_ for clean-up.
> > > 
> > > Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
> > > ---
> > > 
> > > CCing Philipp Zabel.
> > > I see his sob in commit 06c47e6286d5.
> > 
> > At the time I was concerned with the reset_control_array addition and
> > didn't look closely at the exclusive vs shared issue.
> > >  drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> > > b/drivers/usb/dwc3/dwc3-of-simple.c
> > > index e54c362..bd6ab65 100644
> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > > @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device 
> > > *pdev)
> > >   platform_set_drvdata(pdev, simple);
> > >   simple->dev = dev;
> > > 
> > > - simple->resets = of_reset_control_array_get_optional_exclusive(np);
> > > + simple->resets = devm_reset_control_array_get_optional_shared(dev);
> > 
> > From the usage in the driver, it does indeed look like _shared reset
> > usage is appropriate. I assume that the hardware has no need for the
> > reset to be asserted right before probe or after remove, it just
> > requires that the reset line is kept deasserted while the driver is
> > probed.
> > 
> > >   if (IS_ERR(simple->resets)) {
> > >   ret = PTR_ERR(simple->resets);
> > >   dev_err(dev, "failed to get device resets, err=%d\n", ret);
> > > @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct 
> > > platform_device *pdev)
> > > 
> > >   ret = reset_control_deassert(simple->resets);
> > >   if (ret)
> > > - goto err_resetc_put;
> > > + return ret;
> > > 
> > >   ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
> > >   "clocks", "#clock-cells"));
> > > @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct 
> > > platform_device *pdev)
> > >  err_resetc_assert:
> > >   reset_control_assert(simple->resets);
> > > 
> > > -err_resetc_put:
> > > - reset_control_put(simple->resets);
> > >   return ret;
> > >  }
> > > 
> > > @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct 
> > > platform_device *pdev)
> > >   simple->num_clocks = 0;
> > > 
> > >   reset_control_assert(simple->resets);
> > > - reset_control_put(simple->resets);
> > > 
> > >   pm_runtime_put_sync(dev);
> > >   pm_runtime_disable(dev);
> > 
> > Changing to devm_ changes the order here. Whether or not it could be a
> > problem to assert the reset only after pm_runtime_put (or potentially
> > never), I can't say. I assume this is a non-issue, but somebody who
> > knows the hardware better would have to decide.
> 
> 
> 
> I do not understand what you mean.

Sorry for the confusion, I have mixed up things here.

> Can you describe your concern in more details?
> 
> I am not touching reset_control_assert() here.

With the change to shared reset control, reset_control_assert
potentially does nothing, so it could be possible that
pm_runtime_put_sync cuts the power before the reset es asserted again.

> I am delaying the call for reset_control_put().

Yes, please disregard my comment about the devm_ change, that should
have no effect whatsoever and looks fine to me.

> If I understand reset_control_put() correctly,
> the effects of this change are:
>   - The ref_count and module ownership for the reset controller
> driver will be held a little longer
>   - The call for kfree() will be a little bit delayed.

Correct.

> Why do you need knowledge about this hardware?

Is it ok to keep the reset deasserted while the power is cut? Or do you
have to guarantee that drivers sharing the same reset also keep the same
power domains active?

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control

[Philipp Zabel]

On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
> This driver handles the reset control in a common manner; deassert
> resets before use, assert them after use.  There is no good reason
> why it should be exclusive.

Is this preemptive cleanup, or do you have hardware on the horizon that
shares these reset lines with other peripherals?

> Also, use devm_ for clean-up.
> 
> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
> ---
> 
> CCing Philipp Zabel.
> I see his sob in commit 06c47e6286d5.

At the time I was concerned with the reset_control_array addition and
didn't look closely at the exclusive vs shared issue.

>  drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index e54c362..bd6ab65 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>   platform_set_drvdata(pdev, simple);
>   simple->dev = dev;
>  
> - simple->resets = of_reset_control_array_get_optional_exclusive(np);
> + simple->resets = devm_reset_control_array_get_optional_shared(dev);

>From the usage in the driver, it does indeed look like _shared reset
usage is appropriate. I assume that the hardware has no need for the
reset to be asserted right before probe or after remove, it just
requires that the reset line is kept deasserted while the driver is
probed.

>   if (IS_ERR(simple->resets)) {
>   ret = PTR_ERR(simple->resets);
>   dev_err(dev, "failed to get device resets, err=%d\n", ret);
> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>  
>   ret = reset_control_deassert(simple->resets);
>   if (ret)
> - goto err_resetc_put;
> + return ret;
>  
>   ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
>   "clocks", "#clock-cells"));
> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>  err_resetc_assert:
>   reset_control_assert(simple->resets);
>  
> -err_resetc_put:
> - reset_control_put(simple->resets);
>   return ret;
>  }
>  
> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device 
> *pdev)
>   simple->num_clocks = 0;
>  
>   reset_control_assert(simple->resets);
> - reset_control_put(simple->resets);
>  
>   pm_runtime_put_sync(dev);
>   pm_runtime_disable(dev);

Changing to devm_ changes the order here. Whether or not it could be a
problem to assert the reset only after pm_runtime_put (or potentially
never), I can't say. I assume this is a non-issue, but somebody who
knows the hardware better would have to decide.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Not enough bandwidth with Magewell XI100DUSB-HDMI

[Philipp Zabel]

Hi,

On Fri, Jan 19, 2018 at 2:10 PM, Michael Tretter
 wrote:
> I found the old thread and it sounds exactly like my issue. Different
> camera, but same xHCI controller.

I have exactly the same issue with the xHCI controller of my laptop and
"Oculus Sensor" USB3 isochronous mostly-UVC cameras:

00:14.0 USB controller: Intel Corporation Wildcat Point-LP USB xHCI
Controller (rev 03) (prog-if 30 [XHCI])
Subsystem: Lenovo Wildcat Point-LP USB xHCI Controller
Flags: bus master, medium devsel, latency 0, IRQ 44
Memory at f222 (64-bit, non-prefetchable) [size=64K]
Capabilities: [70] Power Management version 2
Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
Kernel driver in use: xhci_hcd
Kernel modules: xhci_pci

T:  Bus=02 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  4 Spd=5000 MxCh= 0
D:  Ver= 3.00 Cls=ef(misc ) Sub=02 Prot=01 MxPS= 9 #Cfgs=  1
P:  Vendor=2833 ProdID=0211 Rev= 0.00
S:  Manufacturer=Oculus VR
S:  Product=Rift Sensor
S:  SerialNumber=WMTD3034300BCT
C:* #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=800mA
A:  FirstIf#= 0 IfCount= 2 Cls=ff(vend.) Sub=03 Prot=00
I:* If#= 0 Alt= 0 #EPs= 1 Cls=ff(vend.) Sub=01 Prot=00 Driver=uvcvideo
E:  Ad=83(I) Atr=03(Int.) MxPS=  32 Ivl=128ms
I:* If#= 1 Alt= 0 #EPs= 0 Cls=ff(vend.) Sub=02 Prot=00 Driver=uvcvideo
I:  If#= 1 Alt= 1 #EPs= 1 Cls=ff(vend.) Sub=02 Prot=00 Driver=uvcvideo
E:  Ad=81(I) Atr=05(Isoc) MxPS=1024 Ivl=125us
I:  If#= 1 Alt= 2 #EPs= 1 Cls=ff(vend.) Sub=02 Prot=00 Driver=uvcvideo
E:  Ad=81(I) Atr=05(Isoc) MxPS=1024 Ivl=125us

Any USB2 or USB3 device that I plug in while the first camera is streaming in
altsetting 1 or 2 causes the bandwidth error. The same happens when I try to
change the altsetting on an isochronous endpoint of an already plugged device.
While the camera is in altsetting 0, other devices can be probed and work.

> For some tests, I changed the xhci_change_max_exit_latency() function
> to ignore the requested MEL and set the MEL to 0. Now the USB devices
> are detected correctly.

Exactly the same thing helps here, as well. With this hack, streaming from two
of those cameras at the same time works without any apparent problem:

--8<--
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 297536c9fd00..3cb4a60d8822 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4025,7 +4025,7 @@ static int __maybe_unused
xhci_change_max_exit_latency(struct xhci_hcd *xhci,
ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
slot_ctx = xhci_get_slot_ctx(xhci, command->in_ctx);
slot_ctx->dev_info2 &= cpu_to_le32(~((u32) MAX_EXIT));
-   slot_ctx->dev_info2 |= cpu_to_le32(max_exit_latency);
+   slot_ctx->dev_info2 |= cpu_to_le32(0);
slot_ctx->dev_state = 0;

xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
-->8--

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

Hi Bjorn,

On Wed, 2017-11-01 at 15:24 -0700, Bjorn Andersson wrote:
[...]
> > > This looks more or less identical to how regulators and clocks already
> > > deals with resources in bulk; see regulator_bulk_data and clk_bulk_data
> > > and their associated functions.
> > > 
> > > I would really like to see that you follow this model, to make it easier
> > > for developers to work with and use the various subsystems.
> > 
> > These APIs have two undesirable (in this case) properties; the driver
> > has to know the number of resets and their identifiers in advance, and
> > singular resets and bulk reset arrays can't be used interchangeably.
> 
> As a writer of device drivers as well as dts files I greatly appreciate
> when this expectations is encoded in the kernel, so that it is clear
> when the DT node is missing some resource - rather than having random
> reboots because of spelling mistakes or variations between hardware
> revisions.
> 
> We tend to express these things explicitly in the kernel, as magic
> interfaces makes things harder to debug.

I have no control over how most of those bindings are designed. While I
prefer bindings to explicitly specify resets by identifier where
possible, there are some generic bindings that just don't have this
information.
See for example the ohci/ehci-platform USB host drivers, which are used
for platform integration on various SoCs, or the Tegra pmc driver, which
has to handle resets for other peripherals when power gating.

Also, I currently don't see many drivers that would profit much from a
bulk API, as many drivers that request multiple reset controls have to
handle them individually anyway, to guarantee ordering or reset timings.
One candidate would be the pcie-qcom driver. If you are aware of more
potential users of a bulk API, please let me know.

> > Both are not well suited to this use case, which is "triggering one or
> > any number of anonymous resets together".
> > 
> 
> Triggering one is just a special case of N. 
>
> But this does not change the fact that the reset framework interface
> looks and function in a fundamentally different way than the clock and
> regulator equivalents, which will be confusing - in particular since
> most drivers will use 2 or 3 of these.

There is no way to make them work all exactly the same, as the resources
 themselves are used in different ways. Surely we should try to minimize
the API differences to allow transfer of expectations, but that should
not be the only goal.
For example neither clock nor regulator framework have support for
exclusive clocks/regulators that can be forced-off, and _optional
requests are handled differently in the gpio and regulator frameworks.

That being said, I'd be happy to add a bulk API if that actually helps
some drivers.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Fri, Oct 20, 2017 at 04:51:24PM +0100, Jon Hunter wrote:
> Hi Philipp,
> 
> On 19/10/17 16:17, Philipp Zabel wrote:
> > Hi Jon, Thierry,
> > 
> > On Wed, 2017-07-19 at 17:59 +0200, Philipp Zabel wrote:
> >> From: Vivek Gautam <vivek.gau...@codeaurora.org>
> >>
> >> Make use of of_reset_control_array_get_exclusive() to manage
> >> an array of reset controllers available with the device.
> >>
> >> Cc: Jon Hunter <jonath...@nvidia.com>
> >> Cc: Thierry Reding <tred...@nvidia.com>
> >> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> >> [p.za...@pengutronix.de: switch to hidden reset control array]
> >> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > 
> > will you pick this up now that the prerequisite patch 1 is contained in
> > master?
> > Please let me know if there are any issues with this patch.
> > 
> > regards
> > Philipp
> > 
> >> ---
> >> No changes since v6.
> >> ---
> >>  drivers/soc/tegra/pmc.c | 82 
> >> -
> >>  1 file changed, 20 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> >> index e233dd5dcab3d..749b218147a19 100644
> >> --- a/drivers/soc/tegra/pmc.c
> >> +++ b/drivers/soc/tegra/pmc.c
> >> @@ -124,8 +124,7 @@ struct tegra_powergate {
> >>unsigned int id;
> >>struct clk **clks;
> >>unsigned int num_clks;
> >> -  struct reset_control **resets;
> >> -  unsigned int num_resets;
> >> +  struct reset_control *reset;
> >>  };
> >>  
> >>  struct tegra_io_pad_soc {
> >> @@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
> >> tegra_powergate *pg)
> >>return err;
> >>  }
> >>  
> >> -static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> >> +static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> >>  {
> >> -  unsigned int i;
> >> -  int err;
> >> -
> >> -  for (i = 0; i < pg->num_resets; i++) {
> >> -  err = reset_control_assert(pg->resets[i]);
> >> -  if (err)
> >> -  return err;
> >> -  }
> >> -
> >> -  return 0;
> >> +  return reset_control_assert(pg->reset);
> >>  }
> >>  
> >> -static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
> >> +static inline int tegra_powergate_reset_deassert(struct tegra_powergate 
> >> *pg)
> >>  {
> >> -  unsigned int i;
> >> -  int err;
> >> -
> >> -  for (i = 0; i < pg->num_resets; i++) {
> >> -  err = reset_control_deassert(pg->resets[i]);
> >> -  if (err)
> >> -  return err;
> >> -  }
> >> -
> >> -  return 0;
> >> +  return reset_control_deassert(pg->reset);
> >>  }
> 
> Nit-pick ... I think we should just get rid of this inline functions now
> and just call reset_control_assert/deassert() where these are used.
> 
[...]
> 
> Otherwise ...
> 
> Acked-by: Jon Hunter <jonath...@nvidia.com>
> 
> Thierry has just sent out his pull requests for v4.15 so we may have
> missed this release :-(

Thanks, I'll drop the inline functions and send a new version, then.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

Hi,

On Thu, 2017-10-19 at 11:54 -0700, Bjorn Andersson wrote:
> On Wed 19 Jul 08:59 PDT 2017, Philipp Zabel wrote:
> 
> > From: Vivek Gautam <vivek.gau...@codeaurora.org>
> > 
> > Many devices may want to request a bunch of resets and control them. So
> > it's better to manage them as an array. Add APIs to _get() an array of
> > reset_control, reusing the _assert(), _deassert(), and _reset() APIs for
> > single reset controls. Since reset controls already may control multiple
> > reset lines with a single hardware bit, from the user perspective, reset
> > control arrays are not at all different from single reset controls.
> > Note that these APIs don't guarantee that the reset lines managed in the
> > array are handled in any particular order.
> > 
> > Cc: Felipe Balbi <ba...@kernel.org>
> > Cc: Jon Hunter <jonath...@nvidia.com>
> > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > [p.za...@pengutronix.de: changed API to hide reset control arrays behind
> >  struct reset_control]
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> 
> This looks more or less identical to how regulators and clocks already
> deals with resources in bulk; see regulator_bulk_data and clk_bulk_data
> and their associated functions.
> 
> I would really like to see that you follow this model, to make it easier
> for developers to work with and use the various subsystems.

These APIs have two undesirable (in this case) properties; the driver
has to know the number of resets and their identifiers in advance, and
singular resets and bulk reset arrays can't be used interchangeably.
Both are not well suited to this use case, which is "triggering one or
any number of anonymous resets together".

I have nothing against adding a bulk API as well. There are already
users such as the pcie-qcom driver that could profit from it.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Fri, 2017-10-20 at 17:10 +0530, Manu Gautam wrote:
> Hi,
> 
> 
> On 10/19/2017 5:17 PM, Philipp Zabel wrote:
> > From: Vivek Gautam <vivek.gau...@codeaurora.org>
> > 
> > Add support to get a list of resets available for the device.
> > These resets must be kept de-asserted until the device is
> > in use.
> > 
> > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > [p.za...@pengutronix.de: switch to hidden reset control array]
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > ---
> > v7: Rebased onto 
> > git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
> > ---
> >  drivers/usb/dwc3/dwc3-of-simple.c | 27 +--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> > b/drivers/usb/dwc3/dwc3-of-simple.c
> > index e129c32780818..ceea1619f8aa3 100644
> > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > @@ -28,11 +28,13 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  struct dwc3_of_simple {
> > struct device   *dev;
> > struct clk  **clks;
> > int num_clocks;
> > +   struct reset_control*resets;
> >  };
> >  
> >  static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int 
> > count)
> > @@ -95,10 +97,21 @@ static int dwc3_of_simple_probe(struct platform_device 
> > *pdev)
> > platform_set_drvdata(pdev, simple);
> > simple->dev = dev;
> >  
> > +   simple->resets = of_reset_control_array_get_optional_exclusive(np);
> > +   if (IS_ERR(simple->resets)) {
> > +   ret = PTR_ERR(simple->resets);
> > +   dev_err(dev, "failed to get device resets, err=%d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = reset_control_deassert(simple->resets);
> > +   if (ret)
> > +   goto err_resetc_put;
> > +
> 
> Are these reset signals asserted by default after power on?
> I think correct way to handle this should be to have explicit 
> reset_control_assert
> before deassert.

I can only assume. If the resets can be initially deasserted (for
example because a bootloader may leave them in this state) and it is
critical for the device internal state to be reset for the driver to
function, the control should be asserted explicitly.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

Hi Jon, Thierry,

On Wed, 2017-07-19 at 17:59 +0200, Philipp Zabel wrote:
> From: Vivek Gautam <vivek.gau...@codeaurora.org>
> 
> Make use of of_reset_control_array_get_exclusive() to manage
> an array of reset controllers available with the device.
> 
> Cc: Jon Hunter <jonath...@nvidia.com>
> Cc: Thierry Reding <tred...@nvidia.com>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> [p.za...@pengutronix.de: switch to hidden reset control array]
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>

will you pick this up now that the prerequisite patch 1 is contained in
master?
Please let me know if there are any issues with this patch.

regards
Philipp

> ---
> No changes since v6.
> ---
>  drivers/soc/tegra/pmc.c | 82 
> -
>  1 file changed, 20 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e233dd5dcab3d..749b218147a19 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -124,8 +124,7 @@ struct tegra_powergate {
>   unsigned int id;
>   struct clk **clks;
>   unsigned int num_clks;
> - struct reset_control **resets;
> - unsigned int num_resets;
> + struct reset_control *reset;
>  };
>  
>  struct tegra_io_pad_soc {
> @@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
> tegra_powergate *pg)
>   return err;
>  }
>  
> -static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_assert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_assert(pg->reset);
>  }
>  
> -static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_deassert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_deassert(pg->reset);
>  }
>  
>  static int tegra_powergate_power_up(struct tegra_powergate *pg,
> @@ -566,8 +547,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
> struct clk *clk,
>   pg.id = id;
>   pg.clks = 
>   pg.num_clks = 1;
> - pg.resets = 
> - pg.num_resets = 1;
> + pg.reset = IS_ERR(rst) ? NULL : rst;
>  
>   err = tegra_powergate_power_up(, false);
>   if (err)
> @@ -755,45 +735,26 @@ static int tegra_powergate_of_get_clks(struct 
> tegra_powergate *pg,
>  static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
>struct device_node *np, bool off)
>  {
> - struct reset_control *rst;
> - unsigned int i, count;
>   int err;
>  
> - count = of_count_phandle_with_args(np, "resets", "#reset-cells");
> - if (count == 0)
> - return -ENODEV;
> -
> - pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
> - if (!pg->resets)
> - return -ENOMEM;
> -
> - for (i = 0; i < count; i++) {
> - pg->resets[i] = of_reset_control_get_by_index(np, i);
> - if (IS_ERR(pg->resets[i])) {
> - err = PTR_ERR(pg->resets[i]);
> - goto error;
> - }
> -
> - if (off)
> - err = reset_control_assert(pg->resets[i]);
> - else
> - err = reset_control_deassert(pg->resets[i]);
> -
> - if (err) {
> - reset_control_put(pg->resets[i]);
> - goto error;
> - }
> + pg->reset = of_reset_control_array_get_exclusive(np);
> + if (IS_ERR(pg->reset)) {
> + pr_err("failed to get device resets\n");
> + return PTR_ERR(pg->reset);
>   }
>  
> - pg->num_resets = count;
> + if (off)
> + err = reset_control_assert(pg->reset);
> + else
> + err = reset_control_deassert(pg->reset);
>  
> - return 0;
> + if (err)
> + goto put_reset;
>  
> -error:
> - while (i--)
> - reset_control_put(pg->resets[i]);
> + retur

Re: [PATCH v7 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Thu, 2017-10-19 at 14:31 +0300, Felipe Balbi wrote:
> Hi,
> 
> Philipp Zabel <p.za...@pengutronix.de> writes:
> > Hi Felipe,
> > 
> > On Thu, 2017-10-19 at 12:38 +0300, Felipe Balbi wrote:
> > > Philipp Zabel <p.za...@pengutronix.de> writes:
> > > 
> > > > From: Vivek Gautam <vivek.gau...@codeaurora.org>
> > > > 
> > > > Add support to get a list of resets available for the device.
> > > > These resets must be kept de-asserted until the device is
> > > > in use.
> > > > 
> > > > Cc: Felipe Balbi <ba...@kernel.org>
> > > > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > > > [p.za...@pengutronix.de: switch to hidden reset control array]
> > > > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > > 
> > > now this seems like it depends on patch 1/4, right? Is the reset API
> > > change going on v4.15?
> > 
> > Thank you for the reminder. Patch 1 has made it into v4.14-rc1, commit
> > 17c82e206d2a ("reset: Add APIs to manage array of resets"). The other
> > patches could be picked up for v4.15 without any dependency issues.
> 
> patch 2 applied fine. patch 3 fails to apply hunk 2.
> 
> checking file drivers/usb/dwc3/dwc3-of-simple.c
> Hunk #1 succeeded at 28 (offset -1 lines).
> Hunk #2 FAILED at 98.
> 1 out of 5 hunks FAILED
> 
> 
> Care to rebase on my testing/next?

Thanks, I'll resend patch 3 separately.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Add support to get a list of resets available for the device.
These resets must be kept de-asserted until the device is
in use.

Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
v7: Rebased onto git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
testing/next
---
 drivers/usb/dwc3/dwc3-of-simple.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index e129c32780818..ceea1619f8aa3 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -28,11 +28,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct dwc3_of_simple {
struct device   *dev;
struct clk  **clks;
int num_clocks;
+   struct reset_control*resets;
 };
 
 static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
@@ -95,10 +97,21 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
platform_set_drvdata(pdev, simple);
simple->dev = dev;
 
+   simple->resets = of_reset_control_array_get_optional_exclusive(np);
+   if (IS_ERR(simple->resets)) {
+   ret = PTR_ERR(simple->resets);
+   dev_err(dev, "failed to get device resets, err=%d\n", ret);
+   return ret;
+   }
+
+   ret = reset_control_deassert(simple->resets);
+   if (ret)
+   goto err_resetc_put;
+
ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
"clocks", "#clock-cells"));
if (ret)
-   return ret;
+   goto err_resetc_assert;
 
ret = of_platform_populate(np, NULL, NULL, dev);
if (ret) {
@@ -107,7 +120,7 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
-   return ret;
+   goto err_resetc_assert;
}
 
pm_runtime_set_active(dev);
@@ -115,6 +128,13 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
pm_runtime_get_sync(dev);
 
return 0;
+
+err_resetc_assert:
+   reset_control_assert(simple->resets);
+
+err_resetc_put:
+   reset_control_put(simple->resets);
+   return ret;
 }
 
 static int dwc3_of_simple_remove(struct platform_device *pdev)
@@ -130,6 +150,9 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
+   reset_control_assert(simple->resets);
+   reset_control_put(simple->resets);
+
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

Hi Felipe,

On Thu, 2017-10-19 at 12:38 +0300, Felipe Balbi wrote:
> Philipp Zabel <p.za...@pengutronix.de> writes:
> 
> > From: Vivek Gautam <vivek.gau...@codeaurora.org>
> > 
> > Add support to get a list of resets available for the device.
> > These resets must be kept de-asserted until the device is
> > in use.
> > 
> > Cc: Felipe Balbi <ba...@kernel.org>
> > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > [p.za...@pengutronix.de: switch to hidden reset control array]
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> 
> now this seems like it depends on patch 1/4, right? Is the reset API
> change going on v4.15?

Thank you for the reminder. Patch 1 has made it into v4.14-rc1, commit
17c82e206d2a ("reset: Add APIs to manage array of resets"). The other
patches could be picked up for v4.15 without any dependency issues.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 0/4] reset: APIs to manage a list of resets

[Philipp Zabel]

A set of patches to allow consumers to get and de/assert or trigger
a number of resets at the same time. A patch on top of Vivek's original
API extension is added to hide the reset_control_array behind a struct
reset_control so that the consumer doesn't have to care about the difference
between a singular reset control and a reset control controlling an array
of resets, except when requesting it.

This series also contains reset controls patches for dwc3-of-simple
and tegra pmc drivers.
A small patch is added in this series to correctly re-order the
resource handling in dwc3_of_simple_remove().

The series is tested on torvald's master branch the device tree
patches to enable usb on db820c.

Changes since v6:
 - Removed leftover reset_control_array_put stub.

Changes since v5:
 - Fixed devm/of_reset_control_array_get stub return values in the
   "reset: hide reset control arrays behind struct reset_control" patch.
 - Merged "reset: hide reset control arrays behind struct reset_control" patch
   into "reset: Add APIs to manage array of resets" patch, to avoid adding
   new API functions in one patch that are removed in the other.
 - Updated commit message of "soc/tegra: pmc: Use the new reset APIs to manage
   reset controllers" patch.
 - Dropped already merged "reset: use kref for reference counting" patch.

Changes since v4:
 - Added a patch to hide reset control arrays behind struct reset_control
   and adapted the consumer patches. This could be merged with the reset
   array API patch if we think this is a good idea.

Changes since v3:
 - Squashed of_reset_control_get_count() patch in the second patch that
   adds the reset control array APIs.
 - The error path after getting count through of_reset_control_get_count()
   now returns NULL pointer in case when 'optional' flag is true.
 - Added code in reset_control_array_assert() to deassert the
   already asserted resets in the error case.
 - Using of_reset_control_array_get_optional_exclusive() in dwc3 patch
   to request the reset control array.
 - Added a patch to fix the order in which resources are handled in
   dwc3_of_simple_remove() path.
 - Added tegra_powergate->reset to take care of single reset control
   passed from the client drivers.

Changes since v2:
 - Addressed comments to make APIs inline with gpiod API.
 - Moved number of reset controls in 'struct reset_control_array'
   so that the footprint is reduced.
 - of_reset_control_array_get() and devm_reset_control_array_get()
   now return pointer to the newly created reset control array.
 - Added comments to mention that the reset control array APIs don't
   guarantee any particular order when handling the reset controls.
 - Dropped 'name' from reset_control_array' since the interface is meant
   for a bunch of anonymous resets that can all be asserted or deasserted
   in arbitrary order.
 - Fixed returns for APIs reported by kbuild.
 - Fixed 'for' clause guards reported by kbuild.

Changes since v1:
 - Addressed comment for error handling in of_reset_control_get_count()
 - Added patch to manage reset controller array.
 - Rebased dwc3-of-simple changes based on the new set of APIs
   for reset control array.
 - Added a patch for soc/tegra/pmc driver to use the new set of
   reset control array APIs.

Vivek Gautam (4):
  reset: Add APIs to manage array of resets
  usb: dwc3: of-simple: Re-order resource handling in remove
  usb: dwc3: of-simple: Add support to get resets for the device
  soc/tegra: pmc: Use the new reset APIs to manage reset controllers

 drivers/reset/core.c  | 211 +-
 drivers/soc/tegra/pmc.c   |  82 ---
 drivers/usb/dwc3/dwc3-of-simple.c |  29 +-
 include/linux/reset.h |  68 
 4 files changed, 324 insertions(+), 66 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Add support to get a list of resets available for the device.
These resets must be kept de-asserted until the device is
in use.

Cc: Felipe Balbi <ba...@kernel.org>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
No changes since v6.
---
 drivers/usb/dwc3/dwc3-of-simple.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index a9bac09d3750d..23d2221bde9df 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -29,11 +29,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct dwc3_of_simple {
struct device   *dev;
struct clk  **clks;
int num_clocks;
+   struct reset_control*resets;
 };
 
 static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
@@ -96,9 +98,20 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, simple);
simple->dev = dev;
 
+   simple->resets = of_reset_control_array_get_optional_exclusive(np);
+   if (IS_ERR(simple->resets)) {
+   ret = PTR_ERR(simple->resets);
+   dev_err(dev, "failed to get device resets, err=%d\n", ret);
+   return ret;
+   }
+
+   ret = reset_control_deassert(simple->resets);
+   if (ret)
+   goto err_resetc_put;
+
ret = dwc3_of_simple_clk_init(simple, of_clk_get_parent_count(np));
if (ret)
-   return ret;
+   goto err_resetc_assert;
 
ret = of_platform_populate(np, NULL, NULL, dev);
if (ret) {
@@ -107,7 +120,7 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
-   return ret;
+   goto err_resetc_assert;
}
 
pm_runtime_set_active(dev);
@@ -115,6 +128,13 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
pm_runtime_get_sync(dev);
 
return 0;
+
+err_resetc_assert:
+   reset_control_assert(simple->resets);
+
+err_resetc_put:
+   reset_control_put(simple->resets);
+   return ret;
 }
 
 static int dwc3_of_simple_remove(struct platform_device *pdev)
@@ -130,6 +150,9 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
+   reset_control_assert(simple->resets);
+   reset_control_put(simple->resets);
+
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 2/4] usb: dwc3: of-simple: Re-order resource handling in remove

[Philipp Zabel]

From: Vivek Gautam 

Move clock handling after of_platform_depopulate to achieve
a sequence that is reverse of the probe sequence.

Cc: Felipe Balbi 
Signed-off-by: Vivek Gautam 
---
No changes since v6.
---
 drivers/usb/dwc3/dwc3-of-simple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index fe414e7a9c78c..a9bac09d3750d 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -123,13 +123,13 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
struct device   *dev = >dev;
int i;
 
+   of_platform_depopulate(dev);
+
for (i = 0; i < simple->num_clocks; i++) {
clk_disable_unprepare(simple->clks[i]);
clk_put(simple->clks[i]);
}
 
-   of_platform_depopulate(dev);
-
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 1/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Many devices may want to request a bunch of resets and control them. So
it's better to manage them as an array. Add APIs to _get() an array of
reset_control, reusing the _assert(), _deassert(), and _reset() APIs for
single reset controls. Since reset controls already may control multiple
reset lines with a single hardware bit, from the user perspective, reset
control arrays are not at all different from single reset controls.
Note that these APIs don't guarantee that the reset lines managed in the
array are handled in any particular order.

Cc: Felipe Balbi <ba...@kernel.org>
Cc: Jon Hunter <jonath...@nvidia.com>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: changed API to hide reset control arrays behind
 struct reset_control]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
Changes since v6:
 - Removed leftover reset_control_array_put stub.
---
 drivers/reset/core.c  | 211 +-
 include/linux/reset.h |  68 
 2 files changed, 278 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 0090784ff4105..c8fb4426b218a 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -43,11 +43,24 @@ struct reset_control {
unsigned int id;
struct kref refcnt;
bool shared;
+   bool array;
atomic_t deassert_count;
atomic_t triggered_count;
 };
 
 /**
+ * struct reset_control_array - an array of reset controls
+ * @base: reset control for compatibility with reset control API functions
+ * @num_rstcs: number of reset controls
+ * @rstc: array of reset controls
+ */
+struct reset_control_array {
+   struct reset_control base;
+   unsigned int num_rstcs;
+   struct reset_control *rstc[];
+};
+
+/**
  * of_reset_simple_xlate - translate reset_spec to the reset line number
  * @rcdev: a pointer to the reset controller device
  * @reset_spec: reset line specifier as found in the device tree
@@ -135,6 +148,65 @@ int devm_reset_controller_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_reset_controller_register);
 
+static inline struct reset_control_array *
+rstc_to_array(struct reset_control *rstc) {
+   return container_of(rstc, struct reset_control_array, base);
+}
+
+static int reset_control_array_reset(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_reset(resets->rstc[i]);
+   if (ret)
+   return ret;
+   }
+
+   return 0;
+}
+
+static int reset_control_array_assert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_assert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_deassert(resets->rstc[i]);
+   return ret;
+}
+
+static int reset_control_array_deassert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_deassert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_assert(resets->rstc[i]);
+   return ret;
+}
+
+static inline bool reset_control_is_array(struct reset_control *rstc)
+{
+   return rstc->array;
+}
+
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
@@ -158,6 +230,9 @@ int reset_control_reset(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_reset(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->reset)
return -ENOTSUPP;
 
@@ -202,6 +277,9 @@ int reset_control_assert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_assert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->assert)
return -ENOTSUPP;
 
@@ -240,6 +318,9 @@ int reset_control_deassert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_deassert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->deassert)
return -ENOTSUPP;
 
@@ -266,7 +347,7 @@ int reset_control_status(struct reset_control *rstc)
if (!rstc)
return 0;
 
-   if (WARN_ON(IS_ERR(rstc)))
+   if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
  

[PATCH v7 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Make use of of_reset_control_array_get_exclusive() to manage
an array of reset controllers available with the device.

Cc: Jon Hunter <jonath...@nvidia.com>
Cc: Thierry Reding <tred...@nvidia.com>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
No changes since v6.
---
 drivers/soc/tegra/pmc.c | 82 -
 1 file changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e233dd5dcab3d..749b218147a19 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -124,8 +124,7 @@ struct tegra_powergate {
unsigned int id;
struct clk **clks;
unsigned int num_clks;
-   struct reset_control **resets;
-   unsigned int num_resets;
+   struct reset_control *reset;
 };
 
 struct tegra_io_pad_soc {
@@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
tegra_powergate *pg)
return err;
 }
 
-static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_assert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_assert(pg->reset);
 }
 
-static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_deassert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_deassert(pg->reset);
 }
 
 static int tegra_powergate_power_up(struct tegra_powergate *pg,
@@ -566,8 +547,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
struct clk *clk,
pg.id = id;
pg.clks = 
pg.num_clks = 1;
-   pg.resets = 
-   pg.num_resets = 1;
+   pg.reset = IS_ERR(rst) ? NULL : rst;
 
err = tegra_powergate_power_up(, false);
if (err)
@@ -755,45 +735,26 @@ static int tegra_powergate_of_get_clks(struct 
tegra_powergate *pg,
 static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
 struct device_node *np, bool off)
 {
-   struct reset_control *rst;
-   unsigned int i, count;
int err;
 
-   count = of_count_phandle_with_args(np, "resets", "#reset-cells");
-   if (count == 0)
-   return -ENODEV;
-
-   pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
-   if (!pg->resets)
-   return -ENOMEM;
-
-   for (i = 0; i < count; i++) {
-   pg->resets[i] = of_reset_control_get_by_index(np, i);
-   if (IS_ERR(pg->resets[i])) {
-   err = PTR_ERR(pg->resets[i]);
-   goto error;
-   }
-
-   if (off)
-   err = reset_control_assert(pg->resets[i]);
-   else
-   err = reset_control_deassert(pg->resets[i]);
-
-   if (err) {
-   reset_control_put(pg->resets[i]);
-   goto error;
-   }
+   pg->reset = of_reset_control_array_get_exclusive(np);
+   if (IS_ERR(pg->reset)) {
+   pr_err("failed to get device resets\n");
+   return PTR_ERR(pg->reset);
}
 
-   pg->num_resets = count;
+   if (off)
+   err = reset_control_assert(pg->reset);
+   else
+   err = reset_control_deassert(pg->reset);
 
-   return 0;
+   if (err)
+   goto put_reset;
 
-error:
-   while (i--)
-   reset_control_put(pg->resets[i]);
+   return 0;
 
-   kfree(pg->resets);
+put_reset:
+   reset_control_put(pg->reset);
 
return err;
 }
@@ -885,10 +846,7 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, 
struct device_node *np)
pm_genpd_remove(>genpd);
 
 remove_resets:
-   while (pg->num_resets--)
-   reset_control_put(pg->resets[pg->num_resets]);
-
-   kfree(pg->resets);
+   reset_control_put(pg->reset);
 
 remove_clks:
while (pg->num_clks--)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 086/102] usb: chipidea: msm: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Peter Chen <peter.c...@nxp.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c 
b/drivers/usb/chipidea/ci_hdrc_msm.c
index 0bdfcdcbf7a5a..0cee09192d946 100644
--- a/drivers/usb/chipidea/ci_hdrc_msm.c
+++ b/drivers/usb/chipidea/ci_hdrc_msm.c
@@ -197,7 +197,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
  CI_HDRC_OVERRIDE_PHY_CONTROL;
ci->pdata.notify_event = ci_hdrc_msm_notify_event;
 
-   reset = devm_reset_control_get(>dev, "core");
+   reset = devm_reset_control_get_exclusive(>dev, "core");
if (IS_ERR(reset))
return PTR_ERR(reset);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 092/102] usb: phy: qcom-8x16-usb: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Felipe Balbi <ba...@kernel.org>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/phy/phy-qcom-8x16-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-qcom-8x16-usb.c 
b/drivers/usb/phy/phy-qcom-8x16-usb.c
index b6a83a5cbad36..b9c31dc7bd026 100644
--- a/drivers/usb/phy/phy-qcom-8x16-usb.c
+++ b/drivers/usb/phy/phy-qcom-8x16-usb.c
@@ -232,7 +232,7 @@ static int phy_8x16_read_devicetree(struct phy_8x16 *qphy)
if (ret)
return ret;
 
-   qphy->phy_reset = devm_reset_control_get(dev, "phy");
+   qphy->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
if (IS_ERR(qphy->phy_reset))
return PTR_ERR(qphy->phy_reset);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 087/102] usb: dwc2: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: John Youn <johny...@synopsys.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/dwc2/platform.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index daf0d37acb37f..b9ec45b5f85d4 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -211,7 +211,8 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
 {
int i, ret;
 
-   hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
+   hsotg->reset = devm_reset_control_get_optional_exclusive(hsotg->dev,
+"dwc2");
if (IS_ERR(hsotg->reset)) {
ret = PTR_ERR(hsotg->reset);
dev_err(hsotg->dev, "error getting reset control %d\n", ret);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 089/102] usb: host: xhci-tegra: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Mathias Nyman <mathias.ny...@intel.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Thierry Reding <thierry.red...@gmail.com>
Cc: Jonathan Hunter <jonath...@nvidia.com>
Cc: linux-usb@vger.kernel.org
Cc: linux-te...@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/host/xhci-tegra.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 74436f8ca5382..ba53ee63d450c 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -933,14 +933,16 @@ static int tegra_xusb_probe(struct platform_device *pdev)
if (IS_ERR(tegra->padctl))
return PTR_ERR(tegra->padctl);
 
-   tegra->host_rst = devm_reset_control_get(>dev, "xusb_host");
+   tegra->host_rst = devm_reset_control_get_exclusive(>dev,
+  "xusb_host");
if (IS_ERR(tegra->host_rst)) {
err = PTR_ERR(tegra->host_rst);
dev_err(>dev, "failed to get xusb_host reset: %d\n", err);
goto put_padctl;
}
 
-   tegra->ss_rst = devm_reset_control_get(>dev, "xusb_ss");
+   tegra->ss_rst = devm_reset_control_get_exclusive(>dev,
+"xusb_ss");
if (IS_ERR(tegra->ss_rst)) {
err = PTR_ERR(tegra->ss_rst);
dev_err(>dev, "failed to get xusb_ss reset: %d\n", err);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 088/102] usb: host: ehci-tegra: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Alan Stern <st...@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Thierry Reding <thierry.red...@gmail.com>
Cc: Jonathan Hunter <jonath...@nvidia.com>
Cc: linux-usb@vger.kernel.org
Cc: linux-te...@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/host/ehci-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 9a3d7db5be57c..c60c43f66f313 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -94,7 +94,8 @@ static int tegra_reset_usb_controller(struct platform_device 
*pdev)
struct reset_control *usb1_reset;
 
if (!has_utmi_pad_registers)
-   usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+   usb1_reset = of_reset_control_get_exclusive(phy_np,
+   
"utmi-pads");
else
usb1_reset = tegra->rst;
 
@@ -450,7 +451,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
goto cleanup_hcd_create;
}
 
-   tegra->rst = devm_reset_control_get(>dev, "usb");
+   tegra->rst = devm_reset_control_get_exclusive(>dev, "usb");
if (IS_ERR(tegra->rst)) {
dev_err(>dev, "Can't get ehci reset\n");
err = PTR_ERR(tegra->rst);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 091/102] usb: phy: msm: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Felipe Balbi <ba...@kernel.org>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/phy/phy-msm-usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
index 8fb86a5f458e0..c4d93b391d281 100644
--- a/drivers/usb/phy/phy-msm-usb.c
+++ b/drivers/usb/phy/phy-msm-usb.c
@@ -1653,11 +1653,11 @@ static int msm_otg_read_dt(struct platform_device 
*pdev, struct msm_otg *motg)
if (!pdata->phy_type)
return 1;
 
-   motg->link_rst = devm_reset_control_get(>dev, "link");
+   motg->link_rst = devm_reset_control_get_exclusive(>dev, "link");
if (IS_ERR(motg->link_rst))
return PTR_ERR(motg->link_rst);
 
-   motg->phy_rst = devm_reset_control_get(>dev, "phy");
+   motg->phy_rst = devm_reset_control_get_exclusive(>dev, "phy");
if (IS_ERR(motg->phy_rst))
motg->phy_rst = NULL;
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 090/102] usb: musb: sunxi: explicitly request exclusive reset control

[Philipp Zabel]

Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
reset lines") started to transition the reset control request API calls
to explicitly state whether the driver needs exclusive or shared reset
control behavior. Convert all drivers requesting exclusive resets to the
explicit API call so the temporary transition helpers can be removed.

No functional changes.

Cc: Bin Liu <b-...@ti.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Maxime Ripard <maxime.rip...@free-electrons.com>
Cc: Chen-Yu Tsai <w...@csie.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/musb/sunxi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index c9a09b5bb6e59..7eec1acbc729a 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -747,7 +747,7 @@ static int sunxi_musb_probe(struct platform_device *pdev)
}
 
if (test_bit(SUNXI_MUSB_FL_HAS_RESET, >flags)) {
-   glue->rst = devm_reset_control_get(>dev, NULL);
+   glue->rst = devm_reset_control_get_exclusive(>dev, NULL);
if (IS_ERR(glue->rst)) {
if (PTR_ERR(glue->rst) == -EPROBE_DEFER)
return -EPROBE_DEFER;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

On Tue, 2017-06-20 at 04:21 +0800, kbuild test robot wrote:
> Hi Vivek,
> 
> [auto build test WARNING on pza/reset/next]
> [also build test WARNING on v4.12-rc6 next-20170619]
> [cannot apply to balbi-usb/next]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Philipp-Zabel/reset-Add-APIs-to-manage-array-of-resets/20170620-021320
> base:   git://git.pengutronix.de/git/pza/linux reset/next
> config: i386-randconfig-c0-06200218 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All warnings (new ones prefixed by >>):
> 
>In file included from drivers/gpu//drm/nouveau/include/nvif/os.h:28:0,
> from drivers/gpu//drm/nouveau/include/nvkm/core/os.h:3,
> from drivers/gpu//drm/nouveau/include/nvkm/core/event.h:3,
> from 
> drivers/gpu//drm/nouveau/include/nvkm/core/device.h:3,
> from 
> drivers/gpu//drm/nouveau/include/nvkm/core/subdev.h:3,
> from 
> drivers/gpu//drm/nouveau/include/nvkm/core/engine.h:4,
> from 
> drivers/gpu//drm/nouveau/include/nvkm/engine/fifo.h:3,
> from drivers/gpu//drm/nouveau/nvkm/engine/fifo/priv.h:4,
> from drivers/gpu//drm/nouveau/nvkm/engine/fifo/chan.h:4,
> from 
> drivers/gpu//drm/nouveau/nvkm/engine/fifo/channv50.h:4,
> from 
> drivers/gpu//drm/nouveau/nvkm/engine/fifo/chang84.c:24:
> >> include/linux/reset.h:110:37: warning: 'struct reset_control_array' 
> >> declared inside parameter list
> void reset_control_array_put(struct reset_control_array *resets)
> ^
> >> include/linux/reset.h:110:37: warning: its scope is only this definition 
> >> or declaration, which is probably not what you want
> 
> vim +110 include/linux/reset.h
> 
> 94return optional ? NULL : ERR_PTR(-ENOTSUPP);
> 95}
> 96
> 97static inline struct reset_control *
> 98devm_reset_control_array_get(struct device *dev, bool shared, 
> bool optional)
> 99{
>100return optional ? NULL : ERR_PTR(-ENOTSUPP);
>101}
>102
>103static inline struct reset_control *
>104of_reset_control_array_get(struct device_node *np, bool shared, 
> bool optional)
>105{
>106return optional ? NULL : ERR_PTR(-ENOTSUPP);
>107}
>108
>109static inline
>  > 110void reset_control_array_put(struct reset_control_array *resets)
>111{
>112}

reset_control_array_put is static now, I forgot to remove this stub.

regards
  * Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 0/4] reset: APIs to manage a list of resets

[Philipp Zabel]

A set of patches to allow consumers to get and de/assert or trigger
a number of resets at the same time. A patch on top of Vivek's original
API extension is added to hide the reset_control_array behind a struct
reset_control so that the consumer doesn't have to care about the difference
between a singular reset control and a reset control controlling an array
of resets, except when requesting it.

This series also contains reset controls patches for dwc3-of-simple
and tegra pmc drivers.
A small patch is added in this series to correctly re-order the
resource handling in dwc3_of_simple_remove().

The series is tested on torvald's master branch the device tree
patches to enable usb on db820c.

Changes since v5:
 - Fixed devm/of_reset_control_array_get stub return values in the
   "reset: hide reset control arrays behind struct reset_control" patch.
 - Merged "reset: hide reset control arrays behind struct reset_control" patch
   into "reset: Add APIs to manage array of resets" patch, to avoid adding
   new API functions in one patch that are removed in the other.
 - Updated commit message of "soc/tegra: pmc: Use the new reset APIs to manage
   reset controllers" patch.
 - Dropped already merged "reset: use kref for reference counting" patch.

Changes since v4:
 - Added a patch to hide reset control arrays behind struct reset_control
   and adapted the consumer patches. This could be merged with the reset
   array API patch if we think this is a good idea.

Changes since v3:
 - Squashed of_reset_control_get_count() patch in the second patch that
   adds the reset control array APIs.
 - The error path after getting count through of_reset_control_get_count()
   now returns NULL pointer in case when 'optional' flag is true.
 - Added code in reset_control_array_assert() to deassert the
   already asserted resets in the error case.
 - Using of_reset_control_array_get_optional_exclusive() in dwc3 patch
   to request the reset control array.
 - Added a patch to fix the order in which resources are handled in
   dwc3_of_simple_remove() path.
 - Added tegra_powergate->reset to take care of single reset control
   passed from the client drivers.

Changes since v2:
 - Addressed comments to make APIs inline with gpiod API.
 - Moved number of reset controls in 'struct reset_control_array'
   so that the footprint is reduced.
 - of_reset_control_array_get() and devm_reset_control_array_get()
   now return pointer to the newly created reset control array.
 - Added comments to mention that the reset control array APIs don't
   guarantee any particular order when handling the reset controls.
 - Dropped 'name' from reset_control_array' since the interface is meant
   for a bunch of anonymous resets that can all be asserted or deasserted
   in arbitrary order.
 - Fixed returns for APIs reported by kbuild.
 - Fixed 'for' clause guards reported by kbuild.

Changes since v1:
 - Addressed comment for error handling in of_reset_control_get_count()
 - Added patch to manage reset controller array.
 - Rebased dwc3-of-simple changes based on the new set of APIs
   for reset control array.
 - Added a patch for soc/tegra/pmc driver to use the new set of
   reset control array APIs.

Vivek Gautam (4):
  reset: Add APIs to manage array of resets
  usb: dwc3: of-simple: Re-order resource handling in remove
  usb: dwc3: of-simple: Add support to get resets for the device
  soc/tegra: pmc: Use the new reset APIs to manage reset controllers

 drivers/reset/core.c  | 211 +-
 drivers/soc/tegra/pmc.c   |  82 ---
 drivers/usb/dwc3/dwc3-of-simple.c |  29 +-
 include/linux/reset.h |  73 +
 4 files changed, 329 insertions(+), 66 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 1/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Many devices may want to request a bunch of resets and control them. So
it's better to manage them as an array. Add APIs to _get() an array of
reset_control, reusing the _assert(), _deassert(), and _reset() APIs for
single reset controls. Since reset controls already may control multiple
reset lines with a single hardware bit, from the user perspective, reset
control arrays are not at all different from single reset controls.
Note that these APIs don't guarantee that the reset lines managed in the
array are handled in any particular order.

Cc: Felipe Balbi <ba...@kernel.org>
Cc: Jon Hunter <jonath...@nvidia.com>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: changed API to hide reset control arrays behind
 struct reset_control]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
Changes since v5:
 - Merged "reset: hide reset control arrays behind struct reset_control" patch
   into this one, fixed devm/of_reset_control_array_get stub return values in
   the new patch.
---
 drivers/reset/core.c  | 211 +-
 include/linux/reset.h |  73 +
 2 files changed, 283 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 0090784ff4105..c8fb4426b218a 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -43,11 +43,24 @@ struct reset_control {
unsigned int id;
struct kref refcnt;
bool shared;
+   bool array;
atomic_t deassert_count;
atomic_t triggered_count;
 };
 
 /**
+ * struct reset_control_array - an array of reset controls
+ * @base: reset control for compatibility with reset control API functions
+ * @num_rstcs: number of reset controls
+ * @rstc: array of reset controls
+ */
+struct reset_control_array {
+   struct reset_control base;
+   unsigned int num_rstcs;
+   struct reset_control *rstc[];
+};
+
+/**
  * of_reset_simple_xlate - translate reset_spec to the reset line number
  * @rcdev: a pointer to the reset controller device
  * @reset_spec: reset line specifier as found in the device tree
@@ -135,6 +148,65 @@ int devm_reset_controller_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_reset_controller_register);
 
+static inline struct reset_control_array *
+rstc_to_array(struct reset_control *rstc) {
+   return container_of(rstc, struct reset_control_array, base);
+}
+
+static int reset_control_array_reset(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_reset(resets->rstc[i]);
+   if (ret)
+   return ret;
+   }
+
+   return 0;
+}
+
+static int reset_control_array_assert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_assert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_deassert(resets->rstc[i]);
+   return ret;
+}
+
+static int reset_control_array_deassert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_deassert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_assert(resets->rstc[i]);
+   return ret;
+}
+
+static inline bool reset_control_is_array(struct reset_control *rstc)
+{
+   return rstc->array;
+}
+
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
@@ -158,6 +230,9 @@ int reset_control_reset(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_reset(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->reset)
return -ENOTSUPP;
 
@@ -202,6 +277,9 @@ int reset_control_assert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_assert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->assert)
return -ENOTSUPP;
 
@@ -240,6 +318,9 @@ int reset_control_deassert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_deassert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->deassert)
return -ENOTSUPP;
 
@@ -266,7 +347,7 @@ int reset_control_status(struct reset_control *rstc)
if

[PATCH v6 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Add support to get a list of resets available for the device.
These resets must be kept de-asserted until the device is
in use.

Cc: Felipe Balbi <ba...@kernel.org>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
No changes since v5.
---
 drivers/usb/dwc3/dwc3-of-simple.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index a9bac09d3750d..23d2221bde9df 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -29,11 +29,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct dwc3_of_simple {
struct device   *dev;
struct clk  **clks;
int num_clocks;
+   struct reset_control*resets;
 };
 
 static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
@@ -96,9 +98,20 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, simple);
simple->dev = dev;
 
+   simple->resets = of_reset_control_array_get_optional_exclusive(np);
+   if (IS_ERR(simple->resets)) {
+   ret = PTR_ERR(simple->resets);
+   dev_err(dev, "failed to get device resets, err=%d\n", ret);
+   return ret;
+   }
+
+   ret = reset_control_deassert(simple->resets);
+   if (ret)
+   goto err_resetc_put;
+
ret = dwc3_of_simple_clk_init(simple, of_clk_get_parent_count(np));
if (ret)
-   return ret;
+   goto err_resetc_assert;
 
ret = of_platform_populate(np, NULL, NULL, dev);
if (ret) {
@@ -107,7 +120,7 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
-   return ret;
+   goto err_resetc_assert;
}
 
pm_runtime_set_active(dev);
@@ -115,6 +128,13 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
pm_runtime_get_sync(dev);
 
return 0;
+
+err_resetc_assert:
+   reset_control_assert(simple->resets);
+
+err_resetc_put:
+   reset_control_put(simple->resets);
+   return ret;
 }
 
 static int dwc3_of_simple_remove(struct platform_device *pdev)
@@ -130,6 +150,9 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
+   reset_control_assert(simple->resets);
+   reset_control_put(simple->resets);
+
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Make use of of_reset_control_array_get_exclusive() to manage
an array of reset controllers available with the device.

Cc: Jon Hunter <jonath...@nvidia.com>
Cc: Thierry Reding <tred...@nvidia.com>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
Changes since v5: updated commit message, added missing SoB
---
 drivers/soc/tegra/pmc.c | 82 -
 1 file changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e233dd5dcab3d..749b218147a19 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -124,8 +124,7 @@ struct tegra_powergate {
unsigned int id;
struct clk **clks;
unsigned int num_clks;
-   struct reset_control **resets;
-   unsigned int num_resets;
+   struct reset_control *reset;
 };
 
 struct tegra_io_pad_soc {
@@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
tegra_powergate *pg)
return err;
 }
 
-static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_assert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_assert(pg->reset);
 }
 
-static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_deassert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_deassert(pg->reset);
 }
 
 static int tegra_powergate_power_up(struct tegra_powergate *pg,
@@ -566,8 +547,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
struct clk *clk,
pg.id = id;
pg.clks = 
pg.num_clks = 1;
-   pg.resets = 
-   pg.num_resets = 1;
+   pg.reset = IS_ERR(rst) ? NULL : rst;
 
err = tegra_powergate_power_up(, false);
if (err)
@@ -755,45 +735,26 @@ static int tegra_powergate_of_get_clks(struct 
tegra_powergate *pg,
 static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
 struct device_node *np, bool off)
 {
-   struct reset_control *rst;
-   unsigned int i, count;
int err;
 
-   count = of_count_phandle_with_args(np, "resets", "#reset-cells");
-   if (count == 0)
-   return -ENODEV;
-
-   pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
-   if (!pg->resets)
-   return -ENOMEM;
-
-   for (i = 0; i < count; i++) {
-   pg->resets[i] = of_reset_control_get_by_index(np, i);
-   if (IS_ERR(pg->resets[i])) {
-   err = PTR_ERR(pg->resets[i]);
-   goto error;
-   }
-
-   if (off)
-   err = reset_control_assert(pg->resets[i]);
-   else
-   err = reset_control_deassert(pg->resets[i]);
-
-   if (err) {
-   reset_control_put(pg->resets[i]);
-   goto error;
-   }
+   pg->reset = of_reset_control_array_get_exclusive(np);
+   if (IS_ERR(pg->reset)) {
+   pr_err("failed to get device resets\n");
+   return PTR_ERR(pg->reset);
}
 
-   pg->num_resets = count;
+   if (off)
+   err = reset_control_assert(pg->reset);
+   else
+   err = reset_control_deassert(pg->reset);
 
-   return 0;
+   if (err)
+   goto put_reset;
 
-error:
-   while (i--)
-   reset_control_put(pg->resets[i]);
+   return 0;
 
-   kfree(pg->resets);
+put_reset:
+   reset_control_put(pg->reset);
 
return err;
 }
@@ -885,10 +846,7 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, 
struct device_node *np)
pm_genpd_remove(>genpd);
 
 remove_resets:
-   while (pg->num_resets--)
-   reset_control_put(pg->resets[pg->num_resets]);
-
-   kfree(pg->resets);
+   reset_control_put(pg->reset);
 
 remove_clks:
while (pg->num_clks--)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/4] usb: dwc3: of-simple: Re-order resource handling in remove

[Philipp Zabel]

From: Vivek Gautam 

Move clock handling after of_platform_depopulate to achieve
a sequence that is reverse of the probe sequence.

Cc: Felipe Balbi 
Signed-off-by: Vivek Gautam 
---
No changes since v5.
---
 drivers/usb/dwc3/dwc3-of-simple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index fe414e7a9c78c..a9bac09d3750d 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -123,13 +123,13 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
struct device   *dev = >dev;
int i;
 
+   of_platform_depopulate(dev);
+
for (i = 0; i < simple->num_clocks; i++) {
clk_disable_unprepare(simple->clks[i]);
clk_put(simple->clks[i]);
}
 
-   of_platform_depopulate(dev);
-
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/6] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Thu, 2017-06-01 at 18:52 +0200, Philipp Zabel wrote:
> From: Vivek Gautam <vivek.gau...@codeaurora.org>
> 
> Make use of reset_control_array_*() set of APIs to manage
> an array of reset controllers available with the device.
> 
> Cc: Jon Hunter <jonath...@nvidia.com>
> Cc: Thierry Reding <tred...@nvidia.com>
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
>  drivers/soc/tegra/pmc.c | 82 
> -
>  1 file changed, 20 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e233dd5dcab3d..749b218147a19 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -124,8 +124,7 @@ struct tegra_powergate {
>   unsigned int id;
>   struct clk **clks;
>   unsigned int num_clks;
> - struct reset_control **resets;
> - unsigned int num_resets;
> + struct reset_control *reset;

Jon, do you think this changed API is a better fit for the tegra/pmc use
case? The reset control array is now handled behind the scenes. Except
for the initial of_reset_control_array_get_exclusive() call, there is no
difference to the user between single reset controls and reset control
arrays anymore.

regards
Philipp

>  };
>  
>  struct tegra_io_pad_soc {
> @@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
> tegra_powergate *pg)
>   return err;
>  }
>  
> -static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_assert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_assert(pg->reset);
>  }
>  
> -static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_deassert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_deassert(pg->reset);
>  }
>  
>  static int tegra_powergate_power_up(struct tegra_powergate *pg,
> @@ -566,8 +547,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
> struct clk *clk,
>   pg.id = id;
>   pg.clks = 
>   pg.num_clks = 1;
> - pg.resets = 
> - pg.num_resets = 1;
> + pg.reset = IS_ERR(rst) ? NULL : rst;
>  
>   err = tegra_powergate_power_up(, false);
>   if (err)
> @@ -755,45 +735,26 @@ static int tegra_powergate_of_get_clks(struct 
> tegra_powergate *pg,
>  static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
>struct device_node *np, bool off)
>  {
> - struct reset_control *rst;
> - unsigned int i, count;
>   int err;
>  
> - count = of_count_phandle_with_args(np, "resets", "#reset-cells");
> - if (count == 0)
> - return -ENODEV;
> -
> - pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
> - if (!pg->resets)
> - return -ENOMEM;
> -
> - for (i = 0; i < count; i++) {
> - pg->resets[i] = of_reset_control_get_by_index(np, i);
> - if (IS_ERR(pg->resets[i])) {
> - err = PTR_ERR(pg->resets[i]);
> - goto error;
> - }
> -
> - if (off)
> - err = reset_control_assert(pg->resets[i]);
> - else
> - err = reset_control_deassert(pg->resets[i]);
> -
> - if (err) {
> - reset_control_put(pg->resets[i]);
> - goto error;
> - }
> + pg->reset = of_reset_control_array_get_exclusive(np);
> + if (IS_ERR(pg->reset)) {
> + pr_err("failed to get device resets\n");
> + return PTR_ERR(pg->reset);
>   }
>  
> - pg->num_resets = count;
> + if (off)
> + err = reset_control_assert(pg->reset);
> + else
> + err = reset_control_deassert(pg->reset);
>  
> - return 0;
> + if (err)
> + goto put_reset;
>  
> -error:
> - while (i--)
> - reset_control_put(pg

Re: [PATCH v5 3/6] reset: hide reset control arrays behind struct reset_control

[Philipp Zabel]

Hi Vivek,

On Tue, 2017-06-13 at 12:16 +0530, Vivek Gautam wrote:
[...]
> > @@ -102,18 +94,6 @@ static inline struct reset_control 
> > *__devm_reset_control_get(
> > return optional ? NULL : ERR_PTR(-ENOTSUPP);
> >   }
> >   
> > -static inline
> > -int reset_control_array_assert(struct reset_control_array *resets)
> > -{
> > -   return 0;
> > -}
> > -
> > -static inline
> > -int reset_control_array_deassert(struct reset_control_array *resets)
> > -{
> > -   return 0;
> > -}
> > -
> >   static inline struct reset_control_array *
> 
> This has to return just 'struct reset_control *'.
> That should resolve kbuild errors.
>
> Rest looks good to me.
> Reviewed-by: Vivek Gautam 

Thanks, I had already fixed this locally, prompted by the kbuild test
robot. I'll send a v6. I would like to merge this patch into the first
"reset: Add APIs to manage array of resets" patch, if that's ok with
you.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] HID: usbhid: Add HID_QUIRK_NO_INIT_REPORTS for Oculus Rift CV1

[Philipp Zabel]

Hi Benjamin,

On Wed, Jun 7, 2017 at 9:11 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> Hi Philip,
>
> On Jun 07 2017 or thereabouts, Philipp Zabel wrote:
[...]
> 2 things:
> - the 2 patches should be squashed together. There is no point having a
>   single patch for just 2 defines not used anywhere else.
> - I believe you don't even need this quirk in the development version of
>   the hid tree. Would you mind trying a plain v4.12-rcX and report if
>   you still need the quirk?

Thanks, you are right. I just checked on v4.12-rc4 and now it works without
the quirk. Sorry for the noise.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] HID: usbhid: Add HID_QUIRK_NO_INIT_REPORTS for Oculus Rift CV1

[Philipp Zabel]

When plugging in an Oculus Rift CV1 HMD, it takes a long time until the hidraw
devices appear, specifically two control transfers time out querying the HID
report descriptors:

$ echo 1 > /sys/module/hid/parameters/debug

usb 1-3.1: new full-speed USB device number 11 using xhci_hcd
usb 1-3.1: New USB device found, idVendor=2833, idProduct=0031
usb 1-3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-3.1: Product: Rift
usb 1-3.1: Manufacturer: Oculus VR, Inc.
drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 0
hid-generic 0003:2833:0031.0005: Kicking head 1 tail 0
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x0101 wIndex=0x wLength=62
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x010b wIndex=0x wLength=64

[10 s delay]

drivers/hid/usbhid/hid-core.c: timeout waiting for ctrl or out queue to 
clear
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x010c wIndex=0x wLength=64
hid-generic 0003:2833:0031.0005: usb_submit_urb(ctrl) failed: -1
hid-generic 0003:2833:0031.0005: timeout initializing reports
hid-generic 0003:2833:0031.0005: hiddev0,hidraw0: USB HID v1.10 Device 
[Oculus VR, Inc. Rift] on usb-:00:14.0-3.1/input0
drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 1
hid-generic 0003:2833:0031.0006: Kicking head 1 tail 0
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x0101 wIndex=0x0001 wLength=62
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x010b wIndex=0x0001 wLength=64

[10 s delay]

drivers/hid/usbhid/hid-core.c: timeout waiting for ctrl or out queue to 
clear
drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report 
wValue=0x010c wIndex=0x0001 wLength=64
hid-generic 0003:2833:0031.0006: usb_submit_urb(ctrl) failed: -1
hid-generic 0003:2833:0031.0006: timeout initializing reports
hid-generic 0003:2833:0031.0006: hiddev0,hidraw1: USB HID v1.10 Device 
[Oculus VR, Inc. Rift] on usb-:00:14.0-3.1/input1
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver

These timeouts disappear when setting HID_QUIRK_NO_INIT_REPORTS. The hidraw
devices appear quickly and are functional.

Signed-off-by: Philipp Zabel <philipp.za...@gmail.com>
---
 drivers/hid/usbhid/hid-quirks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 6316498b7812..7b2df4042167 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -111,6 +111,7 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL, 
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, 
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE, 
HID_QUIRK_NO_INIT_REPORTS },
+   { USB_VENDOR_ID_OCULUSVR, USB_DEVICE_ID_RIFT_CV1, 
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_1610, HID_QUIRK_NOGET 
},
{ USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_1640, HID_QUIRK_NOGET 
},
{ USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_USB_OPTICAL_MOUSE, 
HID_QUIRK_ALWAYS_POLL },
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] HID: Add Oculus Rift CV1 id

[Philipp Zabel]

Add VID/PID for Oculus Rift CV1.

Signed-off-by: Philipp Zabel <philipp.za...@gmail.com>
---
 drivers/hid/hid-ids.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 8ca1e8ce0af2..2953d53a8cc9 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1133,6 +1133,9 @@
 #define USB_VENDOR_ID_MULTIPLE_17810x1781
 #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD0x0a9d
 
+#define USB_VENDOR_ID_OCULUSVR 0x2833
+#define USB_DEVICE_ID_RIFT_CV1 0x0031
+
 #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
 #define USB_DEVICE_ID_RAPHNET_2NES2SNES0x0002
 #define USB_DEVICE_ID_RAPHNET_4NES4SNES0x0003
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/6] usb: dwc3: of-simple: Re-order resource handling in remove

[Philipp Zabel]

From: Vivek Gautam 

Move clock handling after of_platform_depopulate to achieve
a sequence that is reverse of the probe sequence.

Cc: Felipe Balbi 
Signed-off-by: Vivek Gautam 
---
 drivers/usb/dwc3/dwc3-of-simple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index fe414e7a9c78c..a9bac09d3750d 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -123,13 +123,13 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
struct device   *dev = >dev;
int i;
 
+   of_platform_depopulate(dev);
+
for (i = 0; i < simple->num_clocks; i++) {
clk_disable_unprepare(simple->clks[i]);
clk_put(simple->clks[i]);
}
 
-   of_platform_depopulate(dev);
-
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/6] reset: Add APIs to manage array of resets

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Many devices may want to request a bunch of resets
and control them. So it's better to manage them as an
array. Add APIs to _get(), _assert(), and _deassert()
an array of reset_control.
Note that, these APIs don't guarantee that the reset lines
managed in the array are handled in any particular order.

Cc: Felipe Balbi <ba...@kernel.org>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
Acked-by: Jon Hunter <jonath...@nvidia.com>
Tested-by: Jon Hunter <jonath...@nvidia.com>
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/reset/core.c  | 202 ++
 include/linux/reset.h |  93 +++
 2 files changed, 295 insertions(+)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 0090784ff4105..1747000757211 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -472,3 +472,205 @@ int device_reset(struct device *dev)
return ret;
 }
 EXPORT_SYMBOL_GPL(device_reset);
+
+/**
+ * APIs to manage an array of reset controls.
+ */
+/**
+ * of_reset_control_get_count - Count number of resets available with a device
+ *
+ * @node: device node that contains 'resets'.
+ *
+ * Returns positive reset count on success, or error number on failure and
+ * on count being zero.
+ */
+static int of_reset_control_get_count(struct device_node *node)
+{
+   int count;
+
+   if (!node)
+   return -EINVAL;
+
+   count = of_count_phandle_with_args(node, "resets", "#reset-cells");
+   if (count == 0)
+   count = -ENOENT;
+
+   return count;
+}
+
+/**
+ * reset_control_array_assert: assert a list of resets
+ *
+ * @resets: reset control array holding info about the list of resets
+ *
+ * This API doesn't guarantee that the reset lines controlled by
+ * the reset array are asserted in any particular order.
+ *
+ * Returns 0 on success or error number on failure.
+ */
+int reset_control_array_assert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   if (!resets)
+   return 0;
+
+   if (IS_ERR(resets))
+   return -EINVAL;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_assert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_deassert(resets->rstc[i]);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_array_assert);
+
+/**
+ * reset_control_array_deassert: deassert a list of resets
+ *
+ * @resets: reset control array holding info about the list of resets
+ *
+ * This API doesn't guarantee that the reset lines controlled by
+ * the reset array are deasserted in any particular order.
+ *
+ * Returns 0 on success or error number on failure.
+ */
+int reset_control_array_deassert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   if (!resets)
+   return 0;
+
+   if (IS_ERR(resets))
+   return -EINVAL;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_deassert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_assert(resets->rstc[i]);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_array_deassert);
+
+static void devm_reset_control_array_release(struct device *dev, void *res)
+{
+   reset_control_array_put(*(struct reset_control_array **)res);
+}
+
+/**
+ * of_reset_control_array_get - Get a list of reset controls using
+ * device node.
+ *
+ * @np: device node for the device that requests the reset controls array
+ * @shared: whether reset controls are shared or not
+ * @optional: whether it is optional to get the reset controls
+ *
+ * Returns pointer to allocated reset_control_array on success or
+ * error on failure
+ */
+struct reset_control_array *
+of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
+{
+   struct reset_control_array *resets;
+   struct reset_control *rstc;
+   int num, i;
+   void *err;
+
+   num = of_reset_control_get_count(np);
+   if (num < 0)
+   return optional ? NULL : ERR_PTR(num);
+
+   resets = kzalloc(sizeof(*resets) + sizeof(resets->rstc[0]) * num,
+GFP_KERNEL);
+   if (!resets)
+   return ERR_PTR(-ENOMEM);
+
+   for (i = 0; i < num; i++) {
+   rstc = __of_reset_control_get(np, NULL, i, shared, optional);
+   if (IS_ERR(rstc)) {
+   err = ERR_CAST(rstc);
+   goto err_rst;
+   }
+   resets->rstc[i] = rstc;
+   }
+   resets->num_rstcs = num;
+
+   return resets;
+
+err_rs

[PATCH v5 6/6] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Make use of reset_control_array_*() set of APIs to manage
an array of reset controllers available with the device.

Cc: Jon Hunter <jonath...@nvidia.com>
Cc: Thierry Reding <tred...@nvidia.com>
Cc: Philipp Zabel <p.za...@pengutronix.de>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
---
 drivers/soc/tegra/pmc.c | 82 -
 1 file changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e233dd5dcab3d..749b218147a19 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -124,8 +124,7 @@ struct tegra_powergate {
unsigned int id;
struct clk **clks;
unsigned int num_clks;
-   struct reset_control **resets;
-   unsigned int num_resets;
+   struct reset_control *reset;
 };
 
 struct tegra_io_pad_soc {
@@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
tegra_powergate *pg)
return err;
 }
 
-static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_assert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_assert(pg->reset);
 }
 
-static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
 {
-   unsigned int i;
-   int err;
-
-   for (i = 0; i < pg->num_resets; i++) {
-   err = reset_control_deassert(pg->resets[i]);
-   if (err)
-   return err;
-   }
-
-   return 0;
+   return reset_control_deassert(pg->reset);
 }
 
 static int tegra_powergate_power_up(struct tegra_powergate *pg,
@@ -566,8 +547,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
struct clk *clk,
pg.id = id;
pg.clks = 
pg.num_clks = 1;
-   pg.resets = 
-   pg.num_resets = 1;
+   pg.reset = IS_ERR(rst) ? NULL : rst;
 
err = tegra_powergate_power_up(, false);
if (err)
@@ -755,45 +735,26 @@ static int tegra_powergate_of_get_clks(struct 
tegra_powergate *pg,
 static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
 struct device_node *np, bool off)
 {
-   struct reset_control *rst;
-   unsigned int i, count;
int err;
 
-   count = of_count_phandle_with_args(np, "resets", "#reset-cells");
-   if (count == 0)
-   return -ENODEV;
-
-   pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
-   if (!pg->resets)
-   return -ENOMEM;
-
-   for (i = 0; i < count; i++) {
-   pg->resets[i] = of_reset_control_get_by_index(np, i);
-   if (IS_ERR(pg->resets[i])) {
-   err = PTR_ERR(pg->resets[i]);
-   goto error;
-   }
-
-   if (off)
-   err = reset_control_assert(pg->resets[i]);
-   else
-   err = reset_control_deassert(pg->resets[i]);
-
-   if (err) {
-   reset_control_put(pg->resets[i]);
-   goto error;
-   }
+   pg->reset = of_reset_control_array_get_exclusive(np);
+   if (IS_ERR(pg->reset)) {
+   pr_err("failed to get device resets\n");
+   return PTR_ERR(pg->reset);
}
 
-   pg->num_resets = count;
+   if (off)
+   err = reset_control_assert(pg->reset);
+   else
+   err = reset_control_deassert(pg->reset);
 
-   return 0;
+   if (err)
+   goto put_reset;
 
-error:
-   while (i--)
-   reset_control_put(pg->resets[i]);
+   return 0;
 
-   kfree(pg->resets);
+put_reset:
+   reset_control_put(pg->reset);
 
return err;
 }
@@ -885,10 +846,7 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, 
struct device_node *np)
pm_genpd_remove(>genpd);
 
 remove_resets:
-   while (pg->num_resets--)
-   reset_control_put(pg->resets[pg->num_resets]);
-
-   kfree(pg->resets);
+   reset_control_put(pg->reset);
 
 remove_clks:
while (pg->num_clks--)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 5/6] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

From: Vivek Gautam <vivek.gau...@codeaurora.org>

Add support to get a list of resets available for the device.
These resets must be kept de-asserted until the device is
in use.

Cc: Felipe Balbi <ba...@kernel.org>
Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
[p.za...@pengutronix.de: switch to hidden reset control array]
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index a9bac09d3750d..23d2221bde9df 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -29,11 +29,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct dwc3_of_simple {
struct device   *dev;
struct clk  **clks;
int num_clocks;
+   struct reset_control*resets;
 };
 
 static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
@@ -96,9 +98,20 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, simple);
simple->dev = dev;
 
+   simple->resets = of_reset_control_array_get_optional_exclusive(np);
+   if (IS_ERR(simple->resets)) {
+   ret = PTR_ERR(simple->resets);
+   dev_err(dev, "failed to get device resets, err=%d\n", ret);
+   return ret;
+   }
+
+   ret = reset_control_deassert(simple->resets);
+   if (ret)
+   goto err_resetc_put;
+
ret = dwc3_of_simple_clk_init(simple, of_clk_get_parent_count(np));
if (ret)
-   return ret;
+   goto err_resetc_assert;
 
ret = of_platform_populate(np, NULL, NULL, dev);
if (ret) {
@@ -107,7 +120,7 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
-   return ret;
+   goto err_resetc_assert;
}
 
pm_runtime_set_active(dev);
@@ -115,6 +128,13 @@ static int dwc3_of_simple_probe(struct platform_device 
*pdev)
pm_runtime_get_sync(dev);
 
return 0;
+
+err_resetc_assert:
+   reset_control_assert(simple->resets);
+
+err_resetc_put:
+   reset_control_put(simple->resets);
+   return ret;
 }
 
 static int dwc3_of_simple_remove(struct platform_device *pdev)
@@ -130,6 +150,9 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
clk_put(simple->clks[i]);
}
 
+   reset_control_assert(simple->resets);
+   reset_control_put(simple->resets);
+
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 3/6] reset: hide reset control arrays behind struct reset_control

[Philipp Zabel]

Reset controls already may control multiple reset lines with a single
hardware bit. So from the user perspective, reset control arrays are not
at all different from single reset controls.
Therefore, hide reset control arrays behind struct reset_control to
avoid having to introduce new API functions for array (de)assert/reset.

Cc: Vivek Gautam <vivek.gau...@codeaurora.org>
Cc: Jon Hunter <jonath...@nvidia.com>
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/reset/core.c  | 225 ++
 include/linux/reset.h |  44 +++---
 2 files changed, 128 insertions(+), 141 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 1747000757211..c8fb4426b218a 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -43,11 +43,24 @@ struct reset_control {
unsigned int id;
struct kref refcnt;
bool shared;
+   bool array;
atomic_t deassert_count;
atomic_t triggered_count;
 };
 
 /**
+ * struct reset_control_array - an array of reset controls
+ * @base: reset control for compatibility with reset control API functions
+ * @num_rstcs: number of reset controls
+ * @rstc: array of reset controls
+ */
+struct reset_control_array {
+   struct reset_control base;
+   unsigned int num_rstcs;
+   struct reset_control *rstc[];
+};
+
+/**
  * of_reset_simple_xlate - translate reset_spec to the reset line number
  * @rcdev: a pointer to the reset controller device
  * @reset_spec: reset line specifier as found in the device tree
@@ -135,6 +148,65 @@ int devm_reset_controller_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_reset_controller_register);
 
+static inline struct reset_control_array *
+rstc_to_array(struct reset_control *rstc) {
+   return container_of(rstc, struct reset_control_array, base);
+}
+
+static int reset_control_array_reset(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_reset(resets->rstc[i]);
+   if (ret)
+   return ret;
+   }
+
+   return 0;
+}
+
+static int reset_control_array_assert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_assert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_deassert(resets->rstc[i]);
+   return ret;
+}
+
+static int reset_control_array_deassert(struct reset_control_array *resets)
+{
+   int ret, i;
+
+   for (i = 0; i < resets->num_rstcs; i++) {
+   ret = reset_control_deassert(resets->rstc[i]);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_assert(resets->rstc[i]);
+   return ret;
+}
+
+static inline bool reset_control_is_array(struct reset_control *rstc)
+{
+   return rstc->array;
+}
+
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
@@ -158,6 +230,9 @@ int reset_control_reset(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_reset(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->reset)
return -ENOTSUPP;
 
@@ -202,6 +277,9 @@ int reset_control_assert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_assert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->assert)
return -ENOTSUPP;
 
@@ -240,6 +318,9 @@ int reset_control_deassert(struct reset_control *rstc)
if (WARN_ON(IS_ERR(rstc)))
return -EINVAL;
 
+   if (reset_control_is_array(rstc))
+   return reset_control_array_deassert(rstc_to_array(rstc));
+
if (!rstc->rcdev->ops->deassert)
return -ENOTSUPP;
 
@@ -266,7 +347,7 @@ int reset_control_status(struct reset_control *rstc)
if (!rstc)
return 0;
 
-   if (WARN_ON(IS_ERR(rstc)))
+   if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
return -EINVAL;
 
if (rstc->rcdev->ops->status)
@@ -404,6 +485,16 @@ struct reset_control *__reset_control_get(struct device 
*dev, const char *id,
 }
 EXPORT_SYMBOL_GPL(__reset_control_get);
 
+static void reset_control_array_put(struct reset_control_array *resets)
+{
+   int i;
+
+   mutex_lock(_list_mutex);
+   for (i = 0; i < resets->num_rstcs; i++)
+   __reset_control_put_internal(resets->rstc[i]);
+   mutex_unlock(_l

[PATCH v5 1/6] reset: use kref for reference counting

[Philipp Zabel]

Use kref for reference counting and enjoy the advantages of refcount_t.

Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/reset/core.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index cd739d2fa1603..0090784ff4105 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -40,7 +41,7 @@ struct reset_control {
struct reset_controller_dev *rcdev;
struct list_head list;
unsigned int id;
-   unsigned int refcnt;
+   struct kref refcnt;
bool shared;
atomic_t deassert_count;
atomic_t triggered_count;
@@ -288,7 +289,7 @@ static struct reset_control *__reset_control_get_internal(
if (WARN_ON(!rstc->shared || !shared))
return ERR_PTR(-EBUSY);
 
-   rstc->refcnt++;
+   kref_get(>refcnt);
return rstc;
}
}
@@ -302,18 +303,18 @@ static struct reset_control *__reset_control_get_internal(
rstc->rcdev = rcdev;
list_add(>list, >reset_control_head);
rstc->id = index;
-   rstc->refcnt = 1;
+   kref_init(>refcnt);
rstc->shared = shared;
 
return rstc;
 }
 
-static void __reset_control_put_internal(struct reset_control *rstc)
+static void __reset_control_release(struct kref *kref)
 {
-   lockdep_assert_held(_list_mutex);
+   struct reset_control *rstc = container_of(kref, struct reset_control,
+ refcnt);
 
-   if (--rstc->refcnt)
-   return;
+   lockdep_assert_held(_list_mutex);
 
module_put(rstc->rcdev->owner);
 
@@ -321,6 +322,13 @@ static void __reset_control_put_internal(struct 
reset_control *rstc)
kfree(rstc);
 }
 
+static void __reset_control_put_internal(struct reset_control *rstc)
+{
+   lockdep_assert_held(_list_mutex);
+
+   kref_put(>refcnt, __reset_control_release);
+}
+
 struct reset_control *__of_reset_control_get(struct device_node *node,
 const char *id, int index, bool shared,
 bool optional)
@@ -400,7 +408,6 @@ EXPORT_SYMBOL_GPL(__reset_control_get);
  * reset_control_put - free the reset controller
  * @rstc: reset controller
  */
-
 void reset_control_put(struct reset_control *rstc)
 {
if (IS_ERR_OR_NULL(rstc))
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 0/6] reset: APIs to manage a list of resets

[Philipp Zabel]

A set of patches to allow consumers to get and de/assert or trigger
a number of resets at the same time. A patch on top of Vivek's original
API extension is added to hide the reset_control_array behind a struct
reset_control so that the consumer doesn't have care about the difference
between a singular reset control and reset control controlling an array
of resets, except when requesting the control.

This series also contains reset controls patches for dwc3-of-simple
and tegra pmc drivers.
A small patch is added in this series to correctly re-order the
resource handling in dwc3_of_simple_remove().

The series is tested on torvald's master branch the device tree
patches to enable usb on db820c.

Changes since v4:
 - Added a patch to hide reset control arrays behind struct reset_control
   and adapted the consumer patches. This could be merged with the reset
   array API patch if we think this is a good idea.

Changes since v3:
 - Squashed of_reset_control_get_count() patch in the second patch that
   adds the reset control array APIs.
 - The error path after getting count through of_reset_control_get_count()
   now returns NULL pointer in case when 'optional' flag is true.
 - Added code in reset_control_array_assert() to deassert the
   already asserted resets in the error case.
 - Using of_reset_control_array_get_optional_exclusive() in dwc3 patch
   to request the reset control array.
 - Added a patch to fix the order in which resources are handled in
   dwc3_of_simple_remove() path.
 - Added tegra_powergate->reset to take care of single reset control
   passed from the client drivers.

Changes since v2:
 - Addressed comments to make APIs inline with gpiod API.
 - Moved number of reset controls in 'struct reset_control_array'
   so that the footprint is reduced.
 - of_reset_control_array_get() and devm_reset_control_array_get()
   now return pointer to the newly created reset control array.
 - Added comments to mention that the reset control array APIs don't
   guarantee any particular order when handling the reset controls.
 - Dropped 'name' from reset_control_array' since the interface is meant
   for a bunch of anonymous resets that can all be asserted or deasserted
   in arbitrary order.
 - Fixed returns for APIs reported by kbuild.
 - Fixed 'for' clause guards reported by kbuild.

Changes since v1:
 - Addressed comment for error handling in of_reset_control_get_count()
 - Added patch to manage reset controller array.
 - Rebased dwc3-of-simple changes based on the new set of APIs
   for reset control array.
 - Added a patch for soc/tegra/pmc driver to use the new set of
   reset control array APIs.

Philipp Zabel (2):
  reset: use kref for reference counting
  reset: hide reset control arrays behind struct reset_control

Vivek Gautam (4):
  reset: Add APIs to manage array of resets
  usb: dwc3: of-simple: Re-order resource handling in remove
  usb: dwc3: of-simple: Add support to get resets for the device
  soc/tegra: pmc: Use the new reset APIs to manage reset controllers

 drivers/reset/core.c  | 234 --
 drivers/soc/tegra/pmc.c   |  82 -
 drivers/usb/dwc3/dwc3-of-simple.c |  29 -
 include/linux/reset.h |  73 
 4 files changed, 344 insertions(+), 74 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

Hi,

On Wed, 2017-05-31 at 15:23 +0100, Jon Hunter wrote:
> On 22/05/17 12:23, Vivek Gautam wrote:
> > Make use of reset_control_array_*() set of APIs to manage
> > an array of reset controllers available with the device.
> > 
> > Cc: Jon Hunter <jonath...@nvidia.com>
> > Cc: Thierry Reding <tred...@nvidia.com>
> > Cc: Philipp Zabel <p.za...@pengutronix.de>
> > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > ---
> >  drivers/soc/tegra/pmc.c | 91 
> > +
> >  1 file changed, 31 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index e233dd5dcab3..668f5d3d3635 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -124,8 +124,8 @@ struct tegra_powergate {
> > unsigned int id;
> > struct clk **clks;
> > unsigned int num_clks;
> > -   struct reset_control **resets;
> > -   unsigned int num_resets;
> > +   struct reset_control *reset;
> > +   struct reset_control_array *resets;
> 
> It's a shame we can't avoid this additional reset pointer, but maybe
> there is no good alternative for now. So ...
>
> Acked-by: Jon Hunter <jonath...@nvidia.com>
> Tested-by: Jon Hunter <jonath...@nvidia.com>

Thanks.  I don't see a big functional difference between a
reset_control_array and a reset_control, given that a single reset
control bit already controls multiple reset lines on some devices.

Maybe it would be preferable to let the reset_control_array_get
functions return a struct reset_control that hides the array.
I'll send a v5 to see if that would be sensible.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
> On 25/04/17 05:15, Vivek Gautam wrote:
> > On 04/24/2017 06:15 PM, Jon Hunter wrote:
> >> On 18/04/17 12:21, Vivek Gautam wrote:
> >>> Make use of reset_control_array_*() set of APIs to manage
> >>> an array of reset controllers available with the device.
> >> Before we apply this patch, I need to check to see if the order of the
> >> resets managed by the PMC driver matter. Today the order of the resets
> >> is determined by the order they appear in the DT node and although the
> >> new APIs work in the same way they do not guarantee this. So let me
> >> check to see if we can any concerns about ordering here. Otherwise would
> >> be nice to use these APIs.
> > 
> > Right, that will be perfect.
> 
> So I don't see any restrictions here and so I think this change is fine.

Thank you for checking.

> BTW, for the DT case, is there any reason why we don't just say the
> order will be determine by the order the resets are list in the DT node?

I'd rather not make any promises, so I don't have to care about keeping
them. This makes it easier to think about and allows for more freedom in
changing the core code if needed.

What if in the future there is a use case for enabling a bunch of resets
by flipping a number of bits in a single register at the same time? Or
if people accidentally depend on the ordering when in reality there is a
small delay necessary between assertions that just happens to be hidden
by the framework overhead?

If there is a use case for an array of reset controls that must be
(de)asserted in a fixed order and doesn't need any delay between the
steps and is not suitable to be described by named resets for some
reason, we can discuss this. Until then, I'm happy that tegra pmc can
handle arrays without any particular ordering.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Make use of reset_control_array_*() set of APIs to manage
> an array of reset controllers available with the device.
> 
> Cc: Thierry Reding <tred...@nvidia.com>
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
>  drivers/soc/tegra/pmc.c | 99 
> ++---
>  1 file changed, 36 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e233dd5dcab3..4d039fa3db1b 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -124,8 +124,7 @@ struct tegra_powergate {
>   unsigned int id;
>   struct clk **clks;
>   unsigned int num_clks;
> - struct reset_control **resets;
> - unsigned int num_resets;
> + struct reset_control_array *resets;
>  };
>  
>  struct tegra_io_pad_soc {
> @@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct 
> tegra_powergate *pg)
>   return err;
>  }
>  
> -static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_assert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_array_assert(pg->resets);
>  }
>  
> -static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
>  {
> - unsigned int i;
> - int err;
> -
> - for (i = 0; i < pg->num_resets; i++) {
> - err = reset_control_deassert(pg->resets[i]);
> - if (err)
> - return err;
> - }
> -
> - return 0;
> + return reset_control_array_deassert(pg->resets);
>  }
>  
>  static int tegra_powergate_power_up(struct tegra_powergate *pg,
> @@ -558,6 +539,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
> struct clk *clk,
> struct reset_control *rst)
>  {
>   struct tegra_powergate pg;
> + struct reset_control_array *resets;
>   int err;
>  
>   if (!tegra_powergate_is_available(id))
> @@ -566,12 +548,25 @@ int tegra_powergate_sequence_power_up(unsigned int id, 
> struct clk *clk,
>   pg.id = id;
>   pg.clks = 
>   pg.num_clks = 1;
> - pg.resets = 
> - pg.num_resets = 1;

Oh, I didn't notice that the resets array is often used for a single
reset. I think in this case instead of wrapping a reset_control_array
around the passed reset, you could store it as an additional pointer in
tegra_powergate->reset and add
if (pg->reset)
return reset_control_(de)assert(pg->resets);
to the tegra_powergate_reset_(de)assert functions ...

> +
> + resets = kzalloc(sizeof(*resets) + sizeof(resets->rstc[0]) * 1,
> +  GFP_KERNEL);
> + if (!resets)
> + return -ENOMEM;
> +
> + resets->rstc[0] = rst;
> + pg.resets = resets;
>  
>   err = tegra_powergate_power_up(, false);
> - if (err)
> + if (err) {
>   pr_err("failed to turn on partition %d: %d\n", id, err);
> + goto free_reset;
> + }
> +
> + return 0;
> +
> +free_reset:
> + kfree(resets);
>  

... as having to open code this handling of struct reset_control_array
internals completely negates its convenience.

>   return err;
>  }
> @@ -755,45 +750,26 @@ static int tegra_powergate_of_get_clks(struct 
> tegra_powergate *pg,
>  static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
>struct device_node *np, bool off)
>  {
> - struct reset_control *rst;
> - unsigned int i, count;
>   int err;
>  
> - count = of_count_phandle_with_args(np, "resets", "#reset-cells");
> - if (count == 0)
> - return -ENODEV;
> -
> - pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
> - if (!pg->resets)
> - return -ENOMEM;
> -
> - for (i = 0; i < count; i++) {
> - pg->resets[i] = of_reset_control_get_by_index(np, i);
> - if (IS_ERR(pg->resets[i])) {
> - err = PTR_ERR(pg->resets[i]);
> - goto error;
> - }
> -
> - if (off)
> -   

Re: [PATCH V3 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Add support to get a list of resets available for the device.
> These resets must be kept de-asserted until the device is
> in use.
> 
> Cc: Felipe Balbi <ba...@kernel.org>
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 36 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index fe414e7a9c78..9116df649f0b 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -29,13 +29,39 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct dwc3_of_simple {
>   struct device   *dev;
>   struct clk  **clks;
>   int num_clocks;
> + struct reset_control_array *resets;
>  };
>  
> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple)
> +{
> + struct device   *dev = simple->dev;
> + int ret;
> +
> + simple->resets = of_reset_control_array_get_exclusive(dev->of_node);
> + if (IS_ERR(simple->resets)) {
> + ret = PTR_ERR(simple->resets);
> + if (ret == -ENOENT)
> + /* not all controllers required resets */
> + return 0;

If you use the of_reset_control_array_get_optional_exclusive variant,
you can remove the three lines directly above.

> + dev_err(dev, "failed to get device resets\n");

It would be nice to print the error code here.

> + return ret;
> + }
> +
> + ret = reset_control_array_deassert(simple->resets);
> + if (ret) {
> + reset_control_array_put(simple->resets);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
>  static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
>  {
>   struct device   *dev = simple->dev;
> @@ -100,6 +126,10 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>   if (ret)
>   return ret;
>  
> + ret = dwc3_of_simple_reset_init(simple);
> + if (ret)
> + return ret;
> +

I would just move the contents of dwc3_of_simple_reset_init here and add
a goto error path at the end of dwc3_of_simple_clk_init to handle the
reset control cleanup as needed.

>   ret = of_platform_populate(np, NULL, NULL, dev);
>   if (ret) {
>   for (i = 0; i < simple->num_clocks; i++) {
> @@ -107,6 +137,9 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>   clk_put(simple->clks[i]);
>   }
>  
> + reset_control_array_assert(simple->resets);
> + reset_control_array_put(simple->resets);
> +
>   return ret;
>   }
>  
> @@ -128,6 +161,9 @@ static int dwc3_of_simple_remove(struct platform_device 
> *pdev)
>   clk_put(simple->clks[i]);
>   }
>  
> + reset_control_array_assert(simple->resets);
> + reset_control_array_put(simple->resets);
> +

Must the reset be asserted before of_platform_depopulate?

>   of_platform_depopulate(dev);

Given the order above, I'd expect the reset cleanup here.

>   pm_runtime_put_sync(dev);

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Many devices may want to request a bunch of resets
> and control them. So it's better to manage them as an
> array. Add APIs to _get(), _assert(), and _deassert()
> an array of reset_control.

Thanks! This looks good to me, one small issue below.

> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
>  drivers/reset/core.c  | 177 
> ++
>  include/linux/reset.h |  93 ++
>  2 files changed, 270 insertions(+)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index f0a06a7aca93..54bd3be5e7a4 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -488,3 +488,180 @@ int of_reset_control_get_count(struct device_node *node)
>   return count;
>  }
>  EXPORT_SYMBOL_GPL(of_reset_control_get_count);
> +
> +/**
> + * APIs to manage an array of reset controls.
> + */
> +/**
> + * reset_control_array_assert: assert a list of resets
> + *
> + * @resets: reset control array holding info about the list of resets
> + *
> + * This API doesn't guarantee that the reset lines controlled by
> + * the reset array are asserted in any particular order.
> + *
> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_assert(struct reset_control_array *resets)
> +{
> + int ret, i;
> +
> + if (!resets)
> + return 0;
> +
> + if (IS_ERR(resets))
> + return -EINVAL;
> +
> + for (i = 0; i < resets->num_rstcs; i++) {
> + ret = reset_control_assert(resets->rstc[i]);
> + if (ret)
> + return ret;

This should try to deassert the already asserted resets in the error
case.

> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_assert);
> +
> +/**
> + * reset_control_array_deassert: deassert a list of resets
> + *
> + * @resets: reset control array holding info about the list of resets
> + *
> + * This API doesn't guarantee that the reset lines controlled by
> + * the reset array are deasserted in any particular order.
> + *
> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_deassert(struct reset_control_array *resets)
> +{
> + int ret, i;
> +
> + if (!resets)
> + return 0;
> +
> + if (IS_ERR(resets))
> + return -EINVAL;
> +
> + for (i = 0; i < resets->num_rstcs; i++) {
> + ret = reset_control_deassert(resets->rstc[i]);
> + if (ret)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + while (i--)
> + reset_control_assert(resets->rstc[i]);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_deassert);

As this already does.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/4] reset: Add API to count number of reset available with device

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Count number of reset phandles available with the device node
> to know the resets a given device has.
> 
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
>  drivers/reset/core.c  | 23 +++
>  include/linux/reset.h |  6 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index cd739d2fa160..f0a06a7aca93 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -465,3 +465,26 @@ int device_reset(struct device *dev)
>   return ret;
>  }
>  EXPORT_SYMBOL_GPL(device_reset);
> +
> +/**
> + * of_reset_control_get_count - Count number of resets available with a 
> device
> + *
> + * @node: device node that contains 'resets'.
> + *
> + * Returns positive reset count on success, or error number on failure and
> + * on count being zero.
> + */
> +int of_reset_control_get_count(struct device_node *node)
> +{
> + int count;
> +
> + if (!node)
> + return -EINVAL;
> +
> + count = of_count_phandle_with_args(node, "resets", "#reset-cells");
> + if (count == 0)
> + count = -ENOENT;
> +
> + return count;
> +}
> +EXPORT_SYMBOL_GPL(of_reset_control_get_count);

This doesn't need to be public anymore. You can make it static and merge
it into the second patch.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

Hi Vivek,

On Tue, 2017-04-04 at 16:09 +0530, Vivek Gautam wrote:
[...]
> > I'd prefer to mirror the gpiod API a little, and to have the number
> > contained in the array structure, similar to struct gpio_descs:
[...]
> Alright, i can update this.
> I took regulator_bulk interface as the reference, but now i see
> gpio_descs has a smaller footprint.

Ok, understood.
I think the smaller footprint API is more convenient for the user.

[...]
> >> + * Returns 0 on success or error number on failure
> >> + */
> >> +static int reset_control_array_get(struct device *dev, int num_rsts,
> >> +  struct reset_control_array *resets,
> >> +  bool shared, bool optional)
> > Can we make this count and return a pointer to the newly created array?
> >
> > static struct reset_controls *
> > reset_control_array_get(struct device *dev, bool shared, bool optional)
> > {
> > ...
> >
> > That way the consumer doesn't have to care about counting and array
> > allocation.
> 
> Just trying to think again in line with the regulator bulk APIs.
> Can't a user provide a list of reset controls as data and thus
> request reset controls with a "id" and num_resets available.
> 
> e.g.
> const char * const rst_names[] = {
>   "rst1", "rst2" ...
> };
> xyz_data = {
>   .resets = rst_names;
>   .num = ARRAY_SIZE(rst_names);
> };
> and then the driver making use of reset_control_array APIs
> to request this list of reset controls and assert/deassert them.
> 
> I am assuming this case when one user driver is used across a bunch
> of platforms with different number of resets available.
> We may not want to rely solely on the device tree entries, since
> the resets can be mandatory in few cases and we may want to
> return failure.

The way I understood the array API was as a method of handling a list of
anonymous resets, specified as

resets = < 1>, < 2>, < 3>;
// reset-names = "rst1", "rst2", "rst3"; /* don't care */

in the device tree.

Now if you want to handle a partial list of those as an unordered list,
with special consideration for others, that could be added as a separate
API when there is use for it. But I expect that most potential users of
this array API will not have to make such a distinction.

> >> @@ -85,6 +107,39 @@ static inline struct reset_control 
> >> *__devm_reset_control_get(
> >>return -ENOTSUPP;
> >>   }
> >>   
> >> +static inline int reset_control_array_assert(int num_rsts,
> >> +  struct reset_control_array *resets)
> >> +{
> >> +  return 0;
> >> +}
> >> +
> >> +static inline int reset_control_array_deassert(int num_rsts,
> >> +  struct reset_control_array *resets)
> >> +{
> >> +  return 0;
> >> +}
> > Is this missing a stub for reset_control_array_get?
> 
> No, that's supposed to be a static function.

Oh right, it is static. Please ignore.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

Hi Vivek,

thank you for the patch series. A few comments below: I'd like to reduce
the API surface a bit and include the counting and array allocation in
the _get functions, if possible.

On Mon, 2017-04-03 at 19:12 +0530, Vivek Gautam wrote:
> Many devices may want to request a bunch of resets
> and control them. So it's better to manage them as an
> array. Add APIs to _get(), _assert(), and _deassert()
> an array of reset_control.
> 
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> ---
> 
> Changes since v1:
>  - New patch added to the series.
> 
>  drivers/reset/core.c  | 169 
> ++
>  include/linux/reset.h | 108 
>  2 files changed, 277 insertions(+)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 66db061165cb..fb908aa4f69e 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -477,3 +477,172 @@ int of_reset_control_get_count(struct device_node *node)
>   return count;
>  }
>  EXPORT_SYMBOL_GPL(of_reset_control_get_count);
> +
> +/*
> + * APIs to manage an array of reset controllers
> + */
> +/**
> + * reset_control_array_assert: assert a list of resets
> + *
> + * @resets: reset control array holding info about a list of resets
> + * @num_rsts: number of resets to be asserted.

This should mention the API doesn't make any guarantees that the reset
lines controlled by the reset array are asserted or deasserted in any
particular order.

> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_assert(int num_rsts,
> + struct reset_control_array *resets)

I'd prefer to mirror the gpiod API a little, and to have the number
contained in the array structure, similar to struct gpio_descs:

struct reset_control_array {
unsigned int num_rstcs;
struct reset_control *rstc[];
};

int reset_control_array_assert(struct reset_control_array *resets)
{
...

> +{
> + int ret, i;
> +
> + if (WARN_ON(IS_ERR_OR_NULL(resets)))
> + return -EINVAL;
> +
> + for (i = 0; i < num_rsts; i++) {
> + ret = reset_control_assert(resets[i].rst);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_assert);
> +
> +/**
> + * reset_control_array_deassert: deassert a list of resets
> + *
> + * @resets: reset control array holding info about a list of resets
> + * @num_rsts: number of resets to be deasserted.

Same here, no guarantees on the order in which the resets are
deasserted.

> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_deassert(int num_rsts,
> + struct reset_control_array *resets)
> +{
> + int ret, i;
> +
> + if (WARN_ON(IS_ERR_OR_NULL(resets)))
> + return -EINVAL;
> +
> + for (i = 0; i < num_rsts; i++)
> + ret = reset_control_deassert(resets[i].rst);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + while (--i >= 0)
> + reset_control_assert(resets[i].rst);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_deassert);
> +
> +/**
> + * reset_control_array_get - Get a list of reset controllers

A list of "reset controls".

> + *
> + * @dev: device that requests the list of reset controllers
> + * @num_rsts: number of reset controllers requested
> + * @resets: reset control array holding info about a list of resets
> + * @shared: whether reset controllers are shared or not
> + * @optional: whether it is optional to get the reset controllers
> + *

This should mention that the array API is intended for a list of resets
that just have to be asserted or deasserted, without any requirements on
the order.

> + * Returns 0 on success or error number on failure
> + */
> +static int reset_control_array_get(struct device *dev, int num_rsts,
> + struct reset_control_array *resets,
> + bool shared, bool optional)

Can we make this count and return a pointer to the newly created array?

static struct reset_controls *
reset_control_array_get(struct device *dev, bool shared, bool optional)
{
...

That way the consumer doesn't have to care about counting and array
allocation.

> +{
> + int ret, i;
> + struct reset_control *rstc;
> +
> + for (i = 0; i < num_rsts; i++) {
> + rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
> + 

Re: [PATCH v2 13/14] usb: host: ehci-st: simplify optional reset handling

[Philipp Zabel]

Hi Alan,

On Wed, 2017-03-15 at 10:35 -0400, Alan Stern wrote:
> On Wed, 15 Mar 2017, Philipp Zabel wrote:
> 
> > As of commit bb475230b8e5 ("reset: make optional functions really
> > optional"), the reset framework API calls use NULL pointers to describe
> > optional, non-present reset controls.
> 
> What does it use to describe genuine errors?

Negative error pointers, as before. The only difference is that instead
of returning -ENOENT if no resets are specified in the device tree, the
optional reset_control_get variants now return NULL.

> > This allows to return errors from devm_reset_control_get_optional_shared
> > unconditionally.
> > 
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > ---
> >  drivers/usb/host/ehci-st.c | 8 ++--
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-st.c b/drivers/usb/host/ehci-st.c
> > index be4a2788fc582..12e803d2c98df 100644
> > --- a/drivers/usb/host/ehci-st.c
> > +++ b/drivers/usb/host/ehci-st.c
> > @@ -210,18 +210,14 @@ static int st_ehci_platform_probe(struct 
> > platform_device *dev)
> > devm_reset_control_get_optional_shared(>dev, "power");
> > if (IS_ERR(priv->pwr)) {
> > err = PTR_ERR(priv->pwr);
> > -   if (err == -EPROBE_DEFER)
> > -   goto err_put_clks;
> > -   priv->pwr = NULL;
> > +   goto err_put_clks;
> > }
> >  
> > priv->rst =
> > devm_reset_control_get_optional_shared(>dev, "softreset");
> > if (IS_ERR(priv->rst)) {
> > err = PTR_ERR(priv->rst);
> > -   if (err == -EPROBE_DEFER)
> > -   goto err_put_clks;
> > -   priv->rst = NULL;
> > +   goto err_put_clks;
> > }
> >  
> > if (pdata->power_on) {
> 
> These changes do not agree with the patch description.  If any sort of 
> error besides EPROBE_DEFER occurs then:
>
>   the old code sets priv->pwr or priv->rst to NULL and continues;
> 
>   the new code returns with an error.
> 
> The only way the patch could be equivalent to the existing code would
> be if devm_reset_control_get_optional_shared() returns no errors other
> than EPROBE_DEFER.  But the patch description doesn't say this.
> 
> Alan Stern

You are right, devm_reset_control_get_optional_shared can return:

-ENOMEM, returned by devres_alloc in __devm_reset_control_get,

-EILSEQ, returned by of_property_match_string in __of_reset_control_get
if the "reset-names" DT property is broken,

-EINVAL, returned by of_parse_phandle_with_args in
__of_reset_control_get, if there is a reset phandle specified in the
device tree but it is pointing to an invalid reset controller node
(missing "#reset-cells" property or number of phandle arguments does not
match).

-EINVAL, returned by the reset controller driver's choice of of_xlate,
also if the reset is specified but somehow invalid.

So yes, if there was a genuine error in the device tree, we would now
return the error instead of silently ignoring it as before. I assume
that these errors were never intended to be ignored, it just happened to
be a hassle to separate them from the -ENOENT condition with the old
API.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 12/14] usb: dwc2: simplify optional reset handling

[Philipp Zabel]

As of commit bb475230b8e5 ("reset: make optional functions really
optional"), the reset framework API calls use NULL pointers to describe
optional, non-present reset controls.

This allows to return errors from devm_reset_control_get_optional and to
call reset_control_(de)assert unconditionally.

Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
Acked-by: John Youn <johny...@synopsys.com>
---
 drivers/usb/dwc2/platform.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9564bc76c56f3..daf0d37acb37f 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -214,20 +214,11 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
if (IS_ERR(hsotg->reset)) {
ret = PTR_ERR(hsotg->reset);
-   switch (ret) {
-   case -ENOENT:
-   case -ENOTSUPP:
-   hsotg->reset = NULL;
-   break;
-   default:
-   dev_err(hsotg->dev, "error getting reset control %d\n",
-   ret);
-   return ret;
-   }
+   dev_err(hsotg->dev, "error getting reset control %d\n", ret);
+   return ret;
}
 
-   if (hsotg->reset)
-   reset_control_deassert(hsotg->reset);
+   reset_control_deassert(hsotg->reset);
 
/* Set default UTMI width */
hsotg->phyif = GUSBCFG_PHYIF16;
@@ -326,8 +317,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
if (hsotg->ll_hw_enabled)
dwc2_lowlevel_hw_disable(hsotg);
 
-   if (hsotg->reset)
-   reset_control_assert(hsotg->reset);
+   reset_control_assert(hsotg->reset);
 
return 0;
 }
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 13/14] usb: host: ehci-st: simplify optional reset handling

[Philipp Zabel]

As of commit bb475230b8e5 ("reset: make optional functions really
optional"), the reset framework API calls use NULL pointers to describe
optional, non-present reset controls.

This allows to return errors from devm_reset_control_get_optional_shared
unconditionally.

Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
 drivers/usb/host/ehci-st.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ehci-st.c b/drivers/usb/host/ehci-st.c
index be4a2788fc582..12e803d2c98df 100644
--- a/drivers/usb/host/ehci-st.c
+++ b/drivers/usb/host/ehci-st.c
@@ -210,18 +210,14 @@ static int st_ehci_platform_probe(struct platform_device 
*dev)
devm_reset_control_get_optional_shared(>dev, "power");
if (IS_ERR(priv->pwr)) {
err = PTR_ERR(priv->pwr);
-   if (err == -EPROBE_DEFER)
-   goto err_put_clks;
-   priv->pwr = NULL;
+   goto err_put_clks;
}
 
priv->rst =
devm_reset_control_get_optional_shared(>dev, "softreset");
if (IS_ERR(priv->rst)) {
err = PTR_ERR(priv->rst);
-   if (err == -EPROBE_DEFER)
-   goto err_put_clks;
-   priv->rst = NULL;
+   goto err_put_clks;
}
 
if (pdata->power_on) {
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Wed, 2017-02-22 at 10:54 +0530, Vivek Gautam wrote:
> Add support to get a list of resets available for the device.
> These resets must be kept de-asserted until the device is
> in use.
> 
> Cc: Felipe Balbi 
> Signed-off-by: Vivek Gautam 
> ---
> 
> Based on torvald's master branch.
> 
>  drivers/usb/dwc3/dwc3-of-simple.c | 49 
> +++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index fe414e7a9c78..025de7342d28 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -29,13 +29,52 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct dwc3_of_simple {
>   struct device   *dev;
>   struct clk  **clks;
>   int num_clocks;
> + struct reset_control**resets;
> + int num_resets;
>  };
>  
> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple, int 
> count)
> +{
> + struct device   *dev = simple->dev;
> + int i;
> +
> + simple->num_resets = count;
> +
> + if (!count)
> + return 0;
> +
> + simple->resets = devm_kcalloc(dev, simple->num_resets,
> + sizeof(struct reset_control *), GFP_KERNEL);
> + if (!simple->resets)
> + return -ENOMEM;
> +
> + for (i = 0; i < simple->num_resets; i++) {
> + struct reset_control *reset;
> + int ret;
> +
> + reset = devm_reset_control_get_by_index(dev, i);

Please use devm_reset_control_get_exclusive_by_index instead. See
include/linux/reset.h for details.

> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
> +
> + simple->resets[i] = reset;
> +
> + ret = reset_control_deassert(reset);
> + if (ret) {
> + while (--i >= 0)
> + reset_control_assert(reset);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}

This looks rather generic. Should we have a
reset_control_get/assert/deassert_array functionality at the reset API
level?

>  static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
>  {
>   struct device   *dev = simple->dev;
> @@ -100,6 +139,10 @@ static int dwc3_of_simple_probe(struct platform_device 
> *pdev)
>   if (ret)
>   return ret;
>  
> + ret = dwc3_of_simple_reset_init(simple, of_reset_control_get_count(np));
> + if (ret)
> + return ret;
> +

Not a blocker, but it seems a bit inconsistent to count the reset
controls via the device node (of_...), but then get them via the device
(devm_reset_control_get_... instead of of_reset_control_get_...).

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] reset: Add API to count number of reset available with device

[Philipp Zabel]

Hi Vivek,

On Fri, 2017-03-10 at 20:10 +0530, Vivek Gautam wrote:
> Hi Philipp,
> 
> 
> On Wed, Feb 22, 2017 at 10:54 AM, Vivek Gautam
> <vivek.gau...@codeaurora.org> wrote:
> > Count number of reset phandles available with the device node
> > to know the resets a given device has.
> >
> > Cc: Philipp Zabel <p.za...@pengutronix.de>
> > Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
> > ---
> 
> Any thoughts on this change?
> A small addition that seems useful.

Sorry I missed this one earlier.

> >
> > Based on torvald's master branch.
> >
> >  include/linux/reset.h | 16 
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 5daff15722d3..88f63a640153 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -2,6 +2,7 @@
> >  #define _LINUX_RESET_H_
> >
> >  #include 
> > +#include 
> >
> >  struct reset_control;
> >
> > @@ -234,6 +235,21 @@ static inline struct reset_control 
> > *of_reset_control_get_shared_by_index(
> >  }
> >
> >  /**
> > + * of_reset_control_get_count - Count number of resets available with a 
> > device
> > + * @node: device to be reset by the controller
> > + */
> > +static inline unsigned int of_reset_control_get_count(struct device_node 
> > *node)
> > +{
> > +   int count;
> > +
> > +   count = of_count_phandle_with_args(node, "resets", "#reset-cells");
> > +   if (count < 0)
> > +   return 0;

Please don't silently ignore errors. gpiod_count doesn't ignore errors
either. tegra_powergate_of_resets in drivers/soc/tegra/pmc.c open codes
this, too. This should be changed so it can be used there, too.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Philipp Zabel]

Hi Martin,

Am Mittwoch, den 14.09.2016, 23:23 +0200 schrieb Martin Blumenstingl:
[...]
> > We could add a triggered flag or a counter to struct reset_control, and
> > have reset_control_reset_once do nothing if it is already set /
> > incremented. Since the reset_control goes away with the last consumer,
> > the shared reset line would get triggered again after unbinding both PHY
> > devices.
> I guess that'd do the trick for us:
> - we could use devm_reset_control_get_optional_shared() during probe
> - power_on would then call reset_control_reset()
> - the code in reset_control_reset would be changed: the if
> (WARN_ON(rstc->shared)) would be removed. then we return 0 if
> (rstc->shared && atomic_read(>shared_triggered)). otherwise we
> proceed with the old logic, except that we use
> atomic_set(>shared_triggerred, 1) in case of success (if an
> error was returned we leave it as "not triggered").
> 
> Let me know if you want me to (at least try to) implement that and send an 
> RFC.

Yes, please give it a try. reset_control_reset should still WARN if the
deassert count is set, and reset_(de)assert should do so if triggered is
set. Mixing the two won't work. And it should be documented that shared
reset_control_reset may do nothing if the reset was already triggered by
another consumer.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Philipp Zabel]

Am Dienstag, den 13.09.2016, 20:38 +0200 schrieb Martin Blumenstingl:
> Hi Philipp,
> 
> On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <p.za...@pengutronix.de> wrote:
> > Hi Martin,
> >
> > Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:
> >> On Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman <khil...@baylibre.com> wrote:
> >> > Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:
> >> >
> >> >> On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks <ben.do...@codethink.co.uk> 
> >> >> wrote:
> >> >>> On 08/09/16 21:42, Kevin Hilman wrote:
> >> >>>>
> >> >>>> Ben Dooks <ben.do...@codethink.co.uk> writes:
> >> >>>>
> >> >>>>> On 08/09/16 20:52, Martin Blumenstingl wrote:
> >> >>>>>>
> >> >>>>>> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman <khil...@baylibre.com>
> >> >>>>>> wrote:
> >> >>>>>>>>
> >> >>>>>>>> + phy = devm_phy_create(>dev, NULL, 
> >> >>>>>>>> _meson_usb2_ops);
> >> >>>>>>>> + if (IS_ERR(phy)) {
> >> >>>>>>>> + dev_err(>dev, "failed to create PHY\n");
> >> >>>>>>>> + return PTR_ERR(phy);
> >> >>>>>>>> + }
> >> >>>>>>>> +
> >> >>>>>>>> + if (usb_reset_refcnt++ == 0) {
> >> >>>>>>>> + ret = device_reset(>dev);
> >> >>>>>>>> + if (ret) {
> >> >>>>>>>> + dev_err(>dev, "Failed to reset USB 
> >> >>>>>>>> PHY\n");
> >> >>>>>>>> + return ret;
> >> >>>>>>>> + }
> >> >>>>>>>> + }
> >> >>>>>>>
> >> >>>>>>>
> >> >>>>>>> The ref count + reset here looks like something that could/should 
> >> >>>>>>> be
> >> >>>>>>> handled in a runtime PM callback.
> >> >>>>>>
> >> >>>>>> Unfortunately that doesn't work (as Jerome found out) because both
> >> >>>>>> PHYs are sharing the same reset line.
> >> >>>>>> So if the second PHY would call device_reset then it would also 
> >> >>>>>> reset
> >> >>>>>> the first PHY!
> >> >>>>>>
> >> >>>>>> There's a comment above the declaration of usb_reset_refcnt which
> >> >>>>>> tries to explain this:
> >> >>>>>> "The PHYs are sharing a common reset line -> we are only allowed to
> >> >>>>>> reset once for all PHYs."
> >> >>>>>> Maybe I should move this comment to the "if (usb_reset_refcnt++ == 
> >> >>>>>> 0)
> >> >>>>>> {" line to make it easier to see?
> >> >>>>>>
> >> >>>>>
> >> >>>>> pm-runtime has refcounting in it. When one of the nodes turns on,
> >> >>>>> the pm-runtime will call your driver to say there is a user when
> >> >>>>> this first use turns up.
> >> >>>>>
> >> >>>>> If all the sub-phys turn off and drop their refcount then the driver
> >> >>>>> is called to say there are no more users and you can go to sleep.
> >> >>>>
> >> >>>>
> >> >>>> After a chat w/Martin on IRC, It turns out runtime PM wont help here.
> >> >>>>
> >> >>>> The reason is because there are physically two PHY devices[1].  Those 
> >> >>>> 2
> >> >>>> devices will be treated independely by runtime PM, and have separate
> >> >>>> use-counting, which means doing what I proposed would cause a reset to
> >> >>>> happen when either device was probed.
> >> >>>>
> >> >>>> So, I think it's OK as it is.
> >> >>>
> >> >>

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Philipp Zabel]

Am Dienstag, den 13.09.2016, 17:59 -0700 schrieb Kevin Hilman:
> Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:
> 
> > On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <p.za...@pengutronix.de> 
> > wrote:
> 
> [...]
> 
> >>> I added Philipp and Hans to this thread - maybe they can comment on this.
> >>> To sum it up, our problem is:
> >>> - there are two separate USB PHYs on Meson GXBB
> >>> - both are sharing the same reset line (provided by the reset-meson 
> >>> driver)
> >>> - during initialization of the PHYs we must only call
> >>> reset_control_reset(rstc) once (if we do it for the first *and* second
> >>> PHY then the first PHY gets confused once the second PHY uses the
> >>> reset because the first PHY's state is reset as well)
> >>
> >> If you have an initially asserted reset line and you can enable the
> >> first module by deasserting the reset via reset_control_deassert (and
> >> reset_control_assert to signal when the module may be disabled again
> >> after use), shared resets are for you.
> >>
> >> If you need a reset pulse or have no direct control over the reset line,
> >> (device_reset), the reset framework currently has no solution for this.
> >> The ugly thing about reset_control_once would be that it can't re-reset
> >> modules when unloading and reloading driver modules.
> >
> > The corresponding reset driver in question is reset-meson, which only
> > implements reset (assert/deassert are not implemented). However, I
> > don't know if this is due to hardware design.
> > I think the hardware implements the latter, but maybe Neil can give
> > more information here (I currently don't have access to my board so I
> > cannot test how the hardware actually behaves).
> 
> It's implemented that way because the hardware only supports a reset
> pulse.

Would it be possible to bring down both PHYs drivers, pull the reset
line once, and then bring the drivers back up again?

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Philipp Zabel]

Am Dienstag, den 13.09.2016, 17:59 -0700 schrieb Kevin Hilman:
> Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:
> 
> > On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <p.za...@pengutronix.de> 
> > wrote:
> 
> [...]
> 
> >>> I added Philipp and Hans to this thread - maybe they can comment on this.
> >>> To sum it up, our problem is:
> >>> - there are two separate USB PHYs on Meson GXBB
> >>> - both are sharing the same reset line (provided by the reset-meson 
> >>> driver)
> >>> - during initialization of the PHYs we must only call
> >>> reset_control_reset(rstc) once (if we do it for the first *and* second
> >>> PHY then the first PHY gets confused once the second PHY uses the
> >>> reset because the first PHY's state is reset as well)
> >>
> >> If you have an initially asserted reset line and you can enable the
> >> first module by deasserting the reset via reset_control_deassert (and
> >> reset_control_assert to signal when the module may be disabled again
> >> after use), shared resets are for you.
> >>
> >> If you need a reset pulse or have no direct control over the reset line,
> >> (device_reset), the reset framework currently has no solution for this.
> >> The ugly thing about reset_control_once would be that it can't re-reset
> >> modules when unloading and reloading driver modules.
> >
> > The corresponding reset driver in question is reset-meson, which only
> > implements reset (assert/deassert are not implemented). However, I
> > don't know if this is due to hardware design.
> > I think the hardware implements the latter, but maybe Neil can give
> > more information here (I currently don't have access to my board so I
> > cannot test how the hardware actually behaves).
> 
> It's implemented that way because the hardware only supports a reset
> pulse.

Would it be possible to bring down both PHYs drivers, pull the reset
line once, and then bring the drivers back up again on this hardware?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Philipp Zabel]

Hi Martin,

Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:
> On Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman  wrote:
> > Martin Blumenstingl  writes:
> >
> >> On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks  
> >> wrote:
> >>> On 08/09/16 21:42, Kevin Hilman wrote:
> 
>  Ben Dooks  writes:
> 
> > On 08/09/16 20:52, Martin Blumenstingl wrote:
> >>
> >> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman 
> >> wrote:
> 
>  + phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
>  + if (IS_ERR(phy)) {
>  + dev_err(>dev, "failed to create PHY\n");
>  + return PTR_ERR(phy);
>  + }
>  +
>  + if (usb_reset_refcnt++ == 0) {
>  + ret = device_reset(>dev);
>  + if (ret) {
>  + dev_err(>dev, "Failed to reset USB 
>  PHY\n");
>  + return ret;
>  + }
>  + }
> >>>
> >>>
> >>> The ref count + reset here looks like something that could/should be
> >>> handled in a runtime PM callback.
> >>
> >> Unfortunately that doesn't work (as Jerome found out) because both
> >> PHYs are sharing the same reset line.
> >> So if the second PHY would call device_reset then it would also reset
> >> the first PHY!
> >>
> >> There's a comment above the declaration of usb_reset_refcnt which
> >> tries to explain this:
> >> "The PHYs are sharing a common reset line -> we are only allowed to
> >> reset once for all PHYs."
> >> Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
> >> {" line to make it easier to see?
> >>
> >
> > pm-runtime has refcounting in it. When one of the nodes turns on,
> > the pm-runtime will call your driver to say there is a user when
> > this first use turns up.
> >
> > If all the sub-phys turn off and drop their refcount then the driver
> > is called to say there are no more users and you can go to sleep.
> 
> 
>  After a chat w/Martin on IRC, It turns out runtime PM wont help here.
> 
>  The reason is because there are physically two PHY devices[1].  Those 2
>  devices will be treated independely by runtime PM, and have separate
>  use-counting, which means doing what I proposed would cause a reset to
>  happen when either device was probed.
> 
>  So, I think it's OK as it is.
> >>>
> >>>
> >>> Surely you can do pm_runtime_get/put on the phy's parent platform
> >>> device and do it that way?
> >> could you please be more specific with that (do you mean pdev->dev.parent)?
> >> so we would use pm_runtime_{get_sync,put} with the parent, while we
> >> would still define the runtime_resume in our driver.
> >
> > You'd also need to do get/put on the children, but yes, that's what Ben
> > is suggesting.
> >
> > However, the problem with all of the solutions proposed (runtime PM ones
> > included) is that we're forcing a board-specific design issue (2 devices
> > sharing a reset line) into a driver that should not have any
> > board-specific assumptions in it.
> >
> > For example, if this driver is used on another platform where different
> > PHYs have different reset lines, then one of them (the unlucky one who
> > is not probed first) will never get reset.  So any form of per-device
> > ref-counting is not a portable solution.
> indeed, so in simple words we would need something like
> reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would
> remember internally if any action has already been executed: if not it
> does a _reset, _assert or _deassert and otherwise it does nothing.
> 
> > I'm not sure yet how the reset framework is supposed to handle shared
> > reset lines, but that needs some investigation.  I quick glance and it
> > seems that reset controllers can have shared lines, so that should be
> > investigated.
> I added Philipp and Hans to this thread - maybe they can comment on this.
> To sum it up, our problem is:
> - there are two separate USB PHYs on Meson GXBB
> - both are sharing the same reset line (provided by the reset-meson driver)
> - during initialization of the PHYs we must only call
> reset_control_reset(rstc) once (if we do it for the first *and* second
> PHY then the first PHY gets confused once the second PHY uses the
> reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the
first module by deasserting the reset via reset_control_deassert (and
reset_control_assert to signal when the module may be disabled again
after use), shared resets are for you.

If you need a reset pulse or have no direct control over the reset line,
(device_reset), the reset framework currently has 

Re: [PATCH v2 1/6] binding-doc: power: pwrseq-generic: add binding doc for generic power sequence library

[Philipp Zabel]

Am Mittwoch, den 13.07.2016, 10:06 +0800 schrieb Peter Chen:
> Add binding doc for generic power sequence library.
> 
> Signed-off-by: Peter Chen <peter.c...@nxp.com>
> ---
>  .../bindings/power/pwrseq/pwrseq-generic.txt   | 53 
> ++
>  1 file changed, 53 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt 
> b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> new file mode 100644
> index 000..186c58c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> @@ -0,0 +1,53 @@
> +The generic power sequence library
> +
> +Some hard-wired USB/MMC devices need to do power sequence to let the
> +device work normally,

I would replace "to let the device work normally" with "before the
device can be enumerated [on the bus]" here.

>  the typical power sequence like: enable USB
> +PHY clock, toggle reset pin, etc. But current Linux USB driver
> +lacks of such code to do it, it may cause some hard-wired USB devices
> +works abnormal or can't be recognized by controller at all. The
> +power sequence will be done before this device can be found at USB
> +bus.
> +
> +The power sequence properties is under the device node.
> +
> +Required properties:
> +- power-sequence: this device needs to do power sequence before enumeration

As Joshua pointed out, is this even needed at all?

> +Optional properties:
> +- clocks: the input clock for device.
> +- reset-gpios: Should specify the GPIO for reset.
> +- reset-duration-us: the duration in microsecond for assert reset signal.

With the above two issues sorted out,
Acked-by: Philipp Zabel <p.za...@pengutronix.de>

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] binding-doc: power: pwrseq-generic: add binding doc for generic power sequence library

[Philipp Zabel]

Am Donnerstag, den 07.07.2016, 17:14 +0800 schrieb Peter Chen:
> Add binding doc for generic power sequence library.
> 
> Signed-off-by: Peter Chen 
> ---
>  .../bindings/power/pwrseq/pwrseq-generic.txt   | 56 
> ++
>  1 file changed, 56 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt 
> b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> new file mode 100644
> index 000..4b23834
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> @@ -0,0 +1,56 @@
> +The generic power sequence library
> +
> +Some hard-wired USB/MMC devices need to do power sequence to let the
> +device work normally, the typical power sequence like: enable USB
> +PHY clock, toggle reset pin, etc. But current Linux USB driver
> +lacks of such code to do it, it may cause some hard-wired USB devices
> +works abnormal or can't be recognized by controller at all. The
> +power sequence will be done before this device can be found at USB
> +bus.
> +
> +The power sequence properties is under the device node.
> +
> +Required properties:
> +- power-sequence: this device needs to do power sequence before enumeration
> +
> +Optional properties:
> +- clocks: the input clock for device.
> +- clock-name: must be "pwrseq-clk"

The "-clk" in the clock name is redundant.

> +- pwrseq-reset-gpios: Should specify the GPIO for reset.
> +- pwrseq-reset-duration-us: the duration in microsecond for assert reset 
> signal.

I understand you want to make it explicit that this GPIO is for the
pwrseq library, but are we really gaining anything over just calling
these reset-gpios and reset-duration-us?
The same applies to the clock name above.

> +Below is the example of USB power sequence properties on USB device
> +nodes which have two level USB hubs.
> +
> + {
> + vbus-supply = <_usb_otg1_vbus>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_usb_otg1_id>;
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + hub: genesys@1 {
> + compatible = "usb5e3,608";
> + reg = <1>;
> +
> + power-sequence;
> + clocks = < IMX6SX_CLK_CKO>;
> + clock-names = "pwrseq-clk";
> + pwrseq-reset-gpios = < 5 GPIO_ACTIVE_LOW>; /* hub reset 
> pin */
> + pwrseq-reset-duration-us = <10>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + ethernet: asix@1 {
> + compatible = "usbb95,1708";
> + reg = <1>;
> +
> + power-sequence;
> + clocks = < IMX6SX_CLK_IPG>;
> + clock-names = "pwrseq-clk";
> + pwrseq-reset-gpios = < 6 GPIO_ACTIVE_LOW>; /* 
> ethernet_rst */
> + pwrseq-reset-duration-us = <15>;
> + };

This looks weird. The hub and ethernet chips don't have "pwrseq" clock
and reset input pins. I'd remove the clock-names and pwrseq- reset
prefix.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] reset: Supply *_shared variant calls when using *_optional APIs

[Philipp Zabel]

Hi Lee,

On Wed, Jun 29, 2016 at 10:37:34AM +0100, Lee Jones wrote:
[...]
> > > > If that is not the case, go ahead. Otherwise I could provide
> > > > a tag at commit 2485394d9e0b ("reset: TRIVIAL: Add line break at same
> > > > place for similar APIs") for you to merge.
> > > 
> > > Yes please.  If you could tag a branch containing commit:
> > > 
> > >   reset: Supply *_shared variant calls when using *_optional APIs
> > > 
> > > ... that would be perfect.  Would you mind sending me the tag name
> > > ASAP please, since I am ready to send my pull-request to Linus.
> > 
> > git://git.pengutronix.de/git/pza/linux.git tags/reset-shared-optional
> 
> Ah!  If I rebase on that tag, I will also take in all of the patches
> between it and v4.7-rc1, which is 14 commits.  Is there any way you
> can place it onto it's own branch, then we both merge it into our
> respective trees?

I have now moved your patches into their own branch at
git://git.pengutronix.de/git/pza/linux.git tags/reset-explicit-api
and merged that back in.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] reset: Supply *_shared variant calls when using *_optional APIs

[Philipp Zabel]

Am Mittwoch, den 29.06.2016, 09:06 +0100 schrieb Lee Jones:
> On Wed, 29 Jun 2016, Philipp Zabel wrote:
> > Am Dienstag, den 28.06.2016, 09:56 +0100 schrieb Lee Jones:
> > > Philipp,
> > > 
> > > I need this to go into the -rcs too.
> > > 
> > > Can I add it with your Ack please?
> > 
> > I have already added your patches to my branch, and I intend to send a
> > pull request for it tomorrow. I am afraid applying this patch in another
> > branch would create a merge conflict with
> > git://git.pengutronix.de/git/pza/linux.git reset/next
> > in arm-soc. If that is not the case, go ahead. Otherwise I could provide
> > a tag at commit 2485394d9e0b ("reset: TRIVIAL: Add line break at same
> > place for similar APIs") for you to merge.
> 
> Yes please.  If you could tag a branch containing commit:
> 
>   reset: Supply *_shared variant calls when using *_optional APIs
> 
> ... that would be perfect.  Would you mind sending me the tag name
> ASAP please, since I am ready to send my pull-request to Linus.

git://git.pengutronix.de/git/pza/linux.git tags/reset-shared-optional

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] reset: Supply *_shared variant calls when using *_optional APIs

[Philipp Zabel]

Hi Lee,

Am Dienstag, den 28.06.2016, 09:56 +0100 schrieb Lee Jones:
> Philipp,
> 
> I need this to go into the -rcs too.
> 
> Can I add it with your Ack please?

I have already added your patches to my branch, and I intend to send a
pull request for it tomorrow. I am afraid applying this patch in another
branch would create a merge conflict with
git://git.pengutronix.de/git/pza/linux.git reset/next
in arm-soc. If that is not the case, go ahead. Otherwise I could provide
a tag at commit 2485394d9e0b ("reset: TRIVIAL: Add line break at same
place for similar APIs") for you to merge.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] reset: Reorder inline reset_control_get*() wrappers

[Philipp Zabel]

Am Montag, den 06.06.2016, 16:56 +0100 schrieb Lee Jones:
> We're about to split the current API into two, where consumers will
> be forced to be explicit when requesting reset lines.  The choice
> will be to either the call the *_exclusive or *_shared variant
> depending on whether they can actually tolorate not being asserted
> when that request is made.
> 
> The new API will look like this once reorded and complete:
> 
>   reset_control_get_exclusive()
>   reset_control_get_shared()
>   reset_control_get_optional_exclusive()
>   reset_control_get_optional_shared()
>   of_reset_control_get_exclusive()
>   of_reset_control_get_shared()
>   of_reset_control_get_exclusive_by_index()
>   of_reset_control_get_shared_by_index()
>   devm_reset_control_get_exclusive()
>   devm_reset_control_get_shared()
>   devm_reset_control_get_optional_exclusive()
>   devm_reset_control_get_optional_shared()
>   devm_reset_control_get_exclusive_by_index()
>   devm_reset_control_get_shared_by_index()
> 
> Signed-off-by: Lee Jones 

I have applied patches 1-5 to my reset/next branch.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] usb: dwc3: st: Inform the reset framework that our reset line may be shared

[Philipp Zabel]

Am Montag, den 06.06.2016, 16:56 +0100 schrieb Lee Jones:
> On the STiH410 B2120 development board the MiPHY28lp shares its reset
> line with the Synopsys DWC3 SuperSpeed (SS) USB 3.0 Dual-Role-Device
> (DRD).  New functionality in the reset subsystems forces consumers to
> be explicit when requesting shared/exclusive reset lines.
> 
> Signed-off-by: Lee Jones 
> ---
>  drivers/usb/dwc3/dwc3-st.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> index 5c0adb9..e77bacb 100644
> --- a/drivers/usb/dwc3/dwc3-st.c
> +++ b/drivers/usb/dwc3/dwc3-st.c
> @@ -227,7 +227,8 @@ static int st_dwc3_probe(struct platform_device *pdev)
>   dev_vdbg(>dev, "glue-logic addr 0x%p, syscfg-reg offset 0x%x\n",
>dwc3_data->glue_base, dwc3_data->syscfg_reg_off);
>  
> - dwc3_data->rstc_pwrdn = devm_reset_control_get(dev, "powerdown");
> + dwc3_data->rstc_pwrdn =
> + devm_reset_control_get_exclusive(dev, "powerdown");

This hunk is not critical. If you split it into a separate patch, this
one doesn't depend on patch 2. The same applies to patch 8.
That way I could take patches 1-5 through reset/next and you wouldn't
have to wait for them to be merged into arm-soc.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [STLinux Kernel] [PATCH 0/7] reset: Consumers to explicitly request 'exclusive' or 'shared' lines

[Philipp Zabel]

Hi Lee,

Am Dienstag, den 07.06.2016, 10:24 +0100 schrieb Lee Jones:
[...]
> I guess it should really go into the -rcs, yes.  Since Hans' patch
> actually breaks a lot of devices.  I'm pretty surprised a patch
> capable of this much damage was actually accepted to be honest.

I wasn't aware there were more drivers already sharing resets, and
arguably before Hans' patches this wasn't really supported at all.
It only worked by circumstance.

> A better approach would have been to issue a warning, but keep the
> semantics the same for at least a couple of releases.  However, I
> guess the damage has been done now, so let's do what we can do fix
> it.

Agreed.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] reset: Return -ENOTSUPP when not configured

[Philipp Zabel]

Hi John,

Am Dienstag, den 31.05.2016, 16:55 -0700 schrieb John Youn:
> Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo]
> functions wrappers"), the "optional" functions returned -ENOTSUPP when
> CONFIG_RESET_CONTROLLER was not set.
> 
> Revert back to the old behavior by changing the new
> __devm_reset_control_get() and __of_reset_control_get() functions to
> return ERR_PTR(-ENOTSUPP) when compiled without CONFIG_RESET_CONTROLLER.
> 
> Otherwise they will return -EINVAL causing users to think that an error
> occurred when CONFIG_RESET_CONTROLLER is not set.
> 
> Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions 
> wrappers")
> Signed-off-by: John Youn 
> ---
> 
> Hi Philipp,
> 
> Here are the changes as discussed previously. It just returns
> -ENOTSUPP directly when not configured. Tested and works with DWC2
> driver with Dinh's patch that adds the reset controller, using a
> configuration with no reset controller configured.

Thank you, I've applied the patch to my reset/next branch.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] reset: Put back *_optional variants

[Philipp Zabel]

Am Montag, den 30.05.2016, 13:32 +0200 schrieb Hans de Goede:
> Hi,
> 
> On 30-05-16 12:18, Philipp Zabel wrote:
> > Hi,
> >
> > Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede:
> > [...]
> >>>> So IMHO the following change would be a better way to fix this:
> >>>>
> >>>> --- a/include/linux/reset.h
> >>>> +++ b/include/linux/reset.h
> >>>> @@ -65,14 +65,14 @@ static inline struct reset_control 
> >>>> *__of_reset_control_get(
> >>>>  struct device_node *node,
> >>>>  const char *id, int index, int 
> >>>> shared)
> >>>>   {
> >>>> -   return ERR_PTR(-EINVAL);
> >>>> + return ERR_PTR(-ENOTSUPP);
> >>>>   }
> >>>>
> >>>>   static inline struct reset_control *__devm_reset_control_get(
> >>>>  struct device *dev,
> >>>>  const char *id, int index, int 
> >>>> shared)
> >>>>   {
> >>>> -   return ERR_PTR(-EINVAL);
> >>>> + return ERR_PTR(-ENOTSUPP);
> >>>>   }
> >>>>
> >>>>   #endif /* CONFIG_RESET_CONTROLLER */
> >>>
> >>>
> >>> I'm good with this. However, per Philipp on a previous thread, the
> >>> intended behavior is to return -EINVAL for the non-optional functions.
> >>>
> >>> http://marc.info/?l=linux-usb=146156945528848=2
> >
> > Adding Dinh to Cc: because he wanted this changed from -EINVAL.
> > My point then was that WARN_ON + -EINVAL is indented in this case.

No idea how this happened, but I meant "intended", obviously :)

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] reset: Put back *_optional variants

[Philipp Zabel]

Hi,

Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede:
[...]
> >> So IMHO the following change would be a better way to fix this:
> >>
> >> --- a/include/linux/reset.h
> >> +++ b/include/linux/reset.h
> >> @@ -65,14 +65,14 @@ static inline struct reset_control 
> >> *__of_reset_control_get(
> >>  struct device_node *node,
> >>  const char *id, int index, int 
> >> shared)
> >>   {
> >> -   return ERR_PTR(-EINVAL);
> >> + return ERR_PTR(-ENOTSUPP);
> >>   }
> >>
> >>   static inline struct reset_control *__devm_reset_control_get(
> >>  struct device *dev,
> >>  const char *id, int index, int 
> >> shared)
> >>   {
> >> -   return ERR_PTR(-EINVAL);
> >> + return ERR_PTR(-ENOTSUPP);
> >>   }
> >>
> >>   #endif /* CONFIG_RESET_CONTROLLER */
> >
> >
> > I'm good with this. However, per Philipp on a previous thread, the
> > intended behavior is to return -EINVAL for the non-optional functions.
> >
> > http://marc.info/?l=linux-usb=146156945528848=2

Adding Dinh to Cc: because he wanted this changed from -EINVAL.
My point then was that WARN_ON + -EINVAL is indented in this case.

Given that the warning backtrace already helps to identify the issue
(CONFIG_RESET_CONTROLLER disabled), and that changing the return value
to -ENOTSUPP improves consistency with the rest of the API and reduces
the amount of inline wrappers, I concur.

> As Philipp rightfully points out, calling the non-optional functions
> without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to
> care too much about the error code in that case, as long as we return
> an error.
> 
> Also note that even before my patch things were already inconsistent
> with the of_...get... functions already always returning
> -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems
> best and also KISS to just return -ENOTSUPP from all get functions
> when CONFIG_RESET_CONTROLLER is not set.
> 
> Anyways this is Philipp's call, this is all just my 2 cents.

Let the stubs return -ENOTSUPP consistently. A quick grep doesn't show
any driver handling -EINVAL explicitly either.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Philipp Zabel]

Hi Thierry,

Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding:
> From: Thierry Reding <tred...@nvidia.com>
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [3.365019] [ cut here ]
> [3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 
> __of_reset_control_get+0x16c/0x23c
> [3.382151] Modules linked in:
> [3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
> 4.6.0-rc6-next-20160503 #140
> [3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [3.399046] [] (unwind_backtrace) from [] 
> (show_stack+0x10/0x14)
> [3.406787] [] (show_stack) from [] 
> (dump_stack+0x90/0xa4)
> [3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100)
> [3.420964] [] (__warn) from [] 
> (warn_slowpath_null+0x20/0x28)
> [3.428525] [] (warn_slowpath_null) from [] 
> (__of_reset_control_get+0x16c/0x23c)
> [3.437648] [] (__of_reset_control_get) from [] 
> (tegra_ehci_probe+0x394/0x518)
> [3.446600] [] (tegra_ehci_probe) from [] 
> (platform_drv_probe+0x4c/0xb0)
> [3.455029] [] (platform_drv_probe) from [] 
> (driver_probe_device+0x1ec/0x330)
> [3.463892] [] (driver_probe_device) from [] 
> (__driver_attach+0xb8/0xbc)
> [3.472320] [] (__driver_attach) from [] 
> (bus_for_each_dev+0x68/0x9c)
> [3.480489] [] (bus_for_each_dev) from [] 
> (bus_add_driver+0x1a0/0x218)
> [3.488743] [] (bus_add_driver) from [] 
> (driver_register+0x78/0xf8)
> [3.496738] [] (driver_register) from [] 
> (do_one_initcall+0x40/0x170)
> [3.504909] [] (do_one_initcall) from [] 
> (kernel_init_freeable+0x158/0x1f8)
> [3.513600] [] (kernel_init_freeable) from [] 
> (kernel_init+0x8/0x114)
> [3.521770] [] (kernel_init) from [] 
> (ret_from_fork+0x14/0x3c)
> [3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel <p.za...@pengutronix.de>
> Cc: Hans de Goede <hdego...@redhat.com>
> Signed-off-by: Thierry Reding <tred...@nvidia.com>
> ---
> Changes in v3:
> - reword commit message to more accurately describe the hardware design

Reviewed-by: Philipp Zabel <p.za...@pengutronix.de>

> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..8396b622f238 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct 
> platform_device *pdev)
>   struct usb_hcd *hcd = platform_get_drvdata(pdev);
>   struct tegra_ehci_hcd *tegra =
>   (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> + bool has_utmi_pad_registers = false;
>  
>   phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>   if (!phy_np)
>   return -ENOENT;
>  
> + if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> + has_utmi_pad_registers = true;
> +
>   if (!usb1_reset_attempted) {
>   struct reset_con

Re: [PATCHv6 2/2] usb: dwc2: Add reset control to dwc2

[Philipp Zabel]

Hi John,

Am Freitag, den 22.04.2016, 20:31 -0700 schrieb John Youn:
> On 4/20/2016 2:31 PM, dingu...@opensource.altera.com wrote:
> > From: Dinh Nguyen 
> > 
> > Allow for platforms that have a reset controller driver in place to bring
> > the USB IP out of reset.
> > 
> > Signed-off-by: Dinh Nguyen 
> > ---
> > v6: fix 80 line checkpatch warning in dev_err print
> > v5: updated error conditions for not finding the reset property
> > v4: use dev_dbg() if not a -EPROBE_DEFER
> > v3: fix compile error
> > v2: move to lowlevel_hw_init()
> > ---
> >  drivers/usb/dwc2/core.h |  1 +
> >  drivers/usb/dwc2/platform.c | 21 +
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index 3c58d63..f748132 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -837,6 +837,7 @@ struct dwc2_hsotg {
> > void *priv;
> > int irq;
> > struct clk *clk;
> > +   struct reset_control *reset;
> >  
> > unsigned int queuing_high_bandwidth:1;
> > unsigned int srp_success:1;
> > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> > index 88629be..fa2c097 100644
> > --- a/drivers/usb/dwc2/platform.c
> > +++ b/drivers/usb/dwc2/platform.c
> > @@ -45,6 +45,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  
> > @@ -337,6 +338,23 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg 
> > *hsotg)
> >  {
> > int i, ret;
> >  
> > +   hsotg->reset = devm_reset_control_get(hsotg->dev, "dwc2");
> > +   if (IS_ERR(hsotg->reset)) {
> > +   ret = PTR_ERR(hsotg->reset);
> > +   switch (ret) {
> > +   case -ENOENT:
> > +   hsotg->reset = NULL;
> > +   break;
> > +   default:
> > +   dev_err(hsotg->dev, "error getting reset control %d\n",
> > +   ret);
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   if (hsotg->reset)
> > +   reset_control_deassert(hsotg->reset);
> > +
> > /* Set default UTMI width */
> > hsotg->phyif = GUSBCFG_PHYIF16;
> >  
> > @@ -434,6 +452,9 @@ static int dwc2_driver_remove(struct platform_device 
> > *dev)
> > if (hsotg->ll_hw_enabled)
> > dwc2_lowlevel_hw_disable(hsotg);
> >  
> > +   if (hsotg->reset)
> > +   reset_control_assert(hsotg->reset);
> > +
> > return 0;
> >  }
> >  
> > 
> 
> Hi Dinh,
> 
> On my system, with no reset controller configured, this fails
> devm_reset_control_get() with -EINVAL, and it also spits out a
> backtrace.
>
> Hi Philipp,
> 
> I think devm_reset_control_get() should return either NULL or some
> other error to indicate that the reset controller is not
> configured. That way we can check it and handle that condition. If we
> check for -EINVAL, we might miss a legitimate error that is returning
> -EINVAL.
> 
> Also I think the WARN_ON(1) should be removed for this case.
> 
> Any thoughts on this or other suggestions to resolve it?

It is invalid to call (devm_)reset_control_get if the framework is
disabled, so that error code is correct. The only reason there are even
stubs for (devm_)reset_control_get is for compile tests. Either enable
CONFIG_RESET_CONTROLLER or use (devm_)reset_control_get_optional.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/2] ?hci-platform: Add support for controllers with more then one reset line

[Philipp Zabel]

Am Samstag, den 09.04.2016, 16:15 +0200 schrieb Hans de Goede:
> Hi Greg, et al,
> 
> Here is a resend of my last version of the patch set to support usb
> controllers which have multiple resets.
> 
> Unfortunately the shared reset controller support this depends on did
> not make it into 4.6. Philipp Zabel, the reset maintainer (in the Cc)
> has put these patches in a stable branch / tag here:
> 
> git://git.pengutronix.de/git/pza/linux.git tags/reset-for-4.7
> 
> Can you please merge in this tag and then these 2 patches for 4.7 ?

Acked-by: Philipp Zabel <p.za...@pengutronix.de>

Note that as of now, the tag is not yet merged into the arm-soc
maintainer's for-next branch. I've sent the pull request on Mar 31 and
expect it to be merged in the next round.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/1] USB: core: let USB device know device node

[Philipp Zabel]

Hi Peter,

Am Dienstag, den 16.02.2016, 16:42 +0800 schrieb Peter Chen:
> From: Peter Chen <peter.c...@freescale.com>
> 
> Although most of USB devices are hot-plug's, there are still some devices
> are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> If these kinds of USB devices are multiple functions, and they can supply
> other interfaces like i2c, gpios for other devices, we may need to
> describe these at device tree.
> 
> In this commit, it uses "reg" in dts as physical port number to match
> the phyiscal port number decided by USB core, if they are the same,
> then the device node is for the device we are creating for USB core.
> 
> Signed-off-by: Peter Chen <peter.c...@freescale.com>
> ---
> Changes for v5:
> - Refine the code how to get the device node at usb_alloc_dev according
>   to Alan's comment.
> - Point out "usbVID,PID" is the recommented compatible, and others
>   compatibles at binding doc could also be used.
> 
> Changes for v4:
> - The range of "reg" should be 1-31, changing device node address
>   style as in lower case hexadecimal with leading zeroes suppressed
>   [binding doc, usb-device.txt]
> - Improve the example at binding doc, it describes node from the top
>   (the controller)
> - Delete the struct of_node * within struct usb_device
> - Using usb_hcd_find_raw_port_number to get raw port number under root
>   hub port
> 
> Changes for v3:
> - typo: s/descirbe/describe/
> 
> Changes for v2:
> - Fix build error reported by kbuild robot, lack of "static" for
>   inline usb_of_get_child_node
> - Fix typo, "devcie_node" -> "device_node"
> - Add kernel-doc for of_node at struct usb_device
> 
> Changes from RFC:
> - Fix the error address for binding doc, and add compatible for binding doc
> - Change get child node API from "usb_of_find_node" to
>   "usb_of_get_child_node"
> - Delete unecessary header files
> - One typo
> 
>  .../devicetree/bindings/usb/usb-device.txt | 26 
>  drivers/usb/core/Makefile  |  2 +-
>  drivers/usb/core/of.c  | 47 
> ++
>  drivers/usb/core/usb.c | 10 +
>  include/linux/usb/of.h |  7 
>  5 files changed, 91 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
>  create mode 100644 drivers/usb/core/of.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt 
> b/Documentation/devicetree/bindings/usb/usb-device.txt
> new file mode 100644
> index 000..a997a23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -0,0 +1,26 @@
> +Generic USB Device Properties
> +
> +Usually, we only use device tree for hard wired USB device.
> +The reference binding doc is from:
> +http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> +
> +Required properties:
> +- compatible: usbVID,PID. The other compatible strings from the above
> +  standard binding could also be used, but a device adhering to this
> +  binding may leave out all except for usbVID,PID.

I asked to document the format of VID and PID ("lower case hexadecimal
with leading zeroes suppressed") here.

> +- reg: the port number which this device is connecting to, the range
> +  is 1-31.
> +
> +Example:
> +
> + {
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hub: genesys@1 {
> + compatible = "usb05e3,0608";

This should be:
compatible = "usb5e3,608";

After those two issues are fixed,
Acked-by: Philipp Zabel <p.za...@pengutronix.de>

best regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/1] USB: core: let USB device know device node

[Philipp Zabel]

Am Freitag, den 05.02.2016, 14:11 +0800 schrieb Peter Chen:
[...]
> > The reference recommendation states that for single-configuration USB
> > devices the compatible should contain all of the applicable strings from
> > the list starting with 2) "usbVID,PID.REV" and ending with 11)
> > "usb,device". Are we going to ignore this?
>
> I have not seen benefits if we write several compatibles in dts,
> the information of compatibles listed in doc can be got during
> the enumeration.
> 
> I suggest we use the simple pattern for this compatible, in that
> case, every one can be easy to follow it, and will not be confused
> which compatibles should be used, and the style can be unify.

Just pointing it out, a comment why this differs from the recommendation
would be nice to avoid confusion.

[...]
> > > + compatible = "usb05e3,0608";
> > > + reg = <0x1>;
> > > + };
> > 
> > I'd have written this node as:
> > 
> > hub: hub@1 {
> > compatible = "usb5e3,608", "usb5e3,class9",
> >  "usb,class9", "usb,device";
> > reg = <1>;
> > };
> 
> The reg should be hexadecimal, do we need to add "0x" before the value?

The unit-address name part should be hexadecimal, the reg property value
doesn't have to be. As long as the value is < 10 I don't see a problem.

> > As another example, I'd like to introduce the USB WLAN Adapter soldered
> > onto the imx6q-gk802 board to its power enable GPIO via the device tree:
> > 
> > /* Internal USB port (USBH1) */
> >  {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > status = "okay";
> > 
> > /* RTL8192CU 802.11n WLAN Adapter */
> > device@1 {
> > compatible = "usbbda,8176.200", "usbbda,8176",
> >  "usb,device";
> > reg = <1>;
> > 
> > enable-gpios = < 0 GPIO_ACTIVE_LOW>;
> > };
> > };
> > 
> 
> It is okay to use your example, but I still insist like below:

Sorry for being imprecise. The hub example is fine for the
documentation. I just wanted to illustrate how and why I am interested
in this discussion.

best regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/1] USB: core: let USB device know device node

[Philipp Zabel]

Hi Peter,

Am Montag, den 25.01.2016, 15:24 +0800 schrieb Peter Chen:
> Although most of USB devices are hot-plug's, there are still some devices
> are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> If these kinds of USB devices are multiple functions, and they can supply
> other interfaces like i2c, gpios for other devices, we may need to
> describe these at device tree.
> 
> In this commit, it uses "reg" in dts as physical port number to match
> the phyiscal port number decided by USB core, if they are the same,
> then the device node is for the device we are creating for USB core.
> 
> Signed-off-by: Peter Chen 
> ---
> Changes for v4:
> - The range of "reg" should be 1-31, changing device node address
>   style as in lower case hexadecimal with leading zeroes suppressed
>   [binding doc, usb-device.txt]
> - Improve the example at binding doc, it describes node from the top
>   (the controller)
> - Delete the struct of_node * within struct usb_device
> - Using usb_hcd_find_raw_port_number to get raw port number under root
>   hub port
> 
> Changes for v3:
> - typo: s/descirbe/describe/
> 
> Changes for v2:
> - Fix build error reported by kbuild robot, lack of "static" for
>   inline usb_of_get_child_node
> - Fix typo, "devcie_node" -> "device_node"
> - Add kernel-doc for of_node at struct usb_device
> 
> Changes from RFC:
> - Fix the error address for binding doc, and add compatible for binding doc
> - Change get child node API from "usb_of_find_node" to
>   "usb_of_get_child_node"
> - Delete unecessary header files
> - One typo
> 
>  .../devicetree/bindings/usb/usb-device.txt | 25 
>  drivers/usb/core/Makefile  |  2 +-
>  drivers/usb/core/of.c  | 47 
> ++
>  drivers/usb/core/usb.c | 14 ++-
>  include/linux/usb/of.h |  7 
>  5 files changed, 92 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
>  create mode 100644 drivers/usb/core/of.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt 
> b/Documentation/devicetree/bindings/usb/usb-device.txt
> new file mode 100644
> index 000..c702885
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -0,0 +1,25 @@
> +Generic USB Device Properties
> +
> +Usually, we only use device tree for hard wired USB device.
> +The reference binding doc is from:
> +http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> +
> +Required properties:
> +- compatible: usbVID,PID

The reference recommendation states that for single-configuration USB
devices the compatible should contain all of the applicable strings from
the list starting with 2) "usbVID,PID.REV" and ending with 11)
"usb,device". Are we going to ignore this?

The document also states that "the textual representation of VID, PID
[...] shall be in lower case hexadecimal with leading zeroes
suppressed". This should be documented here.

> +- reg: the port number which this device is connecting to, the range
> +  is 1-31.
> +
> +
> +Example:
> +
> + {
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hub: genesys@1 {

There are node names specified for hubs and mass storage devices:
"hub", "storage", "cdrom", "tape", "solid-state", and "device" for
everything else. Should we follow this recommendation?

> + compatible = "usb05e3,0608";
> + reg = <0x1>;
> + };

I'd have written this node as:

hub: hub@1 {
compatible = "usb5e3,608", "usb5e3,class9",
 "usb,class9", "usb,device";
reg = <1>;
};

As another example, I'd like to introduce the USB WLAN Adapter soldered
onto the imx6q-gk802 board to its power enable GPIO via the device tree:

/* Internal USB port (USBH1) */
 {
#address-cells = <1>;
#size-cells = <0>;
status = "okay";

/* RTL8192CU 802.11n WLAN Adapter */
device@1 {
compatible = "usbbda,8176.200", "usbbda,8176",
 "usb,device";
reg = <1>;

enable-gpios = < 0 GPIO_ACTIVE_LOW>;
};
};

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel]

Am Samstag, den 19.12.2015, 11:55 +0100 schrieb Hans de Goede:
> On 18-12-15 12:08, Maxime Ripard wrote:
[...]
> > I guess we could also have something like this:
> >
> >* The driver gets the reference to the reset line using
> >  reset_control_get or its shared variant.
> >
> >  - If you call reset_control_get on a free line, it succeeds, and
> >marks the line in exclusive use.
> >  - If you call reset_control_get on a busy line, it fails with
> >EBUSY
> >
> >  - If you call the shared variant on a free line, it succeeds
> >  - If you call the shared variant on a busy exclusive line, it
> >fails with EBUSY
> >  - If you call the shared variant on a busy !exclusive line, it
> >succeeds.
>>
> >* The customer driver can then call reset_control_assert / deassert:
> >
> >  - If the reset line is in exclusive use, the assertion happens
> >right away, it succeeds and returns 0.
> >
> >  - If the reset line is in a !exclusive use, but with a single
> >user, the assertion happens right away, it succeeds and returns
> >0.
>
> Ack for all of the above, this is what I had in mind at first, but I started
> with a more lightweight version as I'm lazy :)  If Philipp likes this
> suggestion I can rework my patch-set into this.

Seconded, this all sounds good to me.

> >  - If the reset line is in a !exclusive use with more than 1 user,
> >the refcount is modified and an error is returned to notify that
> >it didn't happen.
>
> Also ack, except for returning the error, if a driver has used
> reset_control_get_shared, it should simply be aware that doing an assert
> might not necessarily actually assert the line, just like doing a clk-disable
> does not really necessary disable the clock, etc. If a driver is not prepared
> to deal with this, it should simply not use reset_control_get_shared.
>
> I see returning an error if the assert did not happen due to other users /
> deassert_count != 0 as inconsistent compared to how clks, regulators and phys
> handle this, these all simply return success in this case.

I wouldn't want drivers to have to differentiate between relevant and
irrelevant error codes, so in the clock-like refcounting use case
reset_assert should not return an error if it just correctly decremented
the refcount. I'd still prefer to have separate API for the counted
must_deassert/may_assert vs the exclusive must_assert/must_deassert use
cases, but I just can't think of a good name.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel]

Hi Maxime,

Am Mittwoch, den 16.12.2015, 11:29 +0100 schrieb Maxime Ripard:
> On Mon, Dec 14, 2015 at 10:50:55AM +0100, Philipp Zabel wrote:
> > Am Montag, den 14.12.2015, 10:36 +0100 schrieb Maxime Ripard:
> > > Hi,
> > > 
> > > On Fri, Dec 11, 2015 at 04:41:58PM +0100, Hans de Goede wrote:
> > > > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > > > index c4c097d..1cca8ce 100644
> > > > --- a/include/linux/reset.h
> > > > +++ b/include/linux/reset.h
> > > > @@ -11,6 +11,8 @@ int reset_control_reset(struct reset_control *rstc);
> > > >  int reset_control_assert(struct reset_control *rstc);
> > > >  int reset_control_deassert(struct reset_control *rstc);
> > > >  int reset_control_status(struct reset_control *rstc);
> > > > +int reset_control_assert_shared(struct reset_control *rstc);
> > > > +int reset_control_deassert_shared(struct reset_control *rstc);
> > > 
> > > Shouldn't that be handled in reset_control_get directly?

I think I see your point now. Maybe we should add a flags parameter to
reset_control_get and/or wrap it in two versions,
reset_control_get_exclusive and reset_control_get_shared (or just add
the _shared variant). Then reset_control_get(_exclusive) could return
-EBUSY if a reset line is already in use.

> > This is about different expectations of the caller.
> > A driver calling reset_control_assert expects the reset line to be
> > asserted after the call.
> 
> Is that behaviour documented explicitly somewhere?

/** 
  
 * reset_control_assert - asserts the reset line
 * @rstc: reset controller
 */

Also, that expected behavior matches the function name, which I like.
So I still welcome adding new API calls for the shared/refcounting
variant.

> > A driver calling reset_control_assert_shared
> > just signals that it doesn't care about the state of the reset line
> > anymore.
> > We could just as well call the two new functions
> > reset_control_deassert_get and reset_control_deassert_put.
> 
> What happens if you mix them? What happens if you have several drivers
> ignoring this API?

The core should give useful error messages and disallow non-shared reset
calls on shared lines.

> The current default API totally allows to have several drivers getting
> the same reset line, and happily poking that reset line without any
> way for the others to A) know they're not the single users B) let them
> know their device has been reset.

That's why I'd like the WARN_ON and error return in reset_control_* when
the reset_line reference count is > 1.

> And not being able to tell at the consumer level if and when our
> device is going to be reset behind our back is a big issue. Because
> then, we simply have to expect it can be reset at any point in time,
> good luck writing a driver with that expectation.

Yes, that is unacceptable.

> The reset framework should make sure that the shared case is an
> exception, and not the default case (and make sure that it cannot
> happen in the default case). Just like for any other framework with
> similar resources constraints.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel]

Hi Hans,

Am Freitag, den 11.12.2015, 19:21 +0100 schrieb Hans de Goede:
[...]
> >> @@ -119,13 +134,55 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
> >>   int reset_control_deassert(struct reset_control *rstc)
> >>   {
> >
> > Maybe WARN_ON(rstc->line->refcnt > 1) ?
> 
> I assume you mean deasser_cnt ? Seems reasonable with that change.

I meant refcnt. Currently drivers sharing reset lines (refcnt > 1)
and then using the non-shared reset control functions are bound to cause
unexpected behaviour. Now we can detect this for the first time.

> >>if (rstc->rcdev->ops->deassert)
> >> -  return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
> >> +  return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->line->id);
> >>
> >>return -ENOTSUPP;
> >>   }
> >>   EXPORT_SYMBOL_GPL(reset_control_deassert);
> >>
> >>   /**
> >> + * reset_control_assert_shared - asserts a shared reset line
> >> + * @rstc: reset controller
> >> + *
> >> + * Assert a shared reset line, this functions decreases the deassert count
> >> + * of the line by one and asserts it if, and only if, the deassert count
> >> + * reaches 0.
> >
> > "After calling this function the shared reset line may be asserted, or
> >   it may still be deasserted, as long as other users keep it so."
> 
> I take it this is to replace the text about "deassert count" ?

I thought you might append something like it. I just imagine that when
reading the documentation it might be helpful to also see the intended
effect described, especially given that a call to an _assert_ function
might leave the reset line in deasserted state, depending on the
refcount.

> >> + */
> >> +int reset_control_assert_shared(struct reset_control *rstc)
> >> +{
> >> +  if (!rstc->rcdev->ops->assert)
> >> +  return -ENOTSUPP;
> >
> > WARN_ON(rstc->line->deassert_cnt == 0)
> >
> > Actually, what to do in this case? Assume ops->assert was called, or do
> > it again to be sure? Certainly we don't want to wrap deassert_cnt, or
> > the next deassert_shared will do nothing.
> >
> >> +  rstc->line->deassert_cnt--;
> >> +  if (rstc->line->deassert_cnt)
> >
> > deassert_cnt isn't protected by any lock.
> 
> Right, good catch. I believe the best way to fix this is to change 
> deassert_cnt
> into an atomic_t and use atomic_dec_return / atomic_int_return,

Yes, that would work.

> Downside of using an atomic_t is that doing the WARN_ON you are asking for 
> above
> will not be race free, then, but since it is a should never happen scenario I
> guess we do not care about the check not being 100% race free. Or we can even
> just leave out the check ?

Since this is only a warning to notify driver developers we don't
support shared resets (apart from the clock-like use)

Not if we warn about refcnt instead of deassert_cnt above.

[...]
> >> + * of the line by one and deasserts the reset line (if it was not already
> >> + * deasserted).
> >
> > "After calling this function, the shared reset line is guaranteed to be
> >   deasserted."
> 
> Same question as above.

Same imprecise answer. I'd like to see the expected state after calling
this function in the description, in addition to the mechanism. This is
more important for the assert function. After calling deassert, the
reset line is deasserted, no reason to be surprised about that.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Am Freitag, den 11.12.2015, 19:28 +0100 schrieb Hans de Goede:
[...]
> >> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt 
> >> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >> index a12d601..0701812 100644
> >> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >> @@ -18,7 +18,7 @@ Optional properties:
> >>- clocks : a list of phandle + clock specifier pairs
> >>- phys : phandle + phy specifier pair
> >>- phy-names : "usb"
> >> - - resets : phandle + reset specifier pair
> >> + - resets : a list of phandle + reset specifier pairs
> >
> > Are there documented names for these resets?
> 
> This binding is a generic ehci controller binding, so even if
> the names are documented for the allwinner SoC we should
> not use names, just like the binding is deliberately not
> using names for the clocks either to keep it generic, so
> that we can reuse the binding + driver with many different SoCs.

I know, I'm just interested in understanding why this is necessary ...

> > Is the companion you
> > mention the Port Control?
> 
> Sort of, with USB-2, USB-1 compatibility is handled via a mux on the
> datalines (controlled by the EHCI controller Port Control) which muxes
> the lines to an USB-1 controller (typically either UHCI or OHCI) when the
> device does not connect after USB-2 highspeed handshaking.
> 
> This USB-1 controller (or controller_S_ in some case since the
> USB-1 companions may have less root-ports per controller then the EHCI
> has root-ports) is called the companion controller.
> 
> The 2 controllers are supposed to be 100% independent but on the H3
> Allwinner has done something (not documented) which requires one to
> deassert reset on both before you can talk to either one.

... so thank you for the explanation.

Acked-by: Philipp Zabel <p.za...@pengutronix.de>

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel]

Am Montag, den 14.12.2015, 10:36 +0100 schrieb Maxime Ripard:
> Hi,
> 
> On Fri, Dec 11, 2015 at 04:41:58PM +0100, Hans de Goede wrote:
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index c4c097d..1cca8ce 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -11,6 +11,8 @@ int reset_control_reset(struct reset_control *rstc);
> >  int reset_control_assert(struct reset_control *rstc);
> >  int reset_control_deassert(struct reset_control *rstc);
> >  int reset_control_status(struct reset_control *rstc);
> > +int reset_control_assert_shared(struct reset_control *rstc);
> > +int reset_control_deassert_shared(struct reset_control *rstc);
> 
> Shouldn't that be handled in reset_control_get directly?

This is about different expectations of the caller.
A driver calling reset_control_assert expects the reset line to be
asserted after the call. A driver calling reset_control_assert_shared
just signals that it doesn't care about the state of the reset line
anymore.
We could just as well call the two new functions
reset_control_deassert_get and reset_control_deassert_put.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Am Freitag, den 11.12.2015, 16:41 +0100 schrieb Hans de Goede:
> From: Reinder de Haan 
> 
> At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple
> reset lines, the controller will not initialize while the reset for
> its companion is still asserted, which means we need to de-assert
> 2 resets for the controller to work.
> 
> Signed-off-by: Reinder de Haan 
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Use the new reset_control_[de]assert_shared reset-controller functions
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  2 +-
>  drivers/usb/host/ehci-platform.c   | 47 
> +-
>  2 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt 
> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index a12d601..0701812 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -18,7 +18,7 @@ Optional properties:
>   - clocks : a list of phandle + clock specifier pairs
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
> - - resets : phandle + reset specifier pair
> + - resets : a list of phandle + reset specifier pairs

Are there documented names for these resets? Is the companion you
mention the Port Control?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants

[Philipp Zabel]

Hi Hans,

thanks for moving this forward.

Am Freitag, den 11.12.2015, 16:41 +0100 schrieb Hans de Goede:
> Add reset_control_deassert_shared / reset_control_assert_shared
> functions which are intended for use by drivers for hw blocks which
> (may) share a reset line with another driver / hw block.
> 
> Unlike the regular reset_control_[de]assert functions these functions
> keep track of how often deassert_shared / assert_shared have been called
> and keep the line deasserted as long as deassert has been called more
> times than assert.
>
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/reset/core.c | 121 
> ---
>  include/linux/reset-controller.h |   2 +
>  include/linux/reset.h|   2 +
>  3 files changed, 116 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 9ab9290..8c3436c 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -22,16 +22,29 @@ static DEFINE_MUTEX(reset_controller_list_mutex);
>  static LIST_HEAD(reset_controller_list);
>  
>  /**
> + * struct reset_line - a reset line
> + * @list: list entry for the reset controllers reset line list
> + * @id:   ID of the reset line in the reset controller device
> + * @refcnt:   Number of reset_control structs referencing this device
> + * @deassert_cnt: Number of times this reset line has been deasserted
> + */
> +struct reset_line {
> + struct list_head list;
> + unsigned int id;
> + unsigned int refcnt;
> + unsigned int deassert_cnt;
> +};

I'd move rcdev into reset_line, too. That way the description is
complete, and we don't duplicate rcdev when there are multiple
reset_controls pointing here.

> +/**
>   * struct reset_control - a reset control
>   * @rcdev: a pointer to the reset controller device
>   * this reset control belongs to
> - * @id: ID of the reset controller in the reset
> - *  controller device
> + * @line:  reset line for this reset control
>   */
>  struct reset_control {
>   struct reset_controller_dev *rcdev;
> + struct reset_line *line;
>   struct device *dev;
> - unsigned int id;
>  };
>  
>  /**
[...]
> @@ -119,13 +134,55 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
>  int reset_control_deassert(struct reset_control *rstc)
>  {

Maybe WARN_ON(rstc->line->refcnt > 1) ?

>   if (rstc->rcdev->ops->deassert)
> - return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
> + return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->line->id);
>  
>   return -ENOTSUPP;
>  }
>  EXPORT_SYMBOL_GPL(reset_control_deassert);
>  
>  /**
> + * reset_control_assert_shared - asserts a shared reset line
> + * @rstc: reset controller
> + *
> + * Assert a shared reset line, this functions decreases the deassert count
> + * of the line by one and asserts it if, and only if, the deassert count
> + * reaches 0.

"After calling this function the shared reset line may be asserted, or
 it may still be deasserted, as long as other users keep it so."

> + */
> +int reset_control_assert_shared(struct reset_control *rstc)
> +{
> + if (!rstc->rcdev->ops->assert)
> + return -ENOTSUPP;

WARN_ON(rstc->line->deassert_cnt == 0)

Actually, what to do in this case? Assume ops->assert was called, or do
it again to be sure? Certainly we don't want to wrap deassert_cnt, or
the next deassert_shared will do nothing.

> + rstc->line->deassert_cnt--;
> + if (rstc->line->deassert_cnt)

deassert_cnt isn't protected by any lock.

> + return 0;
> +
> + return rstc->rcdev->ops->assert(rstc->rcdev, rstc->line->id);
> +}
> +EXPORT_SYMBOL_GPL(reset_control_assert_shared);
> +
> +/**
> + * reset_control_deassert_shared - deasserts a shared reset line
> + * @rstc: reset controller
> + *
> + * Assert a shared reset line, this functions increases the deassert count

Deassert

> + * of the line by one and deasserts the reset line (if it was not already
> + * deasserted).

"After calling this function, the shared reset line is guaranteed to be
 deasserted."

> + */
> +int reset_control_deassert_shared(struct reset_control *rstc)
> +{
> + if (!rstc->rcdev->ops->deassert)
> + return -ENOTSUPP;
> +
> + rstc->line->deassert_cnt++;
> + if (rstc->line->deassert_cnt != 1)
> + return 0;
> +
> + return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->line->id);
> +}
> +EXPORT_SYMBOL_GPL(reset_control_deassert_shared);
> +
> +/**
>   * reset_control_status - returns a negative errno if not supported, a
>   * positive value if the reset line is asserted, or zero if the reset
>   * line is not asserted.
> @@ -134,12 +191,47 @@ EXPORT_SYMBOL_GPL(reset_control_deassert);
>  int reset_control_status(struct reset_control *rstc)
>  {
>   if (rstc->rcdev->ops->status)
> - return 

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Philipp Zabel]

Hi Peter,

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> Add dt-binding documentation for generic onboard USB HUB.
> 
> Signed-off-by: Peter Chen 
> ---
>  .../bindings/usb/generic-onboard-hub.txt   | 28 
> ++
>  1 file changed, 28 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt 
> b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> new file mode 100644
> index 000..ea92205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> @@ -0,0 +1,28 @@
> +Generic Onboard USB HUB
>+
> +Required properties:
> +- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB
controller, right? The "Open Firmware recommended practice: USB"
document already describes how to represent USB devices in a generic
manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it
could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */


> +Optional properties:
> +- clocks: the input clock for HUB.
> +
> +- clock-names: Should be "external_clk"

Is clock-names necessary for a single clock?

> +- hub-reset-gpios: Should specify the GPIO for reset.

I'd prefer that to be just "reset-gpios", it is the only reset property
in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to
indicate polarity.

> +- hub-reset-active-high: the active reset signal is high, if this property is
> +  not set, the active reset signal is low.

Then this could be dropped.

> +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> +  is microsecond.

And consequently, this could just be called "reset-duration-us".
It might make sense to define the same for other reset GPIOs, too.
The Freescale FEC, for example, has "phy-reset-duration" (in ms)
already.

> +
> +Example:
> +
> + usb_hub1 {
> + compatible = "generic-onboard-hub";
> + clocks = < IMX6QDL_CLK_CKO>;
> + clock-names = "external_clk";
> + hub-reset-gpios = < 12 0>;
> + hub-reset-active-high;
> + hub-reset-duration-us = <2>;
> + };

best regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Udoo support for chipidea

[Philipp Zabel]

Am Freitag, den 20.11.2015, 16:05 +0800 schrieb Peter Chen:
> On Thu, Nov 19, 2015 at 11:26:50AM -0500, Alan Stern wrote:
> > On Thu, 19 Nov 2015, Philipp Zabel wrote:
> > 
> > > On Wed, Oct 21, 2015 at 10:39:00AM +0800, Peter Chen wrote:
> > > > On Tue, Oct 20, 2015 at 02:09:38PM -0200, Fabio Estevam wrote:
> > > > > Hi Peter,
> > > > > 
> > > > > On Mon, Oct 19, 2015 at 12:50 AM, Peter Chen 
> > > > > <peter.c...@freescale.com> wrote:
> > > > > 
> > > > > > Add linux-usb.
> > > > > >
> > > > > > Patryk, your problem is you may need to open 24M OSC for HUB 2514
> > > > > > manually, and you have used IMX6QDL_CLK_CKO for it in the design,
> > > > > > but this clock is not controller's clock, controller's clock has
> > > > > > already decided at SoC dts file (imx6qdl.dtsi), you don't need to
> > > > > > override it at board's dts file.
> > > > > >
> > > > > > You can try delete clock property at imx6qdl-udoo.dtsi, if it still
> > > > > > can't work, try to open IMX6QDL_CLK_CKO at one place to test.
> > > > > 
> > > > > What is the appropriate place to acquire and enable the USB hub clock?
> > > > > 
> > > > > This issue has appeared several times and it seems we don't have a
> > > > > solution for this yet.
> > > > > 
> > > > > Any suggestions?
> > > > 
> > > > Add Alan.
> > > > 
> > > > Hi Alan, we have several designs that the on-board HUB need to
> > > > be reset by gpio pin and its clock is also from the board or
> > > > the SoC. Any suggestions how to add these platform information
> > > > for HUB device?
> > > 
> > > How about putting it in the device tree?
> > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > clocks and reset-gpios properties could be added to the USB hub node.
> > 
> > Something like this is necessary.  Instead of making the hub driver
> > take care of the reset gpio and the clock, I suggest you make the host
> > controller's platform driver do these things.
> > 
> > This is because USB hubs are generic devices, not specific to any 
> > platform and (usually) hot-pluggable.  Associating platform-specific 
> > data with a hub is out of the ordinary, and it deserves to be handled 
> > by platform-specific code -- there is no such code in the hub driver.
> > 
> If this on-board HUB is special, its reset pin needs to be toggled
> after power on, no matter for Intel or ARM platforms, how we handle
> it? It is not feasible we add this for all platform drivers.

Further whether the GPIO reset line is initially in asserted state and
just needs to change to deasserted, or whether the GPIO reset line is
initially in deasserted state and needs to be pulsed for a certain time
is probably hub IC specific.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Am Mittwoch, den 18.11.2015, 21:20 +1100 schrieb Julian Calaby:
> Hi All,
> 
> On Wed, Nov 18, 2015 at 9:18 PM, Maxime Ripard
> <maxime.rip...@free-electrons.com> wrote:
> > On Wed, Nov 18, 2015 at 10:46:52AM +0100, Philipp Zabel wrote:
> >> Hi Hans,
> >>
> >> Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede:
> >> > On 16-11-15 18:01, Philipp Zabel wrote:
> >> > > If there are two devices sharing the same reset line that is initially
> >> > > held asserted, do the two drivers somehow have to synchronize before
> >> > > releasing the reset together?
> >> >
> >> > Not to my knowledge, I suggest that we simply treat this same as
> >> > regulators / clocks where the first one to enable it actually gets
> >> > it enabled (de-asserted in case of a reset), and the last one
> >> > to disable (assert) it (so dropping the usage counter back to 0) leads
> >> > to it being asserted again.
> >> >
> >> > This seems to work well enough for clocks / regulators and phys, and
> >> > at least for my use-case it should work fine for the shared reset too
> >> > (I will test to make sure of course).
> >> >
> >> > So from my pov a simple counter should suffice, does that work for you ?
> >>
> >> I don't quite understand what counting will help with, then. The first
> >> driver to call reset_control_deassert will deassert the reset, whether
> >> you count or not.
> >> But if the two drivers have deasserted an initially asserted reset, a
> >> reset_control_assert for one of them will silently fail.
> >
> > Then maybe we can just make it return an error when someone calls
> > _assert or _reset on a reset line that has more than one user?
>
> Just to set another cat amongst the pigeons: What if a driver needs to
> toggle the shared reset line of some IP block to get it out of some
> broken state?

We'd need driver support for that. Possibly using notifications?

In the hypothetical case of two GPUs sharing the same reset line, one
currently currently busy processing a command stream, and the other one
stuck, the driver for the stuck one would request a reset, the framework
would have to give notice to the other driver, allow it to veto or block
the reset until it has parked the hardware and stored state.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Hi Julian,

Am Mittwoch, den 18.11.2015, 23:32 +1100 schrieb Julian Calaby:
[...]
> Assuming board designers are sufficiently ... stupid, won't most
> drivers eventually need to use the _shared variants? Wouldn't it be
> (horrifically more complicated and) better to just build this into the
> generic code and switch on the shared reset line handling if multiple
> devices take references to the same reset line? (Can we run through
> the device tree and mark them in advance? What happens with overlays?)

Either way, I think we need some way to differentiate between two
different meanings of reset_control_assert - reset_control_may_assert
(the clock-like case) and reset_control_must_assert (the fix broken
hardware state case).

> That said, I suspect that to deal sensibly with shared reset lines
> we're also going to need some way to init devices in lockstep with
> others, i.e. we can't deassert device A until it's clocks are running,
> however the reset line is shared with device B who also can't be
> deasserted until it's clocks are running. This seems like it'll get
> rather complicated quickly, to say the least.
> 
> It almost seems that, unless some other board has a similarly broken
> design, that it might almost be easier to just make variants of
> generic-ehci and generic-ohci to handle this one particular case.

On i.MX6Q the 2D rasterizer and 2D vector graphics cores share a reset
line, but there we have the luxury of a single driver handling both (and
furthermore the cores have their own soft reset bits).

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Hi Hans,

Am Mittwoch, den 18.11.2015, 11:38 +0100 schrieb Hans de Goede:
> Hi,
> 
> On 18-11-15 10:46, Philipp Zabel wrote:
> > Hi Hans,
> >
> > Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede:
> >> On 16-11-15 18:01, Philipp Zabel wrote:
> >>> If there are two devices sharing the same reset line that is initially
> >>> held asserted, do the two drivers somehow have to synchronize before
> >>> releasing the reset together?
> >>
> >> Not to my knowledge, I suggest that we simply treat this same as
> >> regulators / clocks where the first one to enable it actually gets
> >> it enabled (de-asserted in case of a reset), and the last one
> >> to disable (assert) it (so dropping the usage counter back to 0) leads
> >> to it being asserted again.
> >>
> >> This seems to work well enough for clocks / regulators and phys, and
> >> at least for my use-case it should work fine for the shared reset too
> >> (I will test to make sure of course).
> >>
> >> So from my pov a simple counter should suffice, does that work for you ?
> >
> > I don't quite understand what counting will help with, then. The first
> > driver to call reset_control_deassert will deassert the reset, whether
> > you count or not.
> > But if the two drivers have deasserted an initially asserted reset, a
> > reset_control_assert for one of them will silently fail.
> 
> Correct, which is what we want, although I would not call it silently
> fail I would call it a nop as it is intended.
>
> > I fear the deassertion count maps well to the case where resets are used
> > just like clocks (when inactive modules are kept in reset), but I'm not
> > sure this is useful in the case of resets that are kept deasserted most
> > of the time, only to be asserted for a short pulse. Maybe we have to
> > differentiate between the two cases?
> 
> Ack. I think that the "just like clocks" case is the more common one,

We have to accommodate both.

>  and it seems to me that the short-pulse case should use reset_control_reset.

reset_control_reset can only be used if the reset controller knows the
necessary reset pulse duration itself.

> Maybe we need to provide a default implementation of reset_control_reset which
> does the pulse when no implementation is provided by the driver ?

I have thought about adding a version that takes a delay parameter, but
then you'd end up with udelay and msleep variants.

> Although that brings the question with it what to do with the deassert_count 
> in
> that case, as some drivers may also use that for the initial deassert... I 
> guess
> we could document to not do that if you want to assure that no other drivers
> muck with the reset-line ...
> 
> Hmm, this is getting messy pretty quickly. New proposal:
> 
> 1) Add a concept of shared resets, adding: reset_control_get_shared and
> devm_reset_control_get_shared functions, which set a shared bool
> in struct reset_control

Yes, that is a good idea. And disallow reset_control_get (non-shared) on
an already in-use control.

> 2) Add int refcnt to struct reset_controller_dev, which gets
> incremented/decremented on reset_control_get/reset_control_put
> do a BUG_ON on refcnt == 1 in the get functions when the non-shared
> variant gets called (this is optional but probably a good extra
> check)

Maybe add a new structure between reset_controller_dev and
reset_control, or keep the reset_control structures themselves on a list
and hand them out again for reset_control_get_shared and do the
reference counting there?
Otherwise the reset_controller_dev would have to allocate a (possibly
huge, sparse) array of reference counters.

> 3) Do the whole deassert_count thingy only when the shared bool is true
> 
> 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true

Sounds good, though I'd prefer WARN_ON over BUG_ON.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Hi Hans,

Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede:
> On 16-11-15 18:01, Philipp Zabel wrote:
> > If there are two devices sharing the same reset line that is initially
> > held asserted, do the two drivers somehow have to synchronize before
> > releasing the reset together?
> 
> Not to my knowledge, I suggest that we simply treat this same as
> regulators / clocks where the first one to enable it actually gets
> it enabled (de-asserted in case of a reset), and the last one
> to disable (assert) it (so dropping the usage counter back to 0) leads
> to it being asserted again.
> 
> This seems to work well enough for clocks / regulators and phys, and
> at least for my use-case it should work fine for the shared reset too
> (I will test to make sure of course).
> 
> So from my pov a simple counter should suffice, does that work for you ?

I don't quite understand what counting will help with, then. The first
driver to call reset_control_deassert will deassert the reset, whether
you count or not.
But if the two drivers have deasserted an initially asserted reset, a
reset_control_assert for one of them will silently fail.

I fear the deassertion count maps well to the case where resets are used
just like clocks (when inactive modules are kept in reset), but I'm not
sure this is useful in the case of resets that are kept deasserted most
of the time, only to be asserted for a short pulse. Maybe we have to
differentiate between the two cases?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Udoo support for chipidea

[Philipp Zabel]

On Wed, Oct 21, 2015 at 10:39:00AM +0800, Peter Chen wrote:
> On Tue, Oct 20, 2015 at 02:09:38PM -0200, Fabio Estevam wrote:
> > Hi Peter,
> > 
> > On Mon, Oct 19, 2015 at 12:50 AM, Peter Chen  
> > wrote:
> > 
> > > Add linux-usb.
> > >
> > > Patryk, your problem is you may need to open 24M OSC for HUB 2514
> > > manually, and you have used IMX6QDL_CLK_CKO for it in the design,
> > > but this clock is not controller's clock, controller's clock has
> > > already decided at SoC dts file (imx6qdl.dtsi), you don't need to
> > > override it at board's dts file.
> > >
> > > You can try delete clock property at imx6qdl-udoo.dtsi, if it still
> > > can't work, try to open IMX6QDL_CLK_CKO at one place to test.
> > 
> > What is the appropriate place to acquire and enable the USB hub clock?
> > 
> > This issue has appeared several times and it seems we don't have a
> > solution for this yet.
> > 
> > Any suggestions?
> 
> Add Alan.
> 
> Hi Alan, we have several designs that the on-board HUB need to
> be reset by gpio pin and its clock is also from the board or
> the SoC. Any suggestions how to add these platform information
> for HUB device?

How about putting it in the device tree?
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
clocks and reset-gpios properties could be added to the USB hub node.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines

[Philipp Zabel]

Am Montag, den 16.11.2015, 17:49 +0100 schrieb Hans de Goede:
> Hi,
> 
> On 16-11-15 17:04, Chen-Yu Tsai wrote:
> > On Mon, Nov 16, 2015 at 11:42 PM, Alan Stern  
> > wrote:
> >> On Sun, 15 Nov 2015, Hans de Goede wrote:
> >>
> >>> From: Reinder de Haan 
> >>>
> >>> At least the EHCI found on the Allwinnner H3 SoC needs multiple reset
> >>> lines, the controller will not initialize while the reset for its
> >>> companion OHCI is still asserted, which means we need to de-assert
> >>> 2 reset-controllers for this EHCI controller to work.
> >>
> >> I assume that reset_control_deassert() is smart enough to maintain a
> >> count of de-assertions, and it doesn't actually turn on the reset line
> >> until the count drops to 0.  Right?
> >
> > No it doesn't. That might be a problem when 2 devices (such as EHCI / OHCI
> > pairs) share a reset line, one probes successfully while the other doesn't.
> > Hans?
> 
> Ugh, good catch Alan, so I think the best way to solve this is to
> actually make reset_control do a deassert / (re)assert count like
> is done already for clocks, there is bound to be more hardware out there
> which shares a reset line between 2 related blocks.
> 
> I'll whip up a patch for this, submit it and then we'll see.

If there are two devices sharing the same reset line that is initially
held asserted, do the two drivers somehow have to synchronize before
releasing the reset together?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] reset: Add of_reset_control_get_by_index

[Philipp Zabel]

Hi Hans,

Am Sonntag, den 15.11.2015, 20:44 +0100 schrieb Hans de Goede:
> From: Reinder de Haan 
> 
> In some cases it is useful to be able to get a reset-controller by
> index rather then by name. E.g. for a generic ip-block driver such
> as the ehci-platform drivers which needs to support more then one reset,
> without knowing the names of the reset lines (as that would make it
> non generic).
> 
> Signed-off-by: Reinder de Haan 
> Signed-off-by: Hans de Goede 

Thank you for the patch, but I already have a similar patch in the
queue. I have included it in a pull request sent to the arm-soc
maintainers today.

best regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: chipidea: usbmisc_imx: delete clock information

[Philipp Zabel]

Hi Peter,

Am Freitag, den 12.12.2014, 15:09 +0800 schrieb Peter Chen:
 All imx usb controller's non core registers uses the same clock gate with
 core registers, the usbmisc_imx is the library for imx glue driver, the
 glue keeps clock on when it calls usbmisc_imx API to change non-core register.

Is this true for all i.MX variants down to i.MX25?

 Besides, we will support runtime pm in the future, it also needs to
 close this clock when the usb is not in use.

I would have expected the clk_prepare_enable / disable_unprepare to
move from probe / remove into the init / post callbacks instead.

regards
Philipp

 Philipp Zabel also verifies it at imx6q platform, see
 http://www.spinics.net/lists/linux-usb/msg118491.html
 
 Cc: Philipp Zabel p.za...@pengutronix.de
 Signed-off-by: Peter Chen peter.c...@freescale.com
 ---
  drivers/usb/chipidea/usbmisc_imx.c | 19 ---
  1 file changed, 19 deletions(-)
 
 diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
 b/drivers/usb/chipidea/usbmisc_imx.c
 index 58591e9..e988e36 100644
 --- a/drivers/usb/chipidea/usbmisc_imx.c
 +++ b/drivers/usb/chipidea/usbmisc_imx.c
 @@ -11,7 +11,6 @@
  
  #include linux/module.h
  #include linux/of_platform.h
 -#include linux/clk.h
  #include linux/err.h
  #include linux/io.h
  #include linux/delay.h
 @@ -69,7 +68,6 @@ struct usbmisc_ops {
  struct imx_usbmisc {
   void __iomem *base;
   spinlock_t lock;
 - struct clk *clk;
   const struct usbmisc_ops *ops;
  };
  
 @@ -322,7 +320,6 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
  {
   struct resource *res;
   struct imx_usbmisc *data;
 - int ret;
   struct of_device_id *tmp_dev;
  
   data = devm_kzalloc(pdev-dev, sizeof(*data), GFP_KERNEL);
 @@ -336,20 +333,6 @@ static int usbmisc_imx_probe(struct platform_device 
 *pdev)
   if (IS_ERR(data-base))
   return PTR_ERR(data-base);
  
 - data-clk = devm_clk_get(pdev-dev, NULL);
 - if (IS_ERR(data-clk)) {
 - dev_err(pdev-dev,
 - failed to get clock, err=%ld\n, PTR_ERR(data-clk));
 - return PTR_ERR(data-clk);
 - }
 -
 - ret = clk_prepare_enable(data-clk);
 - if (ret) {
 - dev_err(pdev-dev,
 - clk_prepare_enable failed, err=%d\n, ret);
 - return ret;
 - }
 -
   tmp_dev = (struct of_device_id *)
   of_match_device(usbmisc_imx_dt_ids, pdev-dev);
   data-ops = (const struct usbmisc_ops *)tmp_dev-data;
 @@ -360,8 +343,6 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
  
  static int usbmisc_imx_remove(struct platform_device *pdev)
  {
 - struct imx_usbmisc *usbmisc = dev_get_drvdata(pdev-dev);
 - clk_disable_unprepare(usbmisc-clk);
   return 0;
  }
  


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: chipidea: usbmisc: skip clocks on i.MX6

[Philipp Zabel]

Hi Peter,

Am Mittwoch, den 10.12.2014, 13:09 +0800 schrieb Peter Chen:
 On Tue, Dec 09, 2014 at 05:13:35PM +0100, Philipp Zabel wrote:
  On i.MX6Q, the USBMISC registers are clocked by the IPG clock,
  so there is no need to enable USBOH3 for the USBMISC driver.
 
 Access the registers at usbmisc needs the same clock with chipidea core,
 both are usboh3.

Alright, I'll drop this patch.

It was my understanding that the usbmisc register space is clocked by
ipg_clk_s, which is not gated by usboh3_clk_enable:
Having a look at the i.MX 6Dual/6Quad Applications Processor Reference
Manual, Rev. 1, 04/2013, Figure 65-1 USB block diagram shows the USB
Control Register (usbmisc) directly connected via IP Bus1 separately
from the host cores (these are connected behind a PL301).
In chapter 18.4 System Clocks, the table entry for the block instance
USB shows that the usboh3_clk_enable bit gates only the ipg_ahb_clk,
ipg_clk_s_pl301, test_clk_240m, test_clk_480m, and test_clk_60m inputs
to the USB block. The ipg_clk_s (from ipg_clk_root) and ipg_clk_32khz
inputs are not marked as gated. Still, it seems that indeed the usbmisc
region is inaccessible after gating usboh3 by unbinding the usb drivers.

regards
Philipp

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] usb: chipidea: usbmisc: skip clocks on i.MX6

[Philipp Zabel]

On i.MX6Q, the USBMISC registers are clocked by the IPG clock,
so there is no need to enable USBOH3 for the USBMISC driver.

Signed-off-by: Philipp Zabel p.za...@pengutronix.de
---
 drivers/usb/chipidea/usbmisc_imx.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 926c997..f7b878d 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -338,23 +338,27 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
if (IS_ERR(data-base))
return PTR_ERR(data-base);
 
-   data-clk = devm_clk_get(pdev-dev, NULL);
-   if (IS_ERR(data-clk)) {
-   dev_err(pdev-dev,
-   failed to get clock, err=%ld\n, PTR_ERR(data-clk));
-   return PTR_ERR(data-clk);
-   }
-
-   ret = clk_prepare_enable(data-clk);
-   if (ret) {
-   dev_err(pdev-dev,
-   clk_prepare_enable failed, err=%d\n, ret);
-   return ret;
-   }
-
tmp_dev = (struct of_device_id *)
of_match_device(usbmisc_imx_dt_ids, pdev-dev);
data-ops = (const struct usbmisc_ops *)tmp_dev-data;
+
+   /* on i.MX6Q, the USBMISC register space is clocked by the IPG clock */
+   if (data-ops != imx6q_usbmisc_ops) {
+   data-clk = devm_clk_get(pdev-dev, NULL);
+   if (IS_ERR(data-clk)) {
+   dev_err(pdev-dev, failed to get clock, err=%ld\n,
+   PTR_ERR(data-clk));
+   return PTR_ERR(data-clk);
+   }
+
+   ret = clk_prepare_enable(data-clk);
+   if (ret) {
+   dev_err(pdev-dev,
+   clk_prepare_enable failed, err=%d\n, ret);
+   return ret;
+   }
+   }
+
platform_set_drvdata(pdev, data);
 
return 0;
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: dts: imx6qdl: remove USBOH3 clock from usbmisc, it is clocked by IPG

[Philipp Zabel]

On i.MX6Q, the USBMISC registers are clocked by the IPG clock,
so there is no need to enable USBOH3 for the USBMISC driver.

Signed-off-by: Philipp Zabel p.za...@pengutronix.de
---
 arch/arm/boot/dts/imx6qdl.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 28e6878..ff3edb0 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -896,7 +896,6 @@
#index-cells = 1;
compatible = fsl,imx6q-usbmisc;
reg = 0x02184800 0x200;
-   clocks = clks IMX6QDL_CLK_USBOH3;
};
 
fec: ethernet@02188000 {
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: generic: add vbus support

[Philipp Zabel]

On Fri, Nov 21, 2014 at 6:24 PM, Robert Jarzmik robert.jarz...@free.fr wrote:
 Felipe Balbi ba...@ti.com writes:

 On Sun, Nov 09, 2014 at 03:58:14PM +0100, Robert Jarzmik wrote:
 +if (!enabled) {
 +ret = regulator_enable(vbus_draw);
 +if (ret  0)
 +return;
 +nop-vbus_draw_enabled = 1;

 do you really need this flag here ? How about:

   if (regulator_is_enabled(vbus_draw))
   foo();
 Good question, I copy-pasted that code from gpio-vbus...
 Philipp, do you have an insight here ?

Regulator refcounting is per regulator_dev, not per struct regulator,
so for multiple consumer scenarios it is necessary to either balance
enable/disable calls, or, if that can't be guaranteed, the consumer
has to keep its own enable flag. The latter is the case for the vbus_draw
callback.
Also regulator_is_enabled does not just return (refcount  0),
but might try to read the actual regulator hardware state (and fail),
depending on the regulator driver.

While I can't see a reason to ever share the vbus_draw regulator with
another consumer, and for gpio regulators regulator_is_enabled never
fails, so for the current users relying on regulator_is_enabled should
work here, I think keeping the flag is the safe thing to do.

regards
Philipp
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] usb: phy: convert gpio-vbus to gpio_desc

[Philipp Zabel]

On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik robert.jarz...@free.fr wrote:
 In order to prepare device-tree conversion, port gpio_vbus to use
 gpio_desc.

 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr

Acked-by: Philipp Zabel philipp.za...@gmail.com

regards
Philipp
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/3] usb: phy: device-tree documentation for gpio-vbus

[Philipp Zabel]

Hi Robert,

On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik robert.jarz...@free.fr wrote:
 Add documentation for device-tree binding of the generic gpio-vbus phy
 controller.

 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
 Cc: devicet...@vger.kernel.org
 ---
  .../devicetree/bindings/phy/gpio-vbus.txt  | 23 
 ++
  1 file changed, 23 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/phy/gpio-vbus.txt

 diff --git a/Documentation/devicetree/bindings/phy/gpio-vbus.txt 
 b/Documentation/devicetree/bindings/phy/gpio-vbus.txt
 new file mode 100644
 index 000..ffcfd35
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/phy/gpio-vbus.txt
 @@ -0,0 +1,23 @@
 +GPIO VBus phy
 +=
 +
 +gpio-vbus is a phy controller supporting VBus detection and pullup 
 activation on
 +GPIOs.

Maybe duplicate the comment from the driver here how his is about
B peripheral only devices

 +Required properties:
 +- compatible : should be generic,phy-gpio-vbus

I'm not sure about the compatible value. I have not seen the generic,
vendor prefix, and all the other generic gpio-something bindings don't
use any prefix: gpio-gate-clock, gpio-leds, gpio-beeper,
pps-gpio, etc.

 +- #phy-cells : from the generic PHY bindings, must be 1.
 +- gpios  : set of 2 gpio properties (see gpio.txt for gpio properties 
 format)
 +   first gpio is required, it's the VBus sensing input gpio
 +  second gpio is optional, it's the D+ pullup controlling output
 +  gpio
 +
 +Optional properties:
 +- wakeup : boolean, if true the VBus gpio will wakeup the platform

The vbus_draw regulator should be part of this binding, I think.

 +Example:
 +   usb2phy : gpio-vbus@13 {
 +   compatible = generic,phy-gpio-vbus;
 +   gpios = gpio 13 GPIO_ACTIVE_LOW;
 +   wakeup;

This on the other hand might be too generic.
I'd like to see just wakeup used here, but the other bindings prefix
linux, (or gpio-key,).

regards
Philipp
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html