| Message ID | 20240425082138.374445-1-thomas.bonnefille@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add I2C support on TH1520 | expand |
On Thu, Apr 25, 2024 at 10:21:31AM +0200, Thomas Bonnefille wrote: > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > and a default configuration for the BeagleV-Ahead. It appears that the > TH1520 I2C is already supported in the upstream kernel through the > Synopsis Designware I2C adapter driver. > As there is no clock driver for this board as of today, this patch > series uses a fixed-clock named i2c_ic_clk. > There is also no pinctrl driver yet so pinmux must be handled manually > for now. https://lore.kernel.org/linux-riscv/20240103132852.298964-1-emil.renner.berthing@canonical.com/ https://lore.kernel.org/linux-riscv/20240110-clk-th1520-v1-0-8b0682567984@tenstorrent.com/ These seem to be the most recent versions of each. I guess they're both just waiting for their authors to have the time to get another version written.
On 4/25/24 11:00 AM, Conor Dooley wrote: > On Thu, Apr 25, 2024 at 10:21:31AM +0200, Thomas Bonnefille wrote: >> This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC >> and a default configuration for the BeagleV-Ahead. It appears that the >> TH1520 I2C is already supported in the upstream kernel through the >> Synopsis Designware I2C adapter driver. > >> As there is no clock driver for this board as of today, this patch >> series uses a fixed-clock named i2c_ic_clk. >> There is also no pinctrl driver yet so pinmux must be handled manually >> for now. > > https://lore.kernel.org/linux-riscv/20240103132852.298964-1-emil.renner.berthing@canonical.com/ > https://lore.kernel.org/linux-riscv/20240110-clk-th1520-v1-0-8b0682567984@tenstorrent.com/ > > These seem to be the most recent versions of each. I guess they're both > just waiting for their authors to have the time to get another version > written. Yes, actually, I used Emil's patch with a custom pinctrl configuration to test the I2C. It appeared to be ready enough for my use.
On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > and a default configuration for the BeagleV-Ahead. It appears that the > TH1520 I2C is already supported in the upstream kernel through the > Synopsis Designware I2C adapter driver. > As there is no clock driver for this board as of today, this patch > series uses a fixed-clock named i2c_ic_clk. > There is also no pinctrl driver yet so pinmux must be handled manually > for now. > It also fixes the order of the nodes in the device tree to comply with > device-tree coding-style. > > Thomas Bonnefille (4): > dt-bindings: i2c: dw: Document compatible thead,th1520-i2c > riscv: boot: dts: thead: Fix node ordering in TH1520 device tree > riscv: dts: thead: Add TH1520 I2C nodes > riscv: dts: thead: Enable I2C on the BeagleV-Ahead > > .../bindings/i2c/snps,designware-i2c.yaml | 12 ++ > .../boot/dts/thead/th1520-beaglev-ahead.dts | 22 ++++ > arch/riscv/boot/dts/thead/th1520.dtsi | 120 ++++++++++++++---- > 3 files changed, 127 insertions(+), 27 deletions(-) > > -- > 2.44.0 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 from schema $id: http://devicetree.org/schemas/i2c/i2c-controller.yaml#
On Thu, Apr 25, 2024 at 10:00:10AM +0100, Conor Dooley wrote: > On Thu, Apr 25, 2024 at 10:21:31AM +0200, Thomas Bonnefille wrote: > > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > > and a default configuration for the BeagleV-Ahead. It appears that the > > TH1520 I2C is already supported in the upstream kernel through the > > Synopsis Designware I2C adapter driver. > > > As there is no clock driver for this board as of today, this patch > > series uses a fixed-clock named i2c_ic_clk. > > There is also no pinctrl driver yet so pinmux must be handled manually > > for now. > > https://lore.kernel.org/linux-riscv/20240103132852.298964-1-emil.renner.berthing@canonical.com/ > https://lore.kernel.org/linux-riscv/20240110-clk-th1520-v1-0-8b0682567984@tenstorrent.com/ > > These seem to be the most recent versions of each. I guess they're both > just waiting for their authors to have the time to get another version > written. Thanks for the impetus. I've been meaning to post but it keeps getting pushed down the stack. I'll post an updated clk series today. I do now have the emmc working with a real clock property. thanks, drew
On Thu, Apr 25, 2024 at 09:51:26AM -0500, Rob Herring wrote: > > On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: > > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > > and a default configuration for the BeagleV-Ahead. It appears that the > > TH1520 I2C is already supported in the upstream kernel through the > > Synopsis Designware I2C adapter driver. > > As there is no clock driver for this board as of today, this patch > > series uses a fixed-clock named i2c_ic_clk. > > There is also no pinctrl driver yet so pinmux must be handled manually > > for now. > > It also fixes the order of the nodes in the device tree to comply with > > device-tree coding-style. > > > > Thomas Bonnefille (4): > > dt-bindings: i2c: dw: Document compatible thead,th1520-i2c > > riscv: boot: dts: thead: Fix node ordering in TH1520 device tree > > riscv: dts: thead: Add TH1520 I2C nodes > > riscv: dts: thead: Enable I2C on the BeagleV-Ahead > > > > .../bindings/i2c/snps,designware-i2c.yaml | 12 ++ > > .../boot/dts/thead/th1520-beaglev-ahead.dts | 22 ++++ > > arch/riscv/boot/dts/thead/th1520.dtsi | 120 ++++++++++++++---- > > 3 files changed, 127 insertions(+), 27 deletions(-) > > > > -- > > 2.44.0 > > > > > > > > > My bot found new DTB warnings on the .dts files added or changed in this > series. > > Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings > are fixed by another series. Ultimately, it is up to the platform > maintainer whether these warnings are acceptable or not. No need to reply > unless the platform maintainer has comments. > > If you already ran DT checks and didn't see these error(s), then > make sure dt-schema is up to date: > > pip3 install dtschema --upgrade > > > New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: > > arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 The bot is not freaking out here, 50 MHz is indeed more than 5 MHz :)
From: Conor Dooley <conor.dooley@microchip.com> On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > and a default configuration for the BeagleV-Ahead. It appears that the > TH1520 I2C is already supported in the upstream kernel through the > Synopsis Designware I2C adapter driver. > As there is no clock driver for this board as of today, this patch > series uses a fixed-clock named i2c_ic_clk. > There is also no pinctrl driver yet so pinmux must be handled manually > for now. > It also fixes the order of the nodes in the device tree to comply with > device-tree coding-style. > > [...] Applied to riscv-dt-for-next, thanks! [2/4] riscv: boot: dts: thead: Fix node ordering in TH1520 device tree (no commit info) I removed the "boot:" so b4 is probably confused. Thanks, Conor.
On 4/25/24 6:35 PM, Conor Dooley wrote: > On Thu, Apr 25, 2024 at 09:51:26AM -0500, Rob Herring wrote: >> >> On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: >>> This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC >>> and a default configuration for the BeagleV-Ahead. It appears that the >>> TH1520 I2C is already supported in the upstream kernel through the >>> Synopsis Designware I2C adapter driver. >>> As there is no clock driver for this board as of today, this patch >>> series uses a fixed-clock named i2c_ic_clk. >>> There is also no pinctrl driver yet so pinmux must be handled manually >>> for now. >>> It also fixes the order of the nodes in the device tree to comply with >>> device-tree coding-style. >>> >>> Thomas Bonnefille (4): >>> dt-bindings: i2c: dw: Document compatible thead,th1520-i2c >>> riscv: boot: dts: thead: Fix node ordering in TH1520 device tree >>> riscv: dts: thead: Add TH1520 I2C nodes >>> riscv: dts: thead: Enable I2C on the BeagleV-Ahead >>> >>> .../bindings/i2c/snps,designware-i2c.yaml | 12 ++ >>> .../boot/dts/thead/th1520-beaglev-ahead.dts | 22 ++++ >>> arch/riscv/boot/dts/thead/th1520.dtsi | 120 ++++++++++++++---- >>> 3 files changed, 127 insertions(+), 27 deletions(-) >>> >>> -- >>> 2.44.0 >>> >>> >>> >> >> >> My bot found new DTB warnings on the .dts files added or changed in this >> series. >> >> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings >> are fixed by another series. Ultimately, it is up to the platform >> maintainer whether these warnings are acceptable or not. No need to reply >> unless the platform maintainer has comments. >> >> If you already ran DT checks and didn't see these error(s), then >> make sure dt-schema is up to date: >> >> pip3 install dtschema --upgrade >> >> >> New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: >> >> arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 > > The bot is not freaking out here, 50 MHz is indeed more than 5 MHz :) 5MHz is the maximum clock-frequency, ie. the I2C bus frequency. This is actually set to 100kHz for I2C0 in the DT: &i2c0 { status = "okay"; clock-frequency = <100000>; <---- ... }; 50MHz is the "fixed-clock" frequency, that is the clock feeding the I2C IP block: i2c_ic_clk: i2c-clock { compatible = "fixed-clock"; clock-frequency = <50000000>; <----- #clock-cells = <0>; }; My guess is that the bot confused the clock-frequency parameter for the bus clock (SCL) with the i2c-ic-clock value for the controller itself during the checks. Do you agree with this or am I misunderstanding the error ? If I lower the fixed-clock frequency to eg. 100kHz, the error is gone. But I guess the 5MHz limit should probably not apply to the input clock? Best regards, Thomas
On Fri, Apr 26, 2024 at 04:12:00PM +0200, Thomas Bonnefille wrote: > > > On 4/25/24 6:35 PM, Conor Dooley wrote: > > On Thu, Apr 25, 2024 at 09:51:26AM -0500, Rob Herring wrote: > > > > > > On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: > > > > This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC > > > > and a default configuration for the BeagleV-Ahead. It appears that the > > > > TH1520 I2C is already supported in the upstream kernel through the > > > > Synopsis Designware I2C adapter driver. > > > > As there is no clock driver for this board as of today, this patch > > > > series uses a fixed-clock named i2c_ic_clk. > > > > There is also no pinctrl driver yet so pinmux must be handled manually > > > > for now. > > > > It also fixes the order of the nodes in the device tree to comply with > > > > device-tree coding-style. > > > > > > > > Thomas Bonnefille (4): > > > > dt-bindings: i2c: dw: Document compatible thead,th1520-i2c > > > > riscv: boot: dts: thead: Fix node ordering in TH1520 device tree > > > > riscv: dts: thead: Add TH1520 I2C nodes > > > > riscv: dts: thead: Enable I2C on the BeagleV-Ahead > > > > > > > > .../bindings/i2c/snps,designware-i2c.yaml | 12 ++ > > > > .../boot/dts/thead/th1520-beaglev-ahead.dts | 22 ++++ > > > > arch/riscv/boot/dts/thead/th1520.dtsi | 120 ++++++++++++++---- > > > > 3 files changed, 127 insertions(+), 27 deletions(-) > > > > > > > > -- > > > > 2.44.0 > > > > > > > > > > > > > > > > > > > > > My bot found new DTB warnings on the .dts files added or changed in this > > > series. > > > > > > Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings > > > are fixed by another series. Ultimately, it is up to the platform > > > maintainer whether these warnings are acceptable or not. No need to reply > > > unless the platform maintainer has comments. > > > > > > If you already ran DT checks and didn't see these error(s), then > > > make sure dt-schema is up to date: > > > > > > pip3 install dtschema --upgrade > > > > > > > > > New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: > > > > > > arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 > > > > The bot is not freaking out here, 50 MHz is indeed more than 5 MHz :) > > 5MHz is the maximum clock-frequency, ie. the I2C bus frequency. > This is actually set to 100kHz for I2C0 in the DT: > > &i2c0 { > status = "okay"; > clock-frequency = <100000>; <---- > ... > }; > > 50MHz is the "fixed-clock" frequency, that is the clock feeding the I2C > IP block: > > i2c_ic_clk: i2c-clock { > compatible = "fixed-clock"; > clock-frequency = <50000000>; <----- > #clock-cells = <0>; > }; > > My guess is that the bot confused the clock-frequency parameter for the bus > clock (SCL) with the i2c-ic-clock value for the controller itself during the > checks. > > Do you agree with this or am I misunderstanding the error ? > > If I lower the fixed-clock frequency to eg. 100kHz, the error is gone. But I > guess the 5MHz limit should probably not apply to the input clock? Heh, I know why that's happening - it's your node name. The pattern for i2c controllers is "^i2c(@.*)?": https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml Rob co-incidentally (or maybe not) put out a patch for fixed-frequency clock names, suggesting using clock as a prefix: https://lore.kernel.org/all/20240425183810.3079069-1-robh@kernel.org/ If we switched to that format, I believe your problem goes away. Cheers, Conor.
On 4/26/24 4:42 PM, Conor Dooley wrote: > On Fri, Apr 26, 2024 at 04:12:00PM +0200, Thomas Bonnefille wrote: >> >> >> On 4/25/24 6:35 PM, Conor Dooley wrote: >>> On Thu, Apr 25, 2024 at 09:51:26AM -0500, Rob Herring wrote: >>>> >>>> On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: >>>>> This adds I2C support in the device tree of the T-Head TH1520 RISCV-SoC >>>>> and a default configuration for the BeagleV-Ahead. It appears that the >>>>> TH1520 I2C is already supported in the upstream kernel through the >>>>> Synopsis Designware I2C adapter driver. >>>>> As there is no clock driver for this board as of today, this patch >>>>> series uses a fixed-clock named i2c_ic_clk. >>>>> There is also no pinctrl driver yet so pinmux must be handled manually >>>>> for now. >>>>> It also fixes the order of the nodes in the device tree to comply with >>>>> device-tree coding-style. >>>>> >>>>> Thomas Bonnefille (4): >>>>> dt-bindings: i2c: dw: Document compatible thead,th1520-i2c >>>>> riscv: boot: dts: thead: Fix node ordering in TH1520 device tree >>>>> riscv: dts: thead: Add TH1520 I2C nodes >>>>> riscv: dts: thead: Enable I2C on the BeagleV-Ahead >>>>> >>>>> .../bindings/i2c/snps,designware-i2c.yaml | 12 ++ >>>>> .../boot/dts/thead/th1520-beaglev-ahead.dts | 22 ++++ >>>>> arch/riscv/boot/dts/thead/th1520.dtsi | 120 ++++++++++++++---- >>>>> 3 files changed, 127 insertions(+), 27 deletions(-) >>>>> >>>>> -- >>>>> 2.44.0 >>>>> >>>>> >>>>> >>>> >>>> >>>> My bot found new DTB warnings on the .dts files added or changed in this >>>> series. >>>> >>>> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings >>>> are fixed by another series. Ultimately, it is up to the platform >>>> maintainer whether these warnings are acceptable or not. No need to reply >>>> unless the platform maintainer has comments. >>>> >>>> If you already ran DT checks and didn't see these error(s), then >>>> make sure dt-schema is up to date: >>>> >>>> pip3 install dtschema --upgrade >>>> >>>> >>>> New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: >>>> >>>> arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 >>> >>> The bot is not freaking out here, 50 MHz is indeed more than 5 MHz :) >> >> 5MHz is the maximum clock-frequency, ie. the I2C bus frequency. >> This is actually set to 100kHz for I2C0 in the DT: >> >> &i2c0 { >> status = "okay"; >> clock-frequency = <100000>; <---- >> ... >> }; >> >> 50MHz is the "fixed-clock" frequency, that is the clock feeding the I2C >> IP block: >> >> i2c_ic_clk: i2c-clock { >> compatible = "fixed-clock"; >> clock-frequency = <50000000>; <----- >> #clock-cells = <0>; >> }; >> >> My guess is that the bot confused the clock-frequency parameter for the bus >> clock (SCL) with the i2c-ic-clock value for the controller itself during the >> checks. >> >> Do you agree with this or am I misunderstanding the error ? >> >> If I lower the fixed-clock frequency to eg. 100kHz, the error is gone. But I >> guess the 5MHz limit should probably not apply to the input clock? > > Heh, I know why that's happening - it's your node name. > The pattern for i2c controllers is "^i2c(@.*)?": > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml > > Rob co-incidentally (or maybe not) put out a patch for fixed-frequency > clock names, suggesting using clock as a prefix: > https://lore.kernel.org/all/20240425183810.3079069-1-robh@kernel.org/ > If we switched to that format, I believe your problem goes away. > > Cheers, > Conor. > Indeed it was the node name, when I switched the name of the fixed clock to clock-i2c-ic, the error was gone, thank you. :) But i2c_ic_clk shouldn't match "^i2c(@.*)?", one of my teammates suggested that the error may instead come from this line https://elixir.bootlin.com/linux/latest/source/scripts/dtc/checks.c#L1023. But as Drew is working on new iterations of the TH1520's clock driver I will just delete this fixed clock and publish a second version depending on his patch. Thank you, Thomas
On Mon, Apr 29, 2024 at 03:07:55PM +0200, Thomas Bonnefille wrote: > On 4/26/24 4:42 PM, Conor Dooley wrote: > > On Fri, Apr 26, 2024 at 04:12:00PM +0200, Thomas Bonnefille wrote: > > > On 4/25/24 6:35 PM, Conor Dooley wrote: > > > > On Thu, Apr 25, 2024 at 09:51:26AM -0500, Rob Herring wrote: > > > > > On Thu, 25 Apr 2024 10:21:31 +0200, Thomas Bonnefille wrote: > > > > > New warnings running 'make CHECK_DTBS=y thead/th1520-beaglev-ahead.dtb' for 20240425082138.374445-1-thomas.bonnefille@bootlin.com: > > > > > > > > > > arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dtb: i2c-clock: clock-frequency:0:0: 50000000 is greater than the maximum of 5000000 > > > > > > > > The bot is not freaking out here, 50 MHz is indeed more than 5 MHz :) > > > > > > 5MHz is the maximum clock-frequency, ie. the I2C bus frequency. > > > This is actually set to 100kHz for I2C0 in the DT: > > > > > > &i2c0 { > > > status = "okay"; > > > clock-frequency = <100000>; <---- > > > ... > > > }; > > > > > > 50MHz is the "fixed-clock" frequency, that is the clock feeding the I2C > > > IP block: > > > > > > i2c_ic_clk: i2c-clock { > > > compatible = "fixed-clock"; > > > clock-frequency = <50000000>; <----- > > > #clock-cells = <0>; > > > }; > > > > > > My guess is that the bot confused the clock-frequency parameter for the bus > > > clock (SCL) with the i2c-ic-clock value for the controller itself during the > > > checks. > > > > > > Do you agree with this or am I misunderstanding the error ? > > > > > > If I lower the fixed-clock frequency to eg. 100kHz, the error is gone. But I > > > guess the 5MHz limit should probably not apply to the input clock? > > > > Heh, I know why that's happening - it's your node name. > > The pattern for i2c controllers is "^i2c(@.*)?": > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/i2c/i2c-controller.yaml > > > > Rob co-incidentally (or maybe not) put out a patch for fixed-frequency > > clock names, suggesting using clock as a prefix: > > https://lore.kernel.org/all/20240425183810.3079069-1-robh@kernel.org/ > > If we switched to that format, I believe your problem goes away. > Indeed it was the node name, when I switched the name of the fixed clock to > clock-i2c-ic, the error was gone, thank you. :) > But i2c_ic_clk shouldn't match "^i2c(@.*)?", one of my teammates suggested The pattern in i2c-controller.yaml is only applied to nodenames, the label (which is what i2c_ic_clk is) doesn't matter and could be left as-is. > that the error may instead come from this line > https://elixir.bootlin.com/linux/latest/source/scripts/dtc/checks.c#L1023. > > But as Drew is working on new iterations of the TH1520's clock driver I will > just delete this fixed clock and publish a second version depending on his > patch. Sure, but you may be waiting a while for that to land :) Cheers, Conor.