| Message ID | 62705234.gKJDtUQLyT@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Sep 23, 2014 at 07:37:25PM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 11:55:15 Felipe Balbi wrote: > > On Tue, Sep 23, 2014 at 06:44:40PM +0200, Arnd Bergmann wrote: > > > On Tuesday 23 September 2014 15:36:45 Antoine Tenart wrote: > > > > On Tue, Sep 23, 2014 at 12:39:04PM +0200, Arnd Bergmann wrote: > > > > > On Tuesday 23 September 2014 12:28:03 Antoine Tenart wrote: > > > > > > + if (dev->of_node) { > > > > > > + ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata); > > > > > > + if (ret) > > > > > > + goto clk_err; > > > > > > + } else { > > > > > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > > > > > > + if (ret) > > > > > > + goto clk_err; > > > > > > + } > > > > > > > > > > > > > > > > Why do you care about the non-DT case here? I think it would be nicer to > > > > > open-code the ci_hdrc_usb2_dt_probe() function in here and remove > > > > > the dma_set_mask_and_coherent(), which should not even be necessary for > > > > > the case where you have a hardwired platform device. > > > > > > > > > > > > > I thought we agreed to call dma_set_mask_and_coherent(): > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273335.html > > > > > > > > I do not have a strong opinion on this as I only use the dt case for my > > > > usage. > > > > > > The question is more about who actually wants the non-DT case. > > > > > > Since this is a new driver, I suspect that the answer is "nobody", > > > as the existing board files are all for legacy platforms that we > > > are not going to adapt for this driver. > > > > wait a minute... will the legacy platforms be adapted to DT and, thus, > > to this driver in the future ? I really don't want to keep several > > copies of chipidea driver just because there are still some legacy > > platforms still using them. I have said in the past and will say again, > > everybody should move to the generic chipidea driver at the earliest > > opportunity so we avoid duplication of work. > > > > Sorry, my mistake. The intention that this new driver is meant to > replace the existing ones wasn't clear to me from the changelog, and > if I'd been involved in the discussion before, then I've forgotten > about it. > > It absolutely makes sense to migrate to a common driver, and in that > case we should keep the platform_data handling and > dma_set_mask_and_coherent() call in there, so we can do the two > conversions (migrating from platform specific frontends to the generic > one, and migrating from platform_data to DT) on independent schedules. makes sense to me. > Eventually I'd like all of the existing users of the platform_data > path to move to DT, but that should not hold up the other cleanup if > it takes longer. yeah, certainly. > There is however still my point that we shouldn't have an extra > platform device that is not attached to the device node. I think the > generic driver should just be part of the common code, without an > extra framework. Something like the (entirely untested) patch below. yeah, that's what I did on dwc3 at least. We support platform_data and DT on the core driver. As for glue layers, we have ST, Qcom, PCI, OMAP, Exynos and Keystone. The only difference is that core dwc3 still doesn't know about clocks, but that's not an issue right now because we're not yet supporting pm_runtime.
On Tue, Sep 23, 2014 at 07:37:25PM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 11:55:15 Felipe Balbi wrote: > > On Tue, Sep 23, 2014 at 06:44:40PM +0200, Arnd Bergmann wrote: > > > On Tuesday 23 September 2014 15:36:45 Antoine Tenart wrote: > > > > On Tue, Sep 23, 2014 at 12:39:04PM +0200, Arnd Bergmann wrote: > > > > > On Tuesday 23 September 2014 12:28:03 Antoine Tenart wrote: > > > > > > + if (dev->of_node) { > > > > > > + ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata); > > > > > > + if (ret) > > > > > > + goto clk_err; > > > > > > + } else { > > > > > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > > > > > > + if (ret) > > > > > > + goto clk_err; > > > > > > + } > > > > > > > > > > > > > > > > Why do you care about the non-DT case here? I think it would be nicer to > > > > > open-code the ci_hdrc_usb2_dt_probe() function in here and remove > > > > > the dma_set_mask_and_coherent(), which should not even be necessary for > > > > > the case where you have a hardwired platform device. > > > > > > > > > > > > > I thought we agreed to call dma_set_mask_and_coherent(): > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/273335.html > > > > > > > > I do not have a strong opinion on this as I only use the dt case for my > > > > usage. > > > > > > The question is more about who actually wants the non-DT case. > > > > > > Since this is a new driver, I suspect that the answer is "nobody", > > > as the existing board files are all for legacy platforms that we > > > are not going to adapt for this driver. > > > > wait a minute... will the legacy platforms be adapted to DT and, thus, > > to this driver in the future ? I really don't want to keep several > > copies of chipidea driver just because there are still some legacy > > platforms still using them. I have said in the past and will say again, > > everybody should move to the generic chipidea driver at the earliest > > opportunity so we avoid duplication of work. > > > > Sorry, my mistake. The intention that this new driver is meant to > replace the existing ones wasn't clear to me from the changelog, > and if I'd been involved in the discussion before, then I've forgotten > about it. > > It absolutely makes sense to migrate to a common driver, and in that > case we should keep the platform_data handling and dma_set_mask_and_coherent() > call in there, so we can do the two conversions (migrating from platform > specific frontends to the generic one, and migrating from platform_data > to DT) on independent schedules. Eventually I'd like all of the existing > users of the platform_data path to move to DT, but that should not > hold up the other cleanup if it takes longer. > > There is however still my point that we shouldn't have an extra platform > device that is not attached to the device node. I think the generic driver > should just be part of the common code, without an extra framework. > Something like the (entirely untested) patch below. > > Arnd Thanks, Arnd. Antoine is adding a generic chipdea glue layer driver, which like ehci generic platform driver: drivers/usb/host/ehci-platform.c, since other architectures like MIPS (Someone submitted mips chipidea driver before) may not have device tree support, I think non-dt support is also needed. It is a good suggestion for adding DT support for core driver, Since we did not do it at the first, it is a little embarrass at current situation. - For the new chipidea glue drivers, it is ok we can have a child node for core device at glue device node, and some common entries can be there like: phy, vbus, dr_mode, etc. We need to add support for getting these common things for both through device tree and platform data (parent is DT support and parent is non-DT support) at core driver. - For the existing glue drivers, since we can't change existed dts, we can only do it from future SoC support. Then, in this kinds of glue drivers, we need to support for both create core driver by node and by current calling platform_device_add way. Peter > > --- > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h > index 9563cb56d564..a2b20c1342f1 100644 > --- a/drivers/usb/chipidea/ci.h > +++ b/drivers/usb/chipidea/ci.h > @@ -207,6 +207,7 @@ struct ci_hdrc { > bool id_event; > bool b_sess_valid_event; > bool imx28_write_fix; > + struct clk *clk; > }; > > static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci) > diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c > index 6eae1de587f2..03ef35997dd8 100644 > --- a/drivers/usb/chipidea/ci_hdrc_usb2.c > +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c > @@ -70,6 +70,7 @@ static int ci_hdrc_usb2_probe(struct platform_device *pdev) > } > > if (dev->of_node) { > + ret = ci_get_platdata(dev, platdata); > ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata); > if (ret) > goto clk_err; > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 619d13e29995..32613751e731 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -478,6 +478,15 @@ static int ci_get_platdata(struct device *dev, > if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL) > platdata->flags |= CI_HDRC_FORCE_FULLSPEED; > > + platdata->phy = of_phy_get(dev->of_node, 0); > + if (IS_ERR(platdata->phy)) { > + if (PTR_ERR(platdata->phy) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + /* PHY is optional */ > + platdata->phy = NULL; > + } > + > return 0; > } > > @@ -559,6 +568,12 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) > dev_dbg(ci->dev, "It is OTG capable controller\n"); > } > > +static const struct ci_hdrc_platform_data ci_default_pdata = { > + .capoffset = DEF_CAPOFFSET, > + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | > + CI_HDRC_DISABLE_STREAMING, > +}; > + > static int ci_hdrc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -568,11 +583,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) > int ret; > enum usb_dr_mode dr_mode; > > - if (!dev_get_platdata(dev)) { > - dev_err(dev, "platform data missing\n"); > - return -ENODEV; > - } > - > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(dev, res); > if (IS_ERR(base)) > @@ -586,6 +596,32 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > ci->dev = dev; > ci->platdata = dev_get_platdata(dev); > + if (!ci->platdata) { > + ci->platdata = devm_kmemdup(dev, &ci_default_pdata, > + sizeof(ci_default_pdata), > + GFP_KERNEL); > + > + if (!ci->platdata) > + return -ENOMEM; > + > + ret = ci_get_platdata(dev, platdata); > + if (ret) > + return ERR_PTR(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; > + } > + } > + > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + if (ret) > + return ret; > + > ci->imx28_write_fix = !!(ci->platdata->flags & > CI_HDRC_IMX28_WRITE_FIX); > > @@ -702,8 +738,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) > } > > platform_set_drvdata(pdev, ci); > - ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name, > - ci); > + ret = devm_request_irq(ci->irq, ci_irq, IRQF_SHARED, > + ci->platdata->name, ci); > if (ret) > goto stop; > > @@ -711,10 +747,13 @@ static int ci_hdrc_probe(struct platform_device *pdev) > ci_hdrc_otg_fsm_start(ci); > > ret = dbg_create_files(ci); > - if (!ret) > - return 0; > + if (ret) > + goto stop; > + > + pm_runtime_no_callbacks(dev); > + pm_runtime_enable(dev); > + return 0; > > - free_irq(ci->irq, ci); > stop: > ci_role_destroy(ci); > deinit_phy: > @@ -727,22 +766,32 @@ static int ci_hdrc_remove(struct platform_device *pdev) > { > struct ci_hdrc *ci = platform_get_drvdata(pdev); > > + pm_runtime_disable(&pdev->dev); > dbg_remove_files(ci); > free_irq(ci->irq, ci); > ci_role_destroy(ci); > ci_hdrc_enter_lpm(ci, true); > usb_phy_shutdown(ci->transceiver); > kfree(ci->hw_bank.regmap); > + clk_disable_unprepare(ci->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_driver = { > .probe = ci_hdrc_probe, > .remove = ci_hdrc_remove, > .driver = { > .name = "ci_hdrc", > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ci_hdrc_usb2_of_match), > }, > }; > >
On Wednesday 24 September 2014 10:27:52 Peter Chen wrote: > > Antoine is adding a generic chipdea glue layer driver, which like ehci generic > platform driver: drivers/usb/host/ehci-platform.c, since other architectures > like MIPS (Someone submitted mips chipidea driver before) may not have device > tree support, I think non-dt support is also needed. Right. > It is a good suggestion for adding DT support for core driver, Since we did > not do it at the first, it is a little embarrass at current situation. > > - For the new chipidea glue drivers, it is ok we can have a child node > for core device at glue device node, and some common entries can be there > like: phy, vbus, dr_mode, etc. We need to add support for getting > these common things for both through device tree and platform data > (parent is DT support and parent is non-DT support) at core driver. I don't really want to see the child devices show up in DT, as this is really an implementation detail of the way that the driver currently works, and IMHO we should change that. I know that Felipe disagrees with me on this, but almost every other driver that has soc-specific specializations does not use parent/child nodes, neither in DT nor in Linux. Instead you have a single device node that gets distinguished by "compatible" string. > - For the existing glue drivers, since we can't change existed dts, we can > only do it from future SoC support. Then, in this kinds of glue drivers, > we need to support for both create core driver by node and by current > calling platform_device_add way. We should be able to easily change the ci_hdrc_add_device call into a different exported function that calls a version of the probe() code directly, as we do in all sorts of other drivers (ehci, ohci, dwc2, ..., but not dwc3 and musb as they are maintained by Felipe). We can also gradually move in some of the other glue drivers into the main driver if the differences are small enough. Moving the PCI driver over to this model requires a little more work but is absolutely doable (again, see ehci, ahci, sdhci examples) by moving the platform_device handling out of core.c and changing it just operate on the plain 'struct device', with an exported probe function that gets called on either the platform device or the pci device. Arnd
On Wednesday 24 September 2014 09:44:19 Arnd Bergmann wrote: > > We can also gradually move in some of the other glue drivers into > the main driver if the differences are small enough. > FWIW, I've just looked at the other glue drivers that already exist: - zevio can just get merged into the common driver, all that seems to be needed for that is the additional compatible string, and keying off the ci_default_zevio_platdata on the .data field of the of_device_id table. - msm has a custom notifier, which justifies leaving it in a separate driver, but it's also small enough that it wouldn't hurt to have that merged into the main driver too. - imx requires a lot of other things, in particular the dependency on the usbmisc driver means we don't want to have that in the core anyway, so we can't really merge that in. - the proposed ar933x driver again looks almost trivial, so no reason for a separate glue driver for that. Arnd
On Wed, Sep 24, 2014 at 10:30:41AM +0200, Arnd Bergmann wrote: > On Wednesday 24 September 2014 09:44:19 Arnd Bergmann wrote: > > > > We can also gradually move in some of the other glue drivers into > > the main driver if the differences are small enough. > > > > FWIW, I've just looked at the other glue drivers that already > exist: > > - zevio can just get merged into the common driver, all that seems > to be needed for that is the additional compatible string, and > keying off the ci_default_zevio_platdata on the .data field of > the of_device_id table. > > - msm has a custom notifier, which justifies leaving it in a separate > driver, but it's also small enough that it wouldn't hurt to have > that merged into the main driver too. > > - imx requires a lot of other things, in particular the dependency > on the usbmisc driver means we don't want to have that in the > core anyway, so we can't really merge that in. > > - the proposed ar933x driver again looks almost trivial, so no reason > for a separate glue driver for that. > > Arnd Thanks, Arnd. So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver (dwc3, musb, chipidea) you are talking about, right? Except for creating another platform driver as well as related DT node (optional), are there any advantages compared to current IP core driver structure? In this thread, we are talking about creating common platform driver for glue layer, its design purpose (adapt it for as many as platforms) should be the same, no matter the IP core part is a LIB or platform driver, am I missing something?
On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > (dwc3, musb, chipidea) you are talking about, right? Except for > creating another platform driver as well as related DT node (optional), > are there any advantages compared to current IP core driver structure? Having a library module usually requires less code, and is more consistent with other drivers, which helps new developers understand what the driver is doing. The most important aspect though is the DT binding: once the structure with separate kernel drivers leaks out into the DT format, you can't easily change the driver any more, e.g. to make a property that was introduced for one glue driver more general so it can get handled by the common part. Having a single node lets us convert to the library model later, so that would be a strong reason to keep the DT binding simple, without child nodes. Following that logic, it turns into another reason for using the library model to start with, because otherwise the child device does not have any DT properties itself and has to rely on the parent device properties, which somewhat complicates the driver structure. > In this thread, we are talking about creating common platform driver for glue > layer, its design purpose (adapt it for as many as platforms) should be the > same, no matter the IP core part is a LIB or platform driver, am I missing > something? No, you are absolutely right with that, introducing the generic glue driver is orthogonal to the structure of the core driver in principle. I mainly brought it up because I noticed how this patch could be done in a simpler way by combining the new generic glue with the move to a library driver model. Arnd
On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > (dwc3, musb, chipidea) you are talking about, right? Except for > > creating another platform driver as well as related DT node (optional), > > are there any advantages compared to current IP core driver structure? > > Having a library module usually requires less code, and is more consistent > with other drivers, which helps new developers understand what the > driver is doing. Yes, more consistent is a good reason. > > The most important aspect though is the DT binding: once the structure > with separate kernel drivers leaks out into the DT format, you can't > easily change the driver any more, e.g. to make a property that was > introduced for one glue driver more general so it can get handled by > the common part. Having a single node lets us convert to the library > model later, so that would be a strong reason to keep the DT binding > simple, without child nodes. > > Following that logic, it turns into another reason for using the library > model to start with, because otherwise the child device does not have > any DT properties itself and has to rely on the parent device properties, > which somewhat complicates the driver structure. > Although it is not a problem for chipidea, chipidea core exports API to handle parent's common DT entries. It is a little inconsistent for current chipidea core driver, it works as a driver, but exports some APIs for other drivers use which is like a library. One more benefit I can see is the platform driver can visit the common struct like io address if using library, currently, the platform driver isn't suitable to visit core driver's struct, since they are two drivers. For chipidea, it doesn't has child device DT node, so it can convert to core library + platform driver model with moderate effort, I will do it at next two versions, thanks.
Hi, On Wed, Sep 24, 2014 at 09:44:19AM +0200, Arnd Bergmann wrote: > > It is a good suggestion for adding DT support for core driver, Since we did > > not do it at the first, it is a little embarrass at current situation. > > > > - For the new chipidea glue drivers, it is ok we can have a child node > > for core device at glue device node, and some common entries can be there > > like: phy, vbus, dr_mode, etc. We need to add support for getting > > these common things for both through device tree and platform data > > (parent is DT support and parent is non-DT support) at core driver. > > I don't really want to see the child devices show up in DT, as this is > really an implementation detail of the way that the driver currently works, > and IMHO we should change that. > > I know that Felipe disagrees with me on this, but almost every other > driver that has soc-specific specializations does not use parent/child > nodes, neither in DT nor in Linux. Instead you have a single device > node that gets distinguished by "compatible" string. I have two answers for this: 1) We need to let different buses use the same device. The same IP can be used on PCI, platform, OMAP (yes, OMAP is pretty much a bus, even though we're using platform-bus with a bunch of code to match things), AMBA, etc. 2) I like to mode the HW in code and if you look into RTL you'll see that PCIe, OMAP, QCOM, Exynos, etc, take a semi-bus-agnostic core IP and write a wrapper IP to make it fit into the SoC. That Wrapper is also an IP of its own and has its own clocks, sometimes its own IRQs. Not to mention that PM requirements for different architectures might be (and actually are) different, while PM requirements for the core IP is "generic" because it comes from IP provider. By using a parent device, we can hide all platform-/bus-specific details on the parent driver and keep core driver "pristine". I see many, many benefits of doing things this way and many of the benefits are already cooked into the driver model itself, so why not use it ? > > - For the existing glue drivers, since we can't change existed dts, we can > > only do it from future SoC support. Then, in this kinds of glue drivers, > > we need to support for both create core driver by node and by current > > calling platform_device_add way. > > We should be able to easily change the ci_hdrc_add_device call into > a different exported function that calls a version of the probe() > code directly, as we do in all sorts of other drivers (ehci, ohci, > dwc2, ..., but not dwc3 and musb as they are maintained by Felipe). Go back a few commits and you'll see EHCI couldn't even be built with different glues (say ehci-omap and ehci-pci). > We can also gradually move in some of the other glue drivers into > the main driver if the differences are small enough. you'll end up with a bunch of conflicting requirements very soon. In fact that's one reason for MUSB being a mess. It was, originally, a single driver with platform-specific glue chosen in build-time. To this day, we still can't have different DMA glues on the kernel and TUSB6010-glue doesn't work with anybody else. When you have a single driver like that, you end up accepting platform-specific details into the core IP just because the changes are small enough. > Moving the PCI driver over to this model requires a little more > work but is absolutely doable (again, see ehci, ahci, sdhci examples) > by moving the platform_device handling out of core.c and changing > it just operate on the plain 'struct device', with an exported > probe function that gets called on either the platform device > or the pci device. sounds like a disaster waiting to happen for me.
On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > (dwc3, musb, chipidea) you are talking about, right? Except for > > creating another platform driver as well as related DT node (optional), > > are there any advantages compared to current IP core driver structure? > > Having a library module usually requires less code, and is more > consistent with other drivers, which helps new developers understand > what the driver is doing. I beg to differ. You end-up having to pass function pointers through platform_data to handle differences which could be handled by the core IP. > The most important aspect though is the DT binding: once the structure > with separate kernel drivers leaks out into the DT format, you can't > easily change the driver any more, e.g. to make a property that was > introduced for one glue driver more general so it can get handled by > the common part. Having a single node lets us convert to the library > model later, so that would be a strong reason to keep the DT binding > simple, without child nodes. > > Following that logic, it turns into another reason for using the library > model to start with, because otherwise the child device does not have > any DT properties itself and has to rely on the parent device properties, > which somewhat complicates the driver structure. this is bullcrap. Take a look at Documentation/devicetree/bindings/usb/dwc3.txt: synopsys DWC3 CORE DWC3- USB3 CONTROLLER Required properties: - compatible: must be "snps,dwc3" - reg : Address and length of the register set for the device - interrupts: Interrupts used by the dwc3 controller. Optional properties: - usb-phy : array of phandle for the PHY device. The first element in the array is expected to be a handle to the USB2/HS PHY and the second element is expected to be a handle to the USB3/SS PHY - phys: from the *Generic PHY* bindings - phy-names: from the *Generic PHY* bindings - tx-fifo-resize: determines if the FIFO *has* to be reallocated. This is usually a subnode to DWC3 glue to which it is connected. dwc3@4a030000 { compatible = "snps,dwc3"; reg = <0x4a030000 0xcfff>; interrupts = <0 92 4> usb-phy = <&usb2_phy>, <&usb3,phy>; tx-fifo-resize; }; This contains all the attributes it needs to work. In fact, this could be used without any glue.
On Thu, Sep 25, 2014 at 09:15:53AM -0500, Felipe Balbi wrote: > On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > > (dwc3, musb, chipidea) you are talking about, right? Except for > > > creating another platform driver as well as related DT node (optional), > > > are there any advantages compared to current IP core driver structure? > > > > Having a library module usually requires less code, and is more > > consistent with other drivers, which helps new developers understand > > what the driver is doing. > > I beg to differ. You end-up having to pass function pointers through > platform_data to handle differences which could be handled by the core > IP. > > > The most important aspect though is the DT binding: once the structure > > with separate kernel drivers leaks out into the DT format, you can't > > easily change the driver any more, e.g. to make a property that was > > introduced for one glue driver more general so it can get handled by > > the common part. Having a single node lets us convert to the library > > model later, so that would be a strong reason to keep the DT binding > > simple, without child nodes. > > > > Following that logic, it turns into another reason for using the library > > model to start with, because otherwise the child device does not have > > any DT properties itself and has to rely on the parent device properties, > > which somewhat complicates the driver structure. > > this is bullcrap. Take a look at > Documentation/devicetree/bindings/usb/dwc3.txt: > > synopsys DWC3 CORE > > DWC3- USB3 CONTROLLER > > Required properties: > - compatible: must be "snps,dwc3" > - reg : Address and length of the register set for the device > - interrupts: Interrupts used by the dwc3 controller. > > Optional properties: > - usb-phy : array of phandle for the PHY device. The first element > in the array is expected to be a handle to the USB2/HS PHY and > the second element is expected to be a handle to the USB3/SS PHY > - phys: from the *Generic PHY* bindings > - phy-names: from the *Generic PHY* bindings > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > This is usually a subnode to DWC3 glue to which it is connected. > > dwc3@4a030000 { > compatible = "snps,dwc3"; > reg = <0x4a030000 0xcfff>; > interrupts = <0 92 4> > usb-phy = <&usb2_phy>, <&usb3,phy>; > tx-fifo-resize; > }; > > This contains all the attributes it needs to work. In fact, this could > be used without any glue. > If you want to add "vbus-supply" core node to support ID switch, what's the plan for that? Is it possible that the glue layer needs to access core register (eg, portsc.phcd during suspend)? The wrapper IP may wait some signals at core IP to change.
HI, On Fri, Sep 26, 2014 at 07:39:34AM +0800, Peter Chen wrote: > On Thu, Sep 25, 2014 at 09:15:53AM -0500, Felipe Balbi wrote: > > On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > > > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > > > (dwc3, musb, chipidea) you are talking about, right? Except for > > > > creating another platform driver as well as related DT node (optional), > > > > are there any advantages compared to current IP core driver structure? > > > > > > Having a library module usually requires less code, and is more > > > consistent with other drivers, which helps new developers understand > > > what the driver is doing. > > > > I beg to differ. You end-up having to pass function pointers through > > platform_data to handle differences which could be handled by the core > > IP. > > > > > The most important aspect though is the DT binding: once the structure > > > with separate kernel drivers leaks out into the DT format, you can't > > > easily change the driver any more, e.g. to make a property that was > > > introduced for one glue driver more general so it can get handled by > > > the common part. Having a single node lets us convert to the library > > > model later, so that would be a strong reason to keep the DT binding > > > simple, without child nodes. > > > > > > Following that logic, it turns into another reason for using the library > > > model to start with, because otherwise the child device does not have > > > any DT properties itself and has to rely on the parent device properties, > > > which somewhat complicates the driver structure. > > > > this is bullcrap. Take a look at > > Documentation/devicetree/bindings/usb/dwc3.txt: > > > > synopsys DWC3 CORE > > > > DWC3- USB3 CONTROLLER > > > > Required properties: > > - compatible: must be "snps,dwc3" > > - reg : Address and length of the register set for the device > > - interrupts: Interrupts used by the dwc3 controller. > > > > Optional properties: > > - usb-phy : array of phandle for the PHY device. The first element > > in the array is expected to be a handle to the USB2/HS PHY and > > the second element is expected to be a handle to the USB3/SS PHY > > - phys: from the *Generic PHY* bindings > > - phy-names: from the *Generic PHY* bindings > > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > > > This is usually a subnode to DWC3 glue to which it is connected. > > > > dwc3@4a030000 { > > compatible = "snps,dwc3"; > > reg = <0x4a030000 0xcfff>; > > interrupts = <0 92 4> > > usb-phy = <&usb2_phy>, <&usb3,phy>; > > tx-fifo-resize; > > }; > > > > This contains all the attributes it needs to work. In fact, this could > > be used without any glue. > > > > If you want to add "vbus-supply" core node to support ID switch, what's > the plan for that? send a patch ? Just make sure it's optional because not everybody needs a vbus-supply. Also, DRD will still take a few merge windows to become a reality. I don't want to merge something before considering it carefuly, otherwise we will having which is broken and doesn't work for everybody ;-) > Is it possible that the glue layer needs to access core register > (eg, portsc.phcd during suspend)? The wrapper IP may wait some signals > at core IP to change. why would a glue layer need to access registers from the core ? That sounds very odd. I haven't seen that and will, definitely, NACK such a patch :-) can you further describe why you think a glue layer might need to access core IP's registers ?
Hi again, On Thu, Sep 25, 2014 at 07:37:50PM -0500, Felipe Balbi wrote: > On Fri, Sep 26, 2014 at 07:39:34AM +0800, Peter Chen wrote: > > On Thu, Sep 25, 2014 at 09:15:53AM -0500, Felipe Balbi wrote: > > > On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > > > > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > > > > (dwc3, musb, chipidea) you are talking about, right? Except for > > > > > creating another platform driver as well as related DT node (optional), > > > > > are there any advantages compared to current IP core driver structure? > > > > > > > > Having a library module usually requires less code, and is more > > > > consistent with other drivers, which helps new developers understand > > > > what the driver is doing. > > > > > > I beg to differ. You end-up having to pass function pointers through > > > platform_data to handle differences which could be handled by the core > > > IP. > > > > > > > The most important aspect though is the DT binding: once the structure > > > > with separate kernel drivers leaks out into the DT format, you can't > > > > easily change the driver any more, e.g. to make a property that was > > > > introduced for one glue driver more general so it can get handled by > > > > the common part. Having a single node lets us convert to the library > > > > model later, so that would be a strong reason to keep the DT binding > > > > simple, without child nodes. > > > > > > > > Following that logic, it turns into another reason for using the library > > > > model to start with, because otherwise the child device does not have > > > > any DT properties itself and has to rely on the parent device properties, > > > > which somewhat complicates the driver structure. > > > > > > this is bullcrap. Take a look at > > > Documentation/devicetree/bindings/usb/dwc3.txt: > > > > > > synopsys DWC3 CORE > > > > > > DWC3- USB3 CONTROLLER > > > > > > Required properties: > > > - compatible: must be "snps,dwc3" > > > - reg : Address and length of the register set for the device > > > - interrupts: Interrupts used by the dwc3 controller. > > > > > > Optional properties: > > > - usb-phy : array of phandle for the PHY device. The first element > > > in the array is expected to be a handle to the USB2/HS PHY and > > > the second element is expected to be a handle to the USB3/SS PHY > > > - phys: from the *Generic PHY* bindings > > > - phy-names: from the *Generic PHY* bindings > > > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > > > > > This is usually a subnode to DWC3 glue to which it is connected. > > > > > > dwc3@4a030000 { > > > compatible = "snps,dwc3"; > > > reg = <0x4a030000 0xcfff>; > > > interrupts = <0 92 4> > > > usb-phy = <&usb2_phy>, <&usb3,phy>; > > > tx-fifo-resize; > > > }; > > > > > > This contains all the attributes it needs to work. In fact, this could > > > be used without any glue. > > > > > > > If you want to add "vbus-supply" core node to support ID switch, what's > > the plan for that? > > send a patch ? Just make sure it's optional because not everybody needs > a vbus-supply. Also, DRD will still take a few merge windows to become a > reality. I don't want to merge something before considering it carefuly, > otherwise we will having which is broken and doesn't work for everybody > ;-) > > > Is it possible that the glue layer needs to access core register > > (eg, portsc.phcd during suspend)? The wrapper IP may wait some signals > > at core IP to change. > > why would a glue layer need to access registers from the core ? That > sounds very odd. I haven't seen that and will, definitely, NACK such a > patch :-) > > can you further describe why you think a glue layer might need to access > core IP's registers ? I just realised we're talking about chipidea here... in any case, it's still valid to ask why would glue need to fiddle with core IP's registers.
On Thursday 25 September 2014 19:39:34 Felipe Balbi wrote: > > > > why would a glue layer need to access registers from the core ? That > > sounds very odd. I haven't seen that and will, definitely, NACK such a > > patch > > > > can you further describe why you think a glue layer might need to access > > core IP's registers ? > > I just realised we're talking about chipidea here... in any case, it's > still valid to ask why would glue need to fiddle with core IP's > registers. Generally, the glue driver wouldn't access the registers, but I don't think it's important to prevent it from doing that. In some cases, a glue driver needs to override a function of the core driver, e.g. to work around an errata. We have a lot of those quirks in ATA drivers, one example from ahci_mvebu.c is static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv) { /* * Enable the regret bit to allow the SATA unit to regret a * request that didn't receive an acknowlegde and avoid a * deadlock */ writel(0x4, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_ADDR); writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA); } Arnd
Hi, On Fri, Sep 26, 2014 at 09:20:54AM +0200, Arnd Bergmann wrote: > On Thursday 25 September 2014 19:39:34 Felipe Balbi wrote: > > > > > > why would a glue layer need to access registers from the core ? That > > > sounds very odd. I haven't seen that and will, definitely, NACK such a > > > patch > > > > > > can you further describe why you think a glue layer might need to access > > > core IP's registers ? > > > > I just realised we're talking about chipidea here... in any case, it's > > still valid to ask why would glue need to fiddle with core IP's > > registers. > > Generally, the glue driver wouldn't access the registers, but I don't > think it's important to prevent it from doing that. In some cases, sure it is. Have already gone through debugging sessions just because someone fiddled with registers they shouldn't. Also RMK's L2 rework patchset is another example of why it's important to prevent other layers from messing with registers they don't really own. > a glue driver needs to override a function of the core driver, e.g. > to work around an errata. We have a lot of those quirks in ATA drivers, pass a quirk flag and let core driver handle it. > one example from ahci_mvebu.c is > > static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv) > { > /* > * Enable the regret bit to allow the SATA unit to regret a > * request that didn't receive an acknowlegde and avoid a > * deadlock > */ > writel(0x4, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_ADDR); > writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA); I would rather see: if (this_is_one_of_the_broken_mvebu_versions(hpriv)) quirks |= AHCI_NEEDS_REGRET_BIT; then let core handle the rest. If other glue has the same bug and needs the workaround, we don't duplicate code.
On Thu, Sep 25, 2014 at 07:37:50PM -0500, Felipe Balbi wrote: > HI, > > On Fri, Sep 26, 2014 at 07:39:34AM +0800, Peter Chen wrote: > > On Thu, Sep 25, 2014 at 09:15:53AM -0500, Felipe Balbi wrote: > > > On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > > > > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > > > > (dwc3, musb, chipidea) you are talking about, right? Except for > > > > > creating another platform driver as well as related DT node (optional), > > > > > are there any advantages compared to current IP core driver structure? > > > > > > > > Having a library module usually requires less code, and is more > > > > consistent with other drivers, which helps new developers understand > > > > what the driver is doing. > > > > > > I beg to differ. You end-up having to pass function pointers through > > > platform_data to handle differences which could be handled by the core > > > IP. > > > > > > > The most important aspect though is the DT binding: once the structure > > > > with separate kernel drivers leaks out into the DT format, you can't > > > > easily change the driver any more, e.g. to make a property that was > > > > introduced for one glue driver more general so it can get handled by > > > > the common part. Having a single node lets us convert to the library > > > > model later, so that would be a strong reason to keep the DT binding > > > > simple, without child nodes. > > > > > > > > Following that logic, it turns into another reason for using the library > > > > model to start with, because otherwise the child device does not have > > > > any DT properties itself and has to rely on the parent device properties, > > > > which somewhat complicates the driver structure. > > > > > > this is bullcrap. Take a look at > > > Documentation/devicetree/bindings/usb/dwc3.txt: > > > > > > synopsys DWC3 CORE > > > > > > DWC3- USB3 CONTROLLER > > > > > > Required properties: > > > - compatible: must be "snps,dwc3" > > > - reg : Address and length of the register set for the device > > > - interrupts: Interrupts used by the dwc3 controller. > > > > > > Optional properties: > > > - usb-phy : array of phandle for the PHY device. The first element > > > in the array is expected to be a handle to the USB2/HS PHY and > > > the second element is expected to be a handle to the USB3/SS PHY > > > - phys: from the *Generic PHY* bindings > > > - phy-names: from the *Generic PHY* bindings > > > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > > > > > This is usually a subnode to DWC3 glue to which it is connected. > > > > > > dwc3@4a030000 { > > > compatible = "snps,dwc3"; > > > reg = <0x4a030000 0xcfff>; > > > interrupts = <0 92 4> > > > usb-phy = <&usb2_phy>, <&usb3,phy>; > > > tx-fifo-resize; > > > }; > > > > > > This contains all the attributes it needs to work. In fact, this could > > > be used without any glue. > > > > > > > If you want to add "vbus-supply" core node to support ID switch, what's > > the plan for that? > > send a patch ? Just make sure it's optional because not everybody needs > a vbus-supply. Also, DRD will still take a few merge windows to become a > reality. I don't want to merge something before considering it carefuly, > otherwise we will having which is broken and doesn't work for everybody > ;-) > > > Is it possible that the glue layer needs to access core register > > (eg, portsc.phcd during suspend)? The wrapper IP may wait some signals > > at core IP to change. > > why would a glue layer need to access registers from the core ? That > sounds very odd. I haven't seen that and will, definitely, NACK such a > patch :-) > > can you further describe why you think a glue layer might need to access > core IP's registers ? > My mistake, we can just wait core layer for finishing before doing PHY and wrapper operation.
On Thu, Sep 25, 2014 at 07:37:50PM -0500, Felipe Balbi wrote: > HI, > > On Fri, Sep 26, 2014 at 07:39:34AM +0800, Peter Chen wrote: > > On Thu, Sep 25, 2014 at 09:15:53AM -0500, Felipe Balbi wrote: > > > On Wed, Sep 24, 2014 at 02:23:38PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 24 September 2014 19:29:05 Peter Chen wrote: > > > > > > > > > > So, it is IP CORE LIB (you suggest) vs IP CORE Platform Driver > > > > > (dwc3, musb, chipidea) you are talking about, right? Except for > > > > > creating another platform driver as well as related DT node (optional), > > > > > are there any advantages compared to current IP core driver structure? > > > > > > > > Having a library module usually requires less code, and is more > > > > consistent with other drivers, which helps new developers understand > > > > what the driver is doing. > > > > > > I beg to differ. You end-up having to pass function pointers through > > > platform_data to handle differences which could be handled by the core > > > IP. > > > > > > > The most important aspect though is the DT binding: once the structure > > > > with separate kernel drivers leaks out into the DT format, you can't > > > > easily change the driver any more, e.g. to make a property that was > > > > introduced for one glue driver more general so it can get handled by > > > > the common part. Having a single node lets us convert to the library > > > > model later, so that would be a strong reason to keep the DT binding > > > > simple, without child nodes. > > > > > > > > Following that logic, it turns into another reason for using the library > > > > model to start with, because otherwise the child device does not have > > > > any DT properties itself and has to rely on the parent device properties, > > > > which somewhat complicates the driver structure. > > > > > > this is bullcrap. Take a look at > > > Documentation/devicetree/bindings/usb/dwc3.txt: > > > > > > synopsys DWC3 CORE > > > > > > DWC3- USB3 CONTROLLER > > > > > > Required properties: > > > - compatible: must be "snps,dwc3" > > > - reg : Address and length of the register set for the device > > > - interrupts: Interrupts used by the dwc3 controller. > > > > > > Optional properties: > > > - usb-phy : array of phandle for the PHY device. The first element > > > in the array is expected to be a handle to the USB2/HS PHY and > > > the second element is expected to be a handle to the USB3/SS PHY > > > - phys: from the *Generic PHY* bindings > > > - phy-names: from the *Generic PHY* bindings > > > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > > > > > This is usually a subnode to DWC3 glue to which it is connected. > > > > > > dwc3@4a030000 { > > > compatible = "snps,dwc3"; > > > reg = <0x4a030000 0xcfff>; > > > interrupts = <0 92 4> > > > usb-phy = <&usb2_phy>, <&usb3,phy>; > > > tx-fifo-resize; > > > }; > > > > > > Is it possible that the glue layer needs to access core register > > (eg, portsc.phcd during suspend)? The wrapper IP may wait some signals > > at core IP to change. > > why would a glue layer need to access registers from the core ? That > sounds very odd. I haven't seen that and will, definitely, NACK such a > patch :-) > Just have a case for glue layer needs to know core layer's status. The controller's wakeup logic is usually at glue layer, but we may need to enable different wakeup for roles. Eg, if the current role is host (controller is otg capable), we can't enable id wakeup, otherwise, the user will complain plug out Micro-AB cable wakes up system which is not expected.
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 9563cb56d564..a2b20c1342f1 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -207,6 +207,7 @@ struct ci_hdrc { bool id_event; bool b_sess_valid_event; bool imx28_write_fix; + struct clk *clk; }; static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci) diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c index 6eae1de587f2..03ef35997dd8 100644 --- a/drivers/usb/chipidea/ci_hdrc_usb2.c +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c @@ -70,6 +70,7 @@ static int ci_hdrc_usb2_probe(struct platform_device *pdev) } if (dev->of_node) { + ret = ci_get_platdata(dev, platdata); ret = ci_hdrc_usb2_dt_probe(dev, ci_pdata); if (ret) goto clk_err; diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 619d13e29995..32613751e731 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -478,6 +478,15 @@ static int ci_get_platdata(struct device *dev, if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL) platdata->flags |= CI_HDRC_FORCE_FULLSPEED; + platdata->phy = of_phy_get(dev->of_node, 0); + if (IS_ERR(platdata->phy)) { + if (PTR_ERR(platdata->phy) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* PHY is optional */ + platdata->phy = NULL; + } + return 0; } @@ -559,6 +568,12 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) dev_dbg(ci->dev, "It is OTG capable controller\n"); } +static const struct ci_hdrc_platform_data ci_default_pdata = { + .capoffset = DEF_CAPOFFSET, + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | + CI_HDRC_DISABLE_STREAMING, +}; + static int ci_hdrc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -568,11 +583,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) int ret; enum usb_dr_mode dr_mode; - if (!dev_get_platdata(dev)) { - dev_err(dev, "platform data missing\n"); - return -ENODEV; - } - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(dev, res); if (IS_ERR(base)) @@ -586,6 +596,32 @@ static int ci_hdrc_probe(struct platform_device *pdev) ci->dev = dev; ci->platdata = dev_get_platdata(dev); + if (!ci->platdata) { + ci->platdata = devm_kmemdup(dev, &ci_default_pdata, + sizeof(ci_default_pdata), + GFP_KERNEL); + + if (!ci->platdata) + return -ENOMEM; + + ret = ci_get_platdata(dev, platdata); + if (ret) + return ERR_PTR(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; + } + } + + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); + if (ret) + return ret; + ci->imx28_write_fix = !!(ci->platdata->flags & CI_HDRC_IMX28_WRITE_FIX); @@ -702,8 +738,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) } platform_set_drvdata(pdev, ci); - ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name, - ci); + ret = devm_request_irq(ci->irq, ci_irq, IRQF_SHARED, + ci->platdata->name, ci); if (ret) goto stop; @@ -711,10 +747,13 @@ static int ci_hdrc_probe(struct platform_device *pdev) ci_hdrc_otg_fsm_start(ci); ret = dbg_create_files(ci); - if (!ret) - return 0; + if (ret) + goto stop; + + pm_runtime_no_callbacks(dev); + pm_runtime_enable(dev); + return 0; - free_irq(ci->irq, ci); stop: ci_role_destroy(ci); deinit_phy: @@ -727,22 +766,32 @@ static int ci_hdrc_remove(struct platform_device *pdev) { struct ci_hdrc *ci = platform_get_drvdata(pdev); + pm_runtime_disable(&pdev->dev); dbg_remove_files(ci); free_irq(ci->irq, ci); ci_role_destroy(ci); ci_hdrc_enter_lpm(ci, true); usb_phy_shutdown(ci->transceiver); kfree(ci->hw_bank.regmap); + clk_disable_unprepare(ci->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_driver = { .probe = ci_hdrc_probe, .remove = ci_hdrc_remove, .driver = { .name = "ci_hdrc", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(ci_hdrc_usb2_of_match), }, };