Re: [PATCH 5/6] phy: qcom-usb-hs: Fix unbalanced notifier registration

[Bjorn Andersson]

On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote:

> Phy power on/off cycle can happen several times during device life.
> We then need to balance the extcon notifier registration accordingly.
> 
> Fixes: f0b5c2c96370 ("phy: qcom-usb-hs: Replace the extcon API")
> Signed-off-by: Loic Poulain 
> ---
>  drivers/phy/qualcomm/phy-qcom-usb-hs.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c 
> b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> index 2d0c70b..92e9d94 100644
> --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> @@ -181,6 +181,12 @@ static int qcom_usb_hs_phy_power_off(struct phy *phy)
>  {
>   struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy);
>  
> + if (uphy->vbus_edev) {
> + devm_extcon_unregister_notifier(>ulpi->dev,
> + uphy->vbus_edev, EXTCON_USB,
> + >vbus_notify);

As I presume the power_on should always be matched with a power_off I
wonder if it really is appropriate to use the devres version of this
API.

Should we drop the devm_ from registration in power_on as well?

> + }
> +
>   regulator_disable(uphy->v3p3);
>   regulator_disable(uphy->v1p8);
>   clk_disable_unprepare(uphy->sleep_clk);

Regards,
Bjorn

Re: [PATCH 1/6] usb: chipidea: Add dynamic pinctrl selection

[Bjorn Andersson]

On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote:

> Some hardware implementations require to configure pins differently
> according to the USB role (host/device), this can be an update of the
> pins routing or a simple GPIO value change.
> 
> This patch introduces new optional "host" and "device" pinctrls.
> If these pinctrls are defined by the device, they are respectively
> selected on host/device role start.
> 
> If a default pinctrl exist, it is restored on host/device role stop.
> 

The implementation looks reasonable, but we're actually just toggling a
gpio using pinctrl states. Do you see any reason not to control this mux
through the gpio api?

Regards,
Bjorn

> Signed-off-by: Loic Poulain 
> ---
>  drivers/usb/chipidea/core.c  | 19 +++
>  drivers/usb/chipidea/host.c  |  9 +
>  drivers/usb/chipidea/udc.c   |  9 +
>  include/linux/usb/chipidea.h |  6 ++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 85fc6db..03e52fc 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -46,6 +46,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev,
>   else
>   cable->connected = false;
>   }
> +
> + platdata->pctl = devm_pinctrl_get(dev);
> + if (!IS_ERR(platdata->pctl)) {
> + struct pinctrl_state *p;
> +
> + p = pinctrl_lookup_state(platdata->pctl, "default");
> + if (!IS_ERR(p))
> + platdata->pins_default = p;
> +
> + p = pinctrl_lookup_state(platdata->pctl, "host");
> + if (!IS_ERR(p))
> + platdata->pins_host = p;
> +
> + p = pinctrl_lookup_state(platdata->pctl, "device");
> + if (!IS_ERR(p))
> + platdata->pins_device = p;
> + }
> +
>   return 0;
>  }
>  
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index af45aa32..55dbd49 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../host/ehci.h"
>  
> @@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci)
>   }
>   }
>  
> + if (ci->platdata->pins_host)
> + pinctrl_select_state(ci->platdata->pctl,
> +  ci->platdata->pins_host);
> +
>   ret = usb_add_hcd(hcd, 0, 0);
>   if (ret) {
>   goto disable_reg;
> @@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci)
>   }
>   ci->hcd = NULL;
>   ci->otg.host = NULL;
> +
> + if (ci->platdata->pins_host && ci->platdata->pins_default)
> + pinctrl_select_state(ci->platdata->pctl,
> +  ci->platdata->pins_default);
>  }
>  
>  
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 9852ec5..c04384e 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "ci.h"
>  #include "udc.h"
> @@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>  
>  static int udc_id_switch_for_device(struct ci_hdrc *ci)
>  {
> + if (ci->platdata->pins_device)
> + pinctrl_select_state(ci->platdata->pctl,
> +  ci->platdata->pins_device);
> +
>   if (ci->is_otg)
>   /* Clear and enable BSV irq */
>   hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> @@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
>   hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
>  
>   ci->vbus_active = 0;
> +
> + if (ci->platdata->pins_device && ci->platdata->pins_default)
> + pinctrl_select_state(ci->platdata->pctl,
> +  ci->platdata->pins_default);
>  }
>  
>  /**
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 07f9936..63758c3 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -77,6 +77,12 @@ struct ci_hdrc_platform_data {
>   struct ci_hdrc_cablevbus_extcon;
>   struct ci_hdrc_cableid_extcon;
>   u32 phy_clkgate_delay_us;
> +
> + /* pins */
> + struct pinctrl *pctl;
> + struct pinctrl_state *pins_default;
> + struct pinctrl_state *pins_host;
> + struct pinctrl_state *pins_device;
>  };
>  
>  /* Default offset of capability registers */
> -- 
> 2.7.4
> 

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

