| Message ID | 1411043650-31712-1-git-send-email-hock.leong.kweh@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 9/18/2014 2:34 PM, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > When the CONFIG_HAVE_CLK is selected for the system, the stmmac_pci_probe > will fail with dmesg: > [ 2.167225] stmmaceth 0000:00:14.6: enabling device (0000 -> 0002) > [ 2.178267] stmmaceth 0000:00:14.6: enabling bus mastering > [ 2.178436] stmmaceth 0000:00:14.6: irq 24 for MSI/MSI-X > [ 2.178703] stmmaceth 0000:00:14.6: stmmac_dvr_probe: warning: cannot > get CSR clock > [ 2.186503] stmmac_pci_probe: main driver probe failed > [ 2.194003] stmmaceth 0000:00:14.6: disabling bus mastering > [ 2.196473] stmmaceth: probe of 0000:00:14.6 failed with error -2 > > This patch fix the issue by breaking the dependency to devm_clk_get() > as the CSR clock can be obtained at priv->plat->clk_csr from pci driver. > > Reported-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 08addd6..ea3859a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -2714,10 +2714,15 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device, > > priv->stmmac_clk = devm_clk_get(priv->device, STMMAC_RESOURCE_NAME); > if (IS_ERR(priv->stmmac_clk)) { > - dev_warn(priv->device, "%s: warning: cannot get CSR clock\n", Hmm I am not sure this is the right fix. The driver has to fail if the main clock is not found. Indeed dev_warn has to be changed in dev_err. Take a look at Documentation/networking/stmmac.txt but I will post some patch to improve the documentation adding further detail for clocks too. The the logic behind the code is that the CSR clock will be set at runtime if in case of priv->plat->clk_csr ==0 or it will be forced to a fixed value if passed from the platform instead of. IIRC This was required on some platforms time ago. For sure the driver is designed to fail in case of no main clock is found. Peppe > - __func__); > - ret = PTR_ERR(priv->stmmac_clk); > - goto error_clk_get; > + if (!priv->plat->clk_csr) { > + dev_warn(priv->device, > + "%s: warning: cannot get CSR clock\n", > + __func__); > + ret = PTR_ERR(priv->stmmac_clk); > + goto error_clk_get; > + } else { > + priv->stmmac_clk = NULL; > + } > } > clk_prepare_enable(priv->stmmac_clk); > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] > Sent: Thursday, September 18, 2014 10:50 PM > On 9/18/2014 2:34 PM, Kweh Hock Leong wrote: > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Hmm I am not sure this is the right fix. The driver has to fail if the main clock is > not found. Indeed dev_warn has to be changed in dev_err. > > Take a look at Documentation/networking/stmmac.txt but I will post some > patch to improve the documentation adding further detail for clocks too. > > The the logic behind the code is that the CSR clock will be set at runtime if in > case of priv->plat->clk_csr ==0 or it will be forced to a fixed value if passed > from the platform instead of. > IIRC This was required on some platforms time ago. > For sure the driver is designed to fail in case of no main clock is found. > > Peppe Hi Peppe, I understand your point from the code below (at file stmmac_main.c line 2784): /* If a specific clk_csr value is passed from the platform * this means that the CSR Clock Range selection cannot be * changed at run-time and it is fixed. Viceversa the driver'll try to * set the MDC clock dynamically according to the csr actual * clock input. */ if (!priv->plat->clk_csr) stmmac_clk_csr_set(priv); else priv->clk_csr = priv->plat->clk_csr; I did search through the whole stmmac_main.c file and found that only stmmac_clk_csr_set() function is leveraging the priv->stmmac_clk params for it calculation. By the logic point of view, I do not need priv->stmmac_clk when I got priv->plat->clk_csr. With this thinking, I propose this fix as when the probe get priv->plat->clk_csr, it shouldn't fail if priv->stmmac_clk has the error value. Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kweh Hock Leong <hock.leong.kweh@intel.com> Date: Thu, 18 Sep 2014 20:34:10 +0800 > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > When the CONFIG_HAVE_CLK is selected for the system, the stmmac_pci_probe > will fail with dmesg: > [ 2.167225] stmmaceth 0000:00:14.6: enabling device (0000 -> 0002) > [ 2.178267] stmmaceth 0000:00:14.6: enabling bus mastering > [ 2.178436] stmmaceth 0000:00:14.6: irq 24 for MSI/MSI-X > [ 2.178703] stmmaceth 0000:00:14.6: stmmac_dvr_probe: warning: cannot > get CSR clock > [ 2.186503] stmmac_pci_probe: main driver probe failed > [ 2.194003] stmmaceth 0000:00:14.6: disabling bus mastering > [ 2.196473] stmmaceth: probe of 0000:00:14.6 failed with error -2 > > This patch fix the issue by breaking the dependency to devm_clk_get() > as the CSR clock can be obtained at priv->plat->clk_csr from pci driver. > > Reported-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> Giuseppe, Kweh, where are we with this patch? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Tuesday, September 23, 2014 2:19 AM > From: Kweh Hock Leong <hock.leong.kweh@intel.com> > Date: Thu, 18 Sep 2014 20:34:10 +0800 > > Giuseppe, Kweh, where are we with this patch? We are discussing whether this is the correct fix to the issue. Below is the discussion log: > > Hmm I am not sure this is the right fix. The driver has to fail if the > > main clock is not found. Indeed dev_warn has to be changed in dev_err. > > > > Take a look at Documentation/networking/stmmac.txt but I will post > > some patch to improve the documentation adding further detail for clocks too. > > > > The the logic behind the code is that the CSR clock will be set at > > runtime if in case of priv->plat->clk_csr ==0 or it will be forced to > > a fixed value if passed from the platform instead of. > > IIRC This was required on some platforms time ago. > > For sure the driver is designed to fail in case of no main clock is found. > > > > Peppe > > Hi Peppe, > > I understand your point from the code below (at file stmmac_main.c line 2784): > > /* If a specific clk_csr value is passed from the platform > * this means that the CSR Clock Range selection cannot be > * changed at run-time and it is fixed. Viceversa the driver'll try to > * set the MDC clock dynamically according to the csr actual > * clock input. > */ > if (!priv->plat->clk_csr) > stmmac_clk_csr_set(priv); > else > priv->clk_csr = priv->plat->clk_csr; > > > I did search through the whole stmmac_main.c file and found that only stmmac_clk_csr_set() > function is leveraging the priv->stmmac_clk params for it calculation. By the logic point of view, > I do not need priv->stmmac_clk when I got priv->plat->clk_csr. With this thinking, I propose this > fix as when the probe get priv->plat->clk_csr, it shouldn't fail if priv->stmmac_clk has the error value. Hi Peppe, Are you trying to tell that if there is a fix CSR clock value, but driver cannot obtain the clk from devm_clk_get() (even it is not in use), the driver should fail the probe? You have a case with this condition: HW allow SW to discover the STMMAC controller but HW/SW configures not to supply the CLOCK to STMMAC controller? My understanding to these priv->plat->clk_csr and priv->stmmac_clk is that 1st one is fixed and 2nd one is dynamic. When fixed value occurs, dynamic one would not be use. Do I understand this correctly? Thanks. Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/23/2014 3:16 AM, Kweh, Hock Leong wrote: >> -----Original Message----- >> From: David Miller [mailto:davem@davemloft.net] >> Sent: Tuesday, September 23, 2014 2:19 AM >> From: Kweh Hock Leong <hock.leong.kweh@intel.com> >> Date: Thu, 18 Sep 2014 20:34:10 +0800 >> >> Giuseppe, Kweh, where are we with this patch? > > We are discussing whether this is the correct fix to the issue. Below is the discussion log: > >>> Hmm I am not sure this is the right fix. The driver has to fail if the >>> main clock is not found. Indeed dev_warn has to be changed in dev_err. >>> >>> Take a look at Documentation/networking/stmmac.txt but I will post >>> some patch to improve the documentation adding further detail for clocks too. >>> >>> The the logic behind the code is that the CSR clock will be set at >>> runtime if in case of priv->plat->clk_csr ==0 or it will be forced to >>> a fixed value if passed from the platform instead of. >>> IIRC This was required on some platforms time ago. >>> For sure the driver is designed to fail in case of no main clock is found. >>> >>> Peppe >> >> Hi Peppe, >> >> I understand your point from the code below (at file stmmac_main.c line 2784): >> >> /* If a specific clk_csr value is passed from the platform >> * this means that the CSR Clock Range selection cannot be >> * changed at run-time and it is fixed. Viceversa the driver'll try to >> * set the MDC clock dynamically according to the csr actual >> * clock input. >> */ >> if (!priv->plat->clk_csr) >> stmmac_clk_csr_set(priv); >> else >> priv->clk_csr = priv->plat->clk_csr; >> >> >> I did search through the whole stmmac_main.c file and found that only stmmac_clk_csr_set() >> function is leveraging the priv->stmmac_clk params for it calculation. By the logic point of view, >> I do not need priv->stmmac_clk when I got priv->plat->clk_csr. With this thinking, I propose this >> fix as when the probe get priv->plat->clk_csr, it shouldn't fail if priv->stmmac_clk has the error value. > > Hi Peppe, > > Are you trying to tell that if there is a fix CSR clock value, but driver cannot obtain the clk from devm_clk_get() > (even it is not in use), the driver should fail the probe? > > You have a case with this condition: > HW allow SW to discover the STMMAC controller but HW/SW configures not to supply the CLOCK to STMMAC controller? > > My understanding to these priv->plat->clk_csr and priv->stmmac_clk is that 1st one is fixed and 2nd one is dynamic. > When fixed value occurs, dynamic one would not be use. Do I understand this correctly? the logic is: the priv->stmmac_clk must be always provided from the platform then we have two cases: 1) if priv->plat->clk_csr is also passed then it will be adopt in the mdio functions to program the Reg4[5:2] This was required in the past IIRC on SPEAr platforms. 2) if priv->plat->clk_csr is not passed from the platform then the priv->clk_csr will be set according to the priv->stmmac_clk and always used in the mdio part. So IIUC now you are asking for not passing the priv->stmmac_clk and warning this event w/o failing. Why you cannot pass this clock? peppe > > Thanks. > > > Regards, > Wilson > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] > Sent: Tuesday, September 23, 2014 2:10 PM > > the logic is: the priv->stmmac_clk must be always provided from the platform > then we have two cases: > > 1) if priv->plat->clk_csr is also passed then it will be adopt in the > mdio functions to program the Reg4[5:2] > This was required in the past IIRC on SPEAr platforms. > > 2) if priv->plat->clk_csr is not passed from the platform then the > priv->clk_csr will be set according to the priv->stmmac_clk > and always used in the mdio part. > > So IIUC now you are asking for not passing the priv->stmmac_clk and warning > this event w/o failing. Why you cannot pass this clock? > > peppe Hi peppe, Appreciate for the explanation. Just to clarify that I am not asking not to pass in the priv->stmmac_clk. In fact, the fix will fail at case 2 if driver cannot obtain the priv->stmmac_clk, but just not the case 1. For case 1, seem like it does not require the stmmac_clk then I think it should be OK not to fail it when driver did not get stmmac_clk but have the clk_csr set. Anyway, I can change the fix by adding the clock registration APIs being call at the stmmac_pci.c probe there before calling stmmac_dvr_probe. By doing this, it created a dependency to the pci driver that must have CONFIG_HAVE_CLK to be turned on. Besides, I would need you guys to provide me information on other platforms about what is the best value to set? Can I just set to zero since the stmmac_pci driver is always using the priv->plat->clk_csr? Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/23/2014 9:03 AM, Kweh, Hock Leong wrote: >> -----Original Message----- >> From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] >> Sent: Tuesday, September 23, 2014 2:10 PM >> >> the logic is: the priv->stmmac_clk must be always provided from the platform >> then we have two cases: >> >> 1) if priv->plat->clk_csr is also passed then it will be adopt in the >> mdio functions to program the Reg4[5:2] >> This was required in the past IIRC on SPEAr platforms. >> >> 2) if priv->plat->clk_csr is not passed from the platform then the >> priv->clk_csr will be set according to the priv->stmmac_clk >> and always used in the mdio part. >> >> So IIUC now you are asking for not passing the priv->stmmac_clk and warning >> this event w/o failing. Why you cannot pass this clock? >> >> peppe > > > Hi peppe, > > Appreciate for the explanation. Just to clarify that I am not asking not to pass in the priv->stmmac_clk. > In fact, the fix will fail at case 2 if driver cannot obtain the priv->stmmac_clk, but just not the case 1. > For case 1, seem like it does not require the stmmac_clk then I think it should be OK not to fail it when > driver did not get stmmac_clk but have the clk_csr set. ok we can do that but this clock is also managed when the iface is down. Maybe it could be convenient to manage it for power consumption. What do you think? > Anyway, I can change the fix by adding the clock registration APIs being call at the stmmac_pci.c probe there before > calling stmmac_dvr_probe. By doing this, it created a dependency to the pci driver that must have CONFIG_HAVE_CLK > to be turned on. Besides, I would need you guys to provide me information on other platforms about what is the best > value to set? Can I just set to zero since the stmmac_pci driver is always using the priv->plat->clk_csr? > > > Regards, > Wilson > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] > Sent: Wednesday, September 24, 2014 2:10 PM > > > > Hi peppe, > > > > Appreciate for the explanation. Just to clarify that I am not asking not to > pass in the priv->stmmac_clk. > > In fact, the fix will fail at case 2 if driver cannot obtain the priv->stmmac_clk, > but just not the case 1. > > For case 1, seem like it does not require the stmmac_clk then I think > > it should be OK not to fail it when driver did not get stmmac_clk but have > the clk_csr set. > > ok we can do that but this clock is also managed when the iface is down. > Maybe it could be convenient to manage it for power consumption. > What do you think? Hi peppe, I don't really get what you mean here. Are you telling that you are OK with the fix? Or you are referring to the bottom idea which introduce clock registration APIs to stmmac_pci driver? Regarding the power management, isn't this taking care by the PCI framework itself for the PCI devices / PCI cards? Sorry, may be would need you to provide a big picture to this. Thanks. :) > > > Anyway, I can change the fix by adding the clock registration APIs > > being call at the stmmac_pci.c probe there before calling > > stmmac_dvr_probe. By doing this, it created a dependency to the pci > > driver that must have CONFIG_HAVE_CLK to be turned on. Besides, I > would need you guys to provide me information on other platforms about > what is the best value to set? Can I just set to zero since the stmmac_pci > driver is always using the priv->plat->clk_csr? > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/24/2014 12:48 PM, Kweh, Hock Leong wrote: >> -----Original Message----- >> From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] >> Sent: Wednesday, September 24, 2014 2:10 PM >>> >>> Hi peppe, >>> >>> Appreciate for the explanation. Just to clarify that I am not asking not to >> pass in the priv->stmmac_clk. >>> In fact, the fix will fail at case 2 if driver cannot obtain the priv->stmmac_clk, >> but just not the case 1. >>> For case 1, seem like it does not require the stmmac_clk then I think >>> it should be OK not to fail it when driver did not get stmmac_clk but have >> the clk_csr set. >> >> ok we can do that but this clock is also managed when the iface is down. >> Maybe it could be convenient to manage it for power consumption. >> What do you think? > > Hi peppe, > > I don't really get what you mean here. Are you telling that you are OK with the fix? > Or you are referring to the bottom idea which introduce clock registration APIs to > stmmac_pci driver? I just meant that if no clock is passed then the probe doesn't fail and we can keep a dev_warn. Pls surround the code with a comment and repost the patch. Let me know and sorry for the delay on replying. peppe > > Regarding the power management, isn't this taking care by the PCI framework itself > for the PCI devices / PCI cards? > > Sorry, may be would need you to provide a big picture to this. Thanks. :) > >> >>> Anyway, I can change the fix by adding the clock registration APIs >>> being call at the stmmac_pci.c probe there before calling >>> stmmac_dvr_probe. By doing this, it created a dependency to the pci >>> driver that must have CONFIG_HAVE_CLK to be turned on. Besides, I >> would need you guys to provide me information on other platforms about >> what is the best value to set? Can I just set to zero since the stmmac_pci >> driver is always using the priv->plat->clk_csr? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] > Sent: Friday, September 26, 2014 8:06 PM > > > From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com] > > > Sent: Wednesday, September 24, 2014 2:10 PM > > I just meant that if no clock is passed then the probe doesn't fail and we can > keep a dev_warn. Pls surround the code with a comment and repost the > patch. Let me know and sorry for the delay on replying. > > peppe > Now I got you. Okay, will do that and send out the V2 patch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 08addd6..ea3859a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2714,10 +2714,15 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device, priv->stmmac_clk = devm_clk_get(priv->device, STMMAC_RESOURCE_NAME); if (IS_ERR(priv->stmmac_clk)) { - dev_warn(priv->device, "%s: warning: cannot get CSR clock\n", - __func__); - ret = PTR_ERR(priv->stmmac_clk); - goto error_clk_get; + if (!priv->plat->clk_csr) { + dev_warn(priv->device, + "%s: warning: cannot get CSR clock\n", + __func__); + ret = PTR_ERR(priv->stmmac_clk); + goto error_clk_get; + } else { + priv->stmmac_clk = NULL; + } } clk_prepare_enable(priv->stmmac_clk);