diff mbox

[v2,3/4] arm/dts: Add tps65910 regulator DT data to am335x-evm.dts

Message ID 1343110735-10640-4-git-send-email-anilkumar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

AnilKumar, Chimata July 24, 2012, 6:18 a.m. UTC
Add tps65910 regulator device tree data to AM335x-EVM by adding
regulator consumers with tightened constraints and regulator-name.
TPS65910 regulator handle can be obtained by using this regulator
name.

This patch also add I2C node with I2C frequency and tps65910 PMIC
I2C slave address.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 arch/arm/boot/dts/am335x-evm.dts |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

AnilKumar, Chimata Aug. 14, 2012, 2:25 p.m. UTC | #1
Hi Mark,

On Tue, Jul 24, 2012 at 11:48:54, AnilKumar, Chimata wrote:
> Add tps65910 regulator device tree data to AM335x-EVM by adding
> regulator consumers with tightened constraints and regulator-name.
> TPS65910 regulator handle can be obtained by using this regulator
> name.
> 
> This patch also add I2C node with I2C frequency and tps65910 PMIC
> I2C slave address.
> 
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
>  arch/arm/boot/dts/am335x-evm.dts |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index d6a97d9..e4e1ccb 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -18,3 +18,31 @@
>  		reg = <0x80000000 0x10000000>; /* 256 MB */
>  	};
>  };
> +
> +&i2c1 {
> +	clock-frequency = <400000>;
> +
> +	tps: tps@2D {
> +		reg = <0x2D>;
> +	};
> +};
> +
> +/include/ "tps65910.dtsi"
> +
> +&vdd1_reg {
> +	/* VDD_MPU voltage limits 0.95V - 1.26V with +/-4% tolerance */
> +	regulator-name = "vdd_mpu";
> +	regulator-min-microvolt = <912500>;
> +	regulator-max-microvolt = <1312500>;
> +	regulator-boot-on;
> +	regulator-always-on;
> +};

I want to understand how to add parent regulator nodes? Like input-supply
node to regulators.

With "regulator: tps65910: add support for input supply" patch, regulator
registration fails if we are not providing the input_supply node with
proper input supply name.

AM335x EVM have one dedicated input supply which provide supplies to all
the PMIC regulators. According to tps65910 "VDD1 regulator" input supply
is from vcc1 so we have to create a dummy/parent/some node to handle this.
Example below has the details.

In am335x-evm.dts file

vcc1_supply: fixedregulator@0 {
                compatible = "regulator-fixed";
                regulator-name = "vcc1";
                regulator-boot-on;
};

/include/ "tps65910.dtsi"

&tps {
	regulators {
		vcc1-supply = <&vcc1_supply>;
		...
	};
};

Similarly we have to add 7 more fixed regulators entries to provide input
supply to different PMIC regulators.

Am I doing correct thing? Or is there any better way to handle this?

Regards
AnilKumar
Mark Brown Aug. 15, 2012, 12:53 p.m. UTC | #2
On Tue, Aug 14, 2012 at 02:25:18PM +0000, AnilKumar, Chimata wrote:

> AM335x EVM have one dedicated input supply which provide supplies to all
> the PMIC regulators. According to tps65910 "VDD1 regulator" input supply
> is from vcc1 so we have to create a dummy/parent/some node to handle this.
> Example below has the details.

Your device tree should accurately reflect the hardware.

> Similarly we have to add 7 more fixed regulators entries to provide input
> supply to different PMIC regulators.

If you have all these unrelated supplies in the system then your device
tree should accurately reflect that.  However this would be a very
unusual hardware design.  Usually there would be a very small set of
system supply rails, frequently only one.
AnilKumar, Chimata Aug. 15, 2012, 4:47 p.m. UTC | #3
Hi Mark,

On Wed, Aug 15, 2012 at 18:23:39, Mark Brown wrote:
> On Tue, Aug 14, 2012 at 02:25:18PM +0000, AnilKumar, Chimata wrote:
> 
> > AM335x EVM have one dedicated input supply which provide supplies to all
> > the PMIC regulators. According to tps65910 "VDD1 regulator" input supply
> > is from vcc1 so we have to create a dummy/parent/some node to handle this.
> > Example below has the details.
> 
> Your device tree should accurately reflect the hardware.
> 
> > Similarly we have to add 7 more fixed regulators entries to provide input
> > supply to different PMIC regulators.
> 
> If you have all these unrelated supplies in the system then your device
> tree should accurately reflect that.  However this would be a very

