diff mbox

[RFC,2/3] power: mxs_power: add driver for mxs power subsystem

Message ID 1416514477-19190-3-git-send-email-stefan.wahren@i2se.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

Stefan Wahren Nov. 20, 2014, 8:14 p.m. UTC
This patch adds a minimal driver for the Freescale i.MX23, i.MX28
power subsystem. It's required to trigger the probing of the underlying
drivers like on-chip regulators. Additionally the drivers supports
the configuration of the DC-DC clock frequency to avoid possible
interferences.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/power/Kconfig     |    8 ++
 drivers/power/Makefile    |    1 +
 drivers/power/mxs_power.c |  226 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 235 insertions(+)
 create mode 100644 drivers/power/mxs_power.c

--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sebastian Reichel Jan. 21, 2015, 11:01 p.m. UTC | #1
Hi Stefan,

Sorry for the big delay.

On Thu, Nov 20, 2014 at 08:14:36PM +0000, Stefan Wahren wrote:
> This patch adds a minimal driver for the Freescale i.MX23, i.MX28
> power subsystem. It's required to trigger the probing of the underlying
> drivers like on-chip regulators. Additionally the drivers supports
> the configuration of the DC-DC clock frequency to avoid possible
> interferences.

I would expect PLL to be board specific and part of DT. Why is it
specified as parameter?

Apart from that the driver/patch looks fine to me.

-- Sebastian
Stefan Wahren Jan. 22, 2015, 8:08 a.m. UTC | #2
Hi Sebastian,

[add Marek]

Am 22.01.2015 um 00:01 schrieb Sebastian Reichel:
> Hi Stefan,
>
> Sorry for the big delay.

a late answer is better than no answer :)

>
> On Thu, Nov 20, 2014 at 08:14:36PM +0000, Stefan Wahren wrote:
>> This patch adds a minimal driver for the Freescale i.MX23, i.MX28
>> power subsystem. It's required to trigger the probing of the underlying
>> drivers like on-chip regulators. Additionally the drivers supports
>> the configuration of the DC-DC clock frequency to avoid possible
>> interferences.
> I would expect PLL to be board specific and part of DT. Why is it
> specified as parameter?

I think the switching frequency of the DC-DC belongs to the
configuration and don't describe how the hardware is connected. But i
don't have a problem to change it into a DT property.

Does a common property name exists for the switching frequency or would
it be vendor specific?

> Apart from that the driver/patch looks fine to me.

In the meantime i made some bugfixes on these patches. Now i think it
would be better to integrate the on-chip regulator driver into the patch
series. That would clarify the function of the power driver.

> -- Sebastian

Best regards
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sébastien Szymanski Jan. 22, 2015, 9:37 a.m. UTC | #3
Hello,

On 01/22/2015 09:08 AM, Stefan Wahren wrote:
> 
> In the meantime i made some bugfixes on these patches. Now i think it
> would be better to integrate the on-chip regulator driver into the patch
> series. That would clarify the function of the power driver.
> 

I was about to resend this series with a poweroff driver for the imx28
SoCs. Should I wait this series get merged?

I thought adding a poweroff child node like this:

power: power@80044000 {
	...
	poweroff: poweroff@80044100 {
		compatible = "fsl,imx28-poweroff";
		reg = <0x80044100 0x10>;
	};
	...
};

Here is the driver:

/*
 * MXS SoCs reset code.
 *
 * Copyright (c) 2015 Armadeus Systems.
 *
 * Author: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
 *
 * based on imx-snvs-poweroff.c
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 and
 * only version 2 as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 */

#include <linux/err.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>

#define MXS_SET_ADDR                    0x4

#define POWER_RESET_UNLOCK__KEY         (0x3E77 << 16)
#define POWER_RESET_PWD                 0x1

static void __iomem *power_reset;

static void do_mxs_poweroff(void)
{
        writel(POWER_RESET_UNLOCK__KEY | POWER_RESET_PWD,
               power_reset + MXS_SET_ADDR);
}

static int mxs_poweroff_probe(struct platform_device *pdev)
{
        power_reset = of_iomap(pdev->dev.of_node, 0);
        if (!power_reset) {
                dev_err(&pdev->dev, "failed to get memory\n");
                return -ENODEV;
        }

        pm_power_off = do_mxs_poweroff;

        dev_info(&pdev->dev, "power off function set!\n");

        return 0;
}

static const struct of_device_id of_mxs_poweroff_match[] = {
        { .compatible = "fsl,imx28-poweroff", },
        {},
};
MODULE_DEVICE_TABLE(of, of_mxs_poweroff_match);

static struct platform_driver mxs_poweroff_driver = {
        .probe = mxs_poweroff_probe,
        .driver = {
                .name = "mxs-poweroff",
                .of_match_table = of_match_ptr(of_mxs_poweroff_match),
        },
};

