* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ulf Hansson @ 2016-11-24 13:34 UTC (permalink / raw)
To: Ziji Hu, Adrian Hunter
Cc: Gregory CLEMENT, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding, Romain Perier
In-Reply-To: <dd230463-04f6-df31-7056-1a185eb6cfc7@marvell.com>
On 24 November 2016 at 13:41, Ziji Hu <huziji@marvell.com> wrote:
> Hi Ulf,
>
> On 2016/11/24 18:43, Ulf Hansson wrote:
>> On 31 October 2016 at 12:09, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> From: Ziji Hu <huziji@marvell.com>
>>>
> <snip>
>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + unsigned char voltage = ios->signal_voltage;
>>> +
>>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>>> + return __emmc_signal_voltage_switch(mmc, voltage);
>>> +
>>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>> + voltage);
>>> + return -EINVAL;
>>
>> This wrapper function seems unnessarry. It only adds a dev_err(), so
>> then might as well do that in __emmc_signal_voltage_switch().
>>
> Sure. Will merge it back to __emmc_signal_voltage_switch().
>
>>> +}
>>> +
>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /*
>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>> + * disabled. However, sdhci_set_clock will also disable the Internal
>>> + * clock in mmc_set_signal_voltage().
>>
>> If that's the case then that is wrong in the generic sdhci code.
>> What's the reason why it can't be fixed there instead of having this
>> workaround?
>>
> In my very own opinion, SD Spec doesn't specify whether SDCLK should be
> enabled or not during power setting.
> Enabling SDCLK might be a special condition only required by our SDHC.
> I try to avoid breaking other vendors' SDHC functionality
> if their SDHCs require SDCLK disabled.
> Thus I prefer to keep it inside our SDHC driver.
I let Adrian comment on this.
For sure we should avoid breaking other sdhci variant, but on the
other hand *if* the generic code is wrong we should fix it!
>
>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>> + * Thus here manually enable internal clock.
>>> + *
>>> + * After switch completes, it is unnecessary to disable internal clock,
>>> + * since keeping internal clock active obeys SD spec.
>>> + */
>>> + enable_xenon_internal_clk(host);
>>> +
>>> + if (priv->emmc_slot)
>>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>>> +
>>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>>> +}
>>> +
>>> +/*
>>> + * After determining which slot is used for SDIO,
>>> + * some additional task is required.
>>> + */
>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + u32 reg;
>>> + u8 slot_idx;
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + /* Link the card for delay adjustment */
>>> + priv->card_candidate = card;
>>> + /* Set tuning functionality of this slot */
>>> + xenon_slot_tuning_setup(host);
>>
>> This looks weird. I assume this can be done as a part of the regular
>> tuning seqeunce!?
>>
> It is our SDHC specific preparation prior to tuning, rather than a
> standard step in spec.
> Thus I leave it inside our driver.
My point is that this isn't the purpose of ->init_card(). thus you are
abusing it.
Try to make it work in another way, please. I think you can.
>
>>> +
>>> + slot_idx = priv->slot_idx;
>>> + if (!mmc_card_sdio(card)) {
>>> + /* Clear SDIO Card Inserted indication */
>>
>> Why do you need this?
>>
>> If you need to reset this, I think it's better to do it from
>> ->set_ios() at MMC_POWER_OFF.
>>
> This field indicates SDIO card and controls async interrupt feature
> of SDIO in our SDHC.
> This async interrupt feature is enabled when SDIO card is inserted.
> It should be disabled if SD card is inserted instead.
What do you mean by SDIO async interupts? Are you talking about SDIO
irqs on DAT1 line?
Those is supposed to be enabled when someone explicitly requests them,
not when the card is inserted.
In other words when an SDIO func driver have called sdio_claim_irq().
Moreover, we have ->enable_sdio_irq() ops that deals with this.
[...]
>>> +
>>> + /*
>>> + * Xenon Specific property:
>>> + * emmc: explicitly indicate whether this slot is for eMMC
>>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>>> + * tun-count: the interval between re-tuning
>>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>> + */
>>> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>> + priv->emmc_slot = true;
>>
>> So, you need this because of the eMMC voltage switch behaviour, right?
>>
>> Then I would rather like to describe this a generic DT bindings for
>> the eMMC voltage level support. There have acutally been some earlier
>> discussions for this, but we haven't yet made some changes.
>>
>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>> supported the ->start_signal_voltage_switch() ops may return -EINVAL.
>> This would inform the mmc core to move on to the next supported
>> voltage level. There might be some minor additional changes to the mmc
>> card initialization sequence, but those should be simple.
>>
>> I can help out to look into this, unless you want to do it yourself of course!?
>>
> Yes. One of the reasons is to provide eMMC specific voltage setting.
> But in my very own opinion, it should be irrelevant to voltage level.
> The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch.
> It will become more complex with different SOC implementation details.
Got it. Although I think we can cope with that fine just by using the
different SD/eMMC speed modes settings defined in DT (or from the
SDHCI caps register)
> Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage
> setting should be executed.
> Thus an flag is required here to tell driver to execute eMMC voltage setting.
>
> Besides, additional eMMC specific settings might be implemented in future, besides
> voltage setting. Most of them should be completed before MMC driver recognizes the
> card type. Thus I have to keep this flag to indicate current SDHC is for eMMC.
I doubt you will need a generic "eMMC" flag, but let's see when we go forward.
Currently it's clear you don't need such a flag, so I will submit a
change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
there, to see if it suits your needs.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 0/2] OF phandle nexus support + GPIO nexus
From: Pantelis Antoniou @ 2016-11-24 12:47 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, Linus Walleij, linux-kernel, linux-gpio, Rob Herring,
Mark Brown, Frank Rowand, linux-arm-kernel
In-Reply-To: <20161124102529.20212-1-stephen.boyd@linaro.org>
Hi Stephen,
> On Nov 24, 2016, at 12:25 , Stephen Boyd <stephen.boyd@linaro.org> wrote:
>
> This is one small chunk of work related to DT overlays for expansion
> boards. It would be good to have a way to expose #<list>-cells types of
> providers through a connector in a standard way. So we introduce a way
> to make "nexus" nodes for these types of properties to remap the consumer
> number space to the other side of the connector's number space. It's
> basically a copy of the interrupt nexus implementation, but without
> the address space matching design and interrupt-parent walking.
>
> The first patch implements a generic method to do this, and the second patch
> adds a unit test for it. The third patch is more of an example than anything
> else. It shows how we would modify frameworks to use the new API.
>
Excellent. It was about time this happened.
> Stephen Boyd (3):
> of: Support parsing phandle argument lists through a nexus node
> of: unittest: Add phandle remapping test
> gpio: Support gpio nexus dt bindings
>
> drivers/gpio/gpiolib-of.c | 5 +-
> drivers/of/base.c | 146 ++++++++++++++++++++++++++++
> drivers/of/unittest-data/testcases.dts | 11 +++
> drivers/of/unittest-data/tests-phandle.dtsi | 24 +++++
> drivers/of/unittest.c | 124 +++++++++++++++++++++++
> include/linux/of.h | 14 +++
> 6 files changed, 322 insertions(+), 2 deletions(-)
>
> --
> 2.10.0.297.gf6727b0
>
Comments inline…
Regards
— Pantelis
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 4/7] clk: qcom: ipq4019: Added all the frequencies for apps cpu
From: Abhishek Sahu @ 2016-11-24 12:46 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, mturquette, galak, pradeepb, mmcclint, varada,
sricharan, architt, ntelkar, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, devicetree
In-Reply-To: <20161102012450.GD16026@codeaurora.org>
On 2016-11-02 06:54, Stephen Boyd wrote:
> On 09/21, Abhishek Sahu wrote:
>> The APPS CPU clock does not contain all the frequencies in its
>> frequency table so this patch adds the same.
>>
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>> drivers/clk/qcom/gcc-ipq4019.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-ipq4019.c
>> b/drivers/clk/qcom/gcc-ipq4019.c
>> index 211c68c..160e0cf 100644
>> --- a/drivers/clk/qcom/gcc-ipq4019.c
>> +++ b/drivers/clk/qcom/gcc-ipq4019.c
>> @@ -565,10 +565,20 @@ static struct clk_rcg2 sdcc1_apps_clk_src = {
>> };
>>
>> static const struct freq_tbl ftbl_gcc_apps_clk[] = {
>> - F(48000000, P_XO, 1, 0, 0),
>> + F(48000000, P_XO, 1, 0, 0),
>> F(200000000, P_FEPLL200, 1, 0, 0),
>> + F(380000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(409000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(444000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(484000000, P_DDRPLLAPSS, 1, 0, 0),
>> F(500000000, P_FEPLL500, 1, 0, 0),
>> + F(507000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(532000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(560000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(592000000, P_DDRPLLAPSS, 1, 0, 0),
>> F(626000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(666000000, P_DDRPLLAPSS, 1, 0, 0),
>> + F(710000000, P_DDRPLLAPSS, 1, 0, 0),
>> { }
>> };
>
> Can't we have the determine_rate callback know the speeds of the
> "fixed" PLLs and use those first if the rate hits exactly? And
> then if that doesn't happen go try ddrpllapps and set the rate on
> it? I'm hoping we can get rid of this frequency table.
This clock is being registered with QCOM clk_rcg2 operations which
already has determine_rate callback based on this frequency table.
Currently all the frequencies are being generated without HID
divider but in future, we can have some frequency which will use
dividers also.
--
Abhishek Sahu
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ziji Hu @ 2016-11-24 12:41 UTC (permalink / raw)
To: Ulf Hansson, Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding, Romain Perier
In-Reply-To: <CAPDyKFpqPSqiEi=0UW5LoZmy+y-KHm9nbcGKBSy3RzchdLU9cA@mail.gmail.com>
Hi Ulf,
On 2016/11/24 18:43, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> From: Ziji Hu <huziji@marvell.com>
>>
<snip>
>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + unsigned char voltage = ios->signal_voltage;
>> +
>> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> + (voltage == MMC_SIGNAL_VOLTAGE_180))
>> + return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> + voltage);
>> + return -EINVAL;
>
> This wrapper function seems unnessarry. It only adds a dev_err(), so
> then might as well do that in __emmc_signal_voltage_switch().
>
Sure. Will merge it back to __emmc_signal_voltage_switch().
>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + /*
>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>> + * disabled. However, sdhci_set_clock will also disable the Internal
>> + * clock in mmc_set_signal_voltage().
>
> If that's the case then that is wrong in the generic sdhci code.
> What's the reason why it can't be fixed there instead of having this
> workaround?
>
In my very own opinion, SD Spec doesn't specify whether SDCLK should be
enabled or not during power setting.
Enabling SDCLK might be a special condition only required by our SDHC.
I try to avoid breaking other vendors' SDHC functionality
if their SDHCs require SDCLK disabled.
Thus I prefer to keep it inside our SDHC driver.
>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>> + * Thus here manually enable internal clock.
>> + *
>> + * After switch completes, it is unnecessary to disable internal clock,
>> + * since keeping internal clock active obeys SD spec.
>> + */
>> + enable_xenon_internal_clk(host);
>> +
>> + if (priv->emmc_slot)
>> + return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> + return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + u32 reg;
>> + u8 slot_idx;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + /* Link the card for delay adjustment */
>> + priv->card_candidate = card;
>> + /* Set tuning functionality of this slot */
>> + xenon_slot_tuning_setup(host);
>
> This looks weird. I assume this can be done as a part of the regular
> tuning seqeunce!?
>
It is our SDHC specific preparation prior to tuning, rather than a
standard step in spec.
Thus I leave it inside our driver.
>> +
>> + slot_idx = priv->slot_idx;
>> + if (!mmc_card_sdio(card)) {
>> + /* Clear SDIO Card Inserted indication */
>
> Why do you need this?
>
> If you need to reset this, I think it's better to do it from
> ->set_ios() at MMC_POWER_OFF.
>
This field indicates SDIO card and controls async interrupt feature
of SDIO in our SDHC.
This async interrupt feature is enabled when SDIO card is inserted.
It should be disabled if SD card is inserted instead.
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> +
>> + if (mmc_card_mmc(card)) {
>> + mmc->caps |= MMC_CAP_NONREMOVABLE;
>> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>> + mmc->caps |= MMC_CAP_1_8V_DDR;
>> + /*
>> + * Force to clear BUS_TEST to
>> + * skip bus_test_pre and bus_test_post
>> + */
>> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>> + MMC_CAP2_PACKED_CMD;
>> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
>> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
>
> Most of this can be specified as DT configurations. Please use that instead.
>
> More importantly, please don't use the ->init_card() ops to assign
> host caps. If not DT, please do it from ->probe().
>
Sure. Will try to use DT instead.
>> + }
>> + } else {
>> + /*
>> + * Set SDIO Card Inserted indication
>> + * to inform that the current slot is for SDIO
>> + */
>> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + reg |= (1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>
> So this makes sence to have in the ->init_card() ops. The rest above, not.
>
>> + }
>> +}
>> +
>> +static int xenon_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> +
>> + if (host->timing == MMC_TIMING_UHS_DDR50)
>> + return 0;
>> +
>> + return sdhci_execute_tuning(mmc, opcode);
>> +}
>> +
>> +static void xenon_replace_mmc_host_ops(struct sdhci_host *host)
>> +{
>> + host->mmc_host_ops.set_ios = xenon_set_ios;
>> + host->mmc_host_ops.start_signal_voltage_switch =
>> + xenon_start_signal_voltage_switch;
>> + host->mmc_host_ops.init_card = xenon_init_card;
>> + host->mmc_host_ops.execute_tuning = xenon_execute_tuning;
>> +}
>> +
>> +static int xenon_probe_dt(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct mmc_host *mmc = host->mmc;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int err;
>> + u32 slot_idx, nr_slot;
>> + u32 tuning_count;
>> + u32 reg;
>> +
>> + /* Standard MMC property */
>> + err = mmc_of_parse(mmc);
>> + if (err)
>> + return err;
>> +
>> + /* Standard SDHCI property */
>> + sdhci_get_of_property(pdev);
>> +
>> + /*
>> + * Xenon Specific property:
>> + * emmc: explicitly indicate whether this slot is for eMMC
>> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>> + * tun-count: the interval between re-tuning
>> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>> + */
>> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
>> + priv->emmc_slot = true;
>
> So, you need this because of the eMMC voltage switch behaviour, right?
>
> Then I would rather like to describe this a generic DT bindings for
> the eMMC voltage level support. There have acutally been some earlier
> discussions for this, but we haven't yet made some changes.
>
> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
> allows the host driver to accept I/O voltage switches to 3.3V. If not
> supported the ->start_signal_voltage_switch() ops may return -EINVAL.
> This would inform the mmc core to move on to the next supported
> voltage level. There might be some minor additional changes to the mmc
> card initialization sequence, but those should be simple.
>
> I can help out to look into this, unless you want to do it yourself of course!?
>
Yes. One of the reasons is to provide eMMC specific voltage setting.
But in my very own opinion, it should be irrelevant to voltage level.
The eMMC voltage setting on our SDHC is different from SD/SDIO voltage switch.
It will become more complex with different SOC implementation details.
Unfortunately, MMC driver cannot determine the card type yet when eMMC voltage
setting should be executed.
Thus an flag is required here to tell driver to execute eMMC voltage setting.
Besides, additional eMMC specific settings might be implemented in future, besides
voltage setting. Most of them should be completed before MMC driver recognizes the
card type. Thus I have to keep this flag to indicate current SDHC is for eMMC.
>> + else
>> + priv->emmc_slot = false;
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-slotno", &slot_idx)) {
>> + nr_slot = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> + nr_slot &= NR_SUPPORTED_SLOT_MASK;
>> + if (unlikely(slot_idx > nr_slot)) {
>> + dev_err(mmc_dev(mmc), "Slot Index %d exceeds Number of slots %d\n",
>> + slot_idx, nr_slot);
>> + return -EINVAL;
>> + }
>> + } else {
>> + priv->slot_idx = 0x0;
>> + }
>> +
>> + if (!of_property_read_u32(np, "marvell,xenon-tun-count",
>> + &tuning_count)) {
>> + if (unlikely(tuning_count >= TMR_RETUN_NO_PRESENT)) {
>> + dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
>> + DEF_TUNING_COUNT);
>> + tuning_count = DEF_TUNING_COUNT;
>> + }
>> + } else {
>> + priv->tuning_count = DEF_TUNING_COUNT;
>> + }
>
> To make the code a bit easier...
>
> Maybe set "priv->tuning_count = DEF_TUNING_COUNT" before the "if", and
> instead have the of_property_read_u32() to update the value when set.
>
Yes. You are correct.
>> +
>> + if (of_property_read_bool(np, "marvell,xenon-mask-conflict-err")) {
>> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
>> + reg |= MASK_CMD_CONFLICT_ERROR;
>> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static int xenon_slot_probe(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* Enable slot */
>> + xenon_enable_slot(host, slot_idx);
>> +
>> + /* Enable ACG */
>> + xenon_set_acg(host, true);
>> +
>> + /* Enable Parallel Transfer Mode */
>> + xenon_enable_slot_parallel_tran(host, slot_idx);
>> +
>> + priv->timing = MMC_TIMING_FAKE;
>> + priv->clock = 0;
>
> What are these used for?
>
During card initialization, our SDHC PHY setting depends on current
timing and SDCLK frequency.
priv->timing and priv->clock will be used in PHY setting later.
It can be considered as a clean-up.
Anyway, it does look ugly. I will improve them after our PHY setting
passes your review.
>> +
>> + return 0;
>> +}
>> +
>> +static void xenon_slot_remove(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + u8 slot_idx = priv->slot_idx;
>> +
>> + /* disable slot */
>> + xenon_disable_slot(host, slot_idx);
>> +}
>> +
>> +static int sdhci_xenon_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_host *host;
>> + struct clk *clk, *axi_clk;
>> + struct sdhci_xenon_priv *priv;
>> + int err;
>> +
>> + host = sdhci_pltfm_init(pdev, &sdhci_xenon_pdata,
>> + sizeof(struct sdhci_xenon_priv));
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + pltfm_host = sdhci_priv(host);
>> + priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> + xenon_set_acg(host, false);
>> +
>> + /*
>> + * Link Xenon specific mmc_host_ops function,
>> + * to replace standard ones in sdhci_ops.
>> + */
>> + xenon_replace_mmc_host_ops(host);
>> +
>> + clk = devm_clk_get(&pdev->dev, "core");
>> + if (IS_ERR(clk)) {
>> + dev_err(&pdev->dev, "Failed to setup input clk.\n");
>> + err = PTR_ERR(clk);
>> + goto free_pltfm;
>> + }
>> + clk_prepare_enable(clk);
>
> Check error code.
>
>> + pltfm_host->clk = clk;
>
> Why not assign pltfm_host->clk immedately when doing devm_clk_get(),
> that would make this a bit cleaner, right?
>
Yes, of course.
>> +
>> + /*
>> + * Some SOCs require additional clock to
>> + * manage AXI bus clock.
>> + * It is optional.
>> + */
>> + axi_clk = devm_clk_get(&pdev->dev, "axi");
>> + if (!IS_ERR(axi_clk)) {
>> + clk_prepare_enable(axi_clk);
>> + priv->axi_clk = axi_clk;
>> + }
>
> Same comments as for the above core clock.
>
OK.
>> +
>> + err = xenon_probe_dt(pdev);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = xenon_slot_probe(host);
>> + if (err)
>> + goto err_clk;
>> +
>> + err = sdhci_add_host(host);
>> + if (err)
>> + goto remove_slot;
>> +
>> + return 0;
>> +
>> +remove_slot:
>> + xenon_slot_remove(host);
>> +err_clk:
>> + clk_disable_unprepare(pltfm_host->clk);
>> + if (!IS_ERR(axi_clk))
>> + clk_disable_unprepare(axi_clk);
>> +free_pltfm:
>> + sdhci_pltfm_free(pdev);
>> + return err;
>> +}
>> +
>> +static int sdhci_xenon_remove(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
>> +
>> + xenon_slot_remove(host);
>> +
>> + sdhci_remove_host(host, dead);
>> +
>> + clk_disable_unprepare(pltfm_host->clk);
>> + clk_disable_unprepare(priv->axi_clk);
>> +
>> + sdhci_pltfm_free(pdev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
>> + { .compatible = "marvell,xenon-sdhci",},
>> + { .compatible = "marvell,armada-3700-sdhci",},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>> +
>> +static struct platform_driver sdhci_xenon_driver = {
>> + .driver = {
>> + .name = "xenon-sdhci",
>> + .of_match_table = sdhci_xenon_dt_ids,
>> + .pm = &sdhci_pltfm_pmops,
>> + },
>> + .probe = sdhci_xenon_probe,
>> + .remove = sdhci_xenon_remove,
>> +};
>> +
>> +module_platform_driver(sdhci_xenon_driver);
>> +
>> +MODULE_DESCRIPTION("SDHCI platform driver for Marvell Xenon SDHC");
>> +MODULE_AUTHOR("Hu Ziji <huziji@marvell.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
>> new file mode 100644
>> index 000000000000..4601d0a4b22f
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
>
> I don't think you need a specific header for this, let's instead just
> put everthing in the c-file.
>
Some definitions inside this file will also be referred in PHY setting in
sdhci-xenon-phy.c.
Thus I put all the definitions together into a header file.
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author: Hu Ziji <huziji@marvell.com>
>> + * Date: 2016-8-24
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + */
>> +#ifndef SDHCI_XENON_H_
>> +#define SDHCI_XENON_H_
>> +
>> +#include <linux/clk.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/of.h>
>> +#include "sdhci.h"
>> +
>> +/* Register Offset of SD Host Controller SOCP self-defined register */
>> +#define SDHC_SYS_CFG_INFO 0x0104
>> +#define SLOT_TYPE_SDIO_SHIFT 24
>> +#define SLOT_TYPE_EMMC_MASK 0xFF
>> +#define SLOT_TYPE_EMMC_SHIFT 16
>> +#define SLOT_TYPE_SD_SDIO_MMC_MASK 0xFF
>> +#define SLOT_TYPE_SD_SDIO_MMC_SHIFT 8
>> +#define NR_SUPPORTED_SLOT_MASK 0x7
>> +
>> +#define SDHC_SYS_OP_CTRL 0x0108
>> +#define AUTO_CLKGATE_DISABLE_MASK BIT(20)
>> +#define SDCLK_IDLEOFF_ENABLE_SHIFT 8
>> +#define SLOT_ENABLE_SHIFT 0
>> +
>> +#define SDHC_SYS_EXT_OP_CTRL 0x010C
>> +#define MASK_CMD_CONFLICT_ERROR BIT(8)
>> +
>> +#define SDHC_SLOT_OP_STATUS_CTRL 0x0128
>> +#define DELAY_90_DEGREE_MASK_EMMC5 BIT(7)
>> +#define DELAY_90_DEGREE_SHIFT_EMMC5 7
>> +#define EMMC_5_0_PHY_FIXED_DELAY_MASK 0x7F
>> +#define EMMC_PHY_FIXED_DELAY_MASK 0xFF
>> +#define EMMC_PHY_FIXED_DELAY_WINDOW_MIN (EMMC_PHY_FIXED_DELAY_MASK >> 3)
>> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
>> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN (SDH_PHY_FIXED_DELAY_MASK >> 4)
>> +
>> +#define TUN_CONSECUTIVE_TIMES_SHIFT 16
>> +#define TUN_CONSECUTIVE_TIMES_MASK 0x7
>> +#define TUN_CONSECUTIVE_TIMES 0x4
>> +#define TUNING_STEP_SHIFT 12
>> +#define TUNING_STEP_MASK 0xF
>> +#define TUNING_STEP_DIVIDER BIT(6)
>> +
>> +#define FORCE_SEL_INVERSE_CLK_SHIFT 11
>> +
>> +#define SDHC_SLOT_EMMC_CTRL 0x0130
>> +#define ENABLE_DATA_STROBE BIT(24)
>> +#define SET_EMMC_RSTN BIT(16)
>> +#define DISABLE_RD_DATA_CRC BIT(14)
>> +#define DISABLE_CRC_STAT_TOKEN BIT(13)
>> +#define EMMC_VCCQ_MASK 0x3
>> +#define EMMC_VCCQ_1_8V 0x1
>> +#define EMMC_VCCQ_3_3V 0x3
>> +
>> +#define SDHC_SLOT_RETUNING_REQ_CTRL 0x0144
>> +/* retuning compatible */
>> +#define RETUNING_COMPATIBLE 0x1
>> +
>> +#define SDHC_SLOT_EXT_PRESENT_STATE 0x014C
>> +#define LOCK_STATE 0x1
>> +
>> +#define SDHC_SLOT_DLL_CUR_DLY_VAL 0x0150
>> +
>> +/* Tuning Parameter */
>> +#define TMR_RETUN_NO_PRESENT 0xF
>> +#define DEF_TUNING_COUNT 0x9
>> +
>> +#define MMC_TIMING_FAKE 0xFF
>> +
>> +#define DEFAULT_SDCLK_FREQ (400000)
>> +
>> +/* Xenon specific Mode Select value */
>> +#define XENON_SDHCI_CTRL_HS200 0x5
>> +#define XENON_SDHCI_CTRL_HS400 0x6
>
> For all defines above:
>
> All these defines needs some *SDHCI* prefix. Can you please update that.
Sure. Will add prefix for all of them.
>
>> +
>> +struct sdhci_xenon_priv {
>> + /*
>> + * The bus_width, timing, and clock fields in below
>> + * record the current setting of Xenon SDHC.
>> + * Driver will call a Sampling Fixed Delay Adjustment
>> + * if any setting is changed.
>> + */
>> + unsigned char bus_width;
>> + unsigned char timing;
>
> These two are not used. Please remove.
>
The above two variables will be used in PHY setting
in sdhci-xenon-phy.c.
Could you please help review them in next patch?
>> + unsigned char tuning_count;
>> + unsigned int clock;
>
> "clock" isn't used, please remove.
>
It will be accessed in PHY setting in sdhci-xenon-phy.c.
Could you please help review it in next patch?
>> + struct clk *axi_clk;
>> +
>> + /* Slot idx */
>> + u8 slot_idx;
>> + /* Whether this slot is for eMMC */
>> + bool emmc_slot;
>> +
>> + /*
>> + * When initializing card, Xenon has to determine card type and
>> + * adjust Sampling Fixed delay for the speed mode in which
>> + * DLL tuning is not support.
>> + * However, at that time, card structure is not linked to mmc_host.
>> + * Thus a card pointer is added here to provide
>> + * the delay adjustment function with the card structure
>> + * of the card during initialization.
>> + *
>> + * It is only valid during initialization after it is updated in
>> + * xenon_init_card().
>> + * Do not access this variable in normal transfers after
>> + * initialization completes.
>> + */
>> + struct mmc_card *card_candidate;
>
> Not activley used in this change, please remove and let's discuss it
> in the next step.
>
This varible will be accessed in PHY setting in sdhci-xenon-phy.c.
I would like to discuss about it in PHY file. Could you please help
review it in next patch?
Thank you.
Best regards,
Hu Ziji
>> +};
>> +
>> +static inline int enable_xenon_internal_clk(struct sdhci_host *host)
>> +{
>> + u32 reg;
>> + u8 timeout;
>> +
>> + reg = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
>> + reg |= SDHCI_CLOCK_INT_EN;
>> + sdhci_writel(host, reg, SDHCI_CLOCK_CONTROL);
>> + /* Wait max 20 ms */
>> + timeout = 20;
>> + while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>> + & SDHCI_CLOCK_INT_STABLE)) {
>> + if (timeout == 0) {
>> + pr_err("%s: Internal clock never stabilised.\n",
>> + mmc_hostname(host->mmc));
>> + return -ETIMEDOUT;
>> + }
>> + timeout--;
>> + mdelay(1);
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> --
>> git-series 0.8.10
>
> Kind regards
> Uffe
>
^ permalink raw reply
* [PATCH v3 2/2] dt-bindings: power: add bindings for sbs-charger
From: Nicolas Saenz Julienne @ 2016-11-24 12:33 UTC (permalink / raw)
To: sre, robh+dt, mark.rutland
Cc: linux-kernel, devicetree, linux-pm, nicolas.saenz
In-Reply-To: <1479990823-25841-1-git-send-email-nicolas.saenz@prodys.net>
Adds device tree documentation for SBS charger compilant devices as defined
here: http://sbs-forum.org/specs/sbc110.pdf
Signed-off-by: Nicolas Saenz Julienne <nicolas.saenz@prodys.net>
---
v2 -> v3:
- add part number as compatible
.../bindings/power/supply/sbs_sbs-charger.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
diff --git a/Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt b/Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
new file mode 100644
index 0000000..f6b6027
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
@@ -0,0 +1,24 @@
+SBS sbs-charger
+~~~~~~~~~~
+
+Required properties:
+ - compatible: should contain one of the following:
+ - "lltc,ltc4100"
+ - "sbs,sbs-charger"
+
+Optional properties:
+- interrupt-parent: Should be the phandle for the interrupt controller. Use in
+ conjunction with "interrupts".
+- interrupts: Interrupt mapping for GPIO IRQ. Use in conjunction with
+ "interrupt-parent". If an interrupt is not provided the driver will switch
+ automatically to polling.
+
+Example:
+
+ ltc4100@9 {
+ compatible = "sbs,sbs-charger";
+ reg = <0x9>;
+ interrupt-parent = <&gpio6>;
+ interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+ };
+
--
2.7.4
^ permalink raw reply related
* [PATCH v3 1/2] power: supply: add sbs-charger driver
From: Nicolas Saenz Julienne @ 2016-11-24 12:33 UTC (permalink / raw)
To: sre, robh+dt, mark.rutland
Cc: linux-kernel, devicetree, linux-pm, nicolas.saenz
In-Reply-To: <1479990823-25841-1-git-send-email-nicolas.saenz@prodys.net>
This adds support for sbs-charger compilant chips as defined here:
http://sbs-forum.org/specs/sbc110.pdf
This was tested on a arm board connected to an LTC41000 battery charger
chip.
Signed-off-by: Nicolas Saenz Julienne <nicolas.saenz@prodys.net>
---
v2 -> v3:
- add readable_reg() function to regmap config
- update compatible strings with part number
v1 -> v2:
- add spec link in header
- use proper gpio/interrupt interface
- update regmap configuration (max register & endianness)
- dropped oldschool .supplied_to assignments
- use devm_* APIs
drivers/power/supply/Kconfig | 6 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sbs-charger.c | 275 +++++++++++++++++++++++++++++++++++++
3 files changed, 282 insertions(+)
create mode 100644 drivers/power/supply/sbs-charger.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 76806a0..42877ff 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -164,6 +164,12 @@ config BATTERY_SBS
Say Y to include support for SBS battery driver for SBS-compliant
gas gauges.
+config CHARGER_SBS
+ tristate "SBS Compliant charger"
+ depends on I2C
+ help
+ Say Y to include support for SBS compilant battery chargers.
+
config BATTERY_BQ27XXX
tristate "BQ27xxx battery driver"
help
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 36c599d..06d9ef5 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_BATTERY_COLLIE) += collie_battery.o
obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o
obj-$(CONFIG_BATTERY_SBS) += sbs-battery.o
+obj-$(CONFIG_CHARGER_SBS) += sbs-charger.o
obj-$(CONFIG_BATTERY_BQ27XXX) += bq27xxx_battery.o
obj-$(CONFIG_BATTERY_BQ27XXX_I2C) += bq27xxx_battery_i2c.o
obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o
diff --git a/drivers/power/supply/sbs-charger.c b/drivers/power/supply/sbs-charger.c
new file mode 100644
index 0000000..2c4cd45
--- /dev/null
+++ b/drivers/power/supply/sbs-charger.c
@@ -0,0 +1,275 @@
+/*
+ * Copyright (c) 2016, Prodys S.L.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This adds support for sbs-charger compilant chips as defined here:
+ * http://sbs-forum.org/specs/sbc110.pdf
+ *
+ * Implemetation based on sbs-battery.c
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/power_supply.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_gpio.h>
+#include <linux/bitops.h>
+
+#define SBS_CHARGER_REG_SPEC_INFO 0x11
+#define SBS_CHARGER_REG_STATUS 0x13
+#define SBS_CHARGER_REG_ALARM_WARNING 0x16
+
+#define SBS_CHARGER_STATUS_CHARGE_INHIBITED BIT(1)
+#define SBS_CHARGER_STATUS_RES_COLD BIT(9)
+#define SBS_CHARGER_STATUS_RES_HOT BIT(10)
+#define SBS_CHARGER_STATUS_BATTERY_PRESENT BIT(14)
+#define SBS_CHARGER_STATUS_AC_PRESENT BIT(15)
+
+#define SBS_CHARGER_POLL_TIME 500
+
+struct sbs_info {
+ struct i2c_client *client;
+ struct power_supply *power_supply;
+ struct regmap *regmap;
+ struct delayed_work work;
+ unsigned int last_state;
+};
+
+static int sbs_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct sbs_info *chip = power_supply_get_drvdata(psy);
+ unsigned int reg;
+
+ reg = chip->last_state;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = !!(reg & SBS_CHARGER_STATUS_BATTERY_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_ONLINE:
+ val->intval = !!(reg & SBS_CHARGER_STATUS_AC_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_STATUS:
+ val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+
+ if (!(reg & SBS_CHARGER_STATUS_BATTERY_PRESENT))
+ val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ else if (reg & SBS_CHARGER_STATUS_AC_PRESENT &&
+ !(reg & SBS_CHARGER_STATUS_CHARGE_INHIBITED))
+ val->intval = POWER_SUPPLY_STATUS_CHARGING;
+ else
+ val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+
+ break;
+
+ case POWER_SUPPLY_PROP_HEALTH:
+ if (reg & SBS_CHARGER_STATUS_RES_COLD)
+ val->intval = POWER_SUPPLY_HEALTH_COLD;
+ if (reg & SBS_CHARGER_STATUS_RES_HOT)
+ val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
+ else
+ val->intval = POWER_SUPPLY_HEALTH_GOOD;
+
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int sbs_check_state(struct sbs_info *chip)
+{
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(chip->regmap, SBS_CHARGER_REG_STATUS, ®);
+ if (!ret && reg != chip->last_state) {
+ chip->last_state = reg;
+ power_supply_changed(chip->power_supply);
+ return 1;
+ }
+
+ return 0;
+}
+
+static void sbs_delayed_work(struct work_struct *work)
+{
+ struct sbs_info *chip = container_of(work, struct sbs_info, work.work);
+
+ sbs_check_state(chip);
+
+ schedule_delayed_work(&chip->work,
+ msecs_to_jiffies(SBS_CHARGER_POLL_TIME));
+}
+
+static irqreturn_t sbs_irq_thread(int irq, void *data)
+{
+ struct sbs_info *chip = data;
+ int ret;
+
+ ret = sbs_check_state(chip);
+
+ return ret ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static enum power_supply_property sbs_properties[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_HEALTH,
+};
+
+static bool sbs_readable_reg(struct device *dev, unsigned int reg)
+{
+ if (reg < SBS_CHARGER_REG_SPEC_INFO)
+ return false;
+ else
+ return true;
+}
+
+static bool sbs_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case SBS_CHARGER_REG_STATUS:
+ return true;
+ }
+
+ return false;
+}
+
+static const struct regmap_config sbs_regmap = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .max_register = SBS_CHARGER_REG_ALARM_WARNING,
+ .readable_reg = sbs_readable_reg,
+ .volatile_reg = sbs_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_LITTLE, /* since based on SMBus */
+};
+
+static const struct power_supply_desc sbs_desc = {
+ .name = "sbs-charger",
+ .type = POWER_SUPPLY_TYPE_MAINS,
+ .properties = sbs_properties,
+ .num_properties = ARRAY_SIZE(sbs_properties),
+ .get_property = sbs_get_property,
+};
+
+static int sbs_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct power_supply_config psy_cfg = {};
+ struct sbs_info *chip;
+ int ret, val;
+
+ chip = devm_kzalloc(&client->dev, sizeof(struct sbs_info), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->client = client;
+ psy_cfg.of_node = client->dev.of_node;
+ psy_cfg.drv_data = chip;
+
+ i2c_set_clientdata(client, chip);
+
+ chip->regmap = devm_regmap_init_i2c(client, &sbs_regmap);
+ if (IS_ERR(chip->regmap))
+ return PTR_ERR(chip->regmap);
+
+ /*
+ * Before we register, we need to make sure we can actually talk
+ * to the battery.
+ */
+ ret = regmap_read(chip->regmap, SBS_CHARGER_REG_STATUS, &val);
+ if (ret) {
+ dev_err(&client->dev, "Failed to get device status\n");
+ return ret;
+ }
+ chip->last_state = val;
+
+ chip->power_supply = devm_power_supply_register(&client->dev, &sbs_desc,
+ &psy_cfg);
+ if (IS_ERR(chip->power_supply)) {
+ dev_err(&client->dev, "Failed to register power supply\n");
+ return PTR_ERR(chip->power_supply);
+ }
+
+ /*
+ * The sbs-charger spec doesn't impose the use of an interrupt. So in
+ * the case it wasn't provided we use polling in order get the charger's
+ * status.
+ */
+ if (client->irq) {
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL, sbs_irq_thread,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ dev_name(&client->dev), chip);
+ if (ret) {
+ dev_err(&client->dev, "Failed to request irq, %d\n", ret);
+ return ret;
+ }
+ } else {
+ INIT_DELAYED_WORK(&chip->work, sbs_delayed_work);
+ schedule_delayed_work(&chip->work,
+ msecs_to_jiffies(SBS_CHARGER_POLL_TIME));
+ }
+
+ dev_info(&client->dev,
+ "%s: smart charger device registered\n", client->name);
+
+ return 0;
+}
+
+static int sbs_remove(struct i2c_client *client)
+{
+ struct sbs_info *chip = i2c_get_clientdata(client);
+
+ cancel_delayed_work_sync(&chip->work);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id sbs_dt_ids[] = {
+ { .compatible = "lltc,ltc4100" },
+ { .compatible = "sbs,sbs-charger" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sbs_dt_ids);
+#endif
+
+static const struct i2c_device_id sbs_id[] = {
+ { "sbs-charger", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sbs_id);
+
+static struct i2c_driver sbs_driver = {
+ .probe = sbs_probe,
+ .remove = sbs_remove,
+ .id_table = sbs_id,
+ .driver = {
+ .name = "sbs-charger",
+ .of_match_table = of_match_ptr(sbs_dt_ids),
+ },
+};
+module_i2c_driver(sbs_driver);
+
+MODULE_AUTHOR("Nicolas Saenz Julienne <nicolassaenzj@gmail.com>");
+MODULE_DESCRIPTION("SBS smart charger driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related
* [PATCH v3 0/2] power: supply: add sbs-charger driver
From: Nicolas Saenz Julienne @ 2016-11-24 12:33 UTC (permalink / raw)
To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
nicolas.saenz-gbiq2sxWoaasTnJN9+BGXg
Hi,
This series adds support for all SBS compatible battery chargers, as defined
here: http://sbs-forum.org/specs/sbc110.pdf.
The first patch changes the sbs-battery device name in order to be able to
create a proper supplier/supplied relation between the two of them.
The second introduces the driver.
Regards,
Nicolas
changes since v2:
- updated driver and dt-binding with Sebatian's comments
changes since v1:
- added dt bindings
- updated driver with Sebastian's comments
- s/Nicola/Nicolas/ in commits
Nicolas Saenz Julienne (2):
power: supply: add sbs-charger driver
dt-bindings: power: add bindings for sbs-charger
.../bindings/power/supply/sbs_sbs-charger.txt | 24 ++
drivers/power/supply/Kconfig | 6 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sbs-charger.c | 275 +++++++++++++++++++++
4 files changed, 306 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
create mode 100644 drivers/power/supply/sbs-charger.c
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] drivers/of: Export phandle iterators
From: Robin Murphy @ 2016-11-24 12:33 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Frank Rowand
In-Reply-To: <CAL_JsqLNCZnKyUmzYAuJSLhVNf1MhnSp_uEw4jn=WcLsw2GCZQ@mail.gmail.com>
Hi Rob,
On 23/11/16 21:49, Rob Herring wrote:
> On Wed, Nov 23, 2016 at 1:06 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> Modular drivers may want to use of_for_each_phandle() - export its
>> constituent functions.
>
> Do you have a user?
I've been experimenting with modular IOMMU drivers on top of the
probe-deferral stuff[1], and the ARM SMMU driver in particular demands a
whole pile of exports. I thought I'd throw this one out now since nearly
all the other of_* functions are exported already, but I'm quite happy
to sit on it if you'd prefer to wait for a concrete in-tree need (it'll
be a while yet).
Robin.
[1]:http://www.linux-arm.org/git?p=linux-rm.git;a=shortlog;h=refs/heads/iommu/defer
^ permalink raw reply
* [PATCH v9 4/4] tests: Add overlay tests
From: Pantelis Antoniou @ 2016-11-24 12:31 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1479990693-14260-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Add a number of tests for dynamic objects/overlays.
Re-use the original test by moving the contents to a .dtsi include
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
tests/overlay_overlay_dtc.dts | 76 +----------------------------------
tests/overlay_overlay_dtc.dtsi | 83 +++++++++++++++++++++++++++++++++++++++
tests/overlay_overlay_new_dtc.dts | 11 ++++++
tests/overlay_overlay_simple.dts | 12 ++++++
tests/run_tests.sh | 20 ++++++++++
5 files changed, 127 insertions(+), 75 deletions(-)
create mode 100644 tests/overlay_overlay_dtc.dtsi
create mode 100644 tests/overlay_overlay_new_dtc.dts
create mode 100644 tests/overlay_overlay_simple.dts
diff --git a/tests/overlay_overlay_dtc.dts b/tests/overlay_overlay_dtc.dts
index 30d2362..ca943ea 100644
--- a/tests/overlay_overlay_dtc.dts
+++ b/tests/overlay_overlay_dtc.dts
@@ -8,78 +8,4 @@
/dts-v1/;
/plugin/;
-/ {
- /* Test that we can change an int by another */
- fragment@0 {
- target = <&test>;
-
- __overlay__ {
- test-int-property = <43>;
- };
- };
-
- /* Test that we can replace a string by a longer one */
- fragment@1 {
- target = <&test>;
-
- __overlay__ {
- test-str-property = "foobar";
- };
- };
-
- /* Test that we add a new property */
- fragment@2 {
- target = <&test>;
-
- __overlay__ {
- test-str-property-2 = "foobar2";
- };
- };
-
- /* Test that we add a new node (by phandle) */
- fragment@3 {
- target = <&test>;
-
- __overlay__ {
- new-node {
- new-property;
- };
- };
- };
-
- fragment@5 {
- target = <&test>;
-
- __overlay__ {
- local: new-local-node {
- new-property;
- };
- };
- };
-
- fragment@6 {
- target = <&test>;
-
- __overlay__ {
- test-phandle = <&test>, <&local>;
- };
- };
-
- fragment@7 {
- target = <&test>;
-
- __overlay__ {
- test-several-phandle = <&local>, <&local>;
- };
- };
-
- fragment@8 {
- target = <&test>;
-
- __overlay__ {
- sub-test-node {
- new-sub-test-property;
- };
- };
- };
-};
+/include/ "overlay_overlay_dtc.dtsi"
diff --git a/tests/overlay_overlay_dtc.dtsi b/tests/overlay_overlay_dtc.dtsi
new file mode 100644
index 0000000..8ea8d5d
--- /dev/null
+++ b/tests/overlay_overlay_dtc.dtsi
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+/ {
+ /* Test that we can change an int by another */
+ fragment@0 {
+ target = <&test>;
+
+ __overlay__ {
+ test-int-property = <43>;
+ };
+ };
+
+ /* Test that we can replace a string by a longer one */
+ fragment@1 {
+ target = <&test>;
+
+ __overlay__ {
+ test-str-property = "foobar";
+ };
+ };
+
+ /* Test that we add a new property */
+ fragment@2 {
+ target = <&test>;
+
+ __overlay__ {
+ test-str-property-2 = "foobar2";
+ };
+ };
+
+ /* Test that we add a new node (by phandle) */
+ fragment@3 {
+ target = <&test>;
+
+ __overlay__ {
+ new-node {
+ new-property;
+ };
+ };
+ };
+
+ fragment@5 {
+ target = <&test>;
+
+ __overlay__ {
+ local: new-local-node {
+ new-property;
+ };
+ };
+ };
+
+ fragment@6 {
+ target = <&test>;
+
+ __overlay__ {
+ test-phandle = <&test>, <&local>;
+ };
+ };
+
+ fragment@7 {
+ target = <&test>;
+
+ __overlay__ {
+ test-several-phandle = <&local>, <&local>;
+ };
+ };
+
+ fragment@8 {
+ target = <&test>;
+
+ __overlay__ {
+ sub-test-node {
+ new-sub-test-property;
+ };
+ };
+ };
+};
diff --git a/tests/overlay_overlay_new_dtc.dts b/tests/overlay_overlay_new_dtc.dts
new file mode 100644
index 0000000..14d3f54
--- /dev/null
+++ b/tests/overlay_overlay_new_dtc.dts
@@ -0,0 +1,11 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+/dts-v1/ /plugin/;
+
+/include/ "overlay_overlay_dtc.dtsi"
diff --git a/tests/overlay_overlay_simple.dts b/tests/overlay_overlay_simple.dts
new file mode 100644
index 0000000..8657e1e
--- /dev/null
+++ b/tests/overlay_overlay_simple.dts
@@ -0,0 +1,12 @@
+/*
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+&test {
+ test-int-property = <43>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e4139dd..99c09bc 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -181,6 +181,26 @@ overlay_tests () {
run_dtc_test -@ -I dts -O dtb -o overlay_base_with_symbols.test.dtb overlay_base.dts
run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.dtb overlay_overlay_dtc.dts
run_test overlay overlay_base_with_symbols.test.dtb overlay_overlay_with_symbols.test.dtb
+
+ # new /plugin/ format
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_new_with_symbols.test.dtb overlay_overlay_new_dtc.dts
+
+ # test new magic option
+ run_dtc_test -M@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_dtc.dts
+
+ # test plugin source to dtb and back
+ run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc.test.dts overlay_overlay_with_symbols.test.dtb
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.test.dtb overlay_overlay_dtc.test.dts
+ run_test dtbs_equal_ordered overlay_overlay_with_symbols.test.dtb overlay_overlay_with_symbols.test.test.dtb
+
+ # test simplified plugin syntax
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_simple.dtb overlay_overlay_simple.dts
+
+ # test plugin source to dtb and back (with new magic)
+ run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc_new_magic.test.dts overlay_overlay_with_symbols_new_magic.test.dtb
+ run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.test.dtb overlay_overlay_dtc_new_magic.test.dts
+ run_test dtbs_equal_ordered overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_with_symbols_new_magic.test.test.dtb
+
fi
# Bad fixup tests
--
2.1.4
^ permalink raw reply related
* [PATCH v9 3/4] dtc: Plugin and fixup support
From: Pantelis Antoniou @ 2016-11-24 12:31 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1479990693-14260-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
This patch enable the generation of symbols & local fixup information
for trees compiled with the -@ (--symbols) option.
Using this patch labels in the tree and their users emit information
in __symbols__ and __local_fixups__ nodes.
The __fixups__ node make possible the dynamic resolution of phandle
references which are present in the plugin tree but lie in the
tree that are applying the overlay against.
While there is a new magic number for dynamic device tree/overlays blobs
it is by default disabled. This is in order to give time for DT blob
methods to be updated.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Documentation/manual.txt | 25 +++++-
checks.c | 8 +-
dtc-lexer.l | 5 ++
dtc-parser.y | 49 +++++++++--
dtc.c | 39 +++++++-
dtc.h | 20 ++++-
fdtdump.c | 2 +-
flattree.c | 17 ++--
fstree.c | 2 +-
libfdt/fdt.c | 2 +-
libfdt/fdt.h | 3 +-
livetree.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
tests/mangle-layout.c | 7 +-
treesource.c | 7 +-
14 files changed, 380 insertions(+), 31 deletions(-)
diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..65fbf09 100644
--- a/Documentation/manual.txt
+++ b/Documentation/manual.txt
@@ -119,6 +119,24 @@ Options:
Make space for <number> reserve map entries
Relevant for dtb and asm output only.
+ -@
+ Generates a __symbols__ node at the root node of the resulting blob
+ for any node labels used, and for any local references using phandles
+ it also generates a __local_fixups__ node that tracks them.
+
+ When using the /plugin/ tag all unresolved label references to
+ be tracked in the __fixups__ node, making dynamic resolution possible.
+
+ -A
+ Generate automatically aliases for all node labels. This is similar to
+ the -@ option (the __symbols__ node contain identical information) but
+ the semantics are slightly different since no phandles are automatically
+ generated for labeled nodes.
+
+ -M
+ Generate blobs with the new FDT magic number. By default blobs with the
+ standard FDT magic number are generated.
+
-S <bytes>
Ensure the blob at least <bytes> long, adding additional
space if needed.
@@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
Here is a very rough overview of the layout of a DTS source file:
- sourcefile: list_of_memreserve devicetree
+ sourcefile: versioninfo plugindecl list_of_memreserve devicetree
memreserve: label 'memreserve' ADDR ADDR ';'
| label 'memreserve' ADDR '-' ADDR ';'
devicetree: '/' nodedef
+ versioninfo: '/' 'dts-v1' '/' ';'
+
+ plugindecl: '/' 'plugin' '/' ';'
+ | /* empty */
+
nodedef: '{' list_of_property list_of_subnode '}' ';'
property: label PROPNAME '=' propdata ';'
diff --git a/checks.c b/checks.c
index 609975a..bc03d42 100644
--- a/checks.c
+++ b/checks.c
@@ -486,8 +486,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
refnode = get_node_by_ref(dt, m->ref);
if (! refnode) {
- FAIL(c, "Reference to non-existent node or label \"%s\"\n",
- m->ref);
+ if (!(bi->versionflags & VF_PLUGIN))
+ FAIL(c, "Reference to non-existent node or "
+ "label \"%s\"\n", m->ref);
+ else /* mark the entry as unresolved */
+ *((cell_t *)(prop->val.val + m->offset)) =
+ cpu_to_fdt32(0xffffffff);
continue;
}
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 790fbf6..40bbc87 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
return DT_V1;
}
+<*>"/plugin/" {
+ DPRINT("Keyword: /plugin/\n");
+ return DT_PLUGIN;
+ }
+
<*>"/memreserve/" {
DPRINT("Keyword: /memreserve/\n");
BEGIN_DEFAULT();
diff --git a/dtc-parser.y b/dtc-parser.y
index 14aaf2e..4afc592 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -19,6 +19,7 @@
*/
%{
#include <stdio.h>
+#include <inttypes.h>
#include "dtc.h"
#include "srcpos.h"
@@ -33,6 +34,9 @@ extern void yyerror(char const *s);
extern struct boot_info *the_boot_info;
extern bool treesource_error;
+
+/* temporary while the tree is not built */
+static unsigned int the_versionflags;
%}
%union {
@@ -52,9 +56,11 @@ extern bool treesource_error;
struct node *nodelist;
struct reserve_info *re;
uint64_t integer;
+ unsigned int flags;
}
%token DT_V1
+%token DT_PLUGIN
%token DT_MEMRESERVE
%token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
%token DT_BITS
@@ -71,6 +77,8 @@ extern bool treesource_error;
%type <data> propdata
%type <data> propdataprefix
+%type <flags> versioninfo
+%type <flags> plugindecl
%type <re> memreserve
%type <re> memreserves
%type <array> arrayprefix
@@ -101,16 +109,36 @@ extern bool treesource_error;
%%
sourcefile:
- v1tag memreserves devicetree
+ versioninfo plugindecl memreserves devicetree
+ {
+ the_boot_info = build_boot_info($1 | $2, $3, $4,
+ guess_boot_cpuid($4));
+ }
+ ;
+
+versioninfo:
+ v1tag
{
- the_boot_info = build_boot_info($2, $3,
- guess_boot_cpuid($3));
+ the_versionflags |= VF_DT_V1;
+ $$ = the_versionflags;
}
;
v1tag:
DT_V1 ';'
+ | DT_V1
| DT_V1 ';' v1tag
+
+plugindecl:
+ DT_PLUGIN ';'
+ {
+ the_versionflags |= VF_PLUGIN;
+ $$ = VF_PLUGIN;
+ }
+ | /* empty */
+ {
+ $$ = 0;
+ }
;
memreserves:
@@ -161,10 +189,14 @@ devicetree:
{
struct node *target = get_node_by_ref($1, $2);
- if (target)
+ if (target) {
merge_nodes(target, $3);
- else
- ERROR(&@2, "Label or path %s not found", $2);
+ } else {
+ if (the_versionflags & VF_PLUGIN)
+ add_orphan_node($1, $3, $2);
+ else
+ ERROR(&@2, "Label or path %s not found", $2);
+ }
$$ = $1;
}
| devicetree DT_DEL_NODE DT_REF ';'
@@ -179,6 +211,11 @@ devicetree:
$$ = $1;
}
+ | /* empty */
+ {
+ /* build empty node */
+ $$ = name_node(build_node(NULL, NULL), "");
+ }
;
nodedef:
diff --git a/dtc.c b/dtc.c
index 9dcf640..a654267 100644
--- a/dtc.c
+++ b/dtc.c
@@ -32,6 +32,9 @@ int minsize; /* Minimum blob size */
int padsize; /* Additional padding to blob */
int alignsize; /* Additional padding to blob accroding to the alignsize */
int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
+int symbol_fixup_support; /* enable symbols & fixup support */
+int auto_label_aliases; /* auto generate labels -> aliases */
+int new_magic; /* use new FDT magic values for objects */
static int is_power_of_2(int x)
{
@@ -59,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
#define FDT_VERSION(version) _FDT_VERSION(version)
#define _FDT_VERSION(version) #version
static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
static struct option const usage_long_opts[] = {
{"quiet", no_argument, NULL, 'q'},
{"in-format", a_argument, NULL, 'I'},
@@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
{"phandle", a_argument, NULL, 'H'},
{"warning", a_argument, NULL, 'W'},
{"error", a_argument, NULL, 'E'},
+ {"symbols", no_argument, NULL, '@'},
+ {"auto-alias", no_argument, NULL, 'A'},
+ {"new-magic", no_argument, NULL, 'M'},
{"help", no_argument, NULL, 'h'},
{"version", no_argument, NULL, 'v'},
{NULL, no_argument, NULL, 0x0},
@@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
"\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
"\n\tEnable/disable warnings (prefix with \"no-\")",
"\n\tEnable/disable errors (prefix with \"no-\")",
+ "\n\tEnable symbols/fixup support",
+ "\n\tEnable auto-alias of labels",
+ "\n\tUse new blog magic value",
"\n\tPrint this help and exit",
"\n\tPrint version and exit",
NULL,
@@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
fclose(f);
magic = fdt32_to_cpu(magic);
- if (magic == FDT_MAGIC)
+ if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
return "dtb";
return guess_type_by_name(fname, fallback);
@@ -172,6 +181,7 @@ int main(int argc, char *argv[])
FILE *outf = NULL;
int outversion = DEFAULT_FDT_VERSION;
long long cmdline_boot_cpuid = -1;
+ fdt32_t out_magic = FDT_MAGIC;
quiet = 0;
reservenum = 0;
@@ -249,6 +259,16 @@ int main(int argc, char *argv[])
parse_checks_option(false, true, optarg);
break;
+ case '@':
+ symbol_fixup_support = 1;
+ break;
+ case 'A':
+ auto_label_aliases = 1;
+ break;
+ case 'M':
+ new_magic = 1;
+ break;
+
case 'h':
usage(NULL);
default:
@@ -306,6 +326,14 @@ int main(int argc, char *argv[])
fill_fullpaths(bi->dt, "");
process_checks(force, bi);
+ if (auto_label_aliases)
+ generate_label_tree(bi->dt, "aliases", false);
+
+ if (symbol_fixup_support) {
+ generate_label_tree(bi->dt, "__symbols__", true);
+ generate_fixups_tree(bi->dt);
+ }
+
if (sort)
sort_tree(bi);
@@ -318,12 +346,15 @@ int main(int argc, char *argv[])
outname, strerror(errno));
}
+ if (new_magic && (bi->versionflags & VF_PLUGIN))
+ out_magic = FDT_MAGIC_DTBO;
+
if (streq(outform, "dts")) {
dt_to_source(outf, bi);
} else if (streq(outform, "dtb")) {
- dt_to_blob(outf, bi, outversion);
+ dt_to_blob(outf, bi, out_magic, outversion);
} else if (streq(outform, "asm")) {
- dt_to_asm(outf, bi, outversion);
+ dt_to_asm(outf, bi, out_magic, outversion);
} else if (streq(outform, "null")) {
/* do nothing */
} else {
diff --git a/dtc.h b/dtc.h
index 32009bc..889b8f8 100644
--- a/dtc.h
+++ b/dtc.h
@@ -55,6 +55,9 @@ extern int minsize; /* Minimum blob size */
extern int padsize; /* Additional padding to blob */
extern int alignsize; /* Additional padding to blob accroding to the alignsize */
extern int phandle_format; /* Use linux,phandle or phandle properties */
+extern int symbol_fixup_support;/* enable symbols & fixup support */
+extern int auto_label_aliases; /* auto generate labels -> aliases */
+extern int new_magic; /* use new FDT magic values for objects */
#define PHANDLE_LEGACY 0x1
#define PHANDLE_EPAPR 0x2
@@ -195,6 +198,7 @@ struct node *build_node_delete(void);
struct node *name_node(struct node *node, char *name);
struct node *chain_node(struct node *first, struct node *list);
struct node *merge_nodes(struct node *old_node, struct node *new_node);
+void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
void add_property(struct node *node, struct property *prop);
void delete_property_by_name(struct node *node, char *name);
@@ -202,6 +206,8 @@ void delete_property(struct property *prop);
void add_child(struct node *parent, struct node *child);
void delete_node_by_name(struct node *parent, char *name);
void delete_node(struct node *node);
+void append_to_property(struct node *node,
+ char *name, const void *data, int len);
const char *get_unitname(struct node *node);
struct property *get_property(struct node *node, const char *propname);
@@ -237,14 +243,22 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
struct boot_info {
+ unsigned int versionflags;
struct reserve_info *reservelist;
struct node *dt; /* the device tree */
uint32_t boot_cpuid_phys;
};
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+/* version flags definitions */
+#define VF_DT_V1 0x0001 /* /dts-v1/ */
+#define VF_PLUGIN 0x0002 /* /plugin/ */
+
+struct boot_info *build_boot_info(unsigned int versionflags,
+ struct reserve_info *reservelist,
struct node *tree, uint32_t boot_cpuid_phys);
void sort_tree(struct boot_info *bi);
+void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
+void generate_fixups_tree(struct node *dt);
/* Checks */
@@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
/* Flattened trees */
-void dt_to_blob(FILE *f, struct boot_info *bi, int version);
-void dt_to_asm(FILE *f, struct boot_info *bi, int version);
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
struct boot_info *dt_from_blob(const char *fname);
diff --git a/fdtdump.c b/fdtdump.c
index a9a2484..dd63ac2 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -201,7 +201,7 @@ int main(int argc, char *argv[])
p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
if (!p)
break;
- if (fdt_magic(p) == FDT_MAGIC) {
+ if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
/* try and validate the main struct */
off_t this_len = endp - p;
fdt32_t max_version = 17;
diff --git a/flattree.c b/flattree.c
index a9d9520..57d76cf 100644
--- a/flattree.c
+++ b/flattree.c
@@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
}
static void make_fdt_header(struct fdt_header *fdt,
+ fdt32_t magic,
struct version_info *vi,
int reservesize, int dtsize, int strsize,
int boot_cpuid_phys)
@@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
memset(fdt, 0xff, sizeof(*fdt));
- fdt->magic = cpu_to_fdt32(FDT_MAGIC);
+ fdt->magic = cpu_to_fdt32(magic);
fdt->version = cpu_to_fdt32(vi->version);
fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
@@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
fdt->size_dt_struct = cpu_to_fdt32(dtsize);
}
-void dt_to_blob(FILE *f, struct boot_info *bi, int version)
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
{
struct version_info *vi = NULL;
int i;
@@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
reservebuf = flatten_reserve_list(bi->reservelist, vi);
/* Make header */
- make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
+ make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
bi->boot_cpuid_phys);
/*
@@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
}
}
-void dt_to_asm(FILE *f, struct boot_info *bi, int version)
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
{
struct version_info *vi = NULL;
int i;
@@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
struct node *tree;
uint32_t val;
int flags = 0;
+ unsigned int versionflags = VF_DT_V1;
f = srcfile_relative_open(fname, NULL);
@@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
}
magic = fdt32_to_cpu(magic);
- if (magic != FDT_MAGIC)
+ if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
die("Blob has incorrect magic number\n");
+ if (magic == FDT_MAGIC_DTBO)
+ versionflags |= VF_PLUGIN;
+
rc = fread(&totalsize, sizeof(totalsize), 1, f);
if (ferror(f))
die("Error reading DT blob size: %s\n", strerror(errno));
@@ -942,5 +947,5 @@ struct boot_info *dt_from_blob(const char *fname)
fclose(f);
- return build_boot_info(reservelist, tree, boot_cpuid_phys);
+ return build_boot_info(versionflags, reservelist, tree, boot_cpuid_phys);
}
diff --git a/fstree.c b/fstree.c
index 6d1beec..54f520b 100644
--- a/fstree.c
+++ b/fstree.c
@@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
tree = read_fstree(dirname);
tree = name_node(tree, "");
- return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
+ return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
}
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 22286a1..28d422c 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -57,7 +57,7 @@
int fdt_check_header(const void *fdt)
{
- if (fdt_magic(fdt) == FDT_MAGIC) {
+ if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
/* Complete tree */
if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
return -FDT_ERR_BADVERSION;
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index 526aedb..493cd55 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
@@ -55,7 +55,7 @@
#ifndef __ASSEMBLY__
struct fdt_header {
- fdt32_t magic; /* magic word FDT_MAGIC */
+ fdt32_t magic; /* magic word FDT_MAGIC[|_DTBO] */
fdt32_t totalsize; /* total size of DT block */
fdt32_t off_dt_struct; /* offset to structure */
fdt32_t off_dt_strings; /* offset to strings */
@@ -93,6 +93,7 @@ struct fdt_property {
#endif /* !__ASSEMBLY */
#define FDT_MAGIC 0xd00dfeed /* 4: version, 4: total size */
+#define FDT_MAGIC_DTBO 0xd00dfdb0 /* DTBO magic */
#define FDT_TAGSIZE sizeof(fdt32_t)
#define FDT_BEGIN_NODE 0x1 /* Start node: full name */
diff --git a/livetree.c b/livetree.c
index 3dc7559..1a2f4b1 100644
--- a/livetree.c
+++ b/livetree.c
@@ -216,6 +216,31 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
return old_node;
}
+void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
+{
+ static unsigned int next_orphan_fragment = 0;
+ struct node *node = xmalloc(sizeof(*node));
+ struct property *p;
+ struct data d = empty_data;
+ char *name;
+
+ memset(node, 0, sizeof(*node));
+
+ d = data_add_marker(d, REF_PHANDLE, ref);
+ d = data_append_integer(d, 0xffffffff, 32);
+
+ p = build_property("target", d);
+ add_property(node, p);
+
+ xasprintf(&name, "fragment@%u",
+ next_orphan_fragment++);
+ name_node(node, name);
+ name_node(new_node, "__overlay__");
+
+ add_child(dt, node);
+ add_child(node, new_node);
+}
+
struct node *chain_node(struct node *first, struct node *list)
{
assert(first->next_sibling == NULL);
@@ -296,6 +321,23 @@ void delete_node(struct node *node)
delete_labels(&node->labels);
}
+void append_to_property(struct node *node,
+ char *name, const void *data, int len)
+{
+ struct data d;
+ struct property *p;
+
+ p = get_property(node, name);
+ if (p) {
+ d = data_append_data(p->val, data, len);
+ p->val = d;
+ } else {
+ d = data_append_data(empty_data, data, len);
+ p = build_property(name, d);
+ add_property(node, p);
+ }
+}
+
struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
{
struct reserve_info *new = xmalloc(sizeof(*new));
@@ -335,12 +377,14 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
return list;
}
-struct boot_info *build_boot_info(struct reserve_info *reservelist,
+struct boot_info *build_boot_info(unsigned int versionflags,
+ struct reserve_info *reservelist,
struct node *tree, uint32_t boot_cpuid_phys)
{
struct boot_info *bi;
bi = xmalloc(sizeof(*bi));
+ bi->versionflags = versionflags;
bi->reservelist = reservelist;
bi->dt = tree;
bi->boot_cpuid_phys = boot_cpuid_phys;
@@ -709,3 +753,182 @@ void sort_tree(struct boot_info *bi)
sort_reserve_entries(bi);
sort_node(bi->dt);
}
+
+/* utility helper to avoid code duplication */
+static struct node *build_and_name_child_node(struct node *parent, char *name)
+{
+ struct node *node;
+
+ node = build_node(NULL, NULL);
+ name_node(node, xstrdup(name));
+ add_child(parent, node);
+
+ return node;
+}
+
+static void generate_label_tree_internal(struct node *dt, struct node *node,
+ struct node *an, bool allocph)
+{
+ struct node *c;
+ struct property *p;
+ struct label *l;
+
+ /* if if there are labels */
+ if (node->labels) {
+ /* now add the label in the node */
+ for_each_label(node->labels, l) {
+ /* check whether the label already exists */
+ p = get_property(an, l->label);
+ if (p) {
+ fprintf(stderr, "WARNING: label %s already"
+ " exists in /%s", l->label,
+ an->name);
+ continue;
+ }
+
+ /* insert it */
+ p = build_property(l->label,
+ data_copy_escape_string(node->fullpath,
+ strlen(node->fullpath)));
+ add_property(an, p);
+ }
+
+ /* force allocation of a phandle for this node */
+ if (allocph)
+ (void)get_node_phandle(dt, node);
+ }
+
+ for_each_child(node, c)
+ generate_label_tree_internal(dt, c, an, allocph);
+}
+
+void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
+{
+ struct node *an;
+
+ for_each_child(dt, an)
+ if (streq(gen_node_name, an->name))
+ break;
+
+ if (!an)
+ an = build_and_name_child_node(dt, gen_node_name);
+ if (!an)
+ die("Could not build label node /%s\n", gen_node_name);
+
+ generate_label_tree_internal(dt, dt, an, allocph);
+}
+
+static char *fixups_name = "__fixups__";
+static char *local_fixups_name = "__local_fixups__";
+
+static void add_fixup_entry(struct node *dt, struct node *node,
+ struct property *prop, struct marker *m)
+{
+ struct node *fn; /* fixup node */
+ char *entry;
+
+ /* m->ref can only be a REF_PHANDLE, but check anyway */
+ assert(m->type == REF_PHANDLE);
+
+ /* fn is the node we're putting entries in */
+ fn = get_subnode(dt, fixups_name);
+ assert(fn != NULL);
+
+ /* there shouldn't be any ':' in the arguments */
+ if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
+ die("arguments should not contain ':'\n");
+
+ xasprintf(&entry, "%s:%s:%u",
+ node->fullpath, prop->name, m->offset);
+ append_to_property(fn, m->ref, entry, strlen(entry) + 1);
+}
+
+static void add_local_fixup_entry(struct node *dt, struct node *node,
+ struct property *prop, struct marker *m,
+ struct node *refnode)
+{
+ struct node *lfn, *wn, *nwn; /* local fixup node, walk node, new */
+ uint32_t value_32;
+ char *s, *e, *comp;
+ int len;
+
+ /* fn is the node we're putting entries in */
+ lfn = get_subnode(dt, local_fixups_name);
+ assert(lfn != NULL);
+
+ /* walk the path components creating nodes if they don't exist */
+ comp = xmalloc(strlen(node->fullpath) + 1);
+ /* start skipping the first / */
+ s = node->fullpath + 1;
+ wn = lfn;
+ while (*s) {
+ /* retrieve path component */
+ e = strchr(s, '/');
+ if (e == NULL)
+ e = s + strlen(s);
+ len = e - s;
+ memcpy(comp, s, len);
+ comp[len] = '\0';
+
+ /* if no node exists, create it */
+ nwn = get_subnode(wn, comp);
+ if (!nwn)
+ nwn = build_and_name_child_node(wn, comp);
+ wn = nwn;
+
+ /* last path component */
+ if (!*e)
+ break;
+
+ /* next path component */
+ s = e + 1;
+ }
+ free(comp);
+
+ value_32 = cpu_to_fdt32(m->offset);
+ append_to_property(wn, prop->name, &value_32, sizeof(value_32));
+}
+
+static void generate_fixups_tree_internal(struct node *dt, struct node *node)
+{
+ struct node *c;
+ struct property *prop;
+ struct marker *m;
+ struct node *refnode;
+
+ for_each_property(node, prop) {
+ m = prop->val.markers;
+ for_each_marker_of_type(m, REF_PHANDLE) {
+ refnode = get_node_by_ref(dt, m->ref);
+ if (!refnode)
+ add_fixup_entry(dt, node, prop, m);
+ else
+ add_local_fixup_entry(dt, node, prop, m,
+ refnode);
+ }
+ }
+
+ for_each_child(node, c)
+ generate_fixups_tree_internal(dt, c);
+}
+
+void generate_fixups_tree(struct node *dt)
+{
+ struct node *an;
+
+ for_each_child(dt, an)
+ if (streq(fixups_name, an->name))
+ break;
+
+ if (!an)
+ build_and_name_child_node(dt, fixups_name);
+
+ for_each_child(dt, an)
+ if (streq(local_fixups_name, an->name))
+ break;
+
+ if (!an)
+ build_and_name_child_node(dt, local_fixups_name);
+
+ generate_fixups_tree_internal(dt, dt);
+}
diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
index a76e51e..d29ebc6 100644
--- a/tests/mangle-layout.c
+++ b/tests/mangle-layout.c
@@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
buf->size = newsize;
}
-static void new_header(struct bufstate *buf, int version, const void *fdt)
+static void new_header(struct bufstate *buf, fdt32_t magic, int version,
+ const void *fdt)
{
int hdrsize;
@@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
expand_buf(buf, hdrsize);
memset(buf->buf, 0, hdrsize);
- fdt_set_magic(buf->buf, FDT_MAGIC);
+ fdt_set_magic(buf->buf, magic);
fdt_set_version(buf->buf, version);
fdt_set_last_comp_version(buf->buf, 16);
fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
@@ -145,7 +146,7 @@ int main(int argc, char *argv[])
if (fdt_version(fdt) < 17)
CONFIG("Input tree must be v17");
- new_header(&buf, version, fdt);
+ new_header(&buf, FDT_MAGIC, version, fdt);
while (*blockorder) {
add_block(&buf, version, *blockorder, fdt);
diff --git a/treesource.c b/treesource.c
index a55d1d1..75e920d 100644
--- a/treesource.c
+++ b/treesource.c
@@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
{
struct reserve_info *re;
- fprintf(f, "/dts-v1/;\n\n");
+ fprintf(f, "/dts-v1/");
+
+ if (bi->versionflags & VF_PLUGIN)
+ fprintf(f, " /plugin/");
+
+ fprintf(f, ";\n\n");
for (re = bi->reservelist; re; re = re->next) {
struct label *l;
--
2.1.4
^ permalink raw reply related
* [PATCH v9 2/4] dtc: Document the dynamic plugin internals
From: Pantelis Antoniou @ 2016-11-24 12:31 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1479990693-14260-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Provides the document explaining the internal mechanics of
plugins and options.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
1 file changed, 318 insertions(+)
create mode 100644 Documentation/dt-object-internal.txt
diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
new file mode 100644
index 0000000..d5b841e
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,318 @@
+Device Tree Dynamic Object format internals
+-------------------------------------------
+
+The Device Tree for most platforms is a static representation of
+the hardware capabilities. This is insufficient for many platforms
+that need to dynamically insert device tree fragments to the
+running kernel's live tree.
+
+This document explains the the device tree object format and the
+modifications made to the device tree compiler, which make it possible.
+
+1. Simplified Problem Definition
+--------------------------------
+
+Assume we have a platform which boots using following simplified device tree.
+
+---- foo.dts -----------------------------------------------------------------
+ /* FOO platform */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+ };
+ };
+---- foo.dts -----------------------------------------------------------------
+
+We have a number of peripherals that after probing (using some undefined method)
+should result in different device tree configuration.
+
+We cannot boot with this static tree because due to the configuration of the
+foo platform there exist multiple conficting peripherals DT fragments.
+
+So for the bar peripheral we would have this:
+
+---- foo+bar.dts -------------------------------------------------------------
+ /* FOO platform + bar peripheral */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ };
+ };
+ };
+---- foo+bar.dts -------------------------------------------------------------
+
+While for the baz peripheral we would have this:
+
+---- foo+baz.dts -------------------------------------------------------------
+ /* FOO platform + baz peripheral */
+ / {
+ compatible = "corp,foo";
+
+ /* shared resources */
+ res: res {
+ /* baz resources */
+ baz_res: res_baz { ... };
+ };
+
+ /* On chip peripherals */
+ ocp: ocp {
+ /* peripherals that are always instantiated */
+ peripheral1 { ... };
+
+ /* baz peripheral */
+ baz {
+ compatible = "corp,baz";
+ /* reference to another point in the tree */
+ ref-to-res = <&baz_res>;
+ ... /* various properties and child nodes */
+ };
+ };
+ };
+---- foo+baz.dts -------------------------------------------------------------
+
+We note that the baz case is more complicated, since the baz peripheral needs to
+reference another node in the DT tree.
+
+2. Device Tree Object Format Requirements
+-----------------------------------------
+
+Since the device tree is used for booting a number of very different hardware
+platforms it is imperative that we tread very carefully.
+
+2.a) No changes to the Device Tree binary format for the base tree. We cannot
+modify the tree format at all and all the information we require should be
+encoded using device tree itself. We can add nodes that can be safely ignored
+by both bootloaders and the kernel. The plugin dtb's are optionally tagged
+with a different magic number in the header but otherwise they too are simple
+blobs.
+
+2.b) Changes to the DTS source format should be absolutely minimal, and should
+only be needed for the DT fragment definitions, and not the base boot DT.
+
+2.c) An explicit option should be used to instruct DTC to generate the required
+information needed for object resolution. Platforms that don't use the
+dynamic object format can safely ignore it.
+
+2.d) Finally, DT syntax changes should be kept to a minimum. It should be
+possible to express everything using the existing DT syntax.
+
+3. Implementation
+-----------------
+
+The basic unit of addressing in Device Tree is the phandle. Turns out it's
+relatively simple to extend the way phandles are generated and referenced
+so that it's possible to dynamically convert symbolic references (labels)
+to phandle values. This is a valid assumption as long as the author uses
+reference syntax and does not assign phandle values manually (which might
+be a problem with decompiled source files).
+
+We can roughly divide the operation into two steps.
+
+3.a) Compilation of the base board DTS file using the '-@' option
+generates a valid DT blob with an added __symbols__ node at the root node,
+containing a list of all nodes that are marked with a label.
+
+Using the foo.dts file above the following node will be generated;
+
+$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
+$ fdtdump foo.dtb
+...
+/ {
+ ...
+ res {
+ ...
+ phandle = <0x00000001>;
+ ...
+ };
+ ocp {
+ ...
+ phandle = <0x00000002>;
+ ...
+ };
+ __symbols__ {
+ res="/res";
+ ocp="/ocp";
+ };
+};
+
+Notice that all the nodes that had a label have been recorded, and that
+phandles have been generated for them.
+
+This blob can be used to boot the board normally, the __symbols__ node will
+be safely ignored both by the bootloader and the kernel (the only loss will
+be a few bytes of memory and disk space).
+
+3.b) The Device Tree fragments must be compiled with the same option but they
+must also have a tag (/plugin/) that allows undefined references to nodes
+that are not present at compilation time to be recorded so that the runtime
+loader can fix them.
+
+So the bar peripheral's DTS format would be of the form:
+
+/dts-v1/ /plugin/; /* allow undefined references and record them */
+/ {
+ .... /* various properties for loader use; i.e. part id etc. */
+ fragment@0 {
+ target = <&ocp>;
+ __overlay__ {
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+};
+
+Note that there's a target property that specifies the location where the
+contents of the overlay node will be placed, and it references the node
+in the foo.dts file.
+
+$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
+$ fdtdump bar.dtbo
+...
+/ {
+ ... /* properties */
+ fragment@0 {
+ target = <0xffffffff>;
+ __overlay__ {
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+ __fixups__ {
+ ocp = "/fragment@0:target:0";
+ };
+};
+
+No __symbols__ has been generated (no label in bar.dts).
+Note that the target's ocp label is undefined, so the phandle handle
+value is filled with the illegal value '0xffffffff', while a __fixups__
+node has been generated, which marks the location in the tree where
+the label lookup should store the runtime phandle value of the ocp node.
+
+The format of the __fixups__ node entry is
+
+ <label> = "<local-full-path>:<property-name>:<offset>";
+
+<label> Is the label we're referring
+<local-full-path> Is the full path of the node the reference is
+<property-name> Is the name of the property containing the
+ reference
+<offset> The offset (in bytes) of where the property's
+ phandle value is located.
+
+Doing the same with the baz peripheral's DTS format is a little bit more
+involved, since baz contains references to local labels which require
+local fixups.
+
+/dts-v1/ /plugin/; /* allow undefined label references and record them */
+/ {
+ .... /* various properties for loader use; i.e. part id etc. */
+ fragment@0 {
+ target = <&res>;
+ __overlay__ {
+ /* baz resources */
+ baz_res: res_baz { ... };
+ };
+ };
+ fragment@1 {
+ target = <&ocp>;
+ __overlay__ {
+ /* baz peripheral */
+ baz {
+ compatible = "corp,baz";
+ /* reference to another point in the tree */
+ ref-to-res = <&baz_res>;
+ ... /* various properties and child nodes */
+ }
+ };
+ };
+};
+
+Note that &bar_res reference.
+
+$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
+$ fdtdump baz.dtbo
+...
+/ {
+ ... /* properties */
+ fragment@0 {
+ target = <0xffffffff>;
+ __overlay__ {
+ res_baz {
+ ....
+ phandle = <0x00000001>;
+ };
+ };
+ };
+ fragment@1 {
+ target = <0xffffffff>;
+ __overlay__ {
+ baz {
+ compatible = "corp,baz";
+ ... /* various properties and child nodes */
+ ref-to-res = <0x00000001>;
+ }
+ };
+ };
+ __fixups__ {
+ res = "/fragment@0:target:0";
+ ocp = "/fragment@1:target:0";
+ };
+ __local_fixups__ {
+ fragment@1 {
+ __overlay__ {
+ baz {
+ ref-to-res = <0>;
+ };
+ };
+ };
+ };
+};
+
+This is similar to the bar case, but the reference of a local label by the
+baz node generates a __local_fixups__ entry that records the place that the
+local reference is being made. No matter how phandles are allocated from dtc
+the run time loader must apply an offset to each phandle in every dynamic
+DT object loaded. The __local_fixups__ node records the place of every
+local reference so that the loader can apply the offset.
+
+There is an alternative syntax to the expanded form for overlays with phandle
+targets which makes the format similar to the one using in .dtsi include files.
+
+So for the &ocp target example above one can simply write:
+
+/dts-v1/ /plugin/;
+&ocp {
+ /* bar peripheral */
+ bar {
+ compatible = "corp,bar";
+ ... /* various properties and child nodes */
+ }
+};
+
+The resulting dtb object is identical.
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v9 1/4] checks: Pass boot_info to the check methods
From: Pantelis Antoniou @ 2016-11-24 12:31 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
In-Reply-To: <1479990693-14260-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
As preparation for overlay support we need to pass boot info
as an extra boot_info parameter to each check method.
No other functional changes are made.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
checks.c | 113 +++++++++++++++++++++++++++++++++------------------------------
1 file changed, 59 insertions(+), 54 deletions(-)
diff --git a/checks.c b/checks.c
index 0381c98..609975a 100644
--- a/checks.c
+++ b/checks.c
@@ -40,7 +40,8 @@ enum checkstatus {
struct check;
-typedef void (*check_fn)(struct check *c, struct node *dt, struct node *node);
+typedef void (*check_fn)(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node);
struct check {
const char *name;
@@ -97,19 +98,20 @@ static inline void check_msg(struct check *c, const char *fmt, ...)
check_msg((c), __VA_ARGS__); \
} while (0)
-static void check_nodes_props(struct check *c, struct node *dt, struct node *node)
+static void check_nodes_props(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct node *child;
TRACE(c, "%s", node->fullpath);
if (c->fn)
- c->fn(c, dt, node);
+ c->fn(c, bi, dt, node);
for_each_child(node, child)
- check_nodes_props(c, dt, child);
+ check_nodes_props(c, bi, dt, child);
}
-static bool run_check(struct check *c, struct node *dt)
+static bool run_check(struct check *c, struct boot_info *bi, struct node *dt)
{
bool error = false;
int i;
@@ -123,7 +125,7 @@ static bool run_check(struct check *c, struct node *dt)
for (i = 0; i < c->num_prereqs; i++) {
struct check *prq = c->prereq[i];
- error = error || run_check(prq, dt);
+ error = error || run_check(prq, bi, dt);
if (prq->status != PASSED) {
c->status = PREREQ;
check_msg(c, "Failed prerequisite '%s'",
@@ -134,7 +136,7 @@ static bool run_check(struct check *c, struct node *dt)
if (c->status != UNCHECKED)
goto out;
- check_nodes_props(c, dt, dt);
+ check_nodes_props(c, bi, dt, dt);
if (c->status == UNCHECKED)
c->status = PASSED;
@@ -153,15 +155,15 @@ out:
*/
/* A check which always fails, for testing purposes only */
-static inline void check_always_fail(struct check *c, struct node *dt,
- struct node *node)
+static inline void check_always_fail(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
FAIL(c, "always_fail check");
}
CHECK(always_fail, check_always_fail, NULL);
-static void check_is_string(struct check *c, struct node *root,
- struct node *node)
+static void check_is_string(struct check *c, struct boot_info *bi,
+ struct node *root, struct node *node)
{
struct property *prop;
char *propname = c->data;
@@ -179,8 +181,8 @@ static void check_is_string(struct check *c, struct node *root,
#define ERROR_IF_NOT_STRING(nm, propname) \
ERROR(nm, check_is_string, (propname))
-static void check_is_cell(struct check *c, struct node *root,
- struct node *node)
+static void check_is_cell(struct check *c, struct boot_info *bi,
+ struct node *root, struct node *node)
{
struct property *prop;
char *propname = c->data;
@@ -202,8 +204,8 @@ static void check_is_cell(struct check *c, struct node *root,
* Structural check functions
*/
-static void check_duplicate_node_names(struct check *c, struct node *dt,
- struct node *node)
+static void check_duplicate_node_names(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct node *child, *child2;
@@ -217,8 +219,8 @@ static void check_duplicate_node_names(struct check *c, struct node *dt,
}
ERROR(duplicate_node_names, check_duplicate_node_names, NULL);
-static void check_duplicate_property_names(struct check *c, struct node *dt,
- struct node *node)
+static void check_duplicate_property_names(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop, *prop2;
@@ -239,8 +241,8 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
#define DIGITS "0123456789"
#define PROPNODECHARS LOWERCASE UPPERCASE DIGITS ",._+*#?-"
-static void check_node_name_chars(struct check *c, struct node *dt,
- struct node *node)
+static void check_node_name_chars(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
int n = strspn(node->name, c->data);
@@ -250,8 +252,8 @@ static void check_node_name_chars(struct check *c, struct node *dt,
}
ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
-static void check_node_name_format(struct check *c, struct node *dt,
- struct node *node)
+static void check_node_name_format(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
if (strchr(get_unitname(node), '@'))
FAIL(c, "Node %s has multiple '@' characters in name",
@@ -259,8 +261,8 @@ static void check_node_name_format(struct check *c, struct node *dt,
}
ERROR(node_name_format, check_node_name_format, NULL, &node_name_chars);
-static void check_unit_address_vs_reg(struct check *c, struct node *dt,
- struct node *node)
+static void check_unit_address_vs_reg(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
const char *unitname = get_unitname(node);
struct property *prop = get_property(node, "reg");
@@ -283,8 +285,8 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
}
WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);
-static void check_property_name_chars(struct check *c, struct node *dt,
- struct node *node)
+static void check_property_name_chars(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
@@ -305,9 +307,10 @@ ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
((prop) ? (prop)->name : ""), \
((prop) ? "' in " : ""), (node)->fullpath
-static void check_duplicate_label(struct check *c, struct node *dt,
- const char *label, struct node *node,
- struct property *prop, struct marker *mark)
+static void check_duplicate_label(struct check *c, struct boot_info *bi,
+ struct node *dt, const char *label,
+ struct node *node, struct property *prop,
+ struct marker *mark)
{
struct node *othernode = NULL;
struct property *otherprop = NULL;
@@ -331,29 +334,30 @@ static void check_duplicate_label(struct check *c, struct node *dt,
DESCLABEL_ARGS(othernode, otherprop, othermark));
}
-static void check_duplicate_label_node(struct check *c, struct node *dt,
- struct node *node)
+static void check_duplicate_label_node(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct label *l;
struct property *prop;
for_each_label(node->labels, l)
- check_duplicate_label(c, dt, l->label, node, NULL, NULL);
+ check_duplicate_label(c, bi, dt, l->label, node, NULL, NULL);
for_each_property(node, prop) {
struct marker *m = prop->val.markers;
for_each_label(prop->labels, l)
- check_duplicate_label(c, dt, l->label, node, prop, NULL);
+ check_duplicate_label(c, bi, dt, l->label, node, prop, NULL);
for_each_marker_of_type(m, LABEL)
- check_duplicate_label(c, dt, m->ref, node, prop, m);
+ check_duplicate_label(c, bi, dt, m->ref, node, prop, m);
}
}
ERROR(duplicate_label, check_duplicate_label_node, NULL);
-static cell_t check_phandle_prop(struct check *c, struct node *root,
- struct node *node, const char *propname)
+static cell_t check_phandle_prop(struct check *c, struct boot_info *bi,
+ struct node *root, struct node *node,
+ const char *propname)
{
struct property *prop;
struct marker *m;
@@ -398,8 +402,8 @@ static cell_t check_phandle_prop(struct check *c, struct node *root,
return phandle;
}
-static void check_explicit_phandles(struct check *c, struct node *root,
- struct node *node)
+static void check_explicit_phandles(struct check *c, struct boot_info *bi,
+ struct node *root, struct node *node)
{
struct node *other;
cell_t phandle, linux_phandle;
@@ -407,9 +411,9 @@ static void check_explicit_phandles(struct check *c, struct node *root,
/* Nothing should have assigned phandles yet */
assert(!node->phandle);
- phandle = check_phandle_prop(c, root, node, "phandle");
+ phandle = check_phandle_prop(c, bi, root, node, "phandle");
- linux_phandle = check_phandle_prop(c, root, node, "linux,phandle");
+ linux_phandle = check_phandle_prop(c, bi, root, node, "linux,phandle");
if (!phandle && !linux_phandle)
/* No valid phandles; nothing further to check */
@@ -433,8 +437,8 @@ static void check_explicit_phandles(struct check *c, struct node *root,
}
ERROR(explicit_phandles, check_explicit_phandles, NULL);
-static void check_name_properties(struct check *c, struct node *root,
- struct node *node)
+static void check_name_properties(struct check *c, struct boot_info *bi,
+ struct node *root, struct node *node)
{
struct property **pp, *prop = NULL;
@@ -467,8 +471,8 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
* Reference fixup functions
*/
-static void fixup_phandle_references(struct check *c, struct node *dt,
- struct node *node)
+static void fixup_phandle_references(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
@@ -495,8 +499,8 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
ERROR(phandle_references, fixup_phandle_references, NULL,
&duplicate_node_names, &explicit_phandles);
-static void fixup_path_references(struct check *c, struct node *dt,
- struct node *node)
+static void fixup_path_references(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
@@ -534,8 +538,8 @@ WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
WARNING_IF_NOT_STRING(model_is_string, "model");
WARNING_IF_NOT_STRING(status_is_string, "status");
-static void fixup_addr_size_cells(struct check *c, struct node *dt,
- struct node *node)
+static void fixup_addr_size_cells(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
@@ -558,8 +562,8 @@ WARNING(addr_size_cells, fixup_addr_size_cells, NULL,
#define node_size_cells(n) \
(((n)->size_cells == -1) ? 1 : (n)->size_cells)
-static void check_reg_format(struct check *c, struct node *dt,
- struct node *node)
+static void check_reg_format(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
int addr_cells, size_cells, entrylen;
@@ -587,8 +591,8 @@ static void check_reg_format(struct check *c, struct node *dt,
}
WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
-static void check_ranges_format(struct check *c, struct node *dt,
- struct node *node)
+static void check_ranges_format(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *prop;
int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen;
@@ -631,8 +635,8 @@ WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
/*
* Style checks
*/
-static void check_avoid_default_addr_size(struct check *c, struct node *dt,
- struct node *node)
+static void check_avoid_default_addr_size(struct check *c, struct boot_info *bi,
+ struct node *dt, struct node *node)
{
struct property *reg, *ranges;
@@ -657,6 +661,7 @@ WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
&addr_size_cells);
static void check_obsolete_chosen_interrupt_controller(struct check *c,
+ struct boot_info *bi,
struct node *dt,
struct node *node)
{
@@ -773,7 +778,7 @@ void process_checks(bool force, struct boot_info *bi)
struct check *c = check_table[i];
if (c->warn || c->error)
- error = error || run_check(c, dt);
+ error = error || run_check(c, bi, dt);
}
if (error) {
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v9 0/4] dtc: Dynamic DT support
From: Pantelis Antoniou @ 2016-11-24 12:31 UTC (permalink / raw)
To: David Gibson
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA,
Pantelis Antoniou
This patchset adds Dynamic DT support in the DTC compiler
as used in a number of boards like the beaglebone/rpi/chip and others.
The first patch passes an extra boot_info argument to the check methods.
The second patch documents the internals of overlay generation, while
the third one adds dynamic object/overlay support proper.
The last patch simply adds a few overlay tests verifying operation.
This patchset is against DTC mainline and is also available for a pull
request from https://github.com/pantoniou/dtc/tree/overlays
Regards
-- Pantelis
Changes since v8:
* Removed extra member of boot_info in each node; passing boot_info
parameter to the check methods instead.
* Reworked yacc syntax that supports both old and new plugin syntax
* Added handling for new magic number (enabled by 'M' switch).
* Dropped dtbo/asmo formats.
* Added overlay testsuite.
* Addressed last version maintainer comments.
Changes since v7:
* Dropped xasprintf & backward compatibility patch
* Rebased against dgibson's overlay branch
* Minor doc wording fixes.
Changes since v6:
* Introduced xasprintf
* Added append_to_property and used it
* Changed some die()'s to assert
* Reordered node generation to respect sort
* Addressed remaining maintainer changes from v6
Changes since v5:
* Rebase to latest dtc version.
* Addressed all the maintainer requested changes from v5
* Added new magic value for dynamic objects and new format
Changes since v4:
* Rebase to latest dtc version.
* Completely redesigned the generation of resolution data.
Now instead of being generated as part of blob generation
they are created in the live tree.
* Consequently the patchset is much smaller.
* Added -A auto-label alias generation option.
* Addressed maintainer comments.
* Added syntactic sugar for overlays in the form of .dtsi
* Added /dts-v1/ /plugin/ preferred plugin form and deprecate
the previous form (although still works for backward compatibility)
Changes since v3:
* Rebase to latest dtc version.
Changes since v2:
* Split single patch to a patchset.
* Updated to dtc mainline.
* Changed __local_fixups__ format
* Clean up for better legibility.
Pantelis Antoniou (4):
checks: Pass boot_info to the check methods
dtc: Document the dynamic plugin internals
dtc: Plugin and fixup support
tests: Add overlay tests
Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
Documentation/manual.txt | 25 ++-
checks.c | 121 +++++++------
dtc-lexer.l | 5 +
dtc-parser.y | 49 +++++-
dtc.c | 39 ++++-
dtc.h | 20 ++-
fdtdump.c | 2 +-
flattree.c | 17 +-
fstree.c | 2 +-
libfdt/fdt.c | 2 +-
libfdt/fdt.h | 3 +-
livetree.c | 225 ++++++++++++++++++++++++-
tests/mangle-layout.c | 7 +-
tests/overlay_overlay_dtc.dts | 76 +--------
tests/overlay_overlay_dtc.dtsi | 83 +++++++++
tests/overlay_overlay_new_dtc.dts | 11 ++
tests/overlay_overlay_simple.dts | 12 ++
tests/run_tests.sh | 20 +++
treesource.c | 7 +-
20 files changed, 884 insertions(+), 160 deletions(-)
create mode 100644 Documentation/dt-object-internal.txt
create mode 100644 tests/overlay_overlay_dtc.dtsi
create mode 100644 tests/overlay_overlay_new_dtc.dts
create mode 100644 tests/overlay_overlay_simple.dts
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/6] arm64: arch_timer: Add device tree binding for hisilicon-161601 erratum
From: John Garry @ 2016-11-24 12:12 UTC (permalink / raw)
To: Ding Tianhong, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, marc.zyngier-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, oss-fOR+EgIDQEHk1uMJSBkQmQ,
devicetree-u79uwXL29TY76Z2rM5mHXA,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linuxarm-hv44wF8Li93QT0dZR+AlfA,
hanjun.guo-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <c3020c0d-bc45-2bb1-f53a-c5e76ab45499-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
On 21/11/2016 12:49, Ding Tianhong wrote:
> Ping....
Hi,
was there a cover letter for 0/6? I never saw it.
Thanks,
John
>
> On 2016/11/15 20:16, Ding Tianhong wrote:
>> This erratum describes a bug in logic outside the core, so MIDR can't be
>> used to identify its presence, and reading an SoC-specific revision
>> register from common arch timer code would be awkward. So, describe it
>> in the device tree.
>>
>> v2: Use the new erratum name and update the description.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> index ef5fbe9..c27b2c4 100644
>> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> @@ -31,6 +31,14 @@ to deliver its interrupts via SPIs.
>> This also affects writes to the tval register, due to the implicit
>> counter read.
>>
>> +- hisilicon,erratum-161601 : A boolean property. Indicates the presence of
>> + erratum 161601, which says that reading the counter is unreliable unless
>> + reading twice on the register and the value of the second read is larger
>> + than the first by less than 32. If the verification is unsuccessful, then
>> + discard the value of this read and repeat this procedure until the verification
>> + is successful. This also affects writes to the tval register, due to the
>> + implicit counter read.
>> +
>> ** Optional properties:
>>
>> - arm,cpu-registers-not-fw-configured : Firmware does not initialize
>>
>
> _______________________________________________
> linuxarm mailing list
> linuxarm-hv44wF8Li93QT0dZR+AlfA@public.gmane.org
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-24 11:37 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Ziji Hu, Jack(SH) Zhu,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Shiwu Zhang, Victor Gu, Wei(SOCP)
In-Reply-To: <a05ffd140f4edc02fc3128db8445b2264cf38723.1477911954.git-series.gregory.clement@free-electrons.com>
On 31 October 2016 at 12:09, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> From: Ziji Hu <huziji@marvell.com>
>
> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
> Three types of PHYs are supported.
>
> Add support to multiple types of PHYs init and configuration.
> Add register definitions of PHYs.
>
> Signed-off-by: Hu Ziji <huziji@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> MAINTAINERS | 2 +-
> drivers/mmc/host/Makefile | 2 +-
> drivers/mmc/host/sdhci-xenon-phy.c | 1181 +++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci-xenon-phy.h | 157 ++++-
> drivers/mmc/host/sdhci-xenon.c | 4 +-
> drivers/mmc/host/sdhci-xenon.h | 17 +-
> 6 files changed, 1361 insertions(+), 2 deletions(-)
> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
> create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
Can you please consider to split this up somehow!? It would make it
easier to review...
Anyway, allow me to provide some initial feedback, particularly around
those things that Adrian and you requested for my input.
[...]
>
> +
> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
> +{
> + int err;
> + u8 *ext_csd = NULL;
> +
> + err = mmc_get_ext_csd(card, &ext_csd);
> + kfree(ext_csd);
Why do you read the ext csd here?
> +
> + return err;
> +}
> +
> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = SD_IO_RW_DIRECT;
> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + if (err)
> + return err;
> +
> + if (cmd.resp[0] & R5_ERROR)
> + return -EIO;
> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> + return -EINVAL;
> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
> + return -ERANGE;
> + return 0;
No thanks! MMC/SD/SDIO protocol code belongs in the core.
> +}
> +
> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = MMC_SEND_STATUS;
> + cmd.arg = card->rca << 16;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + return err;
No thanks! MMC/SD/SDIO protocol code belongs in the core.
> +}
> +
[...]
> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
> +{
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_card *card;
> + int ret = 0;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + if (!host->clock) {
> + priv->clock = 0;
> + return 0;
> + }
> +
> + /*
> + * The timing, frequency or bus width is changed,
> + * better to set eMMC PHY based on current setting
> + * and adjust Xenon SDHC delay.
> + */
> + if ((host->clock == priv->clock) &&
> + (ios->bus_width == priv->bus_width) &&
> + (ios->timing == priv->timing))
> + return 0;
> +
> + xenon_phy_set(host, ios->timing);
> +
> + /* Update the record */
> + priv->bus_width = ios->bus_width;
> + /* Temp stage from HS200 to HS400 */
> + if (((priv->timing == MMC_TIMING_MMC_HS200) &&
> + (ios->timing == MMC_TIMING_MMC_HS)) ||
> + ((ios->timing == MMC_TIMING_MMC_HS) &&
> + (priv->clock > host->clock))) {
> + priv->timing = ios->timing;
> + priv->clock = host->clock;
> + return 0;
> + }
> + /*
> + * Skip temp stages from HS400 t0 HS200:
> + * from 200MHz to 52MHz in HS400
> + * from HS400 to HS DDR in 52MHz
> + * from HS DDR to HS in 52MHz
> + * from HS to HS200 in 52MHz
> + */
> + if (((priv->timing == MMC_TIMING_MMC_HS400) &&
> + ((host->clock == MMC_HIGH_52_MAX_DTR) ||
> + (ios->timing == MMC_TIMING_MMC_DDR52))) ||
> + ((priv->timing == MMC_TIMING_MMC_DDR52) &&
> + (ios->timing == MMC_TIMING_MMC_HS)) ||
> + ((ios->timing == MMC_TIMING_MMC_HS200) &&
> + (ios->clock == MMC_HIGH_52_MAX_DTR))) {
> + priv->timing = ios->timing;
> + priv->clock = host->clock;
> + return 0;
> + }
> + priv->timing = ios->timing;
> + priv->clock = host->clock;
> +
> + /* Legacy mode is a special case */
> + if (ios->timing == MMC_TIMING_LEGACY)
> + return 0;
> +
> + if (mmc->card)
> + card = mmc->card;
> + else
> + /*
> + * Only valid during initialization
> + * before mmc->card is set
> + */
> + card = priv->card_candidate;
> + if (unlikely(!card)) {
> + dev_warn(mmc_dev(mmc), "card is not present\n");
> + return -EINVAL;
> + }
That your host need to hold a copy of the card pointer, tells me that
something is not really correct.
I might be wrong, if this turns out to be a special case, but I doubt
it. Although, if it *is* a special such case, we shall most likely try
to extend the the mmc core layer instead of adding all these hacks in
your host driver.
[...]
Another suggestion of a general improvement; could you perhaps try to
add some brief information about what goes on in function headers.
Perhaps that could help to more easily understand things.
Kind regards
Uffe
^ permalink raw reply
* [PATCH V4 10/10] PM / OPP: Don't assume platform doesn't have regulators
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.
If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.
Handle that properly by making sure that the DT didn't have any entries
for supply voltages as well.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/opp/core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index d3f4c625b809..337057783e73 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -734,7 +734,20 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */
if (!regulators) {
+ unsigned long u_volt = opp->supplies[0].u_volt;
+
rcu_read_unlock();
+
+ /*
+ * DT contained supply ratings? Consider platform failed to set
+ * regulators.
+ */
+ if (unlikely(u_volt)) {
+ dev_err(dev, "%s: Regulator not registered with OPP core\n",
+ __func__);
+ return -EINVAL;
+ }
+
return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
}
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 09/10] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
If a platform specific OPP driver has called this routine first and set
the regulators, then the second call from cpufreq-dt driver will hit the
WARN_ON(). Remove the WARN_ON(), but continue to return error in such
cases.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 7c945d5950bf..d3f4c625b809 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1469,7 +1469,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
}
/* Already have regulators set */
- if (WARN_ON(opp_table->regulators)) {
+ if (opp_table->regulators) {
ret = -EBUSY;
goto err;
}
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 08/10] PM / OPP: Allow platform specific custom set_opp() callbacks
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
The generic set_opp() handler isn't sufficient for platforms with
complex DVFS. For example, some TI platforms have multiple regulators
for a CPU device. The order in which various supplies need to be
programmed is only known to the platform code and its best to leave it
to it.
This patch implements APIs to register platform specific set_opp()
callback.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 116 +++++++++++++++++++++++++++++++++++++++++-
drivers/base/power/opp/opp.h | 2 +
include/linux/pm_opp.h | 10 ++++
3 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 99491f4099e5..7c945d5950bf 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -673,6 +673,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
struct opp_table *opp_table;
unsigned long freq, old_freq;
+ int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data);
struct dev_pm_opp *old_opp, *opp;
struct regulator **regulators;
struct dev_pm_set_opp_data *data;
@@ -737,6 +738,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
}
+ if (opp_table->set_opp)
+ set_opp = opp_table->set_opp;
+ else
+ set_opp = _generic_set_opp;
+
data = opp_table->set_opp_data;
data->regulators = regulators;
data->regulator_count = opp_table->regulator_count;
@@ -754,7 +760,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
rcu_read_unlock();
- return _generic_set_opp(dev, data);
+ return set_opp(dev, data);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -888,6 +894,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
if (opp_table->regulators)
return;
+ if (opp_table->set_opp)
+ return;
+
/* Release clk */
if (!IS_ERR(opp_table->clk))
clk_put(opp_table->clk);
@@ -1488,7 +1497,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
opp_table->regulator_count = count;
- /* Allocate block only once to pass to ->set_rate() */
+ /* Allocate block only once to pass to ->set_opp() */
ret = _allocate_set_opp_data(opp_table);
if (ret)
goto free_regulators;
@@ -1563,6 +1572,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/**
+ * dev_pm_opp_register_set_opp_helper() - Register custom OPP set rate helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_opp: Custom set OPP helper.
+ *
+ * This is useful to support complex platforms (like platforms with multiple
+ * regulators per device), instead of the generic OPP set rate helper.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_register_set_opp_helper(struct device *dev,
+ int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data))
+{
+ struct opp_table *opp_table;
+ int ret;
+
+ if (!set_opp)
+ return -EINVAL;
+
+ mutex_lock(&opp_table_lock);
+
+ opp_table = _add_opp_table(dev);
+ if (!opp_table) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ /* This should be called before OPPs are initialized */
+ if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ /* Already have custom set_opp helper */
+ if (WARN_ON(opp_table->set_opp)) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ opp_table->set_opp = set_opp;
+
+ mutex_unlock(&opp_table_lock);
+ return 0;
+
+err:
+ _remove_opp_table(opp_table);
+unlock:
+ mutex_unlock(&opp_table_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper);
+
+/**
+ * dev_pm_opp_register_put_opp_helper() - Releases resources blocked for
+ * set_opp helper
+ * @dev: Device for which custom set_opp helper has to be cleared.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_register_put_opp_helper(struct device *dev)
+{
+ struct opp_table *opp_table;
+
+ mutex_lock(&opp_table_lock);
+
+ /* Check for existing table for 'dev' first */
+ opp_table = _find_opp_table(dev);
+ if (IS_ERR(opp_table)) {
+ dev_err(dev, "Failed to find opp_table: %ld\n",
+ PTR_ERR(opp_table));
+ goto unlock;
+ }
+
+ if (!opp_table->set_opp) {
+ dev_err(dev, "%s: Doesn't have custom set_opp helper set\n",
+ __func__);
+ goto unlock;
+ }
+
+ /* Make sure there are no concurrent readers while updating opp_table */
+ WARN_ON(!list_empty(&opp_table->opp_list));
+
+ opp_table->set_opp = NULL;
+
+ /* Try freeing opp_table if this was the last blocking resource */
+ _remove_opp_table(opp_table);
+
+unlock:
+ mutex_unlock(&opp_table_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper);
+
+/**
* dev_pm_opp_add() - Add an OPP table from a table definitions
* @dev: device for which we do this operation
* @freq: Frequency in Hz for this OPP
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index a05e43912c6b..5c038701fe34 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -141,6 +141,7 @@ enum opp_table_access {
* @clk: Device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators
+ * @set_opp: Platform specific set_opp callback
* @set_opp_data: Data to be passed to set_opp callback
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
@@ -179,6 +180,7 @@ struct opp_table {
struct regulator **regulators;
unsigned int regulator_count;
+ int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data);
struct dev_pm_set_opp_data *set_opp_data;
#ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2969519bf5f7..cb5bc4747502 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -113,6 +113,8 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct device *dev);
int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
void dev_pm_opp_put_regulators(struct device *dev);
+int dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data));
+void dev_pm_opp_register_put_opp_helper(struct device *dev);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -212,6 +214,14 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
+static inline int dev_pm_opp_register_set_opp_helper(struct device *dev,
+ int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data))
+{
+ return -ENOTSUPP;
+}
+
+static inline void dev_pm_opp_register_put_opp_helper(struct device *dev) {}
+
static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
{
return -ENOTSUPP;
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 07/10] PM / OPP: Separate out _generic_opp_set_rate()
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
Later patches would add support for custom opp_set_rate callbacks. This
patch separates out the code for generic opp_set_rate handler in order
to prepare for that.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 180 +++++++++++++++++++++++++++++-------------
drivers/base/power/opp/opp.h | 3 +
include/linux/pm_opp.h | 33 ++++++++
3 files changed, 163 insertions(+), 53 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 2d5c726c920f..99491f4099e5 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -596,6 +596,69 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
return ret;
}
+static inline int
+_generic_set_opp_clk_only(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long freq)
+{
+ int ret;
+
+ ret = clk_set_rate(clk, freq);
+ if (ret) {
+ dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+ ret);
+ }
+
+ return ret;
+}
+
+static int _generic_set_opp(struct device *dev,
+ struct dev_pm_set_opp_data *data)
+{
+ struct dev_pm_opp_supply *old_supply = data->old_opp.supplies;
+ struct dev_pm_opp_supply *new_supply = data->new_opp.supplies;
+ unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
+ struct regulator *reg = data->regulators[0];
+ int ret;
+
+ /* This function only supports single regulator per device */
+ if (WARN_ON(data->regulator_count > 1)) {
+ dev_err(dev, "multiple regulators are not supported\n");
+ return -EINVAL;
+ }
+
+ /* Scaling up? Scale voltage before frequency */
+ if (freq > old_freq) {
+ ret = _set_opp_voltage(dev, reg, new_supply);
+ if (ret)
+ goto restore_voltage;
+ }
+
+ /* Change frequency */
+ ret = _generic_set_opp_clk_only(dev, data->clk, old_freq, freq);
+ if (ret)
+ goto restore_voltage;
+
+ /* Scaling down? Scale voltage after frequency */
+ if (freq < old_freq) {
+ ret = _set_opp_voltage(dev, reg, new_supply);
+ if (ret)
+ goto restore_freq;
+ }
+
+ return 0;
+
+restore_freq:
+ if (_generic_set_opp_clk_only(dev, data->clk, freq, old_freq))
+ dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+ __func__, old_freq);
+restore_voltage:
+ /* This shouldn't harm even if the voltages weren't updated earlier */
+ if (old_supply->u_volt)
+ _set_opp_voltage(dev, reg, old_supply);
+
+ return ret;
+}
+
/**
* dev_pm_opp_set_rate() - Configure new OPP based on frequency
* @dev: device for which we do this operation
@@ -609,12 +672,12 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
struct opp_table *opp_table;
+ unsigned long freq, old_freq;
struct dev_pm_opp *old_opp, *opp;
- struct regulator *reg = ERR_PTR(-ENXIO);
+ struct regulator **regulators;
+ struct dev_pm_set_opp_data *data;
struct clk *clk;
- unsigned long freq, old_freq;
- struct dev_pm_opp_supply old_supply, new_supply;
- int ret;
+ int ret, size;
if (unlikely(!target_freq)) {
dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
@@ -663,64 +726,35 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return ret;
}
- if (opp_table->regulators) {
- /* This function only supports single regulator per device */
- if (WARN_ON(opp_table->regulator_count > 1)) {
- dev_err(dev, "multiple regulators not supported\n");
- rcu_read_unlock();
- return -EINVAL;
- }
+ dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
+ old_freq, freq);
- reg = opp_table->regulators[0];
+ regulators = opp_table->regulators;
+
+ /* Only frequency scaling */
+ if (!regulators) {
+ rcu_read_unlock();
+ return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
}
+ data = opp_table->set_opp_data;
+ data->regulators = regulators;
+ data->regulator_count = opp_table->regulator_count;
+ data->clk = clk;
+
+ data->old_opp.rate = old_freq;
+ size = sizeof(*opp->supplies) * opp_table->regulator_count;
if (IS_ERR(old_opp))
- old_supply.u_volt = 0;
+ memset(data->old_opp.supplies, 0, size);
else
- memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
+ memcpy(data->old_opp.supplies, old_opp->supplies, size);
- memcpy(&new_supply, opp->supplies, sizeof(new_supply));
+ data->new_opp.rate = freq;
+ memcpy(data->new_opp.supplies, opp->supplies, size);
rcu_read_unlock();
- /* Scaling up? Scale voltage before frequency */
- if (freq > old_freq) {
- ret = _set_opp_voltage(dev, reg, &new_supply);
- if (ret)
- goto restore_voltage;
- }
-
- /* Change frequency */
-
- dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
- __func__, old_freq, freq);
-
- ret = clk_set_rate(clk, freq);
- if (ret) {
- dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
- ret);
- goto restore_voltage;
- }
-
- /* Scaling down? Scale voltage after frequency */
- if (freq < old_freq) {
- ret = _set_opp_voltage(dev, reg, &new_supply);
- if (ret)
- goto restore_freq;
- }
-
- return 0;
-
-restore_freq:
- if (clk_set_rate(clk, old_freq))
- dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
- __func__, old_freq);
-restore_voltage:
- /* This shouldn't harm even if the voltages weren't updated earlier */
- if (old_supply.u_volt)
- _set_opp_voltage(dev, reg, &old_supply);
-
- return ret;
+ return _generic_set_opp(dev, data);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -1354,6 +1388,38 @@ void dev_pm_opp_put_prop_name(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
+static int _allocate_set_opp_data(struct opp_table *opp_table)
+{
+ struct dev_pm_set_opp_data *data;
+ int len, count = opp_table->regulator_count;
+
+ if (WARN_ON(!count))
+ return -EINVAL;
+
+ /* space for set_opp_data */
+ len = sizeof(*data);
+
+ /* space for old_opp.supplies and new_opp.supplies */
+ len += 2 * sizeof(struct dev_pm_opp_supply) * count;
+
+ data = kzalloc(len, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->old_opp.supplies = (void *)(data + 1);
+ data->new_opp.supplies = data->old_opp.supplies + count;
+
+ opp_table->set_opp_data = data;
+
+ return 0;
+}
+
+static void _free_set_opp_data(struct opp_table *opp_table)
+{
+ kfree(opp_table->set_opp_data);
+ opp_table->set_opp_data = NULL;
+}
+
/**
* dev_pm_opp_set_regulators() - Set regulator names for the device
* @dev: Device for which regulator name is being set.
@@ -1422,6 +1488,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
opp_table->regulator_count = count;
+ /* Allocate block only once to pass to ->set_rate() */
+ ret = _allocate_set_opp_data(opp_table);
+ if (ret)
+ goto free_regulators;
+
mutex_unlock(&opp_table_lock);
return 0;
@@ -1431,6 +1502,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
kfree(opp_table->regulators);
opp_table->regulators = NULL;
+ opp_table->regulator_count = 0;
err:
_remove_opp_table(opp_table);
unlock:
@@ -1476,6 +1548,8 @@ void dev_pm_opp_put_regulators(struct device *dev)
for (i = opp_table->regulator_count - 1; i >= 0; i--)
regulator_put(opp_table->regulators[i]);
+ _free_set_opp_data(opp_table);
+
kfree(opp_table->regulators);
opp_table->regulators = NULL;
opp_table->regulator_count = 0;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 5b0f7e53bede..a05e43912c6b 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -141,6 +141,7 @@ enum opp_table_access {
* @clk: Device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators
+ * @set_opp_data: Data to be passed to set_opp callback
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
*
@@ -178,6 +179,8 @@ struct opp_table {
struct regulator **regulators;
unsigned int regulator_count;
+ struct dev_pm_set_opp_data *set_opp_data;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 27eea9bfc5ed..2969519bf5f7 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -17,6 +17,8 @@
#include <linux/err.h>
#include <linux/notifier.h>
+struct clk;
+struct regulator;
struct dev_pm_opp;
struct device;
@@ -40,6 +42,37 @@ struct dev_pm_opp_supply {
unsigned long u_amp;
};
+/**
+ * struct dev_pm_opp_info - OPP freq/voltage/current values
+ * @rate: Target clk rate in hz
+ * @supplies: Array of voltage/current values for all power supplies
+ *
+ * This structure stores the freq/voltage/current values for a single OPP.
+ */
+struct dev_pm_opp_info {
+ unsigned long rate;
+ struct dev_pm_opp_supply *supplies;
+};
+
+/**
+ * struct dev_pm_set_opp_data - Set OPP data
+ * @old_opp: Old OPP info
+ * @new_opp: New OPP info
+ * @regulators: Array of regulator pointers
+ * @regulator_count: Number of regulators
+ * @clk: Pointer to clk
+ *
+ * This structure contains all information required for setting an OPP.
+ */
+struct dev_pm_set_opp_data {
+ struct dev_pm_opp_info old_opp;
+ struct dev_pm_opp_info new_opp;
+
+ struct regulator **regulators;
+ unsigned int regulator_count;
+ struct clk *clk;
+};
+
#if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 06/10] PM / OPP: Add infrastructure to manage multiple regulators
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
This patch adds infrastructure to manage multiple regulators and updates
the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with
multiple regulators.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 222 ++++++++++++++++++++++++++-------------
drivers/base/power/opp/debugfs.c | 52 +++++++--
drivers/base/power/opp/of.c | 103 +++++++++++++-----
drivers/base/power/opp/opp.h | 10 +-
drivers/cpufreq/cpufreq-dt.c | 9 +-
include/linux/pm_opp.h | 8 +-
6 files changed, 282 insertions(+), 122 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 37fad2eb0f47..2d5c726c920f 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)
* Return: voltage in micro volt corresponding to the opp, else
* return 0
*
+ * This is useful only for devices with single power supply.
+ *
* Locking: This function must be called under rcu_read_lock(). opp is a rcu
* protected pointer. This means that opp which could have been fetched by
* opp_find_freq_{exact,ceil,floor} functions is valid as long as we are
@@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
if (IS_ERR_OR_NULL(tmp_opp))
pr_err("%s: Invalid parameters\n", __func__);
else
- v = tmp_opp->supply.u_volt;
+ v = tmp_opp->supplies[0].u_volt;
return v;
}
@@ -222,10 +224,13 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
{
struct opp_table *opp_table;
struct dev_pm_opp *opp;
- struct regulator *reg;
+ struct regulator *reg, **regulators;
unsigned long latency_ns = 0;
- unsigned long min_uV = ~0, max_uV = 0;
- int ret;
+ int ret, size, i, count;
+ struct {
+ unsigned long min;
+ unsigned long max;
+ } *uV;
rcu_read_lock();
@@ -235,21 +240,41 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
return 0;
}
- reg = opp_table->regulator;
- if (IS_ERR(reg)) {
+ count = opp_table->regulator_count;
+
+ if (!count) {
/* Regulator may not be required for device */
rcu_read_unlock();
return 0;
}
- list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
- if (!opp->available)
- continue;
+ size = count * sizeof(*regulators);
+ regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
+ if (!regulators) {
+ rcu_read_unlock();
+ return 0;
+ }
+
+ uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL);
+ if (!uV) {
+ kfree(regulators);
+ rcu_read_unlock();
+ return 0;
+ }
- if (opp->supply.u_volt_min < min_uV)
- min_uV = opp->supply.u_volt_min;
- if (opp->supply.u_volt_max > max_uV)
- max_uV = opp->supply.u_volt_max;
+ for (i = 0; i < count; i++) {
+ uV[i].min = ~0;
+ uV[i].max = 0;
+
+ list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
+ if (!opp->available)
+ continue;
+
+ if (opp->supplies[i].u_volt_min < uV[i].min)
+ uV[i].min = opp->supplies[i].u_volt_min;
+ if (opp->supplies[i].u_volt_max > uV[i].max)
+ uV[i].max = opp->supplies[i].u_volt_max;
+ }
}
rcu_read_unlock();
@@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
* The caller needs to ensure that opp_table (and hence the regulator)
* isn't freed, while we are executing this routine.
*/
- ret = regulator_set_voltage_time(reg, min_uV, max_uV);
- if (ret > 0)
- latency_ns = ret * 1000;
+ for (i = 0; reg = regulators[i], i < count; i++) {
+ ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max);
+ if (ret > 0)
+ latency_ns += ret * 1000;
+ }
+
+ kfree(uV);
+ kfree(regulators);
return latency_ns;
}
@@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
struct opp_table *opp_table;
struct dev_pm_opp *old_opp, *opp;
- struct regulator *reg;
+ struct regulator *reg = ERR_PTR(-ENXIO);
struct clk *clk;
unsigned long freq, old_freq;
struct dev_pm_opp_supply old_supply, new_supply;
@@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return ret;
}
+ if (opp_table->regulators) {
+ /* This function only supports single regulator per device */
+ if (WARN_ON(opp_table->regulator_count > 1)) {
+ dev_err(dev, "multiple regulators not supported\n");
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+
+ reg = opp_table->regulators[0];
+ }
+
if (IS_ERR(old_opp))
old_supply.u_volt = 0;
else
- memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
-
- memcpy(&new_supply, &opp->supply, sizeof(new_supply));
+ memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
- reg = opp_table->regulator;
+ memcpy(&new_supply, opp->supplies, sizeof(new_supply));
rcu_read_unlock();
@@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
_of_init_opp_table(opp_table, dev);
- /* Set regulator to a non-NULL error value */
- opp_table->regulator = ERR_PTR(-ENXIO);
-
/* Find clk for the device */
opp_table->clk = clk_get(dev, NULL);
if (IS_ERR(opp_table->clk)) {
@@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
if (opp_table->prop_name)
return;
- if (!IS_ERR(opp_table->regulator))
+ if (opp_table->regulators)
return;
/* Release clk */
@@ -924,35 +960,50 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
struct opp_table **opp_table)
{
struct dev_pm_opp *opp;
+ int count, supply_size;
+ struct opp_table *table;
- /* allocate new OPP node */
- opp = kzalloc(sizeof(*opp), GFP_KERNEL);
- if (!opp)
+ table = _add_opp_table(dev);
+ if (!table)
return NULL;
- INIT_LIST_HEAD(&opp->node);
+ /* Allocate space for at least one supply */
+ count = table->regulator_count ? table->regulator_count : 1;
+ supply_size = sizeof(*opp->supplies) * count;
- *opp_table = _add_opp_table(dev);
- if (!*opp_table) {
- kfree(opp);
+ /* allocate new OPP node + and supplies structures */
+ opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
+ if (!opp) {
+ kfree(table);
return NULL;
}
+ /* Put the supplies at the end of the OPP structure as an empty array */
+ opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
+ INIT_LIST_HEAD(&opp->node);
+
+ *opp_table = table;
+
return opp;
}
static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
struct opp_table *opp_table)
{
- struct regulator *reg = opp_table->regulator;
-
- if (!IS_ERR(reg) &&
- !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
- opp->supply.u_volt_max)) {
- pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
- __func__, opp->supply.u_volt_min,
- opp->supply.u_volt_max);
- return false;
+ struct regulator *reg;
+ int i;
+
+ for (i = 0; i < opp_table->regulator_count; i++) {
+ reg = opp_table->regulators[i];
+
+ if (!regulator_is_supported_voltage(reg,
+ opp->supplies[i].u_volt_min,
+ opp->supplies[i].u_volt_max)) {
+ pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
+ __func__, opp->supplies[i].u_volt_min,
+ opp->supplies[i].u_volt_max);
+ return false;
+ }
}
return true;
@@ -984,12 +1035,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Duplicate OPPs */
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
- __func__, opp->rate, opp->supply.u_volt,
- opp->available, new_opp->rate, new_opp->supply.u_volt,
- new_opp->available);
+ __func__, opp->rate, opp->supplies[0].u_volt,
+ opp->available, new_opp->rate,
+ new_opp->supplies[0].u_volt, new_opp->available);
+ /* Should we compare voltages for all regulators here ? */
return opp->available &&
- new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
+ new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
}
new_opp->opp_table = opp_table;
@@ -1056,9 +1108,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
/* populate the opp table */
new_opp->rate = freq;
tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
- new_opp->supply.u_volt = u_volt;
- new_opp->supply.u_volt_min = u_volt - tol;
- new_opp->supply.u_volt_max = u_volt + tol;
+ new_opp->supplies[0].u_volt = u_volt;
+ new_opp->supplies[0].u_volt_min = u_volt - tol;
+ new_opp->supplies[0].u_volt_max = u_volt + tol;
new_opp->available = true;
new_opp->dynamic = dynamic;
@@ -1303,12 +1355,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
/**
- * dev_pm_opp_set_regulator() - Set regulator name for the device
+ * dev_pm_opp_set_regulators() - Set regulator names for the device
* @dev: Device for which regulator name is being set.
- * @name: Name of the regulator.
+ * @names: Array of pointers to the names of the regulator.
+ * @count: Number of regulators.
*
* In order to support OPP switching, OPP layer needs to know the name of the
- * device's regulator, as the core would be required to switch voltages as well.
+ * device's regulators, as the core would be required to switch voltages as
+ * well.
*
* This must be called before any OPPs are initialized for the device.
*
@@ -1318,11 +1372,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
* that this function is *NOT* called under RCU protection or in contexts where
* mutex cannot be locked.
*/
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
+ unsigned int count)
{
struct opp_table *opp_table;
struct regulator *reg;
- int ret;
+ int ret, i;
mutex_lock(&opp_table_lock);
@@ -1338,26 +1393,44 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
goto err;
}
- /* Already have a regulator set */
- if (WARN_ON(!IS_ERR(opp_table->regulator))) {
+ /* Already have regulators set */
+ if (WARN_ON(opp_table->regulators)) {
ret = -EBUSY;
goto err;
}
- /* Allocate the regulator */
- reg = regulator_get_optional(dev, name);
- if (IS_ERR(reg)) {
- ret = PTR_ERR(reg);
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "%s: no regulator (%s) found: %d\n",
- __func__, name, ret);
+
+ opp_table->regulators = kmalloc_array(count,
+ sizeof(*opp_table->regulators),
+ GFP_KERNEL);
+ if (!opp_table->regulators) {
+ ret = -ENOMEM;
goto err;
}
- opp_table->regulator = reg;
+ for (i = 0; i < count; i++) {
+ reg = regulator_get_optional(dev, names[i]);
+ if (IS_ERR(reg)) {
+ ret = PTR_ERR(reg);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "%s: regulator (%s) not found: %d\n",
+ __func__, names[i], ret);
+ goto free_regulators;
+ }
+
+ opp_table->regulators[i] = reg;
+ }
+
+ opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock);
return 0;
+free_regulators:
+ while (i != 0)
+ regulator_put(opp_table->regulators[--i]);
+
+ kfree(opp_table->regulators);
+ opp_table->regulators = NULL;
err:
_remove_opp_table(opp_table);
unlock:
@@ -1365,11 +1438,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
return ret;
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
/**
- * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
+ * @dev: Device for which regulators were set.
*
* Locking: The internal opp_table and opp structures are RCU protected.
* Hence this function internally uses RCU updater strategy with mutex locks
@@ -1377,9 +1450,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
* that this function is *NOT* called under RCU protection or in contexts where
* mutex cannot be locked.
*/
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulators(struct device *dev)
{
struct opp_table *opp_table;
+ int i;
mutex_lock(&opp_table_lock);
@@ -1391,16 +1465,20 @@ void dev_pm_opp_put_regulator(struct device *dev)
goto unlock;
}
- if (IS_ERR(opp_table->regulator)) {
- dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+ if (!opp_table->regulators) {
+ dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
goto unlock;
}
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
- regulator_put(opp_table->regulator);
- opp_table->regulator = ERR_PTR(-ENXIO);
+ for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ regulator_put(opp_table->regulators[i]);
+
+ kfree(opp_table->regulators);
+ opp_table->regulators = NULL;
+ opp_table->regulator_count = 0;
/* Try freeing opp_table if this was the last blocking resource */
_remove_opp_table(opp_table);
@@ -1408,7 +1486,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
unlock:
mutex_unlock(&opp_table_lock);
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/**
* dev_pm_opp_add() - Add an OPP table from a table definitions
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index c897676ca35f..95f433db4ac7 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -15,6 +15,7 @@
#include <linux/err.h>
#include <linux/init.h>
#include <linux/limits.h>
+#include <linux/slab.h>
#include "opp.h"
@@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
debugfs_remove_recursive(opp->dentry);
}
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
+ struct opp_table *opp_table,
+ struct dentry *pdentry)
+{
+ struct dentry *d;
+ int i = 0;
+ char *name;
+
+ /* Always create at least supply-0 directory */
+ do {
+ name = kasprintf(GFP_KERNEL, "supply-%d", i);
+
+ /* Create per-opp directory */
+ d = debugfs_create_dir(name, pdentry);
+
+ kfree(name);
+
+ if (!d)
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
+ &opp->supplies[i].u_volt))
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
+ &opp->supplies[i].u_volt_min))
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
+ &opp->supplies[i].u_volt_max))
+ return false;
+
+ if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
+ &opp->supplies[i].u_amp))
+ return false;
+ } while (++i < opp_table->regulator_count);
+
+ return true;
+}
+
int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
{
struct dentry *pdentry = opp_table->dentry;
@@ -63,16 +104,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
+ if (!opp_debug_create_supplies(opp, opp_table, d))
return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index bdf409d42126..3f7d2591b173 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/device.h>
#include <linux/of.h>
+#include <linux/slab.h>
#include <linux/export.h>
#include "opp.h"
@@ -101,16 +102,16 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
return true;
}
-/* TODO: Support multiple regulators */
static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
struct opp_table *opp_table)
{
- u32 microvolt[3] = {0};
- u32 val;
- int count, ret;
+ u32 *microvolt, *microamp = NULL;
+ int supplies, vcount, icount, ret, i, j;
struct property *prop = NULL;
char name[NAME_MAX];
+ supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
+
/* Search for "opp-microvolt-<name>" */
if (opp_table->prop_name) {
snprintf(name, sizeof(name), "opp-microvolt-%s",
@@ -128,34 +129,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
return 0;
}
- count = of_property_count_u32_elems(opp->np, name);
- if (count < 0) {
+ vcount = of_property_count_u32_elems(opp->np, name);
+ if (vcount < 0) {
dev_err(dev, "%s: Invalid %s property (%d)\n",
- __func__, name, count);
- return count;
+ __func__, name, vcount);
+ return vcount;
}
- /* There can be one or three elements here */
- if (count != 1 && count != 3) {
- dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n",
- __func__, name, count);
+ /* There can be one or three elements per supply */
+ if (vcount != supplies && vcount != supplies * 3) {
+ dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
+ __func__, name, vcount, supplies);
return -EINVAL;
}
- ret = of_property_read_u32_array(opp->np, name, microvolt, count);
+ microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
+ if (!microvolt)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(opp->np, name, microvolt, vcount);
if (ret) {
dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
- return -EINVAL;
- }
-
- opp->supply.u_volt = microvolt[0];
-
- if (count == 1) {
- opp->supply.u_volt_min = opp->supply.u_volt;
- opp->supply.u_volt_max = opp->supply.u_volt;
- } else {
- opp->supply.u_volt_min = microvolt[1];
- opp->supply.u_volt_max = microvolt[2];
+ ret = -EINVAL;
+ goto free_microvolt;
}
/* Search for "opp-microamp-<name>" */
@@ -172,10 +168,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
prop = of_find_property(opp->np, name, NULL);
}
- if (prop && !of_property_read_u32(opp->np, name, &val))
- opp->supply.u_amp = val;
+ if (prop) {
+ icount = of_property_count_u32_elems(opp->np, name);
+ if (icount < 0) {
+ dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
+ name, icount);
+ ret = icount;
+ goto free_microvolt;
+ }
- return 0;
+ if (icount != supplies) {
+ dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
+ __func__, name, icount, supplies);
+ ret = -EINVAL;
+ goto free_microvolt;
+ }
+
+ microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
+ if (!microamp) {
+ ret = -EINVAL;
+ goto free_microvolt;
+ }
+
+ ret = of_property_read_u32_array(opp->np, name, microamp,
+ icount);
+ if (ret) {
+ dev_err(dev, "%s: error parsing %s: %d\n", __func__,
+ name, ret);
+ ret = -EINVAL;
+ goto free_microamp;
+ }
+ }
+
+ for (i = 0, j = 0; i < supplies; i++) {
+ opp->supplies[i].u_volt = microvolt[j++];
+
+ if (vcount == supplies) {
+ opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
+ opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
+ } else {
+ opp->supplies[i].u_volt_min = microvolt[j++];
+ opp->supplies[i].u_volt_max = microvolt[j++];
+ }
+
+ if (microamp)
+ opp->supplies[i].u_amp = microamp[i];
+ }
+
+free_microamp:
+ kfree(microamp);
+free_microvolt:
+ kfree(microvolt);
+
+ return ret;
}
/**
@@ -304,8 +349,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
__func__, new_opp->turbo, new_opp->rate,
- new_opp->supply.u_volt, new_opp->supply.u_volt_min,
- new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
+ new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
+ new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
/*
* Notify the changes in the availability of the operable
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 8a02516542c2..5b0f7e53bede 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -61,7 +61,7 @@ extern struct list_head opp_tables;
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @rate: Frequency in hertz
- * @supply: Power supply voltage/current values
+ * @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
* frequency from any other OPP's frequency.
* @opp_table: points back to the opp_table struct this opp belongs to
@@ -80,7 +80,7 @@ struct dev_pm_opp {
bool suspend;
unsigned long rate;
- struct dev_pm_opp_supply supply;
+ struct dev_pm_opp_supply *supplies;
unsigned long clock_latency_ns;
@@ -139,7 +139,8 @@ enum opp_table_access {
* @supported_hw_count: Number of elements in supported_hw array.
* @prop_name: A name to postfix to many DT properties, while parsing them.
* @clk: Device's clock handle
- * @regulator: Supply regulator
+ * @regulators: Supply regulators
+ * @regulator_count: Number of power supply regulators
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
*
@@ -174,7 +175,8 @@ struct opp_table {
unsigned int supported_hw_count;
const char *prop_name;
struct clk *clk;
- struct regulator *regulator;
+ struct regulator **regulators;
+ unsigned int regulator_count;
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..15cb26118dc7 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
*/
name = find_supply_name(cpu_dev);
if (name) {
- ret = dev_pm_opp_set_regulator(cpu_dev, name);
+ const char *names[] = {name};
+
+ ret = dev_pm_opp_set_regulators(cpu_dev, names,
+ ARRAY_SIZE(names));
if (ret) {
dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
policy->cpu, ret);
@@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
out_free_opp:
dev_pm_opp_of_cpumask_remove_table(policy->cpus);
if (name)
- dev_pm_opp_put_regulator(cpu_dev);
+ dev_pm_opp_put_regulators(cpu_dev);
out_put_clk:
clk_put(cpu_clk);
@@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
if (priv->reg_name)
- dev_pm_opp_put_regulator(priv->cpu_dev);
+ dev_pm_opp_put_regulators(priv->cpu_dev);
clk_put(policy->clk);
kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f69126e2bb59..27eea9bfc5ed 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -78,8 +78,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
void dev_pm_opp_put_supported_hw(struct device *dev);
int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
+void dev_pm_opp_put_regulators(struct device *dev);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -186,12 +186,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count)
{
return -ENOTSUPP;
}
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulators(struct device *dev) {}
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 05/10] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
Pass the entire supply structure instead of all of its fields.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 44 +++++++++++++++++--------------------------
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 8d6006151c9a..37fad2eb0f47 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -542,8 +542,7 @@ static struct clk *_get_opp_clk(struct device *dev)
}
static int _set_opp_voltage(struct device *dev, struct regulator *reg,
- unsigned long u_volt, unsigned long u_volt_min,
- unsigned long u_volt_max)
+ struct dev_pm_opp_supply *supply)
{
int ret;
@@ -554,14 +553,15 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
return 0;
}
- dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, u_volt_min,
- u_volt, u_volt_max);
+ dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+ supply->u_volt_min, supply->u_volt, supply->u_volt_max);
- ret = regulator_set_voltage_triplet(reg, u_volt_min, u_volt,
- u_volt_max);
+ ret = regulator_set_voltage_triplet(reg, supply->u_volt_min,
+ supply->u_volt, supply->u_volt_max);
if (ret)
dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
- __func__, u_volt_min, u_volt, u_volt_max, ret);
+ __func__, supply->u_volt_min, supply->u_volt,
+ supply->u_volt_max, ret);
return ret;
}
@@ -583,8 +583,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
struct regulator *reg;
struct clk *clk;
unsigned long freq, old_freq;
- unsigned long u_volt, u_volt_min, u_volt_max;
- unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
+ struct dev_pm_opp_supply old_supply, new_supply;
int ret;
if (unlikely(!target_freq)) {
@@ -634,17 +633,12 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return ret;
}
- if (IS_ERR(old_opp)) {
- old_u_volt = 0;
- } else {
- old_u_volt = old_opp->supply.u_volt;
- old_u_volt_min = old_opp->supply.u_volt_min;
- old_u_volt_max = old_opp->supply.u_volt_max;
- }
+ if (IS_ERR(old_opp))
+ old_supply.u_volt = 0;
+ else
+ memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
- u_volt = opp->supply.u_volt;
- u_volt_min = opp->supply.u_volt_min;
- u_volt_max = opp->supply.u_volt_max;
+ memcpy(&new_supply, &opp->supply, sizeof(new_supply));
reg = opp_table->regulator;
@@ -652,8 +646,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Scaling up? Scale voltage before frequency */
if (freq > old_freq) {
- ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
- u_volt_max);
+ ret = _set_opp_voltage(dev, reg, &new_supply);
if (ret)
goto restore_voltage;
}
@@ -672,8 +665,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Scaling down? Scale voltage after frequency */
if (freq < old_freq) {
- ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
- u_volt_max);
+ ret = _set_opp_voltage(dev, reg, &new_supply);
if (ret)
goto restore_freq;
}
@@ -686,10 +678,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
__func__, old_freq);
restore_voltage:
/* This shouldn't harm even if the voltages weren't updated earlier */
- if (old_u_volt) {
- _set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
- old_u_volt_max);
- }
+ if (old_supply.u_volt)
+ _set_opp_voltage(dev, reg, &old_supply);
return ret;
}
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 04/10] PM / OPP: Manage supply's voltage/current in a separate structure
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
This is a preparatory step for multiple regulator per device support.
Move the voltage/current variables to a new structure.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 44 +++++++++++++++++++++-------------------
drivers/base/power/opp/debugfs.c | 8 ++++----
drivers/base/power/opp/of.c | 18 ++++++++--------
drivers/base/power/opp/opp.h | 11 +++-------
include/linux/pm_opp.h | 16 +++++++++++++++
5 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 056527a3fb4e..8d6006151c9a 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -112,7 +112,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
if (IS_ERR_OR_NULL(tmp_opp))
pr_err("%s: Invalid parameters\n", __func__);
else
- v = tmp_opp->u_volt;
+ v = tmp_opp->supply.u_volt;
return v;
}
@@ -246,10 +246,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
if (!opp->available)
continue;
- if (opp->u_volt_min < min_uV)
- min_uV = opp->u_volt_min;
- if (opp->u_volt_max > max_uV)
- max_uV = opp->u_volt_max;
+ if (opp->supply.u_volt_min < min_uV)
+ min_uV = opp->supply.u_volt_min;
+ if (opp->supply.u_volt_max > max_uV)
+ max_uV = opp->supply.u_volt_max;
}
rcu_read_unlock();
@@ -637,14 +637,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
if (IS_ERR(old_opp)) {
old_u_volt = 0;
} else {
- old_u_volt = old_opp->u_volt;
- old_u_volt_min = old_opp->u_volt_min;
- old_u_volt_max = old_opp->u_volt_max;
+ old_u_volt = old_opp->supply.u_volt;
+ old_u_volt_min = old_opp->supply.u_volt_min;
+ old_u_volt_max = old_opp->supply.u_volt_max;
}
- u_volt = opp->u_volt;
- u_volt_min = opp->u_volt_min;
- u_volt_max = opp->u_volt_max;
+ u_volt = opp->supply.u_volt;
+ u_volt_min = opp->supply.u_volt_min;
+ u_volt_max = opp->supply.u_volt_max;
reg = opp_table->regulator;
@@ -957,10 +957,11 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
struct regulator *reg = opp_table->regulator;
if (!IS_ERR(reg) &&
- !regulator_is_supported_voltage(reg, opp->u_volt_min,
- opp->u_volt_max)) {
+ !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
+ opp->supply.u_volt_max)) {
pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
- __func__, opp->u_volt_min, opp->u_volt_max);
+ __func__, opp->supply.u_volt_min,
+ opp->supply.u_volt_max);
return false;
}
@@ -993,11 +994,12 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Duplicate OPPs */
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
- __func__, opp->rate, opp->u_volt, opp->available,
- new_opp->rate, new_opp->u_volt, new_opp->available);
+ __func__, opp->rate, opp->supply.u_volt,
+ opp->available, new_opp->rate, new_opp->supply.u_volt,
+ new_opp->available);
- return opp->available && new_opp->u_volt == opp->u_volt ?
- 0 : -EEXIST;
+ return opp->available &&
+ new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
}
new_opp->opp_table = opp_table;
@@ -1064,9 +1066,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
/* populate the opp table */
new_opp->rate = freq;
tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
- new_opp->u_volt = u_volt;
- new_opp->u_volt_min = u_volt - tol;
- new_opp->u_volt_max = u_volt + tol;
+ new_opp->supply.u_volt = u_volt;
+ new_opp->supply.u_volt_min = u_volt - tol;
+ new_opp->supply.u_volt_max = u_volt + tol;
new_opp->available = true;
new_opp->dynamic = dynamic;
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index ef1ae6b52042..c897676ca35f 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -63,16 +63,16 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->u_volt))
+ if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->u_volt_min))
+ if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->u_volt_max))
+ if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
return -ENOMEM;
- if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->u_amp))
+ if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 5b3755e49731..bdf409d42126 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -148,14 +148,14 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
return -EINVAL;
}
- opp->u_volt = microvolt[0];
+ opp->supply.u_volt = microvolt[0];
if (count == 1) {
- opp->u_volt_min = opp->u_volt;
- opp->u_volt_max = opp->u_volt;
+ opp->supply.u_volt_min = opp->supply.u_volt;
+ opp->supply.u_volt_max = opp->supply.u_volt;
} else {
- opp->u_volt_min = microvolt[1];
- opp->u_volt_max = microvolt[2];
+ opp->supply.u_volt_min = microvolt[1];
+ opp->supply.u_volt_max = microvolt[2];
}
/* Search for "opp-microamp-<name>" */
@@ -173,7 +173,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
}
if (prop && !of_property_read_u32(opp->np, name, &val))
- opp->u_amp = val;
+ opp->supply.u_amp = val;
return 0;
}
@@ -303,9 +303,9 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
mutex_unlock(&opp_table_lock);
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
- __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
- new_opp->u_volt_min, new_opp->u_volt_max,
- new_opp->clock_latency_ns);
+ __func__, new_opp->turbo, new_opp->rate,
+ new_opp->supply.u_volt, new_opp->supply.u_volt_min,
+ new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
/*
* Notify the changes in the availability of the operable
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 96cd30ac6c1d..8a02516542c2 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -61,10 +61,7 @@ extern struct list_head opp_tables;
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @rate: Frequency in hertz
- * @u_volt: Target voltage in microvolts corresponding to this OPP
- * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
- * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
- * @u_amp: Maximum current drawn by the device in microamperes
+ * @supply: Power supply voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
* frequency from any other OPP's frequency.
* @opp_table: points back to the opp_table struct this opp belongs to
@@ -83,10 +80,8 @@ struct dev_pm_opp {
bool suspend;
unsigned long rate;
- unsigned long u_volt;
- unsigned long u_volt_min;
- unsigned long u_volt_max;
- unsigned long u_amp;
+ struct dev_pm_opp_supply supply;
+
unsigned long clock_latency_ns;
struct opp_table *opp_table;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..f69126e2bb59 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -24,6 +24,22 @@ enum dev_pm_opp_event {
OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
};
+/**
+ * struct dev_pm_opp_supply - Power supply voltage/current values
+ * @u_volt: Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp: Maximum current drawn by the device in microamperes
+ *
+ * This structure stores the voltage/current values for a single power supply.
+ */
+struct dev_pm_opp_supply {
+ unsigned long u_volt;
+ unsigned long u_volt_min;
+ unsigned long u_volt_max;
+ unsigned long u_amp;
+};
+
#if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 03/10] PM / OPP: Don't use OPP structure outside of rcu protected section
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
The OPP structure must not be used out of the rcu protected section.
Cache the values to be used in separate variables instead.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
drivers/base/power/opp/core.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..056527a3fb4e 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -584,6 +584,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
struct clk *clk;
unsigned long freq, old_freq;
unsigned long u_volt, u_volt_min, u_volt_max;
+ unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
int ret;
if (unlikely(!target_freq)) {
@@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return ret;
}
+ if (IS_ERR(old_opp)) {
+ old_u_volt = 0;
+ } else {
+ old_u_volt = old_opp->u_volt;
+ old_u_volt_min = old_opp->u_volt_min;
+ old_u_volt_max = old_opp->u_volt_max;
+ }
+
u_volt = opp->u_volt;
u_volt_min = opp->u_volt_min;
u_volt_max = opp->u_volt_max;
@@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
__func__, old_freq);
restore_voltage:
/* This shouldn't harm even if the voltages weren't updated earlier */
- if (!IS_ERR(old_opp))
- _set_opp_voltage(dev, reg, old_opp->u_volt,
- old_opp->u_volt_min, old_opp->u_volt_max);
+ if (old_u_volt) {
+ _set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
+ old_u_volt_max);
+ }
return ret;
}
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 02/10] PM / OPP: Reword binding supporting multiple regulators per device
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
On certain platforms (like TI), DVFS for a single device (CPU) requires
configuring multiple power supplies.
The OPP bindings already contains binding and example to explain this
case, but it isn't sufficient.
- There is no way for the code parsing these bindings to know which
voltage values belong to which power supply.
- It is not possible to know the order in which the supplies need to be
configured while switching OPPs.
This patch clarifies on those details by mentioning that such
information is left for the implementation specific bindings to explain.
They may want to hardcode such details or implement their own properties
to get such information. All implementations using multiple regulators
for their devices must provide a binding document explaining their
implementation.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
Documentation/devicetree/bindings/opp/opp.txt | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index f0239f68d186..9f5ca4457b5f 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -86,8 +86,14 @@ properties.
Single entry is for target voltage and three entries are for <target min max>
voltages.
- Entries for multiple regulators must be present in the same order as
- regulators are specified in device's DT node.
+ Entries for multiple regulators shall be provided in the same field separated
+ by angular brackets <>. The OPP binding doesn't provide any provisions to
+ relate the values to their power supplies or the order in which the supplies
+ need to be configured and that is left for the implementation specific
+ binding.
+
+ Entries for all regulators shall be of the same size, i.e. either all use a
+ single value or triplets.
- opp-microvolt-<name>: Named opp-microvolt property. This is exactly similar to
the above opp-microvolt property, but allows multiple voltage ranges to be
@@ -104,10 +110,13 @@ properties.
Should only be set if opp-microvolt is set for the OPP.
- Entries for multiple regulators must be present in the same order as
- regulators are specified in device's DT node. If this property isn't required
- for few regulators, then this should be marked as zero for them. If it isn't
- required for any regulator, then this property need not be present.
+ Entries for multiple regulators shall be provided in the same field separated
+ by angular brackets <>. If current values aren't required for a regulator,
+ then it shall be filled with 0. If current values aren't required for any of
+ the regulators, then this field is not required. The OPP binding doesn't
+ provide any provisions to relate the values to their power supplies or the
+ order in which the supplies need to be configured and that is left for the
+ implementation specific binding.
- opp-microamp-<name>: Named opp-microamp property. Similar to
opp-microvolt-<name> property, but for microamp instead.
--
2.7.1.410.g6faf27b
^ permalink raw reply related
* [PATCH V4 01/10] PM / OPP: Fix incorrect cpu-supply property in binding
From: Viresh Kumar @ 2016-11-24 11:36 UTC (permalink / raw)
To: Rafael Wysocki, nm, sboyd, Viresh Kumar
Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
d-gerlach, broonie, devicetree, Viresh Kumar
In-Reply-To: <cover.1479986491.git.viresh.kumar@linaro.org>
The regulator bindings allow the "<name>-supply" property to define a
single parent supply and not a list of parents.
Fix the wrong example code present in OPP bindings.
While at it also change the compatible string as Rob pointed out earlier
that none of A7 implementation have multiple supplies for the CPU core.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
Documentation/devicetree/bindings/opp/opp.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index ee91cbdd95ee..f0239f68d186 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -386,10 +386,12 @@ Example 4: Handling multiple regulators
/ {
cpus {
cpu@0 {
- compatible = "arm,cortex-a7";
+ compatible = "vendor,cpu-type";
...
- cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
+ vcc0-supply = <&cpu_supply0>;
+ vcc1-supply = <&cpu_supply1>;
+ vcc2-supply = <&cpu_supply2>;
operating-points-v2 = <&cpu0_opp_table>;
};
};
--
2.7.1.410.g6faf27b
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox