Re: [PATCH 1/5] mmc: sdhci-pxav3: fix optional clock name

[Sebastian Hesselbarth]

On 16.10.2015 13:40, Jisheng Zhang wrote:
> On Thu, 15 Oct 2015 23:41:22 +0200
> Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx> wrote:
> > On 12.10.2015 07:46, Jisheng Zhang wrote:
> > > Commit 8afdc9cca27f ("mmc: sdhci-pxav3: Get optional core clock") adds
> > > additional optional clock support, but the clock names  isn't correct.
> > > The current "io" clock is really the PXAv3 SDHCI IP's "core" clock
> > > which is manadatory. The current "core" clock is really the IP's "axi"
> > > clock which is optional.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxxx>
> > > ---
[...]
> > Please split the DT changes from the driver changes.
>
> Such split will break berlin SDHC functionality, I'm not sure whether this is
> acceptable.

I am not going to funnel any driver stuff through berlin-tree anymore
if it isn't really neccessary.

[...]
> > >    - clocks: Array of clocks required for SDHCI; requires at least one for
> > > -    I/O clock.
> > > +    core clock.
> > >    - clock-names: Array of names corresponding to clocks property; shall be
> > > -    "io" for I/O clock and "core" for optional core clock.
> > > +    "core" for core clock and "axi" for optional axi clock.
> >
> > s/axi/bus/ ?
>
> HW call this clk as axi, "bus" seems more generic, right?

Yes, please pick the generic name. Given the age of that IP,
I guess it will run on AHB instead of AXI on PXA.

[...]
> > > -	pxa->clk_io = devm_clk_get(dev, "io");
> > > -	if (IS_ERR(pxa->clk_io))
> > > -		pxa->clk_io = devm_clk_get(dev, NULL);
> > > -	if (IS_ERR(pxa->clk_io)) {
> > > -		dev_err(dev, "failed to get io clock\n");
> > > -		ret = PTR_ERR(pxa->clk_io);
> > > +	pxa->clk_core = devm_clk_get(dev, "core");
> >
> > To maintain backward compatibility, we should still
>
> I just grep the kernel source, found that only mrvl berlin SoCs have two clks,
> other SoCs just have one clk and don't provide clock name. So the question
> here is whether we can break linux backward compatibility for mrvl berlin
> SoCs. I think we could for the following reason:

That is the point: do not break it on purpose.

Try to separate DT and driver changes without breaking the tree
in between.

Sebastian

> 1. rare boards outside mrvl can boot user self-built kernel, most boards should
> be protected by trust-chainloader mechanism.
>
> 2. mrvl berlin SoCs upgrade linux kernel and dtb at the same time
>
> what do you think?
>
> Is it better to refine patch 2, 3, 4, 5 to make use current "io","core" binding
> to clean up the clk's CLK_IGNORE_UNUSED flags? In fact, they doesn't depend
> on patch1. Could you please kindly give advice?
>
> Thanks in advance,
> Jisheng
>
>
>
> > devm_clk_get(dev, "io") here - or even better, first
> > probe for "io" to catch old binding semantics including
> > old "core"/"bus" misnaming.
> >
> > If we detected old semantics, print a warning that
> > firmware should be updated.
> >
> > Sebastian
> >
> > > +	if (IS_ERR(pxa->clk_core))
> > > +		pxa->clk_core = devm_clk_get(dev, NULL);
> > > +	if (IS_ERR(pxa->clk_core)) {
> > > +		dev_err(dev, "failed to get core clock\n");
> > > +		ret = PTR_ERR(pxa->clk_core);
> > >    		goto err_clk_get;
> > >    	}
> > > -	pltfm_host->clk = pxa->clk_io;
> > > -	clk_prepare_enable(pxa->clk_io);
> > > +	pltfm_host->clk = pxa->clk_core;
> > > +	clk_prepare_enable(pxa->clk_core);
> > >
> > > -	pxa->clk_core = devm_clk_get(dev, "core");
> > > -	if (!IS_ERR(pxa->clk_core))
> > > -		clk_prepare_enable(pxa->clk_core);
> > > +	pxa->clk_axi = devm_clk_get(dev, "axi");
> > > +	if (!IS_ERR(pxa->clk_axi))
> > > +		clk_prepare_enable(pxa->clk_axi);
> > >
> > >    	/* enable 1/8V DDR capable */
> > >    	host->mmc->caps |= MMC_CAP_1_8V_DDR;
> > > @@ -475,8 +475,8 @@ err_add_host:
> > >    err_of_parse:
> > >    err_cd_req:
> > >    err_mbus_win:
> > > -	clk_disable_unprepare(pxa->clk_io);
> > >    	clk_disable_unprepare(pxa->clk_core);
> > > +	clk_disable_unprepare(pxa->clk_axi);
> > >    err_clk_get:
> > >    	sdhci_pltfm_free(pdev);
> > >    	return ret;
> > > @@ -494,8 +494,8 @@ static int sdhci_pxav3_remove(struct platform_device *pdev)
> > >
> > >    	sdhci_remove_host(host, 1);
> > >
> > > -	clk_disable_unprepare(pxa->clk_io);
> > >    	clk_disable_unprepare(pxa->clk_core);
> > > +	clk_disable_unprepare(pxa->clk_axi);
> > >
> > >    	sdhci_pltfm_free(pdev);
> > >
> > > @@ -542,9 +542,9 @@ static int sdhci_pxav3_runtime_suspend(struct device *dev)
> > >    	if (ret)
> > >    		return ret;
> > >
> > > -	clk_disable_unprepare(pxa->clk_io);
> > > -	if (!IS_ERR(pxa->clk_core))
> > > -		clk_disable_unprepare(pxa->clk_core);
> > > +	clk_disable_unprepare(pxa->clk_core);
> > > +	if (!IS_ERR(pxa->clk_axi))
> > > +		clk_disable_unprepare(pxa->clk_axi);
> > >
> > >    	return 0;
> > >    }
> > > @@ -555,9 +555,9 @@ static int sdhci_pxav3_runtime_resume(struct device *dev)
> > >    	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > >    	struct sdhci_pxa *pxa = pltfm_host->priv;
> > >
> > > -	clk_prepare_enable(pxa->clk_io);
> > > -	if (!IS_ERR(pxa->clk_core))
> > > -		clk_prepare_enable(pxa->clk_core);
> > > +	clk_prepare_enable(pxa->clk_core);
> > > +	if (!IS_ERR(pxa->clk_axi))
> > > +		clk_prepare_enable(pxa->clk_axi);
> > >
> > >    	return sdhci_runtime_resume_host(host);
> > >    }

--
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/