static int __init mxs_poweroff_init(void)
{
        return platform_driver_register(&mxs_poweroff_driver);
}
device_initcall(mxs_poweroff_init);

Best Regards,
Stefan Wahren Jan. 22, 2015, 9:48 a.m. UTC | #4
Hello,

Am 22.01.2015 um 10:37 schrieb Sébastien SZYMANSKI:
> Hello,
>
> On 01/22/2015 09:08 AM, Stefan Wahren wrote:
>> In the meantime i made some bugfixes on these patches. Now i think it
>> would be better to integrate the on-chip regulator driver into the patch
>> series. That would clarify the function of the power driver.
>>
> I was about to resend this series with a poweroff driver for the imx28
> SoCs.

sounds great.

> Should I wait this series get merged?

Yes, please. In the meantime you can use my repo at github (based on
pm+acpi-3.19-rc4):

https://github.com/lategoodbye/linux-mxs-power

>
> I thought adding a poweroff child node like this:
>
> power: power@80044000 {
> 	...
> 	poweroff: poweroff@80044100 {
> 		compatible = "fsl,imx28-poweroff";
> 		reg = <0x80044100 0x10>;
> 	};
> 	...
> };

Just a note to your driver it would be nice if it's handles i.MX23 and
i.MX28.

> [...]

Regards Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Jan. 25, 2015, 3:04 p.m. UTC | #5
Hi,

[add maintainers from regulator framework]

On Thu, Jan 22, 2015 at 09:08:04AM +0100, Stefan Wahren wrote:
> Am 22.01.2015 um 00:01 schrieb Sebastian Reichel:
> > On Thu, Nov 20, 2014 at 08:14:36PM +0000, Stefan Wahren wrote:
> >> This patch adds a minimal driver for the Freescale i.MX23, i.MX28
> >> power subsystem. It's required to trigger the probing of the underlying
> >> drivers like on-chip regulators. Additionally the drivers supports
> >> the configuration of the DC-DC clock frequency to avoid possible
> >> interferences.
> > I would expect PLL to be board specific and part of DT. Why is it
> > specified as parameter?
> 
> I think the switching frequency of the DC-DC belongs to the
> configuration and don't describe how the hardware is connected. But i
> don't have a problem to change it into a DT property.
> 
> Does a common property name exists for the switching frequency or would
> it be vendor specific?

As far as I know most regulators have fixed switching frequencies,
so there is no common property name so far. I added regulator
framework people, since this property should be regulator specific.

Apart from introducing a custom/common switching-frequency property
the dts could also expose the PLL like this and use the common
clock-frequency property:

power: power@80044000 {
    compatible = "fsl,imx28-power";
    #address-cells = <1>;
    #size-cells = <1>;
    reg = <0x80044000 0x2000>;
    interrupts = <6>;
    ranges;

    powerpll {
        compatible = "fsl,imx28-power-pll"
        #clock-cells = <0>;
        clock-frequency = <12345>;
    }
}

-- Sebastian
Stefan Wahren Jan. 26, 2015, 7:46 p.m. UTC | #6
Hi,

> Sebastian Reichel <sre@kernel.org> hat am 25. Januar 2015 um 16:04
> geschrieben:
>
>
> Hi,
>
> [add maintainers from regulator framework]
>
> On Thu, Jan 22, 2015 at 09:08:04AM +0100, Stefan Wahren wrote:
> > Am 22.01.2015 um 00:01 schrieb Sebastian Reichel:
> > > On Thu, Nov 20, 2014 at 08:14:36PM +0000, Stefan Wahren wrote:
> > >> This patch adds a minimal driver for the Freescale i.MX23, i.MX28
> > >> power subsystem. It's required to trigger the probing of the underlying
> > >> drivers like on-chip regulators. Additionally the drivers supports
> > >> the configuration of the DC-DC clock frequency to avoid possible
> > >> interferences.
> > > I would expect PLL to be board specific and part of DT. Why is it
> > > specified as parameter?
> >
> > I think the switching frequency of the DC-DC belongs to the
> > configuration and don't describe how the hardware is connected. But i
> > don't have a problem to change it into a DT property.
> >
> > Does a common property name exists for the switching frequency or would
> > it be vendor specific?
>
> As far as I know most regulators have fixed switching frequencies,
> so there is no common property name so far. I added regulator
> framework people, since this property should be regulator specific.
>
> Apart from introducing a custom/common switching-frequency property
> the dts could also expose the PLL like this and use the common
> clock-frequency property:
>
> power: power@80044000 {
> compatible = "fsl,imx28-power";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x80044000 0x2000>;
> interrupts = <6>;
> ranges;
>
> powerpll {
> compatible = "fsl,imx28-power-pll"
> #clock-cells = <0>;
> clock-frequency = <12345>;
> }
> }

