2024-01-09 12:02:19

by Emil Renner Berthing

[permalink] [raw]

Subject: Re: [PATCH v2 3/8] riscv: dts: thead: Add TH1520 pin control nodes

Conor Dooley wrote:
> On Wed, Jan 03, 2024 at 02:28:40PM +0100, Emil Renner Berthing wrote:
> > Add nodes for pin controllers on the T-Head TH1520 RISC-V SoC.
> >
> > Signed-off-by: Emil Renner Berthing <[email protected]>
> > ---
> > .../boot/dts/thead/th1520-beaglev-ahead.dts | 4 ++++
> > .../dts/thead/th1520-lichee-module-4a.dtsi | 4 ++++
> > arch/riscv/boot/dts/thead/th1520.dtsi | 24 +++++++++++++++++++
> > 3 files changed, 32 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > index 70e8042c8304..6c56318a8705 100644
> > --- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > +++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > @@ -44,6 +44,10 @@ &osc_32k {
> > clock-frequency = <32768>;
> > };
> >
> > +&aonsys_clk {
> > + clock-frequency = <73728000>;
> > +};
> > +
> > &apb_clk {
> > clock-frequency = <62500000>;
> > };
> > diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > index a802ab110429..9865925be372 100644
> > --- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > +++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > @@ -25,6 +25,10 @@ &osc_32k {
> > clock-frequency = <32768>;
> > };
> >
> > +&aonsys_clk {
> > + clock-frequency = <73728000>;
> > +};
> > +
> > &apb_clk {
> > clock-frequency = <62500000>;
> > };
> > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > index ba4d2c673ac8..e65a306ff575 100644
> > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > @@ -134,6 +134,12 @@ osc_32k: 32k-oscillator {
> > #clock-cells = <0>;
> > };
> >
> > + aonsys_clk: aonsys-clk {
> > + compatible = "fixed-clock";
> > + clock-output-names = "aonsys_clk";
> > + #clock-cells = <0>;
> > + };
>
> Did this stuff sneak into this commit accidentally?

Not really by accident no. It turns out the clock tree has gates for the bus
clock of each pinctrl block and I think it's better to add this clock
dependency to the bindings and driver up front.

Since there is not yet any clock driver the initial device tree for the TH1520
included the dummy apb_clk that two of the pinctrl blocks derive their clock
from, but not the "aonsys" clock needed by the "always-on" pinctrl. I thought
it was better to add this dummy clock with the only (so far) user of it, but if
you have a better idea, let me know.

/Emil

2024-01-09 13:09:41

