* Re: [RFC PATCH 02/10] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.
From: Rob Herring @ 2018-05-22 22:45 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Mark Rutland, David Airlie, devicetree, linux-pm, Stephen Boyd,
Michael Turquette, Derek Basehore, Will Deacon, dri-devel,
linux-kernel, Chanwoo Choi, Kyungmin Park, MyungJoo Ham,
linux-rockchip, kernel, linux-clk, linux-arm-kernel, Lin Huang
In-Reply-To: <20180514211610.26618-3-enric.balletbo@collabora.com>
On Mon, May 14, 2018 at 11:16:02PM +0200, Enric Balletbo i Serra wrote:
> The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
> general register files to know the DRAM type, so add a phandle to the
> syscon that manages these registers.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-05-22 22:46 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=XARtQNo5Cyg78XuT26JUFgn=7BjWRnXPjP566=a-sF1w@mail.gmail.com>
On 05/22/2018 09:43 AM, Doug Anderson wrote:
> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
...
>> Returning the cached (but not sent) initial voltage equal to the min
>> constraint voltage in get_voltage() calls should not cause any problems.
>> This represents the highest voltage that is guaranteed to be output by the
>> regulator. Consumer's should call regulator_set_voltage() to specify
>> their voltage needs. If they simply call regulator_enable(), then the
>> cached voltage will be sent along with the enable request.
>
> I'm still not seeing the argument for initial-voltage here. If we
> added a feature like you describe where we don't send the voltage
> until the first enable couldn't we use the minimum voltage here? If a
> consumer calls regulator_enable() without setting a voltage (which
> seems like a terrible idea for something where the voltage could vary)
> then it would end up at the minimum voltage.
I wasn't proposing the voltage caching feature to be used in the upstream
qcom-rpmh-regulator. I was explaining exactly how the downstream
rpmh-regulator driver works.
However, if the voltage caching feature is acceptable for upstream usage,
then I could add it. With that in place, I see less of a need for the
qcom,regulator-initial-microvolt property and would be ok with removing it
for now.
>>> BTW: have I already said how terrible of a design it is that you can't
>>> read back the voltages that the BIOS set? If we could just read back
>>> what the BIOS set then everything would work great and we could stop
>>> talking about this.
>>
>> Even if such reading were feasible, it would not help the situation
>> substantially. Very few requests are made via the AP RSC before Linux
>> kernel boot, so 0 V values would still be read back for most regulators.
>
> Sure, but all the regulators we're talking about are ones where this
> would help. Said another way: are there any rails that are not
> touched by the bootloader where it's bad to set the minimum voltage?
I'm not sure about this.
> OK, so how's this for a proposal then:
>
> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
> specified that the regulator is enabled then we don't send the
> voltage, we just cache it.
>
> 2. When we see the first enable then we first send the cached voltage
> and then do the enable.
>
> 3. We don't need an "initial voltage" because any rails that are
> expected to be variable voltage the client should be choosing a
> voltage.
>
>
> ...taking the SD card case as an example: if the regulator framework
> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
> because the rail is off as far as Linux is concerned. Then when the
> SD card framework starts up it will set the voltage to 3.3V which will
> overwrite the cache. Then the SD card framework will enable the
> regulator and RPMh will set the voltage to 3.3V and enable.
I am ok with implementing this feature.
However, should the voltage be cached instead of sent immediately any time
that Linux has not explicitly requested the regulator to be enabled, or
only before the first time that an enable request is sent?
1. regulator_set_voltage(reg, 2960000, 2960000)
--> cache voltage=2960 mV
2. regulator_enable(reg)
--> Send voltage=2960 then enable=1
3. regulator_disable(reg)
--> Send enable=0
4. regulator_set_voltage(reg, 1800000, 2960000)
--> A. Send voltage=1800 OR B. cache voltage=1800?
Option A is used on the downstream rpmh-regulator driver in order to avoid
cases where AP votes to disable a regulator that is kept enabled by
another subsystem but then does not lower the voltage requested thanks to
regulator_set_voltage() being called after regulator_disable(). I plan to
go with option A for qcom-rpmh-regulator unless there are objections.
> This whole thing makes me worry about another problem, though. You
> say that the bootloader left the SD card rail on, right? ...but as
> far as Linux is concerned the SD card rail is off (argh, it can't read
> the state that the bootloader left the rail in!)
>
> ...now imagine any of the following:
>
> * The user boots up a kernel where the SD card driver is disabled.
>
> * The user ejects the SD card right after the bootloader used it but
> before the kernel driver started.
>
> When the kernel comes up it will believe that the SD card rail is
> disabled so it won't try to disable it. So the voltage will be left
> on.
We have not encountered issues with regulators getting left on
indefinitely because Linux devices failed to take control of them during
boot. I don't think that this hypothetical issue needs to be addressed in
the first qcom-rpmh-regulator driver patch if at all.
> You can even come up with a less contrived use case here. One could
> argue that the SD card framework really ought to be ensuring VMMC and
> VQMMC are off before it starts tweaking with them just in case the
> bootloader left them on. Thus, it should do:
>
> A) Turn off VMMC and VQMMC
> B) Program VMMC and VQMMC to defaults
> C) Turn on VMMC and VQMMC
>
> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
> off, so step A) will be no-op. Sigh.
Step A) would not work because the regulator's use_count would be 0 and
regulator_disable() can only be called successfully if use_count > 0. The
call would have no impact and it would return an error.
I don't think that this is an avenue that we want to pursue. Consumers
should not be expected to call regulator_disable() before regulator_enable().
> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
> regulator framework understand that we don't know the state? I think
> that might require a pile of patches to the regulator framework, but
> it can't be helped unless we can somehow get RPMh to give us back the
> status of how the bootloader left us (if we had that, we could return
> 0 for anything the bootloader didn't touch and that would be correct
> from the point of view of the AP).
I'm not following what the regulator framework would do as a result of
is_enabled() returning -ENOTRECOVERABLE. If it saw this while processing
a regulator_enable() call then it would continue to enable the regulator.
This value could not be seen while handling a regulator_disable() call
since the is_enabled() callback is not invoked in the disable call-path.
This also seems like an optimization for a problem that we are not
encountering now (or likely to ever encounter). Therefore, I would
suggest that we not try to work this into the initial qcom-rpmh-regulator
patch.
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [RFC PATCH 09/10] arm64: dts: rk3399: Add dfi and dmc nodes.
From: Rob Herring @ 2018-05-22 22:51 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Will Deacon,
Heiko Stuebner, Michael Turquette, Stephen Boyd, Sandy Huang,
David Airlie, linux-pm, linux-kernel, Derek Basehore, linux-clk,
linux-rockchip, dri-devel, Lin Huang, kernel, Sean Paul,
linux-arm-kernel, Nickey Yang, devicetree, Yakir Yang
In-Reply-To: <20180514211610.26618-10-enric.balletbo@collabora.com>
On Mon, May 14, 2018 at 11:16:09PM +0200, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
>
> These are required to support DDR DVFS on rk3399 platform. The patch also
> introduces two new files (rk3399-dram.h and rk3399-dram-default-timing)
> with default DRAM settings.
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> .../rockchip/rk3399-dram-default-timing.dtsi | 38 ++++++++++
> arch/arm64/boot/dts/rockchip/rk3399-dram.h | 73 +++++++++++++++++++
> .../boot/dts/rockchip/rk3399-op1-opp.dtsi | 29 ++++++++
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 20 +++++
> 4 files changed, 160 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram.h
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> new file mode 100644
> index 000000000000..4dfe3e1d8bdf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR X11)
> +/*
> + * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * Author: Lin Huang <hl@rock-chips.com>
> + */
> +
> +#include "rk3399-dram.h"
> +
> +rockchip,ddr3_speed_bin = <21>;
> +rockchip,pd_idle = <0x40>;
> +rockchip,sr_idle = <0x2>;
Don't do includes this way please. These should go under a node.
> +rockchip,sr_mc_gate_idle = <0x3>;
> +rockchip,srpd_lite_idle = <0x4>;
> +rockchip,standby_idle = <0x2000>;
> +rockchip,dram_dll_dis_freq = <300000000>;
> +rockchip,phy_dll_dis_freq = <125000000>;
> +rockchip,auto_pd_dis_freq = <666000000>;
> +rockchip,ddr3_odt_dis_freq = <333000000>;
> +rockchip,ddr3_drv = <DDR3_DS_40ohm>;
> +rockchip,ddr3_odt = <DDR3_ODT_120ohm>;
> +rockchip,phy_ddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr3_odt_dis_freq = <333000000>;
> +rockchip,lpddr3_drv = <LP3_DS_34ohm>;
> +rockchip,lpddr3_odt = <LP3_ODT_240ohm>;
> +rockchip,phy_lpddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr4_odt_dis_freq = <333000000>;
> +rockchip,lpddr4_drv = <LP4_PDDS_60ohm>;
> +rockchip,lpddr4_dq_odt = <LP4_DQ_ODT_40ohm>;
> +rockchip,lpddr4_ca_odt = <LP4_CA_ODT_40ohm>;
> +rockchip,phy_lpddr4_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr4_ck_cs_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_dq_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_odt = <PHY_DRV_ODT_60>;
^ permalink raw reply
* Re: [PATCH 2/2] dmaengine: rcar-dmac: Document R8A77990 bindings
From: Rob Herring @ 2018-05-22 22:58 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, horms, dmaengine, devicetree,
Hiroyuki Yokoyama
In-Reply-To: <1526475979-13891-2-git-send-email-ulrich.hecht+renesas@gmail.com>
On Wed, May 16, 2018 at 03:06:19PM +0200, Ulrich Hecht wrote:
> From: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
>
> Renesas R-Car E3 (R8A77990) SoC also has the R-Car gen2/3 compatible DMA
> controllers, so document the SoC specific binding.
>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
> Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate
From: Ray Jui @ 2018-05-22 23:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, Frank Rowand,
Catalin Marinas, Will Deacon, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list
In-Reply-To: <20180522205457.GA16363@roeck-us.net>
Hi Guenter,
On 5/22/2018 1:54 PM, Guenter Roeck wrote:
> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote:
>> If the watchdog hardware is already enabled during the boot process,
>> when the Linux watchdog driver loads, it should reset the watchdog and
>> tell the watchdog framework. As a result, ping can be generated from
>> the watchdog framework, until the userspace watchdog daemon takes over
>> control
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 1484609..408ffbe 100644
>> --- a/drivers/watchdog/sp805_wdt.c
>> +++ b/drivers/watchdog/sp805_wdt.c
>> @@ -42,6 +42,7 @@
>> /* control register masks */
>> #define INT_ENABLE (1 << 0)
>> #define RESET_ENABLE (1 << 1)
>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE)
>> #define WDTINTCLR 0x00C
>> #define WDTRIS 0x010
>> #define WDTMIS 0x014
>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0);
>> MODULE_PARM_DESC(nowayout,
>> "Set to 1 to keep watchdog running after device release");
>>
>> +/* returns true if wdt is running; otherwise returns false */
>> +static bool wdt_is_running(struct watchdog_device *wdd)
>> +{
>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) ==
>> + ENABLE_MASK)
>> + return true;
>> + else
>> + return false;
>
> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK));
>
Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE);
therefore, a simple !!(expression) would not work? That is, the masked
result needs to be compared against the mask again to ensure both bits
are set, right?
Thanks,
Ray
^ permalink raw reply
* Re: [PATCH v2 07/40] iommu: Add a page fault handler
From: Jacob Pan @ 2018-05-22 23:35 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: bharatku-gjFFaj9aHVfQT0dZR+AlfA, ashok.raj-ral2JQCrhuEAvxtiuMwx3w,
kvm-u79uwXL29TY76Z2rM5mHXA, linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-pci-u79uwXL29TY76Z2rM5mHXA, xuzaibo-hv44wF8Li93QT0dZR+AlfA,
ilias.apalodimas-QSEj5FYQhm4dnm+yROfE0A, will.deacon-5wv7dgnIgG8,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
okaya-sgV2jX0FEOL9JmXXK+q4OQ, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
devicetree-u79uwXL29TY76Z2rM5mHXA, rgummal-gjFFaj9aHVfQT0dZR+AlfA,
rfranz-YGCgFSpz5w/QT0dZR+AlfA, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ,
christian.koenig-5C7GfCeVMHo,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <8a640794-a6f3-fa01-82a9-06479a6f779a-5wv7dgnIgG8@public.gmane.org>
On Mon, 21 May 2018 15:49:40 +0100
Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org> wrote:
> On 18/05/18 19:04, Jacob Pan wrote:
> >> +struct iopf_context {
> >> + struct device *dev;
> >> + struct iommu_fault_event evt;
> >> + struct list_head head;
> >> +};
> >> +
> >> +struct iopf_group {
> >> + struct iopf_context last_fault;
> >> + struct list_head faults;
> >> + struct work_struct work;
> >> +};
> >> +
> > All the page requests in the group should belong to the same device,
> > perhaps struct device tracking should be in iopf_group instead of
> > iopf_context?
>
> Right, this is a leftover from when we kept a global list of partial
> faults. Since the list is now per-device, I can move the dev pointer
> (I think I should also rename iopf_context to iopf_fault, if that's
> all right)
>
> >> +static int iopf_complete(struct device *dev, struct
> >> iommu_fault_event *evt,
> >> + enum page_response_code status)
> >> +{
> >> + struct page_response_msg resp = {
> >> + .addr = evt->addr,
> >> + .pasid = evt->pasid,
> >> + .pasid_present = evt->pasid_valid,
> >> + .page_req_group_id =
> >> evt->page_req_group_id,
> >> + .private_data = evt->iommu_private,
> >> + .resp_code = status,
> >> + };
> >> +
> >> + return iommu_page_response(dev, &resp);
> >> +}
> >> +
> >> +static enum page_response_code
> >> +iopf_handle_single(struct iopf_context *fault)
> >> +{
> >> + /* TODO */
> >> + return -ENODEV;
> >> +}
> >> +
> >> +static void iopf_handle_group(struct work_struct *work)
> >> +{
> >> + struct iopf_group *group;
> >> + struct iopf_context *fault, *next;
> >> + enum page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
> >> +
> >> + group = container_of(work, struct iopf_group, work);
> >> +
> >> + list_for_each_entry_safe(fault, next, &group->faults,
> >> head) {
> >> + struct iommu_fault_event *evt = &fault->evt;
> >> + /*
> >> + * Errors are sticky: don't handle subsequent
> >> faults in the
> >> + * group if there is an error.
> >> + */
> > There might be performance benefit for certain device to continue in
> > spite of error in that the ATS retry may have less fault. Perhaps a
> > policy decision for later enhancement.
>
> Yes I think we should leave it to future work. My current reasoning is
> that subsequent requests are more likely to fail as well and reporting
> the error is more urgent, but we need real workloads to confirm this.
>
> >> + if (status == IOMMU_PAGE_RESP_SUCCESS)
> >> + status = iopf_handle_single(fault);
> >> +
> >> + if (!evt->last_req)
> >> + kfree(fault);
> >> + }
> >> +
> >> + iopf_complete(group->last_fault.dev,
> >> &group->last_fault.evt, status);
> >> + kfree(group);
> >> +}
> >> +
> >> +/**
> >> + * iommu_queue_iopf - IO Page Fault handler
> >> + * @evt: fault event
> >> + * @cookie: struct device, passed to
> >> iommu_register_device_fault_handler.
> >> + *
> >> + * Add a fault to the device workqueue, to be handled by mm.
> >> + */
> >> +int iommu_queue_iopf(struct iommu_fault_event *evt, void *cookie)
> >> +{
> >> + struct iopf_group *group;
> >> + struct iopf_context *fault, *next;
> >> + struct iopf_device_param *iopf_param;
> >> +
> >> + struct device *dev = cookie;
> >> + struct iommu_param *param = dev->iommu_param;
> >> +
> >> + if (WARN_ON(!mutex_is_locked(¶m->lock)))
> >> + return -EINVAL;
> >> +
> >> + if (evt->type != IOMMU_FAULT_PAGE_REQ)
> >> + /* Not a recoverable page fault */
> >> + return IOMMU_PAGE_RESP_CONTINUE;
> >> +
> >> + /*
> >> + * As long as we're holding param->lock, the queue can't
> >> be unlinked
> >> + * from the device and therefore cannot disappear.
> >> + */
> >> + iopf_param = param->iopf_param;
> >> + if (!iopf_param)
> >> + return -ENODEV;
> >> +
> >> + if (!evt->last_req) {
> >> + fault = kzalloc(sizeof(*fault), GFP_KERNEL);
> >> + if (!fault)
> >> + return -ENOMEM;
> >> +
> >> + fault->evt = *evt;
> >> + fault->dev = dev;
> >> +
> >> + /* Non-last request of a group. Postpone until the
> >> last one */
> >> + list_add(&fault->head, &iopf_param->partial);
> >> +
> >> + return IOMMU_PAGE_RESP_HANDLED;
> >> + }
> >> +
> >> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> >> + if (!group)
> >> + return -ENOMEM;
> >> +
> >> + group->last_fault.evt = *evt;
> >> + group->last_fault.dev = dev;
> >> + INIT_LIST_HEAD(&group->faults);
> >> + list_add(&group->last_fault.head, &group->faults);
> >> + INIT_WORK(&group->work, iopf_handle_group);
> >> +
> >> + /* See if we have partial faults for this group */
> >> + list_for_each_entry_safe(fault, next,
> >> &iopf_param->partial, head) {
> >> + if (fault->evt.page_req_group_id ==
> >> evt->page_req_group_id)
> > If all 9 bit group index are used, there can be lots of PRGs. For
> > future enhancement, maybe we can have per group partial list ready
> > to go when LPIG comes in? I will be less searching.
>
> Yes, allocating the PRG from the start could be more efficient. I
> think it's slightly more complicated code so I'd rather see
> performance numbers before implementing it
>
> >> + /* Insert *before* the last fault */
> >> + list_move(&fault->head, &group->faults);
> >> + }
> >> +
> > If you already sorted the group list with last fault at the end,
> > why do you need a separate entry to track the last fault?
>
> Not sure I understand your question, sorry. Do you mean why the
> iopf_group.last_fault? Just to avoid one more kzalloc.
>
kind of :) what i thought was that why can't the last_fault naturally
be the last entry in your group fault list? then there is no need for
special treatment in terms of allocation of the last fault. just my
preference.
> >> +
> >> + queue->flush(queue->flush_arg, dev);
> >> +
> >> + /*
> >> + * No need to clear the partial list. All PRGs containing
> >> the PASID that
> >> + * needs to be decommissioned are whole (the device driver
> >> made sure of
> >> + * it before this function was called). They have been
> >> submitted to the
> >> + * queue by the above flush().
> >> + */
> > So you are saying device driver need to make sure LPIG PRQ is
> > submitted in the flush call above such that partial list is
> > cleared?
>
> Not exactly, it's the IOMMU driver that makes sure all LPIG in its
> queues are submitted by the above flush call. In more details the
> flow is:
>
> * Either device driver calls unbind()/sva_device_shutdown(), or the
> process exits.
> * If the device driver called, then it already told the device to stop
> using the PASID. Otherwise we use the mm_exit() callback to tell the
> device driver to stop using the PASID.
> * In either case, when receiving a stop request from the driver, the
> device sends the LPIGs to the IOMMU queue.
> * Then, the flush call above ensures that the IOMMU reports the LPIG
> with iommu_report_device_fault.
> * While submitting all LPIGs for this PASID to the work queue,
> ipof_queue_fault also picked up all partial faults, so the partial
> list is clean.
>
> Maybe I should improve this comment?
>
thanks for explaining. LPIG submission is done by device asynchronously
w.r.t. driver stopping/decommission PASID. so if we were to use this
flow on vt-d, which does not stall page request queue, then we should
use the iommu model specific flush() callback to ensure LPIG is
received? There could be queue full condition and retry. I am just
trying to understand how and where we can make sure LPIG is
received and all groups are whole.
> > what if
> > there are device failures where device needs to reset (either whole
> > function level or PASID level), should there still be a need to
> > clear the partial list for all associated PASID/group?
>
> I guess the simplest way out, when resetting the device, is for the
> device driver to call iommu_sva_device_shutdown(). Both the IOMMU
> driver's sva_device_shutdown() and remove_device() ops should call
> iopf_queue_remove_device(), which clears the partial list.
>
yes. I think that should work for functional reset.
> Thanks,
> Jean
[Jacob Pan]
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-23 0:08 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <a106c337-0713-e5d6-cb40-13e05f4d361d@codeaurora.org>
Hi,
On Tue, May 22, 2018 at 3:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/22/2018 09:43 AM, Doug Anderson wrote:
>> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
> ...
>>> Returning the cached (but not sent) initial voltage equal to the min
>>> constraint voltage in get_voltage() calls should not cause any problems.
>>> This represents the highest voltage that is guaranteed to be output by the
>>> regulator. Consumer's should call regulator_set_voltage() to specify
>>> their voltage needs. If they simply call regulator_enable(), then the
>>> cached voltage will be sent along with the enable request.
>>
>> I'm still not seeing the argument for initial-voltage here. If we
>> added a feature like you describe where we don't send the voltage
>> until the first enable couldn't we use the minimum voltage here? If a
>> consumer calls regulator_enable() without setting a voltage (which
>> seems like a terrible idea for something where the voltage could vary)
>> then it would end up at the minimum voltage.
>
> I wasn't proposing the voltage caching feature to be used in the upstream
> qcom-rpmh-regulator. I was explaining exactly how the downstream
> rpmh-regulator driver works.
>
> However, if the voltage caching feature is acceptable for upstream usage,
> then I could add it. With that in place, I see less of a need for the
> qcom,regulator-initial-microvolt property and would be ok with removing it
> for now.
I think it's the right thing to do an Mark didn't seem to yell, so I'd
say go for it.
>> OK, so how's this for a proposal then:
>>
>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>> specified that the regulator is enabled then we don't send the
>> voltage, we just cache it.
>>
>> 2. When we see the first enable then we first send the cached voltage
>> and then do the enable.
>>
>> 3. We don't need an "initial voltage" because any rails that are
>> expected to be variable voltage the client should be choosing a
>> voltage.
>>
>>
>> ...taking the SD card case as an example: if the regulator framework
>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>> because the rail is off as far as Linux is concerned. Then when the
>> SD card framework starts up it will set the voltage to 3.3V which will
>> overwrite the cache. Then the SD card framework will enable the
>> regulator and RPMh will set the voltage to 3.3V and enable.
>
> I am ok with implementing this feature.
>
> However, should the voltage be cached instead of sent immediately any time
> that Linux has not explicitly requested the regulator to be enabled, or
> only before the first time that an enable request is sent?
>
> 1. regulator_set_voltage(reg, 2960000, 2960000)
> --> cache voltage=2960 mV
> 2. regulator_enable(reg)
> --> Send voltage=2960 then enable=1
> 3. regulator_disable(reg)
> --> Send enable=0
> 4. regulator_set_voltage(reg, 1800000, 2960000)
> --> A. Send voltage=1800 OR B. cache voltage=1800?
>
> Option A is used on the downstream rpmh-regulator driver in order to avoid
> cases where AP votes to disable a regulator that is kept enabled by
> another subsystem but then does not lower the voltage requested thanks to
> regulator_set_voltage() being called after regulator_disable(). I plan to
> go with option A for qcom-rpmh-regulator unless there are objections.
So one client's vote for a voltage continues to be in effect even if
that client votes to have the regulator disabled? That seems
fundamentally broken in RPMh. I guess my take would be to work around
this RPMh misfeature by saying that whenever Linux votes to disable a
regulator it also votes for a voltage of 0. Then another client of
RPMh won't be affected.
That seems like it would be beneficial in any case. If the AP always
asks for 1.3 V and the modem always asks for 1.1 V for the same rail
then the rail should go down to 1.1 V when the AP says to disable the
rail.
>> This whole thing makes me worry about another problem, though. You
>> say that the bootloader left the SD card rail on, right? ...but as
>> far as Linux is concerned the SD card rail is off (argh, it can't read
>> the state that the bootloader left the rail in!)
>>
>> ...now imagine any of the following:
>>
>> * The user boots up a kernel where the SD card driver is disabled.
>>
>> * The user ejects the SD card right after the bootloader used it but
>> before the kernel driver started.
>>
>> When the kernel comes up it will believe that the SD card rail is
>> disabled so it won't try to disable it. So the voltage will be left
>> on.
>
> We have not encountered issues with regulators getting left on
> indefinitely because Linux devices failed to take control of them during
> boot. I don't think that this hypothetical issue needs to be addressed in
> the first qcom-rpmh-regulator driver patch if at all.
I don't think it hypothetical at all. Linux takes control of a
regulator and then at bootup it disables all regulators that aren't
currently used/enabled. In your case you will report that regulators
are already disabled and thus you'll neuter the kernel's attempt to
disable regulators that nobody is using but that might have been left
on by the bootloader. It seemed like Mark agreed here.
>> You can even come up with a less contrived use case here. One could
>> argue that the SD card framework really ought to be ensuring VMMC and
>> VQMMC are off before it starts tweaking with them just in case the
>> bootloader left them on. Thus, it should do:
>>
>> A) Turn off VMMC and VQMMC
>> B) Program VMMC and VQMMC to defaults
>> C) Turn on VMMC and VQMMC
>>
>> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
>> off, so step A) will be no-op. Sigh.
>
> Step A) would not work because the regulator's use_count would be 0 and
> regulator_disable() can only be called successfully if use_count > 0. The
> call would have no impact and it would return an error.
Are you sure regulator_force_disable() won't do the trick on most
boards (which will report the regulator being enabled at bootup)? I
haven't tried it, but it seems like it might.
...I think you're right that this is a theoretical case at the moment
because I don't think the MMC framework attempts to get this right,
but I don't have time to dig right now. I think it just sets the
voltage to 3.3V and turns the rail on and if the card thinks it should
be at 1.8V because the bootloader left the card in that state then:
whoops. I'd have to walk through the regulator framework more closely
to confirm that though. Certainly on all other boards besides ones
using RPMh the bootloader can leave regulators on and the kernel can
query that state. It seems sane that there would be some way to turn
a regulator in this state off. I know for a fact that if you just
leave the regulator alone (don't claim it or try to enable it) that
Linux will turn it off.
> I don't think that this is an avenue that we want to pursue. Consumers
> should not be expected to call regulator_disable() before regulator_enable().
>
>
>> Do we need to have ".is_enabled" return -ENOTRECOVERABLE to help the
>> regulator framework understand that we don't know the state? I think
>> that might require a pile of patches to the regulator framework, but
>> it can't be helped unless we can somehow get RPMh to give us back the
>> status of how the bootloader left us (if we had that, we could return
>> 0 for anything the bootloader didn't touch and that would be correct
>> from the point of view of the AP).
>
> I'm not following what the regulator framework would do as a result of
> is_enabled() returning -ENOTRECOVERABLE. If it saw this while processing
> a regulator_enable() call then it would continue to enable the regulator.
The important use case here is at bootup when we try to disable all
unused regulators. We need the framework to know that these
regulators might not be disabled so it should go ahead and try to
disable them.
> This value could not be seen while handling a regulator_disable() call
> since the is_enabled() callback is not invoked in the disable call-path.
> This also seems like an optimization for a problem that we are not
> encountering now (or likely to ever encounter). Therefore, I would
> suggest that we not try to work this into the initial qcom-rpmh-regulator
> patch.
I think you haven't thought through the bootup case of disabling all
unused regulators. When you look at that path I think you'll find
it's important to make sure that the regulator framework knows that
you don't know if you're disabled or enabled yet. I think you want to
look at regulator_late_cleanup(). Note that right now
regulator_late_cleanup() doesn't handle error codes from is_enabled
(actually, several places in the regulator core don't), but actually
since the error case and the "enabled" case are probably the same this
might be OK.
-Doug
^ permalink raw reply
* Re: [PATCH v6 5/5] drm/rockchip: support dp training outside dp firmware
From: hl @ 2018-05-23 1:14 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Sean Paul, David Airlie, Chris Zhong, Kishon Vijay Abraham I,
Doug Anderson, Brian Norris, open list:ARM/Rockchip SoC...,
Heiko Stübner, daniel.vetter, jani.nikula, dri-devel,
Linux ARM, linux-kernel, Rob Herring, devicetree@vger.kernel.org
In-Reply-To: <CAFqH_52A6n9fuCFLbDaaAZB1YjyWLMVMViam5BvfYyA75eYr5w@mail.gmail.com>
Hi Enric,
On Wednesday, May 23, 2018 01:06 AM, Enric Balletbo Serra wrote:
> Lin,
>
> 2018-05-22 9:41 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
>> Hi Lin
>>
>> 2018-05-22 3:08 GMT+02:00 hl <hl@rock-chips.com>:
>>> Hi Enric,
>>>
>>>
>>>
>>>
>>> On Monday, May 21, 2018 11:22 PM, Enric Balletbo Serra wrote:
>>>> Hi Lin,
>>>>
>>>> 2018-05-21 11:37 GMT+02:00 Lin Huang <hl@rock-chips.com>:
>>>>> DP firmware uses fixed phy config values to do training, but some
>>>>> boards need to adjust these values to fit for their unique hardware
>>>>> design. So get phy config values from dts and use software link training
>>>>> instead of relying on firmware, if software training fail, keep firmware
>>>>> training as a fallback if sw training fails.
>>>>>
>>>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>>>>> ---
>>>>> Changes in v2:
>>>>> - update patch following Enric suggest
>>>>> Changes in v3:
>>>>> - use variable fw_training instead sw_training_success
>>>>> - base on DP SPCE, if training fail use lower link rate to retry training
>>>>> Changes in v4:
>>>>> - improve cdn_dp_get_lower_link_rate() and cdn_dp_software_train_link()
>>>>> follow Sean suggest
>>>>> Changes in v5:
>>>>> - fix some whitespcae issue
>>>>> Changes in v6:
>>>>> - None
>>>>>
>>>>> drivers/gpu/drm/rockchip/Makefile | 3 +-
>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 24 +-
>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.h | 2 +
>>>>> drivers/gpu/drm/rockchip/cdn-dp-link-training.c | 420
>>>>> ++++++++++++++++++++++++
>>>>> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 31 +-
>>>>> drivers/gpu/drm/rockchip/cdn-dp-reg.h | 38 ++-
>>>>> 6 files changed, 505 insertions(+), 13 deletions(-)
>>>>> create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/Makefile
>>>>> b/drivers/gpu/drm/rockchip/Makefile
>>>>> index a314e21..b932f62 100644
>>>>> --- a/drivers/gpu/drm/rockchip/Makefile
>>>>> +++ b/drivers/gpu/drm/rockchip/Makefile
>>>>> @@ -9,7 +9,8 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>>>>> rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>>>>
>>>>> rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>>>>> -rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
>>>>> +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o \
>>>>> + cdn-dp-link-training.o
>>>>> rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
>>>>> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
>>>>> rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> index cce64c1..d9d0d4d 100644
>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> @@ -629,11 +629,13 @@ static void cdn_dp_encoder_enable(struct
>>>>> drm_encoder *encoder)
>>>>> goto out;
>>>>> }
>>>>> }
>>>>> -
>>>>> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>>>>> - if (ret) {
>>>>> - DRM_DEV_ERROR(dp->dev, "Failed to idle video %d\n", ret);
>>>>> - goto out;
>>>>> + if (dp->use_fw_training == true) {
>>>> You don't need to compare to true. Simply do:
>>>>
>>>> if (dp->use_fw_training)
>>>>
>>>>> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>>>>> + if (ret) {
>>>>> + DRM_DEV_ERROR(dp->dev,
>>>>> + "Failed to idle video %d\n", ret);
>>>>> + goto out;
>>>>> + }
>>>>> }
>>>>>
>>>>> ret = cdn_dp_config_video(dp);
>>>>> @@ -642,11 +644,15 @@ static void cdn_dp_encoder_enable(struct
>>>>> drm_encoder *encoder)
>>>>> goto out;
>>>>> }
>>>>>
>>>>> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>>>>> - if (ret) {
>>>>> - DRM_DEV_ERROR(dp->dev, "Failed to valid video %d\n",
>>>>> ret);
>>>>> - goto out;
>>>>> + if (dp->use_fw_training == true) {
>>>> if (dp->use_fw_training)
>>>>
>>>>> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>>>>> + if (ret) {
>>>>> + DRM_DEV_ERROR(dp->dev,
>>>>> + "Failed to valid video %d\n", ret);
>>>>> + goto out;
>>>>> + }
>>>>> }
>>>>> +
>>>>> out:
>>>>> mutex_unlock(&dp->lock);
>>>>> }
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>>> index 46159b2..77a9793 100644
>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>>>>> @@ -84,6 +84,7 @@ struct cdn_dp_device {
>>>>> bool connected;
>>>>> bool active;
>>>>> bool suspended;
>>>>> + bool use_fw_training;
>>>>>
>>>>> const struct firmware *fw; /* cdn dp firmware */
>>>>> unsigned int fw_version; /* cdn fw version */
>>>>> @@ -106,6 +107,7 @@ struct cdn_dp_device {
>>>>> u8 ports;
>>>>> u8 lanes;
>>>>> int active_port;
>>>>> + u8 train_set[4];
>>>>>
>>>>> u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>>>> bool sink_has_audio;
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>>> new file mode 100644
>>>>> index 0000000..73c3290
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>>> @@ -0,0 +1,420 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>>>>> + * Author: Chris Zhong <zyw@rock-chips.com>
>>>>> + */
> Oh, I just noticed that this is not the preferred format for .c files,
> see the discussion here
>
> https://lkml.org/lkml/2017/11/25/133
So what is the standard， the document:
https://www.kernel.org/doc/html/latest/process/license-rules.html
or Linus's word, do we have official about it?
>
>>>>> +
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/delay.h>
>>>>> +#include <linux/phy/phy.h>
>>>>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>>>> +
>>>>> +#include "cdn-dp-core.h"
>>>>> +#include "cdn-dp-reg.h"
>>>>> +
>>>>> +static void cdn_dp_set_signal_levels(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + struct cdn_dp_port *port = dp->port[dp->active_port];
>>>>> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
>>>>> +
>>>>> + int rate = drm_dp_bw_code_to_link_rate(dp->link.rate);
>>>>> + u8 swing = (dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
>>>>> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
>>>>> + u8 pre_emphasis = (dp->train_set[0] & DP_TRAIN_PRE_EMPHASIS_MASK)
>>>>> + >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
>>>>> +
>>>>> + tcphy->typec_phy_config(port->phy, rate, dp->link.num_lanes,
>>>>> + swing, pre_emphasis);
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_set_pattern(struct cdn_dp_device *dp, uint8_t
>>>>> dp_train_pat)
>>>>> +{
>>>>> + u32 phy_config, global_config;
>>>>> + int ret;
>>>>> + uint8_t pattern = dp_train_pat & DP_TRAINING_PATTERN_MASK;
>>>>> +
>>>>> + global_config = NUM_LANES(dp->link.num_lanes - 1) | SST_MODE |
>>>>> + GLOBAL_EN | RG_EN | ENC_RST_DIS | WR_VHSYNC_FALL;
>>>>> +
>>>>> + phy_config = DP_TX_PHY_ENCODER_BYPASS(0) |
>>>>> + DP_TX_PHY_SKEW_BYPASS(0) |
>>>>> + DP_TX_PHY_DISPARITY_RST(0) |
>>>>> + DP_TX_PHY_LANE0_SKEW(0) |
>>>>> + DP_TX_PHY_LANE1_SKEW(1) |
>>>>> + DP_TX_PHY_LANE2_SKEW(2) |
>>>>> + DP_TX_PHY_LANE3_SKEW(3) |
>>>>> + DP_TX_PHY_10BIT_ENABLE(0);
>>>>> +
>>>>> + if (pattern != DP_TRAINING_PATTERN_DISABLE) {
>>>>> + global_config |= NO_VIDEO;
>>>>> + phy_config |= DP_TX_PHY_TRAINING_ENABLE(1) |
>>>>> + DP_TX_PHY_SCRAMBLER_BYPASS(1) |
>>>>> + DP_TX_PHY_TRAINING_PATTERN(pattern);
>>>>> + }
>>>>> +
>>>>> + ret = cdn_dp_reg_write(dp, DP_FRAMER_GLOBAL_CONFIG,
>>>>> global_config);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("fail to set DP_FRAMER_GLOBAL_CONFIG, error:
>>>>> %d\n",
>>>>> + ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = cdn_dp_reg_write(dp, DP_TX_PHY_CONFIG_REG, phy_config);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("fail to set DP_TX_PHY_CONFIG_REG, error:
>>>>> %d\n",
>>>>> + ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = cdn_dp_reg_write(dp, DPTX_LANE_EN, BIT(dp->link.num_lanes)
>>>>> - 1);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("fail to set DPTX_LANE_EN, error: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
>>>>> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 1);
>>>>> + else
>>>>> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 0);
>>>>> + if (ret)
>>>>> + DRM_ERROR("failed to set DPTX_ENHNCD, error: %x\n", ret);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static u8 cdn_dp_pre_emphasis_max(u8 voltage_swing)
>>>>> +{
>>>>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
>>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>>>>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>>>>> + default:
>>>>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static void cdn_dp_get_adjust_train(struct cdn_dp_device *dp,
>>>>> + uint8_t
>>>>> link_status[DP_LINK_STATUS_SIZE])
>>>>> +{
>>>>> + int i;
>>>>> + uint8_t v = 0, p = 0;
>>>>> + uint8_t preemph_max;
>>>>> +
>>>>> + for (i = 0; i < dp->link.num_lanes; i++) {
>>>>> + v = max(v, drm_dp_get_adjust_request_voltage(link_status,
>>>>> i));
>>>>> + p = max(p,
>>>>> drm_dp_get_adjust_request_pre_emphasis(link_status,
>>>>> + i));
>>>>> + }
>>>>> +
>>>>> + if (v >= VOLTAGE_LEVEL_2)
>>>>> + v = VOLTAGE_LEVEL_2 | DP_TRAIN_MAX_SWING_REACHED;
>>>>> +
>>>>> + preemph_max = cdn_dp_pre_emphasis_max(v);
>>>>> + if (p >= preemph_max)
>>>>> + p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>>>>> +
>>>>> + for (i = 0; i < dp->link.num_lanes; i++)
>>>>> + dp->train_set[i] = v | p;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Pick training pattern for channel equalization. Training Pattern 3
>>>>> for HBR2
>>>>> + * or 1.2 devices that support it, Training Pattern 2 otherwise.
>>>>> + */
>>>>> +static u32 cdn_dp_select_chaneq_pattern(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + u32 training_pattern = DP_TRAINING_PATTERN_2;
>>>>> +
>>>>> + /*
>>>>> + * cdn dp support HBR2 also support TPS3. TPS3 support is also
>>>>> mandatory
>>>>> + * for downstream devices that support HBR2. However, not all
>>>>> sinks
>>>>> + * follow the spec.
>>>>> + */
>>>>> + if (drm_dp_tps3_supported(dp->dpcd))
>>>>> + training_pattern = DP_TRAINING_PATTERN_3;
>>>>> + else
>>>>> + DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3
>>>>> support\n");
>>>>> +
>>>>> + return training_pattern;
>>>>> +}
>>>>> +
>>>>> +
>>>>> +static bool cdn_dp_link_max_vswing_reached(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + int lane;
>>>>> +
>>>>> + for (lane = 0; lane < dp->link.num_lanes; lane++)
>>>>> + if ((dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) ==
>>>>> 0)
>>>>> + return false;
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_update_link_train(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + cdn_dp_set_signal_levels(dp);
>>>>> +
>>>>> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
>>>>> + dp->train_set, dp->link.num_lanes);
>>>>> + if (ret != dp->link.num_lanes)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_set_link_train(struct cdn_dp_device *dp,
>>>>> + uint8_t dp_train_pat)
>>>>> +{
>>>>> + uint8_t buf[sizeof(dp->train_set) + 1];
>>>>> + int ret, len;
>>>>> +
>>>>> + buf[0] = dp_train_pat;
>>>>> + if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
>>>>> + DP_TRAINING_PATTERN_DISABLE) {
>>>>> + /* don't write DP_TRAINING_LANEx_SET on disable */
>>>>> + len = 1;
>>>>> + } else {
>>>>> + /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET
>>>>> */
>>>>> + memcpy(buf + 1, dp->train_set, dp->link.num_lanes);
>>>>> + len = dp->link.num_lanes + 1;
>>>>> + }
>>>>> +
>>>>> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET,
>>>>> + buf, len);
>>>>> + if (ret != len)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_reset_link_train(struct cdn_dp_device *dp,
>>>>> + uint8_t dp_train_pat)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + memset(dp->train_set, 0, sizeof(dp->train_set));
>>>>> +
>>>>> + cdn_dp_set_signal_levels(dp);
>>>>> +
>>>>> + ret = cdn_dp_set_pattern(dp, dp_train_pat);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return cdn_dp_set_link_train(dp, dp_train_pat);
>>>>> +}
>>>>> +
>>>>> +/* Enable corresponding port and start training pattern 1 */
>>>>> +static int cdn_dp_link_training_clock_recovery(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + u8 voltage;
>>>>> + u8 link_status[DP_LINK_STATUS_SIZE];
>>>>> + u32 voltage_tries, max_vswing_tries;
>>>>> + int ret;
>>>>> +
>>>>> + /* clock recovery */
>>>>> + ret = cdn_dp_reset_link_train(dp, DP_TRAINING_PATTERN_1 |
>>>>> + DP_LINK_SCRAMBLING_DISABLE);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("failed to start link train\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + voltage_tries = 1;
>>>>> + max_vswing_tries = 0;
>>>>> + for (;;) {
>>>>> + drm_dp_link_train_clock_recovery_delay(dp->dpcd);
>>>>> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status)
>>>>> !=
>>>>> + DP_LINK_STATUS_SIZE) {
>>>>> + DRM_ERROR("failed to get link status\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (drm_dp_clock_recovery_ok(link_status,
>>>>> dp->link.num_lanes)) {
>>>>> + DRM_DEBUG_KMS("clock recovery OK\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + if (voltage_tries >= 5) {
>>>>> + DRM_DEBUG_KMS("Same voltage tried 5 times\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (max_vswing_tries >= 1) {
>>>>> + DRM_DEBUG_KMS("Max Voltage Swing reached\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + voltage = dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
>>>>> +
>>>>> + /* Update training set as requested by target */
>>>>> + cdn_dp_get_adjust_train(dp, link_status);
>>>>> + if (cdn_dp_update_link_train(dp)) {
>>>>> + DRM_ERROR("failed to update link training\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if ((dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) ==
>>>>> + voltage)
>>>>> + ++voltage_tries;
>>>>> + else
>>>>> + voltage_tries = 1;
>>>>> +
>>>>> + if (cdn_dp_link_max_vswing_reached(dp))
>>>>> + ++max_vswing_tries;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_link_training_channel_equalization(struct
>>>>> cdn_dp_device *dp)
>>>>> +{
>>>>> + int tries, ret;
>>>>> + u32 training_pattern;
>>>>> + uint8_t link_status[DP_LINK_STATUS_SIZE];
>>>>> +
>>>>> + training_pattern = cdn_dp_select_chaneq_pattern(dp);
>>>>> + training_pattern |= DP_LINK_SCRAMBLING_DISABLE;
>>>>> +
>>>>> + ret = cdn_dp_set_pattern(dp, training_pattern);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + ret = cdn_dp_set_link_train(dp, training_pattern);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("failed to start channel equalization\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + for (tries = 0; tries < 5; tries++) {
>>>>> + drm_dp_link_train_channel_eq_delay(dp->dpcd);
>>>>> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status)
>>>>> !=
>>>>> + DP_LINK_STATUS_SIZE) {
>>>>> + DRM_ERROR("failed to get link status\n");
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + /* Make sure clock is still ok */
>>>>> + if (!drm_dp_clock_recovery_ok(link_status,
>>>>> + dp->link.num_lanes)) {
>>>>> + DRM_DEBUG_KMS("Clock recovery check failed\n");
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + if (drm_dp_channel_eq_ok(link_status,
>>>>> dp->link.num_lanes)) {
>>>>> + DRM_DEBUG_KMS("Channel EQ done\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + /* Update training set as requested by target */
>>>>> + cdn_dp_get_adjust_train(dp, link_status);
>>>>> + if (cdn_dp_update_link_train(dp)) {
>>>>> + DRM_ERROR("failed to update link training\n");
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /* Try 5 times, else fail and try at lower BW */
>>>>> + if (tries == 5)
>>>>> + DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
>>>>> +
>>>>> + return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_stop_link_train(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + int ret = cdn_dp_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
>>>>> +
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return cdn_dp_set_link_train(dp, DP_TRAINING_PATTERN_DISABLE);
>>>>> +}
>>>>> +
>>>>> +static int cdn_dp_get_lower_link_rate(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + switch (dp->link.rate) {
>>>>> + case DP_LINK_BW_1_62:
>>>>> + return -EINVAL;
>>>>> + case DP_LINK_BW_2_7:
>>>>> + dp->link.rate = DP_LINK_BW_1_62;
>>>>> + break;
>>>>> + case DP_LINK_BW_5_4:
>>>>> + dp->link.rate = DP_LINK_BW_2_7;
>>>>> + break;
>>>>> + default:
>>>>> + dp->link.rate = DP_LINK_BW_5_4;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp)
>>>>> +{
>>>>> + int ret, stop_err;
>>>>> + u8 link_config[2];
>>>>> + u32 rate, sink_max, source_max;
>>>>> +
>>>>> + ret = drm_dp_dpcd_read(&dp->aux, DP_DPCD_REV, dp->dpcd,
>>>>> + sizeof(dp->dpcd));
>>>>> + if (ret < 0) {
>>>>> + DRM_DEV_ERROR(dp->dev, "Failed to get caps %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + source_max = dp->lanes;
>>>>> + sink_max = drm_dp_max_lane_count(dp->dpcd);
>>>>> + dp->link.num_lanes = min(source_max, sink_max);
>>>>> +
>>>>> + source_max = drm_dp_bw_code_to_link_rate(CDN_DP_MAX_LINK_RATE);
>>>>> + sink_max = drm_dp_max_link_rate(dp->dpcd);
>>>>> + rate = min(source_max, sink_max);
>>>>> + dp->link.rate = drm_dp_link_rate_to_bw_code(rate);
>>>>> +
>>>>> + link_config[0] = 0;
>>>>> + link_config[1] = 0;
>>>>> + if (dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & 0x01)
>>>>> + link_config[1] = DP_SET_ANSI_8B10B;
>>>>> + drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>>>>> +
>>>>> + while (true) {
>>>>> +
>>>>> + /* Write the link configuration data */
>>>>> + link_config[0] = dp->link.rate;
>>>>> + link_config[1] = dp->link.num_lanes;
>>>>> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
>>>>> + link_config[1] |=
>>>>> DP_LANE_COUNT_ENHANCED_FRAME_EN;
>>>>> + drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, link_config,
>>>>> 2);
>>>>> +
>>>>> + ret = cdn_dp_link_training_clock_recovery(dp);
>>>>> + if (ret) {
>>>>> + if (!cdn_dp_get_lower_link_rate(dp))
>>>>> + continue;
>>>>> +
>>>>> + DRM_ERROR("training clock recovery failed: %d\n",
>>>>> ret);
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + ret = cdn_dp_link_training_channel_equalization(dp);
>>>>> + if (ret) {
>>>>> + if (!cdn_dp_get_lower_link_rate(dp))
>>>>> + continue;
>>>>> +
>>>>> + DRM_ERROR("training channel eq failed: %d\n",
>>>>> ret);
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + stop_err = cdn_dp_stop_link_train(dp);
>>>>> + if (stop_err) {
>>>>> + DRM_ERROR("stop training fail, error: %d\n", stop_err);
>>>>> + return stop_err;
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>>> index 979355d..e1273e6 100644
>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>>>>> @@ -17,7 +17,9 @@
>>>>> #include <linux/delay.h>
>>>>> #include <linux/io.h>
>>>>> #include <linux/iopoll.h>
>>>>> +#include <linux/phy/phy.h>
>>>>> #include <linux/reset.h>
>>>>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>>>>
>>>>> #include "cdn-dp-core.h"
>>>>> #include "cdn-dp-reg.h"
>>>>> @@ -189,7 +191,7 @@ static int cdn_dp_mailbox_send(struct cdn_dp_device
>>>>> *dp, u8 module_id,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>>>>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>>>>> {
>>>>> u8 msg[6];
>>>>>
>>>>> @@ -609,6 +611,31 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>>>>> {
>>>>> int ret;
>>>>>
>>>>> + /*
>>>>> + * DP firmware uses fixed phy config values to do training, but
>>>>> some
>>>>> + * boards need to adjust these values to fit for their unique
>>>>> hardware
>>>>> + * design. So if the phy is using custom config values, do
>>>>> software
>>>>> + * link training instead of relying on firmware, if software
>>>>> training
>>>>> + * fail, keep firmware training as a fallback if sw training
>>>>> fails.
>>>>> + */
>>>>> + ret = cdn_dp_software_train_link(dp);
>>>>> + if (ret) {
>>>>> + DRM_DEV_ERROR(dp->dev,
>>>>> + "Failed to do software training %d\n", ret);
>>>>> + goto do_fw_training;
>>>>> + }
>>>> If I understand correctly you are changing current behavior. Before
>>>> this patch, we use always firmware link training, and after this
>>>> patch, we always use software link training. If fails we use the
>>>> firmware link training.
>>>>
>>>> AFAIK my Samsung Chromebook Plus works well with firmware link
>>>> training, so there are any benefits of use software link training
>>>> instead of firmware link training?
>>>>
>>>> Looks to me that we should only do software link training on these
>>>> platforms that need it, so on those that define the phy-config
>>>> property in their DT and use by default firmware link training.
>>> Sean and me have discussed about that, and we all agree to use sw training
>>> instead the
>>> fw training to keep training process consistent, and we do not need add a
>>> varialbe in
>>> struct rockchip_typec_phy(like use_sw_training before) to distinguish sw and
>>> fw training.
>>> If training process implement correctly, sw training not different with fw
>>> training, and as my
>>> test so far, sw training work well on Kevin, ofcourse, we need keep testing
>>> it.
>>>
>> Ok, I can also confirm that software training works well on kevin.
>> Could you mention this change of behavior in the commit message? I
>> think that after these patches the firmware training is unlikely to
>> happen.
>>
>> Thanks,
>> Enric
>>
>>
>>>>> + ret = cdn_dp_reg_write(dp, SOURCE_HDTX_CAR, 0xf);
>>>>> + if (ret) {
>>>>> + DRM_DEV_ERROR(dp->dev,
>>>>> + "Failed to write SOURCE_HDTX_CAR register %d\n", ret);
>>>>> + goto do_fw_training;
>>>>> + }
>>>>> + dp->use_fw_training = false;
>>>>> + return 0;
>>>>> +
>>>>> +do_fw_training:
>>>>> + dp->use_fw_training = true;
>>>>> + DRM_DEV_DEBUG_KMS(dp->dev, "use fw training\n");
>>>>> ret = cdn_dp_training_start(dp);
>>>>> if (ret) {
>>>>> DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n",
>>>>> ret);
>>>>> @@ -623,7 +650,7 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>>>>>
>>>>> DRM_DEV_DEBUG_KMS(dp->dev, "rate:0x%x, lanes:%d\n",
>>>>> dp->link.rate,
>>>>> dp->link.num_lanes);
>>>>> - return ret;
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active)
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>>> index 6580b11..3420771 100644
>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>>>>> @@ -137,7 +137,7 @@
>>>>> #define HPD_EVENT_MASK 0x211c
>>>>> #define HPD_EVENT_DET 0x2120
>>>>>
>>>>> -/* dpyx framer addr */
>>>>> +/* dptx framer addr */
>>>>> #define DP_FRAMER_GLOBAL_CONFIG 0x2200
>>>>> #define DP_SW_RESET 0x2204
>>>>> #define DP_FRAMER_TU 0x2208
>>>>> @@ -431,6 +431,40 @@
>>>>> /* Reference cycles when using lane clock as reference */
>>>>> #define LANE_REF_CYC 0x8000
>>>>>
>>>>> +/* register CM_VID_CTRL */
>>>>> +#define LANE_VID_REF_CYC(x) (((x) & (BIT(24) - 1)) <<
>>>>> 0)
>>>>> +#define NMVID_MEAS_TOLERANCE(x) (((x) & 0xf) <<
>>>>> 24)
>>>>> +
>>>>> +/* register DP_TX_PHY_CONFIG_REG */
>>>>> +#define DP_TX_PHY_TRAINING_ENABLE(x) ((x) & 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_PRBS7 (0 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS1 (1 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS2 (2 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS3 (3 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_TPS4 (4 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_PLTPAT (5 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_D10_2 (6 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_TYPE_HBR2CPAT (8 << 1)
>>>>> +#define DP_TX_PHY_TRAINING_PATTERN(x) ((x) << 1)
>>>>> +#define DP_TX_PHY_SCRAMBLER_BYPASS(x) (((x) & 1) << 5)
>>>>> +#define DP_TX_PHY_ENCODER_BYPASS(x) (((x) & 1) << 6)
>>>>> +#define DP_TX_PHY_SKEW_BYPASS(x) (((x) & 1) << 7)
>>>>> +#define DP_TX_PHY_DISPARITY_RST(x) (((x) & 1) << 8)
>>>>> +#define DP_TX_PHY_LANE0_SKEW(x) (((x) & 7) << 9)
>>>>> +#define DP_TX_PHY_LANE1_SKEW(x) (((x) & 7) << 12)
>>>>> +#define DP_TX_PHY_LANE2_SKEW(x) (((x) & 7) << 15)
>>>>> +#define DP_TX_PHY_LANE3_SKEW(x) (((x) & 7) << 18)
>>>>> +#define DP_TX_PHY_10BIT_ENABLE(x) (((x) & 1) << 21)
>>>>> +
>>>>> +/* register DP_FRAMER_GLOBAL_CONFIG */
>>>>> +#define NUM_LANES(x) ((x) & 3)
>>>>> +#define SST_MODE (0 << 2)
>>>>> +#define RG_EN (0 << 4)
>>>>> +#define GLOBAL_EN BIT(3)
>>>>> +#define NO_VIDEO BIT(5)
>>>>> +#define ENC_RST_DIS BIT(6)
>>>>> +#define WR_VHSYNC_FALL BIT(7)
>>>>> +
>>>>> enum voltage_swing_level {
>>>>> VOLTAGE_LEVEL_0,
>>>>> VOLTAGE_LEVEL_1,
>>>>> @@ -476,6 +510,7 @@ int cdn_dp_set_host_cap(struct cdn_dp_device *dp, u8
>>>>> lanes, bool flip);
>>>>> int cdn_dp_event_config(struct cdn_dp_device *dp);
>>>>> u32 cdn_dp_get_event(struct cdn_dp_device *dp);
>>>>> int cdn_dp_get_hpd_status(struct cdn_dp_device *dp);
>>>>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val);
>>>>> ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr,
>>>>> u8 *data, u16 len);
>>>>> ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr,
>>>>> @@ -489,4 +524,5 @@ int cdn_dp_config_video(struct cdn_dp_device *dp);
>>>>> int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info
>>>>> *audio);
>>>>> int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
>>>>> int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info
>>>>> *audio);
>>>>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp);
>>>>> #endif /* _CDN_DP_REG_H */
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>
>
>
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-05-23 1:19 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=W=BquL5ojORz4HNP889s=-uNJ-QEzUfAxwXj2Oa3cGew@mail.gmail.com>
On 05/22/2018 05:08 PM, Doug Anderson wrote:
> On Tue, May 22, 2018 at 3:46 PM, David Collins <collinsd@codeaurora.org> wrote:
>> On 05/22/2018 09:43 AM, Doug Anderson wrote:
>>> On Mon, May 21, 2018 at 5:00 PM, David Collins <collinsd@codeaurora.org> wrote:
...
>> However, if the voltage caching feature is acceptable for upstream usage,
>> then I could add it. With that in place, I see less of a need for the
>> qcom,regulator-initial-microvolt property and would be ok with removing it
>> for now.
>
> I think it's the right thing to do an Mark didn't seem to yell, so I'd
> say go for it.
I'll remove qcom,regulator-initial-microvolt support and add voltage caching.
>>> OK, so how's this for a proposal then:
>>>
>>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>>> specified that the regulator is enabled then we don't send the
>>> voltage, we just cache it.
>>>
>>> 2. When we see the first enable then we first send the cached voltage
>>> and then do the enable.
>>>
>>> 3. We don't need an "initial voltage" because any rails that are
>>> expected to be variable voltage the client should be choosing a
>>> voltage.
>>>
>>>
>>> ...taking the SD card case as an example: if the regulator framework
>>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>>> because the rail is off as far as Linux is concerned. Then when the
>>> SD card framework starts up it will set the voltage to 3.3V which will
>>> overwrite the cache. Then the SD card framework will enable the
>>> regulator and RPMh will set the voltage to 3.3V and enable.
>>
>> I am ok with implementing this feature.
>>
>> However, should the voltage be cached instead of sent immediately any time
>> that Linux has not explicitly requested the regulator to be enabled, or
>> only before the first time that an enable request is sent?
>>
>> 1. regulator_set_voltage(reg, 2960000, 2960000)
>> --> cache voltage=2960 mV
>> 2. regulator_enable(reg)
>> --> Send voltage=2960 then enable=1
>> 3. regulator_disable(reg)
>> --> Send enable=0
>> 4. regulator_set_voltage(reg, 1800000, 2960000)
>> --> A. Send voltage=1800 OR B. cache voltage=1800?
>>
>> Option A is used on the downstream rpmh-regulator driver in order to avoid
>> cases where AP votes to disable a regulator that is kept enabled by
>> another subsystem but then does not lower the voltage requested thanks to
>> regulator_set_voltage() being called after regulator_disable(). I plan to
>> go with option A for qcom-rpmh-regulator unless there are objections.
>
> So one client's vote for a voltage continues to be in effect even if
> that client votes to have the regulator disabled? That seems
> fundamentally broken in RPMh. I guess my take would be to work around
> this RPMh misfeature by saying that whenever Linux votes to disable a
> regulator it also votes for a voltage of 0. Then another client of
> RPMh won't be affected.
>
> That seems like it would be beneficial in any case. If the AP always
> asks for 1.3 V and the modem always asks for 1.1 V for the same rail
> then the rail should go down to 1.1 V when the AP says to disable the
> rail.
The VRM in RPMh hardware aggregates enable state, output voltage, mode,
and headroom voltage requests independently for each regulator. This
allows for maximum request flexibility and makes no assumptions about
consumer use cases. There is nothing inherently wrong about this approach.
I'm concerned about a blanket policy of setting voltage=0 when issuing
disable requests. That seems to violate the semantics of the
regulator_set_voltage() API which enforces the requested voltage range
until a new range is specified. There may be workarounds that require a
regulator to operate at a specific voltage even when no Linux consumers
explicitly need the regulator enabled.
Given that not masking the voltage on disable is guaranteed to be safe and
does not silently break potential use cases, I plan to stick with this
approach. Therefore, the question about option A or B for voltage caching
is still relevant. I think that option A is safer/more API conforming so
I plan to implement that.
>>> This whole thing makes me worry about another problem, though. You
>>> say that the bootloader left the SD card rail on, right? ...but as
>>> far as Linux is concerned the SD card rail is off (argh, it can't read
>>> the state that the bootloader left the rail in!)
>>>
>>> ...now imagine any of the following:
>>>
>>> * The user boots up a kernel where the SD card driver is disabled.
>>>
>>> * The user ejects the SD card right after the bootloader used it but
>>> before the kernel driver started.
>>>
>>> When the kernel comes up it will believe that the SD card rail is
>>> disabled so it won't try to disable it. So the voltage will be left
>>> on.
>>
>> We have not encountered issues with regulators getting left on
>> indefinitely because Linux devices failed to take control of them during
>> boot. I don't think that this hypothetical issue needs to be addressed in
>> the first qcom-rpmh-regulator driver patch if at all.
>
> I don't think it hypothetical at all. Linux takes control of a
> regulator and then at bootup it disables all regulators that aren't
> currently used/enabled. In your case you will report that regulators
> are already disabled and thus you'll neuter the kernel's attempt to
> disable regulators that nobody is using but that might have been left
> on by the bootloader. It seemed like Mark agreed here.
I did not consider regulator_late_cleanup(). I'll initialize the enabled
value in qcom-rpmh-regulator to -EINVAL to handle this case and also so
that _regulator_enable() succeeds without further core modification.
>>> You can even come up with a less contrived use case here. One could
>>> argue that the SD card framework really ought to be ensuring VMMC and
>>> VQMMC are off before it starts tweaking with them just in case the
>>> bootloader left them on. Thus, it should do:
>>>
>>> A) Turn off VMMC and VQMMC
>>> B) Program VMMC and VQMMC to defaults
>>> C) Turn on VMMC and VQMMC
>>>
>>> ...right now we bootup and pretend to Linux that VMMC and VQMMC start
>>> off, so step A) will be no-op. Sigh.
>>
>> Step A) would not work because the regulator's use_count would be 0 and
>> regulator_disable() can only be called successfully if use_count > 0. The
>> call would have no impact and it would return an error.
>
> Are you sure regulator_force_disable() won't do the trick on most
> boards (which will report the regulator being enabled at bootup)? I
> haven't tried it, but it seems like it might.
regulator_force_disable() would indeed call the disable() callback.
However, this function is designed for emergency uses only and will cause
the use_count to become out of sync with the requested regulator state. I
don't think that we want to suggest to consumers that they call
regulator_force_disable(). It would completely break any shared regulator
uses cases between multiple Linux consumers.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip
From: Levin Du @ 2018-05-23 2:02 UTC (permalink / raw)
To: Rob Herring
Cc: linux-rockchip, Wayne Chou, Heiko Stuebner, devicetree,
Linus Walleij, linux-kernel, linux-gpio, Mark Rutland,
linux-arm-kernel
In-Reply-To: <20180522180238.GA7328@rob-hp-laptop>
On 2018-05-23 2:02 AM, Rob Herring wrote:
> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>> From: Levin Du <djw@t-chip.com.cn>
>>
>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> which do not belong to the general pinctrl.
>>
>> Adding gpio-syscon support makes controlling regulator or
>> LED using these special pins very easy by reusing existing
>> drivers, such as gpio-regulator and led-gpio.
>>
>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>>
>> ---
>>
>> Changes in v2:
>> - Rename gpio_syscon10 to gpio_mute in doc
>>
>> Changes in v1:
>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> - Add doc rockchip,gpio-syscon.txt
>>
>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++
>> drivers/gpio/gpio-syscon.c | 30 ++++++++++++++++
>> 2 files changed, 71 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> new file mode 100644
>> index 0000000..b1b2a67
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> @@ -0,0 +1,41 @@
>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> +
>> +Required properties:
>> +- compatible: Should contain "rockchip,gpio-syscon".
>> +- gpio-controller: Marks the device node as a gpio controller.
>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> + the second cell is used to specify the gpio polarity:
>> + 0 = Active high,
>> + 1 = Active low.
> There's no need for this child node. Just make the parent node a gpio
> controller.
>
> Rob
Hi Rob, it is not clear to me. Do you suggest that the grf node should
be a gpio controller,
like below?
+ grf: syscon at ff100000 {
+ compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
"syscon", "simple-mfd";
+ //...
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <&grf 0x0428 0>;
+ };
or just reserve the following case in the doc?
+ grf: syscon at ff100000 {
+ compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+ //...
+ };
+
+ gpio_mute: gpio-mute {
+ compatible = "rockchip,gpio-syscon";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <&grf 0x0428 0>;
+ };
Thanks
Levin
^ permalink raw reply
* [PATCH v4 0/2] regulator: add QCOM RPMh regulator driver
From: David Collins @ 2018-05-23 2:43 UTC (permalink / raw)
To: broonie, lgirdwood, robh+dt, mark.rutland
Cc: David Collins, linux-arm-msm, linux-arm-kernel, devicetree,
linux-kernel, rnayak, sboyd, dianders
This patch series adds a driver and device tree binding documentation for
PMIC regulator control via Resource Power Manager-hardened (RPMh) on some
Qualcomm Technologies, Inc. SoCs such as SDM845. RPMh is a hardware block
which contains several accelerators which are used to manage various
hardware resources that are shared between the processors of the SoC. The
final hardware state of a regulator is determined within RPMh by performing
max aggregation of the requests made by all of the processors.
The RPMh regulator driver depends upon the RPMh driver [1] and command DB
driver [2] which are both still undergoing review. It also depends upon
three recent regulator changes: [3], [4], and [5].
Changes since v3 [6]:
- Removed support for DT properties qcom,regulator-initial-microvolt
and qcom,headroom-microvolt
- Renamed DT property qcom,allowed-drms-modes to be
qcom,regulator-drms-modes
- Updated DT binding documentation to mention which common regulator
bindings can be used for qcom-rpmh-regulator devices
- Added voltage caching so that voltage requests are only sent to RPMh
after the regulator has been enabled at least once
- Changed 'voltage_selector' default value to be -ENOTRECOVERABLE to
interact with [5]
- Initialized 'enabled' to -EINVAL so that unused regulators are
disabled by regulator_late_cleanup()
- Removed rpmh_regulator_load_default_parameters() as it is no longer
needed
- Updated the mode usage description in qcom,rpmh-regulator.h
Changes since v2 [7]:
- Replaced '_' with '-' in device tree supply property names
- Renamed qcom_rpmh-regulator.c to be qcom-rpmh-regulator.c
- Updated various DT property names to use "microvolt" and "microamp"
- Moved allowed modes constraint specification out of the driver [4]
- Replaced rpmh_client with device pointer to match new RPMh API [1]
- Corrected drms mode threshold checking
- Initialized voltage_selector to -EINVAL when not specified in DT
- Added constants for PMIC regulator hardware modes
- Corrected type sign of mode mapping tables
- Made variable names for mode arrays plural
- Simplified Kconfig depends on
- Removed unnecessary constants and struct fields
- Added some descriptive comments
Changes since v1 [8]:
- Addressed review feedback from Doug, Mark, and Stephen
- Replaced set_voltage()/get_voltage() callbacks with set_voltage_sel()/
get_voltage_sel()
- Added set_bypass()/get_bypass() callbacks for BOB pass-through mode
control
- Removed top-level PMIC data structures
- Removed initialization variables from structs and passed them as
function parameters
- Removed various comments and error messages
- Simplified mode handling
- Refactored per-PMIC rpmh-regulator data specification
- Simplified probe function
- Moved header into DT patch
- Removed redundant property listings from DT binding documentation
[1]: https://lkml.org/lkml/2018/5/9/729
[2]: https://lkml.org/lkml/2018/4/10/714
[3]: https://lkml.org/lkml/2018/4/18/556
[4]: https://lkml.org/lkml/2018/5/11/696
[5]: https://lkml.org/lkml/2018/5/15/1005
[6]: https://lkml.org/lkml/2018/5/11/701
[7]: https://lkml.org/lkml/2018/4/13/687
[8]: https://lkml.org/lkml/2018/3/16/1431
David Collins (2):
regulator: dt-bindings: add QCOM RPMh regulator bindings
regulator: add QCOM RPMh regulator driver
.../bindings/regulator/qcom,rpmh-regulator.txt | 185 +++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-rpmh-regulator.c | 865 +++++++++++++++++++++
.../dt-bindings/regulator/qcom,rpmh-regulator.h | 36 +
5 files changed, 1096 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
create mode 100644 drivers/regulator/qcom-rpmh-regulator.c
create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-05-23 2:43 UTC (permalink / raw)
To: broonie, lgirdwood, robh+dt, mark.rutland
Cc: David Collins, linux-arm-msm, linux-arm-kernel, devicetree,
linux-kernel, rnayak, sboyd, dianders
In-Reply-To: <cover.1527040878.git.collinsd@codeaurora.org>
Introduce bindings for RPMh regulator devices found on some
Qualcomm Technlogies, Inc. SoCs. These devices allow a given
processor within the SoC to make PMIC regulator requests which
are aggregated within the RPMh hardware block along with requests
from other processors in the SoC to determine the final PMIC
regulator hardware state.
Signed-off-by: David Collins <collinsd@codeaurora.org>
---
.../bindings/regulator/qcom,rpmh-regulator.txt | 185 +++++++++++++++++++++
.../dt-bindings/regulator/qcom,rpmh-regulator.h | 36 ++++
2 files changed, 221 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h
diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
new file mode 100644
index 0000000..aaac975
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
@@ -0,0 +1,185 @@
+Qualcomm Technologies, Inc. RPMh Regulators
+
+rpmh-regulator devices support PMIC regulator management via the Voltage
+Regulator Manager (VRM) and Oscillator Buffer (XOB) RPMh accelerators. The APPS
+processor communicates with these hardware blocks via a Resource State
+Coordinator (RSC) using command packets. The VRM allows changing three
+parameters for a given regulator: enable state, output voltage, and operating
+mode. The XOB allows changing only a single parameter for a given regulator:
+its enable state. Despite its name, the XOB is capable of controlling the
+enable state of any PMIC peripheral. It is used for clock buffers, low-voltage
+switches, and LDO/SMPS regulators which have a fixed voltage and mode.
+
+=======================
+Required Node Structure
+=======================
+
+RPMh regulators must be described in two levels of device nodes. The first
+level describes the PMIC containing the regulators and must reside within an
+RPMh device node. The second level describes each regulator within the PMIC
+which is to be used on the board. Each of these regulators maps to a single
+RPMh resource.
+
+The names used for regulator nodes must match those supported by a given PMIC.
+Supported regulator node names:
+ PM8998: smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
+ PMI8998: bob
+ PM8005: smps1 - smps4
+
+========================
+First Level Nodes - PMIC
+========================
+
+- compatible
+ Usage: required
+ Value type: <string>
+ Definition: Must be one of: "qcom,pm8998-rpmh-regulators",
+ "qcom,pmi8998-rpmh-regulators" or
+ "qcom,pm8005-rpmh-regulators".
+
+- qcom,pmic-id
+ Usage: required
+ Value type: <string>
+ Definition: RPMh resource name suffix used for the regulators found on
+ this PMIC. Typical values: "a", "b", "c", "d", "e", "f".
+
+- vdd-s1-supply
+- vdd-s2-supply
+- vdd-s3-supply
+- vdd-s4-supply
+ Usage: optional (PM8998 and PM8005 only)
+ Value type: <phandle>
+ Definition: phandle of the parent supply regulator of one or more of the
+ regulators for this PMIC.
+
+- vdd-s5-supply
+- vdd-s6-supply
+- vdd-s7-supply
+- vdd-s8-supply
+- vdd-s9-supply
+- vdd-s10-supply
+- vdd-s11-supply
+- vdd-s12-supply
+- vdd-s13-supply
+- vdd-l1-l27-supply
+- vdd-l2-l8-l17-supply
+- vdd-l3-l11-supply
+- vdd-l4-l5-supply
+- vdd-l6-supply
+- vdd-l7-l12-l14-l15-supply
+- vdd-l9-supply
+- vdd-l10-l23-l25-supply
+- vdd-l13-l19-l21-supply
+- vdd-l16-l28-supply
+- vdd-l18-l22-supply
+- vdd-l20-l24-supply
+- vdd-l26-supply
+- vin-lvs-1-2-supply
+ Usage: optional (PM8998 only)
+ Value type: <phandle>
+ Definition: phandle of the parent supply regulator of one or more of the
+ regulators for this PMIC.
+
+- vdd-bob-supply
+ Usage: optional (PMI8998 only)
+ Value type: <phandle>
+ Definition: BOB regulator parent supply phandle
+
+===============================
+Second Level Nodes - Regulators
+===============================
+
+- qcom,regulator-drms-modes
+ Usage: required if regulator-allow-set-load is specified;
+ VRM regulators only
+ Value type: <prop-encoded-array>
+ Definition: A list of integers specifying the PMIC regulator modes which
+ can be configured at runtime based upon consumer load needs.
+ Supported values are RPMH_REGULATOR_MODE_* which are defined
+ in [1] (i.e. 0 to 3).
+
+- qcom,drms-mode-max-microamps
+ Usage: required if regulator-allow-set-load is specified;
+ VRM regulators only
+ Value type: <prop-encoded-array>
+ Definition: A list of integers specifying the maximum allowed load
+ current in microamps for each of the modes listed in
+ qcom,regulator-drms-modes (matched 1-to-1 in order).
+ Elements must be specified in order from lowest to highest
+ value.
+
+- qcom,always-wait-for-ack
+ Usage: optional
+ Value type: <empty>
+ Definition: Boolean flag which indicates that the application processor
+ must wait for an ACK or a NACK from RPMh for every request
+ sent for this regulator including those which are for a
+ strictly lower power state.
+
+Other properties defined in Documentation/devicetree/bindings/regulator.txt
+may also be used. regulator-initial-mode and regulator-allowed-modes may be
+specified for VRM regulators using mode values from [1]. regulator-allow-bypass
+may be specified for BOB type regulators managed via VRM.
+regulator-allow-set-load may be specified for VRM regulators. It is used in
+conjunction with qcom,regulator-drms-modes and qcom,drms-mode-max-microamps to
+define a dynamic total load current to mode mapping.
+
+[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
+
+========
+Examples
+========
+
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+&apps_rsc {
+ pm8998-rpmh-regulators {
+ compatible = "qcom,pm8998-rpmh-regulators";
+ qcom,pmic-id = "a";
+
+ vdd-l7-l12-l14-l15-supply = <&pm8998_s5>;
+
+ smps2 {
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ };
+
+ pm8998_s5: smps5 {
+ regulator-min-microvolt = <1904000>;
+ regulator-max-microvolt = <2040000>;
+ };
+
+ ldo7 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_LPM>;
+ regulator-allowed-modes =
+ <RPMH_REGULATOR_MODE_LPM
+ RPMH_REGULATOR_MODE_HPM>;
+ regulator-allow-set-load;
+ qcom,regulator-drms-modes =
+ <RPMH_REGULATOR_MODE_LPM
+ RPMH_REGULATOR_MODE_HPM>;
+ qcom,drms-mode-max-microamps = <10000 1000000>;
+ };
+
+ lvs1 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
+
+ pmi8998-rpmh-regulators {
+ compatible = "qcom,pmi8998-rpmh-regulators";
+ qcom,pmic-id = "b";
+
+ bob {
+ regulator-min-microvolt = <3312000>;
+ regulator-max-microvolt = <3600000>;
+ regulator-allowed-modes =
+ <RPMH_REGULATOR_MODE_AUTO
+ RPMH_REGULATOR_MODE_HPM>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_AUTO>;
+ };
+ };
+};
diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
new file mode 100644
index 0000000..86713dc
--- /dev/null
+++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#ifndef __QCOM_RPMH_REGULATOR_H
+#define __QCOM_RPMH_REGULATOR_H
+
+/*
+ * These mode constants may be used to specify modes for various RPMh regulator
+ * device tree properties (e.g. regulator-initial-mode). Each type of regulator
+ * supports a subset of the possible modes.
+ *
+ * %RPMH_REGULATOR_MODE_RET: Retention mode in which only an extremely small
+ * load current is allowed. This mode is supported
+ * by LDO and SMPS type regulators.
+ * %RPMH_REGULATOR_MODE_LPM: Low power mode in which a small load current is
+ * allowed. This mode corresponds to PFM for SMPS
+ * and BOB type regulators. This mode is supported
+ * by LDO, HFSMPS, BOB, and PMIC4 FTSMPS type
+ * regulators.
+ * %RPMH_REGULATOR_MODE_AUTO: Auto mode in which the regulator hardware
+ * automatically switches between LPM and HPM based
+ * upon the real-time load current. This mode is
+ * supported by HFSMPS, BOB, and PMIC4 FTSMPS type
+ * regulators.
+ * %RPMH_REGULATOR_MODE_HPM: High power mode in which the full rated current
+ * of the regulator is allowed. This mode
+ * corresponds to PWM for SMPS and BOB type
+ * regulators. This mode is supported by all types
+ * of regulators.
+ */
+#define RPMH_REGULATOR_MODE_RET 0
+#define RPMH_REGULATOR_MODE_LPM 1
+#define RPMH_REGULATOR_MODE_AUTO 2
+#define RPMH_REGULATOR_MODE_HPM 3
+
+#endif
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver
From: David Collins @ 2018-05-23 2:43 UTC (permalink / raw)
To: broonie, lgirdwood, robh+dt, mark.rutland
Cc: David Collins, linux-arm-msm, linux-arm-kernel, devicetree,
linux-kernel, rnayak, sboyd, dianders
In-Reply-To: <cover.1527040878.git.collinsd@codeaurora.org>
Add the QCOM RPMh regulator driver to manage PMIC regulators
which are controlled via RPMh on some Qualcomm Technologies, Inc.
SoCs. RPMh is a hardware block which contains several
accelerators which are used to manage various hardware resources
that are shared between the processors of the SoC. The final
hardware state of a regulator is determined within RPMh by
performing max aggregation of the requests made by all of the
processors.
Add support for PMIC regulator control via the voltage regulator
manager (VRM) and oscillator buffer (XOB) RPMh accelerators.
VRM supports manipulation of enable state, voltage, and mode.
XOB supports manipulation of enable state.
Signed-off-by: David Collins <collinsd@codeaurora.org>
---
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-rpmh-regulator.c | 865 ++++++++++++++++++++++++++++++++
3 files changed, 875 insertions(+)
create mode 100644 drivers/regulator/qcom-rpmh-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 4efae3b..1a69bdc 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
Qualcomm RPM as a module. The module will be named
"qcom_rpm-regulator".
+config REGULATOR_QCOM_RPMH
+ tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
+ depends on QCOM_RPMH || COMPILE_TEST
+ help
+ This driver supports control of PMIC regulators via the RPMh hardware
+ block found on Qualcomm Technologies Inc. SoCs. RPMh regulator
+ control allows for voting on regulator state between multiple
+ processors within the SoC.
+
config REGULATOR_QCOM_SMD_RPM
tristate "Qualcomm SMD based RPM regulator driver"
depends on QCOM_SMD_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d81fb02..906f048 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o
obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
new file mode 100644
index 0000000..0217dcc
--- /dev/null
+++ b/drivers/regulator/qcom-rpmh-regulator.c
@@ -0,0 +1,865 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+/**
+ * enum rpmh_regulator_type - supported RPMh accelerator types
+ * %VRM: RPMh VRM accelerator which supports voting on enable, voltage,
+ * and mode of LDO, SMPS, and BOB type PMIC regulators.
+ * %XOB: RPMh XOB accelerator which supports voting on the enable state
+ * of PMIC regulators.
+ */
+enum rpmh_regulator_type {
+ VRM,
+ XOB,
+};
+
+#define RPMH_VRM_HEADROOM_MAX_UV 511000
+
+#define RPMH_REGULATOR_REG_VRM_VOLTAGE 0x0
+#define RPMH_REGULATOR_REG_ENABLE 0x4
+#define RPMH_REGULATOR_REG_VRM_MODE 0x8
+#define RPMH_REGULATOR_REG_VRM_HEADROOM 0xC
+
+#define RPMH_REGULATOR_MODE_COUNT 4
+
+#define PMIC4_LDO_MODE_RETENTION 4
+#define PMIC4_LDO_MODE_LPM 5
+#define PMIC4_LDO_MODE_HPM 7
+
+#define PMIC4_SMPS_MODE_RETENTION 4
+#define PMIC4_SMPS_MODE_PFM 5
+#define PMIC4_SMPS_MODE_AUTO 6
+#define PMIC4_SMPS_MODE_PWM 7
+
+#define PMIC4_BOB_MODE_PASS 0
+#define PMIC4_BOB_MODE_PFM 1
+#define PMIC4_BOB_MODE_AUTO 2
+#define PMIC4_BOB_MODE_PWM 3
+
+/**
+ * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
+ * @regulator_type: RPMh accelerator type used to manage this
+ * regulator
+ * @ops: Pointer to regulator ops callback structure
+ * @voltage_range: The single range of voltages supported by this
+ * PMIC regulator type
+ * @n_voltages: The number of unique voltage set points defined
+ * by voltage_range
+ * @pmic_mode_map: Array indexed by regulator framework mode
+ * containing PMIC hardware modes. Must be large
+ * enough to index all framework modes supported
+ * by this regulator hardware type.
+ * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined
+ * in device tree to a regulator framework mode
+ */
+struct rpmh_vreg_hw_data {
+ enum rpmh_regulator_type regulator_type;
+ const struct regulator_ops *ops;
+ const struct regulator_linear_range voltage_range;
+ int n_voltages;
+ const int *pmic_mode_map;
+ unsigned int (*of_map_mode)(unsigned int mode);
+};
+
+/**
+ * struct rpmh_vreg - individual RPMh regulator data structure encapsulating a
+ * single regulator device
+ * @dev: Device pointer for the top-level PMIC RPMh
+ * regulator parent device. This is used as a
+ * handle in RPMh write requests.
+ * @addr: Base address of the regulator resource within
+ * an RPMh accelerator
+ * @rdesc: Regulator descriptor
+ * @hw_data: PMIC regulator configuration data for this RPMh
+ * regulator
+ * @always_wait_for_ack: Boolean flag indicating if a request must always
+ * wait for an ACK from RPMh before continuing even
+ * if it corresponds to a strictly lower power
+ * state (e.g. enabled --> disabled).
+ * @drms_modes: Array of regulator framework modes which can
+ * be configured dynamically for this regulator
+ * via the set_load() callback.
+ * @drms_mode_max_uAs: Array of maximum load currents in microamps
+ * supported by the corresponding modes in
+ * drms_mode. Elements must be specified in
+ * strictly increasing order.
+ * @drms_mode_count: The number of elements in drms_mode array.
+ * @enabled: Flag indicating if the regulator is enabled or
+ * not
+ * @ever_enabled: Boolean indicating that the regulator has been
+ * explicitly enabled at least once. Voltage
+ * requests should be cached when this flag is not
+ * set.
+ * @bypassed: Boolean indicating if the regulator is in
+ * bypass (pass-through) mode or not. This is
+ * only used by BOB rpmh-regulator resources.
+ * @voltage_selector: Selector used for get_voltage_sel() and
+ * set_voltage_sel() callbacks
+ * @mode: RPMh VRM regulator current framework mode
+ */
+struct rpmh_vreg {
+ struct device *dev;
+ u32 addr;
+ struct regulator_desc rdesc;
+ const struct rpmh_vreg_hw_data *hw_data;
+ bool always_wait_for_ack;
+ unsigned int *drms_modes;
+ int *drms_mode_max_uAs;
+ size_t drms_mode_count;
+
+ int enabled;
+ bool ever_enabled;
+ bool bypassed;
+ int voltage_selector;
+ unsigned int mode;
+};
+
+/**
+ * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
+ * @name: Name for the regulator which also corresponds
+ * to the device tree subnode name of the regulator
+ * @resource_name: RPMh regulator resource name format string.
+ * This must include exactly one field: '%s' which
+ * is filled at run-time with the PMIC ID provided
+ * by device tree property qcom,pmic-id. Example:
+ * "ldo%s1" for RPMh resource "ldoa1".
+ * @supply_name: Parent supply regulator name
+ * @hw_data: Configuration data for this PMIC regulator type
+ */
+struct rpmh_vreg_init_data {
+ const char *name;
+ const char *resource_name;
+ const char *supply_name;
+ const struct rpmh_vreg_hw_data *hw_data;
+};
+
+/**
+ * rpmh_regulator_send_request() - send the request to RPMh
+ * @vreg: Pointer to the RPMh regulator
+ * @cmd: RPMh commands to send
+ * @count: Size of cmd array
+ * @wait_for_ack: Boolean indicating if execution must wait until the
+ * request has been acknowledged as complete
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
+ struct tcs_cmd *cmd, int count, bool wait_for_ack)
+{
+ int ret;
+
+ if (wait_for_ack || vreg->always_wait_for_ack)
+ ret = rpmh_write(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd, count);
+ else
+ ret = rpmh_write_async(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd,
+ count);
+
+ return ret;
+}
+
+static int _rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
+ unsigned int selector, bool wait_for_ack)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
+ };
+ int ret;
+
+ /* VRM voltage control register is set with voltage in millivolts. */
+ cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
+ selector), 1000);
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, wait_for_ack);
+ if (!ret)
+ vreg->voltage_selector = selector;
+
+ return 0;
+}
+
+static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
+ unsigned int selector)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ if (vreg->ever_enabled) {
+ return _rpmh_regulator_vrm_set_voltage_sel(rdev, selector,
+ selector > vreg->voltage_selector);
+ } else {
+ /*
+ * Cache the voltage and send it later when the regulator is
+ * enabled.
+ */
+ vreg->voltage_selector = selector;
+ }
+
+ return 0;
+}
+
+static int rpmh_regulator_vrm_get_voltage_sel(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->voltage_selector;
+}
+
+static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->enabled;
+}
+
+static int rpmh_regulator_enable(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+ .data = 1,
+ };
+ int ret;
+
+ if (!vreg->ever_enabled && vreg->voltage_selector != -ENOTRECOVERABLE) {
+ ret = _rpmh_regulator_vrm_set_voltage_sel(rdev,
+ vreg->voltage_selector, true);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, true);
+ if (!ret) {
+ vreg->enabled = true;
+ vreg->ever_enabled = true;
+ }
+
+ return ret;
+}
+
+static int rpmh_regulator_disable(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_ENABLE,
+ .data = 0,
+ };
+ int ret;
+
+ ret = rpmh_regulator_send_request(vreg, &cmd, 1, false);
+ if (!ret)
+ vreg->enabled = false;
+
+ return ret;
+}
+
+static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
+ unsigned int mode, bool bypassed)
+{
+ struct tcs_cmd cmd = {
+ .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
+ };
+ int pmic_mode;
+
+ if (mode > REGULATOR_MODE_STANDBY)
+ return -EINVAL;
+
+ pmic_mode = vreg->hw_data->pmic_mode_map[mode];
+ if (pmic_mode < 0)
+ return pmic_mode;
+
+ cmd.data = bypassed ? PMIC4_BOB_MODE_PASS : pmic_mode;
+
+ return rpmh_regulator_send_request(vreg, &cmd, 1, true);
+}
+
+static int rpmh_regulator_vrm_set_mode(struct regulator_dev *rdev,
+ unsigned int mode)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int ret;
+
+ if (mode == vreg->mode)
+ return 0;
+
+ ret = rpmh_regulator_vrm_set_mode_bypass(vreg, mode, vreg->bypassed);
+ if (!ret)
+ vreg->mode = mode;
+
+ return ret;
+}
+
+static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ return vreg->mode;
+}
+
+/**
+ * rpmh_regulator_vrm_set_load() - set the regulator mode based upon the load
+ * current requested
+ * @rdev: Regulator device pointer for the rpmh-regulator
+ * @load_uA: Aggregated load current in microamps
+ *
+ * This function is used in the regulator_ops for VRM type RPMh regulator
+ * devices. It updates the mode of the regulator using a table of maximum
+ * load currents per mode specified in device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int i;
+
+ for (i = 0; i < vreg->drms_mode_count - 1; i++)
+ if (vreg->drms_mode_max_uAs[i] >= load_uA)
+ break;
+ if (load_uA > vreg->drms_mode_max_uAs[vreg->drms_mode_count - 1])
+ dev_warn(vreg->dev, "%s: requested load=%d uA greater than max=%d uA\n",
+ vreg->rdesc.name, load_uA,
+ vreg->drms_mode_max_uAs[vreg->drms_mode_count - 1]);
+
+ return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_modes[i]);
+}
+
+static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
+ bool enable)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+ int ret;
+
+ if (vreg->bypassed == enable)
+ return 0;
+
+ ret = rpmh_regulator_vrm_set_mode_bypass(vreg, vreg->mode, enable);
+ if (!ret)
+ vreg->bypassed = enable;
+
+ return ret;
+}
+
+static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
+ bool *enable)
+{
+ struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
+
+ *enable = vreg->bypassed;
+
+ return 0;
+}
+
+static const struct regulator_ops rpmh_regulator_vrm_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+ .set_voltage_sel = rpmh_regulator_vrm_set_voltage_sel,
+ .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_mode = rpmh_regulator_vrm_set_mode,
+ .get_mode = rpmh_regulator_vrm_get_mode,
+ .set_load = rpmh_regulator_vrm_set_load,
+};
+
+static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+ .set_voltage_sel = rpmh_regulator_vrm_set_voltage_sel,
+ .get_voltage_sel = rpmh_regulator_vrm_get_voltage_sel,
+ .list_voltage = regulator_list_voltage_linear_range,
+ .set_mode = rpmh_regulator_vrm_set_mode,
+ .get_mode = rpmh_regulator_vrm_get_mode,
+ .set_load = rpmh_regulator_vrm_set_load,
+ .set_bypass = rpmh_regulator_vrm_set_bypass,
+ .get_bypass = rpmh_regulator_vrm_get_bypass,
+};
+
+static const struct regulator_ops rpmh_regulator_xob_ops = {
+ .enable = rpmh_regulator_enable,
+ .disable = rpmh_regulator_disable,
+ .is_enabled = rpmh_regulator_is_enabled,
+};
+
+/**
+ * rpmh_regulator_parse_vrm_modes() - parse the supported mode configurations
+ * for a VRM RPMh resource from device tree
+ * vreg: Pointer to the individual rpmh-regulator resource
+ * dev: Pointer to the top level rpmh-regulator PMIC device
+ * node: Pointer to the individual rpmh-regulator resource
+ * device node
+ *
+ * This function initializes the drms_modes[] and drms_mode_max_uAs[] arrays of
+ * vreg based upon the values of optional device tree properties.
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
+ struct device *dev, struct device_node *node)
+{
+ const char *prop;
+ int i, len, ret, mode;
+ u32 *buf;
+
+ /* qcom,regulator-drms-modes is optional */
+ prop = "qcom,regulator-drms-modes";
+ len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+ if (len < 0)
+ return 0;
+
+ vreg->drms_modes = devm_kcalloc(dev, len, sizeof(*vreg->drms_modes),
+ GFP_KERNEL);
+ vreg->drms_mode_max_uAs = devm_kcalloc(dev, len,
+ sizeof(*vreg->drms_mode_max_uAs),
+ GFP_KERNEL);
+ if (!vreg->drms_modes || !vreg->drms_mode_max_uAs)
+ return -ENOMEM;
+ vreg->drms_mode_count = len;
+
+ buf = kcalloc(len, sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(node, prop, buf, len);
+ if (ret < 0) {
+ dev_err(dev, "%s: unable to read %s, ret=%d\n",
+ node->name, prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ mode = vreg->hw_data->of_map_mode(buf[i]);
+ if (mode == REGULATOR_MODE_INVALID) {
+ dev_err(dev, "%s: element %d of %s = %u is invalid for this regulator\n",
+ node->name, i, prop, buf[i]);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ vreg->drms_modes[i] = mode;
+ }
+
+ prop = "qcom,drms-mode-max-microamps";
+ len = of_property_count_elems_of_size(node, prop, sizeof(u32));
+ if (len != vreg->drms_mode_count) {
+ dev_err(dev, "%s: invalid element count=%d for %s\n",
+ node->name, len, prop);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ ret = of_property_read_u32_array(node, prop, buf, len);
+ if (ret < 0) {
+ dev_err(dev, "%s: unable to read %s, ret=%d\n",
+ node->name, prop, ret);
+ goto done;
+ }
+
+ for (i = 0; i < len; i++) {
+ vreg->drms_mode_max_uAs[i] = buf[i];
+
+ if (i > 0 && vreg->drms_mode_max_uAs[i]
+ <= vreg->drms_mode_max_uAs[i - 1]) {
+ dev_err(dev, "%s: %s elements are not in ascending order\n",
+ node->name, prop);
+ ret = -EINVAL;
+ goto done;
+ }
+ }
+
+done:
+ kfree(buf);
+ return ret;
+}
+
+/**
+ * rpmh_regulator_init_vreg() - initialize all attributes of an rpmh-regulator
+ * vreg: Pointer to the individual rpmh-regulator resource
+ * dev: Pointer to the top level rpmh-regulator PMIC device
+ * node: Pointer to the individual rpmh-regulator resource
+ * device node
+ * pmic_id: String used to identify the top level rpmh-regulator
+ * PMIC device on the board
+ * rpmh_data: Pointer to a null-terminated array of rpmh-regulator
+ * resources defined for the top level PMIC device
+ *
+ * Return: 0 on success, errno on failure
+ */
+static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev,
+ struct device_node *node, const char *pmic_id,
+ const struct rpmh_vreg_init_data *rpmh_data)
+{
+ struct regulator_config reg_config = {};
+ char rpmh_resource_name[20] = "";
+ struct regulator_dev *rdev;
+ enum rpmh_regulator_type type;
+ struct regulator_init_data *init_data;
+ int ret;
+
+ vreg->dev = dev;
+
+ for (; rpmh_data->name; rpmh_data++)
+ if (!strcmp(rpmh_data->name, node->name))
+ break;
+
+ if (!rpmh_data->name) {
+ dev_err(dev, "Unknown regulator %s\n", node->name);
+ return -EINVAL;
+ }
+
+ scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name),
+ rpmh_data->resource_name, pmic_id);
+
+ vreg->addr = cmd_db_read_addr(rpmh_resource_name);
+ if (!vreg->addr) {
+ dev_err(dev, "%s: could not find RPMh address for resource %s\n",
+ node->name, rpmh_resource_name);
+ return -ENODEV;
+ }
+
+ vreg->rdesc.name = rpmh_data->name;
+ vreg->rdesc.supply_name = rpmh_data->supply_name;
+ vreg->hw_data = rpmh_data->hw_data;
+
+ vreg->enabled = -EINVAL;
+ vreg->voltage_selector = -ENOTRECOVERABLE;
+ vreg->mode = REGULATOR_MODE_INVALID;
+
+ if (rpmh_data->hw_data->n_voltages) {
+ vreg->rdesc.linear_ranges = &rpmh_data->hw_data->voltage_range;
+ vreg->rdesc.n_linear_ranges = 1;
+ vreg->rdesc.n_voltages = rpmh_data->hw_data->n_voltages;
+ }
+
+ type = rpmh_data->hw_data->regulator_type;
+ if (type == VRM) {
+ ret = rpmh_regulator_parse_vrm_modes(vreg, dev, node);
+ if (ret < 0)
+ return ret;
+ }
+
+ vreg->always_wait_for_ack = of_property_read_bool(node,
+ "qcom,always-wait-for-ack");
+
+ vreg->rdesc.owner = THIS_MODULE;
+ vreg->rdesc.type = REGULATOR_VOLTAGE;
+ vreg->rdesc.ops = vreg->hw_data->ops;
+ vreg->rdesc.of_map_mode = vreg->hw_data->of_map_mode;
+
+ init_data = of_get_regulator_init_data(dev, node, &vreg->rdesc);
+ if (!init_data)
+ return -ENOMEM;
+
+ if (type == XOB && init_data->constraints.min_uV &&
+ init_data->constraints.min_uV == init_data->constraints.max_uV) {
+ vreg->rdesc.fixed_uV = init_data->constraints.min_uV;
+ vreg->rdesc.n_voltages = 1;
+ }
+
+ reg_config.dev = dev;
+ reg_config.init_data = init_data;
+ reg_config.of_node = node;
+ reg_config.driver_data = vreg;
+
+ rdev = devm_regulator_register(dev, &vreg->rdesc, ®_config);
+ if (IS_ERR(rdev)) {
+ ret = PTR_ERR(rdev);
+ rdev = NULL;
+ dev_err(dev, "%s: devm_regulator_register() failed, ret=%d\n",
+ node->name, ret);
+ return ret;
+ }
+
+ dev_dbg(dev, "%s regulator registered for RPMh resource %s @ 0x%05X\n",
+ node->name, rpmh_resource_name, vreg->addr);
+
+ return ret;
+}
+
+static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
+ [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM,
+ [REGULATOR_MODE_NORMAL] = -EINVAL,
+ [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM,
+};
+
+static unsigned int rpmh_regulator_pmic4_ldo_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
+ [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
+ [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_INVALID,
+ [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const int pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
+ [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM,
+ [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO,
+ [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM,
+};
+
+static unsigned int rpmh_regulator_pmic4_smps_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_STANDBY,
+ [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
+ [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
+ [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const int pmic_mode_map_pmic4_bob[REGULATOR_MODE_STANDBY + 1] = {
+ [REGULATOR_MODE_INVALID] = -EINVAL,
+ [REGULATOR_MODE_STANDBY] = -EINVAL,
+ [REGULATOR_MODE_IDLE] = PMIC4_BOB_MODE_PFM,
+ [REGULATOR_MODE_NORMAL] = PMIC4_BOB_MODE_AUTO,
+ [REGULATOR_MODE_FAST] = PMIC4_BOB_MODE_PWM,
+};
+
+static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode)
+{
+ static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = {
+ [RPMH_REGULATOR_MODE_RET] = REGULATOR_MODE_INVALID,
+ [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE,
+ [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
+ [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST,
+ };
+
+ if (mode >= RPMH_REGULATOR_MODE_COUNT)
+ return -EINVAL;
+
+ return of_mode_map[mode];
+}
+
+static const struct rpmh_vreg_hw_data pmic4_pldo = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1664000, 0, 255, 8000),
+ .n_voltages = 256,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_pldo_lv = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1256000, 0, 127, 8000),
+ .n_voltages = 128,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_nldo = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
+ .n_voltages = 128,
+ .pmic_mode_map = pmic_mode_map_pmic4_ldo,
+ .of_map_mode = rpmh_regulator_pmic4_ldo_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_hfsmps3 = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
+ .n_voltages = 216,
+ .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_ftsmps426 = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(320000, 0, 258, 4000),
+ .n_voltages = 259,
+ .pmic_mode_map = pmic_mode_map_pmic4_smps,
+ .of_map_mode = rpmh_regulator_pmic4_smps_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_bob = {
+ .regulator_type = VRM,
+ .ops = &rpmh_regulator_vrm_bypass_ops,
+ .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000),
+ .n_voltages = 84,
+ .pmic_mode_map = pmic_mode_map_pmic4_bob,
+ .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode,
+};
+
+static const struct rpmh_vreg_hw_data pmic4_lvs = {
+ .regulator_type = XOB,
+ .ops = &rpmh_regulator_xob_ops,
+ /* LVS hardware does not support voltage or mode configuration. */
+};
+
+#define RPMH_VREG(_name, _resource_name, _hw_data, _supply_name) \
+{ \
+ .name = _name, \
+ .resource_name = _resource_name, \
+ .hw_data = _hw_data, \
+ .supply_name = _supply_name, \
+}
+
+static const struct rpmh_vreg_init_data pm8998_vreg_data[] = {
+ RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd-s1"),
+ RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd-s2"),
+ RPMH_VREG("smps3", "smp%s3", &pmic4_hfsmps3, "vdd-s3"),
+ RPMH_VREG("smps4", "smp%s4", &pmic4_hfsmps3, "vdd-s4"),
+ RPMH_VREG("smps5", "smp%s5", &pmic4_hfsmps3, "vdd-s5"),
+ RPMH_VREG("smps6", "smp%s6", &pmic4_ftsmps426, "vdd-s6"),
+ RPMH_VREG("smps7", "smp%s7", &pmic4_ftsmps426, "vdd-s7"),
+ RPMH_VREG("smps8", "smp%s8", &pmic4_ftsmps426, "vdd-s8"),
+ RPMH_VREG("smps9", "smp%s9", &pmic4_ftsmps426, "vdd-s9"),
+ RPMH_VREG("smps10", "smp%s10", &pmic4_ftsmps426, "vdd-s10"),
+ RPMH_VREG("smps11", "smp%s11", &pmic4_ftsmps426, "vdd-s11"),
+ RPMH_VREG("smps12", "smp%s12", &pmic4_ftsmps426, "vdd-s12"),
+ RPMH_VREG("smps13", "smp%s13", &pmic4_ftsmps426, "vdd-s13"),
+ RPMH_VREG("ldo1", "ldo%s1", &pmic4_nldo, "vdd-l1-l27"),
+ RPMH_VREG("ldo2", "ldo%s2", &pmic4_nldo, "vdd-l2-l8-l17"),
+ RPMH_VREG("ldo3", "ldo%s3", &pmic4_nldo, "vdd-l3-l11"),
+ RPMH_VREG("ldo4", "ldo%s4", &pmic4_nldo, "vdd-l4-l5"),
+ RPMH_VREG("ldo5", "ldo%s5", &pmic4_nldo, "vdd-l4-l5"),
+ RPMH_VREG("ldo6", "ldo%s6", &pmic4_pldo, "vdd-l6"),
+ RPMH_VREG("ldo7", "ldo%s7", &pmic4_pldo_lv, "vdd-l7-l12-l14-l15"),
+ RPMH_VREG("ldo8", "ldo%s8", &pmic4_nldo, "vdd-l2-l8-l17"),
+ RPMH_VREG("ldo9", "ldo%s9", &pmic4_pldo, "vdd-l9"),
+ RPMH_VREG("ldo10", "ldo%s10", &pmic4_pldo, "vdd-l10-l23-l25"),
+ RPMH_VREG("ldo11", "ldo%s11", &pmic4_nldo, "vdd-l3-l11"),
+ RPMH_VREG("ldo12", "ldo%s12", &pmic4_pldo_lv, "vdd-l7-l12-l14-l15"),
+ RPMH_VREG("ldo13", "ldo%s13", &pmic4_pldo, "vdd-l13-l19-l21"),
+ RPMH_VREG("ldo14", "ldo%s14", &pmic4_pldo_lv, "vdd-l7-l12-l14-l15"),
+ RPMH_VREG("ldo15", "ldo%s15", &pmic4_pldo_lv, "vdd-l7-l12-l14-l15"),
+ RPMH_VREG("ldo16", "ldo%s16", &pmic4_pldo, "vdd-l16-l28"),
+ RPMH_VREG("ldo17", "ldo%s17", &pmic4_nldo, "vdd-l2-l8-l17"),
+ RPMH_VREG("ldo18", "ldo%s18", &pmic4_pldo, "vdd-l18-l22"),
+ RPMH_VREG("ldo19", "ldo%s19", &pmic4_pldo, "vdd-l13-l19-l21"),
+ RPMH_VREG("ldo20", "ldo%s20", &pmic4_pldo, "vdd-l20-l24"),
+ RPMH_VREG("ldo21", "ldo%s21", &pmic4_pldo, "vdd-l13-l19-l21"),
+ RPMH_VREG("ldo22", "ldo%s22", &pmic4_pldo, "vdd-l18-l22"),
+ RPMH_VREG("ldo23", "ldo%s23", &pmic4_pldo, "vdd-l10-l23-l25"),
+ RPMH_VREG("ldo24", "ldo%s24", &pmic4_pldo, "vdd-l20-l24"),
+ RPMH_VREG("ldo25", "ldo%s25", &pmic4_pldo, "vdd-l10-l23-l25"),
+ RPMH_VREG("ldo26", "ldo%s26", &pmic4_nldo, "vdd-l26"),
+ RPMH_VREG("ldo27", "ldo%s27", &pmic4_nldo, "vdd-l1-l27"),
+ RPMH_VREG("ldo28", "ldo%s28", &pmic4_pldo, "vdd-l16-l28"),
+ RPMH_VREG("lvs1", "vs%s1", &pmic4_lvs, "vin-lvs-1-2"),
+ RPMH_VREG("lvs2", "vs%s2", &pmic4_lvs, "vin-lvs-1-2"),
+ {},
+};
+
+static const struct rpmh_vreg_init_data pmi8998_vreg_data[] = {
+ RPMH_VREG("bob", "bob%s1", &pmic4_bob, "vdd-bob"),
+ {},
+};
+
+static const struct rpmh_vreg_init_data pm8005_vreg_data[] = {
+ RPMH_VREG("smps1", "smp%s1", &pmic4_ftsmps426, "vdd-s1"),
+ RPMH_VREG("smps2", "smp%s2", &pmic4_ftsmps426, "vdd-s2"),
+ RPMH_VREG("smps3", "smp%s3", &pmic4_ftsmps426, "vdd-s3"),
+ RPMH_VREG("smps4", "smp%s4", &pmic4_ftsmps426, "vdd-s4"),
+ {},
+};
+
+static int rpmh_regulator_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const struct rpmh_vreg_init_data *vreg_data;
+ struct device_node *node;
+ struct rpmh_vreg *vreg;
+ const char *pmic_id;
+ int ret;
+
+ ret = cmd_db_ready();
+ if (ret < 0) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Command DB not available, ret=%d\n", ret);
+ return ret;
+ }
+
+ vreg_data = of_device_get_match_data(dev);
+ if (!vreg_data)
+ return -ENODEV;
+
+ ret = of_property_read_string(dev->of_node, "qcom,pmic-id", &pmic_id);
+ if (ret < 0) {
+ dev_err(dev, "qcom,pmic-id missing in DT node\n");
+ return ret;
+ }
+
+ for_each_available_child_of_node(dev->of_node, node) {
+ vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+ if (!vreg) {
+ of_node_put(node);
+ return -ENOMEM;
+ }
+
+ ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
+ vreg_data);
+ if (ret < 0) {
+ of_node_put(node);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static const struct of_device_id rpmh_regulator_match_table[] = {
+ {
+ .compatible = "qcom,pm8998-rpmh-regulators",
+ .data = pm8998_vreg_data,
+ },
+ {
+ .compatible = "qcom,pmi8998-rpmh-regulators",
+ .data = pmi8998_vreg_data,
+ },
+ {
+ .compatible = "qcom,pm8005-rpmh-regulators",
+ .data = pm8005_vreg_data,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, rpmh_regulator_match_table);
+
+static struct platform_driver rpmh_regulator_driver = {
+ .driver = {
+ .name = "qcom-rpmh-regulator",
+ .of_match_table = of_match_ptr(rpmh_regulator_match_table),
+ },
+ .probe = rpmh_regulator_probe,
+};
+module_platform_driver(rpmh_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm RPMh regulator driver");
+MODULE_LICENSE("GPL v2");
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH v5 0/3] Support for Qualcomm UFS QMP PHY on SDM845
From: Can Guo @ 2018-05-23 3:47 UTC (permalink / raw)
To: subhashj, asutoshd, vivek.gautam, mgautam, kishon, robh+dt,
mark.rutland
Cc: linux-kernel, devicetree, Can Guo
This patch series adds support for UFS QMP PHY on SDM845 and the
compatible string for it. This patch series depends on the current
proposed QMP V3 USB3 UNI PHY support for sdm845 driver [1], on
the DT bindings for the QMP V3 USB3 PHYs based dirver [2], and also
rebased on updated pipe_clk initialization sequence [3]. This series
can only be merged once the dependent patches do.
[1] http://lists-archives.com/linux-kernel/29071659-dt-bindings-phy-qcom-qmp-update-bindings-for-sdm845.html
[2] http://lists-archives.com/linux-kernel/29071660-phy-qcom-qmp-add-qmp-v3-usb3-uni-phy-support-for-sdm845.html
[3] https://patchwork.kernel.org/patch/10376551/
Changes since v4:
- Adds 'ref_aux' clock back to SDM845 UFS PHY clock list.
- Power on PHY before serdes configuration starts.
- Updates the UFS PHY initialization sequence.
- Updates a few UFS PHY registers.
- Incorporated review comments from Vivek and Manu.
Changes since v3:
- Incorporated review comments from Vivek and Rob.
Changes since v2:
- Incorporated review comments from Vivek and Rob.
- Remove "ref_aux" from sdm845 ufs phy clock list structure.
Changes since v1:
- Incorporated review comments from Vivek and Manu.
- Update the commit title of patch 2.
Can Guo (3):
phy: Power on PHY before start Serdes configuration
phy: Add QMP phy based UFS phy support for sdm845
dt-bindings: phy-qcom-qmp: Add UFS phy compatible string for sdm845
.../devicetree/bindings/phy/qcom-qmp-phy.txt | 4 +-
drivers/phy/qualcomm/phy-qcom-qmp.c | 209 +++++++++++++++++++--
drivers/phy/qualcomm/phy-qcom-qmp.h | 15 ++
3 files changed, 215 insertions(+), 13 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH v5 1/3] phy: Power on PHY before start Serdes configuration
From: Can Guo @ 2018-05-23 3:47 UTC (permalink / raw)
To: subhashj, asutoshd, vivek.gautam, mgautam, kishon, robh+dt,
mark.rutland
Cc: linux-kernel, devicetree, Can Guo
In-Reply-To: <20180523034712.3420-1-cang@codeaurora.org>
PHYs should be powered on before register configuration starts.
Signed-off-by: Can Guo <cang@codeaurora.org>
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 97ef942..9bfdba1 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1000,6 +1000,12 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
}
+ /*
+ * Pull out PHY from POWER DOWN state.
+ * This is active low enable signal to power-down PHY.
+ */
+ qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+
/* Serdes configuration */
qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl,
cfg->serdes_tbl_num);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH v5 2/3] phy: Add QMP phy based UFS phy support for sdm845
From: Can Guo @ 2018-05-23 3:47 UTC (permalink / raw)
To: subhashj, asutoshd, vivek.gautam, mgautam, kishon, robh+dt,
mark.rutland
Cc: linux-kernel, devicetree, Can Guo
In-Reply-To: <20180523034712.3420-1-cang@codeaurora.org>
Add UFS PHY support to make SDM845 UFS work with common PHY framework.
Signed-off-by: Can Guo <cang@codeaurora.org>
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 203 +++++++++++++++++++++++++++++++++---
drivers/phy/qualcomm/phy-qcom-qmp.h | 15 +++
2 files changed, 206 insertions(+), 12 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 9bfdba1..229078d 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -156,6 +156,11 @@ enum qphy_reg_layout {
[QPHY_PCS_LFPS_RXTERM_IRQ_STATUS] = 0x170,
};
+static const unsigned int sdm845_ufsphy_regs_layout[] = {
+ [QPHY_START_CTRL] = 0x00,
+ [QPHY_PCS_READY_STATUS] = 0x160,
+};
+
static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x1c),
QMP_PHY_INIT_CFG(QSERDES_COM_CLK_ENABLE1, 0x10),
@@ -601,6 +606,83 @@ enum qphy_reg_layout {
QMP_PHY_INIT_CFG(QPHY_V3_PCS_REFGEN_REQ_CONFIG2, 0x60),
};
+static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes_tbl[] = {
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_BG_TIMER, 0x0a),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_IVCO, 0x07),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_CMN_CONFIG, 0x06),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_EN_SEL, 0xd5),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_RESETSM_CNTRL, 0x20),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_SELECT, 0x30),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_HSCLK_SEL, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP_EN, 0x01),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_CTRL, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_CORE_CLK_EN, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_SVS_MODE_CLK_SEL, 0x05),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_INITVAL1, 0xff),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_INITVAL2, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_DEC_START_MODE0, 0x82),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_CP_CTRL_MODE0, 0x06),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_RCTRL_MODE0, 0x16),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_CCTRL_MODE0, 0x36),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN0_MODE0, 0x3f),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN1_MODE0, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE1_MODE0, 0xda),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE2_MODE0, 0x01),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP1_MODE0, 0xff),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP2_MODE0, 0x0c),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_DEC_START_MODE1, 0x98),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_CP_CTRL_MODE1, 0x06),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_RCTRL_MODE1, 0x16),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_CCTRL_MODE1, 0x36),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN0_MODE1, 0x3f),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN1_MODE1, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE1_MODE1, 0xc1),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE2_MODE1, 0x00),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP1_MODE1, 0x32),
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP2_MODE1, 0x0f),
+
+ /* Rate B */
+ QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x44),
+};
+
+static const struct qmp_phy_init_tbl sdm845_ufsphy_tx_tbl[] = {
+ QMP_PHY_INIT_CFG(QSERDES_V3_TX_LANE_MODE_1, 0x06),
+ QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_RX, 0x07),
+};
+
+static const struct qmp_phy_init_tbl sdm845_ufsphy_rx_tbl[] = {
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_LVL, 0x24),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_CNTRL, 0x0f),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_INTERFACE_MODE, 0x40),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_TERM_BW, 0x5b),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL2, 0x06),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL3, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL4, 0x1b),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SVS_SO_GAIN_HALF, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SVS_SO_GAIN_QUARTER, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SVS_SO_GAIN, 0x04),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x4b),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_PI_CONTROLS, 0x81),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_FASTLOCK_COUNT_LOW, 0x80),
+ QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_MODE_00, 0x59),
+};
+
+static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs_tbl[] = {
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SIGDET_CTRL2, 0x6e),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_TX_LARGE_AMP_DRV_LVL, 0x0a),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_TX_SMALL_AMP_DRV_LVL, 0x02),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SYM_RESYNC_CTRL, 0x03),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_TX_MID_TERM_CTRL1, 0x43),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SIGDET_CTRL1, 0x0f),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_MIN_HIBERN8_TIME, 0x9a),
+ QMP_PHY_INIT_CFG(QPHY_V3_PCS_MULTI_LANE_CTRL1, 0x02),
+};
/* struct qmp_phy_cfg - per-PHY initialization config */
struct qmp_phy_cfg {
@@ -649,9 +731,14 @@ struct qmp_phy_cfg {
/* true, if PHY has a separate DP_COM control block */
bool has_phy_dp_com_ctrl;
+ /* true, if PHY has secondary tx/rx lanes to be configured */
+ bool is_dual_lane_phy;
/* Register offset of secondary tx/rx lanes for USB DP combo PHY */
unsigned int tx_b_lane_offset;
unsigned int rx_b_lane_offset;
+
+ /* true, if PCS block has no separate SW_RESET register */
+ bool no_pcs_sw_reset;
};
/**
@@ -748,6 +835,10 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
"aux", "cfg_ahb", "ref", "com_aux",
};
+static const char * const sdm845_ufs_phy_clk_l[] = {
+ "ref", "ref_aux",
+};
+
/* list of resets */
static const char * const msm8996_pciephy_reset_l[] = {
"phy", "common", "cfg",
@@ -758,7 +849,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
};
/* list of regulators */
-static const char * const msm8996_phy_vreg_l[] = {
+static const char * const qmp_phy_vreg_l[] = {
"vdda-phy", "vdda-pll",
};
@@ -778,8 +869,8 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.num_clks = ARRAY_SIZE(msm8996_phy_clk_l),
.reset_list = msm8996_pciephy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_pciephy_reset_l),
- .vreg_list = msm8996_phy_vreg_l,
- .num_vregs = ARRAY_SIZE(msm8996_phy_vreg_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = pciephy_regs_layout,
.start_ctrl = PCS_START | PLL_READY_GATE_EN,
@@ -809,8 +900,8 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.num_clks = ARRAY_SIZE(msm8996_phy_clk_l),
.reset_list = msm8996_usb3phy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
- .vreg_list = msm8996_phy_vreg_l,
- .num_vregs = ARRAY_SIZE(msm8996_phy_vreg_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = usb3phy_regs_layout,
.start_ctrl = SERDES_START | PCS_START,
@@ -870,8 +961,8 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.num_clks = ARRAY_SIZE(qmp_v3_phy_clk_l),
.reset_list = msm8996_usb3phy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
- .vreg_list = msm8996_phy_vreg_l,
- .num_vregs = ARRAY_SIZE(msm8996_phy_vreg_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = qmp_v3_usb3phy_regs_layout,
.start_ctrl = SERDES_START | PCS_START,
@@ -883,6 +974,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.pwrdn_delay_max = POWER_DOWN_DELAY_US_MAX,
.has_phy_dp_com_ctrl = true,
+ .is_dual_lane_phy = true,
.tx_b_lane_offset = 0x400,
.rx_b_lane_offset = 0x400,
};
@@ -903,8 +995,8 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.num_clks = ARRAY_SIZE(qmp_v3_phy_clk_l),
.reset_list = msm8996_usb3phy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
- .vreg_list = msm8996_phy_vreg_l,
- .num_vregs = ARRAY_SIZE(msm8996_phy_vreg_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = qmp_v3_usb3phy_regs_layout,
.start_ctrl = SERDES_START | PCS_START,
@@ -916,6 +1008,35 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
.pwrdn_delay_max = POWER_DOWN_DELAY_US_MAX,
};
+static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
+ .type = PHY_TYPE_UFS,
+ .nlanes = 2,
+
+ .serdes_tbl = sdm845_ufsphy_serdes_tbl,
+ .serdes_tbl_num = ARRAY_SIZE(sdm845_ufsphy_serdes_tbl),
+ .tx_tbl = sdm845_ufsphy_tx_tbl,
+ .tx_tbl_num = ARRAY_SIZE(sdm845_ufsphy_tx_tbl),
+ .rx_tbl = sdm845_ufsphy_rx_tbl,
+ .rx_tbl_num = ARRAY_SIZE(sdm845_ufsphy_rx_tbl),
+ .pcs_tbl = sdm845_ufsphy_pcs_tbl,
+ .pcs_tbl_num = ARRAY_SIZE(sdm845_ufsphy_pcs_tbl),
+ .clk_list = sdm845_ufs_phy_clk_l,
+ .num_clks = ARRAY_SIZE(sdm845_ufs_phy_clk_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
+ .regs = sdm845_ufsphy_regs_layout,
+
+ .start_ctrl = SERDES_START,
+ .pwrdn_ctrl = SW_PWRDN,
+ .mask_pcs_ready = PCS_READY,
+
+ .is_dual_lane_phy = true,
+ .tx_b_lane_offset = 0x400,
+ .rx_b_lane_offset = 0x400,
+
+ .no_pcs_sw_reset = true,
+};
+
static void qcom_qmp_phy_configure(void __iomem *base,
const unsigned int *regs,
const struct qmp_phy_init_tbl tbl[],
@@ -938,7 +1059,9 @@ static void qcom_qmp_phy_configure(void __iomem *base,
static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
{
const struct qmp_phy_cfg *cfg = qmp->cfg;
+ struct qmp_phy *qphy = qmp->phys[0];
void __iomem *serdes = qmp->serdes;
+ void __iomem *pcs = qphy->pcs;
void __iomem *dp_com = qmp->dp_com;
int ret, i;
@@ -1118,18 +1241,30 @@ static int qcom_qmp_phy_init(struct phy *phy)
/* Tx, Rx, and PCS configurations */
qcom_qmp_phy_configure(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num);
/* Configuration for other LANE for USB-DP combo PHY */
- if (cfg->has_phy_dp_com_ctrl)
+ if (cfg->is_dual_lane_phy)
qcom_qmp_phy_configure(tx + cfg->tx_b_lane_offset, cfg->regs,
cfg->tx_tbl, cfg->tx_tbl_num);
qcom_qmp_phy_configure(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num);
- if (cfg->has_phy_dp_com_ctrl)
+ if (cfg->is_dual_lane_phy)
qcom_qmp_phy_configure(rx + cfg->rx_b_lane_offset, cfg->regs,
cfg->rx_tbl, cfg->rx_tbl_num);
qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
/*
+ * UFS PHY requires the deassert of software reset before serdes start.
+ * For UFS PHY that has not software reset control bits in its address
+ * space, it should skip starting serdes here. UFS PHY Serdes shall
+ * start when UFS explicitly calls PHY power on.
+ */
+ if (cfg->type == PHY_TYPE_UFS) {
+ if (cfg->no_pcs_sw_reset)
+ goto out;
+ goto reset_deassert;
+ }
+
+ /*
* Pull out PHY from POWER DOWN state.
* This is active low enable signal to power-down PHY.
*/
@@ -1138,6 +1273,7 @@ static int qcom_qmp_phy_init(struct phy *phy)
if (cfg->has_pwrdn_delay)
usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
+reset_deassert:
/* Pull PHY out of reset state */
qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
if (cfg->has_phy_dp_com_ctrl)
@@ -1157,6 +1293,7 @@ static int qcom_qmp_phy_init(struct phy *phy)
}
qmp->phy_initialized = true;
+out:
return ret;
err_pcs_ready:
@@ -1179,7 +1316,8 @@ static int qcom_qmp_phy_exit(struct phy *phy)
clk_disable_unprepare(qphy->pipe_clk);
/* PHY reset */
- qphy_setbits(qphy->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+ if (!cfg->no_pcs_sw_reset)
+ qphy_setbits(qphy->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
/* stop SerDes and Phy-Coding-Sublayer */
qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
@@ -1197,6 +1335,43 @@ static int qcom_qmp_phy_exit(struct phy *phy)
return 0;
}
+static int qcom_qmp_phy_poweron(struct phy *phy)
+{
+ struct qmp_phy *qphy = phy_get_drvdata(phy);
+ struct qcom_qmp *qmp = qphy->qmp;
+ const struct qmp_phy_cfg *cfg = qmp->cfg;
+ void __iomem *pcs = qphy->pcs;
+ void __iomem *status;
+ unsigned int mask, val;
+ int ret = 0;
+
+ if (cfg->type != PHY_TYPE_UFS)
+ return 0;
+
+ /*
+ * For UFS PHY that has not software reset control, serdes start
+ * should only happen when UFS driver explicitly calls phy_power_on
+ * after it deasserts software reset.
+ */
+ if (cfg->no_pcs_sw_reset && !qmp->phy_initialized) {
+ /* start SerDes and Phy-Coding-Sublayer */
+ qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
+
+ status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
+ mask = cfg->mask_pcs_ready;
+
+ ret = readl_poll_timeout(status, val, !(val & mask), 1,
+ PHY_INIT_COMPLETE_TIMEOUT);
+ if (ret) {
+ dev_err(qmp->dev, "phy initialization timed-out\n");
+ return ret;
+ }
+ qmp->phy_initialized = true;
+ }
+
+ return ret;
+}
+
static int qcom_qmp_phy_set_mode(struct phy *phy, enum phy_mode mode)
{
struct qmp_phy *qphy = phy_get_drvdata(phy);
@@ -1426,6 +1601,7 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
static const struct phy_ops qcom_qmp_phy_gen_ops = {
.init = qcom_qmp_phy_init,
.exit = qcom_qmp_phy_exit,
+ .power_on = qcom_qmp_phy_poweron,
.set_mode = qcom_qmp_phy_set_mode,
.owner = THIS_MODULE,
};
@@ -1531,6 +1707,9 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
}, {
.compatible = "qcom,sdm845-qmp-usb3-uni-phy",
.data = &qmp_v3_usb3_uniphy_cfg,
+ }, {
+ .compatible = "qcom,sdm845-qmp-ufs-phy",
+ .data = &sdm845_ufsphy_cfg,
},
{ },
};
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.h b/drivers/phy/qualcomm/phy-qcom-qmp.h
index 5d78d43..d201cc3 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.h
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.h
@@ -184,6 +184,8 @@
#define QSERDES_V3_COM_VCO_TUNE2_MODE0 0x0f8
#define QSERDES_V3_COM_VCO_TUNE1_MODE1 0x0fc
#define QSERDES_V3_COM_VCO_TUNE2_MODE1 0x100
+#define QSERDES_V3_COM_VCO_TUNE_INITVAL1 0x104
+#define QSERDES_V3_COM_VCO_TUNE_INITVAL2 0x108
#define QSERDES_V3_COM_VCO_TUNE_TIMER1 0x11c
#define QSERDES_V3_COM_VCO_TUNE_TIMER2 0x120
#define QSERDES_V3_COM_CLK_SELECT 0x138
@@ -211,8 +213,13 @@
/* Only for QMP V3 PHY - RX registers */
#define QSERDES_V3_RX_UCDR_SO_GAIN_HALF 0x00c
#define QSERDES_V3_RX_UCDR_SO_GAIN 0x014
+#define QSERDES_V3_RX_UCDR_SVS_SO_GAIN_HALF 0x024
+#define QSERDES_V3_RX_UCDR_SVS_SO_GAIN_QUARTER 0x028
+#define QSERDES_V3_RX_UCDR_SVS_SO_GAIN 0x02c
#define QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN 0x030
#define QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE 0x034
+#define QSERDES_V3_RX_UCDR_FASTLOCK_COUNT_LOW 0x03c
+#define QSERDES_V3_RX_UCDR_PI_CONTROLS 0x044
#define QSERDES_V3_RX_RX_TERM_BW 0x07c
#define QSERDES_V3_RX_VGA_CAL_CNTRL1 0x0bc
#define QSERDES_V3_RX_VGA_CAL_CNTRL2 0x0c0
@@ -239,6 +246,8 @@
#define QPHY_V3_PCS_TXMGN_V3 0x018
#define QPHY_V3_PCS_TXMGN_V4 0x01c
#define QPHY_V3_PCS_TXMGN_LS 0x020
+#define QPHY_V3_PCS_TX_LARGE_AMP_DRV_LVL 0x02c
+#define QPHY_V3_PCS_TX_SMALL_AMP_DRV_LVL 0x034
#define QPHY_V3_PCS_TXDEEMPH_M6DB_V0 0x024
#define QPHY_V3_PCS_TXDEEMPH_M3P5DB_V0 0x028
#define QPHY_V3_PCS_TXDEEMPH_M6DB_V1 0x02c
@@ -275,6 +284,12 @@
#define QPHY_V3_PCS_FLL_CNT_VAL_L 0x0cc
#define QPHY_V3_PCS_FLL_CNT_VAL_H_TOL 0x0d0
#define QPHY_V3_PCS_FLL_MAN_CODE 0x0d4
+#define QPHY_V3_PCS_RX_SYM_RESYNC_CTRL 0x134
+#define QPHY_V3_PCS_RX_MIN_HIBERN8_TIME 0x138
+#define QPHY_V3_PCS_RX_SIGDET_CTRL1 0x13c
+#define QPHY_V3_PCS_RX_SIGDET_CTRL2 0x140
+#define QPHY_V3_PCS_TX_MID_TERM_CTRL1 0x1bc
+#define QPHY_V3_PCS_MULTI_LANE_CTRL1 0x1c4
#define QPHY_V3_PCS_RX_SIGDET_LVL 0x1d8
#define QPHY_V3_PCS_REFGEN_REQ_CONFIG1 0x20c
#define QPHY_V3_PCS_REFGEN_REQ_CONFIG2 0x210
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH v5 3/3] dt-bindings: phy-qcom-qmp: Add UFS phy compatible string for sdm845
From: Can Guo @ 2018-05-23 3:47 UTC (permalink / raw)
To: subhashj, asutoshd, vivek.gautam, mgautam, kishon, robh+dt,
mark.rutland
Cc: linux-kernel, devicetree, Can Guo
In-Reply-To: <20180523034712.3420-1-cang@codeaurora.org>
Update the compatible string for UFS QMP PHY on SDM845.
Signed-off-by: Can Guo <cang@codeaurora.org>
---
Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index cef8765..930d94c 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -11,7 +11,8 @@ Required properties:
"qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
"qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy,
"qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
- "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
+ "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845,
+ "qcom,sdm845-qmp-ufs-phy" for UFS QMP phy on sdm845.
- reg: offset and length of register set for PHY's common serdes block.
@@ -29,6 +30,7 @@ Required properties:
"aux" for phy aux clock,
"ref" for 19.2 MHz ref clk,
"com_aux" for phy common block aux clock,
+ "ref_aux" for phy reference aux clock,
For "qcom,msm8996-qmp-pcie-phy" must contain:
"aux", "cfg_ahb", "ref".
For "qcom,msm8996-qmp-usb3-phy" must contain:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Viresh Kumar @ 2018-05-23 4:29 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-1-sebastien.szymanski@armadeus.com>
On 22-05-18, 08:28, Sébastien Szymanski wrote:
> Check the max speed supported from the fuses for i.MX6ULL and update the
> operating points table accordingly.
>
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>
> Changes for v3:
> - none
@Sascha and Shawn: Can you guys please Ack this series if there is
nothing wrong with it ?
--
viresh
^ permalink raw reply
* Re: [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Viresh Kumar @ 2018-05-23 4:30 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-1-sebastien.szymanski@armadeus.com>
On 22-05-18, 08:28, Sébastien Szymanski wrote:
> Check the max speed supported from the fuses for i.MX6ULL and update the
> operating points table accordingly.
>
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>
> Changes for v3:
> - none
>
> Changes for v2:
> - none
>
> drivers/cpufreq/imx6q-cpufreq.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 83cf631fc9bc..f094687cae52 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -266,6 +266,8 @@ static void imx6q_opp_check_speed_grading(struct device *dev)
> }
>
> #define OCOTP_CFG3_6UL_SPEED_696MHZ 0x2
> +#define OCOTP_CFG3_6ULL_SPEED_792MHZ 0x2
> +#define OCOTP_CFG3_6ULL_SPEED_900MHZ 0x3
>
> static void imx6ul_opp_check_speed_grading(struct device *dev)
> {
> @@ -287,16 +289,30 @@ static void imx6ul_opp_check_speed_grading(struct device *dev)
> * Speed GRADING[1:0] defines the max speed of ARM:
> * 2b'00: Reserved;
> * 2b'01: 528000000Hz;
> - * 2b'10: 696000000Hz;
> - * 2b'11: Reserved;
> + * 2b'10: 696000000Hz on i.MX6UL, 792000000Hz on i.MX6ULL;
> + * 2b'11: 900000000Hz on i.MX6ULL only;
> * We need to set the max speed of ARM according to fuse map.
> */
> val = readl_relaxed(base + OCOTP_CFG3);
> val >>= OCOTP_CFG3_SPEED_SHIFT;
> val &= 0x3;
> - if (val != OCOTP_CFG3_6UL_SPEED_696MHZ)
> - if (dev_pm_opp_disable(dev, 696000000))
> - dev_warn(dev, "failed to disable 696MHz OPP\n");
> +
> + if (of_machine_is_compatible("fsl,imx6ul")) {
> + if (val != OCOTP_CFG3_6UL_SPEED_696MHZ)
> + if (dev_pm_opp_disable(dev, 696000000))
> + dev_warn(dev, "failed to disable 696MHz OPP\n");
> + }
> +
> + if (of_machine_is_compatible("fsl,imx6ull")) {
> + if (val != OCOTP_CFG3_6ULL_SPEED_792MHZ)
> + if (dev_pm_opp_disable(dev, 792000000))
> + dev_warn(dev, "failed to disable 792MHz OPP\n");
> +
> + if (val != OCOTP_CFG3_6ULL_SPEED_900MHZ)
> + if (dev_pm_opp_disable(dev, 900000000))
> + dev_warn(dev, "failed to disable 900MHz OPP\n");
> + }
> +
> iounmap(base);
> put_node:
> of_node_put(np);
> @@ -356,7 +372,8 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> goto put_reg;
> }
>
> - if (of_machine_is_compatible("fsl,imx6ul"))
> + if (of_machine_is_compatible("fsl,imx6ul") ||
> + of_machine_is_compatible("fsl,imx6ull"))
> imx6ul_opp_check_speed_grading(cpu_dev);
> else
> imx6q_opp_check_speed_grading(cpu_dev);
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH v3 3/3] ARM: dts: imx6ull-colibri-wifi: remove operating points
From: Viresh Kumar @ 2018-05-23 4:30 UTC (permalink / raw)
To: Sébastien Szymanski
Cc: linux-arm-kernel, Rafael J . Wysocki, linux-pm, linux-kernel,
Shawn Guo, Sascha Hauer, Fabio Estevam, Stefan Agner, Rob Herring,
Mark Rutland, devicetree
In-Reply-To: <20180522062853.24799-3-sebastien.szymanski@armadeus.com>
On 22-05-18, 08:28, Sébastien Szymanski wrote:
> Operating points are now defined in the imx6ull.dtsi file so remove
> them from board device trees.
>
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
> arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi b/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
> index 3dffbcd50bf6..183193e8580d 100644
> --- a/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
> +++ b/arch/arm/boot/dts/imx6ull-colibri-wifi.dtsi
> @@ -20,20 +20,6 @@
>
> &cpu0 {
> clock-frequency = <792000000>;
> - operating-points = <
> - /* kHz uV */
> - 792000 1225000
> - 528000 1175000
> - 396000 1025000
> - 198000 950000
> - >;
> - fsl,soc-operating-points = <
> - /* KHz uV */
> - 792000 1175000
> - 528000 1175000
> - 396000 1175000
> - 198000 1175000
> - >;
> };
>
> &iomuxc {
Maybe you should merge this with the previous patch itself.
--
viresh
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-23 5:10 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <b5078a17-4cbb-905e-4cd1-f252a02f97e9@codeaurora.org>
Hi,
On Tue, May 22, 2018 at 6:19 PM, David Collins <collinsd@codeaurora.org> wrote:
>>>> OK, so how's this for a proposal then:
>>>>
>>>> 1. For RPMh-regulator whenever we see a "set voltage" but Linux hasn't
>>>> specified that the regulator is enabled then we don't send the
>>>> voltage, we just cache it.
>>>>
>>>> 2. When we see the first enable then we first send the cached voltage
>>>> and then do the enable.
>>>>
>>>> 3. We don't need an "initial voltage" because any rails that are
>>>> expected to be variable voltage the client should be choosing a
>>>> voltage.
>>>>
>>>>
>>>> ...taking the SD card case as an example: if the regulator framework
>>>> says "set to the minimum: 1.8V" we'll cache this but not apply it yet
>>>> because the rail is off as far as Linux is concerned. Then when the
>>>> SD card framework starts up it will set the voltage to 3.3V which will
>>>> overwrite the cache. Then the SD card framework will enable the
>>>> regulator and RPMh will set the voltage to 3.3V and enable.
>>>
>>> I am ok with implementing this feature.
>>>
>>> However, should the voltage be cached instead of sent immediately any time
>>> that Linux has not explicitly requested the regulator to be enabled, or
>>> only before the first time that an enable request is sent?
>>>
>>> 1. regulator_set_voltage(reg, 2960000, 2960000)
>>> --> cache voltage=2960 mV
>>> 2. regulator_enable(reg)
>>> --> Send voltage=2960 then enable=1
>>> 3. regulator_disable(reg)
>>> --> Send enable=0
>>> 4. regulator_set_voltage(reg, 1800000, 2960000)
>>> --> A. Send voltage=1800 OR B. cache voltage=1800?
>>>
>>> Option A is used on the downstream rpmh-regulator driver in order to avoid
>>> cases where AP votes to disable a regulator that is kept enabled by
>>> another subsystem but then does not lower the voltage requested thanks to
>>> regulator_set_voltage() being called after regulator_disable(). I plan to
>>> go with option A for qcom-rpmh-regulator unless there are objections.
>>
>> So one client's vote for a voltage continues to be in effect even if
>> that client votes to have the regulator disabled? That seems
>> fundamentally broken in RPMh. I guess my take would be to work around
>> this RPMh misfeature by saying that whenever Linux votes to disable a
>> regulator it also votes for a voltage of 0. Then another client of
>> RPMh won't be affected.
>>
>> That seems like it would be beneficial in any case. If the AP always
>> asks for 1.3 V and the modem always asks for 1.1 V for the same rail
>> then the rail should go down to 1.1 V when the AP says to disable the
>> rail.
>
> The VRM in RPMh hardware aggregates enable state, output voltage, mode,
> and headroom voltage requests independently for each regulator. This
> allows for maximum request flexibility and makes no assumptions about
> consumer use cases. There is nothing inherently wrong about this approach.
Just to confirm I'm understanding correctly:
1. AP: request that regulator A be at 1.3 V and enabled
==> actual output: regulator A is 1.3 V and enabled
2. modem: requests that regulator A be at 1.1 V and enabled
==> actual output: regulator A is 1.3 V and enabled
3. AP: request that regulator A be disabled
You're saying that here the output of regulator A will be "1.3 V" and
"enabled".
I just can't see how that can be correct behavior. A given client's
vote for the voltage should really only make sense if that client
wants the regulator enabled. In the case above the kernel would have
no idea that someone else might have the regulator enabled. Why would
it care if that other user gets it at 1.3 V or at 1.1 V?
You have some use case in mind where the kernel would need to have
control over the voltage of a regulator that it thinks is disabled?
Now obviously if the kernel re-enables the regulator then its old
voltage vote should be re-instated right away, but until then its vote
about the voltage shouldn't count. If that means that the kernel has
to "vote" for 0V then I guess that's the way the API works.
> I'm concerned about a blanket policy of setting voltage=0 when issuing
> disable requests. That seems to violate the semantics of the
> regulator_set_voltage() API which enforces the requested voltage range
> until a new range is specified. There may be workarounds that require a
> regulator to operate at a specific voltage even when no Linux consumers
> explicitly need the regulator enabled.
>
> Given that not masking the voltage on disable is guaranteed to be safe and
> does not silently break potential use cases, I plan to stick with this
> approach. Therefore, the question about option A or B for voltage caching
> is still relevant. I think that option A is safer/more API conforming so
> I plan to implement that.
I still can't understand how it ever makes sense to vote for a voltage
for a regulator that you think is disabled. ...but if there's some
reason it does then I guess I'm OK with A.
>>>> This whole thing makes me worry about another problem, though. You
>>>> say that the bootloader left the SD card rail on, right? ...but as
>>>> far as Linux is concerned the SD card rail is off (argh, it can't read
>>>> the state that the bootloader left the rail in!)
>>>>
>>>> ...now imagine any of the following:
>>>>
>>>> * The user boots up a kernel where the SD card driver is disabled.
>>>>
>>>> * The user ejects the SD card right after the bootloader used it but
>>>> before the kernel driver started.
>>>>
>>>> When the kernel comes up it will believe that the SD card rail is
>>>> disabled so it won't try to disable it. So the voltage will be left
>>>> on.
>>>
>>> We have not encountered issues with regulators getting left on
>>> indefinitely because Linux devices failed to take control of them during
>>> boot. I don't think that this hypothetical issue needs to be addressed in
>>> the first qcom-rpmh-regulator driver patch if at all.
>>
>> I don't think it hypothetical at all. Linux takes control of a
>> regulator and then at bootup it disables all regulators that aren't
>> currently used/enabled. In your case you will report that regulators
>> are already disabled and thus you'll neuter the kernel's attempt to
>> disable regulators that nobody is using but that might have been left
>> on by the bootloader. It seemed like Mark agreed here.
>
> I did not consider regulator_late_cleanup(). I'll initialize the enabled
> value in qcom-rpmh-regulator to -EINVAL to handle this case and also so
> that _regulator_enable() succeeds without further core modification.
That's weird that the regulator code has a special case for -EINVAL in
_regulator_enable(). Given how most of the code in the
regulator/core.c doesn't seem to check for error codes I wonder if
anyone is using that... I guess I'd leave it to Mark whether he's
happy with -EINVAL for this case even though it's asymmetric to using
-ENOTRECOVERABLE for getting the voltage.
Are we really sure there aren't any places that the regulator code
needs to handle an error from _regulator_enable()? An an example, in
regulator_resolve_supply() are we sure we should be passing a
regulator_enable() on to our parent supply even if
_regulator_is_enabled() returns an error? I haven't looked in depth
of all use cases, though...
I see you posted a new version. Thanks! I'll try to find some time
soon to review it, but I'll be a bit busy over the next few days.
-Doug
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-23 5:44 UTC (permalink / raw)
To: Sudeep Holla
Cc: Ilia Lin, linux-clk, devicetree, linux-kernel, linux-pm,
linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <20180522130704.GA31065@e107155-lin>
On 22-05-18, 14:07, Sudeep Holla wrote:
> On Tue, May 22, 2018 at 02:29:45PM +0300, Ilia Lin wrote:
> > In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> > the CPU frequency subset and voltage value of each OPP varies
> > based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> > defines the voltage and frequency value based on the msm-id in SMEM
> > and speedbin blown in the efuse combination.
> > The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> > to provide the OPP framework with required information.
> > This is used to determine the voltage and frequency value for each OPP of
> > operating-points-v2 table when it is parsed by the OPP framework.
> >
> > Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> [...]
>
> > +
> > + switch (msm8996_version) {
> > + case MSM8996_V3:
> > + versions = 1 << (unsigned int)(*speedbin);
> > + break;
> > + case MSM8996_SG:
> > + versions = 1 << ((unsigned int)(*speedbin) + 4);
> > + break;
> > + default:
> > + BUG();
> > + break;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + cpu_dev = get_cpu_device(cpu);
> > + if (NULL == cpu_dev) {
> > + ret = -ENODEV;
> > + goto free_opp;
> > + }
> > +
> > + opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> > + &versions, 1);
>
> Will be not get NULL for all CPUs except 0 ?
With my patches, we will get the OPP table again with refcount
incremented. And on failures, we need to call put-supported-hw helper
only for the CPUs for which it passed previously.
--
viresh
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
From: Viresh Kumar @ 2018-05-23 5:48 UTC (permalink / raw)
To: Rob Herring
Cc: Taniya Das, Rafael J. Wysocki, linux-kernel, linux-pm,
Stephen Boyd, Rajendra Nayak, Amit Nischal, devicetree, skannan,
amit.kucheria
In-Reply-To: <20180522193139.GA21623@rob-hp-laptop>
On 22-05-18, 14:31, Rob Herring wrote:
> On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
> > + freq-domain-0 {
> > + compatible = "cpufreq";
> > + reg = <0x17d43920 0x4>,
> > + <0x17d43110 0x500>,
> > + <0x17d41000 0x4>;
> > + reg-names = "perf_base", "lut_base", "en_base";
> > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
I was thinking, can't we add platform specific properties in the CPU
nodes ? If yes, then we can point the phandle of fw node from the CPUs
and this awkward list can go away.
--
viresh
^ permalink raw reply
* Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver
From: Viresh Kumar @ 2018-05-23 5:54 UTC (permalink / raw)
To: Taniya Das
Cc: skannan, Rafael J. Wysocki, linux-kernel, linux-pm, Stephen Boyd,
robh, Rajendra Nayak, Amit Nischal, devicetree, amit.kucheria
In-Reply-To: <bf5b09d3-4d97-5357-3fb2-926227bb7229@codeaurora.org>
On 22-05-18, 16:13, Taniya Das wrote:
> On 5/22/2018 12:36 AM, skannan@codeaurora.org wrote:
> > On 2018-05-21 02:01, Viresh Kumar wrote:
> > > On 19-05-18, 23:04, Taniya Das wrote:
> > > > + /* Make sure the write goes through before proceeding */
> > > > + mb();
> > >
> > > Btw what happens right after this is done ? Are we guaranteed that the
> > > frequency is updated in the hardware after this ? What about enabling
> > > fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
> > > to see how that is done.
> >
> > Yeah, I don't think this is needed really.
> >
>
> Just want to make sure it doesn't really sit in the write buffer before
> return.
As per Saravana you will be dropping it now, right ?
> > > > + index = readl_relaxed(c->perf_base);
> > > > + index = min(index, LUT_MAX_ENTRIES - 1);
> > >
> > > Will the hardware ever read a value over 39 here ?
> >
> > The register could be initialized to whatever before the kernel is
> > brought up. Don't want to depend on it being correct to avoid out of
> > bounds access that could leak data.
Fair enough.
> > > > +static int qcom_read_lut(struct platform_device *pdev,
> > > > + struct cpufreq_qcom *c)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > + u32 data, src, lval, i, core_count, prev_cc;
> > > > +
> > > > + c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > > + sizeof(*c->table), GFP_KERNEL);
> > > > + if (!c->table)
> > > > + return -ENOMEM;
> > > > +
> > > > + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > > + data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> > > > + src = ((data & GENMASK(31, 30)) >> 30);
> > > > + lval = (data & GENMASK(7, 0));
> > > > + core_count = CORE_COUNT_VAL(data);
> > > > +
> > > > + if (!src)
> > > > + c->table[i].frequency = INIT_RATE / 1000;
> > > > + else
> > > > + c->table[i].frequency = XO_RATE * lval / 1000;
> > > > +
> > > > + c->table[i].driver_data = c->table[i].frequency;
> > >
> > > Why do you need to use driver_data here? Why can't you simple use
> > > frequency field in the below conditional expressions ?
> > >
>
> The frequency field would be marked INVALID in case the core count does not
> match and the frequency data would be lost.
>
> > > > +
> > > > + dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > > + i, c->table[i].frequency, core_count);
> > > > +
> > > > + if (core_count != c->max_cores)
> > > > + c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> > > > +
> > > > + /*
> > > > + * Two of the same frequencies with the same core counts means
> > > > + * end of table.
> > > > + */
> > > > + if (i > 0 && c->table[i - 1].driver_data ==
> > > > + c->table[i].driver_data && prev_cc == core_count) {
> > > > + struct cpufreq_frequency_table *prev = &c->table[i - 1];
> > > > +
> > > > + if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> > >
> > > There can only be a single boost frequency at max ?
> >
> > As of now, yes. If that changes, we'll change this code later.
> >
> > > > + prev->flags = CPUFREQ_BOOST_FREQ;
> > > > + prev->frequency = prev->driver_data;
> > >
> > > Okay you are using driver_data as a local variable to keep this value
> > > safe which you might have overwritten. Maybe use a simple variable
> > > prev_freq for this. It would be much more readable in that case and
> > > you wouldn't end up abusing the driver_data field.
> > >
>
> Please correct me, currently the driver_data is not used by cpufreq core and
> that was the reason to use it. In case you still think it is not a good way
> to handle it, I would try to handle it differently.
Yeah, the driver data will not be used by cpufreq core, but I think
the code would be far more readable if you use a local variable like
prev_freq instead.
> > > > + }
> > > > +
> > > > + break;
> > > > + }
> > > > + prev_cc = core_count;
> > > > + }
> > > > + c->table[i].frequency = CPUFREQ_TABLE_END;
> > >
> > > Wouldn't you end up writing on c->table[40].frequency here if there
> > > are 40 frequency value present ?
> >
> > Yeah, the loop condition needs to be fixed.
> >
>
> The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
> Yes in case we have all [0-39] (i.e 40 entries) read from the hardware,
> would store the same and mark the 40th index as table end. Please correct if
> I missed something in your comment.
Ahh, you are right. I misread it.
--
viresh
^ permalink raw reply
* Re: [RFC 12/13] ARM: dts: ti: add dra71-evm FIT description file
From: Tero Kristo @ 2018-05-23 5:55 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland, devicetree, trini, Tony Lindgren,
Russell King - ARM Linux, frowand.list, wmills,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20180522200100.GA23937@rob-hp-laptop>
On 22/05/18 23:01, Rob Herring wrote:
> On Mon, May 21, 2018 at 09:57:54AM +0300, Tero Kristo wrote:
>> On 17/04/18 17:49, Tony Lindgren wrote:
>>> * Tero Kristo <t-kristo@ti.com> [180417 09:36]:
>>>> In typical setup, you can boot a large number of different configs via:
>>>>
>>>> bootm 0x82000000#dra71-evm#nand#lcd-auo-g101evn01.0
>>>>
>>>> ... assuming the configs were named like that, and assuming they would be
>>>> compatible with each other. The am57xx-evm example provided is better, as
>>>> you can chain the different cameras to the available evm configs.
>>>
>>> Why not just do it in the bootloader to put together the dtb?
>>>
>>> Then for external devices, you could just pass info on the
>>> kernel cmdline with lcd=foo camera=bar if they cannot be
>>> detected over I2C.
>>
>> (Added Linux ARM list to CC, this was not part of the original delivery.)
>>
>> Ok trying to resurrect this thread a bit. Is there any kind of consensus how
>> things like this should be handled? Should we add the DT overlay files to
>> kernel tree or not?
>
> IMO, yes.
>
>> Should we add any kind of build infra to kernel tree, and at what level
>> would this be? Just DT overlay file building support, and drop the FIT build
>> support as was proposed in this RFC series or...?
>
> I think I mentioned this already, but I expect that this is going to
> cause a number of conversions of dtsi + dtsi -> dtb into base dts and
> overlay(s) dts files. In doing so, we still need to be able to build the
> original, full dtb.
So you mean like breaking apart the existing .dts files? Are there any
plans to get that done (I know the android folks talk about this but I
don't like their idea.) If we do the split, how are we going to
determine which dts + overlay files are required to get a specific DTB
done? I actually wrote a tool for this purpose which parses the FIT
image configurations and generates plain dtb files out of the info there
if needed, but assuming FIT is abandoned then what...?
>
>> U-boot can obviously parse the base DTB + overlay DTB:s into a single DTB,
>> but this is somewhat clumsy approach and is relatively error prone to get it
>> right.
>
> Why? How is the kernel better?
I am mostly speaking about runtime applying of the overlays. If done
build time, it is obviously on same level. If you apply the base DTS +
overlays from u-boot prompt, it is not too much fun, and if there are
any failures it just won't work, but don't really tell you why not.
>
>> Building the FIT image post kernel build would also be possible, but who
>> would be doing this, is there any need to get this done in generic manner or
>> shall we just add SoC vendor specific tools for this?
>
> I'll tell you up front, I'm not a fan of FIT image (nor uImage,
> Android boot image, $bootloader image). If you want a collection of
> files and some configuration data, use a filesystem and a text file.
Ok, thanks for your frank comments. I believe based on this feedback
I'll try to modify this series into bare minimal overlay support to
kernel, and have the post processing done elsewhere (either u-boot build
or possibly completely separate tool.)
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
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