[PATCH v2 0/2] Add i2c clock manual tuning feature for aspeed-i2c driver

[PATCH v2 0/2] Add i2c clock manual tuning feature for aspeed-i2c driver

[Potin Lai]

Current aspeed-i2c driver could calculate a suited clock divisor and
high/low cycles automatically, and it try to assign high/low periods with
close number of cycles.

Because of board schematic design, sometimes we need to manual tune i2c
clock timing register to get longer high clock cycle to match hardware 
requirement, which is not supportted in current driver.

In this patch series, we add new properties for manually assigning clock
divisor, high clock cycles and low clock cycles.

LINK: [v1] https://lore.kernel.org/all/20220530114056.8722-1-potin.lai.pt@gmail.com/

changes v1 --> v2:
* update bt-bindings documentation
* use meaningful values for properties instead of acture value in register

Potin Lai (1):
  aspeed: i2c: add manual clock setting feature

Potin Lai (1):
  dt-bindings: aspeed-i2c: add properties for manual clock setting

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 ++++++++++++++
 drivers/i2c/busses/i2c-aspeed.c               | 57 ++++++++++++++++++-
 2 files changed, 100 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v2 0/2] Add i2c clock manual tuning feature for aspeed-i2c driver

[Potin Lai]

Current aspeed-i2c driver could calculate a suited clock divisor and
high/low cycles automatically, and it try to assign high/low periods with
close number of cycles.

Because of board schematic design, sometimes we need to manual tune i2c
clock timing register to get longer high clock cycle to match hardware 
requirement, which is not supportted in current driver.

In this patch series, we add new properties for manually assigning clock
divisor, high clock cycles and low clock cycles.

LINK: [v1] https://lore.kernel.org/all/20220530114056.8722-1-potin.lai.pt@gmail.com/

changes v1 --> v2:
* update bt-bindings documentation
* use meaningful values for properties instead of acture value in register

Potin Lai (1):
  aspeed: i2c: add manual clock setting feature

Potin Lai (1):
  dt-bindings: aspeed-i2c: add properties for manual clock setting

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 ++++++++++++++
 drivers/i2c/busses/i2c-aspeed.c               | 57 ++++++++++++++++++-
 2 files changed, 100 insertions(+), 1 deletion(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] aspeed: i2c: add manual clock setting feature

[Potin Lai]

Add manual tuning i2c clock timing register support by reading
following properties.

* aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
* aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
* aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
* aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 57 ++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 67e8b97c0c95..64424f377f27 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -60,6 +60,7 @@
 #define ASPEED_I2CD_TIME_SCL_LOW_MASK			GENMASK(15, 12)
 #define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK		GENMASK(3, 0)
 #define ASPEED_I2CD_TIME_SCL_REG_MAX			GENMASK(3, 0)
+#define ASPEED_I2CD_TIME_BASE_DIVISOR_MAX		32768
 /* 0x08 : I2CD Clock and AC Timing Control Register #2 */
 #define ASPEED_NO_TIMEOUT_CTRL				0
 
@@ -898,6 +899,57 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	return 0;
 }
 
