mbox series

[0/3] Add Aspeed AST2600 I3C support

Message ID 20230808154241.749641-1-dylan_hung@aspeedtech.com
Headers show
Series Add Aspeed AST2600 I3C support | expand

Message

Dylan Hung Aug. 8, 2023, 3:42 p.m. UTC 
This patch series introduces the necessary changes to enable I3C support
for the Aspeed AST2600 I3C controller. Specifically, it addresses the
missing pinctrl configuration and reset control for the I3C
functionality.

Dylan Hung (3):
  ARM: dts: pinctrl-aspeed-g6: Add I3C1 and I3C2 control pins
  dt-bindings: i3c: ast2600: Add resets and reset-names
  i3c: ast2600: Add reset deassertion for global registers

 .../devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml  | 12 ++++++++++--
 arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi      | 10 ++++++++++
 drivers/i3c/master/ast2600-i3c-master.c              |  9 +++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

Comments

Jeremy Kerr Aug. 9, 2023, 12:07 a.m. UTC | #1 
Hi Dylan,

> This patch series introduces the necessary changes to enable I3C
> support for the Aspeed AST2600 I3C controller. Specifically, it addresses the
> missing pinctrl configuration and reset control for the I3C
> functionality.

+1 for the pinctrl changes for the I3C1 and I3C2 controllers (I'll
review and ack separately). I have been testing on I3C3 and up, but just
not with the HVI3C on 1 & 2, hence no pinctrl definition there.

However, I don't think the other two are needed.

For 2/3 and 3/3, you're adding a reset control for the global register
block within the per-controller driver, but we can already do that on a
global basis with the existing syscon device. Hence this earlier change:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mfd/syscon.c?id=7d1e3bd94828ad9fc86f55253cd6fec8edd65394

For this, I have:

    &i3c {
        i3c_global: i3c-global {
            compatible = "aspeed,ast2600-i3c-global", "simple-mfd", "syscon";
            resets = <&syscon ASPEED_RESET_I3C_DMA>;
            reg = <0x0 0x1000>;
        };

        i3c2: i3c-master@4000 {
            compatible = "aspeed,ast2600-i3c";
            reg = <0x4000 0x1000>;
            clocks = <&syscon ASPEED_CLK_GATE_I3C2CLK>;
            pinctrl-names = "default";
            pinctrl-0 = <&pinctrl_i3c3_default>;
            interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
            aspeed,global-regs = <&i3c_global 2>;
            status = "disabled";
        };

        /* ... */
    };

- with no changes needed to any bindings. I haven't needed any other
resets; are there per-controller resets specified in the HW docs you
have?

Does that work for you? If you'd like to test, feel free to use my
sample dts at:

  https://github.com/CodeConstruct/linux/commit/05cac24705fa62d2176ecbbbf15d955cfe86e753

Cheers,


Jeremy
Jeremy Kerr Aug. 9, 2023, 12:28 a.m. UTC | #2 
Hi Dylan,

> - with no changes needed to any bindings. I haven't needed any other
> resets; are there per-controller resets specified in the HW docs you
> have?

... keeping in mind the reset control that the aspeed SCU driver
already provides for you, when you enable the appropriate clock gate:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-aspeed.c#n224

Cheers,


Jeremy
Dylan Hung Aug. 9, 2023, 2:46 a.m. UTC | #3 
Hi Jeremy,

> -----Original Message-----
> From: Jeremy Kerr [mailto:jk@codeconstruct.com.au]
> Sent: Wednesday, August 9, 2023 8:08 AM
> To: Dylan Hung <dylan_hung@aspeedtech.com>;
> alexandre.belloni@bootlin.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; joel@jms.id.au;
> andrew@aj.id.au; p.zabel@pengutronix.de; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>; kobedylan@gmail.com
> Subject: Re: [PATCH 0/3] Add Aspeed AST2600 I3C support
> 
> Hi Dylan,
> 
> > This patch series introduces the necessary changes to enable I3C
> > support for the Aspeed AST2600 I3C controller. Specifically, it
> > addresses the missing pinctrl configuration and reset control for the
> > I3C functionality.
> 
> +1 for the pinctrl changes for the I3C1 and I3C2 controllers (I'll
> review and ack separately). I have been testing on I3C3 and up, but just not
> with the HVI3C on 1 & 2, hence no pinctrl definition there.
> 
> However, I don't think the other two are needed.
> 
> For 2/3 and 3/3, you're adding a reset control for the global register block
> within the per-controller driver, but we can already do that on a global basis
> with the existing syscon device. Hence this earlier change:
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dri
> vers/mfd/syscon.c?id=7d1e3bd94828ad9fc86f55253cd6fec8edd65394
> 
> For this, I have:
> 
>     &i3c {
>         i3c_global: i3c-global {
>             compatible = "aspeed,ast2600-i3c-global", "simple-mfd",
> "syscon";
>             resets = <&syscon ASPEED_RESET_I3C_DMA>;
>             reg = <0x0 0x1000>;
>         };
> 
>         i3c2: i3c-master@4000 {
>             compatible = "aspeed,ast2600-i3c";
>             reg = <0x4000 0x1000>;
>             clocks = <&syscon ASPEED_CLK_GATE_I3C2CLK>;
>             pinctrl-names = "default";
>             pinctrl-0 = <&pinctrl_i3c3_default>;
>             interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
>             aspeed,global-regs = <&i3c_global 2>;
>             status = "disabled";
>         };
> 
>         /* ... */
>     };
> 
> - with no changes needed to any bindings. I haven't needed any other resets;
> are there per-controller resets specified in the HW docs you have?
> 
> Does that work for you? If you'd like to test, feel free to use my sample dts at:
> 
> 
> https://github.com/CodeConstruct/linux/commit/05cac24705fa62d2176ecbb
> bf15d955cfe86e753
> 
> Cheers,
> 
> 
> Jeremy
Dylan Hung Aug. 9, 2023, 3:05 a.m. UTC | #4 
Hi Jeremy,

