Re: [PATCH 3/3] fpga: xilinx-selectmap: add new driver

[Charles Perry]

On Feb 4, 2024, at 3:10 AM, Xu Yilun yilun.xu@xxxxxxxxxxxxxxx wrote:
> On Wed, Jan 31, 2024 at 06:05:33PM -0500, Charles Perry wrote:
>> Xilinx 7 series FPGA can be programmed using a slave parallel port named
>> the SelectMAP interface in the datasheet. This slave 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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  drivers/fpga/Kconfig            |   8 +++
>>  drivers/fpga/Makefile           |   1 +
>>  drivers/fpga/xilinx-core.c      |  11 +++-
>>  drivers/fpga/xilinx-core.h      |   3 +-
>>  drivers/fpga/xilinx-selectmap.c | 106 ++++++++++++++++++++++++++++++++
>>  drivers/fpga/xilinx-spi.c       |   3 +-
>>  6 files changed, 127 insertions(+), 5 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 aff40e9394085..64117759be100 100644
>> --- a/drivers/fpga/xilinx-core.c
>> +++ b/drivers/fpga/xilinx-core.c
>> @@ -180,21 +180,26 @@ static const struct fpga_manager_ops xilinx_core_ops = {
>>  
>>  int xilinx_core_probe(struct xilinx_fpga_core *core, struct device *dev,
>>  		      xilinx_write_func write,
>> -		      xilinx_write_one_dummy_byte_func write_one_dummy_byte)
>> +		      xilinx_write_one_dummy_byte_func write_one_dummy_byte,
>> +		      const char *prog_con_id, const char *init_con_id)
> 
> These gpio name inputs are not necessary except for DTS format concern.
> Is it possible we don't change the names?
> 

Krzysztof won't accept the old names as they are non standard (contain a 
prohibited char "_" and use a "-b" prefix for active low). I agree that it
is cumbersome to have different gpio names injected in the core by the top 
level driver.

Can I change the GPIO names of xlnx,fpga-slave-serial? i.e.:

 * "prog_b" -> "prog"
 * "init-b" -> "init"

and keep a fallback in the driver so that current users of this driver keep
working without adaptation in the DT.

>>  {
>>  	struct fpga_manager *mgr;
>>  
>> +	if (!core || !dev || !write || !write_one_dummy_byte || !prog_con_id ||
>> +	    !init_con_id)
>> +		return -EINVAL;
> 
> These checks belong to Patch #1.
> 

Ok

>> +
>>  	core->dev = dev;
>>  	core->write = write;
>>  	core->write_one_dummy_byte = write_one_dummy_byte;
>>  
>>  	/* PROGRAM_B is active low */
>> -	core->prog_b = devm_gpiod_get(dev, "prog_b", GPIOD_OUT_LOW);
>> +	core->prog_b = devm_gpiod_get(dev, prog_con_id, GPIOD_OUT_LOW);
>>  	if (IS_ERR(core->prog_b))
>>  		return dev_err_probe(dev, PTR_ERR(core->prog_b),
>>  				     "Failed to get PROGRAM_B gpio\n");
>>  
>> -	core->init_b = devm_gpiod_get_optional(dev, "init-b", GPIOD_IN);
>> +	core->init_b = devm_gpiod_get_optional(dev, init_con_id, GPIOD_IN);
>>  	if (IS_ERR(core->init_b))
>>  		return dev_err_probe(dev, PTR_ERR(core->init_b),
>>  				     "Failed to get INIT_B gpio\n");
>> diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
>> index 40e120945ba70..817f0e551d093 100644
>> --- a/drivers/fpga/xilinx-core.h
>> +++ b/drivers/fpga/xilinx-core.h
>> @@ -22,6 +22,7 @@ struct xilinx_fpga_core {
>>  
>>  int xilinx_core_probe(struct xilinx_fpga_core *core, struct device *dev,
>>  		      xilinx_write_func write,
>> -		      xilinx_write_one_dummy_byte_func write_one_dummy_byte);
>> +		      xilinx_write_one_dummy_byte_func write_one_dummy_byte,
>> +		      const char *prog_con_id, const char *init_con_id);
>>  
>>  #endif /* __XILINX_CORE_H */
>> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
>> new file mode 100644
>> index 0000000000000..08054e19bb498
>> --- /dev/null
>> +++ b/drivers/fpga/xilinx-selectmap.c
>> @@ -0,0 +1,106 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Xilinx Spartan6 and 7 Series SelectMAP interface driver
>> + *
>> + * (C) 2024 Charles Perry <charles.perry@xxxxxxxxxxxxxxxxxxxx>
>> + *
>> + * 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>
>> +
>> +struct xilinx_selectmap_conf {
>> +	struct xilinx_fpga_core core;
>> +	void __iomem *base;
>> +	struct gpio_desc *csi_b;
>> +	struct gpio_desc *rdwr_b;
> 
> These 2 gpio_desc are not used globally, maybe remove them here.
> 

Ok

>> +};
>> +
>> +#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_apply_padding(struct xilinx_fpga_core *core)
>> +{
>> +	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>> +
>> +	writeb(0xFF, conf->base);
> 
> Seems only one callback needed. Just use write() to xfer 1 byte?
> 
> Thanks,
> Yilun
> 

Yes, will be changed.

Thank you,
Charles

>> +	return 0;
>> +}
>> +
>> +static int xilinx_selectmap_probe(struct platform_device *pdev)
>> +{
>> +	struct xilinx_selectmap_conf *conf;
>> +	struct resource *r;
>> +	void __iomem *base;
>> +
>> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> +	if (!conf)
>> +		return -ENOMEM;
>> +
>> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
>> +	if (IS_ERR(base))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
>> +				     "ioremap error\n");
>> +	conf->base = base;
>> +
>> +	/* CSI_B is active low */
>> +	conf->csi_b =
>> +		devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(conf->csi_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
>> +				     "Failed to get CSI_B gpio\n");
>> +
>> +	/* RDWR_B is active low */
>> +	conf->rdwr_b =
>> +		devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(conf->rdwr_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
>> +				     "Failed to get RDWR_B gpio\n");
>> +
>> +	return xilinx_core_probe(&conf->core, &pdev->dev,
>> +				 xilinx_selectmap_write,
>> +				 xilinx_selectmap_apply_padding, "prog",
>> +				 "init");
>> +}
>> +
>> +static const struct of_device_id xlnx_selectmap_of_match[] = {
>> +	{
>> +		.compatible = "xlnx,fpga-selectmap",
>> +	},
>> +	{}
>> +};
>> +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@xxxxxxxxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Load Xilinx FPGA firmware over SelectMap");
>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
>> index ec128dee97312..b9ab3d5da004c 100644
>> --- a/drivers/fpga/xilinx-spi.c
>> +++ b/drivers/fpga/xilinx-spi.c
>> @@ -74,7 +74,8 @@ static int xilinx_spi_probe(struct spi_device *spi)
>>  	conf->spi = spi;
>>  
>>  	return xilinx_core_probe(&conf->core, &spi->dev, xilinx_spi_write,
>> -				 xilinx_spi_apply_cclk_cycles);
>> +				 xilinx_spi_apply_cclk_cycles, "prog_b",
>> +				 "init-b");
>>  }
>>  
>>  #ifdef CONFIG_OF
>> --
>> 2.43.0
>>