+/* precondition: bus.lock has been acquired. */
+static int aspeed_i2c_manual_clk_setup(struct aspeed_i2c_bus *bus)
+{
+	u32 divisor, clk_high, clk_low, clk_reg_val;
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-base-clk-div",
+				     &divisor) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-base-clk-div\n");
+		return -EINVAL;
+	} else if (!divisor || divisor > ASPEED_I2CD_TIME_BASE_DIVISOR_MAX ||
+		   BIT(__fls(divisor)) != divisor) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-base-clk-div: %u\n",
+			divisor);
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-clk-high-cycle",
+				     &clk_high) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-clk-high-cycle\n");
+		return -EINVAL;
+	} else if ((clk_high-1) > ASPEED_I2CD_TIME_SCL_REG_MAX) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-clk-high-cycle: %u\n",
+			clk_high);
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-clk-low-cycle",
+				     &clk_low) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-clk-low-cycle\n");
+		return -EINVAL;
+	} else if ((clk_low-1) > ASPEED_I2CD_TIME_SCL_REG_MAX) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-clk-low-cycle: %u\n",
+			clk_low);
+		return -EINVAL;
+	}
+
+	clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
+			ASPEED_I2CD_TIME_THDSTA_MASK |
+			ASPEED_I2CD_TIME_TACST_MASK);
+	clk_reg_val |= (ilog2(divisor) & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK)
+			| (((clk_high-1) << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
+			| (((clk_low-1) << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_LOW_MASK);
+	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
+
+	return 0;
+}
+
 /* precondition: bus.lock has been acquired. */
 static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 			     struct platform_device *pdev)
@@ -908,7 +960,10 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 	/* Disable everything. */
 	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
 
-	ret = aspeed_i2c_init_clk(bus);
+	if (of_property_read_bool(pdev->dev.of_node, "aspeed,i2c-manual-clk"))
+		ret = aspeed_i2c_manual_clk_setup(bus);
+	else
+		ret = aspeed_i2c_init_clk(bus);
 	if (ret < 0)
 		return ret;
 
-- 
2.17.1

[PATCH v2 1/2] aspeed: i2c: add manual clock setting feature

[Potin Lai]

Add manual tuning i2c clock timing register support by reading
following properties.

* aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
* aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
* aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
* aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 57 ++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 67e8b97c0c95..64424f377f27 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -60,6 +60,7 @@
 #define ASPEED_I2CD_TIME_SCL_LOW_MASK			GENMASK(15, 12)
 #define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK		GENMASK(3, 0)
 #define ASPEED_I2CD_TIME_SCL_REG_MAX			GENMASK(3, 0)
+#define ASPEED_I2CD_TIME_BASE_DIVISOR_MAX		32768
 /* 0x08 : I2CD Clock and AC Timing Control Register #2 */
 #define ASPEED_NO_TIMEOUT_CTRL				0
 
@@ -898,6 +899,57 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	return 0;
 }
 
+/* precondition: bus.lock has been acquired. */
+static int aspeed_i2c_manual_clk_setup(struct aspeed_i2c_bus *bus)
+{
+	u32 divisor, clk_high, clk_low, clk_reg_val;
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-base-clk-div",
+				     &divisor) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-base-clk-div\n");
+		return -EINVAL;
+	} else if (!divisor || divisor > ASPEED_I2CD_TIME_BASE_DIVISOR_MAX ||
+		   BIT(__fls(divisor)) != divisor) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-base-clk-div: %u\n",
+			divisor);
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-clk-high-cycle",
+				     &clk_high) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-clk-high-cycle\n");
+		return -EINVAL;
+	} else if ((clk_high-1) > ASPEED_I2CD_TIME_SCL_REG_MAX) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-clk-high-cycle: %u\n",
+			clk_high);
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(bus->dev, "aspeed,i2c-clk-low-cycle",
+				     &clk_low) != 0) {
+		dev_err(bus->dev, "Could not read aspeed,i2c-clk-low-cycle\n");
+		return -EINVAL;
+	} else if ((clk_low-1) > ASPEED_I2CD_TIME_SCL_REG_MAX) {
+		dev_err(bus->dev, "Invalid aspeed,i2c-clk-low-cycle: %u\n",
+			clk_low);
+		return -EINVAL;
+	}
+
+	clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
+			ASPEED_I2CD_TIME_THDSTA_MASK |
+			ASPEED_I2CD_TIME_TACST_MASK);
+	clk_reg_val |= (ilog2(divisor) & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK)
+			| (((clk_high-1) << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
+			| (((clk_low-1) << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_LOW_MASK);
+	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
+
+	return 0;
+}
+
 /* precondition: bus.lock has been acquired. */
 static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 			     struct platform_device *pdev)
@@ -908,7 +960,10 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
 	/* Disable everything. */
 	writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG);
 