In that case I have to add all these fixed regulators.

> unusual hardware design.  Usually there would be a very small set of
> system supply rails, frequently only one.

In case of AM335X-EVM one dedicated supply (5v) feeding to all the input
supplies (vcc1-7 and vccio) of the regulators.

tps65910 PMIC have vcc1-7 and vccio input supplies which are feeding to
all the regulators. (like one - one and one - many).

Tps65910 regulator drivers registration fails if we not providing the
correct input supply with supply name. One option I can see is that
fixed voltage regulators. If we do this then total 21 (8 input supply
fixed regulators and others are out regulators) are present in the system.

If this is the correct approach then I will submit next version of patch.

Thanks
AnilKumar
Mark Brown Aug. 15, 2012, 4:54 p.m. UTC | #4
On Wed, Aug 15, 2012 at 04:47:02PM +0000, AnilKumar, Chimata wrote:
> On Wed, Aug 15, 2012 at 18:23:39, Mark Brown wrote:

> > If you have all these unrelated supplies in the system then your device
> > tree should accurately reflect that.  However this would be a very

> In that case I have to add all these fixed regulators.

Why?  You then immediately go on to say...

> > unusual hardware design.  Usually there would be a very small set of
> > system supply rails, frequently only one.

> In case of AM335X-EVM one dedicated supply (5v) feeding to all the input
> supplies (vcc1-7 and vccio) of the regulators.

...that they all map onto the same physical supply?
AnilKumar, Chimata Aug. 16, 2012, 1:22 p.m. UTC | #5
Mark,

On Wed, Aug 15, 2012 at 22:24:32, Mark Brown wrote:
> On Wed, Aug 15, 2012 at 04:47:02PM +0000, AnilKumar, Chimata wrote:
> > On Wed, Aug 15, 2012 at 18:23:39, Mark Brown wrote:
> 
> > > If you have all these unrelated supplies in the system then your device
> > > tree should accurately reflect that.  However this would be a very
> 
> > In that case I have to add all these fixed regulators.
> 
> Why?  You then immediately go on to say...

Because all the nodes/regulators should be provided/registered otherwise
"regulator_dev_lookup" returns NULL if there are no regulator matches with
the supply_name.

I have gone through this thread
http://patchwork.ozlabs.org/patch/166682/

But problem here is single input supply battery/main is not sufficient.

pmic->desc[i].supply_name = info->vin_name;
with this assignment regulator core searches for exact supply name vcc1 or
vcc2 etc if there are no matches then it will not register any regulators. 

> 
> > > unusual hardware design.  Usually there would be a very small set of
> > > system supply rails, frequently only one.
> 
> > In case of AM335X-EVM one dedicated supply (5v) feeding to all the input
> > supplies (vcc1-7 and vccio) of the regulators.
> 
> ...that they all map onto the same physical supply?
> 

Yes single physical supply, if we want to add single fixed regulator node
to feed all the regulators then driver have to modify to support this.
Similar to revert of "regulator: tps65910: add support for input supply"
Patch.

Thanks
AnilKumar
AnilKumar, Chimata Aug. 16, 2012, 1:25 p.m. UTC | #6
Mark,

Small correction

