mbox series

[v1,0/2] riscv: spacemit: add i2c support to K1 SoC

Message ID 20241015075134.1449458-1-TroyMitchell988@gmail.com
Headers show
Series riscv: spacemit: add i2c support to K1 SoC | expand

Message

Troy Mitchell Oct. 15, 2024, 7:51 a.m. UTC 
Hi all,

This patch implements I2C driver for the SpacemiT K1 SoC,
providing basic support for I2C read/write communication which
compatible with standard I2C bus specifications.

In this version, the driver defaults to use fast-speed-mode and
interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.

The docs of I2C can be found here, in chapter 16.1 I2C [1]

Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]

Troy Mitchell (2):
  dt-bindings: i2c: spacemit: add support for K1 SoC
  i2c: spacemit: add support for SpacemiT K1 SoC

 .../bindings/i2c/spacemit,k1-i2c.yaml         |  59 ++
 drivers/i2c/busses/Kconfig                    |  18 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-k1.c                   | 694 ++++++++++++++++++
 4 files changed, 772 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-k1.c

Comments

Krzysztof Kozlowski Oct. 15, 2024, 8:02 a.m. UTC | #1 
On 15/10/2024 09:51, Troy Mitchell wrote:
> The i2c of K1 supports fast-speed-mode and high-speed-mode,

s/i2c/I2C/

> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> new file mode 100644
> index 000000000000..c1460ec2b323
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C controller embedded in SpacemiT's K1 SoC
> +
> +maintainers:
> +  - Troy Mitchell <troymitchell988@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-i2c

There is no such vendor prefix.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
> +      modes are supported by hardware, possible values are 100000 and 400000.
> +    enum: [100000, 400000]
> +    default: 100000
> +
> +  fifo-disable:

Why is this a property of a board?

Also, missing vendor prefix.


> +    type: boolean
> +    description:
> +      Whether to disable FIFO. If FIFO is turned on, it will be interrupted
> +      only when the FIFO depth is reached, which can reduce the frequency
> +      of interruption.
> +    default: false

Drop

> +
> +unevaluatedProperties: false

This goes after required: block.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +examples:
> +  - |
> +    i2c0: i2c@d4010800 {

Drop unused alias

> +        compatible = "spacemit,k1-i2c";

Best regards,
Krzysztof
Rob Herring (Arm) Oct. 15, 2024, 9:22 a.m. UTC | #2 
On Tue, 15 Oct 2024 15:51:33 +0800, Troy Mitchell wrote:
> The i2c of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.example.dtb: i2c@d4010800: reg: [[0, 3556837376], [0, 56]] is too long
	from schema $id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.example.dtb: i2c@d4010800: Unevaluated properties are not allowed ('reg' was unexpected)
	from schema $id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241015075134.1449458-2-TroyMitchell988@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Conor Dooley Oct. 15, 2024, 4:47 p.m. UTC | #3 
On Tue, Oct 15, 2024 at 10:02:21AM +0200, Krzysztof Kozlowski wrote:
> On 15/10/2024 09:51, Troy Mitchell wrote:
> > The i2c of K1 supports fast-speed-mode and high-speed-mode,
> 
> s/i2c/I2C/
> 
> > and supports FIFO transmission.
> > 
> > Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> > ---
> >  .../bindings/i2c/spacemit,k1-i2c.yaml         | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > new file mode 100644
> > index 000000000000..c1460ec2b323
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: I2C controller embedded in SpacemiT's K1 SoC
> > +
> > +maintainers:
> > +  - Troy Mitchell <troymitchell988@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: spacemit,k1-i2c
> 
> There is no such vendor prefix.

7cf3e9bfc63db ("dt-bindings: vendor-prefixes: add spacemit") will be in
tomorrow's next.
Troy Mitchell Oct. 16, 2024, 2:45 a.m. UTC | #4 
On 2024/10/15 16:02, Krzysztof Kozlowski wrote:
> On 15/10/2024 09:51, Troy Mitchell wrote:
>> The i2c of K1 supports fast-speed-mode and high-speed-mode,
> 
> s/i2c/I2C/
> 
>> and supports FIFO transmission.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> ---
>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 59 +++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> new file mode 100644
>> index 000000000000..c1460ec2b323
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> @@ -0,0 +1,59 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C controller embedded in SpacemiT's K1 SoC
>> +
>> +maintainers:
>> +  - Troy Mitchell <troymitchell988@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: spacemit,k1-i2c
> 
> There is no such vendor prefix.
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-frequency:
>> +    description:
>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>> +      modes are supported by hardware, possible values are 100000 and 400000.
>> +    enum: [100000, 400000]
>> +    default: 100000
>> +
>> +  fifo-disable:
> 
> Why is this a property of a board?
> 
> Also, missing vendor prefix.
> 
> 
>> +    type: boolean
>> +    description:
>> +      Whether to disable FIFO. If FIFO is turned on, it will be interrupted
>> +      only when the FIFO depth is reached, which can reduce the frequency
>> +      of interruption.
>> +    default: false
> 
> Drop

It's a hardware FIFO instead of software.
Is it unnecessary in this file?
If is, why dma can be written in dt-binding.

> 
>> +
>> +unevaluatedProperties: false
> 
> This goes after required: block.
> 
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +examples:
>> +  - |
>> +    i2c0: i2c@d4010800 {
> 
> Drop unused alias
> 
>> +        compatible = "spacemit,k1-i2c";
> 
> Best regards,
> Krzysztof
> 

Best regards,
Troy
Krzysztof Kozlowski Oct. 16, 2024, 7:44 a.m. UTC | #5 
On 16/10/2024 04:45, Troy Mitchell wrote:
>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>>> +      modes are supported by hardware, possible values are 100000 and 400000.
>>> +    enum: [100000, 400000]
>>> +    default: 100000
>>> +
>>> +  fifo-disable:
>>
>> Why is this a property of a board?


Here, this ^^^^^^


>>
>> Also, missing vendor prefix.
>>
>>
>>> +    type: boolean
>>> +    description:
>>> +      Whether to disable FIFO. If FIFO is turned on, it will be interrupted
>>> +      only when the FIFO depth is reached, which can reduce the frequency
>>> +      of interruption.
>>> +    default: false
>>
>> Drop
> 
> It's a hardware FIFO instead of software.
> Is it unnecessary in this file?
> If is, why dma can be written in dt-binding.

Because of what I asked earlier. Which 'dma' property are you asking
about? 'use-dma'? There was rationale provided in favor. I would be more
than happy to see similar rationale here.

Best regards,
Krzysztof