[PATCH] spi: amd: Configure device speed

[PATCH] spi: amd: Configure device speed

[Shreeya Patel]

From: Lucas Tanure <tanureal@opensource.cirrus.com>

Number of clock frequencies are supported by AMD controller
which are mentioned in the amd_spi_freq structure table.

Create mechanism to configure device clock frequency such
that it is the closest frequency supported by the
AMD controller.

Give priority to the device transfer speed and in case
it is not set then use the max clock frequency supported
by the device.

Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 98 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 08df4f8d0531..0cc9fd908f4f 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -36,6 +36,18 @@
 #define AMD_SPI_FIFO_SIZE	70
 #define AMD_SPI_MEM_SIZE	200
 
+#define AMD_SPI_ENA_REG		0x20
+#define AMD_SPI_ALT_SPD_SHIFT	20
+#define AMD_SPI_ALT_SPD_MASK	GENMASK(23, AMD_SPI_ALT_SPD_SHIFT)
+#define AMD_SPI_SPI100_SHIFT	0
+#define AMD_SPI_SPI100_MASK	GENMASK(AMD_SPI_SPI100_SHIFT, AMD_SPI_SPI100_SHIFT)
+#define AMD_SPI_SPEED_REG	0x6C
+#define AMD_SPI_SPD7_SHIFT	8
+#define AMD_SPI_SPD7_MASK	GENMASK(13, AMD_SPI_SPD7_SHIFT)
+
+#define AMD_SPI_MAX_HZ		100000000
+#define AMD_SPI_MIN_HZ		800000
+
 /* M_CMD OP codes for SPI */
 #define AMD_SPI_XFER_TX		1
 #define AMD_SPI_XFER_RX		2
@@ -50,14 +62,41 @@ enum amd_spi_versions {
 	AMD_SPI_V2,
 };
 
+enum amd_spi_speed {
+	F_66_66MHz,
+	F_33_33MHz,
+	F_22_22MHz,
+	F_16_66MHz,
+	F_100MHz,
+	F_800KHz,
+	SPI_SPD7,
+	F_50MHz = 0x4,
+	F_4MHz = 0x32,
+	F_3_17MHz = 0x3F
+};
+
+/**
+ * struct amd_spi_freq - Matches device speed with values to write in regs
+ * @speed_hz: Device frequency
+ * @enable_val: Value to be written to "enable register"
+ * @spd7_val: Some frequencies requires to have a value written at SPISPEED register
+ */
+struct amd_spi_freq {
+	u32 speed_hz;
+	u32 enable_val;
+	u32 spd7_val;
+};
+
 /**
  * struct amd_spi - SPI driver instance
  * @io_remap_addr:	Start address of the SPI controller registers
  * @version:		SPI controller hardware version
+ * @speed_hz:		Device frequency
  */
 struct amd_spi {
 	void __iomem *io_remap_addr;
 	enum amd_spi_versions version;
+	unsigned int speed_hz;
 };
 
 static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
@@ -189,10 +228,61 @@ static int amd_spi_master_setup(struct spi_device *spi)
 	return 0;
 }
 
