* [PATCH 0/6] Use correct LDO5 control registers for PCA9450 @ 2023-02-13 15:58 Frieder Schrempf 2023-02-13 15:58 ` [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios Frieder Schrempf 2023-02-13 15:58 ` [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal Frieder Schrempf 0 siblings, 2 replies; 18+ messages in thread From: Frieder Schrempf @ 2023-02-13 15:58 UTC (permalink / raw) To: devicetree, Frieder Schrempf, linux-arm-kernel, linux-kernel, Mark Brown, Robin Gong Cc: Marek Vasut, Krzysztof Kozlowski, Liam Girdwood, Rob Herring, Sascha Hauer, Shawn Guo From: Frieder Schrempf <frieder.schrempf@kontron.de> This patchset fixes the control of the LDO5 regulator by providing an option for letting the driver know which of the two possible control registers is currently in use by the hardware. It also fixes the enable register for LDO5 to use PCA9450_REG_LDO5CTRL_L as specified by the datasheet. The last patch makes use of the fix by adjusting the devicetree for the Kontron i.MX8MM boards. In Linux this currently doesn't fix any functional issues, but in U-Boot similar changes are needed in order to fix SD card access. See the following thread for more information: https://lists.denx.de/pipermail/u-boot/2023-January/506103.html Frieder Schrempf (6): dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios regulator: pca9450: Fix enable register for LDO5 Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5" regulator: Add operation to let drivers select vsel register regulator: pca9450: Fix control register for LDO5 arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++--- .../dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +-- .../boot/dts/freescale/imx8mm-kontron-bl.dts | 6 +-- .../dts/freescale/imx8mm-kontron-osm-s.dtsi | 1 + .../boot/dts/freescale/imx8mm-kontron-sl.dtsi | 1 + drivers/regulator/helpers.c | 16 ++++++- drivers/regulator/pca9450-regulator.c | 47 ++++++++++++++----- include/linux/regulator/driver.h | 5 ++ 8 files changed, 79 insertions(+), 26 deletions(-) -- 2.39.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios 2023-02-13 15:58 [PATCH 0/6] Use correct LDO5 control registers for PCA9450 Frieder Schrempf @ 2023-02-13 15:58 ` Frieder Schrempf 2023-02-15 20:02 ` Rob Herring 2023-02-13 15:58 ` [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal Frieder Schrempf 1 sibling, 1 reply; 18+ messages in thread From: Frieder Schrempf @ 2023-02-13 15:58 UTC (permalink / raw) To: devicetree, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, Mark Brown, Rob Herring, Robin Gong Cc: Marek Vasut, Frieder Schrempf, Per-Daniel Olsson, Rickard x Andersson From: Frieder Schrempf <frieder.schrempf@kontron.de> The sd-vsel-gpios property is abandoned in its current meaning as an output. We now use it to specify an optional signal that can be evaluated by the driver in order to retrieve the current status of the SD_VSEL signal that is used to select the control register of LDO5. Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> --- .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml index 835b53302db8..c86534538a4e 100644 --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml @@ -40,8 +40,24 @@ properties: description: | list of regulators provided by this controller + properties: + LDO5: + type: object + $ref: regulator.yaml# + description: + Properties for single LDO5 regulator. + + properties: + sd-vsel-gpios: + description: + GPIO that can be used to read the current status of the SD_VSEL + signal in order for the driver to know if LDO5CTRL_L or LDO5CTRL_H + is used by the hardware. + + unevaluatedProperties: false + patternProperties: - "^LDO[1-5]$": + "^LDO[1-4]$": type: object $ref: regulator.yaml# description: @@ -76,11 +92,6 @@ properties: additionalProperties: false - sd-vsel-gpios: - description: GPIO that is used to switch LDO5 between being configured by - LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is - connected to a host GPIO. - nxp,i2c-lt-enable: type: boolean description: -- 2.39.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios 2023-02-13 15:58 ` [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios Frieder Schrempf @ 2023-02-15 20:02 ` Rob Herring 2023-02-16 1:27 ` Marek Vasut 0 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2023-02-15 20:02 UTC (permalink / raw) To: Frieder Schrempf Cc: devicetree, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, Mark Brown, Robin Gong, Marek Vasut, Frieder Schrempf, Per-Daniel Olsson, Rickard x Andersson On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > The sd-vsel-gpios property is abandoned in its current meaning as an > output. We now use it to specify an optional signal that can be > evaluated by the driver in order to retrieve the current status > of the SD_VSEL signal that is used to select the control register > of LDO5. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++++++++++----- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > index 835b53302db8..c86534538a4e 100644 > --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > @@ -40,8 +40,24 @@ properties: > description: | > list of regulators provided by this controller > > + properties: > + LDO5: > + type: object > + $ref: regulator.yaml# > + description: > + Properties for single LDO5 regulator. > + > + properties: > + sd-vsel-gpios: It is a pin on the device, right? Then it belongs in the device node as it was. Can't the direction of the signal tell you how it is used? Assuming the pin is bidirectional? The binding should support any possible way the device is wired, not just what's been seen so far on some boards. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios 2023-02-15 20:02 ` Rob Herring @ 2023-02-16 1:27 ` Marek Vasut 2023-02-16 2:30 ` Rob Herring 0 siblings, 1 reply; 18+ messages in thread From: Marek Vasut @ 2023-02-16 1:27 UTC (permalink / raw) To: Rob Herring, Frieder Schrempf Cc: devicetree, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, Mark Brown, Robin Gong, Frieder Schrempf, Per-Daniel Olsson, Rickard x Andersson On 2/15/23 21:02, Rob Herring wrote: > On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote: >> From: Frieder Schrempf <frieder.schrempf@kontron.de> >> >> The sd-vsel-gpios property is abandoned in its current meaning as an >> output. We now use it to specify an optional signal that can be >> evaluated by the driver in order to retrieve the current status >> of the SD_VSEL signal that is used to select the control register >> of LDO5. >> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >> --- >> .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++++++++++----- >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >> index 835b53302db8..c86534538a4e 100644 >> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >> @@ -40,8 +40,24 @@ properties: >> description: | >> list of regulators provided by this controller >> >> + properties: >> + LDO5: >> + type: object >> + $ref: regulator.yaml# >> + description: >> + Properties for single LDO5 regulator. >> + >> + properties: >> + sd-vsel-gpios: > > It is a pin on the device, right? Then it belongs in the device node as > it was. > > Can't the direction of the signal tell you how it is used? Assuming the > pin is bidirectional? The pin is input to the PMIC, it is unidirection, i.e. SoC(output)---->(input)PMIC > The binding should support any possible way the device is wired, not > just what's been seen so far on some boards. The usage is always the above as far as I can tell. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios 2023-02-16 1:27 ` Marek Vasut @ 2023-02-16 2:30 ` Rob Herring 2023-02-16 10:15 ` Frieder Schrempf 0 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2023-02-16 2:30 UTC (permalink / raw) To: Marek Vasut Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, Mark Brown, Robin Gong, Frieder Schrempf, Per-Daniel Olsson, Rickard x Andersson On Wed, Feb 15, 2023 at 7:27 PM Marek Vasut <marex@denx.de> wrote: > > On 2/15/23 21:02, Rob Herring wrote: > > On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote: > >> From: Frieder Schrempf <frieder.schrempf@kontron.de> > >> > >> The sd-vsel-gpios property is abandoned in its current meaning as an > >> output. We now use it to specify an optional signal that can be > >> evaluated by the driver in order to retrieve the current status > >> of the SD_VSEL signal that is used to select the control register > >> of LDO5. > >> > >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > >> --- > >> .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++++++++++----- > >> 1 file changed, 17 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > >> index 835b53302db8..c86534538a4e 100644 > >> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > >> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml > >> @@ -40,8 +40,24 @@ properties: > >> description: | > >> list of regulators provided by this controller > >> > >> + properties: > >> + LDO5: > >> + type: object > >> + $ref: regulator.yaml# > >> + description: > >> + Properties for single LDO5 regulator. > >> + > >> + properties: > >> + sd-vsel-gpios: > > > > It is a pin on the device, right? Then it belongs in the device node as > > it was. > > > > Can't the direction of the signal tell you how it is used? Assuming the > > pin is bidirectional? > > The pin is input to the PMIC, it is unidirection, i.e. > > SoC(output)---->(input)PMIC > > > The binding should support any possible way the device is wired, not > > just what's been seen so far on some boards. > > The usage is always the above as far as I can tell. This patch is saying the opposite though. Something else drives the signal, but the signal is also routed to the SoC to sample the state. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios 2023-02-16 2:30 ` Rob Herring @ 2023-02-16 10:15 ` Frieder Schrempf 0 siblings, 0 replies; 18+ messages in thread From: Frieder Schrempf @ 2023-02-16 10:15 UTC (permalink / raw) To: Rob Herring, Marek Vasut Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, Liam Girdwood, linux-kernel, Mark Brown, Robin Gong, Per-Daniel Olsson, Rickard x Andersson On 16.02.23 03:30, Rob Herring wrote: > On Wed, Feb 15, 2023 at 7:27 PM Marek Vasut <marex@denx.de> wrote: >> >> On 2/15/23 21:02, Rob Herring wrote: >>> On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote: >>>> From: Frieder Schrempf <frieder.schrempf@kontron.de> >>>> >>>> The sd-vsel-gpios property is abandoned in its current meaning as an >>>> output. We now use it to specify an optional signal that can be >>>> evaluated by the driver in order to retrieve the current status >>>> of the SD_VSEL signal that is used to select the control register >>>> of LDO5. >>>> >>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>>> --- >>>> .../regulator/nxp,pca9450-regulator.yaml | 23 ++++++++++++++----- >>>> 1 file changed, 17 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >>>> index 835b53302db8..c86534538a4e 100644 >>>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >>>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml >>>> @@ -40,8 +40,24 @@ properties: >>>> description: | >>>> list of regulators provided by this controller >>>> >>>> + properties: >>>> + LDO5: >>>> + type: object >>>> + $ref: regulator.yaml# >>>> + description: >>>> + Properties for single LDO5 regulator. >>>> + >>>> + properties: >>>> + sd-vsel-gpios: >>> >>> It is a pin on the device, right? Then it belongs in the device node as >>> it was. Physically it's a pin on the PCA9450 chip. If you look at the block diagram in the datasheet [1] (page 3) you can see though, that the SD_VSEL signal is routed to the LD05 regulator block inside the chip. This makes me think that the signal is best described inside the LDO5 node. >>> >>> Can't the direction of the signal tell you how it is used? Assuming the >>> pin is bidirectional? >> >> The pin is input to the PMIC, it is unidirection, i.e. >> >> SoC(output)---->(input)PMIC >> >>> The binding should support any possible way the device is wired, not >>> just what's been seen so far on some boards. >> >> The usage is always the above as far as I can tell. There is only one usage that is likely to occur and that is the one we describe here. There are other ways to wire up the signal of course and in some unlikely event a hardware engineer might have the idea to hard-wire the SD_VSEL to a fixed level or wire it up to a SoC pin that doesn't have the VSELECT mux option. But I don't really see a good reason for covering these cases in the binding/driver if there are good chances we won't ever need them. > This patch is saying the opposite though. Something else drives the > signal, but the signal is also routed to the SoC to sample the state. SoC PMIC +-----------------------+ +-------------------+ | | | | | | | | | GPIO <----------+ | | | | | | SD_VSEL| +-------+ | | USHC_VSELECT -->+------------------->| LDO5 | | | | | +-------+ | | | | | +-----------------------+ +-------------------+ This is how the setup looks like. The SD_VSEL on the PMIC is always an input. It's driven by the SoC's VSELECT signal (controlled by the USDHC controller) and we use the SION bit in the IOMUX to internally loop back the signal in order to sample it using the GPIO. [1] https://www.nxp.com/docs/en/data-sheet/PCA9450DS.pdf ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 15:58 [PATCH 0/6] Use correct LDO5 control registers for PCA9450 Frieder Schrempf 2023-02-13 15:58 ` [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios Frieder Schrempf @ 2023-02-13 15:58 ` Frieder Schrempf 2023-02-13 16:08 ` Marek Vasut 2023-02-13 16:15 ` Marco Felsch 1 sibling, 2 replies; 18+ messages in thread From: Frieder Schrempf @ 2023-02-13 15:58 UTC (permalink / raw) To: devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo Cc: Marek Vasut, Frieder Schrempf, Fabio Estevam, Heiko Thiery, Krzysztof Kozlowski, NXP Linux Team, Oleksij Rempel, Pengutronix Kernel Team From: Frieder Schrempf <frieder.schrempf@kontron.de> This fixes the LDO5 regulator handling of the pca9450 driver by taking the status of the SD_VSEL into account to determine which configuration register is used for the voltage setting. Even without this change there is no functional issue, as the code for switching the voltage in sdhci.c currently switches both, the VSELECT/SD_VSEL signal and the regulator voltage at the same time and doesn't run into an invalid corner case. We should still make sure, that we always use the correct register when controlling the regulator. At least in U-Boot this fixes an actual bug where the wrong IO voltage is used. Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> --- arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++--- arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts | 6 +++--- arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi | 1 + arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi | 1 + 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts index 8b16bd68576c..bdcd9cd843c7 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts index a079322a3793..ba2a4491cf31 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi index 5172883717d1..90daaf54e704 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { regulator-name = "NVCC_SD (LDO5)"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; }; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi index 1f8326613ee9..7468a8aa771d 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi @@ -195,6 +195,7 @@ reg_nvcc_sd: LDO5 { regulator-name = "NVCC_SD (LDO5)"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; }; }; }; -- 2.39.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 15:58 ` [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal Frieder Schrempf @ 2023-02-13 16:08 ` Marek Vasut 2023-02-13 16:15 ` Frieder Schrempf 2023-02-13 16:15 ` Marco Felsch 1 sibling, 1 reply; 18+ messages in thread From: Marek Vasut @ 2023-02-13 16:08 UTC (permalink / raw) To: Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo Cc: Frieder Schrempf, Fabio Estevam, Heiko Thiery, Krzysztof Kozlowski, NXP Linux Team, Oleksij Rempel, Pengutronix Kernel Team On 2/13/23 16:58, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > This fixes the LDO5 regulator handling of the pca9450 driver by > taking the status of the SD_VSEL into account to determine which > configuration register is used for the voltage setting. > > Even without this change there is no functional issue, as the code > for switching the voltage in sdhci.c currently switches both, the > VSELECT/SD_VSEL signal and the regulator voltage at the same time > and doesn't run into an invalid corner case. > > We should still make sure, that we always use the correct register > when controlling the regulator. At least in U-Boot this fixes an > actual bug where the wrong IO voltage is used. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> I may have a kind-of obvious request, since we removed those SD_VSEL very recently from other boards, could you just revert all those patches and only fill in the SION bit in V2 on all those boards too ? That way, we fix the LDO5 regulator for everyone who used it before. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 16:08 ` Marek Vasut @ 2023-02-13 16:15 ` Frieder Schrempf 0 siblings, 0 replies; 18+ messages in thread From: Frieder Schrempf @ 2023-02-13 16:15 UTC (permalink / raw) To: Marek Vasut, Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo Cc: Fabio Estevam, Heiko Thiery, Krzysztof Kozlowski, NXP Linux Team, Oleksij Rempel, Pengutronix Kernel Team On 13.02.23 17:08, Marek Vasut wrote: > On 2/13/23 16:58, Frieder Schrempf wrote: >> From: Frieder Schrempf <frieder.schrempf@kontron.de> >> >> This fixes the LDO5 regulator handling of the pca9450 driver by >> taking the status of the SD_VSEL into account to determine which >> configuration register is used for the voltage setting. >> >> Even without this change there is no functional issue, as the code >> for switching the voltage in sdhci.c currently switches both, the >> VSELECT/SD_VSEL signal and the regulator voltage at the same time >> and doesn't run into an invalid corner case. >> >> We should still make sure, that we always use the correct register >> when controlling the regulator. At least in U-Boot this fixes an >> actual bug where the wrong IO voltage is used. >> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > I may have a kind-of obvious request, since we removed those SD_VSEL > very recently from other boards, could you just revert all those patches > and only fill in the SION bit in V2 on all those boards too ? That way, > we fix the LDO5 regulator for everyone who used it before. I thought about that, but as the SD_VSEL signal is controlling the LDO5, I found it more useful/correct to have the sd-vsel-gpios property inside the LDO5 regulator node. Therefore the old usage where sd-vsel-gpios was inside the PMIC node isn't compatible anymore. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 15:58 ` [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal Frieder Schrempf 2023-02-13 16:08 ` Marek Vasut @ 2023-02-13 16:15 ` Marco Felsch 2023-02-13 16:18 ` Frieder Schrempf 2023-02-13 18:12 ` Marek Vasut 1 sibling, 2 replies; 18+ messages in thread From: Marco Felsch @ 2023-02-13 16:15 UTC (permalink / raw) To: Frieder Schrempf Cc: devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Marek Vasut, Frieder Schrempf, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam Hi Frieder, thanks for the patch. On 23-02-13, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > This fixes the LDO5 regulator handling of the pca9450 driver by > taking the status of the SD_VSEL into account to determine which > configuration register is used for the voltage setting. > > Even without this change there is no functional issue, as the code > for switching the voltage in sdhci.c currently switches both, the > VSELECT/SD_VSEL signal and the regulator voltage at the same time > and doesn't run into an invalid corner case. > > We should still make sure, that we always use the correct register > when controlling the regulator. At least in U-Boot this fixes an > actual bug where the wrong IO voltage is used. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++--- > arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts | 6 +++--- > arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi | 1 + > arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi | 1 + > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts > index 8b16bd68576c..bdcd9cd843c7 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts > @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >; > }; > > @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >; > }; > > @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >; > }; > }; > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts > index a079322a3793..ba2a4491cf31 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts > @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >; > }; > > @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >; > }; > > @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 The VSELECT pin should be driven by the (u)sdhc core... > >; > }; > }; > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > index 5172883717d1..90daaf54e704 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { > regulator-name = "NVCC_SD (LDO5)"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <3300000>; > + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be muxed as GPIO, which is not the case. So I think that u-boot have a bug within the (u)sdhc core. Regards, Marco > }; > }; > }; > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi > index 1f8326613ee9..7468a8aa771d 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi > @@ -195,6 +195,7 @@ reg_nvcc_sd: LDO5 { > regulator-name = "NVCC_SD (LDO5)"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <3300000>; > + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > }; > }; > }; > -- > 2.39.1 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 16:15 ` Marco Felsch @ 2023-02-13 16:18 ` Frieder Schrempf 2023-02-13 21:02 ` Fabio Estevam 2023-02-13 18:12 ` Marek Vasut 1 sibling, 1 reply; 18+ messages in thread From: Frieder Schrempf @ 2023-02-13 16:18 UTC (permalink / raw) To: Marco Felsch, Frieder Schrempf Cc: devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Marek Vasut, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam On 13.02.23 17:15, Marco Felsch wrote: > Hi Frieder, > > thanks for the patch. > > On 23-02-13, Frieder Schrempf wrote: >> From: Frieder Schrempf <frieder.schrempf@kontron.de> >> >> This fixes the LDO5 regulator handling of the pca9450 driver by >> taking the status of the SD_VSEL into account to determine which >> configuration register is used for the voltage setting. >> >> Even without this change there is no functional issue, as the code >> for switching the voltage in sdhci.c currently switches both, the >> VSELECT/SD_VSEL signal and the regulator voltage at the same time >> and doesn't run into an invalid corner case. >> >> We should still make sure, that we always use the correct register >> when controlling the regulator. At least in U-Boot this fixes an >> actual bug where the wrong IO voltage is used. >> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> >> --- >> arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++--- >> arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts | 6 +++--- >> arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi | 1 + >> arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi | 1 + >> 4 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts >> index 8b16bd68576c..bdcd9cd843c7 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts >> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts >> @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >> >; >> }; >> >> @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >> >; >> }; >> >> @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >> >; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts >> index a079322a3793..ba2a4491cf31 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts >> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts >> @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d0 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d0 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d0 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >> >; >> }; >> >> @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d4 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d4 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d4 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >> >; >> }; >> >> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > > The VSELECT pin should be driven by the (u)sdhc core... > >> >; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> index 5172883717d1..90daaf54e704 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { >> regulator-name = "NVCC_SD (LDO5)"; >> regulator-min-microvolt = <1800000>; >> regulator-max-microvolt = <3300000>; >> + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be > muxed as GPIO, which is not the case. So I think that u-boot have a bug > within the (u)sdhc core. No, we don't mux the signal as GPIO. We still use the VSLECT mux option, but enable the SION bit, so we can read back the current state of the VSELECT signal via the GPIO. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 16:18 ` Frieder Schrempf @ 2023-02-13 21:02 ` Fabio Estevam 0 siblings, 0 replies; 18+ messages in thread From: Fabio Estevam @ 2023-02-13 21:02 UTC (permalink / raw) To: Frieder Schrempf Cc: Marco Felsch, Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Marek Vasut, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery Hi Frieder, On Mon, Feb 13, 2023 at 1:19 PM Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > No, we don't mux the signal as GPIO. We still use the VSLECT mux option, > but enable the SION bit, so we can read back the current state of the > VSELECT signal via the GPIO. Please include this explanation in the commit log. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 16:15 ` Marco Felsch 2023-02-13 16:18 ` Frieder Schrempf @ 2023-02-13 18:12 ` Marek Vasut 2023-02-13 19:56 ` Marco Felsch 1 sibling, 1 reply; 18+ messages in thread From: Marek Vasut @ 2023-02-13 18:12 UTC (permalink / raw) To: Marco Felsch, Frieder Schrempf Cc: devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Frieder Schrempf, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam On 2/13/23 17:15, Marco Felsch wrote: [...] >> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 >> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 >> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 >> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > > The VSELECT pin should be driven by the (u)sdhc core... > >> >; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> index 5172883717d1..90daaf54e704 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { >> regulator-name = "NVCC_SD (LDO5)"; >> regulator-min-microvolt = <1800000>; >> regulator-max-microvolt = <3300000>; >> + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be > muxed as GPIO, which is not the case. So I think that u-boot have a bug > within the (u)sdhc core. The trick here is that the VSELECT is operated by the usdhc block as a function pin, but the PMIC driver can read the current state of the VSELECT pin by reading out the GPIO block SR register. Since the IOMUX SION bit is set on the VSELECT pin, the state of the pin is reflected in the GPIO block SR register even if the pin is muxed as function pin. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 18:12 ` Marek Vasut @ 2023-02-13 19:56 ` Marco Felsch 2023-02-13 20:59 ` Marek Vasut 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2023-02-13 19:56 UTC (permalink / raw) To: Marek Vasut Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Frieder Schrempf, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam Hi Marek, Frieder, On 23-02-13, Marek Vasut wrote: > On 2/13/23 17:15, Marco Felsch wrote: > > [...] > > > > @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 > > > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 > > > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 > > > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > > > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > > > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > > > > The VSELECT pin should be driven by the (u)sdhc core... > > > > > >; > > > }; > > > }; > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > index 5172883717d1..90daaf54e704 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { > > > regulator-name = "NVCC_SD (LDO5)"; > > > regulator-min-microvolt = <1800000>; > > > regulator-max-microvolt = <3300000>; > > > + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > > > > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be > > muxed as GPIO, which is not the case. So I think that u-boot have a bug > > within the (u)sdhc core. > > The trick here is that the VSELECT is operated by the usdhc block as a > function pin, but the PMIC driver can read the current state of the VSELECT > pin by reading out the GPIO block SR register. Since the IOMUX SION bit is > set on the VSELECT pin, the state of the pin is reflected in the GPIO block > SR register even if the pin is muxed as function pin. > Thanks for this explanation :) Why does the regulator driver need to know the current state of this pin? Since the voltage switching requires some cmd's before the actual voltage level switch. So this must be handled within the core. Also after checking the driver, adding the sd-vsel-gpios will request the specified gpio as output-high. Out of curiosity, what's the bug you triggering within U-Boot? Regards, Marco ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 19:56 ` Marco Felsch @ 2023-02-13 20:59 ` Marek Vasut 2023-02-14 8:10 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Marek Vasut @ 2023-02-13 20:59 UTC (permalink / raw) To: Marco Felsch Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Frieder Schrempf, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam On 2/13/23 20:56, Marco Felsch wrote: > Hi Marek, Frieder, Hi, > On 23-02-13, Marek Vasut wrote: >> On 2/13/23 17:15, Marco Felsch wrote: >> >> [...] >> >>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 >>>> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 >>>> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 >>>> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >>>> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >>>> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >>> >>> The VSELECT pin should be driven by the (u)sdhc core... >>> >>>> >; >>>> }; >>>> }; >>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>> index 5172883717d1..90daaf54e704 100644 >>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { >>>> regulator-name = "NVCC_SD (LDO5)"; >>>> regulator-min-microvolt = <1800000>; >>>> regulator-max-microvolt = <3300000>; >>>> + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; >>> >>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be >>> muxed as GPIO, which is not the case. So I think that u-boot have a bug >>> within the (u)sdhc core. >> >> The trick here is that the VSELECT is operated by the usdhc block as a >> function pin, but the PMIC driver can read the current state of the VSELECT >> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is >> set on the VSELECT pin, the state of the pin is reflected in the GPIO block >> SR register even if the pin is muxed as function pin. >> > > Thanks for this explanation :) Why does the regulator driver need to > know the current state of this pin? Because that regulator has an input pin which selects between two states of that regulator, L and H, and whatever L or H is depends on what is configured into the regulator via I2C. To correctly report the state of the regulator, you have to know the state of that input (selector) pin. > Since the voltage switching requires > some cmd's before the actual voltage level switch. So this must be > handled within the core. > > Also after checking the driver, adding the sd-vsel-gpios will request > the specified gpio as output-high. The GPIO would have to be requested as input, obviously. > Out of curiosity, what's the bug you > triggering within U-Boot? AFAICT the readback of the initial state of the regulator (see paragraph above), which affects Linux all the same. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-13 20:59 ` Marek Vasut @ 2023-02-14 8:10 ` Marco Felsch 2023-02-14 8:26 ` Frieder Schrempf 0 siblings, 1 reply; 18+ messages in thread From: Marco Felsch @ 2023-02-14 8:10 UTC (permalink / raw) To: Marek Vasut Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Frieder Schrempf, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam On 23-02-13, Marek Vasut wrote: > On 2/13/23 20:56, Marco Felsch wrote: > > Hi Marek, Frieder, > > Hi, > > > On 23-02-13, Marek Vasut wrote: > > > On 2/13/23 17:15, Marco Felsch wrote: > > > > > > [...] > > > > > > > > @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 > > > > > MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 > > > > > MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 > > > > > MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > > > > > - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > > > > > + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > > > > > > > > The VSELECT pin should be driven by the (u)sdhc core... > > > > > > > > > >; > > > > > }; > > > > > }; > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > > > index 5172883717d1..90daaf54e704 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > > > > > @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { > > > > > regulator-name = "NVCC_SD (LDO5)"; > > > > > regulator-min-microvolt = <1800000>; > > > > > regulator-max-microvolt = <3300000>; > > > > > + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > > > > > > > > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be > > > > muxed as GPIO, which is not the case. So I think that u-boot have a bug > > > > within the (u)sdhc core. > > > > > > The trick here is that the VSELECT is operated by the usdhc block as a > > > function pin, but the PMIC driver can read the current state of the VSELECT > > > pin by reading out the GPIO block SR register. Since the IOMUX SION bit is > > > set on the VSELECT pin, the state of the pin is reflected in the GPIO block > > > SR register even if the pin is muxed as function pin. > > > > > > > Thanks for this explanation :) Why does the regulator driver need to > > know the current state of this pin? > > Because that regulator has an input pin which selects between two states of > that regulator, L and H, and whatever L or H is depends on what is > configured into the regulator via I2C. To correctly report the state of the > regulator, you have to know the state of that input (selector) pin. > > > Since the voltage switching requires > > some cmd's before the actual voltage level switch. So this must be > > handled within the core. > > > > Also after checking the driver, adding the sd-vsel-gpios will request > > the specified gpio as output-high. > > The GPIO would have to be requested as input, obviously. But that isn't the case. According the driver comment they just want to make sure that this GPIO is high to ensure that the correct regulator config registers are used. > > Out of curiosity, what's the bug you > > triggering within U-Boot? > > AFAICT the readback of the initial state of the regulator (see paragraph > above), which affects Linux all the same. According the binding the driver should check this and apply the value to the corresponding L/H register but unfortunately this isn't the case yet. Does U-Boot handle this correctly? Regards, Marco ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-14 8:10 ` Marco Felsch @ 2023-02-14 8:26 ` Frieder Schrempf 2023-02-14 11:46 ` Marco Felsch 0 siblings, 1 reply; 18+ messages in thread From: Frieder Schrempf @ 2023-02-14 8:26 UTC (permalink / raw) To: Marco Felsch, Marek Vasut Cc: Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam Hi Marco, On 14.02.23 09:10, Marco Felsch wrote: > On 23-02-13, Marek Vasut wrote: >> On 2/13/23 20:56, Marco Felsch wrote: >>> Hi Marek, Frieder, >> >> Hi, >> >>> On 23-02-13, Marek Vasut wrote: >>>> On 2/13/23 17:15, Marco Felsch wrote: >>>> >>>> [...] >>>> >>>>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 >>>>>> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 >>>>>> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 >>>>>> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 >>>>>> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 >>>>>> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 >>>>> >>>>> The VSELECT pin should be driven by the (u)sdhc core... >>>>> >>>>>> >; >>>>>> }; >>>>>> }; >>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>>>> index 5172883717d1..90daaf54e704 100644 >>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi >>>>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { >>>>>> regulator-name = "NVCC_SD (LDO5)"; >>>>>> regulator-min-microvolt = <1800000>; >>>>>> regulator-max-microvolt = <3300000>; >>>>>> + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; >>>>> >>>>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be >>>>> muxed as GPIO, which is not the case. So I think that u-boot have a bug >>>>> within the (u)sdhc core. >>>> >>>> The trick here is that the VSELECT is operated by the usdhc block as a >>>> function pin, but the PMIC driver can read the current state of the VSELECT >>>> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is >>>> set on the VSELECT pin, the state of the pin is reflected in the GPIO block >>>> SR register even if the pin is muxed as function pin. >>>> >>> >>> Thanks for this explanation :) Why does the regulator driver need to >>> know the current state of this pin? >> >> Because that regulator has an input pin which selects between two states of >> that regulator, L and H, and whatever L or H is depends on what is >> configured into the regulator via I2C. To correctly report the state of the >> regulator, you have to know the state of that input (selector) pin. >> >>> Since the voltage switching requires >>> some cmd's before the actual voltage level switch. So this must be >>> handled within the core. >>> >>> Also after checking the driver, adding the sd-vsel-gpios will request >>> the specified gpio as output-high. >> >> The GPIO would have to be requested as input, obviously. > > But that isn't the case. According the driver comment they just want to > make sure that this GPIO is high to ensure that the correct regulator > config registers are used. It seems like you look at the wrong code. We previously had the sd-vsel-gpios used as you describe, as an output set to a fixed high level to make sure that the SD_VSEL state matches the driver using the H register. This code is reverted in patch 3 and patch 5 implements the sd-vsel-gpios as input as described by Marek. See: https://lore.kernel.org/lkml/20230213155833.1644366-4-frieder@fris.de/ https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@fris.de/ Sorry, my scripts didn't cc everyone for all patches, which is probably why you missed these changes. > >>> Out of curiosity, what's the bug you >>> triggering within U-Boot? >> >> AFAICT the readback of the initial state of the regulator (see paragraph >> above), which affects Linux all the same. > > According the binding the driver should check this and apply the value > to the corresponding L/H register but unfortunately this isn't the case > yet. Does U-Boot handle this correctly? See above. Thanks Frieder ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal 2023-02-14 8:26 ` Frieder Schrempf @ 2023-02-14 11:46 ` Marco Felsch 0 siblings, 0 replies; 18+ messages in thread From: Marco Felsch @ 2023-02-14 11:46 UTC (permalink / raw) To: Frieder Schrempf Cc: Marek Vasut, Frieder Schrempf, devicetree, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, Rob Herring, Sascha Hauer, Shawn Guo, Oleksij Rempel, Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team, Heiko Thiery, Fabio Estevam On 23-02-14, Frieder Schrempf wrote: > Hi Marco, > > On 14.02.23 09:10, Marco Felsch wrote: > > On 23-02-13, Marek Vasut wrote: > >> On 2/13/23 20:56, Marco Felsch wrote: > >>> Hi Marek, Frieder, > >> > >> Hi, > >> > >>> On 23-02-13, Marek Vasut wrote: > >>>> On 2/13/23 17:15, Marco Felsch wrote: > >>>> > >>>> [...] > >>>> > >>>>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1 0x1d6 > >>>>>> MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2 0x1d6 > >>>>>> MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3 0x1d6 > >>>>>> MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12 0x019 > >>>>>> - MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x1d0 > >>>>>> + MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0x400001d0 > >>>>> > >>>>> The VSELECT pin should be driven by the (u)sdhc core... > >>>>> > >>>>>> >; > >>>>>> }; > >>>>>> }; > >>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > >>>>>> index 5172883717d1..90daaf54e704 100644 > >>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > >>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi > >>>>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 { > >>>>>> regulator-name = "NVCC_SD (LDO5)"; > >>>>>> regulator-min-microvolt = <1800000>; > >>>>>> regulator-max-microvolt = <3300000>; > >>>>>> + sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>; > >>>>> > >>>>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be > >>>>> muxed as GPIO, which is not the case. So I think that u-boot have a bug > >>>>> within the (u)sdhc core. > >>>> > >>>> The trick here is that the VSELECT is operated by the usdhc block as a > >>>> function pin, but the PMIC driver can read the current state of the VSELECT > >>>> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is > >>>> set on the VSELECT pin, the state of the pin is reflected in the GPIO block > >>>> SR register even if the pin is muxed as function pin. > >>>> > >>> > >>> Thanks for this explanation :) Why does the regulator driver need to > >>> know the current state of this pin? > >> > >> Because that regulator has an input pin which selects between two states of > >> that regulator, L and H, and whatever L or H is depends on what is > >> configured into the regulator via I2C. To correctly report the state of the > >> regulator, you have to know the state of that input (selector) pin. > >> > >>> Since the voltage switching requires > >>> some cmd's before the actual voltage level switch. So this must be > >>> handled within the core. > >>> > >>> Also after checking the driver, adding the sd-vsel-gpios will request > >>> the specified gpio as output-high. > >> > >> The GPIO would have to be requested as input, obviously. > > > > But that isn't the case. According the driver comment they just want to > > make sure that this GPIO is high to ensure that the correct regulator > > config registers are used. > > It seems like you look at the wrong code. We previously had the > sd-vsel-gpios used as you describe, as an output set to a fixed high > level to make sure that the SD_VSEL state matches the driver using the H > register. This code is reverted in patch 3 and patch 5 implements the > sd-vsel-gpios as input as described by Marek. See: > > https://lore.kernel.org/lkml/20230213155833.1644366-4-frieder@fris.de/ > https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@fris.de/ > > Sorry, my scripts didn't cc everyone for all patches, which is probably > why you missed these changes. Ah okay this brings light into the dark :) Thanks for the pointers, just received the DT changes. Regards, Marco ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-02-16 10:16 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-13 15:58 [PATCH 0/6] Use correct LDO5 control registers for PCA9450 Frieder Schrempf 2023-02-13 15:58 ` [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios Frieder Schrempf 2023-02-15 20:02 ` Rob Herring 2023-02-16 1:27 ` Marek Vasut 2023-02-16 2:30 ` Rob Herring 2023-02-16 10:15 ` Frieder Schrempf 2023-02-13 15:58 ` [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal Frieder Schrempf 2023-02-13 16:08 ` Marek Vasut 2023-02-13 16:15 ` Frieder Schrempf 2023-02-13 16:15 ` Marco Felsch 2023-02-13 16:18 ` Frieder Schrempf 2023-02-13 21:02 ` Fabio Estevam 2023-02-13 18:12 ` Marek Vasut 2023-02-13 19:56 ` Marco Felsch 2023-02-13 20:59 ` Marek Vasut 2023-02-14 8:10 ` Marco Felsch 2023-02-14 8:26 ` Frieder Schrempf 2023-02-14 11:46 ` Marco Felsch
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).