okay i understand. But doesn't it need a extra driver to set the switching
frequency because of the new compatible string?

At this point i think about moving this feature into the bootloader.

@Fabio, @Marek:

What's your opinion about that?

>
> -- Sebastian

Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 27, 2015, 12:16 a.m. UTC | #7
On Mon, Jan 26, 2015 at 08:46:45PM +0100, Stefan Wahren wrote:

> > > Does a common property name exists for the switching frequency or would
> > > it be vendor specific?

> > As far as I know most regulators have fixed switching frequencies,
> > so there is no common property name so far. I added regulator
> > framework people, since this property should be regulator specific.

No, it's pretty common for there to be a couple of options - there's
normally a performance tradeoff (power for regulation accuracy and/or
cost) which can be selected.  IIRC it's selected at design time as it
affect the choice of passives on the board.

> okay i understand. But doesn't it need a extra driver to set the switching
> frequency because of the new compatible string?

I don't understand this bit at all, sorry.

> 
> At this point i think about moving this feature into the bootloader.
> 
> @Fabio, @Marek:
> 
> What's your opinion about that?
> 
> >
> > -- Sebastian
> 
> Stefan
>
Stefan Wahren Jan. 27, 2015, 6:35 p.m. UTC | #8
Hi Mark,

> Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 01:16 geschrieben:
>
>
> On Mon, Jan 26, 2015 at 08:46:45PM +0100, Stefan Wahren wrote:
>
> > > > Does a common property name exists for the switching frequency or would
> > > > it be vendor specific?
>
> > > As far as I know most regulators have fixed switching frequencies,
> > > so there is no common property name so far. I added regulator
> > > framework people, since this property should be regulator specific.
>
> No, it's pretty common for there to be a couple of options - there's
> normally a performance tradeoff (power for regulation accuracy and/or
> cost) which can be selected. IIRC it's selected at design time as it
> affect the choice of passives on the board.
>
> > okay i understand. But doesn't it need a extra driver to set the switching
> > frequency because of the new compatible string?
>
> I don't understand this bit at all, sorry.

Sebastian suggested a new sub-node in the devicetree:

    powerpll {
        compatible = "fsl,imx28-power-pll"
        #clock-cells = <0>;
        clock-frequency = <12345>;
    }

and i think that the new compatible string needs a separate driver to take care
of the switching frequency.

Or is it okay to leave the handling of the switching frequency in the mxs-power
driver?

Btw if we add a new node to set switching frequency, i think it would be better
to describe the dc-dc convertor and not the pll:

    dcdc {
       compatible = "fsl,imx28-dcdc"
       frequency = <12345>;
    }

What do you think?

Best regards
Stefan

>
> >
> > At this point i think about moving this feature into the bootloader.
> >
> > @Fabio, @Marek:
> >
> > What's your opinion about that?
> >
> > >
> > > -- Sebastian
> >
> > Stefan
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 27, 2015, 7:43 p.m. UTC | #9
On Tue, Jan 27, 2015 at 07:35:09PM +0100, Stefan Wahren wrote:
> > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 01:16 geschrieben:

> > > okay i understand. But doesn't it need a extra driver to set the switching
> > > frequency because of the new compatible string?

> > I don't understand this bit at all, sorry.

> Sebastian suggested a new sub-node in the devicetree:

>     powerpll {
>         compatible = "fsl,imx28-power-pll"
>         #clock-cells = <0>;
>         clock-frequency = <12345>;
>     }

> and i think that the new compatible string needs a separate driver to take care
> of the switching frequency.

I can see that this has been suggested, what I can't understand is why
we would wish to do this but I am missing some context here which may
make it all perfectly clear.

> Or is it okay to leave the handling of the switching frequency in the mxs-power
> driver?

Unless the clock is exposed externally to the regulator either as an
input or an output I'm not sure I see any benefit but I'm possibly
missing something here.  It does sound like there might be a separate
clock IP or something?

> Btw if we add a new node to set switching frequency, i think it would be better
> to describe the dc-dc convertor and not the pll:

>     dcdc {
>        compatible = "fsl,imx28-dcdc"
>        frequency = <12345>;
>     }

> What do you think?

Yes, just adding a new property on the regulator node with all the other
properties seems sensible (we've just gained some helpers which make it
fairly straightforward to do this).
Stefan Wahren Jan. 28, 2015, 10:22 p.m. UTC | #10
Hi Mark,

> Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 20:43 geschrieben:
>
>
> On Tue, Jan 27, 2015 at 07:35:09PM +0100, Stefan Wahren wrote:
> > > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 01:16
> > > geschrieben:
>
> > > > okay i understand. But doesn't it need a extra driver to set the
> > > > switching
> > > > frequency because of the new compatible string?
>
> > > I don't understand this bit at all, sorry.
>
> > Sebastian suggested a new sub-node in the devicetree:
>
> > powerpll {
> > compatible = "fsl,imx28-power-pll"
> > #clock-cells = <0>;
> > clock-frequency = <12345>;
> > }
>
> > and i think that the new compatible string needs a separate driver to take
> > care
> > of the switching frequency.
>
> I can see that this has been suggested, what I can't understand is why
> we would wish to do this but I am missing some context here which may
> make it all perfectly clear.

changing the switching frequency is not a common feature for this IC. In most
cases the clock for the DC-DC switching frequency runs at 24 MHz (ref_xtal).
Unfortunatelly this could cause interferences for example in UHF band. So
switching to ref_pll and changing clock divider is a option to avoid such
interferences without changing hardware.

The relevant register DC-DC Miscellaneous Register (HW_POWER_MISC) is described
in chapter 11.12.10 of the reference manual [1].

I decided to implement it as a module parameter, because i see it's a
configuration option.

[1] - http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf

>
> > Or is it okay to leave the handling of the switching frequency in the
> > mxs-power
> > driver?
>
> Unless the clock is exposed externally to the regulator either as an
> input or an output I'm not sure I see any benefit but I'm possibly
> missing something here. It does sound like there might be a separate
> clock IP or something?
>
> > Btw if we add a new node to set switching frequency, i think it would be
> > better
> > to describe the dc-dc convertor and not the pll:
>
> > dcdc {
> > compatible = "fsl,imx28-dcdc"
> > frequency = <12345>;
> > }
>
> > What do you think?
>
> Yes, just adding a new property on the regulator node with all the other
> properties seems sensible (we've just gained some helpers which make it
> fairly straightforward to do this).
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Jan. 28, 2015, 10:59 p.m. UTC | #11
Hi,

On Wed, Jan 28, 2015 at 11:22:05PM +0100, Stefan Wahren wrote:
> > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 20:43 geschrieben:
> > On Tue, Jan 27, 2015 at 07:35:09PM +0100, Stefan Wahren wrote:
> > > > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 01:16
> > > > geschrieben:
> >
> > > > > okay i understand. But doesn't it need a extra driver to set the
> > > > > switching
> > > > > frequency because of the new compatible string?
> >
> > > > I don't understand this bit at all, sorry.
> >
> > > Sebastian suggested a new sub-node in the devicetree:
> >
> > > powerpll {
> > > compatible = "fsl,imx28-power-pll"
> > > #clock-cells = <0>;
> > > clock-frequency = <12345>;
> > > }
> >
> > > and i think that the new compatible string needs a separate driver to take
> > > care
> > > of the switching frequency.
> >
> > I can see that this has been suggested, what I can't understand is why
> > we would wish to do this but I am missing some context here which may
> > make it all perfectly clear.
> 
> The relevant register DC-DC Miscellaneous Register (HW_POWER_MISC) is described
> in chapter 11.12.10 of the reference manual [1].
>
> [1] - http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf

