Re: [PATCH 2/8] clk: tegra: clock changes for emc scaling support on Tegra210

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5248 bytes --]

On Mon, Mar 25, 2019 at 03:45:17PM +0800, Joseph Lo wrote:
> 1) Introduce low jitter paths for pllp and pll_mb used by the EMC driver.
> 2) 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.
> 3) Export functions to allow accessing the CAR register required for EMC
>    clock scaling. These functions will be used to access the CAR register
>    as part of the scaling sequence.

The fact that you can enumerate 3 logical changes made by this commit
indicates that it should be split up into smaller patches.

> Based on the work of Peter De Schrijver <pdeschrijver@nvidia.com>.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
>  drivers/clk/tegra/clk-tegra210.c         | 112 +++++++++++++++++++----
>  include/dt-bindings/clock/tegra210-car.h |   4 +-
>  include/linux/clk/tegra.h                |   5 +
>  3 files changed, 103 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
> index 7545af763d7a..e17b5279ea69 100644
> --- a/drivers/clk/tegra/clk-tegra210.c
> +++ b/drivers/clk/tegra/clk-tegra210.c
> @@ -47,6 +47,7 @@
>  #define CLK_SOURCE_LA 0x1f8
>  #define CLK_SOURCE_SDMMC2 0x154
>  #define CLK_SOURCE_SDMMC4 0x164
> +#define CLK_SOURCE_EMC_DLL 0x664
>  
>  #define PLLC_BASE 0x80
>  #define PLLC_OUT 0x84
> @@ -234,6 +235,10 @@
>  #define RST_DFLL_DVCO 0x2f4
>  #define DVFS_DFLL_RESET_SHIFT 0
>  
> +#define CLK_RST_CONTROLLER_CLK_OUT_ENB_X_SET	0x284
> +#define CLK_RST_CONTROLLER_CLK_OUT_ENB_X_CLR	0x288
> +#define CLK_OUT_ENB_X_CLK_ENB_EMC_DLL		BIT(14)
> +
>  #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>  #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>  
> @@ -319,12 +324,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)
> @@ -2310,7 +2309,7 @@ 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_emc] = { .dt_id = TEGRA210_CLK_EMC, .present = false },
>  	[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 },
> @@ -2921,6 +2920,82 @@ static int tegra210_init_pllu(void)
>  	return 0;
>  }
>  
> +void tegra210_clk_emc_dll_enable(bool flag)
> +{
> +	unsigned long flags = 0;
> +	u32 offset = flag ? CLK_RST_CONTROLLER_CLK_OUT_ENB_X_SET :
> +		     CLK_RST_CONTROLLER_CLK_OUT_ENB_X_CLR;
> +
> +	spin_lock_irqsave(&emc_lock, flags);
> +
> +	writel_relaxed(CLK_OUT_ENB_X_CLK_ENB_EMC_DLL, clk_base + offset);
> +	readl(clk_base + offset);
> +
> +	spin_unlock_irqrestore(&emc_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(tegra210_clk_emc_dll_enable);
> +
> +void tegra210_clk_emc_dll_update_setting(u32 emc_dll_src_value)

Do we really want to pass the whole register value through this
function? The register has three fields, so perhaps it's safer to pass
the fields individually? Or perhaps we only need to modify a subset of
the fields and can reduce the number of parameters we pass? Letting a
different driver pass any arbitrary value here takes away any means of
checking for validity.

> +{
> +	unsigned long flags = 0;
> +
> +	spin_lock_irqsave(&emc_lock, flags);
> +
> +	writel_relaxed(emc_dll_src_value, clk_base + CLK_SOURCE_EMC_DLL);
> +	readl(clk_base + CLK_SOURCE_EMC_DLL);

Could we not just use a writel() here and do away with the flushing
readl()?

Also, it doesn't look like that spinlock actually protects anything.
You're just writing a value. If anyone else is holding that lock they
will either overwrite our value after we release the lock, or we
overwrite their value when they release the lock.

> +
> +	spin_unlock_irqrestore(&emc_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(tegra210_clk_emc_dll_update_setting);
> +
> +void tegra210_clk_emc_update_setting(u32 emc_src_value)
> +{
> +	unsigned long flags = 0;
> +
> +	spin_lock_irqsave(&emc_lock, flags);
> +
> +	writel_relaxed(emc_src_value, clk_base + CLK_SOURCE_EMC);
> +	readl(clk_base + CLK_SOURCE_EMC);
> +
> +	spin_unlock_irqrestore(&emc_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(tegra210_clk_emc_update_setting);

Same comments as above.

> +
> +u32 tegra210_clk_emc_get_setting(void)
> +{
> +	unsigned long flags = 0;
> +	u32 val;
> +
> +	spin_lock_irqsave(&emc_lock, flags);
> +
> +	val = readl_relaxed(clk_base + CLK_SOURCE_EMC);
> +
> +	spin_unlock_irqrestore(&emc_lock, flags);

Similar to the above, the spinlock doesn't protect anything here.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel