Re: [PATCH] net: stmmac: fix stmmac_pci_probe failed when CONFIG_HAVE_CLK is selected

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

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