Having a look at the datasheet my suggestion for the sub-node
doesn't make sense. I missunderstood how the hardware architecture
looks like, sorry for the noise :(

> changing the switching frequency is not a common feature for this IC. In most
> cases the clock for the DC-DC switching frequency runs at 24 MHz (ref_xtal).
> Unfortunatelly this could cause interferences for example in UHF band. So
> switching to ref_pll and changing clock divider is a option to avoid such
> interferences without changing hardware.
>
> I decided to implement it as a module parameter, because i see it's a
> configuration option.

I think its best to use a DT property then. That way it can be set
appropiatly for boards known to use a band, which could have
interference problems.

So just add a simple property (without a subnode). If I understand
Mark correctly, there's a high chance more drivers will need to
describe the switching frequency, so I suggest to use a vendor
independent property name:

switching-frequency = <12345>;

-- Sebastian
Stefan Wahren Feb. 3, 2015, 8:41 p.m. UTC | #12
Hi Sebastian,

> Sebastian Reichel <sre@kernel.org> hat am 28. Januar 2015 um 23:59
> geschrieben:
>
>
> Hi,
>
> On Wed, Jan 28, 2015 at 11:22:05PM +0100, Stefan Wahren wrote:
> > > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 20:43
> > > geschrieben:
> > > On Tue, Jan 27, 2015 at 07:35:09PM +0100, Stefan Wahren wrote:
> > >
> > changing the switching frequency is not a common feature for this IC. In
> > most
> > cases the clock for the DC-DC switching frequency runs at 24 MHz (ref_xtal).
> > Unfortunatelly this could cause interferences for example in UHF band. So
> > switching to ref_pll and changing clock divider is a option to avoid such
> > interferences without changing hardware.
> >
> > I decided to implement it as a module parameter, because i see it's a
> > configuration option.
>
> I think its best to use a DT property then. That way it can be set
> appropiatly for boards known to use a band, which could have
> interference problems.
>
> So just add a simple property (without a subnode). If I understand
> Mark correctly, there's a high chance more drivers will need to
> describe the switching frequency, so I suggest to use a vendor
> independent property name:
>
> switching-frequency = <12345>;

i'm not sure. So here is the complete current state (power and regulator binding
from imx28.dtsi) including my suggestion for the new property:

	power: power@80044000 {
		compatible = "fsl,imx28-power"; /* handled by mxs_power.c */
		#address-cells = <1>;
		#size-cells = <1>;
		reg = <0x80044000 0x2000>;
		interrupts = <6>;
		switching-frequency = <24000000>; /* new property */
		ranges;

		reg_vddd: regulator@80044040 {
			reg = <0x80044040 0x10>,
			      <0x80044010 0x10>,
			      <0x800440c0 0x10>;
			reg-names = "base-address",
				    "v5ctrl-address",
				    "status-address";
			compatible = "fsl,imx28-vddd"; /* handled by mxs-regulator.c */
			regulator-name = "vddd";
			regulator-min-microvolt = <1350000>;
			regulator-max-microvolt = <1550000>;
			vddd-supply = <&reg_vdda>;
		};

		reg_vdda: regulator@80044050 {
			reg = <0x80044050 0x10>,
			      <0x80044010 0x10>,
			      <0x800440c0 0x10>;
			reg-names = "base-address",
				    "v5ctrl-address",
				    "status-address";
			compatible = "fsl,imx28-vdda"; /* handled by mxs-regulator.c */
			regulator-name = "vdda";
			regulator-min-microvolt = <1725000>;
			regulator-max-microvolt = <1950000>;
			vdda-supply = <&reg_vddio>;
		};

		reg_vddio: regulator@80044060 {
			reg = <0x80044060 0x10>,
			      <0x80044010 0x10>,
			      <0x800440c0 0x10>;
			reg-names = "base-address",
				    "v5ctrl-address",
				    "status-address";
			compatible = "fsl,imx28-vddio"; /* handled by mxs-regulator.c */
			regulator-name = "vddio";
			regulator-min-microvolt = <3000000>;
			regulator-max-microvolt = <3575000>;
			regulator-microvolt-offset = <80000>;
		};
	};

Please correct me if i'm wrong.

Stefan

>
> -- Sebastian
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren Feb. 20, 2015, 10:57 a.m. UTC | #13
> Stefan Wahren <stefan.wahren@i2se.com> hat am 3. Februar 2015 um 21:41
> geschrieben:
>
>
> Hi Sebastian,
>
> > Sebastian Reichel <sre@kernel.org> hat am 28. Januar 2015 um 23:59
> > geschrieben:
> >
> >
> > Hi,
> >
> > On Wed, Jan 28, 2015 at 11:22:05PM +0100, Stefan Wahren wrote:
> > > > Mark Brown <broonie@kernel.org> hat am 27. Januar 2015 um 20:43
> > > > geschrieben:
> > > > On Tue, Jan 27, 2015 at 07:35:09PM +0100, Stefan Wahren wrote:
> > > >
> > > changing the switching frequency is not a common feature for this IC. In
> > > most
> > > cases the clock for the DC-DC switching frequency runs at 24 MHz
> > > (ref_xtal).
> > > Unfortunatelly this could cause interferences for example in UHF band. So
> > > switching to ref_pll and changing clock divider is a option to avoid such
> > > interferences without changing hardware.
> > >
> > > I decided to implement it as a module parameter, because i see it's a
> > > configuration option.
> >
> > I think its best to use a DT property then. That way it can be set
> > appropiatly for boards known to use a band, which could have
> > interference problems.
> >
> > So just add a simple property (without a subnode). If I understand
> > Mark correctly, there's a high chance more drivers will need to
> > describe the switching frequency, so I suggest to use a vendor
> > independent property name:
> >
> > switching-frequency = <12345>;
>
> i'm not sure. So here is the complete current state (power and regulator
> binding
> from imx28.dtsi) including my suggestion for the new property:
>
> power: power@80044000 {
> compatible = "fsl,imx28-power"; /* handled by mxs_power.c */
> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0x80044000 0x2000>;
> interrupts = <6>;
> switching-frequency = <24000000>; /* new property */
> ranges;
>
> reg_vddd: regulator@80044040 {
> reg = <0x80044040 0x10>,
> <0x80044010 0x10>,
> <0x800440c0 0x10>;
> reg-names = "base-address",
> "v5ctrl-address",
> "status-address";
> compatible = "fsl,imx28-vddd"; /* handled by mxs-regulator.c */
> regulator-name = "vddd";
> regulator-min-microvolt = <1350000>;
> regulator-max-microvolt = <1550000>;
> vddd-supply = <&reg_vdda>;
> };
>
> reg_vdda: regulator@80044050 {
> reg = <0x80044050 0x10>,
> <0x80044010 0x10>,
> <0x800440c0 0x10>;
> reg-names = "base-address",
> "v5ctrl-address",
> "status-address";
> compatible = "fsl,imx28-vdda"; /* handled by mxs-regulator.c */
> regulator-name = "vdda";
> regulator-min-microvolt = <1725000>;
> regulator-max-microvolt = <1950000>;
> vdda-supply = <&reg_vddio>;
> };
>
> reg_vddio: regulator@80044060 {
> reg = <0x80044060 0x10>,
> <0x80044010 0x10>,
> <0x800440c0 0x10>;
> reg-names = "base-address",
> "v5ctrl-address",
> "status-address";
> compatible = "fsl,imx28-vddio"; /* handled by mxs-regulator.c */
> regulator-name = "vddio";
> regulator-min-microvolt = <3000000>;
> regulator-max-microvolt = <3575000>;
> regulator-microvolt-offset = <80000>;
> };
> };
>
> Please correct me if i'm wrong.
>
> Stefan

