Re: [PATCH 2/2] clk: axi-clkgen: Add support for FPGA info

From: Moritz Fischer <mdf@kernel.org>
To: Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc: linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	mturquette@baylibre.com, sboyd@kernel.org, lars@metafoo.de,
	linux-fpga@vger.kernel.org, mdf@kernel.org,
	ardeleanalex@gmail.com,
	Mircea Caprioru <mircea.caprioru@analog.com>
Subject: Re: [PATCH 2/2] clk: axi-clkgen: Add support for FPGA info
Date: Wed, 10 Feb 2021 20:05:56 -0800	[thread overview]
Message-ID: <YCStJFVsYIB2ogKU@epycbox.lan> (raw)
In-Reply-To: <20210210101535.47979-2-alexandru.ardelean@analog.com>

On Wed, Feb 10, 2021 at 12:15:35PM +0200, Alexandru Ardelean wrote:
> From: Mircea Caprioru <mircea.caprioru@analog.com>
> 
> This patch adds support for vco maximum and minimum ranges in accordance
VCO
> with fpga speed grade, voltage, device package, technology and family. This
FPGA
> new information is extracted from two new registers implemented in the ip
> core: ADI_REG_FPGA_INFO and ADI_REG_FPGA_VOLTAGE, which are stored in the
> 'include/linux/fpga/adi-axi-common.h' file as they are common to all ADI
> FPGA cores.
Either all caps FPGA or all non-caps.
> 
> Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/clk/clk-axi-clkgen.c | 52 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-axi-clkgen.c b/drivers/clk/clk-axi-clkgen.c
> index ac6ff736ac8f..e4d6c87f8a07 100644
> --- a/drivers/clk/clk-axi-clkgen.c
> +++ b/drivers/clk/clk-axi-clkgen.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
> +#include <linux/fpga/adi-axi-common.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> @@ -240,6 +241,50 @@ static void axi_clkgen_read(struct axi_clkgen *axi_clkgen,
>  	*val = readl(axi_clkgen->base + reg);
>  }
>  
> +static void axi_clkgen_setup_ranges(struct axi_clkgen *axi_clkgen)
> +{
> +	struct axi_clkgen_limits *limits = &axi_clkgen->limits;
> +	unsigned int reg_value;
> +	unsigned int tech, family, speed_grade, voltage;
Reverse xmas-tree please.

xxxxxx
xxx
x
> +
> +	axi_clkgen_read(axi_clkgen, ADI_AXI_REG_FPGA_INFO, &reg_value);
> +	tech = ADI_AXI_INFO_FPGA_TECH(reg_value);
> +	family = ADI_AXI_INFO_FPGA_FAMILY(reg_value);
> +	speed_grade = ADI_AXI_INFO_FPGA_SPEED_GRADE(reg_value);
> +
> +	axi_clkgen_read(axi_clkgen, ADI_AXI_REG_FPGA_VOLTAGE, &reg_value);
> +	voltage = ADI_AXI_INFO_FPGA_VOLTAGE(reg_value);
> +
> +	switch (speed_grade) {
> +	case ADI_AXI_FPGA_SPEED_GRADE_XILINX_1 ... ADI_AXI_FPGA_SPEED_GRADE_XILINX_1LV:
> +		limits->fvco_max = 1200000;
> +		limits->fpfd_max = 450000;
> +		break;
> +	case ADI_AXI_FPGA_SPEED_GRADE_XILINX_2 ... ADI_AXI_FPGA_SPEED_GRADE_XILINX_2LV:
> +		limits->fvco_max = 1440000;
> +		limits->fpfd_max = 500000;
> +		if ((family == ADI_AXI_FPGA_FAMILY_XILINX_KINTEX) |
> +		    (family == ADI_AXI_FPGA_FAMILY_XILINX_ARTIX)) {
> +			if (voltage < 950) {
> +				limits->fvco_max = 1200000;
> +				limits->fpfd_max = 450000;
> +			}
> +		}
> +		break;
> +	case ADI_AXI_FPGA_SPEED_GRADE_XILINX_3:
> +		limits->fvco_max = 1600000;
> +		limits->fpfd_max = 550000;
> +		break;
> +	default:
No warning? Does PCORE_VER_MAJOR(ver) > 0x04 not imply this to be known
or valid?
> +		break;
> +	};
> +
Maybe:
"For Ultrascale+ the speedgrades don't matter" or something as a comment?
> +	if (tech == ADI_AXI_FPGA_TECH_XILINX_ULTRASCALE_PLUS) {
> +		limits->fvco_max = 1600000;
> +		limits->fvco_min = 800000;
> +	}
> +}
> +
>  static int axi_clkgen_wait_non_busy(struct axi_clkgen *axi_clkgen)
>  {
>  	unsigned int timeout = 10000;
> @@ -510,7 +555,7 @@ static int axi_clkgen_probe(struct platform_device *pdev)
>  	struct clk_init_data init;
>  	const char *parent_names[2];
>  	const char *clk_name;
> -	unsigned int i;
> +	unsigned int i, ver;
>  	int ret;
>  
>  	dflt_limits = device_get_match_data(&pdev->dev);
> @@ -537,6 +582,11 @@ static int axi_clkgen_probe(struct platform_device *pdev)
>  
>  	memcpy(&axi_clkgen->limits, dflt_limits, sizeof(axi_clkgen->limits));
>  
> +	axi_clkgen_read(axi_clkgen, ADI_AXI_REG_VERSION, &ver);
> +
> +	if (ADI_AXI_PCORE_VER_MAJOR(ver) > 0x04)
Nit: 0x4 is fine?
> +		axi_clkgen_setup_ranges(axi_clkgen);
> +
>  	clk_name = pdev->dev.of_node->name;
>  	of_property_read_string(pdev->dev.of_node, "clock-output-names",
>  		&clk_name);
> -- 
> 2.17.1
> 
Thanks,
Moritz