All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length
@ 2023-02-05 23:02 Lars-Peter Clausen
  2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
  2023-02-06  8:02 ` [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Michal Simek
  0 siblings, 2 replies; 9+ messages in thread
From: Lars-Peter Clausen @ 2023-02-05 23:02 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Michal Simek, Shubhrajyoti Datta, Rob Herring,
	Krzysztof Kozlowski, linux-i2c, devicetree, Lars-Peter Clausen

The FIFO depth and maximum transfer length are Synthesis configuration
parameters of the Cadence I2C IP. Different SoCs will use different values
for these parameters.

Currently the driver has the FIFO depth hardcoded to 16 and the maximum
transfer length to 255. Trying to use the driver with an IP instance that
uses smaller values for these will work for short transfers. But longer
transfers will fail.

The maximum transfer length can easily be detected at runtime since the
unused MSBs of the transfer length register are hardwired to 0. Writing
0xff and then reading back the value will give the maximum transfer length.

This is unfortunately not possible for the FIFO depth. To handle this
introduce a new devicetree property that allows to describe the FIFO depth
of the I2C controller.

These changes have been tested with
1) The Xilinx MPSoC for which this driver was originally written which has
   the previous hardcoded settings of 16 and 255.
2) Another instance of the Cadence I2C IP with FIFO depth of 8 and maximum
   transfer length of 16.

Without these changes the latter would fail for I2C transfers longer than
8. With the updated driver both work fine even for longer transfers.

Note that the IP core and driver support chaining multiple transfers into a
single longer transfer using the HOLD bit. So the maximum transfer size is
not the limit for the length of the I2C transfer, but the limit for how
much data can be transferred without having to reprogram the control
registers.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 77 ++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index b5d22e7282c2..25c0a8bacdd6 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -114,10 +114,10 @@
 /* timeout for pm runtime autosuspend */
 #define CNDS_I2C_PM_TIMEOUT		1000	/* ms */
 
-#define CDNS_I2C_FIFO_DEPTH		16
+#define CDNS_I2C_FIFO_DEPTH_DEFAULT	16
 #define CDNS_I2C_MAX_TRANSFER_SIZE	255
 /* Transfer size in multiples of data interrupt depth */
-#define CDNS_I2C_TRANSFER_SIZE	(CDNS_I2C_MAX_TRANSFER_SIZE - 3)
+#define CDNS_I2C_TRANSFER_SIZE(max)	((max) - 3)
 
 #define DRIVER_NAME		"cdns-i2c"
 
@@ -211,6 +211,8 @@ struct cdns_i2c {
 	enum cdns_i2c_mode dev_mode;
 	enum cdns_i2c_slave_state slave_state;
 #endif
+	unsigned int fifo_depth;
+	unsigned int transfer_size;
 };
 
 struct cdns_platform_data {
@@ -236,7 +238,7 @@ static void cdns_i2c_clear_bus_hold(struct cdns_i2c *id)
 static inline bool cdns_is_holdquirk(struct cdns_i2c *id, bool hold_wrkaround)
 {
 	return (hold_wrkaround &&
-		(id->curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1));
+		(id->curr_recv_count == id->fifo_depth + 1));
 }
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -431,7 +433,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
 				 * if RX data left is less than or equal to
 				 * FIFO DEPTH unless repeated start is selected
 				 */
-				if (id->recv_count <= CDNS_I2C_FIFO_DEPTH &&
+				if (id->recv_count <= id->fifo_depth &&
 				    !id->bus_hold_flag)
 					cdns_i2c_clear_bus_hold(id);
 
@@ -456,22 +458,22 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
 		if (cdns_is_holdquirk(id, updatetx)) {
 			/* wait while fifo is full */
 			while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
-			       (id->curr_recv_count - CDNS_I2C_FIFO_DEPTH))
+			       (id->curr_recv_count - id->fifo_depth))
 				;
 
 			/*
 			 * Check number of bytes to be received against maximum
 			 * transfer size and update register accordingly.
 			 */
-			if (((int)(id->recv_count) - CDNS_I2C_FIFO_DEPTH) >
-			    CDNS_I2C_TRANSFER_SIZE) {
-				cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
+			if (((int)(id->recv_count) - id->fifo_depth) >
+			    id->transfer_size) {
+				cdns_i2c_writereg(id->transfer_size,
 						  CDNS_I2C_XFER_SIZE_OFFSET);
-				id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE +
-						      CDNS_I2C_FIFO_DEPTH;
+				id->curr_recv_count = id->transfer_size +
+						      id->fifo_depth;
 			} else {
 				cdns_i2c_writereg(id->recv_count -
-						  CDNS_I2C_FIFO_DEPTH,
+						  id->fifo_depth,
 						  CDNS_I2C_XFER_SIZE_OFFSET);
 				id->curr_recv_count = id->recv_count;
 			}
@@ -494,7 +496,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
 		 * space available in FIFO and fill with that many bytes.
 		 */
 		if (id->send_count) {
-			avail_bytes = CDNS_I2C_FIFO_DEPTH -
+			avail_bytes = id->fifo_depth -
 			    cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
 			if (id->send_count > avail_bytes)
 				bytes_to_send = avail_bytes;
@@ -588,7 +590,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	 * Check for the message size against FIFO depth and set the
 	 * 'hold bus' bit if it is greater than FIFO depth.
 	 */
-	if (id->recv_count > CDNS_I2C_FIFO_DEPTH)
+	if (id->recv_count > id->fifo_depth)
 		ctrl_reg |= CDNS_I2C_CR_HOLD;
 
 	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
@@ -603,16 +605,16 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	 * receive if it is less than transfer size and transfer size if
 	 * it is more. Enable the interrupts.
 	 */
-	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
-		cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
+	if (id->recv_count > id->transfer_size) {
+		cdns_i2c_writereg(id->transfer_size,
 				  CDNS_I2C_XFER_SIZE_OFFSET);
-		id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
+		id->curr_recv_count = id->transfer_size;
 	} else {
 		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
 	}
 
 	/* Determine hold_clear based on number of bytes to receive and hold flag */
-	if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
+	if (!id->bus_hold_flag && id->recv_count <= id->fifo_depth) {
 		if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
 			hold_clear = true;
 			if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
@@ -673,7 +675,7 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
 	 * Check for the message size against FIFO depth and set the
 	 * 'hold bus' bit if it is greater than FIFO depth.
 	 */
-	if (id->send_count > CDNS_I2C_FIFO_DEPTH)
+	if (id->send_count > id->fifo_depth)
 		ctrl_reg |= CDNS_I2C_CR_HOLD;
 	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
 
@@ -686,7 +688,7 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
 	 * against the space available, and fill the FIFO accordingly.
 	 * Enable the interrupts.
 	 */
-	avail_bytes = CDNS_I2C_FIFO_DEPTH -
+	avail_bytes = id->fifo_depth -
 				cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
 
 	if (id->send_count > avail_bytes)
@@ -1226,6 +1228,36 @@ static const struct of_device_id cdns_i2c_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, cdns_i2c_of_match);
 
+/**
+ * cdns_i2c_detect_transfer_size - Detect the maximum transfer size supported
+ * @id:		Device private data structure
+ *
+ * Detect the maximum transfer size that is supported by this instance of the
+ * Cadence I2C controller.
+ */
+static void cdns_i2c_detect_transfer_size(struct cdns_i2c *id)
+{
+	u32 val;
+
+	/*
+	 * Writing to the transfer size register is only possible if these two bits
+	 * are set in the control register.
+	 */
+	cdns_i2c_writereg(CDNS_I2C_CR_MS | CDNS_I2C_CR_RW, CDNS_I2C_CR_OFFSET);
+	/*
+	 * The number of writable bits of the transfer size register can be between
+	 * 4 and 8. This is a controlled through a synthesis parameter of the IP
+	 * core and can vary from instance to instance. The unused MSBs always read
+	 * back as 0. Writing 0xff and then reading the value back will report the
+	 * maximum supported transfer size.
+	 */
+	cdns_i2c_writereg(CDNS_I2C_MAX_TRANSFER_SIZE, CDNS_I2C_XFER_SIZE_OFFSET);
+	val = cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
+	id->transfer_size = CDNS_I2C_TRANSFER_SIZE(val);
+	cdns_i2c_writereg(0, CDNS_I2C_XFER_SIZE_OFFSET);
+	cdns_i2c_writereg(0, CDNS_I2C_CR_OFFSET);
+}
+
 /**
  * cdns_i2c_probe - Platform registration call
  * @pdev:	Handle to the platform device structure
@@ -1242,6 +1274,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	struct cdns_i2c *id;
 	int ret, irq;
 	const struct of_device_id *match;
+	u32 val;
 
 	id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
 	if (!id)
@@ -1317,6 +1350,12 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 #endif
 	id->ctrl_reg = CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS;
 
+	val = CDNS_I2C_FIFO_DEPTH_DEFAULT;
+	of_property_read_u32(pdev->dev.of_node, "cdns,fifo-depth", &val);
+	id->fifo_depth = val;
+
+	cdns_i2c_detect_transfer_size(id);
+
 	ret = cdns_i2c_setclk(id->input_clk, id);
 	if (ret) {
 		dev_err(&pdev->dev, "invalid SCL clock: %u Hz\n", id->i2c_clk);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property
  2023-02-05 23:02 [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Lars-Peter Clausen
@ 2023-02-05 23:02 ` Lars-Peter Clausen
  2023-02-06  7:32   ` Michal Simek
                     ` (2 more replies)
  2023-02-06  8:02 ` [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Michal Simek
  1 sibling, 3 replies; 9+ messages in thread
From: Lars-Peter Clausen @ 2023-02-05 23:02 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Michal Simek, Shubhrajyoti Datta, Rob Herring,
	Krzysztof Kozlowski, linux-i2c, devicetree, Lars-Peter Clausen

The depth of the FIFO of the Cadence I2C controller IP is a synthesis
configuration parameter. Different instances of the IP can have different
values. For correct operation software needs to be aware of the size of the
FIFO.

Add the documentation for the devicetree property that describes the FIFO
depth of the IP core.

The default value of 16 is for backwards compatibility reasons with
existing hardware descriptions where this property is not specified and
software has assumed that the FIFO depth is 16.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
index 2e95cda7262a..3daa2fa73257 100644
--- a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
+++ b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
@@ -38,6 +38,12 @@ properties:
     description: |
       Input clock name.
 
+  cdns,fifo-depth:
+    description:
+      Size of the data FIFO in words.
+    $ref: "/schemas/types.yaml#/definitions/uint32"
+    default: 16
+
 required:
   - compatible
   - reg
@@ -57,4 +63,6 @@ examples:
         clock-frequency = <400000>;
         #address-cells = <1>;
         #size-cells = <0>;
+
+        cdns,fifo-depth = <8>;
     };
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property
  2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
@ 2023-02-06  7:32   ` Michal Simek
  2023-02-06  7:41   ` Krzysztof Kozlowski
  2023-02-06 16:58   ` Rob Herring
  2 siblings, 0 replies; 9+ messages in thread
