| Message ID | 1375821851-31609-10-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
On Tue, 6 Aug 2013 22:43:49 +0200 Gerhard Sittig <gsi@denx.de> wrote: ... > diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c > index 46ac1dd..549ff08 100644 > --- a/arch/powerpc/sysdev/fsl_pci.c > +++ b/arch/powerpc/sysdev/fsl_pci.c ... > + clk = devm_clk_get(&pdev->dev, "per"); > + if (!IS_ERR(clk)) { > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(dev, "Could not enable peripheral clock\n"); above line will break building. s/dev,/&pdev->dev,/ Thanks, Anatolij
On Thu, Aug 08, 2013 at 22:12 +0200, Anatolij Gustschin wrote: > > On Tue, 6 Aug 2013 22:43:49 +0200 > Gerhard Sittig <gsi@denx.de> wrote: > ... > > diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c > > index 46ac1dd..549ff08 100644 > > --- a/arch/powerpc/sysdev/fsl_pci.c > > +++ b/arch/powerpc/sysdev/fsl_pci.c > ... > > + clk = devm_clk_get(&pdev->dev, "per"); > > + if (!IS_ERR(clk)) { > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(dev, "Could not enable peripheral clock\n"); > > above line will break building. s/dev,/&pdev->dev,/ Thank you for testing and for the feedback. I've queued this fix for v5. The bug could hide because the file gets compiled on MPC512x, but this specific routine sits behind even more compile time switches (85xx and 86xx related). virtually yours Gerhard Sittig
[ re-created the Cc: list, this is about the PCI clock exclusively ] Of all the "preparation" patches in the series (parts 01-14/31, forming the "peripheral driver cleanup" phase before the introduction of CCF support), this patch remains the last to get picked up. But I'd suggest to leave this patch for now (for v3.12, it's rather late). Either ignore this message and the patch, or see below for why application isn't required now, and an update of this patch is needed and will be appropriate for v3.13. I'm sorry for the confusion, the potentially perceived instability is a result of both widening the series' scope after initial submission as well as a recent extension of test coverage after the scope has been widened. Thank you for your patience! On Tue, Aug 06, 2013 at 22:43 +0200, Gerhard Sittig wrote: > > make the Freescale PCI driver get, prepare and enable the PCI clock > during probe(); the clock gets put upon device close by the devm approach > > clock lookup is non-fatal as not all platforms may provide clock specs > in their device tree, but failure to enable specified clocks are fatal > > the driver appears to not have a remove() routine, so no reference to > the clock is kept during use, and the clock isn't released (the devm > approach will put the clock, but it won't get disabled or unprepared) > > Signed-off-by: Gerhard Sittig <gsi@denx.de> > --- > arch/powerpc/sysdev/fsl_pci.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c > index 46ac1dd..549ff08 100644 > --- a/arch/powerpc/sysdev/fsl_pci.c > +++ b/arch/powerpc/sysdev/fsl_pci.c > @@ -17,6 +17,8 @@ > ... What this patch 09/31 does is add a non-fatal device tree based clock lookup in the fsl_pci_probe() routine, to acquire the PCI clock item appropriately if there is a provider and a DT spec. The patch in v4 has a bug, which has an obvious fix while an update wasn't sent yet, for neither the patch nor the series. There is one more known issue in the series (not with functionality but with policy, specifically in a multi platform configuration), while I don't want to resend the series while known issues are pending. But this is not the problem here. First of all the patch is a NOP in the forseeable future. It won't harm yet its content isn't urgently needed either, to unbreak stuff or to support upcoming features that were communicated before. Further analysis has shown that the patch is incomplete. The 85xx and 86xx platforms will pass through the fsl_pci_probe() routine. That these platforms don't have OF clock providers is not a problem, the patch will remain a NOP then. Its function will kick in when these platforms may grow clock providers (things will transparently keep working, this was the actual intent of the patch). Since the series is about 512x CCF support, the patch will remain a NOP throughout the whole series, but won't harm either. The 83xx and 512x platforms in contrast _don't_ pass through the fsl_pci_probe() routine, instead they call mpc83xx_add_bridge() from within the .setup_arch() callback in platform initialization code, which iterates over the compatible OF nodes, and runs at a point in time where the platform's clock provider has not yet been setup and thus is not available. In this situation any clock lookup will fail, which is not fatal during PCI setup yet won't acquire the clock item and thus will have the common infrastructure disable the "unused" clock much later. There is a workaround for this lack of proper clock acquisition in the peripheral driver. The clock provider needs to pre-enable the PCI clock item upon its initialization, because the peripheral driver can't when it initializes. Checking the same condition in the provider's pre-enable workaround which the .setup_arch() routine is checking before the add_bridge() calls (the presence of compatible nodes) results in correct operation as well as most appropriate resource use (clock enabled when PCI hardware was attached to, and clock disabled in the absence of PCI hardware or driver attachment). So the update of this patch 09/31 will contain - the fix for the copy'n'paste bug in the probe() routine - an appropriate comment in the add_bridge() routine - no change in its nature, the idea remains unaffected The backend (clock provider) will contain the pre-enable workaround for the PCI clock item. As a result, the 83xx, 85xx, and 86xx platforms won't see any change (there is a NOP in probe() and a comment in add_bridge(), neither of which break any operation). The 512x platform will have proper PCI operation in the presence of common clock support. Should 8xxx platforms grow CCF support later, they will transparently keep working (85xx, 86xx), or may add the same simple yet appropriate workaround (83xx). So the outline is there, the approach is straight forward and easily can get implemented, and the resulting code will work for all platforms while there is no potential for breakage. The PCI driver will improve, and all is well. :) There is no need for action for v3.12, and v3.13 can include the improvement. virtually yours Gerhard Sittig
On Wed, Aug 28, 2013 at 14:08 +0200, Gerhard Sittig wrote: > > [ re-created the Cc: list, this is about the PCI clock exclusively ] I just noticed by coincidence that the message which I received back from the linuxppc-dev ML appeared to have dropped Benjamin Herrenschmidt and Kumar Gala from the Cc: list -- while they do appear in the header of the message that I have sent and I can't see what might have caused the loss of information. :-O Do you want me to re-send the message for the benefit of potential followups, or is it OK that you receive the message via the list but potentially without the Cc: attribute? The message was mostly "for your information" and contained a status update, while no action is required or problems need to get resolved. virtually yours Gerhard Sittig
On Wed, 2013-08-28 at 17:59 +0200, Gerhard Sittig wrote: > On Wed, Aug 28, 2013 at 14:08 +0200, Gerhard Sittig wrote: > > > > [ re-created the Cc: list, this is about the PCI clock exclusively ] > > I just noticed by coincidence that the message which I received > back from the linuxppc-dev ML appeared to have dropped Benjamin > Herrenschmidt and Kumar Gala from the Cc: list -- while they do > appear in the header of the message that I have sent and I can't > see what might have caused the loss of information. :-O Don't bother with me. I haven't had the bandwidth to look at that at all. I'll leave Anatolij the responsibility here :-) Cheers, Ben. > Do you want me to re-send the message for the benefit of > potential followups, or is it OK that you receive the message via > the list but potentially without the Cc: attribute? > > The message was mostly "for your information" and contained a > status update, while no action is required or problems need to > get resolved. > > > virtually yours > Gerhard Sittig
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c index 46ac1dd..549ff08 100644 --- a/arch/powerpc/sysdev/fsl_pci.c +++ b/arch/powerpc/sysdev/fsl_pci.c @@ -17,6 +17,8 @@ * Free Software Foundation; either version 2 of the License, or (at your * option) any later version. */ + +#include <linux/clk.h> #include <linux/kernel.h> #include <linux/pci.h> #include <linux/delay.h> @@ -926,12 +928,32 @@ void fsl_pci_assign_primary(void) static int fsl_pci_probe(struct platform_device *pdev) { + struct clk *clk; int ret; struct device_node *node; #ifdef CONFIG_SWIOTLB struct pci_controller *hose; #endif + /* + * clock lookup is non-fatal since the driver is shared among + * platforms and not all of them provide clocks specs in their + * device tree, but failure to enable a specified clock is + * considered fatal + */ + clk = devm_clk_get(&pdev->dev, "per"); + if (!IS_ERR(clk)) { + ret = clk_prepare_enable(clk); + if (ret) { + dev_err(dev, "Could not enable peripheral clock\n"); + return ret; + } + /* + * TODO where to store the 'clk' reference? there appears + * to be no remove() routine which undoes what probe() does + */ + } + node = pdev->dev.of_node; ret = fsl_add_bridge(pdev, fsl_pci_primary == node);
make the Freescale PCI driver get, prepare and enable the PCI clock during probe(); the clock gets put upon device close by the devm approach clock lookup is non-fatal as not all platforms may provide clock specs in their device tree, but failure to enable specified clocks are fatal the driver appears to not have a remove() routine, so no reference to the clock is kept during use, and the clock isn't released (the devm approach will put the clock, but it won't get disabled or unprepared) Signed-off-by: Gerhard Sittig <gsi@denx.de> --- arch/powerpc/sysdev/fsl_pci.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)