Received-SPF: pass (google.com: domain of linux-kernel+bounces-107554-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 19 Mar 2024 09:05:29 -0400 (EDT)
From: Charles Perry <charles.perry@savoirfairelinux.com>
To: Xu Yilun <yilun.xu@linux.intel.com>
Cc: mdf <mdf@kernel.org>, Allen VANDIVER <avandiver@markem-imaje.com>, 
	Brian CODY <bcody@markem-imaje.com>, hao wu <hao.wu@intel.com>, 
	yilun xu <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>, 
	Rob Herring <robh+dt@kernel.org>, 
	krzysztof kozlowski+dt <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, 
	Michal Simek <michal.simek@amd.com>, 
	linux-fpga <linux-fpga@vger.kernel.org>, 
	devicetree <devicetree@vger.kernel.org>, 
	linux-kernel <linux-kernel@vger.kernel.org>, 
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Message-ID: <1292581697.1872237.1710853529674.JavaMail.zimbra@savoirfairelinux.com>
In-Reply-To: <ZfkyMSczuFUdy3tU@yilunxu-OptiPlex-7050>
References: <20240313225746.489253-1-charles.perry@savoirfairelinux.com> <20240313225746.489253-4-charles.perry@savoirfairelinux.com> <ZfkyMSczuFUdy3tU@yilunxu-OptiPlex-7050>
Subject: Re: [PATCH v5 3/3] fpga: xilinx-selectmap: add new driver
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: fpga: xilinx-selectmap: add new driver
Thread-Index: 0HF96KxB+j7D1rBnTxEW357y9Tr3RQ==


On Mar 19, 2024, at 2:35 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Wed, Mar 13, 2024 at 06:57:37PM -0400, Charles Perry wrote:
>> Xilinx 7 series FPGA can be programmed using a parallel port named
>> the SelectMAP interface in the datasheet. This interface is compatible
>> with the i.MX6 EIM bus controller but other types of external memory
>> mapped parallel bus might work.
>> 
>> xilinx-selectmap currently only supports the x8 mode where data is loaded
>> at one byte per rising edge of the clock, with the MSb of each byte
>> presented to the D0 pin.
>> 
>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>> ---
>> Changes since v4: (from Yilun review)
>>  * xilinx-core: select between prog/init and prog_b/init-b
>> 
>>  drivers/fpga/Kconfig            |  8 +++
>>  drivers/fpga/Makefile           |  1 +
>>  drivers/fpga/xilinx-core.c      | 29 +++++++++-
>>  drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
>>  4 files changed, 133 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/fpga/xilinx-selectmap.c
>> 
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index d27a1ebf40838..37b35f58f0dfb 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -67,6 +67,14 @@ config FPGA_MGR_STRATIX10_SOC
>>  config FPGA_MGR_XILINX_CORE
>>  	tristate
>> 
>> +config FPGA_MGR_XILINX_SELECTMAP
>> +	tristate "Xilinx Configuration over SelectMAP"
>> +	depends on HAS_IOMEM
>> +	select FPGA_MGR_XILINX_CORE
>> +	help
>> +	  FPGA manager driver support for Xilinx FPGA configuration
>> +	  over SelectMAP interface.
>> +
>>  config FPGA_MGR_XILINX_SPI
>>  	tristate "Xilinx Configuration over Slave Serial (SPI)"
>>  	depends on SPI
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 7ec795b6a5a70..aeb89bb13517e 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>>  obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
>>  obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
>>  obj-$(CONFIG_FPGA_MGR_XILINX_CORE)	+= xilinx-core.o
>> +obj-$(CONFIG_FPGA_MGR_XILINX_SELECTMAP)	+= xilinx-selectmap.o
>>  obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>>  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>> diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
>> index a35c43382dd5f..ccdeb45eba4ee 100644
>> --- a/drivers/fpga/xilinx-core.c
>> +++ b/drivers/fpga/xilinx-core.c
>> @@ -171,6 +171,29 @@ static int xilinx_core_write_complete(struct fpga_manager
>> *mgr,
>>  	return -ETIMEDOUT;
>>  }
>> 
>> +/**
>> + * xilinx_core_devm_gpiod_get - Obtain a resource-managed GPIO using a
>> + *                              legacy consumer name fallback.
>> + *
>> + * @dev:           Device managing the GPIO
>> + * @con_id:        Consumer id
>> + * @legacy_con_id: Legacy consumer id
>> + * @flags:         optional GPIO initialization flags
>> + */
> 
> No need to have kernel doc comments for internal functions.
> 

Ok, will remove.

>> +static inline struct gpio_desc *
>> +xilinx_core_devm_gpiod_get(struct device *dev, const char *con_id,
>> +			   const char *legacy_con_id, enum gpiod_flags flags)
>> +{
>> +	struct gpio_desc *desc;
>> +
>> +	desc = devm_gpiod_get(dev, con_id, flags);
>> +	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
>> +	    of_device_is_compatible(dev->of_node, "xlnx,fpga-slave-serial"))
>> +		desc = devm_gpiod_get(dev, legacy_con_id, flags);
>> +
>> +	return desc;
>> +}
>> +
>>  static const struct fpga_manager_ops xilinx_core_ops = {
>>  	.state = xilinx_core_state,
>>  	.write_init = xilinx_core_write_init,
>> @@ -186,12 +209,14 @@ int xilinx_core_probe(struct xilinx_fpga_core *core)
>>  		return -EINVAL;
>> 
>>  	/* PROGRAM_B is active low */
>> -	core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
>> +	core->prog_b = xilinx_core_devm_gpiod_get(core->dev, "prog", "prog_b",
>> +						  GPIOD_OUT_LOW);
>>  	if (IS_ERR(core->prog_b))
>>  		return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
>>  				     "Failed to get PROGRAM_B gpio\n");
>> 
>> -	core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
>> +	core->init_b = xilinx_core_devm_gpiod_get(core->dev, "init", "init-b",
>> +						  GPIOD_IN);
>>  	if (IS_ERR(core->init_b))
>>  		return dev_err_probe(core->dev, PTR_ERR(core->init_b),
>>  				     "Failed to get INIT_B gpio\n");
> 
> Please make a separate patch for the naming change. This give a chance
> to explain why the change and how to correctly use the GPIO names.
> 