[Bjorn Andersson]

On Fri 20 Oct 05:20 PDT 2017, Philipp Zabel wrote:

> 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.

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.

> 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.

Regards,
Bjorn
--
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] usb: host: remove ehci-msm.c

[Bjorn Andersson]

On Thu 26 Oct 08:06 PDT 2017, Alan Stern wrote:

> On Thu, 26 Oct 2017, Alex Elder wrote:
> 
> > No Qualcomm SoC requires the "ehci-msm.c" code any more.  So remove it.
> 
> What about old Qualcomm SoCs?  What should they use instead?
> 

These drivers where unfortunately broken by design (host and gadget mode
where implemented in separate drivers that where racing) and the
ChipIdea driver has been updated to support driving the host-mode on
these platforms as well.

So all platforms existing upstream should be covered by the remaining
drivers.

Regards,
Bjorn
--
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

[Bjorn Andersson]

On Wed 19 Jul 08:59 PDT 2017, Philipp Zabel wrote:

> From: Vivek Gautam 
> 
> 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 
> Cc: Jon Hunter 
> Signed-off-by: Vivek Gautam 
> [p.za...@pengutronix.de: changed API to hide reset control arrays behind
>  struct reset_control]
> Signed-off-by: Philipp Zabel 

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.

Regards,
Bjorn
--
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 11/15] remoteproc: make device_type const

[Bjorn Andersson]

On Sat 19 Aug 01:22 PDT 2017, Bhumika Goyal wrote:

> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 

