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

From: Giuseppe CAVALLARO
Date: Tue Sep 23 2014 - 02:10:39 EST


On 9/23/2014 3:16 AM, Kweh, Hock Leong wrote:
-----Original Message-----
From: David Miller [mailto:davem@xxxxxxxxxxxxx]
Sent: Tuesday, September 23, 2014 2:19 AM
From: Kweh Hock Leong <hock.leong.kweh@xxxxxxxxx>
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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/