On Thu, Aug 16, 2012 at 18:52:25, AnilKumar, Chimata wrote:
> Mark,
> 
> On Wed, Aug 15, 2012 at 22:24:32, Mark Brown wrote:
> > On Wed, Aug 15, 2012 at 04:47:02PM +0000, AnilKumar, Chimata wrote:
> > > On Wed, Aug 15, 2012 at 18:23:39, Mark Brown wrote:
> > 
> > > > If you have all these unrelated supplies in the system then your device
> > > > tree should accurately reflect that.  However this would be a very
> > 
> > > In that case I have to add all these fixed regulators.
> > 
> > Why?  You then immediately go on to say...
> 
> Because all the nodes/regulators should be provided/registered otherwise
> "regulator_dev_lookup" returns NULL if there are no regulator matches with
> the supply_name.
> 
> I have gone through this thread
> http://patchwork.ozlabs.org/patch/166682/
> 
> But problem here is single input supply battery/main is not sufficient.
> 
> pmic->desc[i].supply_name = info->vin_name;
> with this assignment regulator core searches for exact supply name vcc1 or
> vcc2 etc if there are no matches then it will not register any regulators. 
> 
> > 
> > > > unusual hardware design.  Usually there would be a very small set of
> > > > system supply rails, frequently only one.
> > 
> > > In case of AM335X-EVM one dedicated supply (5v) feeding to all the input
> > > supplies (vcc1-7 and vccio) of the regulators.
> > 
> > ...that they all map onto the same physical supply?
> > 
> 
> Yes single physical supply, if we want to add single fixed regulator node
> to feed all the regulators then driver have to modify to support this.
> Similar to revert of "regulator: tps65910: add support for input supply"
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy/paste error

Similar to revert of this patch "regulator: tps65910: set input_supply on
desc unconditionally"

Thanks
AnilKumar
Mark Brown Aug. 16, 2012, 2 p.m. UTC | #7
On Thu, Aug 16, 2012 at 01:22:27PM +0000, AnilKumar, Chimata wrote:

> But problem here is single input supply battery/main is not sufficient.

What makes you say this?

> pmic->desc[i].supply_name = info->vin_name;
> with this assignment regulator core searches for exact supply name vcc1 or
> vcc2 etc if there are no matches then it will not register any regulators. 

Why does this mean you need multiple regulators?  A single regulator can
of course supply multiple devices.
AnilKumar, Chimata Aug. 20, 2012, 6:27 a.m. UTC | #8
Hi Mark,

On Thu, Aug 16, 2012 at 19:30:56, Mark Brown wrote:
> On Thu, Aug 16, 2012 at 01:22:27PM +0000, AnilKumar, Chimata wrote:
> 
> > But problem here is single input supply battery/main is not sufficient.
> 
> What makes you say this?
> 
> > pmic->desc[i].supply_name = info->vin_name;
> > with this assignment regulator core searches for exact supply name vcc1 or
> > vcc2 etc if there are no matches then it will not register any regulators. 
> 
> Why does this mean you need multiple regulators?  A single regulator can
> of course supply multiple devices.

I got your point but tps65910 driver is failing if we add single fixed
regulator. Solution is driver needs to change or add multiple fixed
regulators.

Let's take this example

ocp {
        vcc5v_supply: fixedregulator@0 {
                compatible = "regulator-fixed";
                regulator-name = "vcc5v";
                regulator-boot-on;
        };
};

/include/ "tps65910.dtsi"

&tps {
        regulators {
                vcc1-supply = <&vcc5v_supply>;
                vcc2-supply = <&vcc5v_supply>;
                vcc3-supply = <&vcc5v_supply>;
                vcc4-supply = <&vcc5v_supply>;
                vcc5-supply = <&vcc5v_supply>;
                vcc6-supply = <&vcc5v_supply>;
                vcc7-supply = <&vcc5v_supply>;
                vccio-supply = <&vcc5v_supply>;

                vrtc_reg: regulator@0 { /* vrtc input is vcc7 */
                        regulator-always-on;
                };

                ... 
                ...
        };
};

These changes are added to am335x-evm.dts file after this change I
am seeing below error.

vrtc:
tps65910 0-002d: Failed to find supply vcc7
tps65910 0-002d: failed to register tps65910-pmic regulator

To get rid of this error we have to change the driver like this

if (info->vin_name) {
         snprintf(in_supply, 32, "%s-supply", info->vin_name);
         if (of_find_property(np, in_supply, 0))
                  pmic_plat_data->input_supply[idx] = info->vin_name;
}

And

pmic->desc[i].supply_name = pmic_plat_data->input_supply[i];

Regards
AnilKumar
Mark Brown Aug. 20, 2012, 8:07 p.m. UTC | #9
On Mon, Aug 20, 2012 at 06:27:01AM +0000, AnilKumar, Chimata wrote:
> On Thu, Aug 16, 2012 at 19:30:56, Mark Brown wrote:

> > Why does this mean you need multiple regulators?  A single regulator can
> > of course supply multiple devices.

> I got your point but tps65910 driver is failing if we add single fixed
> regulator. Solution is driver needs to change or add multiple fixed
> regulators.

No, really.  One regulator can supply many things, not being a.

> vrtc:
> tps65910 0-002d: Failed to find supply vcc7
> tps65910 0-002d: failed to register tps65910-pmic regulator

So you've specified a whole bunch of supplies and this one individual
supply is not matching (and it's not the first...).  You should
investigate what's gone wrong there rather than bodging around the
problem.  I can't immediately say anything much based on the information
you've given.
AnilKumar, Chimata Aug. 21, 2012, 10:57 a.m. UTC | #10
Hi Mark,

On Tue, Aug 21, 2012 at 01:37:15, Mark Brown wrote:
> On Mon, Aug 20, 2012 at 06:27:01AM +0000, AnilKumar, Chimata wrote:
> > On Thu, Aug 16, 2012 at 19:30:56, Mark Brown wrote:
> 
> > > Why does this mean you need multiple regulators?  A single regulator can
> > > of course supply multiple devices.
> 
> > I got your point but tps65910 driver is failing if we add single fixed
> > regulator. Solution is driver needs to change or add multiple fixed
> > regulators.
> 
> No, really.  One regulator can supply many things, not being a.
> 
> > vrtc:
> > tps65910 0-002d: Failed to find supply vcc7
> > tps65910 0-002d: failed to register tps65910-pmic regulator
> 
> So you've specified a whole bunch of supplies and this one individual
> supply is not matching (and it's not the first...).  You should
> investigate what's gone wrong there rather than bodging around the
> problem.  I can't immediately say anything much based on the information
> you've given.
> 

Sorry for the noise in this issue, I found the root cause. I
was defining the supply names like this which is wrong.

&tps {
        regulators {
                vcc1-supply = <&vcc5v_supply>;
                vcc2-supply = <&vcc5v_supply>;
                vcc3-supply = <&vcc5v_supply>;
                vcc4-supply = <&vcc5v_supply>;
                vcc5-supply = <&vcc5v_supply>;
                vcc6-supply = <&vcc5v_supply>;
                vcc7-supply = <&vcc5v_supply>;
                vccio-supply = <&vcc5v_supply>;

                vrtc_reg: regulator@0 { /* vrtc input is vcc7 */
                        regulator-always-on;
                };

                ... 
                ...
        };
};

This was suppose to be

&tps {
        vcc1-supply = <&vcc5v_supply>;
        vcc2-supply = <&vcc5v_supply>;
        vcc3-supply = <&vcc5v_supply>;
        vcc4-supply = <&vcc5v_supply>;
        vcc5-supply = <&vcc5v_supply>;
        vcc6-supply = <&vcc5v_supply>;
        vcc7-supply = <&vcc5v_supply>;
        vccio-supply = <&vcc5v_supply>;

        regulators {

                vrtc_reg: regulator@0 { /* vrtc input is vcc7 */
                        regulator-always-on;
                };

                ... 
                ...
        };
};

Thanks for your continuous feedback.

Thanks
AnilKumar
diff mbox

Patch

diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index d6a97d9..e4e1ccb 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -18,3 +18,31 @@ 
 		reg = <0x80000000 0x10000000>; /* 256 MB */
 	};
 };
+
+&i2c1 {
+	clock-frequency = <400000>;
+
+	tps: tps@2D {
+		reg = <0x2D>;
+	};
+};
+
+/include/ "tps65910.dtsi"
+
+&vdd1_reg {
+	/* VDD_MPU voltage limits 0.95V - 1.26V with +/-4% tolerance */
+	regulator-name = "vdd_mpu";
+	regulator-min-microvolt = <912500>;
+	regulator-max-microvolt = <1312500>;
+	regulator-boot-on;
+	regulator-always-on;
+};
+
+&vdd2_reg {
+	/* VDD_CORE voltage limits 0.95V - 1.1V with +/-4% tolerance */
+	regulator-name = "vdd_core";
+	regulator-min-microvolt = <912500>;
+	regulator-max-microvolt = <1150000>;
+	regulator-boot-on;
+	regulator-always-on;
+};