Applied, thanks.

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 364ef28..48b2c5d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1360,7 +1360,7 @@ static void rproc_type_release(struct device *dev)
>   kfree(rproc);
>  }
>  
> -static struct device_type rproc_type = {
> +static const struct device_type rproc_type = {
>   .name   = "remoteproc",
>   .release= rproc_type_release,
>  };
> -- 
> 1.9.1
> 
--
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: [RESEND PATCH] usb: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On Fri 21 Oct 10:38 PDT 2016, Stephen Boyd wrote:

> On 10/21, Bjorn Andersson wrote:
> > hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent
> > memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated 
> > manually
> > it will not have any dma_mem or dma_ops assigned, which makes the
> > dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves
> > this by assigning the dma_mem and dma_ops based on the parent's DeviceTree
> > node.
> > 
> > Cc: Stephen Boyd <sb...@codeaurora.org>
> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> > ---
> > 
> > Hi Peter,
> > 
> > After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm
> > systems I realized that we never concluded on this patch. Unfortunately I 
> > can't
> > find it in my mailbox either, so resending it to restart the discussion.
> > 
> 
> I thought we were going to go down the route that Arnd has been
> pushing[1]? That should work, but I haven't tried it yet and
> there are some more fixes on top from Sriram. I think Sriram is
> taking over the patch now?
> 
> [1] https://patchwork.kernel.org/patch/9319527/

Thanks for the pointer, I've heard about it but couldn't find it.

It does make me further wonder about the multi-device model of these
drivers, but I agree with you that it looks like the patch would
solve our issue.

Regards,
Bjorn
--
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

[RESEND PATCH] usb: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent
memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated manually
it will not have any dma_mem or dma_ops assigned, which makes the
dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves
this by assigning the dma_mem and dma_ops based on the parent's DeviceTree
node.

Cc: Stephen Boyd <sb...@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

Hi Peter,

After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm
systems I realized that we never concluded on this patch. Unfortunately I can't
find it in my mailbox either, so resending it to restart the discussion.

Regards,
Bjorn

 drivers/usb/chipidea/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 69426e644d17..6218d83cca25 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -62,6 +62,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -837,6 +838,9 @@ struct platform_device *ci_hdrc_add_device(struct device 
*dev,
pdev->dev.dma_parms = dev->dma_parms;
dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
 
+   if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+   of_dma_configure(>dev, dev->of_node);
+
ret = platform_device_add_resources(pdev, res, nres);
if (ret)
goto err;
-- 
2.5.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 15/21] usb: chipidea: msm: Mux over secondary phy at the right time

[Bjorn Andersson]

On Sun 26 Jun 00:28 PDT 2016, Stephen Boyd wrote:

> We need to pick the correct phy at runtime based on how the SoC
> has been wired onto the board. If the secondary phy is used, take
> it out of reset and mux over to it by writing into the TCSR
> register. Make sure to do this on reset too, because this
> register is reset to the default value (primary phy) after the
> RESET bit is set in USBCMD.
> 
> Cc: Peter Chen 
> Cc: Greg Kroah-Hartman 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/usb/chipidea/ci_hdrc_msm.c | 78 
> +++---
>  1 file changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c 
> b/drivers/usb/chipidea/ci_hdrc_msm.c
[..]
>  
> +static int ci_hdrc_msm_mux_phy(struct ci_hdrc_msm *ci,
> +struct platform_device *pdev)
> +{
> + struct regmap *regmap;
> + struct device_node *syscon;
> + struct device *dev = >dev;
> + u32 off, val;
> + int ret;
> +
> + syscon = of_parse_phandle(dev->of_node, "phy-select", 0);
> + if (!syscon)
> + return 0;
> +
> + regmap = syscon_node_to_regmap(syscon);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = of_property_read_u32_index(dev->of_node, "phy-select", 1, );
> + if (ret < 0) {
> + dev_err(dev, "no offset in syscon\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_index(dev->of_node, "phy-select", 2, );
> + if (ret < 0) {
> + dev_err(dev, "no value in syscon\n");
> + return -EINVAL;
> + }
> +
> + ret = regmap_write(regmap, off, val);

I recently found out (thanks to a comment from Srinivas) that you can
drop the last two error checks by using
of_parse_phandle_with_fixed_args() as in:

  struct of_phandle_args args;

  ret = of_parse_phandle_with_fixed_args(dev->of_node, "phy-select", 2, 0, 
);
  if (ret < 0)
...

  regmap = syscon_node_to_regmap(args.np);
  of_node_put(args.np);
  if (IS_ERR(regmap))
...

  ret = regmap_write(regmap, args.args[0], args.args[1]);

> + if (ret)
> + return ret;
> +
> + ci->secondary_phy = !!val;
> + if (ci->secondary_phy) {
> + val = readl_relaxed(ci->base + HS_PHY_SEC_CTRL);
> + val |= HS_PHY_DIG_CLAMP_N;
> + writel_relaxed(val, ci->base + HS_PHY_SEC_CTRL);
> + }
> +
> + return 0;
> +}
> +
>  static int ci_hdrc_msm_probe(struct platform_device *pdev)
>  {
>   struct ci_hdrc_msm *ci;
>   struct platform_device *plat_ci;
>   struct clk *clk;
>   struct reset_control *reset;
> + struct resource *res;
> + void __iomem *base;

Doesn't look like you need "base".

> + resource_size_t size;
>   int ret;
>  
>   dev_dbg(>dev, "ci_hdrc_msm_probe\n");
> @@ -76,6 +132,15 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
>   if (IS_ERR(clk))
>   return PTR_ERR(clk);
>  
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + size = resource_size(res);
> + ci->base = base = devm_ioremap(>dev, res->start, size);
> + if (!base)
> + return -ENOMEM;

Replace these two snippets with:

  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  ci->base = devm_ioremap_resource(>dev, res);
  if (IS_ERR(ci->base))
return PTR_ERR(ci->base);

> +
>   reset_control_assert(reset);
>   usleep_range(1, 12000);
>   reset_control_deassert(reset);

Regards,
Bjorn
--
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 01/21] of: device: Support loading a module with OF based modalias

[Bjorn Andersson]

On Sun 26 Jun 00:28 PDT 2016, Stephen Boyd wrote:

> In the case of ULPI devices, we want to be able to load the
> driver before registering the device so that we don't get stuck
> in a loop waiting for the phy module to appear and failing usb
> controller probe. Currently we request the ulpi module via the
> ulpi ids, but in the DT case we might need to request it with the
> OF based modalias instead. Add a common function that allows
> anyone to request a module with the OF based modalias.
> 
> Cc: Rob Herring 
> Cc: 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/of/device.c   | 50 
> +++
>  include/linux/of_device.h |  6 ++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..f275e5beb736 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -226,6 +226,56 @@ ssize_t of_device_get_modalias(struct device *dev, char 
> *str, ssize_t len)
>   return tsize;
>  }
>  
> +static ssize_t of_device_modalias_size(struct device *dev)
> +{
> + const char *compat;
> + int cplen, i;
> + ssize_t csize;
> +
> + if ((!dev) || (!dev->of_node))
> + return -ENODEV;
> +
> + /* Name & Type */
> + csize = 5 + strlen(dev->of_node->name) + strlen(dev->of_node->type);

It would be clearer if you replaced 5 with strlen("of:NT"), but...

> +
> + /* Get compatible property if any */
> + compat = of_get_property(dev->of_node, "compatible", );
> + if (!compat)
> + return csize;
> +
> + /* Find true end (we tolerate multiple \0 at the end */
> + for (i = (cplen - 1); i >= 0 && !compat[i]; i--)
> + cplen--;
> + if (!cplen)
> + return csize;
> + cplen++;
> +
> + /* Check space (need cplen+1 chars including final \0) */
> + return csize + cplen;
> +}

...if I understand of_device_get_modalias() correctly you should be able
to replace this function with:

  size = of_device_get_modalias(dev, NULL, 0);

snprintf() will not write to NULL, csize will be larger than 0 so tsize
will be returned before it will memcpy() to the buffer.

> +
> +int of_device_request_module(struct device *dev)
> +{
> + char *str;
> + ssize_t size;
> + int ret;
> +
> + size = of_device_modalias_size(dev);
> + if (size < 0)
> + return size;
> +
> + str = kmalloc(size + 1, GFP_KERNEL);
> + if (!str)
> + return -ENOMEM;
> +
> + of_device_get_modalias(dev, str, size);
> + str[size] = '\0';
> + ret = request_module(str);
> + kfree(str);
> +
> + return ret;
> +}
> +

Regards,
Bjorn
--
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: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <le...@freescale.com> wrote:
> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <le...@freescale.com> wrote:
>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
>> <bjorn.anders...@linaro.org> wrote:
>>> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:
>>>
>>>>
>>>>
>>>> On 22/02/16 05:32, Bjorn Andersson wrote:
>>>> >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
>>>> >to be able to do DMA allocations, so use the of_dma_configure() helper
>>>> >to populate the dma properties and assign an appropriate dma_ops.
[..]
>>>> None of the drivers call of_dma_configure() explicitly, which makes me feel
>>>> that we are doing something wrong. TBH, this should be handled in more
>>>> generic way rather than driver like this having an explicit call to
>>>> of_dma_configure().
>>>>
>>>
>>> I agree, trying to figure out if it should be inherited or something.
>>
>> I also agree.  We need address it in a more generic way.  I did a
>> search for platform_device_add()/platform_device_register() in the
>> kernel source code.  I found a lot of them and many could be also
>> doing DMA.  Looks like it is still too early to assume every device is
>> already getting dma_ops set through bus probe.  Otherwise, many
>> drivers are potentially broken by this assumption.
>
> Any further comment on this topic?  I added the linux-arm mailing list
> which was missing from previous discussion.
>

I had the chance to go through this with Arnd and the verdict is that
devices not described in DT should not do DMA (or allocate buffers for
doing DMA).

So I believe the solution is to fall back on Peter's description; the
chipidea driver is the core driver and the Qualcomm code should just
be a platform layer.

My suggestion is that we turn the chipidea core into a set of APIs
that can called by the platform specific pieces. That way we will have
the chipidea core be the device described in the DT.

I'll try to find some time to prototype this after Connect.

Regards,
Bjorn
--
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: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:

> 
> 
> On 22/02/16 05:32, Bjorn Andersson wrote:
> >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
> >to be able to do DMA allocations, so use the of_dma_configure() helper
> >to populate the dma properties and assign an appropriate dma_ops.
> >
> >Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> >---
> >  drivers/usb/chipidea/core.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> >index 7404064b9bbc..047b9d4e67aa 100644
> >--- a/drivers/usb/chipidea/core.c
> >+++ b/drivers/usb/chipidea/core.c
> >@@ -62,6 +62,7 @@
> >  #include 
> >  #include 
> >  #include 
> >+#include 
> >  #include 
> >  #include 
> >  #include 
> >@@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device 
> >*dev,
> > pdev->dev.dma_parms = dev->dma_parms;
> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >
> >+if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> >+of_dma_configure(>dev, dev->of_node);
> >+
> Would we hit the same issue if we are on non Device tree platforms like ACPI
> or platform device style itself?
> 

As far as I can see, yes.

> 
> > ret = platform_device_add_resources(pdev, res, nres);
> > if (ret)
> > goto err;
> >
> 
> I think this is the side effect of commit
> 1dccb598df549d892b6450c261da54cdd7af44b4(arm64: simplify dma_get_ops)
> 

I agree, before that we would have hit:

__generic_dma_ops() {
..
   else if (acpi_disabled)
   return dma_ops;
...
}

with dma_ops being swiotlb_dma_ops from arm64_dma_init().


But this would not have saved us in the ACPI case, i.e. the result would
have been as with my suggested patch. Poking Arnd here to see if he has
any input.

> None of the drivers call of_dma_configure() explicitly, which makes me feel
> that we are doing something wrong. TBH, this should be handled in more
> generic way rather than driver like this having an explicit call to
> of_dma_configure().
> 

I agree, trying to figure out if it should be inherited or something.

> 
> On the other hand, I think we could also solve the issue by using correct
> device(parent device) while allocating dma/dma-pool.
> 

Unfortunately this solves the allocation part, but in udc-core we try to
map and unmap buffers based on the gadget's parent pointer (i.e. the
chipidea device).


I'm still puzzled to why the chipidea lives as a child device of the msm
device; but as this is a rather common structure I believe this still
needs to be figured out.


@Arnd, the Qualcomm (msm) chipidea driver instantiates a chipidea
device, which tries to do DMA mapping operations on ARM64 and fails,
because we don't have dma_ops specified on the child. Using
of_dma_configure() we can populate the child with appropriate settings
and ops, but we would be the first driver doing so. Do you have any
pointers to follow on what to do here?

Regards,
Bjorn
--
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: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On Sun 21 Feb 22:02 PST 2016, Peter Chen wrote:

> On Sun, Feb 21, 2016 at 09:32:13PM -0800, Bjorn Andersson wrote:
> > On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
> > to be able to do DMA allocations, so use the of_dma_configure() helper
> > to populate the dma properties and assign an appropriate dma_ops.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> > ---
> >  drivers/usb/chipidea/core.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 7404064b9bbc..047b9d4e67aa 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -62,6 +62,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct 
> > device *dev,
> > pdev->dev.dma_parms = dev->dma_parms;
> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >  
> > +   if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > +   of_dma_configure(>dev, dev->of_node);
> > +
> > ret = platform_device_add_resources(pdev, res, nres);
> > if (ret)
> > goto err;
> 
> Just would like to confirm, it will not affect the default behavior
> which the "dma-ranges" is not set at those platforms?
> 

If I read the code correctly, the only difference if you don't specify 
dma-ranges, dma-coherent or specify an iommu is that the dma_ops gets
assigned.

Regards,
Bjorn
--
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] usb: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
to be able to do DMA allocations, so use the of_dma_configure() helper
to populate the dma properties and assign an appropriate dma_ops.

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/usb/chipidea/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 7404064b9bbc..047b9d4e67aa 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -62,6 +62,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device 
*dev,
pdev->dev.dma_parms = dev->dma_parms;
dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
 