On Tue, Jan 09, 2024 at 06:28:19AM -0800, Emil Renner Berthing wrote:
> Conor Dooley wrote:
> > On Tue, Jan 09, 2024 at 04:02:01AM -0800, Emil Renner Berthing wrote:
> > > Conor Dooley wrote:
> > > > On Wed, Jan 03, 2024 at 02:28:40PM +0100, Emil Renner Berthing wrote:
> > > > > Add nodes for pin controllers on the T-Head TH1520 RISC-V SoC.
> > > > >
> > > > > Signed-off-by: Emil Renner Berthing <[email protected]>
> > > > > ---
> > > > > .../boot/dts/thead/th1520-beaglev-ahead.dts | 4 ++++
> > > > > .../dts/thead/th1520-lichee-module-4a.dtsi | 4 ++++
> > > > > arch/riscv/boot/dts/thead/th1520.dtsi | 24 +++++++++++++++++++
> > > > > 3 files changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > index 70e8042c8304..6c56318a8705 100644
> > > > > --- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > +++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > @@ -44,6 +44,10 @@ &osc_32k {
> > > > > clock-frequency = <32768>;
> > > > > };
> > > > >
> > > > > +&aonsys_clk {
> > > > > + clock-frequency = <73728000>;
> > > > > +};
> > > > > +
> > > > > &apb_clk {
> > > > > clock-frequency = <62500000>;
> > > > > };
> > > > > diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > index a802ab110429..9865925be372 100644
> > > > > --- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > +++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > @@ -25,6 +25,10 @@ &osc_32k {
> > > > > clock-frequency = <32768>;
> > > > > };
> > > > >
> > > > > +&aonsys_clk {
> > > > > + clock-frequency = <73728000>;
> > > > > +};
> > > > > +
> > > > > &apb_clk {
> > > > > clock-frequency = <62500000>;
> > > > > };
> > > > > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > index ba4d2c673ac8..e65a306ff575 100644
> > > > > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > @@ -134,6 +134,12 @@ osc_32k: 32k-oscillator {
> > > > > #clock-cells = <0>;
> > > > > };
> > > > >
> > > > > + aonsys_clk: aonsys-clk {
> > > > > + compatible = "fixed-clock";
> > > > > + clock-output-names = "aonsys_clk";
> > > > > + #clock-cells = <0>;
> > > > > + };
> > > >
> > > > Did this stuff sneak into this commit accidentally?
> > >
> > > Not really by accident no. It turns out the clock tree has gates for the bus
> > > clock of each pinctrl block and I think it's better to add this clock
> > > dependency to the bindings and driver up front.
> >
> > Maybe if I had looked a wee bit more deeply I would've noticed that it
> > was used there, but it's always good to mention the rationale in the
> > commit message so that it's more obvious why you're doin it.
>
> You absolutely right. I forgot to update the commit message.
>
> > > Since there is not yet any clock driver the initial device tree for the TH1520
> > > included the dummy apb_clk that two of the pinctrl blocks derive their clock
> > > from, but not the "aonsys" clock needed by the "always-on" pinctrl. I thought
> > > it was better to add this dummy clock with the only (so far) user of it, but if
> > > you have a better idea, let me know.
> >
> > No, that's fine. I was just wondering why there was an unmentioned set
> > of clocks being added. If they're stubbed fixed clocks I dunno if it
> > makes sense to add them to the board.dts/module.dtsi files though. Where
> > do the initial values come from for the rates? Out of reset values or
> > set by firmware that may vary from board to board?
>
> The vendor u-boot sets the PLLs different from the reset values. For now I
> think it's the same code for every board using the Lichee Pi 4A module (and
> probably also for the BeagleV Ahead), but it might still make sense to move the
> freqency to the board instead of the module device tree.

Yeah, think so. Only temporarily though, do you have a clue if anyone is
working on the actual clock driver stuff? Seems pretty Deadge?
https://lore.kernel.org/linux-clk/?q=th1520

Cheers,
Conor.

Attachments:

(No filename) (4.25 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-09 18:30:43

by Drew Fustini

[permalink] [raw]

Subject: Re: [PATCH v2 3/8] riscv: dts: thead: Add TH1520 pin control nodes

On Tue, Jan 09, 2024 at 05:34:11PM +0000, Conor Dooley wrote:
> On Tue, Jan 09, 2024 at 06:28:19AM -0800, Emil Renner Berthing wrote:
> > Conor Dooley wrote:
> > > On Tue, Jan 09, 2024 at 04:02:01AM -0800, Emil Renner Berthing wrote:
> > > > Conor Dooley wrote:
> > > > > On Wed, Jan 03, 2024 at 02:28:40PM +0100, Emil Renner Berthing wrote:
> > > > > > Add nodes for pin controllers on the T-Head TH1520 RISC-V SoC.
> > > > > >
> > > > > > Signed-off-by: Emil Renner Berthing <[email protected]>
> > > > > > ---
> > > > > > .../boot/dts/thead/th1520-beaglev-ahead.dts | 4 ++++
> > > > > > .../dts/thead/th1520-lichee-module-4a.dtsi | 4 ++++
> > > > > > arch/riscv/boot/dts/thead/th1520.dtsi | 24 +++++++++++++++++++
> > > > > > 3 files changed, 32 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > > index 70e8042c8304..6c56318a8705 100644
> > > > > > --- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > > +++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> > > > > > @@ -44,6 +44,10 @@ &osc_32k {
> > > > > > clock-frequency = <32768>;
> > > > > > };
> > > > > >
> > > > > > +&aonsys_clk {
> > > > > > + clock-frequency = <73728000>;
> > > > > > +};
> > > > > > +
> > > > > > &apb_clk {
> > > > > > clock-frequency = <62500000>;
> > > > > > };
> > > > > > diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > > index a802ab110429..9865925be372 100644
> > > > > > --- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > > +++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > > > > > @@ -25,6 +25,10 @@ &osc_32k {
> > > > > > clock-frequency = <32768>;
> > > > > > };
> > > > > >
> > > > > > +&aonsys_clk {
> > > > > > + clock-frequency = <73728000>;
> > > > > > +};
> > > > > > +
> > > > > > &apb_clk {
> > > > > > clock-frequency = <62500000>;
> > > > > > };
> > > > > > diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > > index ba4d2c673ac8..e65a306ff575 100644
> > > > > > --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > > +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> > > > > > @@ -134,6 +134,12 @@ osc_32k: 32k-oscillator {
> > > > > > #clock-cells = <0>;
> > > > > > };
> > > > > >
> > > > > > + aonsys_clk: aonsys-clk {
> > > > > > + compatible = "fixed-clock";
> > > > > > + clock-output-names = "aonsys_clk";
> > > > > > + #clock-cells = <0>;
> > > > > > + };
> > > > >
> > > > > Did this stuff sneak into this commit accidentally?
> > > >
> > > > Not really by accident no. It turns out the clock tree has gates for the bus
> > > > clock of each pinctrl block and I think it's better to add this clock
> > > > dependency to the bindings and driver up front.
> > >
> > > Maybe if I had looked a wee bit more deeply I would've noticed that it
> > > was used there, but it's always good to mention the rationale in the
> > > commit message so that it's more obvious why you're doin it.
> >
> > You absolutely right. I forgot to update the commit message.
> >
> > > > Since there is not yet any clock driver the initial device tree for the TH1520
> > > > included the dummy apb_clk that two of the pinctrl blocks derive their clock
> > > > from, but not the "aonsys" clock needed by the "always-on" pinctrl. I thought
> > > > it was better to add this dummy clock with the only (so far) user of it, but if
> > > > you have a better idea, let me know.
> > >
> > > No, that's fine. I was just wondering why there was an unmentioned set
> > > of clocks being added. If they're stubbed fixed clocks I dunno if it
> > > makes sense to add them to the board.dts/module.dtsi files though. Where
> > > do the initial values come from for the rates? Out of reset values or
> > > set by firmware that may vary from board to board?
> >
> > The vendor u-boot sets the PLLs different from the reset values. For now I
> > think it's the same code for every board using the Lichee Pi 4A module (and
> > probably also for the BeagleV Ahead), but it might still make sense to move the
> > freqency to the board instead of the module device tree.
>
> Yeah, think so. Only temporarily though, do you have a clue if anyone is
> working on the actual clock driver stuff? Seems pretty Deadge?
> https://lore.kernel.org/linux-clk/?q=th1520

