| Message ID | 1473235141-12768-5-git-send-email-jorik@kippendief.biz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 7, 2016 at 3:58 PM, <jorik@kippendief.biz> wrote: > From: Jorik Jonker <jorik@kippendief.biz> > > Users using this UART without RTS/CTS should override the association in > their board specific DTS. All (1) board using this UART uses RTS/CTS, so > this breaks nothing. I'd prefer we only do this for peripherals that have absolutely one possible setting, like RSB, or I2C with only one group of pins. UARTs with 2/4/8 pin configurations don't fall in this category. ChenYu
On Wed, Sep 07, 2016 at 09:58:57AM +0200, jorik@kippendief.biz wrote: > From: Jorik Jonker <jorik@kippendief.biz> > > Users using this UART without RTS/CTS should override the association in > their board specific DTS. All (1) board using this UART uses RTS/CTS, so > this breaks nothing. Using RTS / CTS is very rare among the boards. Forcing it down the throat of every user doesn't seem like the right thing to do. Maxime
Maxime, Chen-Yu: thanks for taking the effort to go through my patches again! On Thu, Sep 08, 2016 at 08:23:17AM +0200, Maxime Ripard wrote: >On Wed, Sep 07, 2016 at 09:58:57AM +0200, jorik@kippendief.biz wrote: >> From: Jorik Jonker <jorik@kippendief.biz> >> >> Users using this UART without RTS/CTS should override the association in >> their board specific DTS. All (1) board using this UART uses RTS/CTS, so >> this breaks nothing. > >Using RTS / CTS is very rare among the boards. Forcing it down the >throat of every user doesn't seem like the right thing to do. So, I'm going for a v5, with these changes: - rename uart0_pins to uart0_pa_pins (as there could be a pf) - associate uart0_pa_pins with uart0 on all H3 board DTS files - put rts/cts in seperate pinmux sets for uart1 (2,3: see below) - associate rx/tx for uart1-3 in H3 DTSI (this is the only option) - associate UART1 rts/cts as pinctrl-1 in sun8i-h3-bananapi-m2-plus (to prevent breakage for existing users) I am a bit in doubt if I should include pinmux definitions for the following things, as Chen-Yu said to only include stuff that is actually used in a board: - uart0_pf_pins, since there is no board using it - uart{2,3}_rts_cts, as I agree RTS/CTS is a bit exotic Maxime/Chen-Yu: what do you think of this? Best, Jorik
On Thu, Sep 08, 2016 at 10:02:13AM +0200, Jorik Jonker wrote: > Maxime, Chen-Yu: thanks for taking the effort to go through my patches > again! > > On Thu, Sep 08, 2016 at 08:23:17AM +0200, Maxime Ripard wrote: > >On Wed, Sep 07, 2016 at 09:58:57AM +0200, jorik@kippendief.biz wrote: > >>From: Jorik Jonker <jorik@kippendief.biz> > >> > >>Users using this UART without RTS/CTS should override the association in > >>their board specific DTS. All (1) board using this UART uses RTS/CTS, so > >>this breaks nothing. > > > >Using RTS / CTS is very rare among the boards. Forcing it down the > >throat of every user doesn't seem like the right thing to do. > > So, I'm going for a v5, with these changes: > - rename uart0_pins to uart0_pa_pins (as there could be a pf) > - associate uart0_pa_pins with uart0 on all H3 board DTS files Please don't. We use that naming scheme everywhere else. Plus, nothing prevents any one from using one PF pin and one PA pin. > - put rts/cts in seperate pinmux sets for uart1 (2,3: see below) > - associate rx/tx for uart1-3 in H3 DTSI (this is the only option) I'm still a bit skeptical about this. This wouldn't be in any way consistant. I prefer to have something consistant and a bit duplicated over something without any duplication but that confuses everyone about what should be placed where. > - associate UART1 rts/cts as pinctrl-1 in sun8i-h3-bananapi-m2-plus > (to prevent breakage for existing users) You can also set it in pinctrl-0. > I am a bit in doubt if I should include pinmux definitions for the following > things, as Chen-Yu said to only include stuff that is actually used in a > board: > > - uart0_pf_pins, since there is no board using it > - uart{2,3}_rts_cts, as I agree RTS/CTS is a bit exotic Don't Maxime
On Thu, Sep 08, 2016 at 11:01:53AM +0200, Maxime Ripard wrote: >On Thu, Sep 08, 2016 at 10:02:13AM +0200, Jorik Jonker wrote: >> So, I'm going for a v5, with these changes: >> - rename uart0_pins to uart0_pa_pins (as there could be a pf) >> - associate uart0_pa_pins with uart0 on all H3 board DTS files > >Please don't. We use that naming scheme everywhere else. Plus, nothing >prevents any one from using one PF pin and one PA pin. OK, I will leave uart0 untouched, that's a good point. >> - put rts/cts in seperate pinmux sets for uart1 (2,3: see below) >> - associate rx/tx for uart1-3 in H3 DTSI (this is the only option) > >I'm still a bit skeptical about this. This wouldn't be in any way >consistant. I prefer to have something consistant and a bit duplicated >over something without any duplication but that confuses everyone >about what should be placed where. > >> - associate UART1 rts/cts as pinctrl-1 in sun8i-h3-bananapi-m2-plus >> (to prevent breakage for existing users) > >You can also set it in pinctrl-0. OK, sounds reasonable, but also a bit contradictive. One the one hand you prefer consistency (so, let uart2-3 follow uart1 and include rts/cts in them), on the other hand the common case over the rare (so split off rts/cts). What should I do with uarts2-3 and should I do that to uart1 too? Moreover, Chen-Yu prefers to drop _a and @0 when they are redundant, which does not appear to be the convention, looking at existing sun*dsti. What's your opinion on this? Best, Jorik
On Thu, Sep 08, 2016 at 11:51:09AM +0200, Jorik Jonker wrote: > >>- put rts/cts in seperate pinmux sets for uart1 (2,3: see below) > >>- associate rx/tx for uart1-3 in H3 DTSI (this is the only option) > > > >I'm still a bit skeptical about this. This wouldn't be in any way > >consistant. I prefer to have something consistant and a bit duplicated > >over something without any duplication but that confuses everyone > >about what should be placed where. > > > >>- associate UART1 rts/cts as pinctrl-1 in sun8i-h3-bananapi-m2-plus > >> (to prevent breakage for existing users) > > > >You can also set it in pinctrl-0. > > OK, sounds reasonable, but also a bit contradictive. One the one hand you > prefer consistency (so, let uart2-3 follow uart1 and include rts/cts in > them) Hmm, I never said that, quite the opposite actually. Any board might use either just RX/TX, or RX/TX and RTS/CTS. I don't see why we should enable RTS/CTS on any board by default. > , on the other hand the common case over the rare (so split off > rts/cts). What should I do with uarts2-3 and should I do that to > uart1 too? You do the exact same thing in both cases. My point was that you could just do: pinctrl-0 = <&uart0_pins_a>, <&uart0_rts_cts_pins_a>; pinctrl-names = "default"; instead of pinctrl-0 = <&uart0_pins_a>; pinctrl-1 = <&uart0_rts_cts_pins_a>; pinctrl-names = "default", "default"; Since they are the exact same pin state. > Moreover, Chen-Yu prefers to drop _a and @0 when they are redundant, which > does not appear to be the convention, looking at existing sun*dsti. What's > your opinion on this? AFAIK, he wanted to remove them when they're not relevant (ie, only one pin state possible). Maxime
diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts index c874270..4c86e45 100644 --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts @@ -182,8 +182,6 @@ }; &uart1 { - pinctrl-names = "default"; - pinctrl-0 = <&uart1_rts_cts_pins>; status = "okay"; }; diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index 6635f3d..685755c 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -414,6 +414,8 @@ resets = <&ccu RST_BUS_UART1>; dmas = <&dma 7>, <&dma 7>; dma-names = "rx", "tx"; + pinctrl-names = "default"; + pinctrl-0 = <&uart1_rts_cts_pins>; status = "disabled"; };