+   if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+   of_dma_configure(>dev, dev->of_node);
+
ret = platform_device_add_resources(pdev, res, nres);
if (ret)
goto err;
-- 
2.5.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 2/3] usb: phy: msm: fix connect/disconnect bug for dragonboard OTG port

[Bjorn Andersson]

On Fri 20 Nov 15:47 PST 2015, Tim Bird wrote:

> Add support for async_irq to wake up driver from low power mode.
> Without this, the power management code never calls resume.
> Remove a spurious interrupt enable in the driver resume function.
> 
> Signed-off-by: Tim Bird 

Sorry Tim for missing these things and not jumping into the discussion
before.

> ---
>  drivers/usb/phy/phy-msm-usb.c | 17 -
>  include/linux/usb/msm_hsusb.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
[..]
> @@ -1732,6 +1731,12 @@ static int msm_otg_probe(struct platform_device *pdev)
>   return motg->irq;
>   }
>  
> + motg->async_irq = platform_get_irq_byname(pdev, "async");
> + if (motg->async_irq < 0) {
> + dev_err(>dev, "platform_get_irq for async irq failed\n");

This is optional, so I must object to this being dev_err(). Except for
development purposes logging this is useless, can't we make it a
dev_dbg?

> + motg->async_irq = 0;
> + }
> +
[..]
> diff --git a/include/linux/usb/msm_hsusb.h b/include/linux/usb/msm_hsusb.h
> index 8c8f685..08c67a3 100644
> --- a/include/linux/usb/msm_hsusb.h
> +++ b/include/linux/usb/msm_hsusb.h
> @@ -164,6 +164,7 @@ struct msm_otg {
>   struct usb_phy phy;
>   struct msm_otg_platform_data *pdata;
>   int irq;
> + int async_irq;

This should be added to the kerneldoc above.

>   struct clk *clk;
>   struct clk *pclk;
>   struct clk *core_clk;

Regards,
Bjorn
--
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: [Device-mainlining] [PATCH v4 1/5] gadget: Introduce the notifier functions

[Bjorn Andersson]

On Fri, Oct 2, 2015 at 10:23 AM, Felipe Balbi via device-mainlining
 wrote:
[..]
> The real difficulty here is coming up with an interface which we're agreeing 
> to
> supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> to userland, we will have to support it.
>

Isn't this the main reason for not creating a user space ABI now?

Running with the in-kernel hooks would allow us to get things working
now, we can change it as we wish, we can explore the difficulties and
we can create an ABI from that - if we need to - that might have a
chance to work for the next 30 years.

Regards,
Bjorn
--
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 0/5] Rename regulator_set_optimum_mode