Yes, I am working on it. Jisheng passed me his work-in-progress based on
that original patch you linked to. I've been trying to work out an issue
with the emmc clock but it seems timely to share what I currently have.
I will post an RFC today.

Thanks,
Drew

2024-01-15 17:37:19

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings

On Wed, Jan 03, 2024 at 02:28:38PM +0100, Emil Renner Berthing wrote:
> Add bindings for the pin controllers on the T-Head TH1520 RISC-V SoC.
>
> Signed-off-by: Emil Renner Berthing <[email protected]>
> ---
> .../pinctrl/thead,th1520-pinctrl.yaml | 372 ++++++++++++++++++
> 1 file changed, 372 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> new file mode 100644
> index 000000000000..d3ad7a7cfdd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> @@ -0,0 +1,372 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/thead,th1520-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: T-Head TH1520 SoC pin controller
> +
> +maintainers:
> + - Emil Renner Berthing <[email protected]>
> +
> +description: |
> + Pinmux and pinconf controller in the T-Head TH1520 RISC-V SoC.
> +
> + The TH1520 has 3 groups of pads each controlled from different memory ranges.
> + Confusingly the memory ranges are named
> + PADCTRL_AOSYS -> PAD Group 1
> + PADCTRL1_APSYS -> PAD Group 2
> + PADCTRL0_APSYS -> PAD Group 3
> +
> + Each pad can be muxed individually to up to 6 different functions. For most
> + pads only a few of those 6 configurations are valid though, and a few pads in
> + group 1 does not support muxing at all.
> +
> + Pinconf is fairly regular except for a few pads in group 1 that either can't
> + be configured or has some special functions. The rest have configurable drive
> + strength, input enable, schmitt trigger, slew rate, pull-up and pull-down in
> + addition to a special strong pull up.
> +
> + Certain pads in group 1 can be muxed to AUDIO_PA0 - AUDIO_PA30 functions and
> + are then meant to be used by the audio co-processor. Each such pad can then
> + be further muxed to either audio GPIO or one of 4 functions such as UART, I2C
> + and I2S. If the audio pad is muxed to one of the 4 functions then pinconf is
> + also configured in different registers. All of this is done from a different
> + AUDIO_IOCTRL memory range and is left to the audio co-processor for now.