Gentle Ping.

Stefan

>
> >
> > -- Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Feb. 23, 2015, 3:38 p.m. UTC | #14
On Fri, Feb 20, 2015 at 11:57:41AM +0100, Stefan Wahren wrote:
> > power: power@80044000 {
> > compatible = "fsl,imx28-power"; /* handled by mxs_power.c */
> > #address-cells = <1>;
> > #size-cells = <1>;
> > reg = <0x80044000 0x2000>;
> > interrupts = <6>;
> > switching-frequency = <24000000>; /* new property */
> > ranges;
> >
> > reg_vddd: regulator@80044040 {
> > reg = <0x80044040 0x10>,
> > <0x80044010 0x10>,
> > <0x800440c0 0x10>;
> > reg-names = "base-address",
> > "v5ctrl-address",
> > "status-address";
> > compatible = "fsl,imx28-vddd"; /* handled by mxs-regulator.c */
> > regulator-name = "vddd";
> > regulator-min-microvolt = <1350000>;
> > regulator-max-microvolt = <1550000>;
> > vddd-supply = <&reg_vdda>;
> > };
> >
> > reg_vdda: regulator@80044050 {
> > reg = <0x80044050 0x10>,
> > <0x80044010 0x10>,
> > <0x800440c0 0x10>;
> > reg-names = "base-address",
> > "v5ctrl-address",
> > "status-address";
> > compatible = "fsl,imx28-vdda"; /* handled by mxs-regulator.c */
> > regulator-name = "vdda";
> > regulator-min-microvolt = <1725000>;
> > regulator-max-microvolt = <1950000>;
> > vdda-supply = <&reg_vddio>;
> > };
> >
> > reg_vddio: regulator@80044060 {
> > reg = <0x80044060 0x10>,
> > <0x80044010 0x10>,
> > <0x800440c0 0x10>;
> > reg-names = "base-address",
> > "v5ctrl-address",
> > "status-address";
> > compatible = "fsl,imx28-vddio"; /* handled by mxs-regulator.c */
> > regulator-name = "vddio";
> > regulator-min-microvolt = <3000000>;
> > regulator-max-microvolt = <3575000>;
> > regulator-microvolt-offset = <80000>;
> > };
> > };
> >
> > Please correct me if i'm wrong.
> >
> > Stefan
> 
> Gentle Ping.

The binding looks ok to me. It would be nice to get an ACK of a DT
binding maintainer, though.

-- Sebastian
Stefan Wahren Feb. 26, 2015, 7:20 p.m. UTC | #15
Hi,