[Bjorn Andersson]

On Wed, Feb 11, 2015 at 7:35 PM, Bjorn Andersson
bjorn.anders...@sonymobile.com wrote:
 Changing the name of the regulator_set_optimum_mode() to
 regulator_set_load() better reflects that the API is doing.


Any comments on this?

I'm going to propose a patch to the mmc framework calling this api, so
it would be good to know before I add another consumer of the api.

Regards,
Bjorn
--
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 0/5] Rename regulator_set_optimum_mode

[Bjorn Andersson]

Changing the name of the regulator_set_optimum_mode() to
regulator_set_load() better reflects that the API is doing.

This series does the name change and move the current consumers
over.

Bjorn Andersson (5):
  regulator: s/regulator_set_optimum_mode/regulator_set_load/
  ufs: Rename of regulator_set_optimum_mode
  usb: phy: ab8500-usb: Rename regulator_set_optimum_mode
  usb: phy: phy-msm-usb: Rename regulator_set_optimum_mode
  regulator: Drop temporary regulator_set_optimum_mode wrapper

 Documentation/power/regulator/consumer.txt |  2 +-
 drivers/regulator/core.c   |  8 
 drivers/scsi/ufs/ufshcd.c  | 27 +++
 drivers/usb/phy/phy-ab8500-usb.c   |  4 ++--
 drivers/usb/phy/phy-msm-usb.c  | 15 +--
 include/linux/regulator/consumer.h |  5 ++---
 6 files changed, 21 insertions(+), 40 deletions(-)