It is still not clear to me if each instance is a different programming
model or the same with just different connections. The latter should
be the same compatible string. That needs to be answered in *this*
patch, not a reply.

> +
> +properties:
> + compatible:
> + enum:
> + - thead,th1520-group1-pinctrl
> + - thead,th1520-group2-pinctrl
> + - thead,th1520-group3-pinctrl
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> +patternProperties:
> + '-[0-9]+$':
> + type: object
> +
> + additionalProperties: false
> +
> + patternProperties:
> + '-pins$':
> + type: object
> + $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> + additionalProperties: false
> +
> + description:
> + A pinctrl node should contain at least one subnode describing one
> + or more pads and their associated pinmux and pinconf settings.
> +
> + properties:
> + pins:
> + $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pins
> + description: List of pads that properties in the node apply to.
> +
> + function:
> + $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/function
> + enum: [ gpio, pwm, uart, ir, i2c, spi, qspi, sdio, audio, i2s,
> + gmac0, gmac1, dpu0, dpu1, isp, hdmi, bootsel, debug,
> + clock, jtag, iso7816, efuse, reset ]
> + description: The mux function to select for the given pins.
> +
> + bias-disable: true
> +
> + bias-pull-up:
> + oneOf:
> + - type: boolean
> + description: Enable the regular 48kOhm pull-up
> + - enum: [ 2100, 48000 ]
> + description: Enable the strong 2.1kOhm pull-up or regular 48kOhm pull-up
> +
> + bias-pull-down:
> + oneOf:
> + - type: boolean
> + - const: 44000
> + description: Enable the regular 44kOhm pull-down
> +
> + drive-strength:
> + enum: [ 1, 2, 3, 5, 7, 8, 10, 12, 13, 15, 16, 18, 20, 21, 23, 25 ]
> + description: Drive strength in mA
> +
> + input-enable: true
> +
> + input-disable: true
> +
> + input-schmitt-enable: true
> +
> + input-schmitt-disable: true
> +
> + slew-rate:
> + maximum: 1
> +
> + required:
> + - pins
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> +
> +additionalProperties: false
> +
> +allOf:
> + - $ref: pinctrl.yaml#
> + - if:
> + properties:
> + compatible:
> + const: thead,th1520-group1-pinctrl
> + then:
> + patternProperties:
> + '-[0-9]+$':
> + patternProperties:
> + '-pins$':
> + properties:
> + pins:
> + items:
> + enum:
> + - OSC_CLK_IN
> + - OSC_CLK_OUT
> + - SYS_RST_N
> + - RTC_CLK_IN
> + - RTC_CLK_OUT
> + - TEST_MODE
> + - DEBUG_MODE
> + - POR_SEL
> + - I2C_AON_SCL
> + - I2C_AON_SDA
> + - CPU_JTG_TCLK
> + - CPU_JTG_TMS
> + - CPU_JTG_TDI
> + - CPU_JTG_TDO
> + - CPU_JTG_TRST
> + - AOGPIO_7
> + - AOGPIO_8
> + - AOGPIO_9
> + - AOGPIO_10
> + - AOGPIO_11
> + - AOGPIO_12
> + - AOGPIO_13
> + - AOGPIO_14
> + - AOGPIO_15
> + - AUDIO_PA0
> + - AUDIO_PA1
> + - AUDIO_PA2
> + - AUDIO_PA3
> + - AUDIO_PA4
> + - AUDIO_PA5
> + - AUDIO_PA6
> + - AUDIO_PA7
> + - AUDIO_PA8
> + - AUDIO_PA9
> + - AUDIO_PA10
> + - AUDIO_PA11
> + - AUDIO_PA12
> + - AUDIO_PA13
> + - AUDIO_PA14
> + - AUDIO_PA15
> + - AUDIO_PA16
> + - AUDIO_PA17
> + - AUDIO_PA27
> + - AUDIO_PA28
> + - AUDIO_PA29
> + - AUDIO_PA30
> + - if:
> + properties:
> + compatible:
> + const: thead,th1520-group2-pinctrl
> + then:
> + patternProperties:
> + '-[0-9]+$':
> + patternProperties:
> + '-pins$':
> + properties:
> + pins:
> + items:
> + enum:
> + - QSPI1_SCLK
> + - QSPI1_CSN0
> + - QSPI1_D0_MOSI
> + - QSPI1_D1_MISO
> + - QSPI1_D2_WP
> + - QSPI1_D3_HOLD
> + - I2C0_SCL
> + - I2C0_SDA
> + - I2C1_SCL
> + - I2C1_SDA
> + - UART1_TXD
> + - UART1_RXD
> + - UART4_TXD
> + - UART4_RXD
> + - UART4_CTSN
> + - UART4_RTSN
> + - UART3_TXD
> + - UART3_RXD
> + - GPIO0_18
> + - GPIO0_19
> + - GPIO0_20
> + - GPIO0_21
> + - GPIO0_22
> + - GPIO0_23
> + - GPIO0_24
> + - GPIO0_25
> + - GPIO0_26
> + - GPIO0_27
> + - GPIO0_28
> + - GPIO0_29
> + - GPIO0_30
> + - GPIO0_31
> + - GPIO1_0
> + - GPIO1_1
> + - GPIO1_2
> + - GPIO1_3
> + - GPIO1_4
> + - GPIO1_5
> + - GPIO1_6
> + - GPIO1_7
> + - GPIO1_8
> + - GPIO1_9
> + - GPIO1_10
> + - GPIO1_11
> + - GPIO1_12
> + - GPIO1_13
> + - GPIO1_14
> + - GPIO1_15
> + - GPIO1_16
> + - CLK_OUT_0
> + - CLK_OUT_1
> + - CLK_OUT_2
> + - CLK_OUT_3
> + - GPIO1_21
> + - GPIO1_22
> + - GPIO1_23
> + - GPIO1_24
> + - GPIO1_25
> + - GPIO1_26
> + - GPIO1_27
> + - GPIO1_28
> + - GPIO1_29
> + - GPIO1_30
> + - if:
> + properties:
> + compatible:
> + const: thead,th1520-group3-pinctrl
> + then:
> + patternProperties:
> + '-[0-9]+$':
> + patternProperties:
> + '-pins$':
> + properties:
> + pins:
> + items:
> + enum:
> + - UART0_TXD
> + - UART0_RXD
> + - QSPI0_SCLK
> + - QSPI0_CSN0
> + - QSPI0_CSN1
> + - QSPI0_D0_MOSI
> + - QSPI0_D1_MISO
> + - QSPI0_D2_WP
> + - QSPI1_D3_HOLD
> + - I2C2_SCL
> + - I2C2_SDA
> + - I2C3_SCL
> + - I2C3_SDA
> + - GPIO2_13
> + - SPI_SCLK
> + - SPI_CSN
> + - SPI_MOSI
> + - SPI_MISO
> + - GPIO2_18
> + - GPIO2_19
> + - GPIO2_20
> + - GPIO2_21
> + - GPIO2_22
> + - GPIO2_23
> + - GPIO2_24
> + - GPIO2_25
> + - SDIO0_WPRTN
> + - SDIO0_DETN
> + - SDIO1_WPRTN
> + - SDIO1_DETN
> + - GPIO2_30
> + - GPIO2_31
> + - GPIO3_0
> + - GPIO3_1
> + - GPIO3_2
> + - GPIO3_3
> + - HDMI_SCL
> + - HDMI_SDA
> + - HDMI_CEC
> + - GMAC0_TX_CLK
> + - GMAC0_RX_CLK
> + - GMAC0_TXEN
> + - GMAC0_TXD0
> + - GMAC0_TXD1
> + - GMAC0_TXD2
> + - GMAC0_TXD3
> + - GMAC0_RXDV
> + - GMAC0_RXD0
> + - GMAC0_RXD1
> + - GMAC0_RXD2
> + - GMAC0_RXD3
> + - GMAC0_MDC
> + - GMAC0_MDIO
> + - GMAC0_COL
> + - GMAC0_CRS
> +
> +examples:
> + - |
> + padctrl0_apsys: pinctrl@ec007000 {
> + compatible = "thead,th1520-group3-pinctrl";
> + reg = <0xec007000 0x1000>;
> + clocks = <&apb_clk>;
> +
> + uart0_pins: uart0-0 {
> + tx-pins {
> + pins = "UART0_TXD";
> + function = "uart";
> + bias-disable;
> + drive-strength = <3>;
> + input-disable;
> + input-schmitt-disable;
> + slew-rate = <0>;
> + };
> +
> + rx-pins {
> + pins = "UART0_RXD";
> + function = "uart";
> + bias-disable;
> + drive-strength = <1>;
> + input-enable;
> + input-schmitt-enable;
> + slew-rate = <0>;
> + };
> + };
> + };
> +
> + padctrl1_apsys: pinctrl@e7f3c000 {
> + compatible = "thead,th1520-group2-pinctrl";
> + reg = <0xe7f3c000 0x1000>;
> + clocks = <&apb_clk>;
> +
> + i2c5_pins: i2c5-0 {
> + i2c-pins {
> + pins = "QSPI1_CSN0", /* I2C5_SCL */
> + "QSPI1_D0_MOSI"; /* I2C5_SDA */
> + function = "i2c";
> + bias-pull-up = <2100>;
> + drive-strength = <7>;
> + input-enable;
> + input-schmitt-enable;
> + slew-rate = <0>;
> + };
> + };
> + };
> --
> 2.43.0
>

