diff mbox series

[v2,3/6] spi: dw: Add Microchip Sparx5 support

Message ID 20200619113121.9984-4-lars.povlsen@microchip.com (mailing list archive)
State Changes Requested
Headers show
Series spi: Adding support for Microchip Sparx5 SoC | expand

Commit Message

Lars Povlsen June 19, 2020, 11:31 a.m. UTC
This adds SPI support for the Sparx5 SoC, which is using the MMIO
Designware SPI controller.

The Sparx5 differs from the Ocelot version in these areas:

 * The Sparx5 SPI controller has 2 different SPI bus interfaces on the
   same controller (don't ask...). As each SPI slave is physically
   located on a particular bus, they must be configured
   accordingly. The microchip,spi-interface2 property is used for
   this. Switching between busses also requires specific
   handling/timing.

 * The CS override is controlled by a new set of registers for
   this purpose.

 * The Sparx5 SPI controller has the RX sample delay register, and it
   must be configured for the (SPI NAND) device on SPI2.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 drivers/spi/spi-dw-mmio.c | 113 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 1 deletion(-)

Comments

Mark Brown June 19, 2020, 12:11 p.m. UTC | #1
On Fri, Jun 19, 2020 at 01:31:18PM +0200, Lars Povlsen wrote:

> +/*
> + * The Designware SPI controller (referred to as master in the
> + * documentation) automatically deasserts chip select when the tx fifo
> + * is empty. The chip selects then needs to be driven by a CS override
> + * register. enable is an active low signal.
> + */
> +static void dw_spi_sparx5_set_cs(struct spi_device *spi, bool nEnable)

The value that is passed in here is the value that should be driven on
the output pin, the driver should not be interpreting the value in any
way here.  Documenting it as nEnable adds a layer of confusion, and it
may not be an active high signal depending on the system.

> +	if (!nEnable) {
> +		/* Ensure CS toggles, so start off all disabled */
> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~0);
> +		/* CS override drive enable */
> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_ENA, 1);

This should just be setting the value to whatever the core asked for it
to be set to, the driver adding extra toggles is likely to disrupt
things.
Lars Povlsen June 22, 2020, 10:46 a.m. UTC | #2
Mark Brown writes:

On Fri, Jun 19, 2020 at 01:31:18PM +0200, Lars Povlsen wrote:

>> +/*
>> + * The Designware SPI controller (referred to as master in the
>> + * documentation) automatically deasserts chip select when the tx fifo
>> + * is empty. The chip selects then needs to be driven by a CS override
>> + * register. enable is an active low signal.
>> + */
>> +static void dw_spi_sparx5_set_cs(struct spi_device *spi, bool nEnable)
>
>The value that is passed in here is the value that should be driven on
>the output pin, the driver should not be interpreting the value in any
>way here.  Documenting it as nEnable adds a layer of confusion, and it
>may not be an active high signal depending on the system.

Ok, I will make the CS function more like the others.

>
>> +	if (!nEnable) {
>> +		/* Ensure CS toggles, so start off all disabled */
>> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~0);
>> +		/* CS override drive enable */
>> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_ENA, 1);
>
>This should just be setting the value to whatever the core asked for it
>to be set to, the driver adding extra toggles is likely to disrupt
>things.

I will have a look at this again. But it was added for a reason. The
issue is that we have two different busses in front of the controller,
so we might need more settle time when switching interface.

Thank you for you comments,

Cheers
Mark Brown June 22, 2020, 12:17 p.m. UTC | #3
On Mon, Jun 22, 2020 at 12:46:33PM +0200, Lars Povlsen wrote:
> On Fri, Jun 19, 2020 at 01:31:18PM +0200, Lars Povlsen wrote:

> >> +	if (!nEnable) {
> >> +		/* Ensure CS toggles, so start off all disabled */
> >> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~0);
> >> +		/* CS override drive enable */
> >> +		regmap_write(dwsmscc->syscon, SPARX5_FORCE_ENA, 1);

> >This should just be setting the value to whatever the core asked for it
> >to be set to, the driver adding extra toggles is likely to disrupt
> >things.

> I will have a look at this again. But it was added for a reason. The
> issue is that we have two different busses in front of the controller,
> so we might need more settle time when switching interface.

If there's a mux that needs to be handled specially that mux should be
described in DT on the relevant boards, there shouldn't just be
something hard coded in the controller driver.

BTW please do not CC subsystem patches to soc@kernel.org unless there's
a specific reason to do so - there's no need for it, these patches won't
get merged via there unless something is going wrong.  Generally the
subsystem maintainers take patches for a given subsystem.
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 403403deae664..78241d93289f5 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -20,6 +20,7 @@ 
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <linux/bitfield.h>
 
 #include "spi-dw.h"
 
