| Message ID | 20230213155833.1644366-1-frieder@fris.de |
|---|---|
| Headers | show |
| Series | Use correct LDO5 control registers for PCA9450 | expand |
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
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.
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
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
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(-)