> Sebastian Reichel <sre@kernel.org> hat am 23. Februar 2015 um 16:38
> geschrieben:
>
>
> On Fri, Feb 20, 2015 at 11:57:41AM +0100, Stefan Wahren wrote:
> > > power: power@80044000 {
> > > compatible = "fsl,imx28-power"; /* handled by mxs_power.c */
> > > #address-cells = <1>;
> > > #size-cells = <1>;
> > > reg = <0x80044000 0x2000>;
> > > interrupts = <6>;
> > > switching-frequency = <24000000>; /* new property */
> > > ranges;
> > >
> > > reg_vddd: regulator@80044040 {
> > > reg = <0x80044040 0x10>,
> > > <0x80044010 0x10>,
> > > <0x800440c0 0x10>;
> > > reg-names = "base-address",
> > > "v5ctrl-address",
> > > "status-address";
> > > compatible = "fsl,imx28-vddd"; /* handled by mxs-regulator.c */
> > > regulator-name = "vddd";
> > > regulator-min-microvolt = <1350000>;
> > > regulator-max-microvolt = <1550000>;
> > > vddd-supply = <&reg_vdda>;
> > > };
> > >
> > > reg_vdda: regulator@80044050 {
> > > reg = <0x80044050 0x10>,
> > > <0x80044010 0x10>,
> > > <0x800440c0 0x10>;
> > > reg-names = "base-address",
> > > "v5ctrl-address",
> > > "status-address";
> > > compatible = "fsl,imx28-vdda"; /* handled by mxs-regulator.c */
> > > regulator-name = "vdda";
> > > regulator-min-microvolt = <1725000>;
> > > regulator-max-microvolt = <1950000>;
> > > vdda-supply = <&reg_vddio>;
> > > };
> > >
> > > reg_vddio: regulator@80044060 {
> > > reg = <0x80044060 0x10>,
> > > <0x80044010 0x10>,
> > > <0x800440c0 0x10>;
> > > reg-names = "base-address",
> > > "v5ctrl-address",
> > > "status-address";
> > > compatible = "fsl,imx28-vddio"; /* handled by mxs-regulator.c */
> > > regulator-name = "vddio";
> > > regulator-min-microvolt = <3000000>;
> > > regulator-max-microvolt = <3575000>;
> > > regulator-microvolt-offset = <80000>;
> > > };
> > > };
> > >
> > > Please correct me if i'm wrong.
> > >
> > > Stefan
> >
> > Gentle Ping.
>
> The binding looks ok to me. It would be nice to get an ACK of a DT
> binding maintainer, though.

any objections about implementing DC-DC switching frequency as a DT property
instead of module parameter?

Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 0108c2a..8a3eaa4 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -43,6 +43,14 @@  config MAX8925_POWER
 	  Say Y here to enable support for the battery charger in the Maxim
 	  MAX8925 PMIC.

+config MXS_POWER
+	tristate "Freescale MXS power subsystem support"
+	depends on ARCH_MXS
+	help
+	  Say Y here to enable support for the Freescale i.MX23/i.MX28
+	  power subsystem. This is a requirement to get access to on-chip
+	  regulators, battery charger and many more.
+
 config WM831X_BACKUP
 	tristate "WM831X backup battery charger support"
 	depends on MFD_WM831X
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index dfa8942..91c0a62 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -10,6 +10,7 @@  obj-$(CONFIG_GENERIC_ADC_BATTERY)	+= generic-adc-battery.o
 obj-$(CONFIG_PDA_POWER)		+= pda_power.o
 obj-$(CONFIG_APM_POWER)		+= apm_power.o
 obj-$(CONFIG_MAX8925_POWER)	+= max8925_power.o
+obj-$(CONFIG_MXS_POWER)		+= mxs_power.o
 obj-$(CONFIG_WM831X_BACKUP)	+= wm831x_backup.o
 obj-$(CONFIG_WM831X_POWER)	+= wm831x_power.o
 obj-$(CONFIG_WM8350_POWER)	+= wm8350_power.o
diff --git a/drivers/power/mxs_power.c b/drivers/power/mxs_power.c
new file mode 100644
index 0000000..6b68064
--- /dev/null
+++ b/drivers/power/mxs_power.c
@@ -0,0 +1,226 @@ 
+/*
+ * Freescale MXS power subsystem
+ *
+ * Copyright (C) 2014 Stefan Wahren
+ *
+ * Inspired by imx-bootlets
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/types.h>
+
+#define BM_POWER_CTRL_POLARITY_VBUSVALID	(1 << 5)
+#define BM_POWER_CTRL_VBUSVALID_IRQ		(1 << 4)
+#define BM_POWER_CTRL_ENIRQ_VBUS_VALID		(1 << 3)
+
+#define HW_POWER_5VCTRL_OFFSET	0x10
+#define HW_POWER_MISC_OFFSET	0x90
+
+#define BM_POWER_5VCTRL_VBUSVALID_THRESH	(7 << 8)
+#define BM_POWER_5VCTRL_PWDN_5VBRNOUT		(1 << 7)
+#define BM_POWER_5VCTRL_VBUSVALID_5VDETECT	(1 << 4)
+
+#define HW_POWER_5VCTRL_VBUSVALID_THRESH_4_40V	(5 << 8)
+
+#define SHIFT_FREQSEL	4
+
+#define BM_POWER_MISC_FREQSEL			(7 << SHIFT_FREQSEL)
+
+#define HW_POWER_MISC_FREQSEL_20000_KHZ		1
+#define HW_POWER_MISC_FREQSEL_24000_KHZ		2
+#define HW_POWER_MISC_FREQSEL_19200_KHZ		3
+
+#define HW_POWER_MISC_SEL_PLLCLK		(1 << 0)
+
+static int dcdc_pll;
+module_param(dcdc_pll, int, 0);
+MODULE_PARM_DESC(dcdc_pll,
+		 "DC-DC PLL frequency (kHz). Use 19200, 20000 or 24000");
+
+struct mxs_power_data {
+	void __iomem *base_addr;
+	struct power_supply dc;
+};
+
+int get_dcdc_clk_freq(struct mxs_power_data *pdata)
+{
+	void __iomem *base = pdata->base_addr;
+	int ret = -EINVAL;
+	u32 val;
+
+	val = readl(base + HW_POWER_MISC_OFFSET);
+
+	/* XTAL source */
+	if ((val & HW_POWER_MISC_SEL_PLLCLK) == 0)
+		return 24000;
+
+	switch ((val & BM_POWER_MISC_FREQSEL) >> SHIFT_FREQSEL) {
+	case HW_POWER_MISC_FREQSEL_20000_KHZ:
+		ret = 20000;
+		break;
+	case HW_POWER_MISC_FREQSEL_24000_KHZ:
+		ret = 24000;
+		break;
+	case HW_POWER_MISC_FREQSEL_19200_KHZ:
+		ret = 19200;
+		break;
+	}
+
+	return ret;
+}
+
+int set_dcdc_clk_freq(struct mxs_power_data *pdata, int kHz)
+{
+	void __iomem *misc = pdata->base_addr + HW_POWER_MISC_OFFSET;
+	u32 val;
+	int ret = 0;
+
+	val = readl(misc);
+
+	val &= ~BM_POWER_MISC_FREQSEL;
+	val &= ~HW_POWER_MISC_SEL_PLLCLK;
+
+	/* Accept only values recommend by Freescale */
+	switch (kHz) {
+	case 19200:
+		val |= HW_POWER_MISC_FREQSEL_19200_KHZ << SHIFT_FREQSEL;
+		break;
+	case 20000:
+		val |= HW_POWER_MISC_FREQSEL_20000_KHZ << SHIFT_FREQSEL;
+		break;
+	case 24000:
+		val |= HW_POWER_MISC_FREQSEL_24000_KHZ << SHIFT_FREQSEL;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (ret)
+		return ret;
+
+	/* First program FREQSEL */
+	writel(val, misc);
+
+	/* then set PLL as clk for DC-DC converter */
+	writel(val | HW_POWER_MISC_SEL_PLLCLK, misc);
+
+	return 0;
+}
+
+static enum power_supply_property mxs_power_dc_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int mxs_power_dc_get_property(struct power_supply *psy,
+				    enum power_supply_property psp,
+				    union power_supply_propval *val)
+{
+	struct mxs_power_data *data = container_of(psy,
+						   struct mxs_power_data, dc);
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = 1;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static const struct of_device_id of_mxs_power_match[] = {
+	{ .compatible = "fsl,imx23-power" },
+	{ .compatible = "fsl,imx28-power" },
+	{ /* end */ }
+};
+MODULE_DEVICE_TABLE(of, of_mxs_power_match);
+
+static int mxs_power_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *res;
+	struct mxs_power_data *data;
+	int ret;
+	int dcdc_clk_freq;
+
+	if (!np) {
+		dev_err(dev, "missing device tree\n");
+		return -EINVAL;
+	}
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->dc.properties = mxs_power_dc_props;
+	data->dc.num_properties = ARRAY_SIZE(mxs_power_dc_props);
+	data->dc.get_property = mxs_power_dc_get_property;
+	data->dc.name = "dc";
+	data->dc.type = POWER_SUPPLY_TYPE_MAINS;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->base_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->base_addr))
+		return PTR_ERR(data->base_addr);
+
+	ret = power_supply_register(dev, &data->dc);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, data);
+
+	if (dcdc_pll)
+		set_dcdc_clk_freq(data, dcdc_pll);
+
+	dcdc_clk_freq = get_dcdc_clk_freq(data);
+
+	dev_info(dev, "DCDC clock freq: %d kHz\n", dcdc_clk_freq);
+
+	return of_platform_populate(np, NULL, NULL, dev);
+}
+
+static int mxs_power_remove(struct platform_device *pdev)
+{
+	struct mxs_power_data *data = platform_get_drvdata(pdev);
+
+	of_platform_depopulate(&pdev->dev);
+	power_supply_unregister(&data->dc);
+
+	return 0;
+}
+
+static struct platform_driver mxs_power_driver = {
+	.driver = {
+		.name	= "mxs_power",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_mxs_power_match,
+	},
+	.probe	= mxs_power_probe,
+	.remove = mxs_power_remove,
+};
+
+module_platform_driver(mxs_power_driver);
+
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Freescale MXS power subsystem");
+MODULE_LICENSE("GPL v2");