* [PATCH v4 3/3] arm64: Introduce command line parameter to disable CNP
From: Catalin Marinas @ 2018-05-23 17:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526638022-4137-4-git-send-email-vladimir.murzin@arm.com>
On Fri, May 18, 2018 at 11:07:02AM +0100, Vladimir Murzin wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 11fc28e..8f59d47 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2636,6 +2636,10 @@
>
> noclflush [BUGS=X86] Don't use the CLFLUSH instruction
>
> + nocnp [ARM64]
> + Disable CNP (Common not Private translations)
> + even if it is supported by processor.
> +
> nodelayacct [KNL] Disable per-task delay accounting
>
> nodsp [SH] Disable hardware DSP at boot time.
Do we actually have a use-case for this command line option? I'm not
considering hardware errata as these are handled separately in the
kernel.
--
Catalin
^ permalink raw reply
* [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate
From: Scott Branden @ 2018-05-23 17:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5a996888-d3d3-9ae6-e438-5de4d5e3ea32@broadcom.com>
Raym
On 18-05-23 09:29 AM, Ray Jui wrote:
> Hi Robin,
>
> On 5/23/2018 4:48 AM, Robin Murphy wrote:
>> On 23/05/18 08:52, Scott Branden wrote:
>>>
>>>
>>> On 18-05-22 04:24 PM, Ray Jui wrote:
>>>> 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?
>>> Ray - your original code looks correct to me.? Easier to read and
>>> less prone to errors as shown in the attempted translation to a
>>> single statement.
>>
>> ?????if (<boolean condition>)
>> ???????? return true;
>> ?????else
>> ???????? return false;
>>
>> still looks really dumb, though, and IMO is actually harder to read
>> than just "return <boolean condition>;" because it forces you to stop
>> and double-check that the logic is, in fact, only doing the obvious
>> thing.
>
> If you can propose a way to modify my original code above to make it
> more readable, I'm fine to make the change.
>
> As I mentioned, I don't think the following change proposed by Guenter
> will work due to the reason I pointed out:
>
> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK));
What they are looking for is:
return ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) ==
ENABLE_MASK);
or maybe:
return !!((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) ==
ENABLE_MASK);
>
>>
>> Robin.
>>
>>
>>
>> p.s. No thanks for making me remember the mind-boggling horror of
>> briefly maintaining part of this legacy codebase... :P
>>
>> $ grep -r '? true : false' --include=*.cpp . | wc -l
>> 951
^ permalink raw reply
* [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate
From: Robin Murphy @ 2018-05-23 17:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5a996888-d3d3-9ae6-e438-5de4d5e3ea32@broadcom.com>
On 23/05/18 17:29, Ray Jui wrote:
> Hi Robin,
>
> On 5/23/2018 4:48 AM, Robin Murphy wrote:
>> On 23/05/18 08:52, Scott Branden wrote:
>>>
>>>
>>> On 18-05-22 04:24 PM, Ray Jui wrote:
>>>> 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?
>>> Ray - your original code looks correct to me.? Easier to read and
>>> less prone to errors as shown in the attempted translation to a
>>> single statement.
>>
>> ?????if (<boolean condition>)
>> ???????? return true;
>> ?????else
>> ???????? return false;
>>
>> still looks really dumb, though, and IMO is actually harder to read
>> than just "return <boolean condition>;" because it forces you to stop
>> and double-check that the logic is, in fact, only doing the obvious
>> thing.
>
> If you can propose a way to modify my original code above to make it
> more readable, I'm fine to make the change.
Well,
return readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK == ENABLE_MASK;
would probably be reasonable to anyone other than the 80-column zealots,
but removing the silly boolean-to-boolean translation idiom really only
emphasises the fact that it's fundamentally a big complex statement; for
maximum clarity I'd be inclined to separate the two logical operations
(read and comparison), e.g.:
u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL);
return wdtcontrol & ENABLE_MASK == ENABLE_MASK;
which is still -3 lines vs. the original.
> As I mentioned, I don't think the following change proposed by Guenter
> will work due to the reason I pointed out:
>
> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK));
FWIW, getting the desired result should only need one logical not
swapping for a bitwise one there:
return !(~readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK);
but that's well into "too clever for its own good" territory ;)
Robin.
^ permalink raw reply
* [PATCH v4 2/3] arm64: KVM: Enable Common Not Private translations
From: Catalin Marinas @ 2018-05-23 17:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526638022-4137-3-git-send-email-vladimir.murzin@arm.com>
On Fri, May 18, 2018 at 11:07:01AM +0100, Vladimir Murzin wrote:
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index a4c1b76..9a651a2 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -472,7 +472,7 @@ static bool need_new_vmid_gen(struct kvm *kvm)
> static void update_vttbr(struct kvm *kvm)
> {
> phys_addr_t pgd_phys;
> - u64 vmid;
> + u64 vmid, cnp = kvm_cpu_has_cnp() ? 1 : 0;
Please define a VTTBR_CNP_BIT here instead of a hard-coded value.
> bool new_gen;
>
> read_lock(&kvm_vmid_lock);
> @@ -522,7 +522,7 @@ static void update_vttbr(struct kvm *kvm)
> pgd_phys = virt_to_phys(kvm->arch.pgd);
> BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
> vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & VTTBR_VMID_MASK(kvm_vmid_bits);
> - kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid;
> + kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid | cnp;
>
> write_unlock(&kvm_vmid_lock);
> }
--
Catalin
^ permalink raw reply
* [PATCH 0/2] get rid of Kconfig symbol MACH_MESON8B
From: Kevin Hilman @ 2018-05-23 17:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180520172353.19256-1-martin.blumenstingl@googlemail.com>
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> as noted by Kevin [0] there are two Kconfig symbols which only differ
> in their Kconfig help text and the list of .dtbs that are being built.
> the goal of this small series is to get rid of the unnecessary
> MACH_MESON8B Kconfig symbol by merging it into MACH_MESON8.
Thanks for the cleanup.
Applied to v4.18/dt with Jerome's tag,
Kevin
^ permalink raw reply
* [PATCH v2 0/4] ARM64: dts: meson-axg: i2c updates
From: Kevin Hilman @ 2018-05-23 17:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517093100.22225-1-jbrunet@baylibre.com>
Jerome Brunet <jbrunet@baylibre.com> writes:
> This patchset fixes a few problems found in the i2c nodes of
> amlogic's meson-axg platform. It also adds the missing pins for
> AO controller so we can use it on the S400
>
> This series has been tested on the S400 board.
>
> Jerome Brunet (4):
> ARM64: dts: meson-axg: clean-up i2c nodes
> ARM64: dts: meson-axg: correct i2c AO clock
> ARM64: dts: meson-axg: add i2c AO pins
> ARM64: dts: meson-axg: enable i2c AO on the S400 board
Applied to v4.18/dt64,
Kevin
^ permalink raw reply
* [PATCH] clk: aspeed: Add 24MHz fixed clock
From: Rob Herring @ 2018-05-23 17:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526633822-17138-1-git-send-email-mine260309@gmail.com>
On Fri, May 18, 2018 at 04:57:02PM +0800, Lei YU wrote:
> Add a 24MHz fixed clock.
> This clock will be used for certain devices, e.g. pwm.
>
> Signed-off-by: Lei YU <mine260309@gmail.com>
> ---
> drivers/clk/clk-aspeed.c | 9 ++++++++-
> include/dt-bindings/clock/aspeed-clock.h | 1 +
> 2 files changed, 9 insertions(+), 1 deletion(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH] firmware: arm_scmi: remove some unnecessary checks
From: Sudeep Holla @ 2018-05-23 16:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180519063715.GB4991@mwanda>
On 19/05/18 07:37, Dan Carpenter wrote:
> The "pi->dom_info" buffer is allocated in init() and it can't be NULL
> here. These tests are sort of weird as well because if "pi->dom_info"
> was NULL but "domain" was non-zero then it would lead to an Oops.
>
Indeed. Thanks for the patch.
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
--
Regards,
Sudeep
^ permalink raw reply
* [PATCH net] net: phy: broadcom: Fix bcm_write_exp()
From: Florian Fainelli @ 2018-05-23 16:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <89a1f8d7-5303-f3c0-aa38-43e64488ec5a@gmail.com>
On 05/22/2018 06:20 PM, Florian Fainelli wrote:
> Hi Andrew,
>
> On 05/22/2018 05:15 PM, Andrew Lunn wrote:
>> On Tue, May 22, 2018 at 05:04:49PM -0700, Florian Fainelli wrote:
>>> On newer PHYs, we need to select the expansion register to write with
>>> setting bits [11:8] to 0xf. This was done correctly by bcm7xxx.c prior
>>> to being migrated to generic code under bcm-phy-lib.c which
>>> unfortunately used the older implementation from the BCM54xx days.
>>
>> Hi Florian
>>
>> Does selecting the expansion register affect access to the standard
>> registers? Does this need locking like the Marvell PHY has when
>> changing pages?
>
> We should probably convert this to the page accessors since the
> expansion, misc and other shadow 0x1c accesses are all indirection
> layers to poke into a different address space of the PHY. That would be
> a separate fix though for a number of reasons.
I realize I did not quite answer your question, the answer to your
question AFAICT is no, setting the expansion register sequence and then
aborting mid-way is not a problem and does not impact the standard MII
registers because of how this is implemented. The registers are accessed
and latched through a specific indirect sequence, but there is no page
switching unlike the Marvell PHYs
--
Florian
^ permalink raw reply
* [PATCH 01/14] memory: ti-emif-sram: Add resume function to recopy sram code
From: Santosh Shilimkar @ 2018-05-23 16:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <739d9bbf-2acc-9c90-db43-cf78f5b184e3@ti.com>
On 5/23/2018 1:47 AM, Keerthy wrote:
>
>
> On Monday 16 April 2018 03:59 PM, Keerthy wrote:
>>
[..]
>>> Instead of this indirect method , why can't just check the previous
>>> deep sleep mode and based on that do copy or not. EMIF power status
>>> register should have something like that ?
>>
>> I will check if we have a register that tells the previous state of sram.
>
> Unfortunately i do not see any such register for knowing SRAM previous
> state in am43 TRM and hence this indirect way of knowing.
>
OK.
>
>>
>>>
>>> Another minor point is even though there is nothing to do in suspend,
>>> might be good to have a callback with comment that nothing to do with
>>> some explanation why not. Don't have strong preference but may for
>>> better readability.
>
> I can add a blank suspend call with comment
>
> "The contents are already present in DDR hence no need to explicitly save"
>
> The comment in resume function pretty much explains the above. So let me
> know if i need to add the suspend callback.
> Please add the empty suspend callback with comment.
Regards,
Santosh
^ permalink raw reply
* [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks
From: Catalin Marinas @ 2018-05-23 16:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180523150337.GO13470@e103592.cambridge.arm.com>
On Wed, May 23, 2018 at 04:03:37PM +0100, Dave P Martin wrote:
> On Wed, May 23, 2018 at 03:56:57PM +0100, Catalin Marinas wrote:
> > On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote:
> > > On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> > > > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > > > > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > > > > cleared except when returning to userspace or returning from a
> > > > > signal: thus, for a true kernel thread no FPSIMD context is ever
> > > > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > > > > ever be saved.
> > > >
> > > > I don't understand this construction proof; from looking at the patch
> > > > below it is not obvious to me why fpsimd_thread_switch() can never have
> > > > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> > > > kernel thread?
> > >
> > > Looking at this again, I think it is poorly worded. This patch aims to
> > > make it true by construction, but it isn't prior to the patch.
> > >
> > > I'm tempted to delete the paragraph: the assertion of both untrue and
> > > not the best way to justify that this patch works.
> > >
> > >
> > > How about:
> > >
> > > -8<-
> > >
> > > The context switch logic already isolates user threads from each other.
> > > This, it is sufficient for isolating user threads from the kernel,
> > > since the goal either way is to ensure that code executing in userspace
> > > cannot see any FPSIMD state except its own. Thus, there is no special
> > > property of kernel threads that we care about except that it is
> > > pointless to save or load FPSIMD register state for them.
> > >
> > > At worst, the removal of all the kernel thread special cases by this
> > > patch would thus spuriously load and save state for kernel threads when
> > > unnecessary.
> > >
> > > But the context switch logic is already deliberately optimised to defer
> > > reloads of the regs until ret_to_user (or sigreturn as a special case),
> > > which kernel threads by definition never reach.
> > >
> > > ->8-
> >
> > The "at worst" paragraph makes it look like it could happen (at least
> > until you reach the last paragraph). Maybe you can just say that
> > wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are
> > always true for kernel threads. You should probably mention this in a
> > comment in the code as well.
>
> What if I just delete the second paragraph, and remove the "But" from
> the start of the third, and append:
>
> "As a result, the wrong_task and wrong_cpu tests in
> fpsimd_thread_switch() will always yield false for kernel threads."
>
> ...with a similar comment in the code?
Sounds fine. With that, feel free to add:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* [PATCH] perf: hisi: fix uncore PMU index ID
From: Will Deacon @ 2018-05-23 16:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a301d72f-687c-0c4f-1d44-83e5bbad5e60@hisilicon.com>
On Tue, May 22, 2018 at 05:18:51PM +0800, Zhangshaokun wrote:
> On 2018/5/22 1:05, Will Deacon wrote:
> > Whilst I'd normally just accept PMU driver submissions for vendor PMUs,
> > this part rang my alarm bells:
> >
> >> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> index 443906e..dcd8e77 100644
> >> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> @@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
> >> static int hisi_hha_pmu_init_data(struct platform_device *pdev,
> >> struct hisi_pmu *hha_pmu)
> >> {
> >> - unsigned long long id;
> >> struct resource *res;
> >> - acpi_status status;
> >> -
> >> - status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
> >> - "_UID", NULL, &id);
> >> - if (ACPI_FAILURE(status))
> >> - return -EINVAL;
> >> -
> >> - hha_pmu->index_id = id;
> >>
> >> /*
> >> - * Use SCCL_ID and UID to identify the HHA PMU, while
> >> + * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
> >> * SCCL_ID is in MPIDR[aff2].
> >> */
> >> if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> >> @@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
> >> dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
> >> return -EINVAL;
> >> }
> >> +
> >> + if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> >> + &hha_pmu->index_id)) {
> >> + dev_err(&pdev->dev, "Can not read hha index-id!\n");
> >> + return -EINVAL;
> >> + }
> >
> > Is this a new DT property? If so, please can you update the binding
> > documentation and get an Ack from a DT maintainer? It's not clear to me
>
> No, it is not a DT property. We don't support DT mode for this platform and
> only support ACPI mode.
Hmm, but by using the firmware-agnostic "device_property_read_u32"
interface, aren't you implicitly supporting it via DT as well? In fact,
don't you now fail the probe if this new property isn't present? Isn't
that a regression?
> > what a "hisilicon,idx-id" is, nor how I would generate on from firmware.
> >
>
> For HiSilicon this platform, it supports multi-sccl. each sccl has more than one uncore
> PMUs. Like HHA uncore PMUs, each sccl has 2-HHA PMUs and idx-id is in _DSD package and
> used to distinguish different HHA PMUs with the same sccl, as follow:
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"hisilicon,scl-id", 0x03},
> Package () {"hisilicon,idx-id", 0x00},
> }
> })
I'm still none the wiser about what this actually is. How is new _DSD crud
supposed to be documented?
Will
^ permalink raw reply
* [PATCH 2/6] dt-bindings: media: rcar-vin: Document data-active
From: Rob Herring @ 2018-05-23 16:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526488352-898-3-git-send-email-jacopo+renesas@jmondi.org>
On Wed, May 16, 2018 at 06:32:28PM +0200, Jacopo Mondi wrote:
> Document 'data-active' property in R-Car VIN device tree bindings.
> The property is optional when running with explicit synchronization
> (eg. BT.601) but mandatory when embedded synchronization is in use (eg.
> BT.656) as specified by the hardware manual.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Documentation/devicetree/bindings/media/rcar_vin.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index c53ce4e..17eac8a 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -63,6 +63,11 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> If both HSYNC and VSYNC polarities are not specified, embedded
> synchronization is selected.
>
> + - data-active: active state of data enable signal (CLOCKENB pin).
> + 0/1 for LOW/HIGH respectively. If not specified, use HSYNC as
> + data enable signal. When using embedded synchronization this
> + property is mandatory.
This doesn't match the common property's definition which AIUI is for
the data lines' polarity. You need a new property. Perhaps a common one.
> +
> - port 1 - sub-nodes describing one or more endpoints connected to
> the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> use the following schema.
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 5/6] ARM: dts: rcar-gen2: Remove unused VIN properties
From: Rob Herring @ 2018-05-23 16:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180516221307.GF17948@bigcity.dyn.berto.se>
On Thu, May 17, 2018 at 12:13:07AM +0200, Niklas S?derlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2018-05-16 18:32:31 +0200, Jacopo Mondi wrote:
> > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN
> > driver and only confuse users. Remove them in all Gen2 SoC that used
> > them.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > arch/arm/boot/dts/r8a7790-lager.dts | 3 ---
> > arch/arm/boot/dts/r8a7791-koelsch.dts | 3 ---
> > arch/arm/boot/dts/r8a7791-porter.dts | 1 -
> > arch/arm/boot/dts/r8a7793-gose.dts | 3 ---
> > arch/arm/boot/dts/r8a7794-alt.dts | 1 -
> > arch/arm/boot/dts/r8a7794-silk.dts | 1 -
> > 6 files changed, 12 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> > index 063fdb6..b56b309 100644
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -873,10 +873,8 @@
> > port {
> > vin0ep2: endpoint {
> > remote-endpoint = <&adv7612_out>;
> > - bus-width = <24>;
>
> I can't really make up my mind if this is a good thing or not. Device
> tree describes the hardware and not what the drivers make use of. And
> the fact is that this bus is 24 bits wide. So I'm not sure we should
> remove these properties. But I would love to hear what others think
> about this.
IMO, this property should only be present when all the pins are not
connected. And by "all", I mean the minimum of what each end of the
graph can support.
Rob
^ permalink raw reply
* [PATCH 0/3] irqchip: meson-gpio: add support for Meson-AXG SoCs
From: Marc Zyngier @ 2018-05-23 16:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0c97c61b-c05b-21cf-c880-80cff04ed6f2@amlogic.com>
On 16/05/18 03:50, Yixun Lan wrote:
> Hi Marc (or anyone else)
>
> On 04/08/18 22:56, Yixun Lan wrote:
>> This series try to add GPIO interrupt controller support for Meson-AXG SoCs.
>> The first patch is a trivial typo fix, I can fold the first two patches
>> together if necessary.
>>
>> Yixun Lan (3):
>> dt-bindings: interrupt-controller: fix the double quotes
>> dt-bindings: interrupt-controller: New binding for Meson-AXG SoC
>> irqchip/meson-gpio: add support for Meson-AXG SoCs
>>
>> .../bindings/interrupt-controller/amlogic,meson-gpio-intc.txt | 11 ++++++-----
>> drivers/irqchip/irq-meson-gpio.c | 5 +++++
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>
>
> please consider this merely a ping..
> will you take this series, or is there anything holding this?
Sure, I'll queue that for 4.18.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties
From: Rob Herring @ 2018-05-23 16:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526488352-898-2-git-send-email-jacopo+renesas@jmondi.org>
On Wed, May 16, 2018 at 06:32:27PM +0200, Jacopo Mondi wrote:
> Describe the optional endpoint properties for endpoint nodes of port at 0
> and port at 1 of the R-Car VIN driver device tree bindings documentation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Documentation/devicetree/bindings/media/rcar_vin.txt | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index a19517e1..c53ce4e 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -53,6 +53,16 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> from external SoC pins described in video-interfaces.txt[1].
> Describing more then one endpoint in port 0 is invalid. Only VIN
> instances that are connected to external pins should have port 0.
> +
> + - Optional properties for endpoint nodes of port at 0:
> + - hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH
> + respectively. Default is active high.
> + - vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH
> + respectively. Default is active high.
> +
> + If both HSYNC and VSYNC polarities are not specified, embedded
> + synchronization is selected.
No need to copy-n-paste from video-interfaces.txt. Just "see
video-interfaces.txt" for the description is fine.
> +
> - port 1 - sub-nodes describing one or more endpoints connected to
> the VIN from local SoC CSI-2 receivers. The endpoint numbers must
> use the following schema.
> @@ -62,6 +72,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> - Endpoint 2 - sub-node describing the endpoint connected to CSI40
> - Endpoint 3 - sub-node describing the endpoint connected to CSI41
>
> + Endpoint nodes of port at 1 do not support any optional endpoint property.
> +
> Device node example for Gen2 platforms
> --------------------------------------
>
> @@ -112,7 +124,6 @@ Board setup example for Gen2 platforms (vin1 composite video input)
>
> vin1ep0: endpoint {
> remote-endpoint = <&adv7180>;
> - bus-width = <8>;
> };
> };
> };
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate
From: Ray Jui @ 2018-05-23 16:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <76d47e02-7a5f-3fc2-3905-cd4aa03ac69c@arm.com>
Hi Robin,
On 5/23/2018 4:48 AM, Robin Murphy wrote:
> On 23/05/18 08:52, Scott Branden wrote:
>>
>>
>> On 18-05-22 04:24 PM, Ray Jui wrote:
>>> 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?
>> Ray - your original code looks correct to me.? Easier to read and less
>> prone to errors as shown in the attempted translation to a single
>> statement.
>
> ????if (<boolean condition>)
> ??????? return true;
> ????else
> ??????? return false;
>
> still looks really dumb, though, and IMO is actually harder to read than
> just "return <boolean condition>;" because it forces you to stop and
> double-check that the logic is, in fact, only doing the obvious thing.
If you can propose a way to modify my original code above to make it
more readable, I'm fine to make the change.
As I mentioned, I don't think the following change proposed by Guenter
will work due to the reason I pointed out:
return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK));
>
> Robin.
>
>
>
> p.s. No thanks for making me remember the mind-boggling horror of
> briefly maintaining part of this legacy codebase... :P
>
> $ grep -r '? true : false' --include=*.cpp . | wc -l
> 951
^ permalink raw reply
* [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Ray Jui @ 2018-05-23 16:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <af81ea74-fb80-11e2-7bdc-d3607bdbd46b@arm.com>
On 5/23/2018 3:57 AM, Robin Murphy wrote:
> On 22/05/18 19:47, Ray Jui wrote:
>> Update the SP805 binding document to add optional 'timeout-sec'
>> devicetree property
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>> ? Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++
>> ? 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>> index edc4f0e..f898a86 100644
>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>> @@ -19,6 +19,8 @@ Required properties:
>> ? Optional properties:
>> ? - interrupts : Should specify WDT interrupt number.
>> +- timeout-sec : Should specify default WDT timeout in seconds. If
>> unset, the
>> +??????????????? default timeout is 30 seconds
>
> According to the SP805 TRM, the default interval is dependent on the
> rate of WDOGCLK, but would typically be a lot longer than that :/
>
> On a related note, anyone have any idea why we seem to have two subtly
> different SP805 bindings defined?
Interesting, I did not even know that until you pointed this out (and
it's funny that I found that I actually reviewed arm,sp805.txt
internally in Broadcom code review).
It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the
other was done by Anup Patel (arm,sp805.txt). Both were merged at the
same time around March 20, 2016: 915c56bc01d6. I'd assume both were sent
out at around the same time.
It sounds like we should definitely remove one of them. Given that
sp805-wdt.txt appears to have more detailed descriptions on the use of
the clocks, should we remove arm,sp805.txt?
Thanks,
Ray
>
> Robin.
>
>> ? Examples:
>>
^ permalink raw reply
* [PATCH v3 12/13] dt-bindings: add binding for px30 power domains
From: Rob Herring @ 2018-05-23 16:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058371-10706-1-git-send-email-zhangqing@rock-chips.com>
On Wed, May 23, 2018 at 02:52:51PM +0800, Elaine Zhang wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
>
> Add binding documentation for the power domains
> found on Rockchip PX30 SoCs.
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> Documentation/devicetree/bindings/soc/rockchip/power_domain.txt | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v3 11/13] dt-bindings: power: add PX30 SoCs header for power-domain
From: Rob Herring @ 2018-05-23 16:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058341-10658-1-git-send-email-zhangqing@rock-chips.com>
On Wed, May 23, 2018 at 02:52:21PM +0800, Elaine Zhang wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
>
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/px30-power.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 include/dt-bindings/power/px30-power.h
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v3 08/13] dt-bindings: power: add RK3228 SoCs header for power-domain
From: Rob Herring @ 2018-05-23 16:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058286-10503-1-git-send-email-zhangqing@rock-chips.com>
On Wed, May 23, 2018 at 02:51:26PM +0800, Elaine Zhang wrote:
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
> include/dt-bindings/power/rk3228-power.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 include/dt-bindings/power/rk3228-power.h
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Rob Herring @ 2018-05-23 16:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6d03576cf90f06afb1194301cb41fc31704def1d.1527040878.git.collinsd@codeaurora.org>
On Tue, May 22, 2018 at 07:43:17PM -0700, David Collins wrote:
> 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
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH] mtd: atmel-quadspi: add suspend/resume hooks
From: Claudiu Beznea @ 2018-05-23 16:08 UTC (permalink / raw)
To: linux-arm-kernel
Implement suspend/resume hooks.
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
drivers/mtd/spi-nor/atmel-quadspi.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c b/drivers/mtd/spi-nor/atmel-quadspi.c
index 6c5708bacad8..85d7610fb920 100644
--- a/drivers/mtd/spi-nor/atmel-quadspi.c
+++ b/drivers/mtd/spi-nor/atmel-quadspi.c
@@ -737,6 +737,28 @@ static int atmel_qspi_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+static int atmel_qspi_suspend(struct device *dev)
+{
+ struct atmel_qspi *aq = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(aq->clk);
+
+ return 0;
+}
+
+static int atmel_qspi_resume(struct device *dev)
+{
+ struct atmel_qspi *aq = dev_get_drvdata(dev);
+
+ clk_prepare_enable(aq->clk);
+
+ return atmel_qspi_init(aq);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_qspi_pm_ops, atmel_qspi_suspend,
+ atmel_qspi_resume);
static const struct of_device_id atmel_qspi_dt_ids[] = {
{ .compatible = "atmel,sama5d2-qspi" },
@@ -749,6 +771,7 @@ static struct platform_driver atmel_qspi_driver = {
.driver = {
.name = "atmel_qspi",
.of_match_table = atmel_qspi_dt_ids,
+ .pm = &atmel_qspi_pm_ops,
},
.probe = atmel_qspi_probe,
.remove = atmel_qspi_remove,
--
2.7.4
^ permalink raw reply related
* [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Mark Brown @ 2018-05-23 15:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=XWVOdJ8rWJnBTwoNqnYqYRYjknkZtUvT86+skbrTdwig@mail.gmail.com>
On Wed, May 23, 2018 at 08:50:22AM -0700, Doug Anderson wrote:
> On Wed, May 23, 2018 at 8:40 AM, Mark Brown <broonie@kernel.org> wrote:
> > It's got to be valid to think about the voltage of a disabled regulator
> > since drivers want to be able make sure that the regulator gets enabled
> > with a sensible config. With most hardware this is really easy since
> > you can just look at the status reported by the hardware but the RPM
> > makes this hard since there's so much write only stuff in there.
> I should be more clear. Certainly it should be valid to set the
> voltage before enabling it so, as you said, the regulator turns on at
> the right voltage. I'm saying that it's weird (to me) to expect that
> setting the voltage for a regulator that a client thinks is disabled
> will affect any real voltages in the system until the regulator is
> enabled. In RPMh apparently setting a voltage of a regulator you
> think is disabled can affect the regulator output if another client
> (unbeknownst to you) happens to have it enabled.
Yes, that's definitely not what's expected but it's unfortunately what
the firmware chose to implement so we may well be stuck with it
unfortunately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180523/59d677fb/attachment.sig>
^ permalink raw reply
* [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-23 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180523154057.GL4828@sirena.org.uk>
Hi,
On Wed, May 23, 2018 at 8:40 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 23, 2018 at 08:23:22AM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, May 23, 2018 at 1:29 AM, Mark Brown <broonie@kernel.org> wrote:
>>
>> > It's arguable either way - you could say that the client gets to specify
>> > a safe range at all times or you could say that the machine constraints
>> > should cover all cases where the hardware is idling. Of course RPMh
>> > is missing anything like the machine constraints (as we can see from all
>> > the fixing up of undesirable hard coding we have to do) so it's kind of
>> > pushed towards the first case.
>
>> OK, fair enough. If others all agree that it's OK to make requests
>> about the voltage of a disabled regulator then I won't stand in the
>> way. I guess it does call into question the whole idea of caching /
>> not sending the voltage until the first enable because it means
>> there's no way for the client to use this feature until they've
>> enabled / disabled the regulator once. If you think of it as invalid
>> to adjust the voltage of a disabled regulator then the caching
>> argument is super clean, but once you say that you should normally be
>
> It's got to be valid to think about the voltage of a disabled regulator
> since drivers want to be able make sure that the regulator gets enabled
> with a sensible config. With most hardware this is really easy since
> you can just look at the status reported by the hardware but the RPM
> makes this hard since there's so much write only stuff in there.
I should be more clear. Certainly it should be valid to set the
voltage before enabling it so, as you said, the regulator turns on at
the right voltage. I'm saying that it's weird (to me) to expect that
setting the voltage for a regulator that a client thinks is disabled
will affect any real voltages in the system until the regulator is
enabled. In RPMh apparently setting a voltage of a regulator you
think is disabled can affect the regulator output if another client
(unbeknownst to you) happens to have it enabled.
^ 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