| Message ID | 20200328171144.51888-8-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: stm32: Repair AV96 board | expand |
Hi Marek On 3/28/20 6:11 PM, Marek Vasut wrote: > Add another mux option for SDMMC2 pins 4..7, this is used on AV96 board. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Patrice Chotard <patrice.chotard@st.com> > Cc: Patrick Delaunay <patrick.delaunay@st.com> > Cc: linux-stm32@st-md-mailman.stormreply.com > To: linux-arm-kernel@lists.infradead.org > --- > arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi > index bfd255dfe81f..6a37af213eb6 100644 > --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi > +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi > @@ -1062,6 +1062,27 @@ pins { > }; > }; > > + sdmmc2_d47_pins_b: sdmmc2-d47-1 { > + pins { > + pinmux = <STM32_PINMUX('A', 8, AF9)>, /* SDMMC2_D4 */ > + <STM32_PINMUX('A', 15, AF9)>, /* SDMMC2_D5 */ > + <STM32_PINMUX('C', 6, AF10)>, /* SDMMC2_D6 */ > + <STM32_PINMUX('C', 7, AF10)>; /* SDMMC2_D7 */ > + slew-rate = <1>; > + drive-push-pull; > + bias-pull-up; > + }; > + }; > + > + sdmmc2_d47_sleep_pins_b: sdmmc2-d47-sleep-1 { > + pins { > + pinmux = <STM32_PINMUX('A', 8, ANALOG)>, /* SDMMC2_D4 */ > + <STM32_PINMUX('A', 15, ANALOG)>, /* SDMMC2_D5 */ > + <STM32_PINMUX('C', 6, ANALOG)>, /* SDMMC2_D6 */ > + <STM32_PINMUX('C', 7, ANALOG)>; /* SDMMC2_D7 */ > + }; > + }; > + > sdmmc3_b4_pins_a: sdmmc3-b4-0 { > pins1 { > pinmux = <STM32_PINMUX('F', 0, AF9)>, /* SDMMC3_D0 */ For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) see https://lore.kernel.org/patchwork/patch/1216452/ I haven't checked other muxing if there are other conflict. Patrice
Hello Patrice, On 3/30/20 1:11 PM, Patrice CHOTARD wrote: > For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) > > see https://lore.kernel.org/patchwork/patch/1216452/ > > I haven't checked other muxing if there are other conflict. (author of linked patch here) I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred if each file defined the pinctrl groups it is using. Cheers Ahmad > > Patrice > _______________________________________________ > Linux-stm32 mailing list > Linux-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32 >
On 3/30/20 1:17 PM, Ahmad Fatoum wrote: > Hello Patrice, Hi, > On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >> For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >> >> see https://lore.kernel.org/patchwork/patch/1216452/ >> >> I haven't checked other muxing if there are other conflict. > > (author of linked patch here) > > I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred if each > file defined the pinctrl groups it is using. I'm not a big fan of that either, because this is gonna be a combinatorial explosion of various pinmux options. But if you have each board define it's pinmux, it's also gonna become a massive amount of duplication (like iMX). So I cannot tell which one is better ... So, just apply one patch or the other, let me know what got applied and I'll rebase on top of that and resubmit if needed.
Hi Marek, On 3/30/20 1:22 PM, Marek Vasut wrote: > On 3/30/20 1:17 PM, Ahmad Fatoum wrote: >> Hello Patrice, > > Hi, > >> On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >>> For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >>> >>> see https://lore.kernel.org/patchwork/patch/1216452/ >>> >>> I haven't checked other muxing if there are other conflict. >> >> (author of linked patch here) >> >> I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred if each >> file defined the pinctrl groups it is using. > > I'm not a big fan of that either, because this is gonna be a > combinatorial explosion of various pinmux options. But if you have each > board define it's pinmux, it's also gonna become a massive amount of > duplication (like iMX). So I cannot tell which one is better ... Mhm. A middle ground could be keeping stm32mp15-pinctrl, but only for the official ST eval kits as HW designers are expected to copy off those and have board specifics in the board/SoM device tree? If it has to be either one or the other, I prefer duplication in the device tree. When the HW misses pull ups or needs to adjust slew rates, you probably don't want a new, slightly different, pinctrl group in the stm32mp15-pinctrl.dtsi for each variant. So you are left with doctoring around with overrides and /delete-property/, while just duplicating the node with the correct properties would've been better for readability IMO. > So, just apply one patch or the other, let me know what got applied and > I'll rebase on top of that and resubmit if needed. Same. Cheers Ahmad
On 3/30/20 1:37 PM, Ahmad Fatoum wrote: > Hi Marek, Hi, > On 3/30/20 1:22 PM, Marek Vasut wrote: >> On 3/30/20 1:17 PM, Ahmad Fatoum wrote: >>> Hello Patrice, >> >> Hi, >> >>> On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >>>> For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >>>> >>>> see https://lore.kernel.org/patchwork/patch/1216452/ >>>> >>>> I haven't checked other muxing if there are other conflict. >>> >>> (author of linked patch here) >>> >>> I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred if each >>> file defined the pinctrl groups it is using. >> >> I'm not a big fan of that either, because this is gonna be a >> combinatorial explosion of various pinmux options. But if you have each >> board define it's pinmux, it's also gonna become a massive amount of >> duplication (like iMX). So I cannot tell which one is better ... > > Mhm. A middle ground could be keeping stm32mp15-pinctrl, but only for the > official ST eval kits as HW designers are expected to copy off those and have > board specifics in the board/SoM device tree? Then you should call it stm32mp1-something-st-eval-pinmux.dtsi , otherwise it's gonna be confusing. > If it has to be either one or the other, I prefer duplication in the device > tree. When the HW misses pull ups or needs to adjust slew rates, you probably > don't want a new, slightly different, pinctrl group in the stm32mp15-pinctrl.dtsi > for each variant. That's a valid point, but then you can override those in the boards' pinmux node for a specific pinmux entry too. > So you are left with doctoring around with overrides and /delete-property/, > while just duplicating the node with the correct properties would've been > better for readability IMO. That is true, but how many of such cases do we have so far ? Maybe it's better to cross that bridge when (if) we come to it.
On 3/30/20 1:45 PM, Marek Vasut wrote: > On 3/30/20 1:37 PM, Ahmad Fatoum wrote: >> Hi Marek, > > Hi, > >> On 3/30/20 1:22 PM, Marek Vasut wrote: >>> On 3/30/20 1:17 PM, Ahmad Fatoum wrote: >>>> Hello Patrice, >>> >>> Hi, >>> >>>> On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >>>>> For your information, another submitted patch uses the same pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >>>>> >>>>> see https://lore.kernel.org/patchwork/patch/1216452/ >>>>> >>>>> I haven't checked other muxing if there are other conflict. >>>> >>>> (author of linked patch here) >>>> >>>> I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred if each >>>> file defined the pinctrl groups it is using. >>> >>> I'm not a big fan of that either, because this is gonna be a >>> combinatorial explosion of various pinmux options. But if you have each >>> board define it's pinmux, it's also gonna become a massive amount of >>> duplication (like iMX). So I cannot tell which one is better ... >> >> Mhm. A middle ground could be keeping stm32mp15-pinctrl, but only for the >> official ST eval kits as HW designers are expected to copy off those and have >> board specifics in the board/SoM device tree? > > Then you should call it stm32mp1-something-st-eval-pinmux.dtsi , > otherwise it's gonna be confusing. > >> If it has to be either one or the other, I prefer duplication in the device >> tree. When the HW misses pull ups or needs to adjust slew rates, you probably >> don't want a new, slightly different, pinctrl group in the stm32mp15-pinctrl.dtsi >> for each variant. > > That's a valid point, but then you can override those in the boards' > pinmux node for a specific pinmux entry too. > >> So you are left with doctoring around with overrides and /delete-property/, >> while just duplicating the node with the correct properties would've been >> better for readability IMO. > > That is true, but how many of such cases do we have so far ? Maybe it's > better to cross that bridge when (if) we come to it. > I agree, and I prefer to keep pins groups definition in stm32mp15-pinctrl.dtsi file. IMO, it is easier for users to find them in only one file. Actually, I already had this discussions with some guys "where pins groups have to be defined ?". For me (and maybe only for me), muxing is SOC dependent, I mean SoC provides a bunch a possible pinmux for each IPs. If we got enough memory spaces (and time to waste also) we could define all possible pinmux (AFx....) for each devices and let board users chose the good one (using stm32mp15-pictrl.dtsi as a database). In board file, you select one possible pin configuration (provided by the SoC) for your device according to your schematic. However you could append pin groups in board file to update bias, slewrate ... If your concern it to embed a bunch of not used pin configuration for a board, we could use /omit-if-no-ref/ tag on pin groups. regards alex
On 3/31/20 10:58 AM, Alexandre Torgue wrote: > > > On 3/30/20 1:45 PM, Marek Vasut wrote: >> On 3/30/20 1:37 PM, Ahmad Fatoum wrote: >>> Hi Marek, >> >> Hi, >> >>> On 3/30/20 1:22 PM, Marek Vasut wrote: >>>> On 3/30/20 1:17 PM, Ahmad Fatoum wrote: >>>>> Hello Patrice, >>>> >>>> Hi, >>>> >>>>> On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >>>>>> For your information, another submitted patch uses the same >>>>>> pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >>>>>> >>>>>> see https://lore.kernel.org/patchwork/patch/1216452/ >>>>>> >>>>>> I haven't checked other muxing if there are other conflict. >>>>> >>>>> (author of linked patch here) >>>>> >>>>> I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred >>>>> if each >>>>> file defined the pinctrl groups it is using. >>>> >>>> I'm not a big fan of that either, because this is gonna be a >>>> combinatorial explosion of various pinmux options. But if you have each >>>> board define it's pinmux, it's also gonna become a massive amount of >>>> duplication (like iMX). So I cannot tell which one is better ... >>> >>> Mhm. A middle ground could be keeping stm32mp15-pinctrl, but only for >>> the >>> official ST eval kits as HW designers are expected to copy off those >>> and have >>> board specifics in the board/SoM device tree? >> >> Then you should call it stm32mp1-something-st-eval-pinmux.dtsi , >> otherwise it's gonna be confusing. >> >>> If it has to be either one or the other, I prefer duplication in the >>> device >>> tree. When the HW misses pull ups or needs to adjust slew rates, you >>> probably >>> don't want a new, slightly different, pinctrl group in the >>> stm32mp15-pinctrl.dtsi >>> for each variant. >> >> That's a valid point, but then you can override those in the boards' >> pinmux node for a specific pinmux entry too. >> >>> So you are left with doctoring around with overrides and >>> /delete-property/, >>> while just duplicating the node with the correct properties would've >>> been >>> better for readability IMO. >> >> That is true, but how many of such cases do we have so far ? Maybe it's >> better to cross that bridge when (if) we come to it. >> > > I agree, and I prefer to keep pins groups definition in > stm32mp15-pinctrl.dtsi file. IMO, it is easier for users to find them in > only one file. Actually, I already had this discussions with some guys > "where pins groups have to be defined ?". For me (and maybe only for > me), muxing is SOC dependent, I mean SoC provides a bunch a possible > pinmux for each IPs. If we got enough memory spaces (and time to waste > also) we could define all possible pinmux (AFx....) for each devices and > let board users chose the good one (using stm32mp15-pictrl.dtsi as a > database). In board file, you select one possible pin configuration > (provided by the SoC) for your device according to your schematic. > However you could append pin groups in board file to update bias, > slewrate ... > If your concern it to embed a bunch of not used pin configuration for a > board, we could use /omit-if-no-ref/ tag on pin groups. Can we instead define pinmux the way e.g. iMX6 does , as separate pins , instead of pinmux groups ?
On 3/31/20 3:38 PM, Marek Vasut wrote: > On 3/31/20 10:58 AM, Alexandre Torgue wrote: >> >> >> On 3/30/20 1:45 PM, Marek Vasut wrote: >>> On 3/30/20 1:37 PM, Ahmad Fatoum wrote: >>>> Hi Marek, >>> >>> Hi, >>> >>>> On 3/30/20 1:22 PM, Marek Vasut wrote: >>>>> On 3/30/20 1:17 PM, Ahmad Fatoum wrote: >>>>>> Hello Patrice, >>>>> >>>>> Hi, >>>>> >>>>>> On 3/30/20 1:11 PM, Patrice CHOTARD wrote: >>>>>>> For your information, another submitted patch uses the same >>>>>>> pinctrl sdmmc2_d47_pins_b node with different muxing (SDMMC2_D5) >>>>>>> >>>>>>> see https://lore.kernel.org/patchwork/patch/1216452/ >>>>>>> >>>>>>> I haven't checked other muxing if there are other conflict. >>>>>> >>>>>> (author of linked patch here) >>>>>> >>>>>> I don't like the central stm32mp15-pinctrl.dtsi. I'd have preferred >>>>>> if each >>>>>> file defined the pinctrl groups it is using. >>>>> >>>>> I'm not a big fan of that either, because this is gonna be a >>>>> combinatorial explosion of various pinmux options. But if you have each >>>>> board define it's pinmux, it's also gonna become a massive amount of >>>>> duplication (like iMX). So I cannot tell which one is better ... >>>> >>>> Mhm. A middle ground could be keeping stm32mp15-pinctrl, but only for >>>> the >>>> official ST eval kits as HW designers are expected to copy off those >>>> and have >>>> board specifics in the board/SoM device tree? >>> >>> Then you should call it stm32mp1-something-st-eval-pinmux.dtsi , >>> otherwise it's gonna be confusing. >>> >>>> If it has to be either one or the other, I prefer duplication in the >>>> device >>>> tree. When the HW misses pull ups or needs to adjust slew rates, you >>>> probably >>>> don't want a new, slightly different, pinctrl group in the >>>> stm32mp15-pinctrl.dtsi >>>> for each variant. >>> >>> That's a valid point, but then you can override those in the boards' >>> pinmux node for a specific pinmux entry too. >>> >>>> So you are left with doctoring around with overrides and >>>> /delete-property/, >>>> while just duplicating the node with the correct properties would've >>>> been >>>> better for readability IMO. >>> >>> That is true, but how many of such cases do we have so far ? Maybe it's >>> better to cross that bridge when (if) we come to it. >>> >> >> I agree, and I prefer to keep pins groups definition in >> stm32mp15-pinctrl.dtsi file. IMO, it is easier for users to find them in >> only one file. Actually, I already had this discussions with some guys >> "where pins groups have to be defined ?". For me (and maybe only for >> me), muxing is SOC dependent, I mean SoC provides a bunch a possible >> pinmux for each IPs. If we got enough memory spaces (and time to waste >> also) we could define all possible pinmux (AFx....) for each devices and >> let board users chose the good one (using stm32mp15-pictrl.dtsi as a >> database). In board file, you select one possible pin configuration >> (provided by the SoC) for your device according to your schematic. >> However you could append pin groups in board file to update bias, >> slewrate ... >> If your concern it to embed a bunch of not used pin configuration for a >> board, we could use /omit-if-no-ref/ tag on pin groups. > > Can we instead define pinmux the way e.g. iMX6 does , as separate pins , > instead of pinmux groups ? > Sorry but what would the advantage to do so ?
On 3/31/20 6:39 PM, Alexandre Torgue wrote: Hi, [...] >>> I agree, and I prefer to keep pins groups definition in >>> stm32mp15-pinctrl.dtsi file. IMO, it is easier for users to find them in >>> only one file. Actually, I already had this discussions with some guys >>> "where pins groups have to be defined ?". For me (and maybe only for >>> me), muxing is SOC dependent, I mean SoC provides a bunch a possible >>> pinmux for each IPs. If we got enough memory spaces (and time to waste >>> also) we could define all possible pinmux (AFx....) for each devices and >>> let board users chose the good one (using stm32mp15-pictrl.dtsi as a >>> database). In board file, you select one possible pin configuration >>> (provided by the SoC) for your device according to your schematic. >>> However you could append pin groups in board file to update bias, >>> slewrate ... >>> If your concern it to embed a bunch of not used pin configuration for a >>> board, we could use /omit-if-no-ref/ tag on pin groups. >> >> Can we instead define pinmux the way e.g. iMX6 does , as separate pins , >> instead of pinmux groups ? >> > > Sorry but what would the advantage to do so ? You'd have per-board pinmux which would be perfect fit for that board, without potentially affecting other boards with changes, without hacking various things like drive-strengths in board files, and without having the combinatorial explosion of the current single pinmux file.
On 3/31/20 6:44 PM, Marek Vasut wrote: > On 3/31/20 6:39 PM, Alexandre Torgue wrote: > > Hi, > > [...] > >>>> I agree, and I prefer to keep pins groups definition in >>>> stm32mp15-pinctrl.dtsi file. IMO, it is easier for users to find them in >>>> only one file. Actually, I already had this discussions with some guys >>>> "where pins groups have to be defined ?". For me (and maybe only for >>>> me), muxing is SOC dependent, I mean SoC provides a bunch a possible >>>> pinmux for each IPs. If we got enough memory spaces (and time to waste >>>> also) we could define all possible pinmux (AFx....) for each devices and >>>> let board users chose the good one (using stm32mp15-pictrl.dtsi as a >>>> database). In board file, you select one possible pin configuration >>>> (provided by the SoC) for your device according to your schematic. >>>> However you could append pin groups in board file to update bias, >>>> slewrate ... >>>> If your concern it to embed a bunch of not used pin configuration for a >>>> board, we could use /omit-if-no-ref/ tag on pin groups. >>> >>> Can we instead define pinmux the way e.g. iMX6 does , as separate pins , >>> instead of pinmux groups ? >>> >> >> Sorry but what would the advantage to do so ? > > You'd have per-board pinmux which would be perfect fit for that board, > without potentially affecting other boards with changes, without hacking > various things like drive-strengths in board files, and without having > the combinatorial explosion of the current single pinmux file. > It is something that we could analysis. Let's follow the way we currently use. I'll let u know, when I have a better view on your proposition. thanks alex
diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi index bfd255dfe81f..6a37af213eb6 100644 --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi @@ -1062,6 +1062,27 @@ pins { }; }; + sdmmc2_d47_pins_b: sdmmc2-d47-1 { + pins { + pinmux = <STM32_PINMUX('A', 8, AF9)>, /* SDMMC2_D4 */ + <STM32_PINMUX('A', 15, AF9)>, /* SDMMC2_D5 */ + <STM32_PINMUX('C', 6, AF10)>, /* SDMMC2_D6 */ + <STM32_PINMUX('C', 7, AF10)>; /* SDMMC2_D7 */ + slew-rate = <1>; + drive-push-pull; + bias-pull-up; + }; + }; + + sdmmc2_d47_sleep_pins_b: sdmmc2-d47-sleep-1 { + pins { + pinmux = <STM32_PINMUX('A', 8, ANALOG)>, /* SDMMC2_D4 */ + <STM32_PINMUX('A', 15, ANALOG)>, /* SDMMC2_D5 */ + <STM32_PINMUX('C', 6, ANALOG)>, /* SDMMC2_D6 */ + <STM32_PINMUX('C', 7, ANALOG)>; /* SDMMC2_D7 */ + }; + }; + sdmmc3_b4_pins_a: sdmmc3-b4-0 { pins1 { pinmux = <STM32_PINMUX('F', 0, AF9)>, /* SDMMC3_D0 */
Add another mux option for SDMMC2 pins 4..7, this is used on AV96 board. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@st.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Patrice Chotard <patrice.chotard@st.com> Cc: Patrick Delaunay <patrick.delaunay@st.com> Cc: linux-stm32@st-md-mailman.stormreply.com To: linux-arm-kernel@lists.infradead.org --- arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)