+static const struct amd_spi_freq amd_spi_freq[] = {
+	{ AMD_SPI_MAX_HZ,   F_100MHz,         0},
+	{       66660000, F_66_66MHz,         0},
+	{       50000000,   SPI_SPD7,   F_50MHz},
+	{       33330000, F_33_33MHz,         0},
+	{       22220000, F_22_22MHz,         0},
+	{       16660000, F_16_66MHz,         0},
+	{        4000000,   SPI_SPD7,    F_4MHz},
+	{        3170000,   SPI_SPD7, F_3_17MHz},
+	{ AMD_SPI_MIN_HZ,   F_800KHz,         0},
+};
+
+static int amd_set_spi_freq(struct amd_spi *amd_spi, u32 speed_hz)
+{
+	unsigned int i, spd7_val, alt_spd;
+
+	if (speed_hz == amd_spi->speed_hz)
+		return 0;
+
+	if (speed_hz < AMD_SPI_MIN_HZ)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(amd_spi_freq); i++)
+		if (speed_hz >= amd_spi_freq[i].speed_hz)
+			break;
+
+	if (speed_hz == amd_spi_freq[i].speed_hz)
+		return 0;
+
+	amd_spi->speed_hz = amd_spi_freq[i].speed_hz;
+
+	alt_spd = (amd_spi_freq[i].enable_val << AMD_SPI_ALT_SPD_SHIFT)
+		   & AMD_SPI_ALT_SPD_MASK;
+	amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, alt_spd,
+			       AMD_SPI_ALT_SPD_MASK);
+
+	if (amd_spi->speed_hz == AMD_SPI_MAX_HZ)
+		amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, 1,
+				       AMD_SPI_SPI100_MASK);
+
+	if (amd_spi_freq[i].spd7_val) {
+		spd7_val = (amd_spi_freq[i].spd7_val << AMD_SPI_SPD7_SHIFT)
+			    & AMD_SPI_SPD7_MASK;
+		amd_spi_setclear_reg32(amd_spi, AMD_SPI_SPEED_REG, spd7_val,
+				       AMD_SPI_SPD7_MASK);
+	}
+
+	return 0;
+}
+
 static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
 				    struct spi_master *master,
 				    struct spi_message *message)
 {
+	struct spi_device *spi = message->spi;
 	struct spi_transfer *xfer = NULL;
 	u8 cmd_opcode;
 	u8 *buf = NULL;
@@ -202,6 +292,12 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
 
 	list_for_each_entry(xfer, &message->transfers,
 			    transfer_list) {
+
+		if (xfer->speed_hz)
+			amd_set_spi_freq(amd_spi, xfer->speed_hz);
+		else
+			amd_set_spi_freq(amd_spi, spi->max_speed_hz);
+
 		if (xfer->rx_buf)
 			m_cmd = AMD_SPI_XFER_RX;
 		if (xfer->tx_buf)
@@ -312,6 +408,8 @@ static int amd_spi_probe(struct platform_device *pdev)
 	master->num_chipselect = 4;
 	master->mode_bits = 0;
 	master->flags = SPI_MASTER_HALF_DUPLEX;
+	master->max_speed_hz = AMD_SPI_MAX_HZ;
+	master->min_speed_hz = AMD_SPI_MIN_HZ;
 	master->setup = amd_spi_master_setup;
 	master->transfer_one_message = amd_spi_master_transfer;
 	master->max_transfer_size = amd_spi_max_transfer_size;
-- 
2.30.2

Re: [PATCH] spi: amd: Configure device speed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Wed, Aug 17, 2022 at 06:48:09PM +0530, Shreeya Patel wrote:

> Create mechanism to configure device clock frequency such
> that it is the closest frequency supported by the
> AMD controller.

You shouldn't use the closest, you should use a frequency which is
strictly less than the requested one - driving things too fast will
generally break but too slow is pretty much always fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: amd: Configure device speed

[Shreeya Patel]

On 17/08/22 19:14, Mark Brown wrote:
> On Wed, Aug 17, 2022 at 06:48:09PM +0530, Shreeya Patel wrote:
>
>> Create mechanism to configure device clock frequency such
>> that it is the closest frequency supported by the
>> AMD controller.
> You shouldn't use the closest, you should use a frequency which is
> strictly less than the requested one - driving things too fast will
> generally break but too slow is pretty much always fine.
Hi Mark,

yes, the code is actually configuring it to use the frequency which is 
strictly less than the requested one.
I just didn't use the correct words. I will make the change in commit 
message for v2 once you review the patch.


Thanks
Shreeya Patel

Re: [PATCH] spi: amd: Configure device speed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On Wed, Aug 17, 2022 at 07:40:31PM +0530, Shreeya Patel wrote:

> yes, the code is actually configuring it to use the frequency which is
> strictly less than the requested one.
> I just didn't use the correct words. I will make the change in commit
> message for v2 once you review the patch.

Please just resend with a fixed commit message, I'll only have to review
it again anyway when you resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: amd: Configure device speed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3230 bytes --]

On Thu, Mar 17, 2022 at 11:14:53AM -0300, André Almeida wrote:

> Create mechanism to configure device clock frequency. For now, it can
> only set the device to use the maximum speed.

This changelog doesn't really tell me what the patch does - what is the
mechanism?  What exactly do you mean by "use the maximum speed" and why
aren't the normal speed setting interfaces supported?

> +/* Set device speed to the maximum frequency supported */
> +static bool max_speed;
> +module_param(max_speed, bool, 0644);

It s very surprising to have a module parameter for this, we should be
setting it by quirk.  If this is system specific presumably it's
possibly per controller system specific so a module parameter doesn't
cut it, and in general we don't have device specific command line quirks
like this.

> +static const struct amd_spi_freq amd_spi_freq[] = {
> +	{ AMD_SPI_MAX_HZ,   F_100MHz,         0},
> +	{       66660000, F_66_66MHz,         0},
> +	{       50000000,   SPI_SPD7,   F_50MHz},
> +	{       33330000, F_33_33MHz,         0},
> +	{       22220000, F_22_22MHz,         0},
> +	{       16660000, F_16_66MHz,         0},
> +	{        4000000,   SPI_SPD7,    F_4MHz},
> +	{        3170000,   SPI_SPD7, F_3_17MHz},
> +	{ AMD_SPI_MIN_HZ,   F_800KHz,         0},
> +};

This looks like a table of specific clock frequencies.  Why not just
implement the standard interface for setting the speed of SPI transfers?

> +static int amd_set_spi_freq(struct amd_spi *amd_spi, u32 speed_hz)
> +{
> +	unsigned int i, spd7_val, enable_val;
> +
> +	if (speed_hz == amd_spi->speed_hz)
> +		return 0;
> +
> +	if (speed_hz < AMD_SPI_MIN_HZ)
> +		return -EINVAL;

Why not just check to see if we don't find a matching value in the
table?

> +	for (i = 0; i < ARRAY_SIZE(amd_spi_freq); i++) {
> +		if (speed_hz >= amd_spi_freq[i].speed_hz) {
> +			if (amd_spi->speed_hz == amd_spi_freq[i].speed_hz)
> +				return 0;
> +
> +			amd_spi->speed_hz = amd_spi_freq[i].speed_hz;

This would be easier to read if it were refactored so that the code
applying the setting is factored out of the loop looking for a matching
entry in the table - that way most of the code would have less
indentation.

> +			enable_val = (amd_spi_freq[i].enable_val << AMD_SPI_ALT_SPD_SHIFT)
> +				     & AMD_SPI_ALT_SPD_MASK;
> +			amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, enable_val,
> +					       AMD_SPI_ALT_SPD_MASK);

So enable_val is actually the value to write to _ALT_SPD in _ENA_REG?
It might be clearer to call it alt_spd or something.  I have to say that
the use of set and clear on the same bitfield in the same operation is
surprising and I'd have to go and check the implementation to verify
that the set happens after the clear - it would be helpful to have an
_update_bits() style helper for these operations, it's more directly
what's being done.

> +
> +			if (amd_spi->speed_hz == AMD_SPI_MAX_HZ)
> +				amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, 1,
> +						       AMD_SPI_SPI100_MASK);

This is the same register as above - could things be combined, and also
doesn't the operation need to be reversed if we ever set a speed other
than the maximum?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] spi: amd: Configure device speed

[André Almeida]

Create mechanism to configure device clock frequency. For now, it can
only set the device to use the maximum speed.

Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
 drivers/spi/spi-amd.c | 98 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index d909afac6e21..ce4cce50496b 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -35,6 +35,36 @@
 
 #define AMD_SPI_MEM_SIZE	200
 
+#define AMD_SPI_ENA_REG		0x20
+#define AMD_SPI_ALT_SPD_SHIFT	20
+#define AMD_SPI_ALT_SPD_MASK	GENMASK(23, AMD_SPI_ALT_SPD_SHIFT)
+#define AMD_SPI_SPI100_SHIFT	0
+#define AMD_SPI_SPI100_MASK	GENMASK(AMD_SPI_SPI100_SHIFT, AMD_SPI_SPI100_SHIFT)
+#define AMD_SPI_SPEED_REG	0x6C
+#define AMD_SPI_SPD7_SHIFT	8
+#define AMD_SPI_SPD7_MASK	GENMASK(13, AMD_SPI_SPD7_SHIFT)
+
+#define AMD_SPI_MAX_HZ		100000000
+#define AMD_SPI_MIN_HZ		800000
+
+/* Set device speed to the maximum frequency supported */
+static bool max_speed;
+module_param(max_speed, bool, 0644);
+
+enum amd_spi_speed {
+	F_66_66MHz,
+	F_33_33MHz,
+	F_22_22MHz,
+	F_16_66MHz,
+	F_100MHz,
+	F_800KHz,
+	SPI_SPD6,
+	SPI_SPD7,
+	F_50MHz = 0x4,
+	F_4MHz = 0x32,
+	F_3_17MHz = 0x3F
+};
+
 /* M_CMD OP codes for SPI */
 #define AMD_SPI_XFER_TX		1
 #define AMD_SPI_XFER_RX		2
@@ -48,6 +78,19 @@ struct amd_spi {
 	void __iomem *io_remap_addr;
 	unsigned long io_base_addr;
 	enum amd_spi_versions version;
+	unsigned int speed_hz;
+};
+
+/**
+ * struct amd_spi_freq - Matches device speed with values to write in regs
+ * @speed_hz: Device frequency
+ * @ena: Value to be written to "enable register"
+ * @spd7: Some frequencies requires to have a value written at SPISPEED register
+ */
+struct amd_spi_freq {
+	unsigned int speed_hz;
+	unsigned int enable_val;
+	unsigned int spd7_val;
 };
 
 static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
@@ -170,12 +213,67 @@ static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
 	}
 }
 
