| Message ID | 1405499166-6726-8-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
On Wednesday 16 July 2014 10:26:01 Antoine Ténart wrote: > + > + if (priv->dma_mask) { > + ret = dma_coerce_mask_and_coherent(&pdev->dev, priv->dma_mask); > + if (ret) > + return ret; > + } > + > As mentioned in my comment for the binding, this is the wrong way to do it. Russell has in the past converted all drivers that did this manually to do dma_coerce_mask_and_coherent() so we can spot them more easily, but we should really be doing this better for new drivers. Can you describe what the restriction is that you want to put on the dma mask? Arnd
Hi Arnd, On Wed, Jul 16, 2014 at 10:41:10AM +0200, Arnd Bergmann wrote: > On Wednesday 16 July 2014 10:26:01 Antoine Ténart wrote: > > + > > + if (priv->dma_mask) { > > + ret = dma_coerce_mask_and_coherent(&pdev->dev, priv->dma_mask); > > + if (ret) > > + return ret; > > + } > > + > > > > As mentioned in my comment for the binding, this is the wrong way to do it. > Russell has in the past converted all drivers that did this manually to > do dma_coerce_mask_and_coherent() so we can spot them more easily, but we > should really be doing this better for new drivers. > > Can you describe what the restriction is that you want to put on the dma mask? Some people wanted the possibility to set the DMA mask as this USB2 CI driver does not do specific Berlin operation and can be reused later. I don't particularly need to call dma_coerce_mask_and_coherent() in my case, as far as I know. They can maybe give the restrictions they might want to put on the DMA mask. Antoine
On Wednesday 16 July 2014 11:15:33 Antoine Ténart wrote: > > On Wed, Jul 16, 2014 at 10:41:10AM +0200, Arnd Bergmann wrote: > > On Wednesday 16 July 2014 10:26:01 Antoine Ténart wrote: > > > + > > > + if (priv->dma_mask) { > > > + ret = dma_coerce_mask_and_coherent(&pdev->dev, priv->dma_mask); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > > > > > As mentioned in my comment for the binding, this is the wrong way to do it. > > Russell has in the past converted all drivers that did this manually to > > do dma_coerce_mask_and_coherent() so we can spot them more easily, but we > > should really be doing this better for new drivers. > > > > Can you describe what the restriction is that you want to put on the dma mask? > > Some people wanted the possibility to set the DMA mask as this USB2 CI > driver does not do specific Berlin operation and can be reused later. > I don't particularly need to call dma_coerce_mask_and_coherent() in my > case, as far as I know. Ok, just remove the call then and rely on the default mask. > They can maybe give the restrictions they might want to put on the DMA > mask. If the restriction is from the bus, it should get handled automatically by the device probe as long as the correct dma-ranges property is there (though we have a small bug there at the moment). If there is a variation of ci13xxx that can't do 32-bit DMA, that should use a different compatible string and pass a fixed mask into dma_set_mask_and_coherent() based on the device. Arnd
> > > > Some people wanted the possibility to set the DMA mask as this USB2 CI > > driver does not do specific Berlin operation and can be reused later. > > I don't particularly need to call dma_coerce_mask_and_coherent() in my > > case, as far as I know. > > Ok, just remove the call then and rely on the default mask. > > > They can maybe give the restrictions they might want to put on the DMA > > mask. > > If the restriction is from the bus, it should get handled automatically > by the device probe as long as the correct dma-ranges property is there > (though we have a small bug there at the moment). If there is a variation > of ci13xxx that can't do 32-bit DMA, that should use a different > compatible string and pass a fixed mask into dma_set_mask_and_coherent() > based on the device. > Correct me if my below understanding is wrong please. For three chipidea users: user_a: don't need dma mask user_b: dma mask value is dma_mask_b user_c: dma mask value is dma_mask_c We don't need to call dma_coerce_mask_and_coherent() at generic chipidea glue driver, and set dma-ranges as dma_mask_b at user_b's dts, and set dma-ranges as dma_mask_c at user_c's dts. Peter
On Wednesday 16 July 2014 11:58:14 Peter Chen wrote: > > > > > > Some people wanted the possibility to set the DMA mask as this USB2 CI > > > driver does not do specific Berlin operation and can be reused later. > > > I don't particularly need to call dma_coerce_mask_and_coherent() in my > > > case, as far as I know. > > > > Ok, just remove the call then and rely on the default mask. > > > > > They can maybe give the restrictions they might want to put on the DMA > > > mask. > > > > If the restriction is from the bus, it should get handled automatically > > by the device probe as long as the correct dma-ranges property is there > > (though we have a small bug there at the moment). If there is a variation > > of ci13xxx that can't do 32-bit DMA, that should use a different > > compatible string and pass a fixed mask into dma_set_mask_and_coherent() > > based on the device. > > > > Correct me if my below understanding is wrong please. > > For three chipidea users: > user_a: don't need dma mask > user_b: dma mask value is dma_mask_b > user_c: dma mask value is dma_mask_c > > We don't need to call dma_coerce_mask_and_coherent() at generic chipidea > glue driver, and set dma-ranges as dma_mask_b at user_b's dts, and set > dma-ranges as dma_mask_c at user_c's dts. The dma-ranges property doesn't just encode a dma-mask for a device, but rather how the children of a bus see the memory address space of the parent. For traditional reasons we default to assuming that a 32-bit dma_mask is correct, but there may be multiple reasons why this is not true: - you have a device connected to a bus that is smaller than 32-bit wide (and that would have a dma-ranges property describing that) - you have a device that has fewer than 32 address lines but is connected to a 32-bit upstream bus (hence the dma-ranges describe all 32 bit, but the driver must set a smaller mask) - you have a 64-bit capable device connected to a 32-bit bus: the driver will ask for a 64-bit mask, but the dma_set_mask() call refuses this because of the bus capabilities. - you have a 64-bit capable device on a 64-bit capable bus, and the dma_set_mask call should succeed. The mask is definitely never "user" specific, but is a combination of the device you have and the bus it is connected to. Things get more complicated when you add in IOMMUs to this, but I'm skipping this for the sake of limiting the confusion. Arnd
> > > > > > > > Some people wanted the possibility to set the DMA mask as this > > > > USB2 CI driver does not do specific Berlin operation and can be > reused later. > > > > I don't particularly need to call dma_coerce_mask_and_coherent() > > > > in my case, as far as I know. > > > > > > Ok, just remove the call then and rely on the default mask. > > > > > > > They can maybe give the restrictions they might want to put on the > > > > DMA mask. > > > > > > If the restriction is from the bus, it should get handled > > > automatically by the device probe as long as the correct dma-ranges > > > property is there (though we have a small bug there at the moment). > > > If there is a variation of ci13xxx that can't do 32-bit DMA, that > > > should use a different compatible string and pass a fixed mask into > > > dma_set_mask_and_coherent() based on the device. > > > > > > > Correct me if my below understanding is wrong please. > > > > For three chipidea users: > > user_a: don't need dma mask > > user_b: dma mask value is dma_mask_b > > user_c: dma mask value is dma_mask_c > > > > We don't need to call dma_coerce_mask_and_coherent() at generic > > chipidea glue driver, and set dma-ranges as dma_mask_b at user_b's > > dts, and set dma-ranges as dma_mask_c at user_c's dts. > > The dma-ranges property doesn't just encode a dma-mask for a device, but > rather how the children of a bus see the memory address space of the > parent. > > For traditional reasons we default to assuming that a 32-bit dma_mask is > correct, but there may be multiple reasons why this is not true: > > - you have a device connected to a bus that is smaller than 32-bit wide > (and that would have a dma-ranges property describing that) > - you have a device that has fewer than 32 address lines but is connected > to a 32-bit upstream bus (hence the dma-ranges describe all 32 bit, > but the driver must set a smaller mask) > - you have a 64-bit capable device connected to a 32-bit bus: the driver > will ask for a 64-bit mask, but the dma_set_mask() call refuses this > because of the bus capabilities. > - you have a 64-bit capable device on a 64-bit capable bus, and the > dma_set_mask call should succeed. > > The mask is definitely never "user" specific, but is a combination of the > device you have and the bus it is connected to. > Thanks, arnd. For chipidea generic glue layer case, if there are three devices who use this driver, and all devices have 32-bit bus, some devices have less 32 address lines. For example: - the device_a doesn't need to use dma_mask - the device_b needs dma_mask as 0xfffffffff - the device_c needs dma_mask as 0xfffffff0, assume it has only 28 address lines My questions are: - Can we not set dma_mask at driver, and only set dma-ranges at dts for device_b and device_c as a solution to cover this different dma mask use case? - If we can't use this solution, would you suggest one? - If we can use this solution, for device_b and device_c, how can we write dma-ranges? I can't find any arm platforms use it, only some powerpc platform use it. According to the definition from Power_ePAPR_APPROVED_v1.1.pdf, it is dma-ranges = <child-bus-address, parent-bus-address, length> but I find the powerpc has different way for using dma-ranges. Peter
On Thursday 17 July 2014 01:20:54 Peter Chen wrote: > Thanks, arnd. > > For chipidea generic glue layer case, if there are three devices who use this > driver, and all devices have 32-bit bus, some devices have less 32 address lines. > For example: > > - the device_a doesn't need to use dma_mask > - the device_b needs dma_mask as 0xfffffffff > - the device_c needs dma_mask as 0xfffffff0, assume it has only 28 address lines This makes no sense. You always need a dma mask, so the first case doesn't exist, and the second one is the default. In the third case, I assume you mean 0x0fffffff, which is a 28-bit mask. > My questions are: > - Can we not set dma_mask at driver, and only set dma-ranges at dts for device_b > and device_c as a solution to cover this different dma mask use case? try to understand my earlier reply. What is the problem with device_b? Is that a limitation of the bus it is connected to, or the version of the chipidea hardware? > - If we can't use this solution, would you suggest one? It depends on what the requirement of the hardware is, as I explained now for three times. > - If we can use this solution, for device_b and device_c, how can we write dma-ranges? > I can't find any arm platforms use it, only some powerpc platform use it. > According to the definition from Power_ePAPR_APPROVED_v1.1.pdf, it is > dma-ranges = <child-bus-address, parent-bus-address, length> > but I find the powerpc has different way for using dma-ranges. It's now handled by of_dma_configure() in drivers/of/platform.c for all architectures. Arnd
On Thu, Jul 17, 2014 at 12:21:56PM +0200, Arnd Bergmann wrote: > On Thursday 17 July 2014 01:20:54 Peter Chen wrote: > > Thanks, arnd. > > > > For chipidea generic glue layer case, if there are three devices who use this > > driver, and all devices have 32-bit bus, some devices have less 32 address lines. > > For example: > > > > - the device_a doesn't need to use dma_mask > > - the device_b needs dma_mask as 0xfffffffff > > - the device_c needs dma_mask as 0xfffffff0, assume it has only 28 address lines > > This makes no sense. You always need a dma mask, so the first case doesn't exist, > and the second one is the default. > > In the third case, I assume you mean 0x0fffffff, which is a 28-bit mask. > > > My questions are: > > - Can we not set dma_mask at driver, and only set dma-ranges at dts for device_b > > and device_c as a solution to cover this different dma mask use case? > > try to understand my earlier reply. What is the problem with device_b? > Is that a limitation of the bus it is connected to, or the version of the > chipidea hardware? > > > - If we can't use this solution, would you suggest one? > > It depends on what the requirement of the hardware is, as I explained > now for three times. > Thanks, Arnd. Currently, we are designing a generic driver, we don't know what's the hardware architecture, we are trying to find a solution how to set dma mask for all possible devices which will use this driver, Antoine's this patch is trying to cover this feature. For example, Marvell Berlin doesn't need to set dma mask specific. http://marc.info/?l=linux-arm-kernel&m=140205228507045&w=2 http://www.spinics.net/lists/linux-usb/msg110598.html Xilinx zynq needs to set dma mask as 0xFFFFFFF0 http://marc.info/?l=linux-usb&m=140384129921706&w=2 FSL i.mx needs to set dma mask as DMA_BIT_MASK(32) https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/chipidea/ci_hdrc_imx.c?h=usb-next
On Thursday 17 July 2014 19:19:15 Peter Chen wrote: > > Currently, we are designing a generic driver, we don't know what's the > hardware architecture, we are trying to find a solution how to set > dma mask for all possible devices which will use this driver, Antoine's > this patch is trying to cover this feature. > > For example, > > Marvell Berlin doesn't need to set dma mask specific. > http://marc.info/?l=linux-arm-kernel&m=140205228507045&w=2 > http://www.spinics.net/lists/linux-usb/msg110598.html > > Xilinx zynq needs to set dma mask as 0xFFFFFFF0 > http://marc.info/?l=linux-usb&m=140384129921706&w=2 > > FSL i.mx needs to set dma mask as DMA_BIT_MASK(32) > https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/chipidea/ci_hdrc_imx.c?h=usb-next Ok, I see now. I think we can safely drop it completely then: - Berlin was only recently added and doesn't need to set the mask because the common code handles it just fine now. - i.mx was added earlier, at a time where it was still necessary - zynq just made a mistake, 0xFFFFFFF0 was never a valid mask and just happened to work by accident. In summary, the 32-bit default mask will work on all of them, and we need no special code or DT properties for it. Arnd
On Thu, Jul 17, 2014 at 07:19:15PM +0800, Peter Chen wrote: > Currently, we are designing a generic driver, we don't know what's the > hardware architecture, we are trying to find a solution how to set > dma mask for all possible devices which will use this driver, Antoine's > this patch is trying to cover this feature. > > For example, > > Marvell Berlin doesn't need to set dma mask specific. > http://marc.info/?l=linux-arm-kernel&m=140205228507045&w=2 > http://www.spinics.net/lists/linux-usb/msg110598.html I can't make sense of those messages. What are you saying - that ci_hdrc on Berlin does not use DMA at all? Or that it doesn't care what the DMA mask is set to? > Xilinx zynq needs to set dma mask as 0xFFFFFFF0 > http://marc.info/?l=linux-usb&m=140384129921706&w=2 Why? The DMA mask does /not/ convey the DMA alignment requirements of the transfers. If you need it to be aligned to 16-bytes, then that's something which is internal to the driver. This is no different from devices which require 32-bit alignment or 64-bit alignment. You can't really expect the DMA subsystem to take care of that for you. The DMA mask is about indicating what memory ranges are acceptable for DMA, and not the alignment. So, in this case, your DMA mask should be DMA_BIT_MASK(32), the same as iMX. However, if your driver does receive memory which is not appropriately aligned, it is the driver's responsibility, not the DMA API, to deal with that appropriately. So, it sounds to me like: - The DT code should be setting the DMA mask appropriately already. - If the driver is using streaming DMA, should call: err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); if (err) handle error; - If the driver is using coherent DMA only: err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); if (err) handle error; - If the driver uses both coherent DMA and streaming DMA: err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (err) handle error; Where the _bus_ has restrictions on the available memory (iow, the higher address bits), these should be dealt with inside the DMA API.
On Wed, Jul 16, 2014 at 10:26:01AM +0200, Antoine Ténart wrote: > Add a USB2 ChipIdea driver for ci13xxx, with optional PHY, clock > and DMA mask, to support USB2 ChipIdea controllers that don't need > specific functions. You may need to indicate it is a generic usb2 glue layer driver at both subject and context. > > Needed for the Marvell Berlin SoCs USB controllers. You can say it is tested at Marvell Berlin SoCs USB controllers. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/usb/chipidea/Makefile | 1 + > drivers/usb/chipidea/ci_hdrc_usb2.c | 147 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 148 insertions(+) > create mode 100644 drivers/usb/chipidea/ci_hdrc_usb2.c > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 2f099c7df7b5..1fc86a2ca22d 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -10,6 +10,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o > > # Glue/Bridge layers go here > > +obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_usb2.o > obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_msm.o > obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_zevio.o > > diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c > new file mode 100644 > index 000000000000..4ec2240d00a1 > --- /dev/null > +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c > @@ -0,0 +1,147 @@ > +/* > + * Copyright (C) 2014 Marvell Technology Group Ltd. > + * > + * Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/clk.h> > +#include <linux/dma-mapping.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/usb/chipidea.h> > +#include <linux/usb/hcd.h> > +#include <linux/usb/ulpi.h> > + > +#include "ci.h" > + > +struct ci_hdrc_usb2_priv { > + struct platform_device *ci_pdev; > + struct clk *clk; > + u32 dma_mask; > +}; > + > +static int ci_hdrc_usb2_dt_probe(struct device *dev, > + struct ci_hdrc_platform_data *ci_pdata, > + struct ci_hdrc_usb2_priv *priv) > +{ > + u32 mask; > + int ret; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (!IS_ERR(priv->clk)) { > + ret = clk_prepare_enable(priv->clk); > + if (ret) { > + dev_err(dev, "failed to enable the clock: %d\n", ret); > + return ret; > + } > + } The clk API may be needed for both DT and non-DT cases. > + > + ci_pdata->phy = of_phy_get(dev->of_node, 0); > + if (IS_ERR(ci_pdata->phy)) { > + if (PTR_ERR(ci_pdata->phy) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + /* PHY is optional */ > + ci_pdata->phy = NULL; > + } > + > + if (of_property_read_u32(dev->of_node, "dma-mask", &mask)) > + priv->dma_mask = mask; > + According to discussion, you may not need dma_mask in driver any more, and use below API for both DT and non-DT case. err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (err) handle error; Peter > + return 0; > +} > + > +static struct ci_hdrc_platform_data ci_default_pdata = { > + .capoffset = DEF_CAPOFFSET, > + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | > + CI_HDRC_DISABLE_STREAMING, > +}; > + > +static int ci_hdrc_usb2_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct ci_hdrc_usb2_priv *priv; > + struct ci_hdrc_platform_data *ci_pdata = dev_get_platdata(&pdev->dev); > + int ret; > + > + if (!ci_pdata) > + ci_pdata = &ci_default_pdata; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + if (dev->of_node) { > + ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata, priv); > + if (ret) > + return ret; > + } > + > + ci_pdata->name = dev_name(&pdev->dev); > + > + if (priv->dma_mask) { > + ret = dma_coerce_mask_and_coherent(&pdev->dev, priv->dma_mask); > + if (ret) > + return ret; > + } > + > + priv->ci_pdev = ci_hdrc_add_device(dev, pdev->resource, > + pdev->num_resources, ci_pdata); > + if (IS_ERR(priv->ci_pdev)) { > + ret = PTR_ERR(priv->ci_pdev); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, > + "failed to register ci_hdrc platform device: %d\n", > + ret); > + goto clk_err; > + } > + > + platform_set_drvdata(pdev, priv); > + > + pm_runtime_no_callbacks(dev); > + pm_runtime_enable(dev); > + > + return 0; > + > +clk_err: > + if (!IS_ERR(priv->clk)) > + clk_disable_unprepare(priv->clk); > + return ret; > +} > + > +static int ci_hdrc_usb2_remove(struct platform_device *pdev) > +{ > + struct ci_hdrc_usb2_priv *priv = platform_get_drvdata(pdev); > + > + pm_runtime_disable(&pdev->dev); > + ci_hdrc_remove_device(priv->ci_pdev); > + clk_disable_unprepare(priv->clk); > + > + return 0; > +} > + > +static const struct of_device_id ci_hdrc_usb2_of_match[] = { > + { .compatible = "chipidea,usb2" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ci_hdrc_usb2_of_match); > + > +static struct platform_driver ci_hdrc_usb2_driver = { > + .probe = ci_hdrc_usb2_probe, > + .remove = ci_hdrc_usb2_remove, > + .driver = { > + .name = "chipidea-usb2", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ci_hdrc_usb2_of_match), > + }, > +}; > +module_platform_driver(ci_hdrc_usb2_driver); > + > +MODULE_DESCRIPTION("ChipIdea HDRC USB2 binding for ci13xxx"); > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > +MODULE_LICENSE("GPL"); > -- > 1.9.1 >
Hi Peter, On Fri, Jul 25, 2014 at 10:18:54AM +0800, Peter Chen wrote: > On Wed, Jul 16, 2014 at 10:26:01AM +0200, Antoine Ténart wrote: > > Add a USB2 ChipIdea driver for ci13xxx, with optional PHY, clock > > and DMA mask, to support USB2 ChipIdea controllers that don't need > > specific functions. > > You may need to indicate it is a generic usb2 glue layer driver at both > subject and context. "USB2 ChipIdea driver for ci13xxx" seemed quite generic to me. > > > > Needed for the Marvell Berlin SoCs USB controllers. > > You can say it is tested at Marvell Berlin SoCs USB controllers. Ok, I'll explicitly say it. > > + > > +static int ci_hdrc_usb2_dt_probe(struct device *dev, > > + struct ci_hdrc_platform_data *ci_pdata, > > + struct ci_hdrc_usb2_priv *priv) > > +{ > > + u32 mask; > > + int ret; > > + > > + priv->clk = devm_clk_get(dev, NULL); > > + if (!IS_ERR(priv->clk)) { > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) { > > + dev_err(dev, "failed to enable the clock: %d\n", ret); > > + return ret; > > + } > > + } > > The clk API may be needed for both DT and non-DT cases. Yes sure, I'll move this to the ci_hdrc_usb2_probe(). > > > + > > + ci_pdata->phy = of_phy_get(dev->of_node, 0); > > + if (IS_ERR(ci_pdata->phy)) { > > + if (PTR_ERR(ci_pdata->phy) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + /* PHY is optional */ > > + ci_pdata->phy = NULL; > > + } > > + > > + if (of_property_read_u32(dev->of_node, "dma-mask", &mask)) > > + priv->dma_mask = mask; > > + > According to discussion, you may not need dma_mask in driver any more, > and use below API for both DT and non-DT case. > > err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > if (err) > handle error; This would only be needed in non-dt cases, as the DMA mask is configured in drivers/of/platform.c otherwise. Thanks! Antoine
> > On Fri, Jul 25, 2014 at 10:18:54AM +0800, Peter Chen wrote: > > On Wed, Jul 16, 2014 at 10:26:01AM +0200, Antoine Ténart wrote: > > > Add a USB2 ChipIdea driver for ci13xxx, with optional PHY, clock and > > > DMA mask, to support USB2 ChipIdea controllers that don't need > > > specific functions. > > > > You may need to indicate it is a generic usb2 glue layer driver at > > both subject and context. > > "USB2 ChipIdea driver for ci13xxx" seemed quite generic to me. > ok > > > > > > Needed for the Marvell Berlin SoCs USB controllers. > > > > You can say it is tested at Marvell Berlin SoCs USB controllers. > > Ok, I'll explicitly say it. > > > > + > > > +static int ci_hdrc_usb2_dt_probe(struct device *dev, > > > + struct ci_hdrc_platform_data *ci_pdata, > > > + struct ci_hdrc_usb2_priv *priv) { > > > + u32 mask; > > > + int ret; > > > + > > > + priv->clk = devm_clk_get(dev, NULL); > > > + if (!IS_ERR(priv->clk)) { > > > + ret = clk_prepare_enable(priv->clk); > > > + if (ret) { > > > + dev_err(dev, "failed to enable the clock: %d\n", ret); > > > + return ret; > > > + } > > > + } > > > > The clk API may be needed for both DT and non-DT cases. > > Yes sure, I'll move this to the ci_hdrc_usb2_probe(). > > > > + > > > + if (of_property_read_u32(dev->of_node, "dma-mask", &mask)) > > > + priv->dma_mask = mask; > > > + > > According to discussion, you may not need dma_mask in driver any more, > > and use below API for both DT and non-DT case. > > > > err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > > if (err) > > handle error; > > This would only be needed in non-dt cases, as the DMA mask is configured > in drivers/of/platform.c otherwise. > Get it, thanks. Peter
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile index 2f099c7df7b5..1fc86a2ca22d 100644 --- a/drivers/usb/chipidea/Makefile +++ b/drivers/usb/chipidea/Makefile @@ -10,6 +10,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o # Glue/Bridge layers go here +obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_usb2.o obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_msm.o obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_zevio.o diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c new file mode 100644 index 000000000000..4ec2240d00a1 --- /dev/null +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c @@ -0,0 +1,147 @@ +/* + * Copyright (C) 2014 Marvell Technology Group Ltd. + * + * Antoine Ténart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/clk.h> +#include <linux/dma-mapping.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/usb/chipidea.h> +#include <linux/usb/hcd.h> +#include <linux/usb/ulpi.h> + +#include "ci.h" + +struct ci_hdrc_usb2_priv { + struct platform_device *ci_pdev; + struct clk *clk; + u32 dma_mask; +}; + +static int ci_hdrc_usb2_dt_probe(struct device *dev, + struct ci_hdrc_platform_data *ci_pdata, + struct ci_hdrc_usb2_priv *priv) +{ + u32 mask; + int ret; + + priv->clk = devm_clk_get(dev, NULL); + if (!IS_ERR(priv->clk)) { + ret = clk_prepare_enable(priv->clk); + if (ret) { + dev_err(dev, "failed to enable the clock: %d\n", ret); + return ret; + } + } + + ci_pdata->phy = of_phy_get(dev->of_node, 0); + if (IS_ERR(ci_pdata->phy)) { + if (PTR_ERR(ci_pdata->phy) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* PHY is optional */ + ci_pdata->phy = NULL; + } + + if (of_property_read_u32(dev->of_node, "dma-mask", &mask)) + priv->dma_mask = mask; + + return 0; +} + +static struct ci_hdrc_platform_data ci_default_pdata = { + .capoffset = DEF_CAPOFFSET, + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | + CI_HDRC_DISABLE_STREAMING, +}; + +static int ci_hdrc_usb2_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct ci_hdrc_usb2_priv *priv; + struct ci_hdrc_platform_data *ci_pdata = dev_get_platdata(&pdev->dev); + int ret; + + if (!ci_pdata) + ci_pdata = &ci_default_pdata; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + if (dev->of_node) { + ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata, priv); + if (ret) + return ret; + } + + ci_pdata->name = dev_name(&pdev->dev); + + if (priv->dma_mask) { + ret = dma_coerce_mask_and_coherent(&pdev->dev, priv->dma_mask); + if (ret) + return ret; + } + + priv->ci_pdev = ci_hdrc_add_device(dev, pdev->resource, + pdev->num_resources, ci_pdata); + if (IS_ERR(priv->ci_pdev)) { + ret = PTR_ERR(priv->ci_pdev); + if (ret != -EPROBE_DEFER) + dev_err(dev, + "failed to register ci_hdrc platform device: %d\n", + ret); + goto clk_err; + } + + platform_set_drvdata(pdev, priv); + + pm_runtime_no_callbacks(dev); + pm_runtime_enable(dev); + + return 0; + +clk_err: + if (!IS_ERR(priv->clk)) + clk_disable_unprepare(priv->clk); + return ret; +} + +static int ci_hdrc_usb2_remove(struct platform_device *pdev) +{ + struct ci_hdrc_usb2_priv *priv = platform_get_drvdata(pdev); + + pm_runtime_disable(&pdev->dev); + ci_hdrc_remove_device(priv->ci_pdev); + clk_disable_unprepare(priv->clk); + + return 0; +} + +static const struct of_device_id ci_hdrc_usb2_of_match[] = { + { .compatible = "chipidea,usb2" }, + { } +}; +MODULE_DEVICE_TABLE(of, ci_hdrc_usb2_of_match); + +static struct platform_driver ci_hdrc_usb2_driver = { + .probe = ci_hdrc_usb2_probe, + .remove = ci_hdrc_usb2_remove, + .driver = { + .name = "chipidea-usb2", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(ci_hdrc_usb2_of_match), + }, +}; +module_platform_driver(ci_hdrc_usb2_driver); + +MODULE_DESCRIPTION("ChipIdea HDRC USB2 binding for ci13xxx"); +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); +MODULE_LICENSE("GPL");
Add a USB2 ChipIdea driver for ci13xxx, with optional PHY, clock and DMA mask, to support USB2 ChipIdea controllers that don't need specific functions. Needed for the Marvell Berlin SoCs USB controllers. Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/usb/chipidea/Makefile | 1 + drivers/usb/chipidea/ci_hdrc_usb2.c | 147 ++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 drivers/usb/chipidea/ci_hdrc_usb2.c