From: Michal Simek @ 2023-02-06  7:32 UTC (permalink / raw)
  To: Lars-Peter Clausen, Wolfram Sang
  Cc: Shubhrajyoti Datta, Rob Herring, Krzysztof Kozlowski, linux-i2c,
	devicetree



On 2/6/23 00:02, Lars-Peter Clausen wrote:
> The depth of the FIFO of the Cadence I2C controller IP is a synthesis
> configuration parameter. Different instances of the IP can have different
> values. For correct operation software needs to be aware of the size of the
> FIFO.
> 
> Add the documentation for the devicetree property that describes the FIFO
> depth of the IP core.
> 
> The default value of 16 is for backwards compatibility reasons with
> existing hardware descriptions where this property is not specified and
> software has assumed that the FIFO depth is 16.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> index 2e95cda7262a..3daa2fa73257 100644
> --- a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> +++ b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> @@ -38,6 +38,12 @@ properties:
>       description: |
>         Input clock name.
>   
> +  cdns,fifo-depth:
> +    description:
> +      Size of the data FIFO in words.
> +    $ref: "/schemas/types.yaml#/definitions/uint32"
> +    default: 16
> +
>   required:
>     - compatible
>     - reg
> @@ -57,4 +63,6 @@ examples:
>           clock-frequency = <400000>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> +

