* [PATCH RESEND v5 0/2] riscv: spacemit: add i2c support to K1 SoC
@ 2025-03-03 5:30 Troy Mitchell
2025-03-03 5:30 ` [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell
2025-03-03 5:30 ` [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell
0 siblings, 2 replies; 14+ messages in thread
From: Troy Mitchell @ 2025-03-03 5:30 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Yixun Lan, Troy Mitchell
Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit,
Conor Dooley
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]
---
I'm sorry for forgetting to CC Yixun Lan and SpacemiT, and for not syncing the Kconfig.
This re-send also corrects the warning mentioned in the CI test.
That's why I'm resending this.
---
Change in v5:
- Path #1:
- Add `clock-names` property
- Modify the clock property into two
- Path #2:
- Enable the APB clock
- Fix comment and code styles
- Fix typo and drop unnecessary description in Kconfig
- Prefix all macro definitions with SPACEMIT_
- Rename `spacemit_i2c_bus_reset` to `spacemit_i2c_conditionally_reset_bus`
- Remove all `unlikely` and `likely`
- Remove unused register and bit macros
- Remove the "err" field, as it only contains a subset of the status field
- Retrieve `clock-frequency` from the device tree instead of using a macro
- Use a local variable to track the current message
- Use `i2c->read` to represent read and write statuses instead of `i2c->dir`
Link to v4:
https://lore.kernel.org/all/20241125-k1-i2c-master-v4-0-0f3d5886336b@gmail.com/
Change in v4:
- Patch #1:
- Change the default value of clock-frequency from 100000 to
400000. This is to correspond to the driver's default value.
- Drop the minimum of clock-frequency
- Modify the description of clock-frequency
- Patch #2:
- Drop the `inline` qualifier from the `spacemit_i2c_xfer_core` function
- Drop the initialization of `ret` to 0 in `spacemit_i2c_xfer_core` function
- Drop useless wrap
Link to v3:
https://lore.kernel.org/all/20241112-k1-i2c-master-v3-0-5005b70dc208@gmail.com/
Change in v3:
- Patch #1:
- Change the maxItems of reg from 2 to 1 in properties
- Modify reg in dts example
- Changed the enum selection for clock-frequency to a range,
setting a minimum value of 1 and a maximum value of 3,300,000.
- Patch #2:
- Drop unused judgement in `spacemit_i2c_xfer_msg`
- Fix the dangling else warning in `spacemit_i2c_is_last_msg`
- Fix the error check for `i2c->base`
- Modify Kconfig dependencies
Link to v2:
https://lore.kernel.org/all/20241028053220.346283-1-TroyMitchell988@gmail.com/
Change in v2:
- Patch #1:
- Change the maxItems of reg from 1 to 2 in properties
- Change 'i2c' to 'I2C' in the commit message.
- Drop fifo-disable property
- Drop alias in dts example
- Move `unevaluatedProperties` after `required:` block
- Patch #2:
- Alphabetize Makefile and Kconfig
- Change `.remove_new` to `.remove` in `struct platform_driver`
- Change `dev_alert` to `dev_warn_ratelimited` in `spacemit_i2c_bus_reset`
- Change `spacemit_i2c_read/write_reg` to `read/writel`
- Change `spacemit_i2c_dt_match` to `spacemit_i2c_of_match`
- Clean up code flow
- Fix unnecessary line wraps
- Move `spacemit_i2c_handle_err` to a suitable location
- Modify Kconfig dependencies
- Use `PTR_ERR(i2c->base)` directly as the `dev_err_probe` parameter instead of
the intermediate variable
Link to v1:
https://lore.kernel.org/all/20241015075134.1449458-1-TroyMitchell988@gmail.com/
---
Troy Mitchell (2):
dt-bindings: i2c: spacemit: add support for K1 SoC
i2c: spacemit: add support for SpacemiT K1 SoC
.../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 59 ++
drivers/i2c/busses/Kconfig | 17 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-k1.c | 617 +++++++++++++++++++++
4 files changed, 694 insertions(+)
---
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
change-id: 20241031-k1-i2c-master-fe7f7b0dce93
prerequisite-change-id: 20240626-k1-01-basic-dt-1aa31eeebcd2:v5
prerequisite-patch-id: 47dcf6861f7d434d25855b379e6d7ef4ce369c9c
prerequisite-patch-id: 77787fe82911923aff15ccf565e8fa451538c3a6
prerequisite-patch-id: b0bdb1742d96c5738f05262c3b0059102761390b
prerequisite-patch-id: 3927d39d8d77e35d5bfe53d9950da574ff8f2054
prerequisite-patch-id: a98039136a4796252a6029e474f03906f2541643
prerequisite-patch-id: c95f6dc0547a2a63a76e3cba0cf5c623b212b4e6
prerequisite-patch-id: 66e750e438ee959ddc2a6f0650814a2d8c989139
prerequisite-patch-id: 29a0fd8c36c1a4340f0d0b68a4c34d2b8abfb1ab
prerequisite-patch-id: 0bdfff661c33c380d1cf00a6c68688e05f88c0b3
prerequisite-patch-id: 99f15718e0bfbb7ed1a96dfa19f35841b004dae9
Best regards,
--
Troy Mitchell <troymitchell988@gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-03 5:30 [PATCH RESEND v5 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell @ 2025-03-03 5:30 ` Troy Mitchell 2025-03-03 9:35 ` Yixun Lan 2025-03-03 5:30 ` [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell 1 sibling, 1 reply; 14+ messages in thread From: Troy Mitchell @ 2025-03-03 5:30 UTC (permalink / raw) To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Troy Mitchell Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley The I2C of K1 supports fast-speed-mode and high-speed-mode, and supports FIFO transmission. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> --- .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 59 ++++++++++++++++++++++ 1 file changed, 59 insertions(+) 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3 --- /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 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + minItems: 2 + maxItems: 2 + + clock-names: + minItems: 2 + maxItems: 2 + + clock-frequency: + description: | + K1 support three different modes which running different frequencies + standard speed mode: up to 100000 (100Hz) + fast speed mode : up to 400000 (400Hz) + high speed mode : up to 3300000 (3.3Mhz) + default: 400000 + maximum: 3300000 + +required: + - compatible + - reg + - interrupts + - clocks + +unevaluatedProperties: false + +examples: + - | + i2c@d4010800 { + compatible = "spacemit,k1-i2c"; + reg = <0xd4010800 0x38>; + interrupt-parent = <&plic>; + interrupts = <36>; + clocks = <&ccu 176>, <&ccu 90>; + clock-names = "apb", "twsi"; + clock-frequency = <100000>; + }; + +... -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-03 5:30 ` [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell @ 2025-03-03 9:35 ` Yixun Lan 2025-03-05 2:11 ` Samuel Holland 0 siblings, 1 reply; 14+ messages in thread From: Yixun Lan @ 2025-03-03 9:35 UTC (permalink / raw) To: Troy Mitchell Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley On 13:30 Mon 03 Mar , Troy Mitchell wrote: > The I2C of K1 supports fast-speed-mode and high-speed-mode, > and supports FIFO transmission. > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> > --- > .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 59 ++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3 > --- /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 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + .. > + clocks: > + minItems: 2 > + maxItems: 2 > + > + clock-names: > + minItems: 2 > + maxItems: 2 I'd suggest to give a brief description and explicit clock name here, you can consult marvell,mv64xxx-i2c.yaml for example > + > + clock-frequency: > + description: | > + K1 support three different modes which running different frequencies > + standard speed mode: up to 100000 (100Hz) > + fast speed mode : up to 400000 (400Hz) > + high speed mode : up to 3300000 (3.3Mhz) > + default: 400000 > + maximum: 3300000 > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +unevaluatedProperties: false > + > +examples: > + - | > + i2c@d4010800 { > + compatible = "spacemit,k1-i2c"; > + reg = <0xd4010800 0x38>; > + interrupt-parent = <&plic>; > + interrupts = <36>; > + clocks = <&ccu 176>, <&ccu 90>; > + clock-names = "apb", "twsi"; 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 from above docs, there are two clocks bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable I'd suggest to name it according to the functionality, thus 'func', 'bus' clock, not its source.. which would make it more system wide consistent > + clock-frequency = <100000>; > + }; > + > +... > > -- > 2.34.1 > -- Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-03 9:35 ` Yixun Lan @ 2025-03-05 2:11 ` Samuel Holland 2025-03-05 3:05 ` Yixun Lan 0 siblings, 1 reply; 14+ messages in thread From: Samuel Holland @ 2025-03-05 2:11 UTC (permalink / raw) To: Yixun Lan, Troy Mitchell Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley Hi Troy, On 2025-03-03 3:35 AM, Yixun Lan wrote: > On 13:30 Mon 03 Mar , Troy Mitchell wrote: >> The I2C of K1 supports fast-speed-mode and high-speed-mode, >> and supports FIFO transmission. >> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> >> --- >> .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 59 ++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3 >> --- /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 >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + > .. >> + clocks: >> + minItems: 2 >> + maxItems: 2 >> + >> + clock-names: >> + minItems: 2 >> + maxItems: 2 > I'd suggest to give a brief description and explicit clock name here, > you can consult marvell,mv64xxx-i2c.yaml for example > >> + >> + clock-frequency: >> + description: | >> + K1 support three different modes which running different frequencies >> + standard speed mode: up to 100000 (100Hz) >> + fast speed mode : up to 400000 (400Hz) >> + high speed mode : up to 3300000 (3.3Mhz) >> + default: 400000 >> + maximum: 3300000 >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + i2c@d4010800 { >> + compatible = "spacemit,k1-i2c"; >> + reg = <0xd4010800 0x38>; >> + interrupt-parent = <&plic>; >> + interrupts = <36>; >> + clocks = <&ccu 176>, <&ccu 90>; >> + clock-names = "apb", "twsi"; > 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) > https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 > from above docs, there are two clocks > bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable > bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable > > I'd suggest to name it according to the functionality, thus 'func', 'bus' > clock, not its source.. which would make it more system wide consistent Also in that same register is: 2 RST RW 0x1 TWSI0 Reset Generation This field resets both the APB and functional domain. - 0: No Reset - 1: Reset Which means you need a 'resets' property in the binding as well. Regards, Samuel >> + clock-frequency = <100000>; >> + }; >> + >> +... >> >> -- >> 2.34.1 >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-05 2:11 ` Samuel Holland @ 2025-03-05 3:05 ` Yixun Lan 2025-03-05 4:01 ` Samuel Holland 0 siblings, 1 reply; 14+ messages in thread From: Yixun Lan @ 2025-03-05 3:05 UTC (permalink / raw) To: Samuel Holland Cc: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley Hi Samuel Holland: On 20:11 Tue 04 Mar , Samuel Holland wrote: > Hi Troy, > > On 2025-03-03 3:35 AM, Yixun Lan wrote: > > On 13:30 Mon 03 Mar , Troy Mitchell wrote: > >> The I2C of K1 supports fast-speed-mode and high-speed-mode, > >> and supports FIFO transmission. > >> > >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> > >> --- > >> .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 59 ++++++++++++++++++++++ > >> 1 file changed, 59 insertions(+) > >> > >> 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3 > >> --- /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 > >> + > >> + reg: > >> + maxItems: 1 > >> + > >> + interrupts: > >> + maxItems: 1 > >> + > > .. > >> + clocks: > >> + minItems: 2 > >> + maxItems: 2 > >> + > >> + clock-names: > >> + minItems: 2 > >> + maxItems: 2 > > I'd suggest to give a brief description and explicit clock name here, > > you can consult marvell,mv64xxx-i2c.yaml for example > > > >> + > >> + clock-frequency: > >> + description: | > >> + K1 support three different modes which running different frequencies > >> + standard speed mode: up to 100000 (100Hz) > >> + fast speed mode : up to 400000 (400Hz) > >> + high speed mode : up to 3300000 (3.3Mhz) > >> + default: 400000 > >> + maximum: 3300000 > >> + > >> +required: > >> + - compatible > >> + - reg > >> + - interrupts > >> + - clocks > >> + > >> +unevaluatedProperties: false > >> + > >> +examples: > >> + - | > >> + i2c@d4010800 { > >> + compatible = "spacemit,k1-i2c"; > >> + reg = <0xd4010800 0x38>; > >> + interrupt-parent = <&plic>; > >> + interrupts = <36>; > >> + clocks = <&ccu 176>, <&ccu 90>; > >> + clock-names = "apb", "twsi"; > > 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) > > https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 > > from above docs, there are two clocks > > bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable > > bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable > > > > I'd suggest to name it according to the functionality, thus 'func', 'bus' > > clock, not its source.. which would make it more system wide consistent > > Also in that same register is: > > 2 RST RW 0x1 TWSI0 Reset Generation > This field resets both the APB and functional domain. > - 0: No Reset > - 1: Reset > > Which means you need a 'resets' property in the binding as well. > right, there is reset needed I'd suggest to add it as an incremental patch later, when we implement real reset driver, and also complete the calling reset consumer API in i2c driver but, let me know if this is not the right way to go > Regards, > Samuel > > >> + clock-frequency = <100000>; > >> + }; > >> + > >> +... > >> > >> -- > >> 2.34.1 > >> > > > > -- Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-05 3:05 ` Yixun Lan @ 2025-03-05 4:01 ` Samuel Holland 2025-03-05 14:10 ` Yixun Lan 0 siblings, 1 reply; 14+ messages in thread From: Samuel Holland @ 2025-03-05 4:01 UTC (permalink / raw) To: Yixun Lan Cc: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley On 2025-03-04 9:05 PM, Yixun Lan wrote: >>>> + clocks = <&ccu 176>, <&ccu 90>; >>>> + clock-names = "apb", "twsi"; >>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) >>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 >>> from above docs, there are two clocks >>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable >>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable >>> >>> I'd suggest to name it according to the functionality, thus 'func', 'bus' >>> clock, not its source.. which would make it more system wide consistent >> >> Also in that same register is: >> >> 2 RST RW 0x1 TWSI0 Reset Generation >> This field resets both the APB and functional domain. >> - 0: No Reset >> - 1: Reset >> >> Which means you need a 'resets' property in the binding as well. >> > right, there is reset needed > > I'd suggest to add it as an incremental patch later, when we > implement real reset driver, and also complete the calling reset > consumer API in i2c driver > > but, let me know if this is not the right way to go If you add the resets property later, that's a breaking change to the DT, because existing devicetrees will not have that property. So you would have to make the reset consumer in the driver optional, even if it's not really optional, to work with older DTs. So it is _possible_ to add incrementally, but not recommended because it adds "legacy" code that never really goes away. It's okay to define the binding as requiring the resets property now, even before the reset controller driver is merged. You just won't be able to add the I2C controller to the DTS until the reset controller binding is merged. But since the reset controller is the same IP block as the clock controller, its binding should be available soon anyway. Regards, Samuel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC 2025-03-05 4:01 ` Samuel Holland @ 2025-03-05 14:10 ` Yixun Lan 0 siblings, 0 replies; 14+ messages in thread From: Yixun Lan @ 2025-03-05 14:10 UTC (permalink / raw) To: Samuel Holland Cc: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit, Conor Dooley Hi Samuel: On 22:01 Tue 04 Mar , Samuel Holland wrote: > On 2025-03-04 9:05 PM, Yixun Lan wrote: > >>>> + clocks = <&ccu 176>, <&ccu 90>; > >>>> + clock-names = "apb", "twsi"; > >>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST) > >>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594 > >>> from above docs, there are two clocks > >>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable > >>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable > >>> > >>> I'd suggest to name it according to the functionality, thus 'func', 'bus' > >>> clock, not its source.. which would make it more system wide consistent > >> > >> Also in that same register is: > >> > >> 2 RST RW 0x1 TWSI0 Reset Generation > >> This field resets both the APB and functional domain. > >> - 0: No Reset > >> - 1: Reset > >> > >> Which means you need a 'resets' property in the binding as well. > >> > > right, there is reset needed > > > > I'd suggest to add it as an incremental patch later, when we > > implement real reset driver, and also complete the calling reset > > consumer API in i2c driver > > > > but, let me know if this is not the right way to go > > If you add the resets property later, that's a breaking change to the DT, > because existing devicetrees will not have that property. So you would have to > make the reset consumer in the driver optional, even if it's not really > optional, to work with older DTs. So it is _possible_ to add incrementally, but > not recommended because it adds "legacy" code that never really goes away. > Ok, that's fair if we want to keep DT backward compatible.. > It's okay to define the binding as requiring the resets property now, even > before the reset controller driver is merged. You just won't be able to add the > I2C controller to the DTS until the reset controller binding is merged. But > since the reset controller is the same IP block as the clock controller, its > binding should be available soon anyway. > yes, this should work, thanks I think we will wait for reset driver -- Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-03 5:30 [PATCH RESEND v5 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell 2025-03-03 5:30 ` [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell @ 2025-03-03 5:30 ` Troy Mitchell 2025-03-03 6:08 ` Wolfram Sang 2025-03-04 0:01 ` Alex Elder 1 sibling, 2 replies; 14+ messages in thread From: Troy Mitchell @ 2025-03-03 5:30 UTC (permalink / raw) To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Troy Mitchell Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit This patch introduces basic I2C support for the SpacemiT K1 SoC, utilizing interrupts for transfers. The driver has been tested using i2c-tools on a Bananapi-F3 board, and basic I2C read/write operations have been confirmed to work. Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> --- drivers/i2c/busses/Kconfig | 17 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-k1.c | 617 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 635 insertions(+) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -783,6 +783,23 @@ config I2C_JZ4780 If you don't know what to do here, say N. +config I2C_K1 + tristate "Spacemit K1 I2C adapter" + depends on ARCH_SPACEMIT || COMPILE_TEST + depends on OF + help + This option enables support for the I2C interface on the Spacemit K1 + platform. + + If you enable this configuration, the kernel will include support for + the I2C adapter specific to the Spacemit K1 platform. This driver can + be used to manage I2C bus transactions, which are necessary for + interfacing with I2C peripherals such as sensors, EEPROMs, and other + devices. + + This driver can also be built as a module. If so, the + module will be called `i2c-k1`. + config I2C_KEBA tristate "KEBA I2C controller support" depends on HAS_IOMEM diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_I2C_IMX) += i2c-imx.o obj-$(CONFIG_I2C_IMX_LPI2C) += i2c-imx-lpi2c.o obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o +obj-$(CONFIG_I2C_K1) += i2c-k1.o obj-$(CONFIG_I2C_KEBA) += i2c-keba.o obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c new file mode 100644 index 0000000000000000000000000000000000000000..6abe05640782dfa93a15d130c58ac879a423e061 --- /dev/null +++ b/drivers/i2c/busses/i2c-k1.c @@ -0,0 +1,617 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com> + */ + + #include <linux/clk.h> + #include <linux/i2c.h> + #include <linux/iopoll.h> + #include <linux/module.h> + #include <linux/of_address.h> + #include <linux/platform_device.h> + +/* spacemit i2c registers */ +#define SPACEMIT_ICR 0x0 /* Control Register */ +#define SPACEMIT_ISR 0x4 /* Status Register */ +#define SPACEMIT_IDBR 0xc /* Data Buffer Register */ +#define SPACEMIT_IBMR 0x1c /* Bus monitor register */ + +/* register SPACEMIT_ICR fields */ +#define SPACEMIT_CR_START BIT(0) /* start bit */ +#define SPACEMIT_CR_STOP BIT(1) /* stop bit */ +#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */ +#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */ +/* Bit 4-7 are reserved */ +#define SPACEMIT_CR_MODE_FAST BIT(8) /* bus mode (master operation) */ +/* Bit 9 is reserved */ +#define SPACEMIT_CR_UR BIT(10) /* unit reset */ +/* Bit 11-12 are reserved */ +#define SPACEMIT_CR_SCLE BIT(13) /* master clock enable */ +#define SPACEMIT_CR_IUE BIT(14) /* unit enable */ +/* Bit 15-17 are reserved */ +#define SPACEMIT_CR_ALDIE BIT(18) /* enable arbitration interrupt */ +#define SPACEMIT_CR_DTEIE BIT(19) /* enable tx interrupts */ +#define SPACEMIT_CR_DRFIE BIT(20) /* enable rx interrupts */ +#define SPACEMIT_CR_GCD BIT(21) /* general call disable */ +#define SPACEMIT_CR_BEIE BIT(22) /* enable bus error ints */ +/* Bit 23-24 are reserved */ +#define SPACEMIT_CR_MSDIE BIT(25) /* master STOP detected int enable */ +#define SPACEMIT_CR_MSDE BIT(26) /* master STOP detected enable */ +#define SPACEMIT_CR_TXDONEIE BIT(27) /* transaction done int enable */ +#define SPACEMIT_CR_TXEIE BIT(28) /* transmit FIFO empty int enable */ +#define SPACEMIT_CR_RXHFIE BIT(29) /* receive FIFO half-full int enable */ +#define SPACEMIT_CR_RXFIE BIT(30) /* receive FIFO full int enable */ +#define SPACEMIT_CR_RXOVIE BIT(31) /* receive FIFO overrun int enable */ + +#define SPACEMIT_I2C_INT_CTRL_MASK (SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \ + SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \ + SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \ + SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \ + SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE) + +/* register SPACEMIT_ISR fields */ +#define SPACEMIT_SR_ACKNAK BIT(14) /* ACK/NACK status */ +#define SPACEMIT_SR_UB BIT(15) /* unit busy */ +#define SPACEMIT_SR_IBB BIT(16) /* i2c bus busy */ +#define SPACEMIT_SR_EBB BIT(17) /* early bus busy */ +#define SPACEMIT_SR_ALD BIT(18) /* arbitration loss detected */ +#define SPACEMIT_SR_ITE BIT(19) /* tx buffer empty */ +#define SPACEMIT_SR_IRF BIT(20) /* rx buffer full */ +#define SPACEMIT_SR_GCAD BIT(21) /* general call address detected */ +#define SPACEMIT_SR_BED BIT(22) /* bus error no ACK/NAK */ +#define SPACEMIT_SR_SAD BIT(23) /* slave address detected */ +#define SPACEMIT_SR_SSD BIT(24) /* slave stop detected */ +/* Bit 25 is reserved */ +#define SPACEMIT_SR_MSD BIT(26) /* master stop detected */ +#define SPACEMIT_SR_TXDONE BIT(27) /* transaction done */ +#define SPACEMIT_SR_TXE BIT(28) /* tx FIFO empty */ +#define SPACEMIT_SR_RXHF BIT(29) /* rx FIFO half-full */ +#define SPACEMIT_SR_RXF BIT(30) /* rx FIFO full */ +#define SPACEMIT_SR_RXOV BIT(31) /* RX FIFO overrun */ + +#define SPACEMIT_I2C_INT_STATUS_MASK (SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \ + SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \ + SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \ + SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \ + SPACEMIT_SR_ALD) + +/* register SPACEMIT_IBMR fields */ +#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */ +#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */ + +/* i2c bus recover timeout: us */ +#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000 + +#define SPACEMIT_I2C_GET_ERR(status) ((status) & \ + (SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)) + +enum spacemit_i2c_state { + STATE_IDLE, + STATE_START, + STATE_READ, + STATE_WRITE, +}; + +/* i2c-spacemit driver's main struct */ +struct spacemit_i2c_dev { + struct device *dev; + struct i2c_adapter adapt; + + /* hardware resources */ + void __iomem *base; + int irq; + u32 clock_freq; + + struct i2c_msg *msgs; + int msg_num; + + /* index of the current message being processed */ + int msg_idx; + u8 *msg_buf; + /* the number of unprocessed bytes remaining in each message */ + size_t unprocessed; + + enum spacemit_i2c_state state; + bool read; + struct completion complete; + u32 status; +}; + +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c) +{ + u32 val; + + val = readl(i2c->base + SPACEMIT_ICR); + val |= SPACEMIT_CR_IUE; + writel(val, i2c->base + SPACEMIT_ICR); +} + +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c) +{ + u32 val; + + val = readl(i2c->base + SPACEMIT_ICR); + val &= ~SPACEMIT_CR_IUE; + writel(val, i2c->base + SPACEMIT_ICR); +} + +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c) +{ + writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR); + udelay(5); + writel(0, i2c->base + SPACEMIT_ICR); +} + +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c) +{ + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); + + dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status); + + if (err & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) { + spacemit_i2c_reset(i2c); + return -EAGAIN; + } + + return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO; +} + +static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c) +{ + u32 status; + + /* if bus is locked, reset unit. 0: locked */ + status = readl(i2c->base + SPACEMIT_IBMR); + if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL)) + return; + + spacemit_i2c_reset(i2c); + usleep_range(10, 20); + + /* check scl status again */ + status = readl(i2c->base + SPACEMIT_IBMR); + if (!(status & SPACEMIT_BMR_SCL)) + dev_warn_ratelimited(i2c->dev, "unit reset failed\n"); +} + +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c) +{ + int ret = 0; + u32 val; + + val = readl(i2c->base + SPACEMIT_ISR); + if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB))) + return 0; + + ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR, val, + !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)), + 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT); + if (ret) + spacemit_i2c_reset(i2c); + + return ret; +} + +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c) +{ + /* in case bus is not released after transfer completes */ + if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) { + spacemit_i2c_conditionally_reset_bus(i2c); + usleep_range(90, 150); + } +} + +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c) +{ + u32 val; + + /* + * Unmask interrupt bits for all xfer mode: + * bus error, arbitration loss detected. + * For transaction complete signal, we use master stop + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE. + */ + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE; + + /* + * Unmask interrupt bits for interrupt xfer mode: + * DBR rx full. + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only + * need to enable when trigger byte transfer to start + * data sending. + */ + val |= SPACEMIT_CR_DRFIE; + + /* set speed bits: default fast mode */ + val |= SPACEMIT_CR_MODE_FAST; + + /* disable response to general call */ + val |= SPACEMIT_CR_GCD; + + /* enable SCL clock output */ + val |= SPACEMIT_CR_SCLE; + + /* enable master stop detected */ + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE; + + writel(val, i2c->base + SPACEMIT_ICR); +} + +static inline void +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask) +{ + writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR); +} + +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c) +{ + u32 slave_addr_rw, val; + struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx; + + i2c->read = cur_msg->flags & I2C_M_RD; + + i2c->state = STATE_START; + + if (cur_msg->flags & I2C_M_RD) + slave_addr_rw = ((cur_msg->addr & 0x7f) << 1) | 1; + else + slave_addr_rw = (cur_msg->addr & 0x7f) << 1; + + writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR); + + /* send start pulse */ + val = readl(i2c->base + SPACEMIT_ICR); + val &= ~SPACEMIT_CR_STOP; + val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE; + writel(val, i2c->base + SPACEMIT_ICR); +} + +static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c) +{ + u32 val; + + val = readl(i2c->base + SPACEMIT_ICR); + val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB; + + if (i2c->read) + val |= SPACEMIT_CR_ACKNAK; + + writel(val, i2c->base + SPACEMIT_ICR); +} + +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c) +{ + unsigned long time_left; + u32 err; + + for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) { + i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf; + i2c->status = 0; + i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len; + + reinit_completion(&i2c->complete); + + spacemit_i2c_start(i2c); + + time_left = wait_for_completion_timeout(&i2c->complete, + i2c->adapt.timeout); + if (time_left == 0) { + dev_err(i2c->dev, "msg completion timeout\n"); + spacemit_i2c_conditionally_reset_bus(i2c); + spacemit_i2c_reset(i2c); + return -ETIMEDOUT; + } + + err = SPACEMIT_I2C_GET_ERR(i2c->status); + if (err) + return spacemit_i2c_handle_err(i2c); + } + + return 0; +} + +static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c) +{ + if (i2c->msg_idx != i2c->msg_num - 1) + return false; + + if (i2c->read) + return i2c->unprocessed == 1; + + return !i2c->unprocessed; +} + +static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c) +{ + /* if transfer completes, SPACEMIT_ISR will handle it */ + if (i2c->status & SPACEMIT_SR_MSD) + return; + + if (i2c->unprocessed) { + writel(*i2c->msg_buf++, i2c->base + SPACEMIT_IDBR); + i2c->unprocessed--; + return; + } + + /* STATE_IDLE avoids trigger next byte */ + i2c->state = STATE_IDLE; + complete(&i2c->complete); +} + +static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c) +{ + if (i2c->unprocessed) { + *i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR); + i2c->unprocessed--; + } + + /* if transfer completes, SPACEMIT_ISR will handle it */ + if (i2c->status & (SPACEMIT_SR_MSD | SPACEMIT_SR_ACKNAK)) + return; + + /* it has to append stop bit in icr that read last byte */ + if (i2c->unprocessed) + return; + + /* STATE_IDLE avoids trigger next byte */ + i2c->state = STATE_IDLE; + complete(&i2c->complete); +} + +static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c) +{ + i2c->state = i2c->read ? STATE_READ : STATE_WRITE; + if (i2c->state == STATE_WRITE) + spacemit_i2c_handle_write(i2c); +} + +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c) +{ + u32 val; + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); + + /* + * send transaction complete signal: + * error happens, detect master stop + */ + if (!err && !(i2c->status & SPACEMIT_SR_MSD)) + return; + + /* + * Here the transaction is already done, we don't need any + * other interrupt signals from now, in case any interrupt + * happens before spacemit_i2c_xfer to disable irq and i2c unit, + * we mask all the interrupt signals and clear the interrupt + * status. + */ + val = readl(i2c->base + SPACEMIT_ICR); + val &= ~SPACEMIT_I2C_INT_CTRL_MASK; + writel(val, i2c->base + SPACEMIT_ICR); + + spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK); + + i2c->state = STATE_IDLE; + complete(&i2c->complete); +} + +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid) +{ + struct spacemit_i2c_dev *i2c = devid; + u32 status, val; + + status = readl(i2c->base + SPACEMIT_ISR); + if (!status) + return IRQ_HANDLED; + + i2c->status = status; + + spacemit_i2c_clear_int_status(i2c, status); + + if (SPACEMIT_I2C_GET_ERR(i2c->status)) + goto err_out; + + val = readl(i2c->base + SPACEMIT_ICR); + val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START); + writel(val, i2c->base + SPACEMIT_ICR); + + switch (i2c->state) { + case STATE_START: + spacemit_i2c_handle_start(i2c); + break; + case STATE_READ: + spacemit_i2c_handle_read(i2c); + break; + case STATE_WRITE: + spacemit_i2c_handle_write(i2c); + break; + default: + break; + } + + if (i2c->state != STATE_IDLE) { + if (spacemit_i2c_is_last_msg(i2c)) { + /* trigger next byte with stop */ + spacemit_i2c_stop(i2c); + } else { + /* trigger next byte */ + val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB; + writel(val, i2c->base + SPACEMIT_ICR); + } + } + +err_out: + spacemit_i2c_err_check(i2c); + return IRQ_HANDLED; +} + +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c) +{ + unsigned long timeout; + int idx = 0, cnt = 0; + + while (idx < i2c->msg_num) { + cnt += (i2c->msgs + idx)->len + 1; + idx++; + } + + /* + * multiply by 9 because each byte in I2C transmission requires + * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit. + */ + timeout = cnt * 9 * USEC_PER_SEC / i2c->clock_freq; + + i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num; +} + +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c) +{ + int ret; + + spacemit_i2c_reset(i2c); + + spacemit_i2c_calc_timeout(i2c); + + spacemit_i2c_init(i2c); + + spacemit_i2c_enable(i2c); + enable_irq(i2c->irq); + + /* i2c wait for bus busy */ + ret = spacemit_i2c_recover_bus_busy(i2c); + if (ret) + return ret; + + ret = spacemit_i2c_xfer_msg(i2c); + if (ret < 0) + dev_dbg(i2c->dev, "i2c transfer error\n"); + + return ret; +} + +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num) +{ + struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt); + int ret; + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); + + i2c->msgs = msgs; + i2c->msg_num = num; + + ret = spacemit_i2c_xfer_core(i2c); + if (!ret) + spacemit_i2c_check_bus_release(i2c); + + disable_irq(i2c->irq); + + spacemit_i2c_disable(i2c); + + if (ret == -ETIMEDOUT || ret == -EAGAIN) + dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, err); + + return ret < 0 ? ret : num; +} + +static u32 spacemit_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); +} + +static const struct i2c_algorithm spacemit_i2c_algo = { + .xfer = spacemit_i2c_xfer, + .functionality = spacemit_i2c_func, +}; + +static int spacemit_i2c_probe(struct platform_device *pdev) +{ + struct clk *clk; + struct device *dev = &pdev->dev; + struct device_node *of_node = pdev->dev.of_node; + struct spacemit_i2c_dev *i2c; + int ret = 0; + + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); + if (!i2c) + return -ENOMEM; + + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq); + if (ret) + return dev_err_probe(dev, ret, "failed to read clock-frequency property"); + + /* For now, this driver doesn't support high-speed. */ + if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) { + dev_warn(dev, "unsupport clock frequency: %d, default: 400000", i2c->clock_freq); + i2c->clock_freq = 400000; + } + + i2c->dev = &pdev->dev; + + i2c->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(i2c->base)) + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap"); + + i2c->irq = platform_get_irq(pdev, 0); + if (i2c->irq < 0) + return dev_err_probe(dev, i2c->irq, "failed to get irq resource"); + + ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler, + IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c); + if (ret) + return dev_err_probe(dev, ret, "failed to request irq"); + + disable_irq(i2c->irq); + + clk = devm_clk_get_enabled(dev, "apb"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock"); + + clk = devm_clk_get_enabled(dev, "twsi"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi clock"); + + i2c_set_adapdata(&i2c->adapt, i2c); + i2c->adapt.owner = THIS_MODULE; + i2c->adapt.algo = &spacemit_i2c_algo; + i2c->adapt.dev.parent = i2c->dev; + i2c->adapt.nr = pdev->id; + + i2c->adapt.dev.of_node = of_node; + i2c->adapt.algo_data = i2c; + + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name)); + + init_completion(&i2c->complete); + + platform_set_drvdata(pdev, i2c); + + ret = i2c_add_numbered_adapter(&i2c->adapt); + if (ret) + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter"); + + return 0; +} + +static void spacemit_i2c_remove(struct platform_device *pdev) +{ + struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev); + + i2c_del_adapter(&i2c->adapt); +} + +static const struct of_device_id spacemit_i2c_of_match[] = { + { .compatible = "spacemit,k1-i2c", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match); + +static struct platform_driver spacemit_i2c_driver = { + .probe = spacemit_i2c_probe, + .remove = spacemit_i2c_remove, + .driver = { + .name = "i2c-k1", + .of_match_table = spacemit_i2c_of_match, + }, +}; +module_platform_driver(spacemit_i2c_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-03 5:30 ` [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell @ 2025-03-03 6:08 ` Wolfram Sang 2025-03-03 7:11 ` Yao Zi 2025-03-04 0:01 ` Alex Elder 1 sibling, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2025-03-03 6:08 UTC (permalink / raw) To: Troy Mitchell Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit [-- Attachment #1: Type: text/plain, Size: 673 bytes --] > +/* spacemit i2c registers */ > +#define SPACEMIT_ICR 0x0 /* Control Register */ > +#define SPACEMIT_ISR 0x4 /* Status Register */ > +#define SPACEMIT_IDBR 0xc /* Data Buffer Register */ > +#define SPACEMIT_IBMR 0x1c /* Bus monitor register */ > + > +/* register SPACEMIT_ICR fields */ > +#define SPACEMIT_CR_START BIT(0) /* start bit */ > +#define SPACEMIT_CR_STOP BIT(1) /* stop bit */ > +#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */ > +#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */ This looks like a lot like a variant of the i2c-pxa register set. Has it been considered to reuse that driver? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-03 6:08 ` Wolfram Sang @ 2025-03-03 7:11 ` Yao Zi 2025-03-03 7:34 ` Wolfram Sang 0 siblings, 1 reply; 14+ messages in thread From: Yao Zi @ 2025-03-03 7:11 UTC (permalink / raw) To: Wolfram Sang, Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit On Mon, Mar 03, 2025 at 07:08:41AM +0100, Wolfram Sang wrote: > > > +/* spacemit i2c registers */ > > +#define SPACEMIT_ICR 0x0 /* Control Register */ > > +#define SPACEMIT_ISR 0x4 /* Status Register */ > > +#define SPACEMIT_IDBR 0xc /* Data Buffer Register */ > > +#define SPACEMIT_IBMR 0x1c /* Bus monitor register */ > > + > > +/* register SPACEMIT_ICR fields */ > > +#define SPACEMIT_CR_START BIT(0) /* start bit */ > > +#define SPACEMIT_CR_STOP BIT(1) /* stop bit */ > > +#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */ > > +#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */ > > This looks like a lot like a variant of the i2c-pxa register set. Has it > been considered to reuse that driver? > Reusing existing driver has been discussed earlier[1] and the answer was no. It really has been a long time. Thanks, Yao Zi [1]: https://lore.kernel.org/all/6015d35d-6d91-4ac1-8ebf-4f79b304370f@gmail.com/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-03 7:11 ` Yao Zi @ 2025-03-03 7:34 ` Wolfram Sang 0 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2025-03-03 7:34 UTC (permalink / raw) To: Yao Zi Cc: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan, linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit [-- Attachment #1: Type: text/plain, Size: 183 bytes --] > Reusing existing driver has been discussed earlier[1] and the answer was > no. It really has been a long time. Thanks for the link. I still agree to my opinion from back then :) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-03 5:30 ` [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell 2025-03-03 6:08 ` Wolfram Sang @ 2025-03-04 0:01 ` Alex Elder 2025-03-06 13:16 ` Troy Mitchell 1 sibling, 1 reply; 14+ messages in thread From: Alex Elder @ 2025-03-04 0:01 UTC (permalink / raw) To: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit On 3/2/25 11:30 PM, Troy Mitchell wrote: > This patch introduces basic I2C support for the SpacemiT K1 SoC, > utilizing interrupts for transfers. > > The driver has been tested using i2c-tools on a Bananapi-F3 board, > and basic I2C read/write operations have been confirmed to work. > > Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> I have some more comments, and some questions. I appreciate seeing some of the changes you've made based on my feedback. > --- > drivers/i2c/busses/Kconfig | 17 ++ > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-k1.c | 617 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 635 insertions(+) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -783,6 +783,23 @@ config I2C_JZ4780 > > If you don't know what to do here, say N. > > +config I2C_K1 > + tristate "Spacemit K1 I2C adapter" > + depends on ARCH_SPACEMIT || COMPILE_TEST > + depends on OF > + help > + This option enables support for the I2C interface on the Spacemit K1 > + platform. > + > + If you enable this configuration, the kernel will include support for > + the I2C adapter specific to the Spacemit K1 platform. This driver can > + be used to manage I2C bus transactions, which are necessary for > + interfacing with I2C peripherals such as sensors, EEPROMs, and other > + devices. > + > + This driver can also be built as a module. If so, the > + module will be called `i2c-k1`. > + > config I2C_KEBA > tristate "KEBA I2C controller support" > depends on HAS_IOMEM > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -74,6 +74,7 @@ obj-$(CONFIG_I2C_IMX) += i2c-imx.o > obj-$(CONFIG_I2C_IMX_LPI2C) += i2c-imx-lpi2c.o > obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o > obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o > +obj-$(CONFIG_I2C_K1) += i2c-k1.o > obj-$(CONFIG_I2C_KEBA) += i2c-keba.o > obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o > obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c > new file mode 100644 > index 0000000000000000000000000000000000000000..6abe05640782dfa93a15d130c58ac879a423e061 > --- /dev/null > +++ b/drivers/i2c/busses/i2c-k1.c > @@ -0,0 +1,617 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com> > + */ > + > + #include <linux/clk.h> > + #include <linux/i2c.h> > + #include <linux/iopoll.h> > + #include <linux/module.h> > + #include <linux/of_address.h> > + #include <linux/platform_device.h> > + > +/* spacemit i2c registers */ > +#define SPACEMIT_ICR 0x0 /* Control Register */ > +#define SPACEMIT_ISR 0x4 /* Status Register */ > +#define SPACEMIT_IDBR 0xc /* Data Buffer Register */ > +#define SPACEMIT_IBMR 0x1c /* Bus monitor register */ > + > +/* register SPACEMIT_ICR fields */ This is minor, but to me "SPACEMIT_ICR register fields" sounds better. If you decide to rearrange this, do so for all of them. > +#define SPACEMIT_CR_START BIT(0) /* start bit */ > +#define SPACEMIT_CR_STOP BIT(1) /* stop bit */ > +#define SPACEMIT_CR_ACKNAK BIT(2) /* send ACK(0) or NAK(1) */ > +#define SPACEMIT_CR_TB BIT(3) /* transfer byte bit */ > +/* Bit 4-7 are reserved */ Bits? (plural) > +#define SPACEMIT_CR_MODE_FAST BIT(8) /* bus mode (master operation) */ > +/* Bit 9 is reserved */ > +#define SPACEMIT_CR_UR BIT(10) /* unit reset */ > +/* Bit 11-12 are reserved */ > +#define SPACEMIT_CR_SCLE BIT(13) /* master clock enable */ > +#define SPACEMIT_CR_IUE BIT(14) /* unit enable */ > +/* Bit 15-17 are reserved */ > +#define SPACEMIT_CR_ALDIE BIT(18) /* enable arbitration interrupt */ > +#define SPACEMIT_CR_DTEIE BIT(19) /* enable tx interrupts */ > +#define SPACEMIT_CR_DRFIE BIT(20) /* enable rx interrupts */ > +#define SPACEMIT_CR_GCD BIT(21) /* general call disable */ > +#define SPACEMIT_CR_BEIE BIT(22) /* enable bus error ints */ > +/* Bit 23-24 are reserved */ > +#define SPACEMIT_CR_MSDIE BIT(25) /* master STOP detected int enable */ > +#define SPACEMIT_CR_MSDE BIT(26) /* master STOP detected enable */ > +#define SPACEMIT_CR_TXDONEIE BIT(27) /* transaction done int enable */ > +#define SPACEMIT_CR_TXEIE BIT(28) /* transmit FIFO empty int enable */ > +#define SPACEMIT_CR_RXHFIE BIT(29) /* receive FIFO half-full int enable */ > +#define SPACEMIT_CR_RXFIE BIT(30) /* receive FIFO full int enable */ > +#define SPACEMIT_CR_RXOVIE BIT(31) /* receive FIFO overrun int enable */ > + > +#define SPACEMIT_I2C_INT_CTRL_MASK (SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \ > + SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \ > + SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \ > + SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \ > + SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE) > + > +/* register SPACEMIT_ISR fields */ /* Bits 0-13 are reserved */ > +#define SPACEMIT_SR_ACKNAK BIT(14) /* ACK/NACK status */ > +#define SPACEMIT_SR_UB BIT(15) /* unit busy */ > +#define SPACEMIT_SR_IBB BIT(16) /* i2c bus busy */ > +#define SPACEMIT_SR_EBB BIT(17) /* early bus busy */ > +#define SPACEMIT_SR_ALD BIT(18) /* arbitration loss detected */ > +#define SPACEMIT_SR_ITE BIT(19) /* tx buffer empty */ > +#define SPACEMIT_SR_IRF BIT(20) /* rx buffer full */ > +#define SPACEMIT_SR_GCAD BIT(21) /* general call address detected */ > +#define SPACEMIT_SR_BED BIT(22) /* bus error no ACK/NAK */ > +#define SPACEMIT_SR_SAD BIT(23) /* slave address detected */ > +#define SPACEMIT_SR_SSD BIT(24) /* slave stop detected */ > +/* Bit 25 is reserved */ > +#define SPACEMIT_SR_MSD BIT(26) /* master stop detected */ > +#define SPACEMIT_SR_TXDONE BIT(27) /* transaction done */ > +#define SPACEMIT_SR_TXE BIT(28) /* tx FIFO empty */ > +#define SPACEMIT_SR_RXHF BIT(29) /* rx FIFO half-full */ > +#define SPACEMIT_SR_RXF BIT(30) /* rx FIFO full */ > +#define SPACEMIT_SR_RXOV BIT(31) /* RX FIFO overrun */ > + > +#define SPACEMIT_I2C_INT_STATUS_MASK (SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \ > + SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \ > + SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \ > + SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \ > + SPACEMIT_SR_ALD) > + > +/* register SPACEMIT_IBMR fields */ > +#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */ > +#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */ > + > +/* i2c bus recover timeout: us */ > +#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000 > + I think it's clearer to just define a mask, something like: #define SPACEMIT_SR_ERROR \ (SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD) And then in the code where needed, do: if (status->i2c & SPACEMIT_ERROR) handle_error(); I'll talk about this more where you use it, below. > +#define SPACEMIT_I2C_GET_ERR(status) ((status) & \ > + (SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)) > + > +enum spacemit_i2c_state { > + STATE_IDLE, > + STATE_START, > + STATE_READ, > + STATE_WRITE, > +}; > + > +/* i2c-spacemit driver's main struct */ > +struct spacemit_i2c_dev { > + struct device *dev; > + struct i2c_adapter adapt; > + > + /* hardware resources */ > + void __iomem *base; > + int irq; > + u32 clock_freq; > + > + struct i2c_msg *msgs; > + int msg_num; > + > + /* index of the current message being processed */ > + int msg_idx; > + u8 *msg_buf; > + /* the number of unprocessed bytes remaining in each message */ > + size_t unprocessed; > + > + enum spacemit_i2c_state state; > + bool read; > + struct completion complete; > + u32 status; > +}; > + > +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c) > +{ > + u32 val; > + > + val = readl(i2c->base + SPACEMIT_ICR); > + val |= SPACEMIT_CR_IUE; > + writel(val, i2c->base + SPACEMIT_ICR); > +} > + > +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c) > +{ > + u32 val; > + > + val = readl(i2c->base + SPACEMIT_ICR); > + val &= ~SPACEMIT_CR_IUE; > + writel(val, i2c->base + SPACEMIT_ICR); > +} > + > +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c) > +{ > + writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR); > + udelay(5); > + writel(0, i2c->base + SPACEMIT_ICR); > +} > + > +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c) > +{ > + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); The macro call above adds no value. > + > + dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status); > + Instead, you can just do this check on i2c->status directly. if (i2c->status & ( ... )) { If you wanted to cache the status value in a local variable, you could do that too, but that doesn't require the macro. > + if (err & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) { > + spacemit_i2c_reset(i2c); > + return -EAGAIN; > + } > + > + return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO; > +} > + > +static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c) > +{ > + u32 status; > + > + /* if bus is locked, reset unit. 0: locked */ > + status = readl(i2c->base + SPACEMIT_IBMR); > + if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL)) > + return; > + > + spacemit_i2c_reset(i2c); > + usleep_range(10, 20); > + > + /* check scl status again */ > + status = readl(i2c->base + SPACEMIT_IBMR); > + if (!(status & SPACEMIT_BMR_SCL)) > + dev_warn_ratelimited(i2c->dev, "unit reset failed\n"); > +} > + I think spacemit_i2c_wait_bus_busy() would be a better name for this function. Actually, are you waiting for the bus to *not* be busy? If so, name it that. > +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c) > +{ > + int ret = 0; There is no need to initialize ret. > + u32 val; > + > + val = readl(i2c->base + SPACEMIT_ISR); > + if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB))) > + return 0; > + > + ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR, val, > + !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)), > + 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT); > + if (ret) > + spacemit_i2c_reset(i2c); > + > + return ret; > +} > + > +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c) > +{ > + /* in case bus is not released after transfer completes */ > + if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) { > + spacemit_i2c_conditionally_reset_bus(i2c); > + usleep_range(90, 150); > + } > +} > + > +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c) > +{ > + u32 val; > + > + /* > + * Unmask interrupt bits for all xfer mode: > + * bus error, arbitration loss detected. > + * For transaction complete signal, we use master stop > + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE. > + */ > + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE; > + > + /* > + * Unmask interrupt bits for interrupt xfer mode: > + * DBR rx full. > + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only > + * need to enable when trigger byte transfer to start > + * data sending. > + */ > + val |= SPACEMIT_CR_DRFIE; > + > + /* set speed bits: default fast mode */ It is not *default* fast mode, it *is* fast mode. (There is no other mode used in this driver, right?) > + val |= SPACEMIT_CR_MODE_FAST; > + > + /* disable response to general call */ > + val |= SPACEMIT_CR_GCD; > + > + /* enable SCL clock output */ > + val |= SPACEMIT_CR_SCLE; > + > + /* enable master stop detected */ > + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE; > + > + writel(val, i2c->base + SPACEMIT_ICR); > +} > + > +static inline void > +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask) > +{ > + writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR); > +} > + > +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c) > +{ > + u32 slave_addr_rw, val; > + struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx; > + > + i2c->read = cur_msg->flags & I2C_M_RD; i2c->read is Boolean. The assignment above will almost certainly do the right thing, but if you did it this way, it would make it clear you were making a Boolean assingment: i2c->read = !!(cur_msg->flags & I2C_M_RD); > + > + i2c->state = STATE_START; > + > + if (cur_msg->flags & I2C_M_RD) > + slave_addr_rw = ((cur_msg->addr & 0x7f) << 1) | 1; > + else > + slave_addr_rw = (cur_msg->addr & 0x7f) << 1; I think you said you were going to implement my suggestion here: slave_addr_rw = (cur_msg->addr & 0x7f) << 1; if (cur_msg->flags & I2C_M_RD) slave_addr_rw |= 1; > + > + writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR); > + > + /* send start pulse */ > + val = readl(i2c->base + SPACEMIT_ICR); > + val &= ~SPACEMIT_CR_STOP; > + val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE; > + writel(val, i2c->base + SPACEMIT_ICR); > +} > + > +static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c) > +{ > + u32 val; > + > + val = readl(i2c->base + SPACEMIT_ICR); > + val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB; > + > + if (i2c->read) > + val |= SPACEMIT_CR_ACKNAK; > + > + writel(val, i2c->base + SPACEMIT_ICR); > +} > + > +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c) > +{ > + unsigned long time_left; > + u32 err; > + > + for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) { > + i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf; > + i2c->status = 0; > + i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len; > + > + reinit_completion(&i2c->complete); > + > + spacemit_i2c_start(i2c); > + > + time_left = wait_for_completion_timeout(&i2c->complete, > + i2c->adapt.timeout); > + if (time_left == 0) { > + dev_err(i2c->dev, "msg completion timeout\n"); > + spacemit_i2c_conditionally_reset_bus(i2c); > + spacemit_i2c_reset(i2c); > + return -ETIMEDOUT; > + } > + > + err = SPACEMIT_I2C_GET_ERR(i2c->status); This would be better as: if (i2c->status & SPACEMIT_SR_ERROR) return spacemit_i2c_handle_err(i2c); > + if (err) > + return spacemit_i2c_handle_err(i2c); > + } > + > + return 0; > +} > + > +static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c) > +{ > + if (i2c->msg_idx != i2c->msg_num - 1) > + return false; > + > + if (i2c->read) > + return i2c->unprocessed == 1; > + > + return !i2c->unprocessed; > +} > + > +static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c) > +{ > + /* if transfer completes, SPACEMIT_ISR will handle it */ > + if (i2c->status & SPACEMIT_SR_MSD) > + return; > + > + if (i2c->unprocessed) { > + writel(*i2c->msg_buf++, i2c->base + SPACEMIT_IDBR); > + i2c->unprocessed--; > + return; > + } > + > + /* STATE_IDLE avoids trigger next byte */ > + i2c->state = STATE_IDLE; > + complete(&i2c->complete); > +} > + > +static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c) > +{ > + if (i2c->unprocessed) { > + *i2c->msg_buf++ = readl(i2c->base + SPACEMIT_IDBR); > + i2c->unprocessed--; > + } > + > + /* if transfer completes, SPACEMIT_ISR will handle it */ > + if (i2c->status & (SPACEMIT_SR_MSD | SPACEMIT_SR_ACKNAK)) > + return; > + > + /* it has to append stop bit in icr that read last byte */ > + if (i2c->unprocessed) > + return; > + > + /* STATE_IDLE avoids trigger next byte */ > + i2c->state = STATE_IDLE; > + complete(&i2c->complete); > +} > + > +static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c) > +{ > + i2c->state = i2c->read ? STATE_READ : STATE_WRITE; > + if (i2c->state == STATE_WRITE) > + spacemit_i2c_handle_write(i2c); > +} > + > +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c) > +{ > + u32 val; > + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); > + > + /* > + * send transaction complete signal: > + * error happens, detect master stop > + */ The next line is the only place you use err in this function. I think you should instead do: if (!(i2c->status & (SPACEMIT_SR_ERROR | SPACEMIT_SR_MSD))) return; > + if (!err && !(i2c->status & SPACEMIT_SR_MSD)) > + return; > + > + /* > + * Here the transaction is already done, we don't need any > + * other interrupt signals from now, in case any interrupt > + * happens before spacemit_i2c_xfer to disable irq and i2c unit, > + * we mask all the interrupt signals and clear the interrupt > + * status. > + */ > + val = readl(i2c->base + SPACEMIT_ICR); > + val &= ~SPACEMIT_I2C_INT_CTRL_MASK; > + writel(val, i2c->base + SPACEMIT_ICR); > + > + spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK); > + > + i2c->state = STATE_IDLE; > + complete(&i2c->complete); > +} > + > +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid) > +{ > + struct spacemit_i2c_dev *i2c = devid; > + u32 status, val; > + > + status = readl(i2c->base + SPACEMIT_ISR); > + if (!status) > + return IRQ_HANDLED; > + > + i2c->status = status; > + > + spacemit_i2c_clear_int_status(i2c, status); > + > + if (SPACEMIT_I2C_GET_ERR(i2c->status)) > + goto err_out; > + > + val = readl(i2c->base + SPACEMIT_ICR); > + val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START); > + writel(val, i2c->base + SPACEMIT_ICR); > + > + switch (i2c->state) { > + case STATE_START: > + spacemit_i2c_handle_start(i2c); > + break; > + case STATE_READ: > + spacemit_i2c_handle_read(i2c); > + break; > + case STATE_WRITE: > + spacemit_i2c_handle_write(i2c); > + break; > + default: > + break; > + } > + > + if (i2c->state != STATE_IDLE) { > + if (spacemit_i2c_is_last_msg(i2c)) { > + /* trigger next byte with stop */ > + spacemit_i2c_stop(i2c); > + } else { > + /* trigger next byte */ > + val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB; > + writel(val, i2c->base + SPACEMIT_ICR); > + } > + } > + > +err_out: > + spacemit_i2c_err_check(i2c); > + return IRQ_HANDLED; > +} > + > +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c) > +{ > + unsigned long timeout; > + int idx = 0, cnt = 0; > + > + while (idx < i2c->msg_num) { > + cnt += (i2c->msgs + idx)->len + 1; > + idx++; > + } > + > + /* > + * multiply by 9 because each byte in I2C transmission requires > + * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit. > + */ > + timeout = cnt * 9 * USEC_PER_SEC / i2c->clock_freq; > + > + i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num; > +} > + > +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c) > +{ > + int ret; > + > + spacemit_i2c_reset(i2c); I don't have a lot of experience with I2C drivers, but is it normal to reset before every transfer? If it is, just tell me that. But if it's not, can you explain why it's necessary here? > + > + spacemit_i2c_calc_timeout(i2c); > + > + spacemit_i2c_init(i2c); > + Here too, maybe I just don't know what most I2C drivers do, but is it necessary to only enable the I2C adapter and its interrupt handler when performing a transfer? > + spacemit_i2c_enable(i2c); > + enable_irq(i2c->irq); > + > + /* i2c wait for bus busy */ > + ret = spacemit_i2c_recover_bus_busy(i2c); > + if (ret) > + return ret; > + > + ret = spacemit_i2c_xfer_msg(i2c); > + if (ret < 0) > + dev_dbg(i2c->dev, "i2c transfer error\n"); If you're reporting the error you might as well say what it is. dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret); > + > + return ret; > +} > + > +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num) > +{ > + struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt); > + int ret; > + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); > + > + i2c->msgs = msgs; > + i2c->msg_num = num; > + > + ret = spacemit_i2c_xfer_core(i2c); > + if (!ret) > + spacemit_i2c_check_bus_release(i2c); > + The enable_irq() call that matches the disable call below is found in spacemit_i2c_xfer_core(). That's where this call belongs. > + disable_irq(i2c->irq); > + Same with the next call--it should be in the same function that its corresponding spacemit_i2c_enable() is called. With these suggestions in mind, I think you can safely just get rid of spacemit_i2c_xfer_core(). It is only called in this one spot (above), and you can just do everything within spacemit_i2c_xfer() instead. > + spacemit_i2c_disable(i2c); > + > + if (ret == -ETIMEDOUT || ret == -EAGAIN) > + dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, err); > + > + return ret < 0 ? ret : num; > +} > + > +static u32 spacemit_i2c_func(struct i2c_adapter *adap) > +{ > + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); > +} > + > +static const struct i2c_algorithm spacemit_i2c_algo = { > + .xfer = spacemit_i2c_xfer, > + .functionality = spacemit_i2c_func, > +}; > + > +static int spacemit_i2c_probe(struct platform_device *pdev) > +{ > + struct clk *clk; > + struct device *dev = &pdev->dev; > + struct device_node *of_node = pdev->dev.of_node; > + struct spacemit_i2c_dev *i2c; > + int ret = 0; There is no need to initialize ret. > + > + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); > + if (!i2c) > + return -ENOMEM; > + > + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq); > + if (ret) > + return dev_err_probe(dev, ret, "failed to read clock-frequency property"); > + > + /* For now, this driver doesn't support high-speed. */ > + if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) { In your device tree binding, you indicate that three different modes are supported, and that the maximum frequency is 3300000 Hz. This says that only ranges from 1-400000 Hz are allowed. In fact, although you look up this clock frequency in DT, I see nothing that actually is affected by this value. I.e., no I2C bus frequency changes, regardless of what frequency you specify. The only place the clock_freq field is used is in calculating the timeout for a transfer. So two things: - My guess is that you are relying on whatever frequency the hardware already is using, and maybe that's 400000 Hz. That's fine, though at some point it should be more directly controlled (set somehow). - Since you don't actually support any other frequency, drop this "clock-frequency" feature for now, and add it when you're ready to actually support it. And I might be wrong about this, but I don't think your (new) DTS binding should specify behavior that is not supported by the driver. -Alex > + dev_warn(dev, "unsupport clock frequency: %d, default: 400000", i2c->clock_freq); > + i2c->clock_freq = 400000; > + } > + > + i2c->dev = &pdev->dev; > + > + i2c->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(i2c->base)) > + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap"); > + > + i2c->irq = platform_get_irq(pdev, 0); > + if (i2c->irq < 0) > + return dev_err_probe(dev, i2c->irq, "failed to get irq resource"); > + > + ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler, > + IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c); > + if (ret) > + return dev_err_probe(dev, ret, "failed to request irq"); > + > + disable_irq(i2c->irq); > + > + clk = devm_clk_get_enabled(dev, "apb"); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock"); > + > + clk = devm_clk_get_enabled(dev, "twsi"); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi clock"); > + > + i2c_set_adapdata(&i2c->adapt, i2c); > + i2c->adapt.owner = THIS_MODULE; > + i2c->adapt.algo = &spacemit_i2c_algo; > + i2c->adapt.dev.parent = i2c->dev; > + i2c->adapt.nr = pdev->id; > + > + i2c->adapt.dev.of_node = of_node; > + i2c->adapt.algo_data = i2c; > + > + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name)); > + > + init_completion(&i2c->complete); > + > + platform_set_drvdata(pdev, i2c); > + > + ret = i2c_add_numbered_adapter(&i2c->adapt); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter"); > + > + return 0; > +} > + > +static void spacemit_i2c_remove(struct platform_device *pdev) > +{ > + struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&i2c->adapt); > +} > + > +static const struct of_device_id spacemit_i2c_of_match[] = { > + { .compatible = "spacemit,k1-i2c", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match); > + > +static struct platform_driver spacemit_i2c_driver = { > + .probe = spacemit_i2c_probe, > + .remove = spacemit_i2c_remove, > + .driver = { > + .name = "i2c-k1", > + .of_match_table = spacemit_i2c_of_match, > + }, > +}; > +module_platform_driver(spacemit_i2c_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC"); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-04 0:01 ` Alex Elder @ 2025-03-06 13:16 ` Troy Mitchell 2025-03-06 20:30 ` Alex Elder 0 siblings, 1 reply; 14+ messages in thread From: Troy Mitchell @ 2025-03-06 13:16 UTC (permalink / raw) To: Alex Elder, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit On 2025/3/4 08:01, Alex Elder wrote: > On 3/2/25 11:30 PM, Troy Mitchell wrote: >> This patch introduces basic I2C support for the SpacemiT K1 SoC, >> utilizing interrupts for transfers. >> >> The driver has been tested using i2c-tools on a Bananapi-F3 board, >> and basic I2C read/write operations have been confirmed to work. >> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> > > I have some more comments, and some questions. I appreciate > seeing some of the changes you've made based on my feedback. Hi, Alex. Thanks for your review. >> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c) >> +{ >> + u32 val; >> + >> + /* >> + * Unmask interrupt bits for all xfer mode: >> + * bus error, arbitration loss detected. >> + * For transaction complete signal, we use master stop >> + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE. >> + */ >> + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE; >> + >> + /* >> + * Unmask interrupt bits for interrupt xfer mode: >> + * DBR rx full. >> + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only >> + * need to enable when trigger byte transfer to start >> + * data sending. >> + */ >> + val |= SPACEMIT_CR_DRFIE; >> + >> + /* set speed bits: default fast mode */ > > It is not *default* fast mode, it *is* fast mode. (There > is no other mode used in this driver, right?) yes. I will talk it below. > >> + val |= SPACEMIT_CR_MODE_FAST; >> + >> + /* disable response to general call */ >> + val |= SPACEMIT_CR_GCD; >> + >> + /* enable SCL clock output */ >> + val |= SPACEMIT_CR_SCLE; >> + >> + /* enable master stop detected */ >> + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE; >> + >> + writel(val, i2c->base + SPACEMIT_ICR); >> +} >> + >> + >> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c) >> +{ >> + int ret; >> + >> + spacemit_i2c_reset(i2c); > > I don't have a lot of experience with I2C drivers, but is it normal > to reset before every transfer? > > If it is, just tell me that. But if it's not, can you explain why > it's necessary here? My initial idea was to keep the I2C state in its initial state before each transmission. But after testing, this is not necessary. I will move it to `probe` function. > >> + >> + spacemit_i2c_calc_timeout(i2c); >> + >> + spacemit_i2c_init(i2c); >> + > > Here too, maybe I just don't know what most I2C drivers do, but > is it necessary to only enable the I2C adapter and its interrupt > handler when performing a transfer? It is necessary to enable before each transmission. I have tested moving the `spacemit_i2c_enable` to the probe function. It will cause transmission errors. As for the `enable_irq`, I think it can be moved to the `probe` function. > >> + spacemit_i2c_enable(i2c); >> + enable_irq(i2c->irq); >> + >> + /* i2c wait for bus busy */ >> + ret = spacemit_i2c_recover_bus_busy(i2c); >> + if (ret) >> + return ret; >> + >> + ret = spacemit_i2c_xfer_msg(i2c); >> + if (ret < 0) >> + dev_dbg(i2c->dev, "i2c transfer error\n"); > > If you're reporting the error you might as well say what > it is. > > dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret); > >> + >> + return ret; >> +} >> + >> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg >> *msgs, int num) >> +{ >> + struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt); >> + int ret; >> + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); >> + >> + i2c->msgs = msgs; >> + i2c->msg_num = num; >> + >> + ret = spacemit_i2c_xfer_core(i2c); >> + if (!ret) >> + spacemit_i2c_check_bus_release(i2c); >> + > > The enable_irq() call that matches the disable call below is > found in spacemit_i2c_xfer_core(). That's where this call > belongs. > >> + disable_irq(i2c->irq); >> + > > Same with the next call--it should be in the same function > that its corresponding spacemit_i2c_enable() is called. > > With these suggestions in mind, I think you can safely > just get rid of spacemit_i2c_xfer_core(). It is only > called in this one spot (above), and you can just do > everything within spacemit_i2c_xfer() instead. > >> + spacemit_i2c_disable(i2c); >> + >> + if (ret == -ETIMEDOUT || ret == -EAGAIN) >> + dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, >> err); >> + >> + return ret < 0 ? ret : num; >> +} >> + >> +static u32 spacemit_i2c_func(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); >> +} >> + >> +static const struct i2c_algorithm spacemit_i2c_algo = { >> + .xfer = spacemit_i2c_xfer, >> + .functionality = spacemit_i2c_func, >> +}; >> + >> +static int spacemit_i2c_probe(struct platform_device *pdev) >> +{ >> + struct clk *clk; >> + struct device *dev = &pdev->dev; >> + struct device_node *of_node = pdev->dev.of_node; >> + struct spacemit_i2c_dev *i2c; >> + int ret = 0; > > There is no need to initialize ret. > >> + >> + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); >> + if (!i2c) >> + return -ENOMEM; >> + >> + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to read clock-frequency >> property"); >> + >> + /* For now, this driver doesn't support high-speed. */ >> + if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) { > > In your device tree binding, you indicate that three different > modes are supported, and that the maximum frequency is 3300000 Hz. > This says that only ranges from 1-400000 Hz are allowed. > > In fact, although you look up this clock frequency in DT, I see > nothing that actually is affected by this value. I.e., no I2C > bus frequency changes, regardless of what frequency you specify. > The only place the clock_freq field is used is in calculating > the timeout for a transfer. > > So two things: > - My guess is that you are relying on whatever frequency the > hardware already is using, and maybe that's 400000 Hz. > That's fine, though at some point it should be more > directly controlled (set somehow). > - Since you don't actually support any other frequency, > drop this "clock-frequency" feature for now, and add it > when you're ready to actually support it. > > And I might be wrong about this, but I don't think your > (new) DTS binding should specify behavior that is not > supported by the driver. > > -Alex I will support standard mode in next version. We just need to modify the function `spacemit_i2c_init`. > >> + dev_warn(dev, "unsupport clock frequency: %d, default: 400000", >> i2c->clock_freq); >> + i2c->clock_freq = 400000; >> + } >> + >> + i2c->dev = &pdev->dev; >> + >> + i2c->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(i2c->base)) >> + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap"); >> + >> + i2c->irq = platform_get_irq(pdev, 0); >> + if (i2c->irq < 0) >> + return dev_err_probe(dev, i2c->irq, "failed to get irq resource"); >> + >> + ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler, >> + IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to request irq"); >> + >> + disable_irq(i2c->irq); >> + >> + clk = devm_clk_get_enabled(dev, "apb"); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock"); >> + >> + clk = devm_clk_get_enabled(dev, "twsi"); >> + if (IS_ERR(clk)) >> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi clock"); >> + >> + i2c_set_adapdata(&i2c->adapt, i2c); >> + i2c->adapt.owner = THIS_MODULE; >> + i2c->adapt.algo = &spacemit_i2c_algo; >> + i2c->adapt.dev.parent = i2c->dev; >> + i2c->adapt.nr = pdev->id; >> + >> + i2c->adapt.dev.of_node = of_node; >> + i2c->adapt.algo_data = i2c; >> + >> + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name)); >> + >> + init_completion(&i2c->complete); >> + >> + platform_set_drvdata(pdev, i2c); >> + >> + ret = i2c_add_numbered_adapter(&i2c->adapt); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter"); >> + >> + return 0; >> +} >> + >> +static void spacemit_i2c_remove(struct platform_device *pdev) >> +{ >> + struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev); >> + >> + i2c_del_adapter(&i2c->adapt); >> +} >> + >> +static const struct of_device_id spacemit_i2c_of_match[] = { >> + { .compatible = "spacemit,k1-i2c", }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match); >> + >> +static struct platform_driver spacemit_i2c_driver = { >> + .probe = spacemit_i2c_probe, >> + .remove = spacemit_i2c_remove, >> + .driver = { >> + .name = "i2c-k1", >> + .of_match_table = spacemit_i2c_of_match, >> + }, >> +}; >> +module_platform_driver(spacemit_i2c_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC"); >> > -- Troy Mitchell ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT K1 SoC 2025-03-06 13:16 ` Troy Mitchell @ 2025-03-06 20:30 ` Alex Elder 0 siblings, 0 replies; 14+ messages in thread From: Alex Elder @ 2025-03-06 20:30 UTC (permalink / raw) To: Troy Mitchell, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Yixun Lan Cc: linux-riscv, linux-i2c, devicetree, linux-kernel, spacemit On 3/6/25 7:16 AM, Troy Mitchell wrote: > On 2025/3/4 08:01, Alex Elder wrote: > >> On 3/2/25 11:30 PM, Troy Mitchell wrote: >>> This patch introduces basic I2C support for the SpacemiT K1 SoC, >>> utilizing interrupts for transfers. >>> >>> The driver has been tested using i2c-tools on a Bananapi-F3 board, >>> and basic I2C read/write operations have been confirmed to work. >>> >>> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> >> >> I have some more comments, and some questions. I appreciate >> seeing some of the changes you've made based on my feedback. > Hi, Alex. Thanks for your review. >>> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c) >>> +{ >>> + u32 val; >>> + >>> + /* >>> + * Unmask interrupt bits for all xfer mode: >>> + * bus error, arbitration loss detected. >>> + * For transaction complete signal, we use master stop >>> + * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE. >>> + */ >>> + val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE; >>> + >>> + /* >>> + * Unmask interrupt bits for interrupt xfer mode: >>> + * DBR rx full. >>> + * For tx empty interrupt SPACEMIT_CR_DTEIE, we only >>> + * need to enable when trigger byte transfer to start >>> + * data sending. >>> + */ >>> + val |= SPACEMIT_CR_DRFIE; >>> + >>> + /* set speed bits: default fast mode */ >> >> It is not *default* fast mode, it *is* fast mode. (There >> is no other mode used in this driver, right?) > yes. I will talk it below. >> >>> + val |= SPACEMIT_CR_MODE_FAST; >>> + >>> + /* disable response to general call */ >>> + val |= SPACEMIT_CR_GCD; >>> + >>> + /* enable SCL clock output */ >>> + val |= SPACEMIT_CR_SCLE; >>> + >>> + /* enable master stop detected */ >>> + val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE; >>> + >>> + writel(val, i2c->base + SPACEMIT_ICR); >>> +} >>> + >>> + >>> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c) >>> +{ >>> + int ret; >>> + >>> + spacemit_i2c_reset(i2c); >> >> I don't have a lot of experience with I2C drivers, but is it normal >> to reset before every transfer? >> >> If it is, just tell me that. But if it's not, can you explain why >> it's necessary here? > > My initial idea was to keep the I2C state in its initial state before each > transmission. > > But after testing, this is not necessary. I will move it to `probe` function. OK, that seems better. But honestly you should do this only if you're certain the reset isn't required before every transfer. I don't know, but I assumed it was there for a reason. >>> + >>> + spacemit_i2c_calc_timeout(i2c); >>> + >>> + spacemit_i2c_init(i2c); >>> + >> >> Here too, maybe I just don't know what most I2C drivers do, but >> is it necessary to only enable the I2C adapter and its interrupt >> handler when performing a transfer? > > It is necessary to enable before each transmission. > > I have tested moving the `spacemit_i2c_enable` to the probe function. > > It will cause transmission errors. > > As for the `enable_irq`, I think it can be moved to the `probe` function. It really depends on whether you intend to rule out any interrupts other than when you are performing a transfer. This might be reasonable, but sometimes drivers will keep an interrupt enabled most of the time, sometimes they restrict when it's enabled. Hence my question. > >> >>> + spacemit_i2c_enable(i2c); >>> + enable_irq(i2c->irq); >>> + >>> + /* i2c wait for bus busy */ >>> + ret = spacemit_i2c_recover_bus_busy(i2c); >>> + if (ret) >>> + return ret; >>> + >>> + ret = spacemit_i2c_xfer_msg(i2c); >>> + if (ret < 0) >>> + dev_dbg(i2c->dev, "i2c transfer error\n"); >> >> If you're reporting the error you might as well say what >> it is. >> >> dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret); >> >>> + >>> + return ret; >>> +} >>> + >>> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg >>> *msgs, int num) >>> +{ >>> + struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt); >>> + int ret; >>> + u32 err = SPACEMIT_I2C_GET_ERR(i2c->status); >>> + >>> + i2c->msgs = msgs; >>> + i2c->msg_num = num; >>> + >>> + ret = spacemit_i2c_xfer_core(i2c); >>> + if (!ret) >>> + spacemit_i2c_check_bus_release(i2c); >>> + >> >> The enable_irq() call that matches the disable call below is >> found in spacemit_i2c_xfer_core(). That's where this call >> belongs. I think the above comment is important. I'll look at your next version of the series to see what you do. >> >>> + disable_irq(i2c->irq); >>> + >> >> Same with the next call--it should be in the same function >> that its corresponding spacemit_i2c_enable() is called. >> >> With these suggestions in mind, I think you can safely >> just get rid of spacemit_i2c_xfer_core(). It is only >> called in this one spot (above), and you can just do >> everything within spacemit_i2c_xfer() instead. >> >>> + spacemit_i2c_disable(i2c); >>> + >>> + if (ret == -ETIMEDOUT || ret == -EAGAIN) >>> + dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, >>> err); >>> + >>> + return ret < 0 ? ret : num; >>> +} >>> + >>> +static u32 spacemit_i2c_func(struct i2c_adapter *adap) >>> +{ >>> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); >>> +} >>> + >>> +static const struct i2c_algorithm spacemit_i2c_algo = { >>> + .xfer = spacemit_i2c_xfer, >>> + .functionality = spacemit_i2c_func, >>> +}; >>> + >>> +static int spacemit_i2c_probe(struct platform_device *pdev) >>> +{ >>> + struct clk *clk; >>> + struct device *dev = &pdev->dev; >>> + struct device_node *of_node = pdev->dev.of_node; >>> + struct spacemit_i2c_dev *i2c; >>> + int ret = 0; >> >> There is no need to initialize ret. >> >>> + >>> + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); >>> + if (!i2c) >>> + return -ENOMEM; >>> + >>> + ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "failed to read clock-frequency >>> property"); >>> + >>> + /* For now, this driver doesn't support high-speed. */ >>> + if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) { >> >> In your device tree binding, you indicate that three different >> modes are supported, and that the maximum frequency is 3300000 Hz. >> This says that only ranges from 1-400000 Hz are allowed. >> >> In fact, although you look up this clock frequency in DT, I see >> nothing that actually is affected by this value. I.e., no I2C >> bus frequency changes, regardless of what frequency you specify. >> The only place the clock_freq field is used is in calculating >> the timeout for a transfer. >> >> So two things: >> - My guess is that you are relying on whatever frequency the >> hardware already is using, and maybe that's 400000 Hz. >> That's fine, though at some point it should be more >> directly controlled (set somehow). >> - Since you don't actually support any other frequency, >> drop this "clock-frequency" feature for now, and add it >> when you're ready to actually support it. >> >> And I might be wrong about this, but I don't think your >> (new) DTS binding should specify behavior that is not >> supported by the driver. >> >> -Alex > > I will support standard mode in next version. I'll wait to see what you do, but please try not to change anything substantive between versions of a patch series. > We just need to modify the function `spacemit_i2c_init`. Thanks for your responses. -Alex >> >>> + dev_warn(dev, "unsupport clock frequency: %d, default: 400000", >>> i2c->clock_freq); >>> + i2c->clock_freq = 400000; >>> + } >>> + >>> + i2c->dev = &pdev->dev; >>> + >>> + i2c->base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(i2c->base)) >>> + return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap"); >>> + >>> + i2c->irq = platform_get_irq(pdev, 0); >>> + if (i2c->irq < 0) >>> + return dev_err_probe(dev, i2c->irq, "failed to get irq resource"); >>> + >>> + ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler, >>> + IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "failed to request irq"); >>> + >>> + disable_irq(i2c->irq); >>> + >>> + clk = devm_clk_get_enabled(dev, "apb"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock"); >>> + >>> + clk = devm_clk_get_enabled(dev, "twsi"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi clock"); >>> + >>> + i2c_set_adapdata(&i2c->adapt, i2c); >>> + i2c->adapt.owner = THIS_MODULE; >>> + i2c->adapt.algo = &spacemit_i2c_algo; >>> + i2c->adapt.dev.parent = i2c->dev; >>> + i2c->adapt.nr = pdev->id; >>> + >>> + i2c->adapt.dev.of_node = of_node; >>> + i2c->adapt.algo_data = i2c; >>> + >>> + strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name)); >>> + >>> + init_completion(&i2c->complete); >>> + >>> + platform_set_drvdata(pdev, i2c); >>> + >>> + ret = i2c_add_numbered_adapter(&i2c->adapt); >>> + if (ret) >>> + return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter"); >>> + >>> + return 0; >>> +} >>> + >>> +static void spacemit_i2c_remove(struct platform_device *pdev) >>> +{ >>> + struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev); >>> + >>> + i2c_del_adapter(&i2c->adapt); >>> +} >>> + >>> +static const struct of_device_id spacemit_i2c_of_match[] = { >>> + { .compatible = "spacemit,k1-i2c", }, >>> + { /* sentinel */ } >>> +}; >>> +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match); >>> + >>> +static struct platform_driver spacemit_i2c_driver = { >>> + .probe = spacemit_i2c_probe, >>> + .remove = spacemit_i2c_remove, >>> + .driver = { >>> + .name = "i2c-k1", >>> + .of_match_table = spacemit_i2c_of_match, >>> + }, >>> +}; >>> +module_platform_driver(spacemit_i2c_driver); >>> + >>> +MODULE_LICENSE("GPL"); >>> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC"); >>> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-03-06 20:31 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-03 5:30 [PATCH RESEND v5 0/2] riscv: spacemit: add i2c support to K1 SoC Troy Mitchell 2025-03-03 5:30 ` [PATCH RESEND v5 1/2] dt-bindings: i2c: spacemit: add support for " Troy Mitchell 2025-03-03 9:35 ` Yixun Lan 2025-03-05 2:11 ` Samuel Holland 2025-03-05 3:05 ` Yixun Lan 2025-03-05 4:01 ` Samuel Holland 2025-03-05 14:10 ` Yixun Lan 2025-03-03 5:30 ` [PATCH RESEND v5 2/2] i2c: spacemit: add support for SpacemiT " Troy Mitchell 2025-03-03 6:08 ` Wolfram Sang 2025-03-03 7:11 ` Yao Zi 2025-03-03 7:34 ` Wolfram Sang 2025-03-04 0:01 ` Alex Elder 2025-03-06 13:16 ` Troy Mitchell 2025-03-06 20:30 ` Alex Elder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).