| Message ID | 20230808154241.749641-1-dylan_hung@aspeedtech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Aspeed AST2600 I3C support | expand |
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
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
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
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