-- 
1.8.2.2

--
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 4/5] usb: phy: phy-msm-usb: Rename regulator_set_optimum_mode

[Bjorn Andersson]

The function regulator_set_optimum_mode() is changing name to
regulator_set_load(), so update the code accordingly.

Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
 drivers/usb/phy/phy-msm-usb.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
index 000fd89..6ed67ea 100644
--- a/drivers/usb/phy/phy-msm-usb.c
+++ b/drivers/usb/phy/phy-msm-usb.c
@@ -142,27 +142,22 @@ static int msm_hsusb_ldo_set_mode(struct msm_otg *motg, 
int on)
int ret = 0;
 
if (on) {
-   ret = regulator_set_optimum_mode(motg-v1p8,
-   USB_PHY_1P8_HPM_LOAD);
+   ret = regulator_set_load(motg-v1p8, USB_PHY_1P8_HPM_LOAD);
if (ret  0) {
pr_err(Could not set HPM for v1p8\n);
return ret;
}
-   ret = regulator_set_optimum_mode(motg-v3p3,
-   USB_PHY_3P3_HPM_LOAD);
+   ret = regulator_set_load(motg-v3p3, USB_PHY_3P3_HPM_LOAD);
if (ret  0) {
pr_err(Could not set HPM for v3p3\n);
-   regulator_set_optimum_mode(motg-v1p8,
-   USB_PHY_1P8_LPM_LOAD);
+   regulator_set_load(motg-v1p8, USB_PHY_1P8_LPM_LOAD);
return ret;
}
} else {
-   ret = regulator_set_optimum_mode(motg-v1p8,
-   USB_PHY_1P8_LPM_LOAD);
+   ret = regulator_set_load(motg-v1p8, USB_PHY_1P8_LPM_LOAD);
if (ret  0)
pr_err(Could not set LPM for v1p8\n);
-   ret = regulator_set_optimum_mode(motg-v3p3,
-   USB_PHY_3P3_LPM_LOAD);
+   ret = regulator_set_load(motg-v3p3, USB_PHY_3P3_LPM_LOAD);
if (ret  0)
pr_err(Could not set LPM for v3p3\n);
}
-- 
1.8.2.2