-	ret = aspeed_i2c_init_clk(bus);
+	if (of_property_read_bool(pdev->dev.of_node, "aspeed,i2c-manual-clk"))
+		ret = aspeed_i2c_manual_clk_setup(bus);
+	else
+		ret = aspeed_i2c_init_clk(bus);
 	if (ret < 0)
 		return ret;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Potin Lai]

Add following properties for manual tuning clock divisor and cycle of
hign/low pulse witdh.

* aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
* aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
* aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
* aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index ea643e6c3ef5..e2f67fe2aa0c 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -12,6 +12,28 @@ maintainers:
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
 
+  - if:
+      properties:
+        compatible:
+          const: st,stm32-uart
+
+    then:
+      properties:
+        aspeed,i2c-clk-high-cycle:
+          maximum: 8
+        aspeed,i2c-clk-low-cycle:
+          maximum: 8
+
+  - if:
+      required:
+        - aspeed,i2c-manual-clk
+
+    then:
+      required:
+        - aspeed,i2c-base-clk-div
+        - aspeed,i2c-clk-high-cycle
+        - aspeed,i2c-clk-low-cycle
+
 properties:
   compatible:
     enum:
@@ -49,6 +71,28 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,i2c-manual-clk:
+    type: boolean
+    description: enable manual clock setting
+
+  aspeed,i2c-base-clk-div:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
+           16384, 32768]
+    description: base clock divisor
+
+  aspeed,i2c-clk-high-cycle:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 16
+    description: cycles of master clock-high pulse width
+
+  aspeed,i2c-clk-low-cycle:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 16
+    description: cycles of master clock-low pulse width
+
 required:
   - reg
   - compatible
-- 
2.17.1

[PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Potin Lai]

Add following properties for manual tuning clock divisor and cycle of
hign/low pulse witdh.

* aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
* aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
* aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
* aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index ea643e6c3ef5..e2f67fe2aa0c 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -12,6 +12,28 @@ maintainers:
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
 
+  - if:
+      properties:
+        compatible:
+          const: st,stm32-uart
+
+    then:
+      properties:
+        aspeed,i2c-clk-high-cycle:
+          maximum: 8
+        aspeed,i2c-clk-low-cycle:
+          maximum: 8
+
+  - if:
+      required:
+        - aspeed,i2c-manual-clk
+
+    then:
+      required:
+        - aspeed,i2c-base-clk-div
+        - aspeed,i2c-clk-high-cycle
+        - aspeed,i2c-clk-low-cycle
+
 properties:
   compatible:
     enum:
@@ -49,6 +71,28 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,i2c-manual-clk:
+    type: boolean
+    description: enable manual clock setting
+
+  aspeed,i2c-base-clk-div:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
+           16384, 32768]
+    description: base clock divisor
+
+  aspeed,i2c-clk-high-cycle:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 16
+    description: cycles of master clock-high pulse width
+
+  aspeed,i2c-clk-low-cycle:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 16
+    description: cycles of master clock-low pulse width
+
 required:
   - reg
   - compatible
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Rob Herring]

On Wed, Jun 01, 2022 at 12:15:12PM +0800, Potin Lai wrote:
> Add following properties for manual tuning clock divisor and cycle of
> hign/low pulse witdh.
> 
> * aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
> * aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
> * aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
> * aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)
> 
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index ea643e6c3ef5..e2f67fe2aa0c 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -12,6 +12,28 @@ maintainers:
>  allOf:
>    - $ref: /schemas/i2c/i2c-controller.yaml#
>  
> +  - if:
> +      properties:
> +        compatible:
> +          const: st,stm32-uart

stm32 uart?

> +
> +    then:
> +      properties:
> +        aspeed,i2c-clk-high-cycle:
> +          maximum: 8
> +        aspeed,i2c-clk-low-cycle:
> +          maximum: 8
> +
> +  - if:
> +      required:
> +        - aspeed,i2c-manual-clk
> +
> +    then:
> +      required:
> +        - aspeed,i2c-base-clk-div
> +        - aspeed,i2c-clk-high-cycle
> +        - aspeed,i2c-clk-low-cycle