Ok

>> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
>> new file mode 100644
>> index 0000000000000..b63f4623f8b2c
>> --- /dev/null
>> +++ b/drivers/fpga/xilinx-selectmap.c
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Xilinx Spartan6 and 7 Series SelectMAP interface driver
>> + *
>> + * (C) 2024 Charles Perry <charles.perry@savoirfairelinux.com>
>> + *
>> + * Manage Xilinx FPGA firmware loaded over the SelectMAP configuration
>> + * interface.
>> + */
>> +
>> +#include "xilinx-core.h"
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of.h>
>> +#include <linux/io.h>
> 
> alphabetical order please.
> 

Ok

>> +
>> +struct xilinx_selectmap_conf {
>> +	struct xilinx_fpga_core core;
>> +	void __iomem *base;
>> +};
>> +
>> +#define to_xilinx_selectmap_conf(obj) \
>> +	container_of(obj, struct xilinx_selectmap_conf, core)
>> +
>> +static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>> +	u32 i;
>> +
>> +	for (i = 0; i < count; ++i)
>> +		writeb(buf[i], conf->base);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xilinx_selectmap_probe(struct platform_device *pdev)
>> +{
>> +	struct xilinx_selectmap_conf *conf;
>> +	struct resource *r;
>> +	void __iomem *base;
>> +	struct gpio_desc *csi_b;
>> +	struct gpio_desc *rdwr_b;
> 
> One gpio_desc is enough, is it?  We don't use these gpio_desc anywhere
> else.
> 

Ok

> BTW, reverse Xmas tree is not strictly required, but please do it when it is
> easy to follow.
>

Ok
 
>> +
>> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> +	if (!conf)
>> +		return -ENOMEM;
>> +
>> +	conf->core.dev = &pdev->dev;
>> +	conf->core.write = xilinx_selectmap_write;
>> +
>> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
> 
> I can't find where 'r' is used.
> 
> Thanks,
> Yilun
> 

I'm guilty of keeping things around in case I need them in the future
here! Ok, will use NULL.

Regards,
Charles

>> +	if (IS_ERR(base))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
>> +				     "ioremap error\n");
>> +	conf->base = base;
>> +
>> +	/* CSI_B is active low */
>> +	csi_b = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(csi_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(csi_b),
>> +				     "Failed to get CSI_B gpio\n");
>> +
>> +	/* RDWR_B is active low */
>> +	rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(rdwr_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(rdwr_b),
>> +				     "Failed to get RDWR_B gpio\n");
>> +
>> +	return xilinx_core_probe(&conf->core);
>> +}
>> +
>> +static const struct of_device_id xlnx_selectmap_of_match[] = {
>> +	{ .compatible = "xlnx,fpga-xc7s-selectmap", }, // Spartan-7
>> +	{ .compatible = "xlnx,fpga-xc7a-selectmap", }, // Artix-7
>> +	{ .compatible = "xlnx,fpga-xc7k-selectmap", }, // Kintex-7
>> +	{ .compatible = "xlnx,fpga-xc7v-selectmap", }, // Virtex-7
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, xlnx_selectmap_of_match);
>> +
>> +static struct platform_driver xilinx_selectmap_driver = {
>> +	.driver = {
>> +		.name = "xilinx-selectmap",
>> +		.of_match_table = xlnx_selectmap_of_match,
>> +	},
>> +	.probe  = xilinx_selectmap_probe,
>> +};
>> +
>> +module_platform_driver(xilinx_selectmap_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Charles Perry <charles.perry@savoirfairelinux.com>");
>> +MODULE_DESCRIPTION("Load Xilinx FPGA firmware over SelectMap");
>> --
>> 2.43.0