--
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 3/5] usb: phy: ab8500-usb: Rename regulator_set_optimum_mode

[Bjorn Andersson]

The function regulator_set_optimum_mode() is changing name to
regulator_set_load(), so update the code accordingly.

Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
 drivers/usb/phy/phy-ab8500-usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/phy/phy-ab8500-usb.c b/drivers/usb/phy/phy-ab8500-usb.c
index 0b1bd23..f5b3b92 100644
--- a/drivers/usb/phy/phy-ab8500-usb.c
+++ b/drivers/usb/phy/phy-ab8500-usb.c
@@ -277,7 +277,7 @@ static void ab8500_usb_regulator_enable(struct ab8500_usb 
*ab)
dev_err(ab-dev, Failed to set the Vintcore to 1.3V, 
ret=%d\n,
ret);
 
-   ret = regulator_set_optimum_mode(ab-v_ulpi, 28000);
+   ret = regulator_set_load(ab-v_ulpi, 28000);
if (ret  0)
dev_err(ab-dev, Failed to set optimum mode 
(ret=%d)\n,
ret);
@@ -317,7 +317,7 @@ static void ab8500_usb_regulator_disable(struct ab8500_usb 
*ab)
ab-saved_v_ulpi, ret);
}
 
-   ret = regulator_set_optimum_mode(ab-v_ulpi, 0);
+   ret = regulator_set_load(ab-v_ulpi, 0);
if (ret  0)
dev_err(ab-dev, Failed to set optimum mode 
(ret=%d)\n,
ret);
-- 
1.8.2.2

--
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