nit: Remove this newline.

> +        cdns,fifo-depth = <8>;
>       };



This should be 1/2 before the patch which actually use it.

M

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property
  2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
  2023-02-06  7:32   ` Michal Simek
@ 2023-02-06  7:41   ` Krzysztof Kozlowski
  2023-02-06 15:20     ` Lars-Peter Clausen
  2023-02-06 16:58   ` Rob Herring
  2 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-06  7:41 UTC (permalink / raw)
  To: Lars-Peter Clausen, Wolfram Sang
  Cc: Michal Simek, Shubhrajyoti Datta, Rob Herring,
	Krzysztof Kozlowski, linux-i2c, devicetree

On 06/02/2023 00:02, Lars-Peter Clausen wrote:
> The depth of the FIFO of the Cadence I2C controller IP is a synthesis
> configuration parameter. Different instances of the IP can have different
> values. For correct operation software needs to be aware of the size of the
> FIFO.

Cannot this be inferred from compatible?

> 
> Add the documentation for the devicetree property that describes the FIFO
> depth of the IP core.
> 
> The default value of 16 is for backwards compatibility reasons with
> existing hardware descriptions where this property is not specified and
> software has assumed that the FIFO depth is 16.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> index 2e95cda7262a..3daa2fa73257 100644
> --- a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> +++ b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> @@ -38,6 +38,12 @@ properties:
>      description: |
>        Input clock name.
>  
> +  cdns,fifo-depth:
> +    description:
> +      Size of the data FIFO in words.
> +    $ref: "/schemas/types.yaml#/definitions/uint32"

Drop quotes.

> +    default: 16

maximum
minimum?


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length
  2023-02-05 23:02 [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Lars-Peter Clausen
  2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
@ 2023-02-06  8:02 ` Michal Simek
  2023-02-16 14:12   ` Lars-Peter Clausen
  1 sibling, 1 reply; 9+ messages in thread
From: Michal Simek @ 2023-02-06  8:02 UTC (permalink / raw)
  To: Lars-Peter Clausen, Wolfram Sang
  Cc: Shubhrajyoti Datta, Rob Herring, Krzysztof Kozlowski, linux-i2c,
	devicetree



On 2/6/23 00:02, Lars-Peter Clausen wrote:
> The FIFO depth and maximum transfer length are Synthesis configuration
> parameters of the Cadence I2C IP. Different SoCs will use different values
> for these parameters.
> 
> Currently the driver has the FIFO depth hardcoded to 16 and the maximum
> transfer length to 255. Trying to use the driver with an IP instance that
> uses smaller values for these will work for short transfers. But longer
> transfers will fail.
> 
> The maximum transfer length can easily be detected at runtime since the
> unused MSBs of the transfer length register are hardwired to 0. Writing
> 0xff and then reading back the value will give the maximum transfer length.

I think that these are pretty much two patches in one.
The first should target transfer length part and second fifo depth part.

> 
> This is unfortunately not possible for the FIFO depth. To handle this
> introduce a new devicetree property that allows to describe the FIFO depth
> of the I2C controller.
 > > These changes have been tested with
> 1) The Xilinx MPSoC for which this driver was originally written which has
>     the previous hardcoded settings of 16 and 255.
> 2) Another instance of the Cadence I2C IP with FIFO depth of 8 and maximum
>     transfer length of 16.
> 
> Without these changes the latter would fail for I2C transfers longer than
> 8. With the updated driver both work fine even for longer transfers.
> 
> Note that the IP core and driver support chaining multiple transfers into a
> single longer transfer using the HOLD bit. So the maximum transfer size is
> not the limit for the length of the I2C transfer, but the limit for how
> much data can be transferred without having to reprogram the control
> registers.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 77 ++++++++++++++++++++++++--------
>   1 file changed, 58 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index b5d22e7282c2..25c0a8bacdd6 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -114,10 +114,10 @@
>   /* timeout for pm runtime autosuspend */
>   #define CNDS_I2C_PM_TIMEOUT		1000	/* ms */
>   
> -#define CDNS_I2C_FIFO_DEPTH		16
> +#define CDNS_I2C_FIFO_DEPTH_DEFAULT	16
>   #define CDNS_I2C_MAX_TRANSFER_SIZE	255
>   /* Transfer size in multiples of data interrupt depth */
> -#define CDNS_I2C_TRANSFER_SIZE	(CDNS_I2C_MAX_TRANSFER_SIZE - 3)
> +#define CDNS_I2C_TRANSFER_SIZE(max)	((max) - 3)
>   
>   #define DRIVER_NAME		"cdns-i2c"
>   
> @@ -211,6 +211,8 @@ struct cdns_i2c {
>   	enum cdns_i2c_mode dev_mode;
>   	enum cdns_i2c_slave_state slave_state;
>   #endif
> +	unsigned int fifo_depth;
> +	unsigned int transfer_size;

Please write also kernel-doc
drivers/i2c/busses/i2c-cadence.c:216: warning: Function parameter or member 
'rinfo' not described in 'cdns_i2c'
drivers/i2c/busses/i2c-cadence.c:216: warning: Function parameter or member 
'fifo_depth' not described in 'cdns_i2c'
drivers/i2c/busses/i2c-cadence.c:216: warning: Function parameter or member 
'transfer_size' not described in 'cdns_i2c'

>   };
>   
>   struct cdns_platform_data {
> @@ -236,7 +238,7 @@ static void cdns_i2c_clear_bus_hold(struct cdns_i2c *id)
>   static inline bool cdns_is_holdquirk(struct cdns_i2c *id, bool hold_wrkaround)
>   {
>   	return (hold_wrkaround &&
> -		(id->curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1));
> +		(id->curr_recv_count == id->fifo_depth + 1));
>   }
>   
>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -431,7 +433,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
>   				 * if RX data left is less than or equal to
>   				 * FIFO DEPTH unless repeated start is selected
>   				 */
> -				if (id->recv_count <= CDNS_I2C_FIFO_DEPTH &&
> +				if (id->recv_count <= id->fifo_depth &&
>   				    !id->bus_hold_flag)
>   					cdns_i2c_clear_bus_hold(id);
>   
> @@ -456,22 +458,22 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
>   		if (cdns_is_holdquirk(id, updatetx)) {
>   			/* wait while fifo is full */
>   			while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
> -			       (id->curr_recv_count - CDNS_I2C_FIFO_DEPTH))
> +			       (id->curr_recv_count - id->fifo_depth))
>   				;
>   
>   			/*
>   			 * Check number of bytes to be received against maximum
>   			 * transfer size and update register accordingly.
>   			 */
> -			if (((int)(id->recv_count) - CDNS_I2C_FIFO_DEPTH) >
> -			    CDNS_I2C_TRANSFER_SIZE) {
> -				cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
> +			if (((int)(id->recv_count) - id->fifo_depth) >
> +			    id->transfer_size) {
> +				cdns_i2c_writereg(id->transfer_size,
>   						  CDNS_I2C_XFER_SIZE_OFFSET);
> -				id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE +
> -						      CDNS_I2C_FIFO_DEPTH;
> +				id->curr_recv_count = id->transfer_size +
> +						      id->fifo_depth;
>   			} else {
>   				cdns_i2c_writereg(id->recv_count -
> -						  CDNS_I2C_FIFO_DEPTH,
> +						  id->fifo_depth,
>   						  CDNS_I2C_XFER_SIZE_OFFSET);
>   				id->curr_recv_count = id->recv_count;
>   			}
> @@ -494,7 +496,7 @@ static irqreturn_t cdns_i2c_master_isr(void *ptr)
>   		 * space available in FIFO and fill with that many bytes.
>   		 */
>   		if (id->send_count) {
> -			avail_bytes = CDNS_I2C_FIFO_DEPTH -
> +			avail_bytes = id->fifo_depth -
>   			    cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
>   			if (id->send_count > avail_bytes)
>   				bytes_to_send = avail_bytes;
> @@ -588,7 +590,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>   	 * Check for the message size against FIFO depth and set the
>   	 * 'hold bus' bit if it is greater than FIFO depth.
>   	 */
> -	if (id->recv_count > CDNS_I2C_FIFO_DEPTH)
> +	if (id->recv_count > id->fifo_depth)
>   		ctrl_reg |= CDNS_I2C_CR_HOLD;
>   
>   	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
> @@ -603,16 +605,16 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>   	 * receive if it is less than transfer size and transfer size if
>   	 * it is more. Enable the interrupts.
>   	 */
> -	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
> -		cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
> +	if (id->recv_count > id->transfer_size) {
> +		cdns_i2c_writereg(id->transfer_size,
>   				  CDNS_I2C_XFER_SIZE_OFFSET);
> -		id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
> +		id->curr_recv_count = id->transfer_size;
>   	} else {
>   		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
>   	}
>   
>   	/* Determine hold_clear based on number of bytes to receive and hold flag */
> -	if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
> +	if (!id->bus_hold_flag && id->recv_count <= id->fifo_depth) {
>   		if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
>   			hold_clear = true;
>   			if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
> @@ -673,7 +675,7 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
>   	 * Check for the message size against FIFO depth and set the
>   	 * 'hold bus' bit if it is greater than FIFO depth.
>   	 */
> -	if (id->send_count > CDNS_I2C_FIFO_DEPTH)
> +	if (id->send_count > id->fifo_depth)
>   		ctrl_reg |= CDNS_I2C_CR_HOLD;
>   	cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET);
>   
> @@ -686,7 +688,7 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
>   	 * against the space available, and fill the FIFO accordingly.
>   	 * Enable the interrupts.
>   	 */
> -	avail_bytes = CDNS_I2C_FIFO_DEPTH -
> +	avail_bytes = id->fifo_depth -
>   				cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
>   
>   	if (id->send_count > avail_bytes)
> @@ -1226,6 +1228,36 @@ static const struct of_device_id cdns_i2c_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, cdns_i2c_of_match);
>   
> +/**
> + * cdns_i2c_detect_transfer_size - Detect the maximum transfer size supported
> + * @id:		Device private data structure
> + *
> + * Detect the maximum transfer size that is supported by this instance of the
> + * Cadence I2C controller.
> + */
> +static void cdns_i2c_detect_transfer_size(struct cdns_i2c *id)
> +{
> +	u32 val;
> +
> +	/*
> +	 * Writing to the transfer size register is only possible if these two bits
> +	 * are set in the control register.
> +	 */
> +	cdns_i2c_writereg(CDNS_I2C_CR_MS | CDNS_I2C_CR_RW, CDNS_I2C_CR_OFFSET);

nit: newline

> +	/*
> +	 * The number of writable bits of the transfer size register can be between
> +	 * 4 and 8. This is a controlled through a synthesis parameter of the IP
> +	 * core and can vary from instance to instance. The unused MSBs always read
> +	 * back as 0. Writing 0xff and then reading the value back will report the
> +	 * maximum supported transfer size.
> +	 */
> +	cdns_i2c_writereg(CDNS_I2C_MAX_TRANSFER_SIZE, CDNS_I2C_XFER_SIZE_OFFSET);
> +	val = cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
> +	id->transfer_size = CDNS_I2C_TRANSFER_SIZE(val);
> +	cdns_i2c_writereg(0, CDNS_I2C_XFER_SIZE_OFFSET);
> +	cdns_i2c_writereg(0, CDNS_I2C_CR_OFFSET);
> +}
> +
>   /**
>    * cdns_i2c_probe - Platform registration call
>    * @pdev:	Handle to the platform device structure
> @@ -1242,6 +1274,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>   	struct cdns_i2c *id;
>   	int ret, irq;
>   	const struct of_device_id *match;
> +	u32 val;
>   
>   	id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
>   	if (!id)
> @@ -1317,6 +1350,12 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>   #endif
>   	id->ctrl_reg = CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS;
>   
> +	val = CDNS_I2C_FIFO_DEPTH_DEFAULT;

you can remove val completely.
id->fifo_depth = CDNS_I2C_FIFO_DEPTH_DEFAULT;
...


> +	of_property_read_u32(pdev->dev.of_node, "cdns,fifo-depth", &val);
> +	id->fifo_depth = val;
> +
> +	cdns_i2c_detect_transfer_size(id);
> +
>   	ret = cdns_i2c_setclk(id->input_clk, id);
>   	if (ret) {
>   		dev_err(&pdev->dev, "invalid SCL clock: %u Hz\n", id->i2c_clk);

In general changes looks good.

M

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property
  2023-02-06  7:41   ` Krzysztof Kozlowski
@ 2023-02-06 15:20     ` Lars-Peter Clausen
  0 siblings, 0 replies; 9+ messages in thread
From: Lars-Peter Clausen @ 2023-02-06 15:20 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Wolfram Sang
  Cc: Michal Simek, Shubhrajyoti Datta, Rob Herring,
	Krzysztof Kozlowski, linux-i2c, devicetree

Thanks for the review.

On 2/5/23 23:41, Krzysztof Kozlowski wrote:
> On 06/02/2023 00:02, Lars-Peter Clausen wrote:
>> The depth of the FIFO of the Cadence I2C controller IP is a synthesis
>> configuration parameter. Different instances of the IP can have different
>> values. For correct operation software needs to be aware of the size of the
>> FIFO.
> Cannot this be inferred from compatible?

The compatible string currently encodes the version of the IP core. The 
FIFO depth is an orthogonal setting. You could of course encode both the 
version and FIFO depth as part of the compatible string. But you'd end 
up with a combinatorial explosion of compatible strings. Like

cdns,i2c-r1p10-fifo-depth-2, ... cdns,i2c-r1p10-fifo-depth-16, ..., 
cdns,i2c-r1p10-fifo-depth-256, cdns,i2c-r1p14-fifo-depth-2, ... 
cdns,i2c-r1p14-fifo-depth-16, ..., cdns,i2c-r1p14-fifo-depth-256,

>
>> Add the documentation for the devicetree property that describes the FIFO
>> depth of the IP core.
>>
>> The default value of 16 is for backwards compatibility reasons with
>> existing hardware descriptions where this property is not specified and
>> software has assumed that the FIFO depth is 16.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
>> index 2e95cda7262a..3daa2fa73257 100644
>> --- a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
>> @@ -38,6 +38,12 @@ properties:
>>       description: |
>>         Input clock name.
>>   
>> +  cdns,fifo-depth:
>> +    description:
>> +      Size of the data FIFO in words.
>> +    $ref: "/schemas/types.yaml#/definitions/uint32"
> Drop quotes.
>
>> +    default: 16
> maximum
> minimum?

Will add a enum, since it has to be a power of two.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property
  2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
  2023-02-06  7:32   ` Michal Simek
  2023-02-06  7:41   ` Krzysztof Kozlowski
@ 2023-02-06 16:58   ` Rob Herring
  2 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2023-02-06 16:58 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Wolfram Sang, Michal Simek, Shubhrajyoti Datta,
	Krzysztof Kozlowski, linux-i2c, devicetree

On Sun, Feb 05, 2023 at 03:02:08PM -0800, Lars-Peter Clausen wrote:
> The depth of the FIFO of the Cadence I2C controller IP is a synthesis
> configuration parameter. Different instances of the IP can have different
> values. For correct operation software needs to be aware of the size of the
> FIFO.
> 
> Add the documentation for the devicetree property that describes the FIFO
> depth of the IP core.
> 
> The default value of 16 is for backwards compatibility reasons with
> existing hardware descriptions where this property is not specified and
> software has assumed that the FIFO depth is 16.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> index 2e95cda7262a..3daa2fa73257 100644
> --- a/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> +++ b/Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
> @@ -38,6 +38,12 @@ properties:
>      description: |
>        Input clock name.
>  
> +  cdns,fifo-depth:

We already have:

fifo-size
rx-fifo-size
tx-fifo-size
fifo-depth
tx-fifo-depth
rx-fifo-depth

And we have cdns,fifo-depth too (among other vendor specific ones), but 
pick a non-vendor specific one.


> +    description:
> +      Size of the data FIFO in words.

What's the word size? Use bytes.

> +    $ref: "/schemas/types.yaml#/definitions/uint32"
> +    default: 16
> +
>  required:
>    - compatible
>    - reg
> @@ -57,4 +63,6 @@ examples:
>          clock-frequency = <400000>;
>          #address-cells = <1>;
>          #size-cells = <0>;
> +
> +        cdns,fifo-depth = <8>;
>      };
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length
  2023-02-06  8:02 ` [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Michal Simek
@ 2023-02-16 14:12   ` Lars-Peter Clausen
  2023-02-17  9:34     ` Michal Simek
  0 siblings, 1 reply; 9+ messages in thread
From: Lars-Peter Clausen @ 2023-02-16 14:12 UTC (permalink / raw)
  To: Michal Simek, Wolfram Sang
  Cc: Shubhrajyoti Datta, Rob Herring, Krzysztof Kozlowski, linux-i2c,
	devicetree

On 2/6/23 00:02, Michal Simek wrote:
>
>
> On 2/6/23 00:02, Lars-Peter Clausen wrote:
>> The FIFO depth and maximum transfer length are Synthesis configuration
>> parameters of the Cadence I2C IP. Different SoCs will use different 
>> values
>> for these parameters.
>>
>> Currently the driver has the FIFO depth hardcoded to 16 and the maximum
>> transfer length to 255. Trying to use the driver with an IP instance 
>> that
>> uses smaller values for these will work for short transfers. But longer
>> transfers will fail.
>>
>> The maximum transfer length can easily be detected at runtime since the
>> unused MSBs of the transfer length register are hardwired to 0. Writing
>> 0xff and then reading back the value will give the maximum transfer 
>> length.
>
> I think that these are pretty much two patches in one.
> The first should target transfer length part and second fifo depth part.
Hm, I'm not sure it is worth separating this out. These two are very 
tightly related, but I can split it.
>
>>   /**
>>    * cdns_i2c_probe - Platform registration call
>>    * @pdev:    Handle to the platform device structure
>> @@ -1242,6 +1274,7 @@ static int cdns_i2c_probe(struct 
>> platform_device *pdev)
>>       struct cdns_i2c *id;
>>       int ret, irq;
>>       const struct of_device_id *match;
>> +    u32 val;
>>         id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
>>       if (!id)
>> @@ -1317,6 +1350,12 @@ static int cdns_i2c_probe(struct 
>> platform_device *pdev)
>>   #endif
>>       id->ctrl_reg = CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | 
>> CDNS_I2C_CR_MS;
>>   +    val = CDNS_I2C_FIFO_DEPTH_DEFAULT;
>
> you can remove val completely.
> id->fifo_depth = CDNS_I2C_FIFO_DEPTH_DEFAULT;
But only if I make fifo_depth a u32.
> ...
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length
  2023-02-16 14:12   ` Lars-Peter Clausen
@ 2023-02-17  9:34     ` Michal Simek
  0 siblings, 0 replies; 9+ messages in thread
From: Michal Simek @ 2023-02-17  9:34 UTC (permalink / raw)
  To: Lars-Peter Clausen, Wolfram Sang
  Cc: Shubhrajyoti Datta, Rob Herring, Krzysztof Kozlowski, linux-i2c,
	devicetree



On 2/16/23 15:12, Lars-Peter Clausen wrote:
> On 2/6/23 00:02, Michal Simek wrote:
>>
>>
>> On 2/6/23 00:02, Lars-Peter Clausen wrote:
>>> The FIFO depth and maximum transfer length are Synthesis configuration
>>> parameters of the Cadence I2C IP. Different SoCs will use different values
>>> for these parameters.
>>>
>>> Currently the driver has the FIFO depth hardcoded to 16 and the maximum
>>> transfer length to 255. Trying to use the driver with an IP instance that
>>> uses smaller values for these will work for short transfers. But longer
>>> transfers will fail.
>>>
>>> The maximum transfer length can easily be detected at runtime since the
>>> unused MSBs of the transfer length register are hardwired to 0. Writing
>>> 0xff and then reading back the value will give the maximum transfer length.
>>
>> I think that these are pretty much two patches in one.
>> The first should target transfer length part and second fifo depth part.
> Hm, I'm not sure it is worth separating this out. These two are very tightly 
> related, but I can split it.

thanks

>>
>>>   /**
>>>    * cdns_i2c_probe - Platform registration call
>>>    * @pdev:    Handle to the platform device structure
>>> @@ -1242,6 +1274,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>>>       struct cdns_i2c *id;
>>>       int ret, irq;
>>>       const struct of_device_id *match;
>>> +    u32 val;
>>>         id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
>>>       if (!id)
>>> @@ -1317,6 +1350,12 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>>>   #endif
>>>       id->ctrl_reg = CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS;
>>>   +    val = CDNS_I2C_FIFO_DEPTH_DEFAULT;
>>
>> you can remove val completely.
>> id->fifo_depth = CDNS_I2C_FIFO_DEPTH_DEFAULT;
> But only if I make fifo_depth a u32.

No problem for me.

M

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-02-17  9:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-05 23:02 [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Lars-Peter Clausen
2023-02-05 23:02 ` [PATCH 2/2] dt-bindings: i2c: cadence: Document `cdns,fifo-depth` property Lars-Peter Clausen
2023-02-06  7:32   ` Michal Simek
2023-02-06  7:41   ` Krzysztof Kozlowski
2023-02-06 15:20     ` Lars-Peter Clausen
2023-02-06 16:58   ` Rob Herring
2023-02-06  8:02 ` [PATCH 1/2] i2c: cadence: Allow to specify the FIFO depth and maximum transfer length Michal Simek
2023-02-16 14:12   ` Lars-Peter Clausen
2023-02-17  9:34     ` Michal Simek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.