Re: [PATCH 10/11] spi: dw: Add support for Pensando Elba SoC

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Wed, Apr 06, 2022 at 04:36:47PM -0700, Brad Larson wrote:
> The Pensando Elba SoC includes a DW apb_ssi v4 controller
> with device specific chip-select control.  The Elba SoC
> provides four chip-selects where the native DW IP supports
> two chip-selects.  The Elba DW_SPI instance has two native
> CS signals that are always overridden.
> 
> Signed-off-by: Brad Larson <brad@xxxxxxxxxxx>
> ---
> Change from V3:
> - Use more descriptive dt property pensando,syscon-spics
> - Minor changes from review input
> 
>  drivers/spi/spi-dw-mmio.c | 85 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 5101c4c6017b..f4636b271818 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -53,6 +53,24 @@ struct dw_spi_mscc {
>  	void __iomem        *spi_mst; /* Not sparx5 */
>  };
>  
> +struct dw_spi_elba {
> +	struct regmap *regmap;
> +	unsigned int reg;
> +};
> +
> +/*
> + * Elba SoC does not use ssi, pin override is used for cs 0,1 and
> + * gpios for cs 2,3 as defined in the device tree.
> + *
> + * cs:  |       1               0
> + * bit: |---3-------2-------1-------0
> + *      |  cs1   cs1_ovr   cs0   cs0_ovr
> + */
> +#define ELBA_SPICS_SHIFT(cs)		(2 * (cs))
> +#define ELBA_SPICS_MASK(cs)		(0x3 << ELBA_SPICS_SHIFT(cs))
> +#define ELBA_SPICS_SET(cs, val)	\
> +			((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs))
> +
>  /*
>   * The Designware SPI controller (referred to as master in the documentation)
>   * automatically deasserts chip select when the tx fifo is empty. The chip
> @@ -238,6 +256,72 @@ static int dw_spi_canaan_k210_init(struct platform_device *pdev,
>  	return 0;
>  }
>  

> +static void elba_spics_set_cs(struct dw_spi_elba *dwselba, int cs, int enable)
> +{
> +	regmap_update_bits(dwselba->regmap, dwselba->reg, ELBA_SPICS_MASK(cs),
> +			   ELBA_SPICS_SET(cs, enable));
> +}
> +
> +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)

The methods naming is ambiguous. Moreover it breaks this module naming
convention. Could you change them to something like:
dw_spi_elba_override_cs()
and
dw_spi_elba_set_cs()
?

> +{
> +	struct dw_spi *dws = spi_master_get_devdata(spi->master);
> +	struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws);
> +	struct dw_spi_elba *dwselba = dwsmmio->priv;
> +	u8 cs;
> +
> +	cs = spi->chip_select;
> +	if (cs < 2) {
> +		/* overridden native chip-select */
> +		elba_spics_set_cs(dwselba, spi->chip_select, enable);
> +	}
> +
> +	/*
> +	 * The DW SPI controller needs a native CS bit selected to start
> +	 * the serial engine and the platform may have fewer native CSs
> +	 * than needed, so use CS0 always.
> +	 */
> +	spi->chip_select = 0;
> +	dw_spi_set_cs(spi, enable);
> +	spi->chip_select = cs;
> +}
> +
> +static int dw_spi_elba_init(struct platform_device *pdev,
> +			    struct dw_spi_mmio *dwsmmio)
> +{
> +	struct of_phandle_args args;
> +	struct dw_spi_elba *dwselba;
> +	struct regmap *regmap;
> +	int rc;
> +
> +	rc = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +			"pensando,syscon-spics", 1, 0, &args);
> +	if (rc) {
> +		dev_err(&pdev->dev, "could not find spics\n");
> +		return rc;
> +	}
> +
> +	regmap = syscon_node_to_regmap(args.np);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(regmap),
> +				     "could not map spics");
> +
> +	dwselba = devm_kzalloc(&pdev->dev, sizeof(*dwselba), GFP_KERNEL);
> +	if (!dwselba)
> +		return -ENOMEM;
> +
> +	dwselba->regmap = regmap;
> +	dwselba->reg = args.args[0];
> +
> +	/* deassert cs */

> +	elba_spics_set_cs(dwselba, 0, 1);
> +	elba_spics_set_cs(dwselba, 1, 1);

What if the CS lines are of the active-high type? In that case basically
you get to do the opposite to what you claim in the comment here.

Note the CS setting into the deactivated state is done in the spi_setup()
method anyway, at the moment of the peripheral SPI device registration
stage (see its calling the spi_set_cs() function). Thus what you are doing
here is redundant.

-Sergey

> +
> +	dwsmmio->priv = dwselba;
> +	dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -352,6 +436,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
>  	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
> +	{ .compatible = "pensando,elba-spi", .data = dw_spi_elba_init},
>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> -- 
> 2.17.1
>