@@ -41,6 +42,12 @@  struct dw_spi_mmio {
 #define MSCC_IF_SI_OWNER_SIBM			1
 #define MSCC_IF_SI_OWNER_SIMC			2
 
+#define SPARX5_CPU_SYSTEM_CTRL_GENERAL_CTRL	0x88
+#define SPARX5_IF_SI_OWNER			GENMASK(7, 6)
+#define SPARX5_IF_SI2_OWNER			GENMASK(5, 4)
+#define SPARX5_FORCE_ENA			0xa4
+#define SPARX5_FORCE_VAL			0xa8
+
 #define MSCC_SPI_MST_SW_MODE			0x14
 #define MSCC_SPI_MST_SW_MODE_SW_PIN_CTRL_MODE	BIT(13)
 #define MSCC_SPI_MST_SW_MODE_SW_SPI_CS(x)	(x << 5)
@@ -54,7 +61,8 @@  struct dw_spi_mmio {
 
 struct dw_spi_mscc {
 	struct regmap       *syscon;
-	void __iomem        *spi_mst;
+	void __iomem        *spi_mst; /* Not sparx5 */
+	u32		    if2mask;  /* sparx5 only */
 };
 
 /*
@@ -134,6 +142,108 @@  static int dw_spi_mscc_jaguar2_init(struct platform_device *pdev,
 				JAGUAR2_IF_SI_OWNER_OFFSET);
 }
 
+/*
+ * Set the owner of the SPI interface
+ */
+static void dw_spi_sparx5_set_owner(struct regmap *syscon,
+				    u8 owner, u8 owner2)
+{
+	u32 val, msk;
+
+	val = FIELD_PREP(SPARX5_IF_SI_OWNER, owner) |
+		FIELD_PREP(SPARX5_IF_SI2_OWNER, owner2);
+	msk = SPARX5_IF_SI_OWNER | SPARX5_IF_SI2_OWNER;
+	regmap_update_bits(syscon,
+			   SPARX5_CPU_SYSTEM_CTRL_GENERAL_CTRL,
+			   msk, val);
+}
+
+static void dw_spi_sparx5_set_cs_owner(struct dw_spi_mmio *dwsmmio,
+				       u8 cs, u8 owner)
+{
+	struct dw_spi_mscc *dwsmscc = dwsmmio->priv;
+	u8 other = (owner == MSCC_IF_SI_OWNER_SIBM ?
+		    MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);
+	if (dwsmscc->if2mask & BIT(cs))
+		/* SPI2 */
+		dw_spi_sparx5_set_owner(dwsmscc->syscon, other, owner);
+	else
+		/* SPI1 */
+		dw_spi_sparx5_set_owner(dwsmscc->syscon, owner, other);
+}
+
+/*
+ * The Designware SPI controller (referred to as master in the
+ * documentation) automatically deasserts chip select when the tx fifo
+ * is empty. The chip selects then needs to be driven by a CS override
+ * register. enable is an active low signal.
+ */
+static void dw_spi_sparx5_set_cs(struct spi_device *spi, bool nEnable)
+{
+	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_mscc *dwsmscc = dwsmmio->priv;
+	u8 cs = spi->chip_select;
+
+	if (!nEnable)
+		dw_spi_sparx5_set_cs_owner(dwsmmio, cs,
+					   MSCC_IF_SI_OWNER_SIMC);
+
+	if (!nEnable) {
+		/* Ensure CS toggles, so start off all disabled */
+		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~0);
+		/* CS override drive enable */
+		regmap_write(dwsmscc->syscon, SPARX5_FORCE_ENA, 1);
+		/* Allow settle */
+		udelay(1);
+		/* Now set CSx enabled */
+		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~BIT(cs));
+	} else {
+		/* CS value */
+		regmap_write(dwsmscc->syscon, SPARX5_FORCE_VAL, ~0);
+		/* CS override drive disable */
+		regmap_write(dwsmscc->syscon, SPARX5_FORCE_ENA, 0);
+	}
+
+	dw_spi_set_cs(spi, nEnable);
+}
+
+static int dw_spi_mscc_sparx5_init(struct platform_device *pdev,
+				   struct dw_spi_mmio *dwsmmio)
+{
+	const char *syscon_name = "microchip,sparx5-cpu-syscon";
+	struct dw_spi_mscc *dwsmscc;
+	struct device_node *nc;
+
+	dwsmscc = devm_kzalloc(&pdev->dev, sizeof(*dwsmscc), GFP_KERNEL);
+	if (!dwsmscc)
+		return -ENOMEM;
+
+	dwsmscc->syscon =
+		syscon_regmap_lookup_by_compatible(syscon_name);
+	if (IS_ERR(dwsmscc->syscon)) {
+		dev_err(&pdev->dev, "No syscon map %s\n", syscon_name);
+		return PTR_ERR(dwsmscc->syscon);
+	}
+
+	/* SPI2 mapping bitmask */
+	for_each_available_child_of_node(pdev->dev.of_node, nc) {
+		u32 cs;
+
+		if (of_property_read_u32(nc, "reg", &cs) == 0 &&
+		    of_property_read_bool(nc, "microchip,spi-interface2"))
+			dwsmscc->if2mask |= BIT(cs);
+	}
+
+	dwsmmio->dws.set_cs = dw_spi_sparx5_set_cs;
+	dwsmmio->priv = dwsmscc;
+
+	/* Register hook to configure CTRLR0 */
+	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
+
+	return 0;
+}
+
 static int dw_spi_alpine_init(struct platform_device *pdev,
 			      struct dw_spi_mmio *dwsmmio)
 {
@@ -297,6 +407,7 @@  static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
+	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);