+static const struct amd_spi_freq amd_spi_freq[] = {
+	{ AMD_SPI_MAX_HZ,   F_100MHz,         0},
+	{       66660000, F_66_66MHz,         0},
+	{       50000000,   SPI_SPD7,   F_50MHz},
+	{       33330000, F_33_33MHz,         0},
+	{       22220000, F_22_22MHz,         0},
+	{       16660000, F_16_66MHz,         0},
+	{        4000000,   SPI_SPD7,    F_4MHz},
+	{        3170000,   SPI_SPD7, F_3_17MHz},
+	{ AMD_SPI_MIN_HZ,   F_800KHz,         0},
+};
+
+static int amd_set_spi_freq(struct amd_spi *amd_spi, u32 speed_hz)
+{
+	unsigned int i, spd7_val, enable_val;
+
+	if (speed_hz == amd_spi->speed_hz)
+		return 0;
+
+	if (speed_hz < AMD_SPI_MIN_HZ)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(amd_spi_freq); i++) {
+		if (speed_hz >= amd_spi_freq[i].speed_hz) {
+			if (amd_spi->speed_hz == amd_spi_freq[i].speed_hz)
+				return 0;
+
+			amd_spi->speed_hz = amd_spi_freq[i].speed_hz;
+
+			enable_val = (amd_spi_freq[i].enable_val << AMD_SPI_ALT_SPD_SHIFT)
+				     & AMD_SPI_ALT_SPD_MASK;
+			amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, enable_val,
+					       AMD_SPI_ALT_SPD_MASK);
+
+			if (amd_spi->speed_hz == AMD_SPI_MAX_HZ)
+				amd_spi_setclear_reg32(amd_spi, AMD_SPI_ENA_REG, 1,
+						       AMD_SPI_SPI100_MASK);
+
+			if (amd_spi_freq[i].spd7_val) {
+				spd7_val = (amd_spi_freq[i].spd7_val << AMD_SPI_SPD7_SHIFT)
+					   & AMD_SPI_SPD7_MASK;
+				amd_spi_setclear_reg32(amd_spi, AMD_SPI_SPEED_REG, spd7_val,
+						       AMD_SPI_SPD7_MASK);
+			}
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+};
+
 static int amd_spi_master_setup(struct spi_device *spi)
 {
 	struct amd_spi *amd_spi = spi_master_get_devdata(spi->master);
 
 	amd_spi_clear_fifo_ptr(amd_spi);
 
+	if (max_speed)
+		amd_set_spi_freq(amd_spi, spi->max_speed_hz);
+
 	return 0;
 }
 
-- 
2.35.1