2024-01-27 23:02:59

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v2 2/8] pinctrl: Add driver for the T-Head TH1520 SoC

On Wed, Jan 3, 2024 at 2:29 PM Emil Renner Berthing
<[email protected]> wrote:

> Add pinctrl driver for the T-Head TH1520 RISC-V SoC.
>
> Signed-off-by: Emil Renner Berthing <[email protected]>

This driver looks mostly fine but I am waiting for the bindings
to be hashed out.

Yours,
Linus Walleij

2024-02-06 19:25:28

Rob Herring wrote:
> On Wed, Jan 03, 2024 at 02:28:38PM +0100, Emil Renner Berthing wrote:
> > Add bindings for the pin controllers on the T-Head TH1520 RISC-V SoC.
> >
> > Signed-off-by: Emil Renner Berthing <[email protected]>
> > ---
> > .../pinctrl/thead,th1520-pinctrl.yaml | 372 ++++++++++++++++++
> > 1 file changed, 372 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..d3ad7a7cfdd1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
> > @@ -0,0 +1,372 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/thead,th1520-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: T-Head TH1520 SoC pin controller
> > +
> > +maintainers:
> > + - Emil Renner Berthing <[email protected]>
> > +
> > +description: |
> > + Pinmux and pinconf controller in the T-Head TH1520 RISC-V SoC.
> > +
> > + The TH1520 has 3 groups of pads each controlled from different memory ranges.
> > + Confusingly the memory ranges are named
> > + PADCTRL_AOSYS -> PAD Group 1
> > + PADCTRL1_APSYS -> PAD Group 2
> > + PADCTRL0_APSYS -> PAD Group 3
> > +
> > + Each pad can be muxed individually to up to 6 different functions. For most
> > + pads only a few of those 6 configurations are valid though, and a few pads in
> > + group 1 does not support muxing at all.
> > +
> > + Pinconf is fairly regular except for a few pads in group 1 that either can't
> > + be configured or has some special functions. The rest have configurable drive
> > + strength, input enable, schmitt trigger, slew rate, pull-up and pull-down in
> > + addition to a special strong pull up.
> > +
> > + Certain pads in group 1 can be muxed to AUDIO_PA0 - AUDIO_PA30 functions and
> > + are then meant to be used by the audio co-processor. Each such pad can then
> > + be further muxed to either audio GPIO or one of 4 functions such as UART, I2C
> > + and I2S. If the audio pad is muxed to one of the 4 functions then pinconf is
> > + also configured in different registers. All of this is done from a different
> > + AUDIO_IOCTRL memory range and is left to the audio co-processor for now.
>
> It is still not clear to me if each instance is a different programming
> model or the same with just different connections. The latter should
> be the same compatible string. That needs to be answered in *this*
> patch, not a reply.

Hi Rob,

Sorry for the late response. I honestly don't know exactly what you mean by
differenty programming models and what the difference is, so I'll need a bit of
help with what you want me to write here.

Any driver for the TH1520 SoC (not just Linux) would need some way to discern
between the 3 pin controllers so they know how many pins to control and what
pinmux settings are valid. Basically they'd need the data in the three
th1520_group{1,2,3}_pins arrays in the driver and a way to know which of them
to use.

https://lore.kernel.org/linux-riscv/[email protected]/

Another solution would be to just have one compatible value, and then let the
driver figure out which of the 3 pin controllers it's probing from the base
physical memory address. That would work fine.

A third solution would be to encode the data in those three arrays into the
device tree, but I thought we didn't want to encode register information in
device trees.

/Emil