* Re: [PATCH] arm64/cache: fix -Woverride-init compiler warnings
From: Will Deacon @ 2019-08-05 14:01 UTC (permalink / raw)
To: Qian Cai; +Cc: Mark Rutland, Catalin Marinas, linux-kernel, linux-arm-kernel
In-Reply-To: <771C4B4C-6D79-419D-9778-5DE1BFC67FBE@lca.pw>
On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote:
>
>
> > On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote:
> >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> >> VIVT I-caches") introduced some compiation warnings from GCC,
> >>
> >> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VIPT] = "VIPT",
> >> ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> >> 'icache_policy_str[2]')
> >> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_PIPT] = "PIPT",
> >> ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> >> 'icache_policy_str[3]')
> >> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VPIPT] = "VPIPT",
> >> ^~~~~~~
> >> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> >> 'icache_policy_str[0]')
> >>
> >> because it initializes icache_policy_str[0 ... 3] twice.
> >>
> >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >> arch/arm64/kernel/cpuinfo.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >> index 876055e37352..193b38da8d96 100644
> >> --- a/arch/arm64/kernel/cpuinfo.c
> >> +++ b/arch/arm64/kernel/cpuinfo.c
> >> @@ -34,10 +34,10 @@
> >> static struct cpuinfo_arm64 boot_cpu_data;
> >>
> >> static char *icache_policy_str[] = {
> >> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
> >> + [ICACHE_POLICY_VPIPT] = "VPIPT",
> >> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN",
> >> [ICACHE_POLICY_VIPT] = "VIPT",
> >> [ICACHE_POLICY_PIPT] = "PIPT",
> >> - [ICACHE_POLICY_VPIPT] = "VPIPT",
> >
> > I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a
> > useful idiom and I think the code is more error-prone the way you have
> > restructured it.
> >
> > Why are you passing -Woverride-init to the compiler anyway? There's only
> > one Makefile that references that option, and it's specific to a pinctrl
> > driver.
>
> Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful
> to catch potential developer mistakes with unintented double-initializations. It is normal to
> start to fix the most of false-positives first before globally enabling the flag by default just like
> “-Wimplicit-fallthrough” mentioned in,
>
> https://lwn.net/Articles/794944/
I think this case is completely different to the implicit fallthrough stuff.
The solution there was simply to add a comment without restructuring the
surrounding code. What your patch does here is actively make the code harder
to understand.
Initialising a static array with a non-zero pattern is a useful idiom and I
don't think we should throw that away just to appease a silly compiler
warning that appears only with non-default build options. Have a look at
the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3] tracing: Function stack size and its name mismatch in arm64
From: Steven Rostedt @ 2019-08-05 13:59 UTC (permalink / raw)
To: Will Deacon
Cc: Jiping Ma, catalin.marinas, will.deacon, linux-kernel,
takahiro.akashi, mingo, Joel Fernandes, kernel-team,
linux-arm-kernel
In-Reply-To: <20190805112524.ajlmouutqckwpqyd@willie-the-truck>
On Mon, 5 Aug 2019 12:25:25 +0100
Will Deacon <will@kernel.org> wrote:
> So, I completely agree with Steve that we shouldn't be littering the core code
> with #ifdef CONFIG_ARM64, but we probably do need something in the arch backend
> if we're going to do this properly, and that in turn is likely to need a very
> different algorithm from the one currently in use.
And I'm perfectly fine if someone wants to tear up the current
implementation, and make one that has callbacks to arch code (with
defaults for those that don't need it).
But it has to be clean. We worked hard to get rid of most "#ifdef"
being scattered in the kernel, and I'm not bringing it back in my code.
-- Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64/efi: fix variable 'si' set but not used
From: Dave Martin @ 2019-08-05 13:53 UTC (permalink / raw)
To: Qian Cai
Cc: linux-efi, ard.biesheuvel, catalin.marinas, linux-kernel, will,
linux-arm-kernel
In-Reply-To: <1564521828-4528-1-git-send-email-cai@lca.pw>
On Tue, Jul 30, 2019 at 05:23:48PM -0400, Qian Cai wrote:
> GCC throws out this warning on arm64.
>
> drivers/firmware/efi/libstub/arm-stub.c: In function 'efi_entry':
> drivers/firmware/efi/libstub/arm-stub.c:132:22: warning: variable 'si'
> set but not used [-Wunused-but-set-variable]
>
> Fix it by making free_screen_info() a static inline function.
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> arch/arm64/include/asm/efi.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e79ce9c3f5c..76a144702586 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -105,7 +105,11 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>
> #define alloc_screen_info(x...) &screen_info
> -#define free_screen_info(x...)
> +
> +static inline void free_screen_info(efi_system_table_t *sys_table_arg,
> + struct screen_info *si)
> +{
> +}
Is this issue caused by the EFI stub being built with non-default
CFLAGS?
The toplevel Makefile specifies -Wno-unused-but-set-variable, which
would silence this warning.
It's debatable whether calling an empty inline function "uses" the
arguments, so I think your patch only silences the warning by accident:
different GCC versions, or clang, might still warn.
I wonder if we're missing any other options that would make sense for
the EFI stub.
[...]
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 05/10] rtc: mt6397: add compatible for mt6323
From: Alexandre Belloni @ 2019-08-05 13:50 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
linux-mediatek, Jonathan Cameron, Matthias Brugger,
Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
David S. Miller
In-Reply-To: <20190729174154.4335-6-frank-w@public-files.de>
On 29/07/2019 19:41:49+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
>
> use mt6397 rtc driver also for mt6323
> compatible needed because of different rtc-base/size see part 7
I still don't think this is a good reason so I would remove that
sentence. Also, it refers to part 7 which is an info that will not be
available when looking at the git history.
>
> changes since v2: splitted this from v2.3 suggested-by Alexandre Belloni
>
Changelog after the --- marker please.
> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> drivers/rtc/rtc-mt6397.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 9370b7fc9f81..21cd9cc8b4c7 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -325,6 +325,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
> mt6397_rtc_resume);
>
> static const struct of_device_id mt6397_rtc_of_match[] = {
> + { .compatible = "mediatek,mt6323-rtc", },
> { .compatible = "mediatek,mt6397-rtc", },
> { }
> };
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 03/10] rtc: mt6397: move some common definitions into rtc.h
From: Alexandre Belloni @ 2019-08-05 13:48 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
linux-mediatek, Jonathan Cameron, Matthias Brugger,
Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
David S. Miller
In-Reply-To: <20190729174154.4335-4-frank-w@public-files.de>
On 29/07/2019 19:41:47+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
>
> move code to separate header-file to reuse definitions later
> in poweroff-driver (drivers/power/reset/mt6323-poweroff.c)
>
> changes since v2: add missing commit-message
The changelog should be after the --- marker.
>
> Suggested-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> drivers/rtc/rtc-mt6397.c | 55 +-------------------------
> include/linux/mfd/mt6397/rtc.h | 71 ++++++++++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+), 54 deletions(-)
> create mode 100644 include/linux/mfd/mt6397/rtc.h
>
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index b46ed4dc7015..c08ee5edf865 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -9,60 +9,7 @@
> #include <linux/module.h>
> #include <linux/regmap.h>
> #include <linux/rtc.h>
> -#include <linux/irqdomain.h>
> -#include <linux/platform_device.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
> -#include <linux/io.h>
> -#include <linux/mfd/mt6397/core.h>
> -
> -#define RTC_BBPU 0x0000
> -#define RTC_BBPU_CBUSY BIT(6)
> -
> -#define RTC_WRTGR 0x003c
> -
> -#define RTC_IRQ_STA 0x0002
> -#define RTC_IRQ_STA_AL BIT(0)
> -#define RTC_IRQ_STA_LP BIT(3)
> -
> -#define RTC_IRQ_EN 0x0004
> -#define RTC_IRQ_EN_AL BIT(0)
> -#define RTC_IRQ_EN_ONESHOT BIT(2)
> -#define RTC_IRQ_EN_LP BIT(3)
> -#define RTC_IRQ_EN_ONESHOT_AL (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> -
> -#define RTC_AL_MASK 0x0008
> -#define RTC_AL_MASK_DOW BIT(4)
> -
> -#define RTC_TC_SEC 0x000a
> -/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> -#define RTC_OFFSET_SEC 0
> -#define RTC_OFFSET_MIN 1
> -#define RTC_OFFSET_HOUR 2
> -#define RTC_OFFSET_DOM 3
> -#define RTC_OFFSET_DOW 4
> -#define RTC_OFFSET_MTH 5
> -#define RTC_OFFSET_YEAR 6
> -#define RTC_OFFSET_COUNT 7
> -
> -#define RTC_AL_SEC 0x0018
> -
> -#define RTC_PDN2 0x002e
> -#define RTC_PDN2_PWRON_ALARM BIT(4)
> -
> -#define RTC_MIN_YEAR 1968
> -#define RTC_BASE_YEAR 1900
> -#define RTC_NUM_YEARS 128
> -#define RTC_MIN_YEAR_OFFSET (RTC_MIN_YEAR - RTC_BASE_YEAR)
> -
> -struct mt6397_rtc {
> - struct device *dev;
> - struct rtc_device *rtc_dev;
> - struct mutex lock;
> - struct regmap *regmap;
> - int irq;
> - u32 addr_base;
> -};
> +#include <linux/mfd/mt6397/rtc.h>
>
> static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> {
> diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> new file mode 100644
> index 000000000000..b702c29e8c74
> --- /dev/null
> +++ b/include/linux/mfd/mt6397/rtc.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2014-2018 MediaTek Inc.
> + *
> + * Author: Tianping.Fang <tianping.fang@mediatek.com>
> + * Sean Wang <sean.wang@mediatek.com>
> + */
> +
> +#ifndef _LINUX_MFD_MT6397_RTC_H_
> +#define _LINUX_MFD_MT6397_RTC_H_
> +
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define RTC_BBPU 0x0000
> +#define RTC_BBPU_CBUSY BIT(6)
> +#define RTC_BBPU_KEY (0x43 << 8)
> +
> +#define RTC_WRTGR 0x003c
> +
> +#define RTC_IRQ_STA 0x0002
> +#define RTC_IRQ_STA_AL BIT(0)
> +#define RTC_IRQ_STA_LP BIT(3)
> +
> +#define RTC_IRQ_EN 0x0004
> +#define RTC_IRQ_EN_AL BIT(0)
> +#define RTC_IRQ_EN_ONESHOT BIT(2)
> +#define RTC_IRQ_EN_LP BIT(3)
> +#define RTC_IRQ_EN_ONESHOT_AL (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> +
> +#define RTC_AL_MASK 0x0008
> +#define RTC_AL_MASK_DOW BIT(4)
> +
> +#define RTC_TC_SEC 0x000a
> +/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> +#define RTC_OFFSET_SEC 0
> +#define RTC_OFFSET_MIN 1
> +#define RTC_OFFSET_HOUR 2
> +#define RTC_OFFSET_DOM 3
> +#define RTC_OFFSET_DOW 4
> +#define RTC_OFFSET_MTH 5
> +#define RTC_OFFSET_YEAR 6
> +#define RTC_OFFSET_COUNT 7
> +
> +#define RTC_AL_SEC 0x0018
> +
> +#define RTC_PDN2 0x002e
> +#define RTC_PDN2_PWRON_ALARM BIT(4)
> +
> +#define RTC_MIN_YEAR 1968
> +#define RTC_BASE_YEAR 1900
> +#define RTC_NUM_YEARS 128
> +#define RTC_MIN_YEAR_OFFSET (RTC_MIN_YEAR - RTC_BASE_YEAR)
> +
> +#define MTK_RTC_POLL_DELAY_US 10
> +#define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> +
> +struct mt6397_rtc {
> + struct device *dev;
> + struct rtc_device *rtc_dev;
> +
> + /* Protect register access from multiple tasks */
> + struct mutex lock;
> + struct regmap *regmap;
> + int irq;
> + u32 addr_base;
> +};
> +
> +#endif /* _LINUX_MFD_MT6397_RTC_H_ */
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 02/10] dt-bindings: add missing mt6397 rtc
From: Alexandre Belloni @ 2019-08-05 13:47 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Kate Stewart, Mark Rutland, linux-kernel, Richard Fontana,
Mauro Carvalho Chehab, Lee Jones, linux-rtc, Allison Randal,
devicetree, linux-pm, Sean Wang, Tianping . Fang, Rob Herring,
linux-mediatek, Jonathan Cameron, Matthias Brugger,
Thomas Gleixner, Eddie Huang, linux-arm-kernel, Alessandro Zummo,
Josef Friedl, Greg Kroah-Hartman, Sebastian Reichel,
David S. Miller
In-Reply-To: <20190729174154.4335-3-frank-w@public-files.de>
On 29/07/2019 19:41:46+0200, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
>
> add missing devicetree-binding document for mt6397 rtc
> in later patch driver is extended with mt6323 chip
>
> changes since v2: splitted rtc-mt6397.txt from first patch
This line should be after the --- marker.
>
> Suggested-By: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
Your SoB should appear last.
> Signed-off-by: Josef Friedl <josef.friedl@speed.at>
> ---
> .../devicetree/bindings/rtc/rtc-mt6397.txt | 29 +++++++++++++++++++
> 1 file changed, 29 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
>
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> new file mode 100644
> index 000000000000..ebd1cf80dcc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> @@ -0,0 +1,29 @@
> +Device-Tree bindings for MediaTek PMIC based RTC
> +
> +MediaTek PMIC based RTC is an independent function of MediaTek PMIC that works
> +as a type of multi-function device (MFD). The RTC can be configured and set up
> +with PMIC wrapper bus which is a common resource shared with the other
> +functions found on the same PMIC.
> +
> +For MediaTek PMIC MFD bindings, see:
> +Documentation/devicetree/bindings/mfd/mt6397.txt
> +
> +For MediaTek PMIC wrapper bus bindings, see:
> +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +
> +Required properties:
> +- compatible: Should be one of follows
> + "mediatek,mt6323-rtc": for MT6323 PMIC
> + "mediatek,mt6397-rtc": for MT6397 PMIC
> +
> +Example:
> +
> + pmic {
> + compatible = "mediatek,mt6323";
> +
> + ...
> +
> + rtc {
> + compatible = "mediatek,mt6323-rtc";
> + };
> + };
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Dave Martin @ 2019-08-05 13:39 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: Tri Vo, linux, linux-kernel, clang-built-linux, Robin Murphy,
linux-arm-kernel
In-Reply-To: <CAJkfWY6StuyMuKG7XdEJrqMsA_Xy02QZKp8r0K2jwSZwBCt+9Q@mail.gmail.com>
On Fri, Aug 02, 2019 at 10:27:30AM -0700, Nathan Huckleberry wrote:
> You're right. Would pushing an extra register be an adequate fix?
Would forcing CONFIG_ARM_UNWIND=y for clang work as an alternative to
this?
Assuming clang supports -funwind-tables or equivalent, this may just
work.
[...]
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 2/2] mmc: Add support for the ASPEED SD controller
From: Adrian Hunter @ 2019-08-05 13:36 UTC (permalink / raw)
To: Andrew Jeffery, linux-mmc
Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed,
ryanchen.aspeed, linux-kernel, robh+dt, joel, linux-arm-kernel
In-Reply-To: <20190805025155.9020-3-andrew@aj.id.au>
On 5/08/19 5:51 AM, Andrew Jeffery wrote:
> Add a minimal driver for ASPEED's SD controller, which exposes two
> SDHCIs.
>
> The ASPEED design implements a common register set for the SDHCIs, and
> moves some of the standard configuration elements out to this common
> area (e.g. 8-bit mode, and card detect configuration which is not
> currently supported).
>
> The SD controller has a dedicated hardware interrupt that is shared
> between the slots. The common register set exposes information on which
> slot triggered the interrupt; early revisions of the patch introduced an
> irqchip for the register, but reality is it doesn't behave as an
> irqchip, and the result fits awkwardly into the irqchip APIs. Instead
> I've taken the simple approach of using the IRQ as a shared IRQ with
> some minor performance impact for the second slot.
>
> Ryan was the original author of the patch - I've taken his work and
> massaged it to drop the irqchip support and rework the devicetree
> integration. The driver has been smoke tested under qemu against a
> minimal SD controller model and lightly tested on an ast2500-evb.
>
> Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
One minor comment otherwise:
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>
> ---
> v3: No change
> v2:
> * Add AST2600 compatible
> * Drop SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN
> * Ensure slot number is valid
> * Fix build with CONFIG_MODULES
> * Fix module license string
> * Non-PCI devices won't die
> * Rename aspeed_sdc_configure_8bit_mode()
> * Rename aspeed_sdhci_pdata
> * Switch to sdhci_enable_clk()
> * Use PTR_ERR() on the right `struct platform_device *`
> ---
> drivers/mmc/host/Kconfig | 12 ++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/sdhci-of-aspeed.c | 328 +++++++++++++++++++++++++++++
> 3 files changed, 341 insertions(+)
> create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 14d89a108edd..0f8a230de2f3 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN
>
> If unsure, say N.
>
> +config MMC_SDHCI_OF_ASPEED
> + tristate "SDHCI OF support for the ASPEED SDHCI controller"
> + depends on MMC_SDHCI_PLTFM
> + depends on OF
> + help
> + This selects the ASPEED Secure Digital Host Controller Interface.
> +
> + If you have a controller with this interface, say Y or M here. You
> + also need to enable an appropriate bus interface.
> +
> + If unsure, say N.
> +
> config MMC_SDHCI_OF_AT91
> tristate "SDHCI OF support for the Atmel SDMMC controller"
> depends on MMC_SDHCI_PLTFM
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 73578718f119..390ee162fe71 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
> obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o
> obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o
> obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o
> +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o
> obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o
> obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o
> obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> new file mode 100644
> index 000000000000..d31785ec90d7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (C) 2019 ASPEED Technology Inc. */
> +/* Copyright (C) 2019 IBM Corp. */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +#define ASPEED_SDC_INFO 0x00
> +#define ASPEED_SDC_S1MMC8 BIT(25)
> +#define ASPEED_SDC_S0MMC8 BIT(24)
> +
> +struct aspeed_sdc {
> + struct clk *clk;
> + struct resource *res;
> +
> + spinlock_t lock;
> + void __iomem *regs;
> +};
> +
> +struct aspeed_sdhci {
> + struct aspeed_sdc *parent;
> + u32 width_mask;
> +};
> +
> +static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> + struct aspeed_sdhci *sdhci,
> + bool bus8)
> +{
> + u32 info;
> +
> + /* Set/clear 8 bit mode */
> + spin_lock(&sdc->lock);
> + info = readl(sdc->regs + ASPEED_SDC_INFO);
> + if (bus8)
> + info |= sdhci->width_mask;
> + else
> + info &= ~sdhci->width_mask;
> + writel(info, sdc->regs + ASPEED_SDC_INFO);
> + spin_unlock(&sdc->lock);
> +}
> +
> +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> + int div;
> + u16 clk;
> +
> + if (clock == host->clock)
> + return;
> +
> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> + if (clock == 0)
> + goto out;
> +
> + for (div = 1; div < 256; div *= 2) {
> + if ((host->max_clk / div) <= clock)
> + break;
> + }
> + div >>= 1;
> +
> + clk = div << SDHCI_DIVIDER_SHIFT;
> +
> + sdhci_enable_clk(host, clk);
> +
> +out:
> + host->clock = clock;
> +}
> +
> +static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width)
> +{
> + struct sdhci_pltfm_host *pltfm_priv;
> + struct aspeed_sdhci *aspeed_sdhci;
> + struct aspeed_sdc *aspeed_sdc;
> + u8 ctrl;
> +
> + pltfm_priv = sdhci_priv(host);
> + aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv);
> + aspeed_sdc = aspeed_sdhci->parent;
> +
> + /* Set/clear 8-bit mode */
> + aspeed_sdc_configure_8bit_mode(aspeed_sdc, aspeed_sdhci,
> + width == MMC_BUS_WIDTH_8);
> +
> + /* Set/clear 1 or 4 bit mode */
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> + if (width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +}
> +
> +static const struct sdhci_ops aspeed_sdhci_ops = {
> + .set_clock = aspeed_sdhci_set_clock,
> + .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> + .set_bus_width = aspeed_sdhci_set_bus_width,
> + .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
> + .reset = sdhci_reset,
> + .set_uhs_signaling = sdhci_set_uhs_signaling,
> +};
> +
> +static const struct sdhci_pltfm_data aspeed_sdhci_pdata = {
> + .ops = &aspeed_sdhci_ops,
> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> + struct resource *res)
> +{
> + resource_size_t delta;
> +
> + if (!res || resource_type(res) != IORESOURCE_MEM)
> + return -EINVAL;
> +
> + if (res->start < dev->parent->res->start)
> + return -EINVAL;
> +
> + delta = res->start - dev->parent->res->start;
> + if (delta & (0x100 - 1))
> + return -EINVAL;
> +
> + return (delta / 0x100) - 1;
> +}
> +
> +static int aspeed_sdhci_probe(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct aspeed_sdhci *dev;
> + struct sdhci_host *host;
> + struct resource *res;
> + int slot;
> + int ret;
> +
> + host = sdhci_pltfm_init(pdev, &aspeed_sdhci_pdata, sizeof(*dev));
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + dev = sdhci_pltfm_priv(pltfm_host);
> + dev->parent = dev_get_drvdata(pdev->dev.parent);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + slot = aspeed_sdhci_calculate_slot(dev, res);
> +
> + if (slot < 0)
> + return slot;
> + else if (slot >= 2)
> + return -EINVAL;
> +
> + dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> +
> + sdhci_get_of_property(pdev);
> +
> + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pltfm_host->clk))
> + return PTR_ERR(pltfm_host->clk);
> +
> + ret = clk_prepare_enable(pltfm_host->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable SDIO clock\n");
> + goto err_pltfm_free;
> + }
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
> + goto err_sdhci_add;
> +
> + ret = sdhci_add_host(host);
> + if (ret)
> + goto err_sdhci_add;
> +
> + return 0;
> +
> +err_sdhci_add:
> + clk_disable_unprepare(pltfm_host->clk);
> +err_pltfm_free:
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}
> +
> +static int aspeed_sdhci_remove(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + int dead = 0;
> +
> + host = platform_get_drvdata(pdev);
> + pltfm_host = sdhci_priv(host);
> +
> + sdhci_remove_host(host, dead);
> +
> + clk_disable_unprepare(pltfm_host->clk);
> +
> + sdhci_pltfm_free(pdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sdhci_of_match[] = {
> + { .compatible = "aspeed,ast2400-sdhci", },
> + { .compatible = "aspeed,ast2500-sdhci", },
> + { .compatible = "aspeed,ast2600-sdhci", },
> + { }
> +};
> +
> +static struct platform_driver aspeed_sdhci_driver = {
> + .driver = {
> + .name = "sdhci-aspeed",
> + .of_match_table = aspeed_sdhci_of_match,
> + },
> + .probe = aspeed_sdhci_probe,
> + .remove = aspeed_sdhci_remove,
> +};
> +
> +static int aspeed_sdc_probe(struct platform_device *pdev)
> +
> +{
> + struct device_node *parent, *child;
> + struct aspeed_sdc *sdc;
> + int ret;
> +
> + sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> + if (!sdc)
> + return -ENOMEM;
> +
> + spin_lock_init(&sdc->lock);
> +
> + sdc->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(sdc->clk))
> + return PTR_ERR(sdc->clk);
> +
> + ret = clk_prepare_enable(sdc->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable SDCLK\n");
> + return ret;
> + }
> +
> + sdc->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sdc->regs = devm_ioremap_resource(&pdev->dev, sdc->res);
> + if (IS_ERR(sdc->regs)) {
> + ret = PTR_ERR(sdc->regs);
> + goto err_clk;
> + }
> +
> + dev_set_drvdata(&pdev->dev, sdc);
> +
> + parent = pdev->dev.of_node;
> + for_each_available_child_of_node(parent, child) {
> + struct platform_device *cpdev;
> +
> + cpdev = of_platform_device_create(child, NULL, &pdev->dev);
> + if (IS_ERR(cpdev)) {
> + of_node_put(child);
> + ret = PTR_ERR(cpdev);
> + goto err_clk;
> + }
> + }
> +
> + return 0;
> +
> +err_clk:
> + clk_disable_unprepare(sdc->clk);
> + return ret;
> +}
> +
> +static int aspeed_sdc_remove(struct platform_device *pdev)
> +{
> + struct aspeed_sdc *sdc = dev_get_drvdata(&pdev->dev);
> +
> + clk_disable_unprepare(sdc->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sdc_of_match[] = {
> + { .compatible = "aspeed,ast2400-sd-controller", },
> + { .compatible = "aspeed,ast2500-sd-controller", },
> + { .compatible = "aspeed,ast2600-sd-controller", },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
> +
> +static struct platform_driver aspeed_sdc_driver = {
> + .driver = {
> + .name = "sd-controller-aspeed",
> + .pm = &sdhci_pltfm_pmops,
> + .of_match_table = aspeed_sdc_of_match,
> + },
> + .probe = aspeed_sdc_probe,
> + .remove = aspeed_sdc_remove,
> +};
> +
> +static int __init aspeed_sdc_init(void)
> +{
> + int rc;
> +
> + rc = platform_driver_register(&aspeed_sdhci_driver);
> + if (rc < 0)
> + return rc;
> +
> + return platform_driver_register(&aspeed_sdc_driver);
Shouldn't aspeed_sdhci_driver be unregistered here if there was an error.
> +}
> +module_init(aspeed_sdc_init);
> +
> +static void __exit aspeed_sdc_exit(void)
> +{
> + platform_driver_unregister(&aspeed_sdc_driver);
> + platform_driver_unregister(&aspeed_sdhci_driver);
> +}
> +module_exit(aspeed_sdc_exit);
> +
> +MODULE_DESCRIPTION("Driver for the ASPEED SD/SDIO/SDHCI Controllers");
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
> +MODULE_LICENSE("GPL");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol
From: Sudeep Holla @ 2019-08-05 13:28 UTC (permalink / raw)
To: Etienne Carriere
Cc: Peng Fan, Souvik Chakravarty, wesleys, Ionela Voinescu, aidapala,
Sudeep Holla, linux-kernel, Saeed Nowshadi, Quentin Perret,
Bo Zhang, Felix Burton, Jim Quinlan, Chris Redpath, pajay,
Gaku Inami, Volodymyr Babchuk, linux-arm-kernel
In-Reply-To: <CAN5uoS-dgtz55O+AAxEFkgtijpHj_-NSy7SkNRAkhEJN5v4PWA@mail.gmail.com>
On Mon, Aug 05, 2019 at 03:16:41PM +0200, Etienne Carriere wrote:
> On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > SCMI v2.0 adds support for "FastChannel" which do not use a message
> > header as they are specialized for a single message.
> >
> > Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> > commands are supported over fastchannels. As they are optional, they
> > need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> > Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
> >
> > Add support for making use of these fastchannels.
> >
> > Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> > Cc: Chris Redpath <Chris.Redpath@arm.com>
> > Cc: Quentin Perret <Quentin.Perret@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> > 1 file changed, 100 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index 6cce3e82e81e..b9144efbd6fb 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -8,6 +8,7 @@
> > #include <linux/bits.h>
> > #include <linux/of.h>
> > #include <linux/io.h>
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_opp.h>
> > #include <linux/sort.h>
> > @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> > return ret;
> > }
> >
> > -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> > +#define SCMI_PERF_FC_RING_DB(doorbell, w) \
>
> Suggest to reformat into a macro style:
> #define SCMI_PERF_FC_RING_DB(doorbell, w) \
> do { \
> (...) \
> } while (0)
>
Sure I can try that.
> > +{ \
> > + u##w val = 0; \
> > + struct scmi_fc_db_info *db = doorbell; \
> > + \
> > + if ((db)->mask) \
>
> remove parentheses. I would say, could simply remove `if (db->mask)` here.
Ah, missed to drop this one. We can avoid reading the value if mask = 0,
so I prefer to keep it.
>
> > + val = ioread##w(db->addr) & db->mask; \
> > + iowrite##w((u##w)db->set | val, db->addr); \
> > +}
> > +
> > +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> > +{
> > + if (!db || !db->addr)
> > + return;
> > +
> > + if (db->width == 1)
> > + SCMI_PERF_FC_RING_DB(db, 8)
> > + else if (db->width == 2)
> > + SCMI_PERF_FC_RING_DB(db, 16)
> > + else if (db->width == 4)
> > + SCMI_PERF_FC_RING_DB(db, 32)
> > + else /* db->width == 8 */
> > +#ifdef CONFIG_64BIT
> > + SCMI_PERF_FC_RING_DB(db, 64)
> > +#else
> > + {
> > + u64 val = 0;
> > +
> > + if (db->mask)
> > + val = ioread64_hi_lo(db->addr) & db->mask;
> > + iowrite64_hi_lo(db->set, db->addr);
>
> Is `value` missing here?
> Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
>
I am still using it. Just if CONFIG_64BIT is enabled and io{read,write}64
are defined. ARM32/MIPS and other 32-bit platform build might fail
otherwise. I don't want to restrict SCMI to 64-bit platforms only.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/9] arm64: Stolen time support
From: Marc Zyngier @ 2019-08-05 13:26 UTC (permalink / raw)
To: Steven Price
Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <6789f477-8ab5-cc54-1ad2-8627917b07c9@arm.com>
On 05/08/2019 14:06, Steven Price wrote:
> On 03/08/2019 19:05, Marc Zyngier wrote:
>> On Fri, 2 Aug 2019 15:50:08 +0100
>> Steven Price <steven.price@arm.com> wrote:
>>
>> Hi Steven,
>>
>>> This series add support for paravirtualized time for arm64 guests and
>>> KVM hosts following the specification in Arm's document DEN 0057A:
>>>
>>> https://developer.arm.com/docs/den0057/a
>>>
>>> It implements support for stolen time, allowing the guest to
>>> identify time when it is forcibly not executing.
>>>
>>> It doesn't implement support for Live Physical Time (LPT) as there are
>>> some concerns about the overheads and approach in the above
>>> specification, and I expect an updated version of the specification to
>>> be released soon with just the stolen time parts.
>>
>> Thanks for posting this.
>>
>> My current concern with this series is around the fact that we allocate
>> memory from the kernel on behalf of the guest. It is the first example
>> of such thing in the ARM port, and I can't really say I'm fond of it.
>>
>> x86 seems to get away with it by having the memory allocated from
>> userspace, why I tend to like more. Yes, put_user is more
>> expensive than a straight store, but this isn't done too often either.
>>
>> What is the rational for your current approach?
>
> As I see it there are 3 approaches that can be taken here:
>
> 1. Hypervisor allocates memory and adds it to the virtual machine. This
> means that everything to do with the 'device' is encapsulated behind the
> KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want the
> stolen time structure to be fast it cannot be a trapping region and has
> to be backed by real memory - in this case allocated by the host kernel.
>
> 2. Host user space allocates memory. Similar to above, but this time
> user space needs to manage the memory region as well as the usual
> KVM_CREATE_DEVICE dance. I've no objection to this, but it means
> kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
> to size the memory region).
>
> 3. Guest kernel "donates" the memory to the hypervisor for the
> structure. As far as I'm aware this is what x86 does. The problems I see
> this approach are:
>
> a) kexec becomes much more tricky - there needs to be a disabling
> mechanism for the guest to stop the hypervisor scribbling on memory
> before starting the new kernel.
>
> b) If there is more than one entity that is interested in the
> information (e.g. firmware and kernel) then this requires some form of
> arbitration in the guest because the hypervisor doesn't want to have to
> track an arbitrary number of regions to update.
>
> c) Performance can suffer if the host kernel doesn't have a suitably
> aligned/sized area to use. As you say - put_user() is more expensive.
> The structure is updated on every return to the VM.
>
>
> Of course x86 does prove the third approach can work, but I'm not sure
> which is actually better. Avoid the kexec cancellation requirements was
> the main driver of the current approach. Although many of the
> conversations about this were also tied up with Live Physical Time which
> adds its own complications.
My current train of thoughts is around (2):
- We don't need a new mechanism to track pages or deal with overlapping
IPA ranges
- We can get rid of the save/restore interface
The drawback is that the amount of memory required per vcpu becomes ABI.
I don't think that's a huge deal, as the hypervisor has the same
contract with the guest.
We also take a small hit with put_user(), but this is only done as a
consequence of vcpu_load() (and not on every entry as you suggest
above). It'd be worth quantifying this overhead before making any
decision one way or another.
Thanks,
M.
--
Jazz is not dead, it just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 0/16] drm: panel related updates
From: Emil Velikov @ 2019-08-05 13:18 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Neil Armstrong, David Airlie, dri-devel, Thierry Reding,
Laurent Pinchart, Marek Vasut, linux-samsung-soc, Vincent Abriou,
Krzysztof Kozlowski, Jonathan Hunter, Maxime Ripard, Kukjin Kim,
NXP Linux Team, Jonas Karlman, Alison Wang, Gwan-gyeong Mun,
Alexios Zavras, Laurent Pinchart, linux-tegra, Thomas Gleixner,
Sean Paul, Allison Randal, linux-arm-kernel, Jernej Skrabec,
Enrico Weigelt, Seung-Woo Kim, Kyungmin Park,
Pengutronix Kernel Team, Shawn Guo
In-Reply-To: <20190804201637.1240-1-sam@ravnborg.org>
On 2019/08/04, Sam Ravnborg wrote:
> The first 9 patches replaces direct use of the drm_panel
> function pointers with their drm_panel_* counterparts.
> The function pointers are only supposed to be used by
> the drm_panel infrastructure and direct use are discouraged.
>
> ili9322 is updated to handle bus_flags in get_modes like everyone else.
> This is in preparation for a later patch series where controller
> becomes an arugument to get_modes() and not like today where drm_panel
> is attached to a controller.
>
> The remaining patches move functionality to the drm_panel core that
> today are repeated in many drivers.
> As preparation for this the inline functions are moved to drm_panel.c
> and kernel-doc is made inline.
> panel-simple is updated to benefit from the additional infrastructure
> and is an example for the simplifications that can be done.
>
> The patchset has been tested on my embedded target,
> and build tested.
>
> Feedback welcome!
>
> The "fix opencoded" patches are all independent and can be applied
> out of order. They were kept here to keep panel related patches in one series.
>
> Sam
>
Thanks for working on this Sam.
Patches 1-13 are:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
-Emil
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol
From: Etienne Carriere @ 2019-08-05 13:16 UTC (permalink / raw)
To: Sudeep Holla
Cc: Peng Fan, Souvik Chakravarty, wesleys, Ionela Voinescu, aidapala,
linux-kernel, Saeed Nowshadi, Quentin Perret, Bo Zhang,
Felix Burton, Jim Quinlan, Chris Redpath, pajay, Gaku Inami,
Volodymyr Babchuk, linux-arm-kernel
In-Reply-To: <20190726135954.11078-3-sudeep.holla@arm.com>
On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> SCMI v2.0 adds support for "FastChannel" which do not use a message
> header as they are specialized for a single message.
>
> Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> commands are supported over fastchannels. As they are optional, they
> need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
>
> Add support for making use of these fastchannels.
>
> Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> Cc: Chris Redpath <Chris.Redpath@arm.com>
> Cc: Quentin Perret <Quentin.Perret@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> 1 file changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 6cce3e82e81e..b9144efbd6fb 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -8,6 +8,7 @@
> #include <linux/bits.h>
> #include <linux/of.h>
> #include <linux/io.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>
> #include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> #include <linux/sort.h>
> @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> return ret;
> }
>
> -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> +#define SCMI_PERF_FC_RING_DB(doorbell, w) \
Suggest to reformat into a macro style:
#define SCMI_PERF_FC_RING_DB(doorbell, w) \
do { \
(...) \
} while (0)
> +{ \
> + u##w val = 0; \
> + struct scmi_fc_db_info *db = doorbell; \
> + \
> + if ((db)->mask) \
remove parentheses. I would say, could simply remove `if (db->mask)` here.
> + val = ioread##w(db->addr) & db->mask; \
> + iowrite##w((u##w)db->set | val, db->addr); \
> +}
> +
> +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> +{
> + if (!db || !db->addr)
> + return;
> +
> + if (db->width == 1)
> + SCMI_PERF_FC_RING_DB(db, 8)
> + else if (db->width == 2)
> + SCMI_PERF_FC_RING_DB(db, 16)
> + else if (db->width == 4)
> + SCMI_PERF_FC_RING_DB(db, 32)
> + else /* db->width == 8 */
> +#ifdef CONFIG_64BIT
> + SCMI_PERF_FC_RING_DB(db, 64)
> +#else
> + {
> + u64 val = 0;
> +
> + if (db->mask)
> + val = ioread64_hi_lo(db->addr) & db->mask;
> + iowrite64_hi_lo(db->set, db->addr);
Is `value` missing here?
Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
> + }
> +#endif
> +}
> +
> +static int scmi_perf_mb_limits_set(const struct scmi_handle *handle, u32 domain,
> u32 max_perf, u32 min_perf)
> {
> int ret;
> @@ -316,7 +352,23 @@ static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> return ret;
> }
>
> -static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> + u32 max_perf, u32 min_perf)
> +{
> + struct scmi_perf_info *pi = handle->perf_priv;
> + struct perf_dom_info *dom = pi->dom_info + domain;
> +
> + if (dom->fc_info && dom->fc_info->limit_set_addr) {
> + iowrite32(max_perf, dom->fc_info->limit_set_addr);
> + iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
> + scmi_perf_fc_ring_db(dom->fc_info->limit_set_db);
> + return 0;
> + }
> +
> + return scmi_perf_mb_limits_set(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
> u32 *max_perf, u32 *min_perf)
> {
> int ret;
> @@ -342,7 +394,22 @@ static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> return ret;
> }
>
> -static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> + u32 *max_perf, u32 *min_perf)
> +{
> + struct scmi_perf_info *pi = handle->perf_priv;
> + struct perf_dom_info *dom = pi->dom_info + domain;
> +
> + if (dom->fc_info && dom->fc_info->limit_get_addr) {
> + *max_perf = ioread32(dom->fc_info->limit_get_addr);
> + *min_perf = ioread32(dom->fc_info->limit_get_addr + 4);
> + return 0;
> + }
> +
> + return scmi_perf_mb_limits_get(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_level_set(const struct scmi_handle *handle, u32 domain,
> u32 level, bool poll)
> {
> int ret;
> @@ -365,7 +432,22 @@ static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> return ret;
> }
>
> -static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> + u32 level, bool poll)
> +{
> + struct scmi_perf_info *pi = handle->perf_priv;
> + struct perf_dom_info *dom = pi->dom_info + domain;
> +
> + if (dom->fc_info && dom->fc_info->level_set_addr) {
> + iowrite32(level, dom->fc_info->level_set_addr);
> + scmi_perf_fc_ring_db(dom->fc_info->level_set_db);
> + return 0;
> + }
> +
> + return scmi_perf_mb_level_set(handle, domain, level, poll);
> +}
> +
> +static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
> u32 *level, bool poll)
> {
> int ret;
> @@ -387,6 +469,20 @@ static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> return ret;
> }
>
> +static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> + u32 *level, bool poll)
> +{
> + struct scmi_perf_info *pi = handle->perf_priv;
> + struct perf_dom_info *dom = pi->dom_info + domain;
> +
> + if (dom->fc_info && dom->fc_info->level_get_addr) {
> + *level = ioread32(dom->fc_info->level_get_addr);
> + return 0;
> + }
> +
> + return scmi_perf_mb_level_get(handle, domain, level, poll);
> +}
> +
> static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
> {
> if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once
From: Emil Velikov @ 2019-08-05 13:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Neil Armstrong, David Airlie, dri-devel, Thierry Reding,
Sam Ravnborg, Marek Vasut, linux-samsung-soc, Sean Paul,
Allison Randal, Krzysztof Kozlowski, Jonathan Hunter,
Maxime Ripard, Kukjin Kim, NXP Linux Team, Jonas Karlman,
Alison Wang, Gwan-gyeong Mun, Alexios Zavras, Laurent Pinchart,
linux-tegra, Thomas Gleixner, Vincent Abriou, linux-arm-kernel,
Jernej Skrabec, Enrico Weigelt, Seung-Woo Kim, Kyungmin Park,
Pengutronix Kernel Team, Shawn Guo
In-Reply-To: <20190805105928.GI29747@pendragon.ideasonboard.com>
On 2019/08/05, Laurent Pinchart wrote:
> Hi Sam,
>
> Thank you for the patch.
>
> On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
> > Many panel drivers duplicate logic to prevent prepare to be called
> > for a panel that is already prepared.
> > Likewise for enable.
> >
> > Implement this logic in drm_panel so the individual drivers
> > no longer needs this.
> > A panel is considered prepared/enabled only if the prepare/enable call
> > succeeds.
> > For disable/unprepare it is unconditionally considered
> > disabled/unprepared.
> >
> > This allows calls to prepare/enable again, even if there were
> > some issue disabling a regulator or similar during disable/unprepare.
>
> Is this the right place to handle this ? Shouldn't the upper layers
> ensure than enable/disable and prepare/unprepare are correcty balanced,
> and not called multiple times ? Adding enabled and prepared state to
> drm_panel not only doesn't align well with atomic state handling, but
> also would hide issues in upper layers that should really be fixed
> there.
>
Fully agreed. Mistakes happen - hiding them, by returning "success" does
not sound like a wise approach.
HTH
Emil
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/9] KVM: arm64: Implement PV_FEATURES call
From: Steven Price @ 2019-08-05 13:14 UTC (permalink / raw)
To: Marc Zyngier
Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190803122124.7700f700@why>
On 03/08/2019 12:21, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:11 +0100
> Steven Price <steven.price@arm.com> wrote:
>
>> This provides a mechanism for querying which paravirtualized features
>> are available in this hypervisor.
>>
>> Also add the header file which defines the ABI for the paravirtualized
>> clock features we're about to add.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> arch/arm64/include/asm/pvclock-abi.h | 20 ++++++++++++++++++++
>> include/linux/arm-smccc.h | 14 ++++++++++++++
>> virt/kvm/arm/hypercalls.c | 9 +++++++++
>> 3 files changed, 43 insertions(+)
>> create mode 100644 arch/arm64/include/asm/pvclock-abi.h
>>
>> diff --git a/arch/arm64/include/asm/pvclock-abi.h b/arch/arm64/include/asm/pvclock-abi.h
>> new file mode 100644
>> index 000000000000..1f7cdc102691
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/pvclock-abi.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 Arm Ltd. */
>> +
>> +#ifndef __ASM_PVCLOCK_ABI_H
>> +#define __ASM_PVCLOCK_ABI_H
>> +
>> +/* The below structures and constants are defined in ARM DEN0057A */
>> +
>> +struct pvclock_vcpu_stolen_time_info {
>> + __le32 revision;
>> + __le32 attributes;
>> + __le64 stolen_time;
>> + /* Structure must be 64 byte aligned, pad to that size */
>> + u8 padding[48];
>> +} __packed;
>> +
>> +#define PV_VM_TIME_NOT_SUPPORTED -1
>
> Isn't the intent for this to be the same value as
> SMCCC_RET_NOT_SUPPORTED?
Yes it is, I guess there's not much point defining it again.
>> +#define PV_VM_TIME_INVALID_PARAMETERS -2
>
> It overlaps with SMCCC_RET_NOT_REQUIRED. Is that a problem? Should we
> consider a spec change for this?
Actually INVALID_PARAMETERS is only for Live Physical Time, since we're
not implementing it here, this can go as well.
Thanks,
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 07/10] regulator: mt6358: Add support for MT6358 regulator
From: Mark Brown @ 2019-08-05 13:10 UTC (permalink / raw)
To: Hsin-Hsiung Wang
Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, srv_heupstream,
devicetree, Sean Wang, Liam Girdwood, linux-kernel,
Richard Fontana, Rob Herring, linux-mediatek, Allison Randal,
linux-rtc, Matthias Brugger, Thomas Gleixner, Eddie Huang,
Lee Jones, Kate Stewart, linux-arm-kernel
In-Reply-To: <1564982518-32163-8-git-send-email-hsin-hsiung.wang@mediatek.com>
[-- Attachment #1.1: Type: text/plain, Size: 1282 bytes --]
On Mon, Aug 05, 2019 at 01:21:55PM +0800, Hsin-Hsiung Wang wrote:
> +static const u32 vmch_voltages[] = {
> + 2900000, 3000000, 3300000,
> +};
> +static const u32 vemc_voltages[] = {
> + 2900000, 3000000, 3300000,
> +};
Several of these tables appear to be identical.
> +static inline unsigned int mt6358_map_mode(unsigned int mode)
> +{
> + return mode == MT6358_BUCK_MODE_AUTO ?
> + REGULATOR_MODE_NORMAL : REGULATOR_MODE_FAST;
> +}
There is no need for this to be an inline and please write normal
conditional statements to improve legibility. There's other examples in
the driver.
> +static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev)
> +{
> + int ret, regval;
> + struct mt6358_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val);
> + if (ret != 0) {
> + dev_info(&rdev->dev,
> + "Failed to get mt6358 Buck %s vsel reg: %d\n",
> + info->desc.name, ret);
dev_err() for errors here and throughout the driver.
> + return ret;
> + }
> +
> + ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask;
> +
> + return ret;
> +}
This looks like a standard get_voltage_sel_regmap()?
> +err_mode:
> + if (ret != 0)
> + return ret;
> +
> + return 0;
Or just return ret unconditionally?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/9] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
To: Marc Zyngier
Cc: peter.maydell, Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190803121343.2f482200@why>
On 03/08/2019 12:13, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:09 +0100
> Steven Price <steven.price@arm.com> wrote:
>
> [+Peter for the userspace aspect of things]
>
> Hi Steve,
>
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Documentation/virtual/kvm/arm/pvtime.txt | 107 +++++++++++++++++++++++
>> 1 file changed, 107 insertions(+)
>> create mode 100644 Documentation/virtual/kvm/arm/pvtime.txt
>>
>> diff --git a/Documentation/virtual/kvm/arm/pvtime.txt b/Documentation/virtual/kvm/arm/pvtime.txt
>> new file mode 100644
>> index 000000000000..e6ae9799e1d5
>> --- /dev/null
>> +++ b/Documentation/virtual/kvm/arm/pvtime.txt
>> @@ -0,0 +1,107 @@
>> +Paravirtualized time support for arm64
>> +======================================
>> +
>> +Arm specification DEN0057/A defined a standard for paravirtualised time
>> +support for Aarch64 guests:
>
> nit: AArch64
>
>> +
>> +https://developer.arm.com/docs/den0057/a
>
> Between this file and the above document, which one is authoritative?
The above document should be authoritative - although I'm still waiting
for the final version to be published. I'm not expecting any changes to
the stolen time part though.
>> +
>> +KVM/Arm64 implements the stolen time part of this specification by providing
>
> nit: KVM/arm64
>
>> +some hypervisor service calls to support a paravirtualized guest obtaining a
>> +view of the amount of time stolen from its execution.
>> +
>> +Two new SMCCC compatible hypercalls are defined:
>> +
>> +PV_FEATURES 0xC5000020
>> +PV_TIME_ST 0xC5000022
>> +
>> +These are only available in the SMC64/HVC64 calling convention as
>> +paravirtualized time is not available to 32 bit Arm guests.
>> +
>> +PV_FEATURES
>> + Function ID: (uint32) : 0xC5000020
>> + PV_func_id: (uint32) : Either PV_TIME_LPT or PV_TIME_ST
>> + Return value: (int32) : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
>> + PV-time feature is supported by the hypervisor.
>
> How is PV_FEATURES discovered? Is the intention to make it a generic
> ARM-wide PV discovery mechanism, not specific to PV time?
SMCCC is mandated for PV time. So, assuming the hypervisor supports
SMCCC, the "NOT_SUPPORTED" return is mandated by SMCCC if PV time isn't
supported.
However, we do also use the SMCCC_ARCH_FEATURES mechanism to check the
existence of PV_FEATURES before use. I'll update the document to call
this out.
>> +
>> +PV_TIME_ST
>> + Function ID: (uint32) : 0xC5000022
>> + Return value: (int64) : IPA of the stolen time data structure for this
>> + (V)CPU. On failure:
>> + NOT_SUPPORTED (-1)
>> +
>
> Is the size implicit? What are the memory attributes? This either needs
> documenting here, or point to the right bit to the spec.
The size is implicit - it's a pointer to the below structure, so the
guest can only rely on the first 16 bytes being valid. The memory
attributes are described in the specification as:
"The calling guest can map the IPA into normal memory with inner and
outer write back caching attributes, in the inner sharable domain"
I'll put those details in this document for completeness.
>> +Stolen Time
>> +-----------
>> +
>> +The structure pointed to by the PV_TIME_ST hypercall is as follows:
>> +
>> + Field | Byte Length | Byte Offset | Description
>> + ----------- | ----------- | ----------- | --------------------------
>> + Revision | 4 | 0 | Must be 0 for version 0.1
>> + Attributes | 4 | 4 | Must be 0
>> + Stolen time | 8 | 8 | Stolen time in unsigned
>> + | | | nanoseconds indicating how
>> + | | | much time this VCPU thread
>> + | | | was involuntarily not
>> + | | | running on a physical CPU.
>> +
>> +The structure will be updated by the hypervisor periodically as time is stolen
>
> Is it really periodic? If so, when is the update frequency?
Hmm, periodic might be the wrong term - there is no guaranteed frequency
of update. The spec says:
"The hypervisor must update this value prior to scheduling a virtual CPU"
I guess that's probably the best description.
>> +from the VCPU. It will be present within a reserved region of the normal
>> +memory given to the guest. The guest should not attempt to write into this
>> +memory. There is a structure by VCPU of the guest.
>
> What if the vcpu writes to it? Does it get a fault?
From the perspective from the specification this is undefined. A fault
would therefore be acceptable but isn't generated in the implementation
defined here.
> If there is a
> structure per vcpu, what is the layout in memory? How does a vcpu find
> its own data structure? Is that the address returned by PV_TIME_ST?
A call to PV_TIME_ST returns the structure for the calling vCPU - I'll
make that explicit. The layout is therefore defined by the hypervisor
and cannot be relied on by the guest. As below this implementation uses
a simple array of structures.
>> +
>> +User space interface
>> +====================
>> +
>> +User space can request that KVM provide the paravirtualized time interface to
>> +a guest by creating a KVM_DEV_TYPE_ARM_PV_TIME device, for example:
>> +
>> + struct kvm_create_device pvtime_device = {
>> + .type = KVM_DEV_TYPE_ARM_PV_TIME,
>> + .attr = 0,
>> + .flags = 0,
>> + };
>> +
>> + pvtime_fd = ioctl(vm_fd, KVM_CREATE_DEVICE, &pvtime_device);
>> +
>> +The guest IPA of the structures must be given to KVM. This is the base address
>
> nit: s/guest //
>
>> +of an array of stolen time structures (one for each VCPU). For example:
>> +
>> + struct kvm_device_attr st_base = {
>> + .group = KVM_DEV_ARM_PV_TIME_PADDR,
>> + .attr = KVM_DEV_ARM_PV_TIME_ST,
>> + .addr = (u64)(unsigned long)&st_paddr
>> + };
>> +
>> + ioctl(pvtime_fd, KVM_SET_DEVICE_ATTR, &st_base);
>
> So the allocation itself is performed by the kernel? What are the
> ordering requirements between creating vcpus and the device? What are
> the alignment requirements for the base address?
The base address should be page aligned - I'll spell that out.
There are currently no ordering requirements between creating vcpus and
the device. However...
>> +
>> +For migration (or save/restore) of a guest it is necessary to save the contents
>> +of the shared page(s) and later restore them. KVM_DEV_ARM_PV_TIME_STATE_SIZE
>> +provides the size of this data and KVM_DEV_ARM_PV_TIME_STATE allows the state
>> +to be read/written.
>
> Is the size variable depending on the number of vcpus?
...yes - so restoring the state after migration must be done after
creating the vcpus. I'll point out that the device should created after.
Thanks for the review,
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/9] arm64: Stolen time support
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
To: Marc Zyngier
Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190803190522.5fec8f7d@why>
On 03/08/2019 19:05, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:08 +0100
> Steven Price <steven.price@arm.com> wrote:
>
> Hi Steven,
>
>> This series add support for paravirtualized time for arm64 guests and
>> KVM hosts following the specification in Arm's document DEN 0057A:
>>
>> https://developer.arm.com/docs/den0057/a
>>
>> It implements support for stolen time, allowing the guest to
>> identify time when it is forcibly not executing.
>>
>> It doesn't implement support for Live Physical Time (LPT) as there are
>> some concerns about the overheads and approach in the above
>> specification, and I expect an updated version of the specification to
>> be released soon with just the stolen time parts.
>
> Thanks for posting this.
>
> My current concern with this series is around the fact that we allocate
> memory from the kernel on behalf of the guest. It is the first example
> of such thing in the ARM port, and I can't really say I'm fond of it.
>
> x86 seems to get away with it by having the memory allocated from
> userspace, why I tend to like more. Yes, put_user is more
> expensive than a straight store, but this isn't done too often either.
>
> What is the rational for your current approach?
As I see it there are 3 approaches that can be taken here:
1. Hypervisor allocates memory and adds it to the virtual machine. This
means that everything to do with the 'device' is encapsulated behind the
KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want the
stolen time structure to be fast it cannot be a trapping region and has
to be backed by real memory - in this case allocated by the host kernel.
2. Host user space allocates memory. Similar to above, but this time
user space needs to manage the memory region as well as the usual
KVM_CREATE_DEVICE dance. I've no objection to this, but it means
kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
to size the memory region).
3. Guest kernel "donates" the memory to the hypervisor for the
structure. As far as I'm aware this is what x86 does. The problems I see
this approach are:
a) kexec becomes much more tricky - there needs to be a disabling
mechanism for the guest to stop the hypervisor scribbling on memory
before starting the new kernel.
b) If there is more than one entity that is interested in the
information (e.g. firmware and kernel) then this requires some form of
arbitration in the guest because the hypervisor doesn't want to have to
track an arbitrary number of regions to update.
c) Performance can suffer if the host kernel doesn't have a suitably
aligned/sized area to use. As you say - put_user() is more expensive.
The structure is updated on every return to the VM.
Of course x86 does prove the third approach can work, but I'm not sure
which is actually better. Avoid the kexec cancellation requirements was
the main driver of the current approach. Although many of the
conversations about this were also tied up with Live Physical Time which
adds its own complications.
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/9] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-05 13:06 UTC (permalink / raw)
To: Zenghui Yu
Cc: kvm, linux-doc, Marc Zyngier, linux-kernel, Russell King,
Catalin Marinas, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <3bdd764a-b6f5-d17e-a703-d8eb13838efc@huawei.com>
On 05/08/2019 04:23, Zenghui Yu wrote:
> Hi Steven,
>
> On 2019/8/2 22:50, Steven Price wrote:
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Documentation/virtual/kvm/arm/pvtime.txt | 107 +++++++++++++++++++++++
>> 1 file changed, 107 insertions(+)
>> create mode 100644 Documentation/virtual/kvm/arm/pvtime.txt
> ^^^^^^^
> This directory has been renamed recently, see:
>
> https://patchwork.ozlabs.org/patch/1136104/
Thanks for pointing that out - I'll move it in the next version.
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] dt-bindings: rng: mtk-rng: Add documentation for MT8516
From: Fabien Parent @ 2019-08-05 13:02 UTC (permalink / raw)
To: robh+dt, matthias.bgg
Cc: devicetree, linux-kernel, Fabien Parent, linux-mediatek,
linux-crypto, linux-arm-kernel
This commit adds the device-tree documentation for the RNG IP on the
MediaTek MT8516 SoC.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
Documentation/devicetree/bindings/rng/mtk-rng.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/rng/mtk-rng.txt b/Documentation/devicetree/bindings/rng/mtk-rng.txt
index 2bc89f133701..dfdcb5cd2ea8 100644
--- a/Documentation/devicetree/bindings/rng/mtk-rng.txt
+++ b/Documentation/devicetree/bindings/rng/mtk-rng.txt
@@ -6,6 +6,7 @@ Required properties:
"mediatek,mt7622-rng", "mediatek,mt7623-rng" : for MT7622
"mediatek,mt7629-rng", "mediatek,mt7623-rng" : for MT7629
"mediatek,mt7623-rng" : for MT7623
+ "mediatek,mt8516-rng", "mediatek,mt7623-rng" : for MT8516
- clocks : list of clock specifiers, corresponding to
entries in clock-names property;
- clock-names : Should contain "rng" entries;
--
2.23.0.rc1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/2] pwm: pwm-mediatek: Add MT8516 SoC support
From: Fabien Parent @ 2019-08-05 12:58 UTC (permalink / raw)
To: thierry.reding, robh+dt, matthias.bgg
Cc: mark.rutland, linux-pwm, devicetree, linux-kernel, Fabien Parent,
linux-mediatek, linux-arm-kernel
In-Reply-To: <20190805125848.15751-1-fparent@baylibre.com>
Add the compatible and the platform data to support PWM on the MT8516
SoC.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
drivers/pwm/pwm-mediatek.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index eb6674ce995f..6697e30811e7 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -302,11 +302,18 @@ static const struct mtk_pwm_platform_data mt7628_pwm_data = {
.has_clks = false,
};
+static const struct mtk_pwm_platform_data mt8516_pwm_data = {
+ .num_pwms = 5,
+ .pwm45_fixup = false,
+ .has_clks = true,
+};
+
static const struct of_device_id mtk_pwm_of_match[] = {
{ .compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data },
{ .compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data },
{ .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data },
{ .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data },
+ { .compatible = "mediatek,mt8516-pwm", .data = &mt8516_pwm_data },
{ },
};
MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
--
2.23.0.rc1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: pwm: pwm-mediatek: Add documentation for MT8516
From: Fabien Parent @ 2019-08-05 12:58 UTC (permalink / raw)
To: thierry.reding, robh+dt, matthias.bgg
Cc: mark.rutland, linux-pwm, devicetree, linux-kernel, Fabien Parent,
linux-mediatek, linux-arm-kernel
Add the device-tree documentation for the PWM IP on the MediaTek
MT8516 SoCs.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 991728cb46cb..9152bf5afe56 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -6,6 +6,7 @@ Required properties:
- "mediatek,mt7622-pwm": found on mt7622 SoC.
- "mediatek,mt7623-pwm": found on mt7623 SoC.
- "mediatek,mt7628-pwm": found on mt7628 SoC.
+ - "mediatek,mt8516-pwm": found on mt8516 SoC.
- reg: physical base address and length of the controller's registers.
- #pwm-cells: must be 2. See pwm.txt in this directory for a description of
the cell format.
--
2.23.0.rc1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* RE: [PATCH v2] arm64: dts: allwinner: a64: Enable eMMC on A64-OLinuXino
From: Martin Ayotte @ 2019-08-05 12:58 UTC (permalink / raw)
To: 'Sunil Mohan Adapa', 'Chen-Yu Tsai'
Cc: 'Maxime Ripard', 'linux-arm-kernel'
In-Reply-To: <de875c31-6cb3-8975-ac3d-de4ab3a851bd@medhas.org>
Fine for me too.
Thanks .
-----Message d'origine-----
De : Sunil Mohan Adapa [mailto:sunil@medhas.org]
Envoyé : Monday, August 05, 2019 1:25 AM
À : Chen-Yu Tsai
Cc : Maxime Ripard; Martin Ayotte; linux-arm-kernel
Objet : Re: [PATCH v2] arm64: dts: allwinner: a64: Enable eMMC on
A64-OLinuXino
On 04/08/19 8:33 pm, Chen-Yu Tsai wrote:
> On Fri, Aug 2, 2019 at 2:47 AM Sunil Mohan Adapa <sunil@medhas.org> wrote:
>>
>> On 01/08/19 6:49 am, Martin Ayotte wrote:
>>> If my SOB could help here, I don't mind since I've done the commit
>>> more than a year ago for Armbian ...
>>>
>>> Signed-off-by: Martin Ayotte <martinayotte@gmai.com>
>>> Tested-by: Martin Ayotte <martinayotte@gmai.com>
>> gmai.com is likely a typo.
>>
>>> On Wed, Jul 31, 2019 at 10:42 PM Chen-Yu Tsai <wens@csie.org
>>>
>>>> Thanks. The patch looks good overall. The authorship is a little
>>>> confusing though. If it was initially done by Martin (CC-ed), then
>>>> he should be the author, and we should get his Signed-off-by if
>>>> possible.
>>
>> Martin is indeed the original author of the patch. Thank you for
reviewing.
>
> I'd like to apply this patch with Martin as the author, if that's OK with
you
> both?
That is completely okay with me.
Thanks,
--
Sunil
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 6/6] firmware: arm_scmi: Check if platform has released shmem before using
From: Sudeep Holla @ 2019-08-05 12:52 UTC (permalink / raw)
To: Etienne Carriere
Cc: Peng Fan, linux-kernel, Bo Zhang, Jim Quinlan, Gaku Inami,
Volodymyr Babchuk, linux-arm-kernel
In-Reply-To: <CAN5uoS_TA5ELTLtHnUbWhaOHyUDjoKZz0S8SfmXBfR+n-=_M3w@mail.gmail.com>
On Mon, Aug 05, 2019 at 02:33:53PM +0200, Etienne Carriere wrote:
> Hello Sudeep,
>
> On Fri, 26 Jul 2019 at 15:46, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Sometimes platfom may take too long to respond to the command and OS
> > might timeout before platform transfer the ownership of the shared
> > memory region to the OS with the response.
> >
> > Since the mailbox channel associated with the channel is freed and new
> > commands are dispatch on the same channel, OS needs to wait until it
> > gets back the ownership. If not, either OS may end up overwriting the
> > platform response for the last command(which is fine as OS timed out
> > that command) or platform might overwrite the payload for the next
> > command with the response for the old.
> >
> > The latter is problematic as platform may end up interpretting the
> > response as the payload. In order to avoid such race, let's wait until
> > the OS gets back the ownership before we prepare the shared memory with
> > the payload for the next command.
> >
> > Reported-by: Jim Quinlan <james.quinlan@broadcom.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/firmware/arm_scmi/driver.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 69bf85fea967..765573756987 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -265,6 +265,14 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)
> > struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> > struct scmi_shared_mem __iomem *mem = cinfo->payload;
> >
> > + /*
> > + * Ideally channel must be free by now unless OS timeout last
> > + * request and platform continued to process the same, wait
> > + * until it releases the shared memory, otherwise we may endup
> > + * overwriting it's response with new command payload or vice-versa
>
> minor typo: s/it's/its/
> maybe also s/command/message/
>
Thanks for taking a look at this, both are fixed locally now.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
From: guillaume La Roque @ 2019-08-05 12:48 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: devicetree, linux-pm, khilman, daniel.lezcano, linux-kernel,
linux-amlogic, linux-arm-kernel
In-Reply-To: <CAFBinCDGSJABnS1L1ULueyeXZaV38qrxEA0a12gB-uyRC_TvPQ@mail.gmail.com>
Hi Martin,
again thanks for your review.
On 8/3/19 8:24 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> (I still don't have any experience with the thermal framework, so
> below are some general comments)
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
> I would add a patch description here:
> "
> Amlogic G12A and G12B SoCs integrate two thermal sensors with the same design.
> One is located close to the DDR (controller?) and the other one is
> located close to the PLLs (between the CPU and GPU).
>
> The calibration data for each of the thermal sensors instance is
> stored in a different location within the AO region.
>
> Implement reading the temperature from each thermal sensor.
>
> The IP block has more functionality, which may be added to this driver
> in the future:
> - reading up to 16 stored temperature samples
it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> - chip reset when the temperature exceeds a configurable threshold
> - up to four interrupts when the temperature has risen above a
> configurable threshold
> - up to four interrupts when the temperature has fallen below a
> configurable threshold
> "
agreed with you to add description and optional without 16 register part.
>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>> drivers/thermal/Kconfig | 11 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
>> 3 files changed, 344 insertions(+)
>> create mode 100644 drivers/thermal/amlogic_thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 9966364a6deb..0f31bb4bc372 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -348,6 +348,17 @@ config MTK_THERMAL
>> Enable this option if you want to have support for thermal management
>> controller present in Mediatek SoCs
>>
>> +config AMLOGIC_THERMAL
> we typically use "MESON" in the Kconfig symbols:
> $ grep -c AMLOGIC .config
> 1
> $ grep -c MESON .config
> 33
>
> I also wonder if we should add G12 or G12A so we don't conflict with
> upcoming thermal sensors with a different design (assuming that this
> will be a thing).
> for example we already have three different USB2 PHY drivers
>
> [...]
i check with Neil and for new family it's better to use Amlogic instead of meson.
i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
>> +/*
>> + * Calculate a temperature value from a temperature code.
>> + * The unit of the temperature is degree Celsius.
> is it really degree Celsius or millicelsius?
good point it's in millicelsius, i will fix all
>
>> + */
>> +static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
> PREFIX_thermal_code_to_millicelsius?
ok
> [...]
>> +static int amlogic_thermal_enable(struct amlogic_thermal *data)
>> +{
>> + clk_prepare_enable(data->clk);
> no clock error handling?
i will fix
>
>> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> + TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
>> +
>> + return 0;
>> +}
>> +
>> +static int amlogic_thermal_disable(struct amlogic_thermal *data)
>> +{
>> + regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> + TSENSOR_CFG_REG1_ENABLE, 0);
>> + clk_disable(data->clk);
> either clk_disable_unprepare here or use only clk_enable in
> amlogic_thermal_enable and move prepare/unprepare somewhere else
i will align with enable and use clk_disable_unprepare
>
>> +
>> + return 0;
>> +}
>> +
>> +static int amlogic_thermal_get_temp(void *data, int *temp)
>> +{
>> + unsigned int tvalue;
>> + struct amlogic_thermal *pdata = data;
>> +
>> + if (!data)
>> + return -EINVAL;
>> +
>> + regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
>> + *temp = code_to_temp(pdata,
>> + tvalue & TSENSOR_READ_TEMP_MASK);
> maybe simply move the implementation from code_to_temp here?
for the optional function it could be a problem if i move all in code_to_temp.
i prefer to have a function which are just do the conversion.
>
> [...]
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
>> + .u_efuse_off = 0x128,
>> + .soc = &amlogic_thermal_g12,
> based on the variable name I expected this to be an enum of some sort.
> I would have expected it to be called calibration_parameters or
> similar to match the explanation in the driver description
> (no need to change it if you prefer it like this, I just want to make
> you aware of this)
ok to rename structure name
>
>> + .regmap_config = &amlogic_thermal_regmap_config_g12,
> if regmap_config is always the same you may also pass it directly to
> devm_regmap_init_mmio
it's the same for now but we don't know if in the future SoCs use same regmap.
>
>> +};
>> +
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
>> + .u_efuse_off = 0xF0,
> I believe we use lower-case hex everywhere else
>
> [...]
will fix
>> +static const struct of_device_id of_amlogic_thermal_match[] = {
>> + {
>> + .compatible = "amlogic,g12-ddr-thermal",
>> + .data = &amlogic_thermal_g12_ddr_param,
>> + },
>> + {
>> + .compatible = "amlogic,g12-cpu-thermal",
>> + .data = &amlogic_thermal_g12_cpu_param,
>> + },
>> + { /* end */ }
> other drivers use "sentinel", but personally I have no preference here
>
> [...]
same for me i will do same as other drivers
>> + pdata->tzd = devm_thermal_zone_of_sensor_register
>> + (&pdev->dev,
>> + 0,
>> + pdata,
>> + &amlogic_thermal_ops);
> I believe the opening brace has to go onto the same line as the function name
>
> [...]
will fix
>> + ret = amlogic_thermal_enable(pdata);
>> + if (ret)
>> + clk_unprepare(pdata->clk);
> clk_disable_unprepare, else you'll leave the clock prepared
will fix
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int amlogic_thermal_suspend(struct device *dev)
>> +{
>> + struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> + return amlogic_thermal_disable(data);
>> +}
>> +
>> +static int amlogic_thermal_resume(struct device *dev)
>> +{
>> + struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> + return amlogic_thermal_enable(data);
>> +}
>> +#endif
> instead of using an #ifdef here annotate the suspend/resume functions
> with __maybe_unused, see [0]
will fix
>
>
> Martin
>
>
> [0] https://lore.kernel.org/patchwork/patch/732981/
[0] https://lore.kernel.org/linux-arm-kernel/20190701104705.18271-1-narmstrong@baylibre.com/
thanks
Guillaume
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
From: guillaume La Roque @ 2019-08-05 12:48 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: devicetree, linux-pm, khilman, daniel.lezcano, linux-kernel,
linux-amlogic, linux-arm-kernel
In-Reply-To: <CAFBinCD-DUEjPgfUEJmHpPjw1ShZy7nemaFBKANmM5M42XBG4Q@mail.gmail.com>
Hi Martin,
On 8/3/19 8:29 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Add minimal thermal zone for DDR and CPU sensor
> so high DDR (controller?) temperatures will throttle Mali and high PLL
> temperatures will throttle the CPU?
> to get things started I'm fine with this, but I think it should be
> mentioned here
i will add in commit description
>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>> .../boot/dts/amlogic/meson-g12a-sei510.dts | 56 +++++++++++++++++++
>> 1 file changed, 56 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> index 979449968a5f..2c16a2cba0a3 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> @@ -10,6 +10,7 @@
>> #include <dt-bindings/input/input.h>
>> #include <dt-bindings/gpio/meson-g12a-gpio.h>
>> #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
>> +#include <dt-bindings/thermal/thermal.h>
>>
>> / {
>> compatible = "seirobotics,sei510", "amlogic,g12a";
>> @@ -33,6 +34,53 @@
>> ethernet0 = ðmac;
>> };
>>
>> + thermal-zones {
>> + cpu-thermal {
>> + polling-delay = <1000>;
>> + polling-delay-passive = <100>;
>> + thermal-sensors = <&cpu_temp>;
>> +
>> + trips {
>> + cpu_critical: cpu-critical {
>> + temperature = <110000>; /* millicelsius */
>> + hysteresis = <2000>; /* millicelsius */
>> + type = "critical";
>> + };
>> + };
>> +
>> + cooling-maps {
>> + map {
>> + trip = <&cpu_critical>;
>> + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> + };
>> + };
>> + };
>> +
>> + ddr-thermal {
>> + polling-delay = <1000>;
>> + polling-delay-passive = <100>;
>> + thermal-sensors = <&ddr_temp>;
>> +
>> + trips {
>> + ddr_critical: ddr-critical {
>> + temperature = <110000>; /* millicelsius */
>> + hysteresis = <2000>; /* millicelsius */
>> + type = "critical";
>> + };
>> + };
>> +
>> + cooling-maps {
>> + map {
>> + trip = <&ddr_critical>;
>> + cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> + };
>> + };
>> + };
>> + };
>> +
>> mono_dac: audio-codec-0 {
>> compatible = "maxim,max98357a";
>> #sound-dai-cells = <0>;
>> @@ -321,6 +369,7 @@
>> operating-points-v2 = <&cpu_opp_table>;
>> clocks = <&clkc CLKID_CPU_CLK>;
>> clock-latency = <50000>;
>> + #cooling-cells = <2>;
>> };
>>
>> &cpu1 {
>> @@ -328,6 +377,7 @@
>> operating-points-v2 = <&cpu_opp_table>;
>> clocks = <&clkc CLKID_CPU_CLK>;
>> clock-latency = <50000>;
>> + #cooling-cells = <2>;
>> };
>>
>> &cpu2 {
>> @@ -335,6 +385,7 @@
>> operating-points-v2 = <&cpu_opp_table>;
>> clocks = <&clkc CLKID_CPU_CLK>;
>> clock-latency = <50000>;
>> + #cooling-cells = <2>;
>> };
>>
>> &cpu3 {
>> @@ -342,6 +393,7 @@
>> operating-points-v2 = <&cpu_opp_table>;
>> clocks = <&clkc CLKID_CPU_CLK>;
>> clock-latency = <50000>;
>> + #cooling-cells = <2>;
>> };
>>
>> &cvbs_vdac_port {
>> @@ -368,6 +420,10 @@
>> status = "okay";
>> };
>>
>> +&mali {
>> + #cooling-cells = <2>;
>> +};
> is there something device-specific in this patch? I'm wondering
> whether we can move all of this go g12a.dtsi to simplify maintenance
> in the future
this is depending of each board. actually it's same on all
but if a new one have a fan this value should be different or not.
>
>
> Martin
thanks,
Guillaume
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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