Re: [PATCH V4 7/8] clk: tegra: Remove the old emc_mux clock for Tegra210

[Joseph Lo <josephl@xxxxxxxxxx>]

On 5/29/19 8:49 PM, Dmitry Osipenko wrote:
> 29.05.2019 11:21, Joseph Lo пишет:
> > Remove the old emc_mux clock and don't use the common EMC clock
> > definition. This will be replaced by a new clock defined in the
> > EMC driver.
> >
> > Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
> > ---
> > v4:
> > - make sure the behavior is compatible with case if the kernel still
> >    uses the older DTB which doesn't have EMC node. And the MC and EMC
> >    clock can still be registered successuflly.
> > v3:
> > - split to 3 patches from the previous version
> > ---
> >   drivers/clk/tegra/clk-tegra210.c | 42 ++++++++++++++++++++------------
> >   1 file changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
> > index 1d52354820ca..8b209e8b5eaf 100644
> > --- a/drivers/clk/tegra/clk-tegra210.c
> > +++ b/drivers/clk/tegra/clk-tegra210.c
> > @@ -28,6 +28,7 @@
> >   #include <dt-bindings/reset/tegra210-car.h>
> >   #include <linux/iopoll.h>
> >   #include <linux/sizes.h>
> > +#include <soc/tegra/emc.h>
> >   #include <soc/tegra/pmc.h>
> >   
> >   #include "clk.h"
> > @@ -324,12 +325,6 @@ static unsigned long tegra210_input_freq[] = {
> >   	[8] = 12000000,
> >   };
> >   
> > -static const char *mux_pllmcp_clkm[] = {
> > -	"pll_m", "pll_c", "pll_p", "clk_m", "pll_m_ud", "pll_mb", "pll_mb",
> > -	"pll_p",
> > -};
> > -#define mux_pllmcp_clkm_idx NULL
> > -
> >   #define PLL_ENABLE			(1 << 30)
> >   
> >   #define PLLCX_MISC1_IDDQ		(1 << 27)
> > @@ -2346,7 +2341,6 @@ static struct tegra_clk tegra210_clks[tegra_clk_max] __initdata = {
> >   	[tegra_clk_i2c2] = { .dt_id = TEGRA210_CLK_I2C2, .present = true },
> >   	[tegra_clk_uartc_8] = { .dt_id = TEGRA210_CLK_UARTC, .present = true },
> >   	[tegra_clk_mipi_cal] = { .dt_id = TEGRA210_CLK_MIPI_CAL, .present = true },
> > -	[tegra_clk_emc] = { .dt_id = TEGRA210_CLK_EMC, .present = true },
> >   	[tegra_clk_usb2] = { .dt_id = TEGRA210_CLK_USB2, .present = true },
> >   	[tegra_clk_bsev] = { .dt_id = TEGRA210_CLK_BSEV, .present = true },
> >   	[tegra_clk_uartd_8] = { .dt_id = TEGRA210_CLK_UARTD, .present = true },
> > @@ -2957,6 +2951,27 @@ static int tegra210_init_pllu(void)
> >   	return 0;
> >   }
> >   
> > +static const struct clk_div_table mc_div_table_tegra210[] = {
> > +	{ .val = 0, .div = 2 },
> > +	{ .val = 1, .div = 4 },
> > +	{ .val = 2, .div = 1 },
> > +	{ .val = 3, .div = 2 },
> > +	{ .val = 0, .div = 0 },
> > +};
> > +
> > +static void tegra210_clk_register_mc(const char *name,
> > +				     const char *parent_name)
> > +{
> > +	struct clk *clk;
> > +
> > +	clk = clk_register_divider_table(NULL, name, parent_name,
> > +					 CLK_IS_CRITICAL,
> > +					 clk_base + CLK_SOURCE_EMC,
> > +					 15, 2, CLK_DIVIDER_READ_ONLY,
> > +					 mc_div_table_tegra210, &emc_lock);
>
> This doesn't look right, you're mixing up the MC divider with the EMC
> divider here. The MC clock is always sourced from EMC and there is only
> one bit for the MC divider, the bit 16 MC_EMC_SAME_FREQ.
>
> When EMC clock is divided down by 2 (bit 15 EMC_CLK_DIV2_EN), then the
> clk-framework will take care of it by calculating the MC rate based on
> the actual parent EMC rate.
>
> > +	clks[TEGRA210_CLK_MC] = clk;
> > +}
> > +
> >   static const char * const sor1_out_parents[] = {
> >   	/*
> >   	 * Bit 0 of the mux selects sor1_pad_clkout, irrespective of bit 1, so
> > @@ -3040,15 +3055,12 @@ static __init void tegra210_periph_clk_init(void __iomem *clk_base,
> >   			CLK_SOURCE_LA, 0);
> >   	clks[TEGRA210_CLK_LA] = clk;
> >   
> > -	/* emc mux */
> > -	clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm,
> > -			       ARRAY_SIZE(mux_pllmcp_clkm), 0,
> > -			       clk_base + CLK_SOURCE_EMC,
> > -			       29, 3, 0, &emc_lock);
> > +	/* emc */
> > +	clk = tegra210_clk_register_emc();
> > +	clks[TEGRA210_CLK_EMC] = clk;
> >   
> > -	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
> > -				    &emc_lock);
> > -	clks[TEGRA210_CLK_MC] = clk;
> > +	/* mc */
> > +	tegra210_clk_register_mc("mc", "emc");
> >   
> >   	/* cml0 */
> >   	clk = clk_register_gate(NULL, "cml0", "pll_e", 0, clk_base + PLLE_AUX,
>
> You should leave the common tegra_clk_register_mc() usage as-is and only
> s/emc_mux/emc/ in the argument.

Ah, yes, that was wrong mixing up with two irrelevant bits. Fixed in my 
local patch.

Thanks,
Joseph