> -----Original Message-----
> From: Jeremy Kerr [mailto:jk@codeconstruct.com.au]
> Sent: Wednesday, August 9, 2023 8:08 AM
> To: Dylan Hung <dylan_hung@aspeedtech.com>;
> alexandre.belloni@bootlin.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; joel@jms.id.au;
> andrew@aj.id.au; p.zabel@pengutronix.de; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>; kobedylan@gmail.com
> Subject: Re: [PATCH 0/3] Add Aspeed AST2600 I3C support
>
> Hi Dylan,
>
> > This patch series introduces the necessary changes to enable I3C
> > support for the Aspeed AST2600 I3C controller. Specifically, it
> > addresses the missing pinctrl configuration and reset control for the
> > I3C functionality.
>
> +1 for the pinctrl changes for the I3C1 and I3C2 controllers (I'll
> review and ack separately). I have been testing on I3C3 and up, but just not
> with the HVI3C on 1 & 2, hence no pinctrl definition there.

Thank you for your review. I3C1 and I3C2 can only operate in low voltage (1.0V/1.2V), which is why there are no HVI3C1 and HVI3C2 pinctrl definitions.

>
> However, I don't think the other two are needed.
>
> For 2/3 and 3/3, you're adding a reset control for the global register block
> within the per-controller driver, but we can already do that on a global basis
> with the existing syscon device. Hence this earlier change:
>

I followed your recommendation and verified that it worked on my end.
Should I resend the pinctrl patch as a stand-alone submission?

>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/dri
> vers/mfd/syscon.c?id=7d1e3bd94828ad9fc86f55253cd6fec8edd65394
>
> For this, I have:
>
>     &i3c {
>         i3c_global: i3c-global {
>             compatible = "aspeed,ast2600-i3c-global", "simple-mfd",
> "syscon";
>             resets = <&syscon ASPEED_RESET_I3C_DMA>;
>             reg = <0x0 0x1000>;
>         };
>
>         i3c2: i3c-master@4000 {
>             compatible = "aspeed,ast2600-i3c";
>             reg = <0x4000 0x1000>;
>             clocks = <&syscon ASPEED_CLK_GATE_I3C2CLK>;
>             pinctrl-names = "default";
>             pinctrl-0 = <&pinctrl_i3c3_default>;
>             interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
>             aspeed,global-regs = <&i3c_global 2>;
>             status = "disabled";
>         };
>
>         /* ... */
>     };
>
> - with no changes needed to any bindings. I haven't needed any other resets;
> are there per-controller resets specified in the HW docs you have?
>
> Does that work for you? If you'd like to test, feel free to use my sample dts at:
>
>
> https://github.com/CodeConstruct/linux/commit/05cac24705fa62d2176ecbb
> bf15d955cfe86e753
>
> Cheers,
>
>
> Jeremy
Jeremy Kerr Aug. 9, 2023, 3:12 a.m. UTC | #5 
Hi Dylan,

> Thank you for your review. I3C1 and I3C2 can only operate in low
> voltage (1.0V/1.2V), which is why there are no HVI3C1 and HVI3C2
> pinctrl definitions.

Yep, and that was config that I hadn't tested (so hadn't proposed
pinctrl definitions for those).

> > For 2/3 and 3/3, you're adding a reset control for the global
> > register block within the per-controller driver, but we can already
> > do that on a global basis with the existing syscon device. Hence
> > this earlier change:
>  
> I followed your recommendation and verified that it worked on my end.

OK, excellent!

> Should I resend the pinctrl patch as a stand-alone submission?

Yes, and feel free to add:

Reviewed-by: Jeremy Kerr <jk@codeconstruct.com.au>

Did your test use my i3c DTS definitions? If so, that's a decent
datapoint that the config works (on something other than my setup), and
so I'll submit upstream. Alternatively, feel free to include it with
your pinctrl change, if you like.

Cheers,


Jeremy