'dependencies' can better express this than an if/then.

However, I think this should all be done in a common way.

> +
>  properties:
>    compatible:
>      enum:
> @@ -49,6 +71,28 @@ properties:
>      description:
>        states that there is another master active on this bus
>  
> +  aspeed,i2c-manual-clk:
> +    type: boolean
> +    description: enable manual clock setting

No need for this as presence of the other properties can determine this.

> +
> +  aspeed,i2c-base-clk-div:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
> +           16384, 32768]
> +    description: base clock divisor

Specify the i2c bus frequency and calculate the divider.

> +
> +  aspeed,i2c-clk-high-cycle:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 16
> +    description: cycles of master clock-high pulse width
> +
> +  aspeed,i2c-clk-low-cycle:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 16
> +    description: cycles of master clock-low pulse width

These 2 should be common. I think you just need a single property 
expressing duty cycle.

Rob

Re: [PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Rob Herring]

On Wed, Jun 01, 2022 at 12:15:12PM +0800, Potin Lai wrote:
> Add following properties for manual tuning clock divisor and cycle of
> hign/low pulse witdh.
> 
> * aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
> * aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
> * aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
> * aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)
> 
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index ea643e6c3ef5..e2f67fe2aa0c 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -12,6 +12,28 @@ maintainers:
>  allOf:
>    - $ref: /schemas/i2c/i2c-controller.yaml#
>  
> +  - if:
> +      properties:
> +        compatible:
> +          const: st,stm32-uart

stm32 uart?

> +
> +    then:
> +      properties:
> +        aspeed,i2c-clk-high-cycle:
> +          maximum: 8
> +        aspeed,i2c-clk-low-cycle:
> +          maximum: 8
> +
> +  - if:
> +      required:
> +        - aspeed,i2c-manual-clk
> +
> +    then:
> +      required:
> +        - aspeed,i2c-base-clk-div
> +        - aspeed,i2c-clk-high-cycle
> +        - aspeed,i2c-clk-low-cycle

'dependencies' can better express this than an if/then.

However, I think this should all be done in a common way.

> +
>  properties:
>    compatible:
>      enum:
> @@ -49,6 +71,28 @@ properties:
>      description:
>        states that there is another master active on this bus
>  
> +  aspeed,i2c-manual-clk:
> +    type: boolean
> +    description: enable manual clock setting

No need for this as presence of the other properties can determine this.

> +
> +  aspeed,i2c-base-clk-div:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
> +           16384, 32768]
> +    description: base clock divisor

Specify the i2c bus frequency and calculate the divider.

> +
> +  aspeed,i2c-clk-high-cycle:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 16
> +    description: cycles of master clock-high pulse width
> +
> +  aspeed,i2c-clk-low-cycle:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 16
> +    description: cycles of master clock-low pulse width

These 2 should be common. I think you just need a single property 
expressing duty cycle.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Potin Lai]

Rob Herring 於 2022/6/6 上午 05:47 寫道:
> On Wed, Jun 01, 2022 at 12:15:12PM +0800, Potin Lai wrote:
>> Add following properties for manual tuning clock divisor and cycle of
>> hign/low pulse witdh.
>>
>> * aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
>> * aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
>> * aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
>> * aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)
>>
>> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
>> ---
>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> index ea643e6c3ef5..e2f67fe2aa0c 100644
>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> @@ -12,6 +12,28 @@ maintainers:
>>  allOf:
>>    - $ref: /schemas/i2c/i2c-controller.yaml#
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          const: st,stm32-uart
> stm32 uart?
Sorry, I will remove it.
>> +
>> +    then:
>> +      properties:
>> +        aspeed,i2c-clk-high-cycle:
>> +          maximum: 8
>> +        aspeed,i2c-clk-low-cycle:
>> +          maximum: 8
>> +
>> +  - if:
>> +      required:
>> +        - aspeed,i2c-manual-clk
>> +
>> +    then:
>> +      required:
>> +        - aspeed,i2c-base-clk-div
>> +        - aspeed,i2c-clk-high-cycle
>> +        - aspeed,i2c-clk-low-cycle
> 'dependencies' can better express this than an if/then.
>
> However, I think this should all be done in a common way.
>
>> +
>>  properties:
>>    compatible:
>>      enum:
>> @@ -49,6 +71,28 @@ properties:
>>      description:
>>        states that there is another master active on this bus
>>  
>> +  aspeed,i2c-manual-clk:
>> +    type: boolean
>> +    description: enable manual clock setting
> No need for this as presence of the other properties can determine this.
>
>> +
>> +  aspeed,i2c-base-clk-div:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
>> +           16384, 32768]
>> +    description: base clock divisor
> Specify the i2c bus frequency and calculate the divider.
>
>> +
>> +  aspeed,i2c-clk-high-cycle:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 16
>> +    description: cycles of master clock-high pulse width
>> +
>> +  aspeed,i2c-clk-low-cycle:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 16
>> +    description: cycles of master clock-low pulse width
> These 2 should be common. I think you just need a single property 
> expressing duty cycle.
>
> Rob

Got it, thank you for the suggestion of duty cycle.
I will use duty cycle for next version.

Potin

Re: [PATCH v2 2/2] dt-bindings: aspeed-i2c: add properties for manual clock setting

[Potin Lai]

Rob Herring 於 2022/6/6 上午 05:47 寫道:
> On Wed, Jun 01, 2022 at 12:15:12PM +0800, Potin Lai wrote:
>> Add following properties for manual tuning clock divisor and cycle of
>> hign/low pulse witdh.
>>
>> * aspeed,i2c-manual-clk: Enable aspeed i2c clock manual setting
>> * aspeed,i2c-base-clk-div: Base Clock divisor (tBaseClk)
>> * aspeed,i2c-clk-high-cycle: Cycles of clock-high pulse (tClkHigh)
>> * aspeed,i2c-clk-low-cycle: Cycles of clock-low pulse (tClkLow)
>>
>> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
>> ---
>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> index ea643e6c3ef5..e2f67fe2aa0c 100644
>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>> @@ -12,6 +12,28 @@ maintainers:
>>  allOf:
>>    - $ref: /schemas/i2c/i2c-controller.yaml#
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          const: st,stm32-uart
> stm32 uart?
Sorry, I will remove it.
>> +
>> +    then:
>> +      properties:
>> +        aspeed,i2c-clk-high-cycle:
>> +          maximum: 8
>> +        aspeed,i2c-clk-low-cycle:
>> +          maximum: 8
>> +
>> +  - if:
>> +      required:
>> +        - aspeed,i2c-manual-clk
>> +
>> +    then:
>> +      required:
>> +        - aspeed,i2c-base-clk-div
>> +        - aspeed,i2c-clk-high-cycle
>> +        - aspeed,i2c-clk-low-cycle
> 'dependencies' can better express this than an if/then.
>
> However, I think this should all be done in a common way.
>
>> +
>>  properties:
>>    compatible:
>>      enum:
>> @@ -49,6 +71,28 @@ properties:
>>      description:
>>        states that there is another master active on this bus
>>  
>> +  aspeed,i2c-manual-clk:
>> +    type: boolean
>> +    description: enable manual clock setting
> No need for this as presence of the other properties can determine this.
>
>> +
>> +  aspeed,i2c-base-clk-div:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192,
>> +           16384, 32768]
>> +    description: base clock divisor
> Specify the i2c bus frequency and calculate the divider.
>
>> +
>> +  aspeed,i2c-clk-high-cycle:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 16
>> +    description: cycles of master clock-high pulse width
>> +
>> +  aspeed,i2c-clk-low-cycle:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 16
>> +    description: cycles of master clock-low pulse width
> These 2 should be common. I think you just need a single property 
> expressing duty cycle.
>
> Rob

Got it, thank you for the suggestion of duty cycle.
I will use duty cycle for next version.

Potin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel