* [PATCH 2/4] clocksource: stm32: use prescaler to adjust the resolution
From: Benjamin Gaignard @ 2017-12-15 8:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171215085247.14946-1-benjamin.gaignard@st.com>
Rather than use fixed prescaler values compute it to get a clock
as close as possible of 10KHz and a resolution of 0.1ms.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/clocksource/timer-stm32.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 23a321cca45b..de721d318065 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -37,6 +37,11 @@
#define TIM_EGR_UG BIT(0)
+#define MAX_TIM_PSC 0xFFFF
+
+/* Target a 10KHz clock to get a resolution of 0.1 ms */
+#define TARGETED_CLK_RATE 10000
+
static int stm32_clock_event_shutdown(struct clock_event_device *evt)
{
struct timer_of *to = to_timer_of(evt);
@@ -83,7 +88,7 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
static void __init stm32_clockevent_init(struct timer_of *to)
{
unsigned long max_delta;
- int prescaler;
+ unsigned long prescaler;
to->clkevt.name = "stm32_clockevent";
to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
@@ -96,13 +101,17 @@ static void __init stm32_clockevent_init(struct timer_of *to)
/* Detect whether the timer is 16 or 32 bits */
writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
- if (max_delta == ~0U) {
- prescaler = 1;
+ to->clkevt.rating = 50;
+ if (max_delta == ~0U)
to->clkevt.rating = 250;
- } else {
- prescaler = 1024;
- to->clkevt.rating = 50;
- }
+
+ /*
+ * Get the highest possible prescaler value to be as close
+ * as possible of TARGETED_CLK_RATE
+ */
+ prescaler = DIV_ROUND_CLOSEST(timer_of_rate(to), TARGETED_CLK_RATE);
+ if (prescaler > MAX_TIM_PSC)
+ prescaler = MAX_TIM_PSC;
writel_relaxed(0, timer_of_base(to) + TIM_ARR);
writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
--
2.15.0
^ permalink raw reply related
* [PATCH 3/4] clocksource: stm32: add clocksource support
From: Benjamin Gaignard @ 2017-12-15 8:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171215085247.14946-1-benjamin.gaignard@st.com>
The stm32 timer hardware is currently only used as a clock event device,
but it can be used as a clocksource as well.
Implement this by enabling the free running counter in the hardware block
and converting the clock event part from a count down event timer to a
comparator based timer.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/clocksource/timer-stm32.c | 114 ++++++++++++++++++++++++++++----------
1 file changed, 86 insertions(+), 28 deletions(-)
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index de721d318065..38eb59bb7f8a 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,6 +16,7 @@
#include <linux/of_irq.h>
#include <linux/clk.h>
#include <linux/reset.h>
+#include <linux/sched_clock.h>
#include <linux/slab.h>
#include "timer-of.h"
@@ -24,17 +25,15 @@
#define TIM_DIER 0x0c
#define TIM_SR 0x10
#define TIM_EGR 0x14
+#define TIM_CNT 0x24
#define TIM_PSC 0x28
#define TIM_ARR 0x2c
+#define TIM_CCR1 0x34
#define TIM_CR1_CEN BIT(0)
-#define TIM_CR1_OPM BIT(3)
+#define TIM_CR1_UDIS BIT(1)
#define TIM_CR1_ARPE BIT(7)
-
-#define TIM_DIER_UIE BIT(0)
-
-#define TIM_SR_UIF BIT(0)
-
+#define TIM_DIER_CC1IE BIT(1)
#define TIM_EGR_UG BIT(0)
#define MAX_TIM_PSC 0xFFFF
@@ -46,29 +45,44 @@ static int stm32_clock_event_shutdown(struct clock_event_device *evt)
{
struct timer_of *to = to_timer_of(evt);
- writel_relaxed(0, timer_of_base(to) + TIM_CR1);
+ writel_relaxed(0, timer_of_base(to) + TIM_DIER);
return 0;
}
-static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
+static int stm32_clock_event_set_next_event(unsigned long evt,
+ struct clock_event_device *clkevt)
{
- struct timer_of *to = to_timer_of(evt);
+ struct timer_of *to = to_timer_of(clkevt);
+ unsigned long now, next;
+
+ next = readl_relaxed(timer_of_base(to) + TIM_CNT) + evt;
+ writel_relaxed(next, timer_of_base(to) + TIM_CCR1);
+ now = readl_relaxed(timer_of_base(to) + TIM_CNT);
+
+ if ((next - now) > evt)
+ return -ETIME;
- writel_relaxed(timer_of_period(to), timer_of_base(to) + TIM_ARR);
- writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, timer_of_base(to) + TIM_CR1);
+ writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
return 0;
}
-static int stm32_clock_event_set_next_event(unsigned long evt,
- struct clock_event_device *clkevt)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evt)
{
- struct timer_of *to = to_timer_of(clkevt);
+ struct timer_of *to = to_timer_of(evt);
- writel_relaxed(evt, timer_of_base(to) + TIM_ARR);
- writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
- timer_of_base(to) + TIM_CR1);
+ return stm32_clock_event_set_next_event(timer_of_period(to), evt);
+}
+
+static int stm32_clock_event_set_oneshot(struct clock_event_device *evt)
+{
+ struct timer_of *to = to_timer_of(evt);
+ unsigned long val;
+
+ val = readl_relaxed(timer_of_base(to) + TIM_CNT);
+ writel_relaxed(val, timer_of_base(to) + TIM_CCR1);
+ writel_relaxed(TIM_DIER_CC1IE, timer_of_base(to) + TIM_DIER);
return 0;
}
@@ -80,6 +94,11 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
writel_relaxed(0, timer_of_base(to) + TIM_SR);
+ if (clockevent_state_periodic(evt))
+ stm32_clock_event_set_periodic(evt);
+ else
+ stm32_clock_event_shutdown(evt);
+
evt->event_handler(evt);
return IRQ_HANDLED;
@@ -88,22 +107,46 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
static void __init stm32_clockevent_init(struct timer_of *to)
{
unsigned long max_delta;
- unsigned long prescaler;
to->clkevt.name = "stm32_clockevent";
- to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
+ to->clkevt.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
to->clkevt.set_state_shutdown = stm32_clock_event_shutdown;
to->clkevt.set_state_periodic = stm32_clock_event_set_periodic;
- to->clkevt.set_state_oneshot = stm32_clock_event_shutdown;
+ to->clkevt.set_state_oneshot = stm32_clock_event_set_oneshot;
to->clkevt.tick_resume = stm32_clock_event_shutdown;
to->clkevt.set_next_event = stm32_clock_event_set_next_event;
/* Detect whether the timer is 16 or 32 bits */
+ max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
+
+ clockevents_config_and_register(&to->clkevt,
+ timer_of_rate(to), 0x1, max_delta);
+}
+
+static void __iomem *stm32_timer_cnt __read_mostly;
+
+static u64 notrace stm32_read_sched_clock(void)
+{
+ return readl_relaxed(stm32_timer_cnt);
+}
+
+static int __init stm32_clocksource_init(struct timer_of *to)
+{
+ unsigned long max_delta, prescaler;
+ int bits = 16;
+
writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);
+ writel_relaxed(0, timer_of_base(to) + TIM_SR);
+ writel_relaxed(0, timer_of_base(to) + TIM_DIER);
+
+ /* Detect whether the timer is 16 or 32 bits */
max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);
+
to->clkevt.rating = 50;
- if (max_delta == ~0U)
+ if (max_delta == ~0U) {
+ bits = 32;
to->clkevt.rating = 250;
+ }
/*
* Get the highest possible prescaler value to be as close
@@ -113,18 +156,27 @@ static void __init stm32_clockevent_init(struct timer_of *to)
if (prescaler > MAX_TIM_PSC)
prescaler = MAX_TIM_PSC;
- writel_relaxed(0, timer_of_base(to) + TIM_ARR);
writel_relaxed(prescaler - 1, timer_of_base(to) + TIM_PSC);
- writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
- writel_relaxed(TIM_DIER_UIE, timer_of_base(to) + TIM_DIER);
- writel_relaxed(0, timer_of_base(to) + TIM_SR);
- /* adjust rate and period given the prescaler value */
+ /* Adjust rate and period given the prescaler value */
to->of_clk.rate = DIV_ROUND_CLOSEST(to->of_clk.rate, prescaler);
to->of_clk.period = DIV_ROUND_UP(to->of_clk.rate, HZ);
- clockevents_config_and_register(&to->clkevt,
- timer_of_rate(to), 0x1, max_delta);
+ /* Make sure that registers are updated */
+ writel_relaxed(TIM_EGR_UG, timer_of_base(to) + TIM_EGR);
+
+ /* Enable controller */
+ writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
+ timer_of_base(to) + TIM_CR1);
+
+ stm32_timer_cnt = timer_of_base(to) + TIM_CNT;
+ sched_clock_register(stm32_read_sched_clock, bits, timer_of_rate(to));
+
+ return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
+ timer_of_rate(to),
+ to->clkevt.rating,
+ bits,
+ clocksource_mmio_readl_up);
}
static int __init stm32_timer_init(struct device_node *node)
@@ -150,10 +202,16 @@ static int __init stm32_timer_init(struct device_node *node)
reset_control_deassert(rstc);
}
+ ret = stm32_clocksource_init(to);
+ if (ret)
+ goto deinit;
+
stm32_clockevent_init(to);
return 0;
+deinit:
+ timer_of_cleanup(to);
err:
kfree(to);
return ret;
--
2.15.0
^ permalink raw reply related
* [PATCH 4/4] clocksource: stm32: Update license and copyright
From: Benjamin Gaignard @ 2017-12-15 8:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171215085247.14946-1-benjamin.gaignard@st.com>
Adopt SPDX License Identifier and add STMicroelectronics
copyright
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/clocksource/timer-stm32.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 38eb59bb7f8a..83603d4b5706 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -1,7 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (C) Maxime Coquelin 2015
+ * Copyright (C) STMicroelectronics 2017 - All Rights Reserved
* Author: Maxime Coquelin <mcoquelin.stm32@gmail.com>
- * License terms: GNU General Public License (GPL), version 2
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
*
* Inspired by time-efm32.c from Uwe Kleine-Koenig
*/
--
2.15.0
^ permalink raw reply related
* [PATCH 1/6] ARM: shmobile: defconfig: Enable PWM
From: Geert Uytterhoeven @ 2017-12-15 8:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513248976-26700-2-git-send-email-fabrizio.castro@bp.renesas.com>
Hi Fabrizio,
On Thu, Dec 14, 2017 at 11:56 AM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> RZ/G1 and R-Car platforms have PWM timers. This patch enables PWM support
> by default.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das@bp.renesas.com>
Thanks for your patch!
If I'm not mistaken, there are no current users of this PWM block in the
current R-Car Gen2 and RZ/G1 DTS files?
If that's correct, I don't think it makes much sense to enable it already.
Simon: what do you think?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v3 04/11] thermal: armada: Rationalize register accesses
From: Baruch Siach @ 2017-12-15 8:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214103011.24713-5-miquel.raynal@free-electrons.com>
Hi Miqu?l,
On Thu, Dec 14, 2017 at 11:30:04AM +0100, Miquel Raynal wrote:
> Bindings were incomplete for a long time by only exposing one of the two
> available control registers. To ease the migration to the full bindings
> (already in use for the Armada 375 SoC), rename the pointers for
> clarification. This way, it will only be needed to add another pointer
> to access the other control register when the time comes.
>
> This avoids dangerous situations where the offset 0 of the control
> area can be either one register or the other depending on the bindings
> used. After this change, device trees of other SoCs could be migrated to
> the "full" bindings if they may benefit from features from the
> unaccessible register, without any change in the driver.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
> drivers/thermal/armada_thermal.c | 86 +++++++++++++++++++++++++---------------
> 1 file changed, 55 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 26698f2d3ca7..e5b184cee79b 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -39,12 +39,21 @@
> #define A375_HW_RESETn BIT(8)
> #define A380_HW_RESET BIT(8)
>
> +/* Legacy bindings */
> +#define LEGACY_CONTROL_MEM_LEN 0x4
> +
> +/* Current bindings with the 2 control registers under the same memory area */
> +#define LEGACY_CONTROL1_OFFSET 0x0
> +#define CONTROL0_OFFSET 0x0
> +#define CONTROL1_OFFSET 0x4
> +
> struct armada_thermal_data;
>
> /* Marvell EBU Thermal Sensor Dev Structure */
> struct armada_thermal_priv {
> - void __iomem *sensor;
> - void __iomem *control;
> + void __iomem *status;
> + void __iomem *control0;
> + void __iomem *control1;
The 'status' -> 'sensor' rename is not mentioned in the commit log. I'd say it
is a matter for a separate patch.
Otherwise, good cleanup.
baruch
> struct armada_thermal_data *data;
> };
>
> @@ -71,45 +80,45 @@ struct armada_thermal_data {
> static void armadaxp_init_sensor(struct platform_device *pdev,
> struct armada_thermal_priv *priv)
> {
> - unsigned long reg;
> + u32 reg;
>
> - reg = readl_relaxed(priv->control);
> + reg = readl_relaxed(priv->control1);
> reg |= PMU_TDC0_OTF_CAL_MASK;
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> /* Reference calibration value */
> reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> /* Reset the sensor */
> - reg = readl_relaxed(priv->control);
> - writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> + reg = readl_relaxed(priv->control1);
> + writel((reg | PMU_TDC0_SW_RST_MASK), priv->control1);
>
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> /* Enable the sensor */
> - reg = readl_relaxed(priv->sensor);
> + reg = readl_relaxed(priv->status);
> reg &= ~PMU_TM_DISABLE_MASK;
> - writel(reg, priv->sensor);
> + writel(reg, priv->status);
> }
>
> static void armada370_init_sensor(struct platform_device *pdev,
> struct armada_thermal_priv *priv)
> {
> - unsigned long reg;
> + u32 reg;
>
> - reg = readl_relaxed(priv->control);
> + reg = readl_relaxed(priv->control1);
> reg |= PMU_TDC0_OTF_CAL_MASK;
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> /* Reference calibration value */
> reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> reg &= ~PMU_TDC0_START_CAL_MASK;
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
>
> msleep(10);
> }
> @@ -117,37 +126,37 @@ static void armada370_init_sensor(struct platform_device *pdev,
> static void armada375_init_sensor(struct platform_device *pdev,
> struct armada_thermal_priv *priv)
> {
> - unsigned long reg;
> + u32 reg;
>
> - reg = readl(priv->control + 4);
> + reg = readl(priv->control1);
> reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT);
> reg &= ~A375_READOUT_INVERT;
> reg &= ~A375_HW_RESETn;
>
> - writel(reg, priv->control + 4);
> + writel(reg, priv->control1);
> msleep(20);
>
> reg |= A375_HW_RESETn;
> - writel(reg, priv->control + 4);
> + writel(reg, priv->control1);
> msleep(50);
> }
>
> static void armada380_init_sensor(struct platform_device *pdev,
> struct armada_thermal_priv *priv)
> {
> - unsigned long reg = readl_relaxed(priv->control);
> + u32 reg = readl_relaxed(priv->control1);
>
> /* Reset hardware once */
> if (!(reg & A380_HW_RESET)) {
> reg |= A380_HW_RESET;
> - writel(reg, priv->control);
> + writel(reg, priv->control1);
> msleep(10);
> }
> }
>
> static bool armada_is_valid(struct armada_thermal_priv *priv)
> {
> - unsigned long reg = readl_relaxed(priv->sensor);
> + u32 reg = readl_relaxed(priv->status);
>
> return reg & priv->data->is_valid_bit;
> }
> @@ -156,7 +165,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
> int *temp)
> {
> struct armada_thermal_priv *priv = thermal->devdata;
> - unsigned long reg;
> + u32 reg;
> unsigned long m, b, div;
>
> /* Valid check */
> @@ -166,7 +175,7 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
> return -EIO;
> }
>
> - reg = readl_relaxed(priv->sensor);
> + reg = readl_relaxed(priv->status);
> reg = (reg >> priv->data->temp_shift) & priv->data->temp_mask;
>
> /* Get formula coeficients */
> @@ -253,6 +262,7 @@ MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
>
> static int armada_thermal_probe(struct platform_device *pdev)
> {
> + void __iomem *control = NULL;
> struct thermal_zone_device *thermal;
> const struct of_device_id *match;
> struct armada_thermal_priv *priv;
> @@ -267,14 +277,28 @@ static int armada_thermal_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(priv->sensor))
> - return PTR_ERR(priv->sensor);
> + priv->status = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->status))
> + return PTR_ERR(priv->status);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - priv->control = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(priv->control))
> - return PTR_ERR(priv->control);
> + control = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(control))
> + return PTR_ERR(control);
> +
> + /*
> + * Legacy DT bindings only described "control1" register (also referred
> + * as "control MSB" on old documentation). New bindings cover
> + * "control0/control LSB" and "control1/control MSB" registers within
> + * the same resource, which is then of size 8 instead of 4.
> + */
> + if ((res->end - res->start) == LEGACY_CONTROL_MEM_LEN) {
> + /* ->control0 unavailable in this configuration */
> + priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> + } else {
> + priv->control0 = control + CONTROL0_OFFSET;
> + priv->control1 = control + CONTROL1_OFFSET;
> + }
>
> priv->data = (struct armada_thermal_data *)match->data;
> priv->data->init_sensor(pdev, priv);
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply
* arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP
From: AKASHI Takahiro @ 2017-12-15 8:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu_G8kBEAdAznVauZVAdJOFkr1vmu0Gf6tOwJfH2CgdufA@mail.gmail.com>
On Wed, Dec 13, 2017 at 12:17:22PM +0000, Ard Biesheuvel wrote:
> On 13 December 2017 at 12:16, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> > On Wed, Dec 13, 2017 at 10:49:27AM +0000, Ard Biesheuvel wrote:
> >> On 13 December 2017 at 10:26, AKASHI Takahiro
> >> <takahiro.akashi@linaro.org> wrote:
> >> > Bhupesh, Ard,
> >> >
> >> > On Wed, Dec 13, 2017 at 03:21:59AM +0530, Bhupesh Sharma wrote:
> >> >> Hi Ard, Akashi
> >> >>
> >> > (snip)
> >> >
> >> >> Looking deeper into the issue, since the arm64 kexec-tools uses the
> >> >> 'linux,usable-memory-range' dt property to allow crash dump kernel to
> >> >> identify its own usable memory and exclude, at its boot time, any
> >> >> other memory areas that are part of the panicked kernel's memory.
> >> >> (see https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
> >> >> , for details)
> >> >
> >> > Right.
> >> >
> >> >> 1). Now when 'kexec -p' is executed, this node is patched up only
> >> >> with the crashkernel memory range:
> >> >>
> >> >> /* add linux,usable-memory-range */
> >> >> nodeoffset = fdt_path_offset(new_buf, "/chosen");
> >> >> result = fdt_setprop_range(new_buf, nodeoffset,
> >> >> PROP_USABLE_MEM_RANGE, &crash_reserved_mem,
> >> >> address_cells, size_cells);
> >> >>
> >> >> (see https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/arm64/kexec-arm64.c#n465
> >> >> , for details)
> >> >>
> >> >> 2). This excludes the ACPI reclaim regions irrespective of whether
> >> >> they are marked as System RAM or as RESERVED. As,
> >> >> 'linux,usable-memory-range' dt node is patched up only with
> >> >> 'crash_reserved_mem' and not 'system_memory_ranges'
> >> >>
> >> >> 3). As a result when the crashkernel boots up it doesn't find this
> >> >> ACPI memory and crashes while trying to access the same:
> >> >>
> >> >> # kexec -p /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname
> >> >> -r`.img --reuse-cmdline -d
> >> >>
> >> >> [snip..]
> >> >>
> >> >> Reserved memory range
> >> >> 000000000e800000-000000002e7fffff (0)
> >> >>
> >> >> Coredump memory ranges
> >> >> 0000000000000000-000000000e7fffff (0)
> >> >> 000000002e800000-000000003961ffff (0)
> >> >> 0000000039d40000-000000003ed2ffff (0)
> >> >> 000000003ed60000-000000003fbfffff (0)
> >> >> 0000001040000000-0000001ffbffffff (0)
> >> >> 0000002000000000-0000002ffbffffff (0)
> >> >> 0000009000000000-0000009ffbffffff (0)
> >> >> 000000a000000000-000000affbffffff (0)
> >> >>
> >> >> 4). So if we revert Ard's patch or just comment the fixing up of the
> >> >> memory cap'ing passed to the crash kernel inside
> >> >> 'arch/arm64/mm/init.c' (see below):
> >> >>
> >> >> static void __init fdt_enforce_memory_region(void)
> >> >> {
> >> >> struct memblock_region reg = {
> >> >> .size = 0,
> >> >> };
> >> >>
> >> >> of_scan_flat_dt(early_init_dt_scan_usablemem, ®);
> >> >>
> >> >> if (reg.size)
> >> >> //memblock_cap_memory_range(reg.base, reg.size); /*
> >> >> comment this out */
> >> >> }
> >> >
> >> > Please just don't do that. It can cause a fatal damage on
> >> > memory contents of the *crashed* kernel.
> >> >
> >> >> 5). Both the above temporary solutions fix the problem.
> >> >>
> >> >> 6). However exposing all System RAM regions to the crashkernel is not
> >> >> advisable and may cause the crashkernel or some crashkernel drivers to
> >> >> fail.
> >> >>
> >> >> 6a). I am trying an approach now, where the ACPI reclaim regions are
> >> >> added to '/proc/iomem' separately as ACPI reclaim regions by the
> >> >> kernel code and on the other hand the user-space 'kexec-tools' will
> >> >> pick up the ACPI reclaim regions from '/proc/iomem' and add it to the
> >> >> dt node 'linux,usable-memory-range'
> >> >
> >> > I still don't understand why we need to carry over the information
> >> > about "ACPI Reclaim memory" to crash dump kernel. In my understandings,
> >> > such regions are free to be reused by the kernel after some point of
> >> > initialization. Why does crash dump kernel need to know about them?
> >> >
> >>
> >> Not really. According to the UEFI spec, they can be reclaimed after
> >> the OS has initialized, i.e., when it has consumed the ACPI tables and
> >> no longer needs them. Of course, in order to be able to boot a kexec
> >> kernel, those regions needs to be preserved, which is why they are
> >> memblock_reserve()'d now.
> >
> > For my better understandings, who is actually accessing such regions
> > during boot time, uefi itself or efistub?
> >
>
> No, only the kernel. This is where the ACPI tables are stored. For
> instance, on QEMU we have
>
> ACPI: RSDP 0x0000000078980000 000024 (v02 BOCHS )
> ACPI: XSDT 0x0000000078970000 000054 (v01 BOCHS BXPCFACP 00000001
> 01000013)
> ACPI: FACP 0x0000000078930000 00010C (v05 BOCHS BXPCFACP 00000001
> BXPC 00000001)
> ACPI: DSDT 0x0000000078940000 0011DA (v02 BOCHS BXPCDSDT 00000001
> BXPC 00000001)
> ACPI: APIC 0x0000000078920000 000140 (v03 BOCHS BXPCAPIC 00000001
> BXPC 00000001)
> ACPI: GTDT 0x0000000078910000 000060 (v02 BOCHS BXPCGTDT 00000001
> BXPC 00000001)
> ACPI: MCFG 0x0000000078900000 00003C (v01 BOCHS BXPCMCFG 00000001
> BXPC 00000001)
> ACPI: SPCR 0x00000000788F0000 000050 (v02 BOCHS BXPCSPCR 00000001
> BXPC 00000001)
> ACPI: IORT 0x00000000788E0000 00007C (v00 BOCHS BXPCIORT 00000001
> BXPC 00000001)
>
> covered by
>
> efi: 0x0000788e0000-0x00007894ffff [ACPI Reclaim Memory ...]
> ...
> efi: 0x000078970000-0x00007898ffff [ACPI Reclaim Memory ...]
OK. I mistakenly understood those regions could be freed after exiting
UEFI boot services.
>
> >> So it seems that kexec does not honour the memblock_reserve() table
> >> when booting the next kernel.
> >
> > not really.
> >
> >> > (In other words, can or should we skip some part of ACPI-related init code
> >> > on crash dump kernel?)
> >> >
> >>
> >> I don't think so. And the change to the handling of ACPI reclaim
> >> regions only revealed the bug, not created it (given that other
> >> memblock_reserve regions may be affected as well)
> >
> > As whether we should honor such reserved regions over kexec'ing
> > depends on each one's specific nature, we will have to take care one-by-one.
> > As a matter of fact, no information about "reserved" memblocks is
> > exposed to user space (via proc/iomem).
> >
>
> That is why I suggested (somewhere in this thread?) to not expose them
> as 'System RAM'. Do you think that could solve this?
Memblock-reserv'ing them is necessary to prevent their corruption and
marking them under another name in /proc/iomem would also be good in order
not to allocate them as part of crash kernel's memory.
But I'm not still convinced that we should export them in useable-
memory-range to crash dump kernel. They will be accessed through
acpi_os_map_memory() and so won't be required to be part of system ram
(or memblocks), I guess.
-> Bhupesh?
Just FYI, on x86, ACPI tables seems to be exposed to crash dump kernel
via a kernel command line parameter, "memmap=".
Thanks,
-Takahiro AKASHI
> >
> >>
> >> >> 6b). The kernel code currently looks like the following:
> >> >>
> >> >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> >> >> index 30ad2f085d1f..867bdec7c692 100644
> >> >> --- a/arch/arm64/kernel/setup.c
> >> >> +++ b/arch/arm64/kernel/setup.c
> >> >> @@ -206,6 +206,7 @@ static void __init request_standard_resources(void)
> >> >> {
> >> >> struct memblock_region *region;
> >> >> struct resource *res;
> >> >> + phys_addr_t addr_start, addr_end;
> >> >>
> >> >> kernel_code.start = __pa_symbol(_text);
> >> >> kernel_code.end = __pa_symbol(__init_begin - 1);
> >> >> @@ -218,9 +219,17 @@ static void __init request_standard_resources(void)
> >> >> res->name = "reserved";
> >> >> res->flags = IORESOURCE_MEM;
> >> >> } else {
> >> >> - res->name = "System RAM";
> >> >> - res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> >> >> + addr_start =
> >> >> __pfn_to_phys(memblock_region_reserved_base_pfn(region));
> >> >> + addr_end =
> >> >> __pfn_to_phys(memblock_region_reserved_end_pfn(region)) - 1;
> >> >> + if ((efi_mem_type(addr_start) == EFI_ACPI_RECLAIM_MEMORY)
> >> >> || (efi_mem_type(addr_end) == EFI_ACPI_RECLAIM_MEMORY)) {
> >> >> + res->name = "ACPI reclaim region";
> >> >> + res->flags = IORESOURCE_MEM;
> >> >> + } else {
> >> >> + res->name = "System RAM";
> >> >> + res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> >> >> + }
> >> >> }
> >> >> +
> >> >> res->start = __pfn_to_phys(memblock_region_memory_base_pfn(region));
> >> >> res->end = __pfn_to_phys(memblock_region_memory_end_pfn(region)) - 1;
> >> >>
> >> >> @@ -292,6 +301,7 @@ void __init setup_arch(char **cmdline_p)
> >> >>
> >> >> request_standard_resources();
> >> >>
> >> >> + efi_memmap_unmap();
> >> >> early_ioremap_reset();
> >> >>
> >> >> if (acpi_disabled)
> >> >> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> >> >> index 80d1a885def5..a7c522eac640 100644
> >> >> --- a/drivers/firmware/efi/arm-init.c
> >> >> +++ b/drivers/firmware/efi/arm-init.c
> >> >> @@ -259,7 +259,6 @@ void __init efi_init(void)
> >> >>
> >> >> reserve_regions();
> >> >> efi_esrt_init();
> >> >> - efi_memmap_unmap();
> >> >>
> >> >> memblock_reserve(params.mmap & PAGE_MASK,
> >> >> PAGE_ALIGN(params.mmap_size +
> >> >>
> >> >>
> >> >> After this change the ACPI reclaim regions are properly recognized in
> >> >> '/proc/iomem':
> >> >>
> >> >> # cat /proc/iomem | grep -i ACPI
> >> >> 396c0000-3975ffff : ACPI reclaim region
> >> >> 39770000-397affff : ACPI reclaim region
> >> >> 398a0000-398bffff : ACPI reclaim region
> >> >>
> >> >> 6c). I am currently changing the 'kexec-tools' and will finish the
> >> >> testing over the next few days.
> >> >>
> >> >> I just wanted to know your opinion on this issue, so that I will be
> >> >> able to propose a fix on the above lines.
> >> >>
> >> >> Also Cc'ing kexec mailing list for more inputs on changes proposed to
> >> >> kexec-tools.
> >> >>
> >> >> Thanks,
> >> >> Bhupesh
^ permalink raw reply
* [PATCH] arm: dts: Remove leading 0x and 0s from bindings notation
From: Krzysztof Kozlowski @ 2017-12-15 8:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214165350.27850-1-malat@debian.org>
On Thu, Dec 14, 2017 at 5:53 PM, Mathieu Malaterre <malat@debian.org> wrote:
> Improve the DTS files by removing all the leading "0x" and zeros to fix the
> following dtc warnings:
>
> Warning (unit_address_format): Node /XXX unit name should not have leading "0x"
>
> and
>
> Warning (unit_address_format): Node /XXX unit name should not have leading 0s
>
> Converted using the following command:
>
> find . -type f \( -iname *.dts -o -iname *.dtsi \) -exec sed -E -i -e "s/@0x([0-9a-fA-F\.]+)\s?\{/@\L\1 \{/g" -e "s/@0+([0-9a-fA-F\.]+)\s?\{/@\L\1 \{/g" {} +
>
> For simplicity, two sed expressions were used to solve each warnings separately.
>
> To make the regex expression more robust a few other issues were resolved,
> namely setting unit-address to lower case, and adding a whitespace before the
> the opening curly brace:
>
> https://elinux.org/Device_Tree_Linux#Linux_conventions
>
> This is a follow up to commit 4c9847b7375a ("dt-bindings: Remove leading 0x from bindings notation")
>
> Reported-by: David Daney <ddaney@caviumnetworks.com>
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> ---
> arch/arm/boot/dts/am3517.dtsi | 4 +--
> arch/arm/boot/dts/arm-realview-eb.dtsi | 18 +++++++-------
> arch/arm/boot/dts/arm-realview-pb1176.dts | 18 +++++++-------
> arch/arm/boot/dts/arm-realview-pb11mp.dts | 18 +++++++-------
> arch/arm/boot/dts/arm-realview-pbx.dtsi | 18 +++++++-------
> arch/arm/boot/dts/artpec6.dtsi | 2 +-
> arch/arm/boot/dts/at91sam9261.dtsi | 2 +-
> arch/arm/boot/dts/at91sam9261ek.dts | 2 +-
> arch/arm/boot/dts/at91sam9263.dtsi | 2 +-
> arch/arm/boot/dts/at91sam9263ek.dts | 2 +-
> arch/arm/boot/dts/at91sam9g25ek.dts | 2 +-
> arch/arm/boot/dts/at91sam9g45.dtsi | 2 +-
> arch/arm/boot/dts/at91sam9m10g45ek.dts | 2 +-
> arch/arm/boot/dts/atlas7.dtsi | 12 ++++-----
> arch/arm/boot/dts/bcm11351.dtsi | 2 +-
> arch/arm/boot/dts/bcm21664.dtsi | 2 +-
> arch/arm/boot/dts/bcm283x.dtsi | 2 +-
> arch/arm/boot/dts/da850-lcdk.dts | 4 +--
> arch/arm/boot/dts/dm8148-evm.dts | 8 +++---
> arch/arm/boot/dts/dm8168-evm.dts | 8 +++---
> arch/arm/boot/dts/dra62x-j5eco-evm.dts | 8 +++---
> arch/arm/boot/dts/exynos5420.dtsi | 36 +++++++++++++--------------
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 2 +-
> arch/arm/boot/dts/imx7d.dtsi | 2 +-
> arch/arm/boot/dts/keystone-k2e-netcp.dtsi | 2 +-
> arch/arm/boot/dts/keystone-k2hk-netcp.dtsi | 2 +-
> arch/arm/boot/dts/keystone-k2l-netcp.dtsi | 2 +-
> arch/arm/boot/dts/omap3-cm-t3x.dtsi | 8 +++---
> arch/arm/boot/dts/omap3-evm-37xx.dts | 8 +++---
> arch/arm/boot/dts/omap3-lilly-a83x.dtsi | 8 +++---
> arch/arm/boot/dts/s3c2416.dtsi | 2 +-
For Exynos and S3C:
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Marc Zyngier @ 2017-12-15 9:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e5ccbf1c-6078-4c43-8aa3-fc226ed358f7@gmail.com>
On 15/12/17 02:27, Jia He wrote:
>
>
[...]
>> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
>>
>> /* Disable the virtual timer */
>> write_sysreg_el0(0, cntv_ctl);
>> + isb();
> My only concern is whether this isb() is required here?
> Sorryif this is a stupid question.I understand little about arm arch
> memory barrier. But seems isb will flush all the instruction prefetch.Do
> you think if an timer interrupt irq arrives, arm will use the previous
> instruction prefetch?
This barrier has little to do with prefetch. It just guarantees that the
code after the isb() is now running with a disabled virtual timer.
Otherwise, a CPU can freely reorder the write_sysreg() until the next
context synchronization event.
An interrupt coming between the write and the barrier will also act as a
context synchronization event. For more details, see the ARMv8 ARM (the
glossary has a section on the concept).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 1/6 v3] mmc: meson-gx-mmc: Fix platform_get_irq's error checking
From: Ulf Hansson @ 2017-12-15 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c1c27676e9f141ace1254b40807bf2dc03f072c6.1511066652.git.arvind.yadav.cs@gmail.com>
On 19 November 2017 at 05:52, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> The platform_get_irq() function returns negative if an error occurs.
> zero or positive number on success. platform_get_irq() error checking
> for zero is not correct.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> changes in v2 :
> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid.
> changes in v3 :
> return -EINVAL instead of irq.
>
> drivers/mmc/host/meson-gx-mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index e0862d3..32a6a22 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -1208,7 +1208,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
> }
>
> irq = platform_get_irq(pdev, 0);
> - if (!irq) {
> + if (irq <= 0) {
> dev_err(&pdev->dev, "failed to get interrupt resource.\n");
> ret = -EINVAL;
> goto free_host;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 2/6 v3] mmc: s3cmci: Fix platform_get_irq's error checking
From: Ulf Hansson @ 2017-12-15 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <870b1a28108a8c0f3885bd345f3d27dda67621d5.1511066652.git.arvind.yadav.cs@gmail.com>
On 19 November 2017 at 05:52, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> The platform_get_irq() function returns negative if an error occurs.
> zero or positive number on success. platform_get_irq() error checking
> for zero is not correct.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> changes in v2 :
> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid.
> changes in v3 :
> return -EINVAL instead of host->irq.
>
> drivers/mmc/host/s3cmci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/s3cmci.c b/drivers/mmc/host/s3cmci.c
> index f7f157a..36daee1 100644
> --- a/drivers/mmc/host/s3cmci.c
> +++ b/drivers/mmc/host/s3cmci.c
> @@ -1658,7 +1658,7 @@ static int s3cmci_probe(struct platform_device *pdev)
> }
>
> host->irq = platform_get_irq(pdev, 0);
> - if (host->irq == 0) {
> + if (host->irq <= 0) {
> dev_err(&pdev->dev, "failed to get interrupt resource.\n");
> ret = -EINVAL;
> goto probe_iounmap;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 3/6 v3] mmc: sdhci-acpi: Handle return value of platform_get_irq
From: Ulf Hansson @ 2017-12-15 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ecd11732b6a29aee23a1be94616755e575b0d438.1511066652.git.arvind.yadav.cs@gmail.com>
On 19 November 2017 at 05:52, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> platform_get_irq() can fail here and we must check its return value.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> changes in v2 :
> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid.
> changes in v3 :
> return -EINVAL instead of host->irq.
>
> drivers/mmc/host/sdhci-acpi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index b988997..0d9965b 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -566,6 +566,10 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
> host->hw_name = "ACPI";
> host->ops = &sdhci_acpi_ops_dflt;
> host->irq = platform_get_irq(pdev, 0);
> + if (host->irq <= 0) {
> + err = -EINVAL;
> + goto err_free;
> + }
>
> host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
> resource_size(iomem));
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 4/6 v3] mmc: sdhci-spear: Handle return value of platform_get_irq
From: Ulf Hansson @ 2017-12-15 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <23382b551ade00543aadc588aa74b0648958b907.1511066652.git.arvind.yadav.cs@gmail.com>
On 19 November 2017 at 05:52, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> platform_get_irq() can fail here and we must check its return value.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> changes in v2 :
> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid.
> changes in v3 :
> return -EINVAL instead of host->irq.
>
> drivers/mmc/host/sdhci-spear.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c
> index 8c0f884..e04485e 100644
> --- a/drivers/mmc/host/sdhci-spear.c
> +++ b/drivers/mmc/host/sdhci-spear.c
> @@ -82,6 +82,10 @@ static int sdhci_probe(struct platform_device *pdev)
> host->hw_name = "sdhci";
> host->ops = &sdhci_pltfm_ops;
> host->irq = platform_get_irq(pdev, 0);
> + if (host->irq <= 0) {
> + ret = -EINVAL;
> + goto err_host;
> + }
> host->quirks = SDHCI_QUIRK_BROKEN_ADMA;
>
> sdhci = sdhci_priv(host);
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 6/6 v3] mmc: sunxi-mmc: Handle return value of platform_get_irq
From: Ulf Hansson @ 2017-12-15 9:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6ad5c6077b96353a77ea30d1b64651c77d5622f6.1511066652.git.arvind.yadav.cs@gmail.com>
On 19 November 2017 at 05:52, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> platform_get_irq() can fail here and we must check its return value.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Thanks, applied for next!
Kind regards
Uffe
> ---
> changes in v2 :
> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid.
> changes in v3 :
> return -EINVAL instead of host->irq.
>
> drivers/mmc/host/sunxi-mmc.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index cc98355d..c926ac8 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1255,6 +1255,11 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> goto error_assert_reset;
>
> host->irq = platform_get_irq(pdev, 0);
> + if (host->irq <= 0) {
> + ret = -EINVAL;
> + goto error_assert_reset;
> + }
> +
> return devm_request_threaded_irq(&pdev->dev, host->irq, sunxi_mmc_irq,
> sunxi_mmc_handle_manual_stop, 0, "sunxi-mmc", host);
>
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH] media: v4l: xilinx: Use SPDX-License-Identifier
From: Mauro Carvalho Chehab @ 2017-12-15 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7952229.SXlKMv2tvC@avalon>
Em Fri, 15 Dec 2017 00:02:21 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> Hi Mauro,
>
> On Thursday, 14 December 2017 23:50:03 EET Mauro Carvalho Chehab wrote:
> > Em Thu, 14 Dec 2017 21:57:06 +0100 Greg KH escreveu:
> > > On Thu, Dec 14, 2017 at 10:44:16PM +0200, Laurent Pinchart wrote:
> > >> On Thursday, 14 December 2017 22:08:51 EET Greg KH wrote:
> > >>> On Thu, Dec 14, 2017 at 09:05:27PM +0200, Laurent Pinchart wrote:
> > >>>> On Thursday, 14 December 2017 20:54:39 EET Joe Perches wrote:
> > >>>>> On Thu, 2017-12-14 at 20:37 +0200, Laurent Pinchart wrote:
> > >>>>>> On Thursday, 14 December 2017 20:32:20 EET Joe Perches wrote:
> > >>>>>>> On Thu, 2017-12-14 at 20:28 +0200, Laurent Pinchart wrote:
> > >>>>>>>> On Thursday, 14 December 2017 19:05:27 EET Mauro Carvalho Chehab
> > >>>>>>>> wrote:
> > >>>>>>>>> Em Fri, 8 Dec 2017 18:05:37 +0530 Dhaval Shah escreveu:
> > >>>>>>>>>> SPDX-License-Identifier is used for the Xilinx Video IP and
> > >>>>>>>>>> related drivers.
> > >>>>>>>>>>
> > >>>>>>>>>> Signed-off-by: Dhaval Shah <dhaval23031987@gmail.com>
> > >>>>>>>>>
> > >>>>>>>>> Hi Dhaval,
> > >>>>>>>>>
> > >>>>>>>>> You're not listed as one of the Xilinx driver maintainers. I'm
> > >>>>>>>>> afraid that, without their explicit acks, sent to the ML, I
> > >>>>>>>>> can't accept a patch touching at the driver's license tags.
> > >>>>>>>>
> > >>>>>>>> The patch doesn't change the license, I don't see why it would
> > >>>>>>>> cause any issue. Greg isn't listed as the maintainer or copyright
> > >>>>>>>> holder of any of the 10k+ files to which he added an SPDX license
> > >>>>>>>> header in the last kernel release.
> > >>>>>>>
> > >>>>>>> Adding a comment line that describes an implicit or
> > >>>>>>> explicit license is different than removing the license
> > >>>>>>> text itself.
> > >>>>>>
> > >>>>>> The SPDX license header is meant to be equivalent to the license
> > >>>>>> text.
> > >>>>>
> > >>>>> I understand that.
> > >>>>> At a minimum, removing BSD license text is undesirable
> > >>>>>
> > >>>>> as that license states:
> > >>>>> * * Redistributions of source code must retain the above copyright
> > >>>>> * notice, this list of conditions and the following disclaimer.
> > >>>>>
> > >>>>> etc...
> > >>>>
> > >>>> But this patch only removes the following text:
> > >>>>
> > >>>> - * This program is free software; you can redistribute it and/or
> > >>>> modify
> > >>>> - * it under the terms of the GNU General Public License version 2 as
> > >>>> - * published by the Free Software Foundation.
> > >>>>
> > >>>> and replaces it by the corresponding SPDX header.
> > >>>>
> > >>>>>> The only reason why the large SPDX patch didn't touch the whole
> > >>>>>> kernel in one go was that it was easier to split in in multiple
> > >>>>>> chunks.
> > >>>>>
> > >>>>> Not really, it was scripted.
> > >>>>
> > >>>> But still manually reviewed as far as I know.
> > >>>>
> > >>>>>> This is no different than not including the full GPL license in
> > >>>>>> every header file but only pointing to it through its name and
> > >>>>>> reference, as every kernel source file does.
> > >>>>>
> > >>>>> Not every kernel source file had a license text
> > >>>>> or a reference to another license file.
> > >>>>
> > >>>> Correct, but the files touched by this patch do.
> > >>>>
> > >>>> This issue is in no way specific to linux-media and should be
> > >>>> decided upon at the top level, not on a per-subsystem basis. Greg,
> > >>>> could you comment on this ?
> > >>>
> > >>> Comment on what exactly? I don't understand the problem here, care to
> > >>> summarize it?
> > >>
> > >> In a nutshell (if I understand it correctly), Dhaval Shah submitted
> > >> https:// patchwork.kernel.org/patch/10102451/ which replaces
> > >>
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> [...]
> > >> - *
> > >> - * This program is free software; you can redistribute it and/or modify
> > >> - * it under the terms of the GNU General Public License version 2 as
> > >> - * published by the Free Software Foundation.
> > >>
> > >> in all .c and .h files of the Xilinx V4L2 driver
> > >> (drivers/media/platform/
> > >> xilinx). I have reviewed the patch and acked it. Mauro then rejected it,
> > >> stating that he can't accept a change to license text without an
> > >> explicit ack from the official driver's maintainers. My position is
> > >> that such a change doesn't change the license and thus doesn't need to
> > >> track all copyright holders, and can be merged without an explicit ack
> > >> from the respective maintainers.
> > >
> > > Yes, I agree with you, no license is being changed here, and no
> > > copyright is either.
> > >
> > > BUT, I know that most major companies are reviewing this process right
> > > now. We have gotten approval from almost all of the major kernel
> > > developer companies to do this, which is great, and supports this work
> > > as being acceptable.
> > >
> > > So it's nice to ask Xilinx if they object to this happening, which I
> > > guess Mauro is trying to say here (in not so many words...) To at least
> > > give them the heads-up that this is what is going to be going on
> > > throughout the kernel tree soon, and if they object, it would be good to
> > > speak up as to why (and if they do, I can put their lawyers in contact
> > > with some lawyers to explain it all to them.)
> >
> > Yes, that's basically what I'm saying.
> >
> > I don't feel comfortable on signing a patch changing the license text
> > without giving the copyright owners an opportunity and enough time
> > to review it and approve, or otherwise comment about such changes.
>
> If I understand you and Greg correctly, you would like to get a general
> approval from Xilinx for SPDX-related changes, but that would be a blanket
> approval that would cover this and all subsequent similar patches. Is that
> correct ? That is reasonable for me.
I doubt any Company's legal department would give a blanket approval for
others to touch on their licensing text.
>
> In that case, could the fact that commit
>
> commit 5fd54ace4721fc5ce2bb5aef6318fcf17f421460
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date: Fri Nov 3 11:28:30 2017 +0100
>
> USB: add SPDX identifiers to all remaining files in drivers/usb/
>
> add SPDX headers to several Xilinx-authored source files constitute such a
> blanket approval ?
If you look at this patch's summary:
651 files changed, 651 insertions(+)
And on the patch contents itself, it is just adding a SPDX header.
It doesn't remove any text from the license.
On Dhaval's patch, it is not only adding SPDX header. It is also removing
the legal text from it:
diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
index 522cdfdd3345..2e5daf7dba1a 100644
--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* Xilinx Video DMA
*
@@ -6,10 +7,6 @@
*
* Contacts: Hyun Kwon <hyun.kwon@xilinx.com>
* Laurent Pinchart <laurent.pinchart@ideasonboard.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
*/
And that's the part I'm more concerned about: we should give Xilinx
enough time to review and approve such change.
Thanks,
Mauro
^ permalink raw reply related
* [PATCH 1/2] soc: imx: gpc: Add i.MX6SX PCI power domain
From: Lucas Stach @ 2017-12-15 9:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513304698-15169-1-git-send-email-festevam@gmail.com>
Am Freitag, den 15.12.2017, 00:24 -0200 schrieb Fabio Estevam:
> > From: Fabio Estevam <fabio.estevam@nxp.com>
>
> i.MX6SX has a PCI power domain in PGC. Add support for it.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Seems like the GPC rework did turn out to work as expected by making it
easy to add additional power domains. :)
I didn't validate the register offsets, so this is:
Acked-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> ?Documentation/devicetree/bindings/power/fsl,imx-gpc.txt |??3 +++
> ?drivers/soc/imx/gpc.c???????????????????????????????????| 16 +++++++++++++++-
> ?2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> index e371b26..441f71e 100644
> --- a/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpc.txt
> @@ -9,6 +9,7 @@ Required properties:
> ???- fsl,imx6q-gpc
> ???- fsl,imx6qp-gpc
> ???- fsl,imx6sl-gpc
> +??- fsl,imx6sx-gpc
> ?- reg: should be register base and length as documented in the
> ???datasheet
> ?- interrupts: Should contain one interrupt specifier for the GPC interrupt
> @@ -29,6 +30,8 @@ Required properties:
> ???PU_DOMAIN??????1
> ???The following additional DOMAIN_INDEX value is valid for i.MX6SL:
> ???DISPLAY_DOMAIN 2
> +??The following additional DOMAIN_INDEX value is valid for i.MX6SX:
> +??PCI_DOMAIN?????3
> ?
> ?- #power-domain-cells: Should be 0
> ?
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 47e7aa9..53f7275 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -273,7 +273,15 @@ static struct imx_pm_domain imx_gpc_domains[] = {
> > ? },
> > ? .reg_offs = 0x240,
> > ? .cntr_pdn_bit = 4,
> > - }
> > + }, {
> > + .base = {
> > + .name = "PCI",
> > + .power_off = imx6_pm_domain_power_off,
> > + .power_on = imx6_pm_domain_power_on,
> > + },
> > + .reg_offs = 0x200,
> > + .cntr_pdn_bit = 6,
> > + },
> ?};
> ?
> ?struct imx_gpc_dt_data {
> @@ -296,10 +304,16 @@ static const struct imx_gpc_dt_data imx6sl_dt_data = {
> > ? .err009619_present = false,
> ?};
> ?
> +static const struct imx_gpc_dt_data imx6sx_dt_data = {
> > + .num_domains = 4,
> > + .err009619_present = false,
> +};
> +
> ?static const struct of_device_id imx_gpc_dt_ids[] = {
> > ? { .compatible = "fsl,imx6q-gpc", .data = &imx6q_dt_data },
> > ? { .compatible = "fsl,imx6qp-gpc", .data = &imx6qp_dt_data },
> > ? { .compatible = "fsl,imx6sl-gpc", .data = &imx6sl_dt_data },
> > + { .compatible = "fsl,imx6sx-gpc", .data = &imx6sx_dt_data },
> > ? { }
> ?};
> ?
^ permalink raw reply
* [PATCH] ARM: dts: imx6q-h100: use usdhc2 VSELECT
From: Michael Tretter @ 2017-12-15 9:26 UTC (permalink / raw)
To: linux-arm-kernel
The uSDHC controller directly provides a VSELECT signal that can be
muxed to the external voltage select. Mux the VSELECT directly to avoid
using a GPIO.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
arch/arm/boot/dts/imx6q-h100.dts | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/arch/arm/boot/dts/imx6q-h100.dts b/arch/arm/boot/dts/imx6q-h100.dts
index a3269f57df2b..450ec967c257 100644
--- a/arch/arm/boot/dts/imx6q-h100.dts
+++ b/arch/arm/boot/dts/imx6q-h100.dts
@@ -108,21 +108,6 @@
regulator-always-on;
};
- reg_nvcc_sd2: regulator-nvcc-sd2 {
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_h100_reg_nvcc_sd2>;
- compatible = "regulator-gpio";
- regulator-name = "NVCC_SD2";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <3300000>;
- regulator-type = "voltage";
- regulator-boot-on;
- regulator-always-on;
- gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
- states = <1800000 0x1
- 3300000 0x0>;
- };
-
reg_usbh1_vbus: regulator-usb-h1-vbus {
compatible = "regulator-fixed";
enable-active-high;
@@ -260,12 +245,6 @@
>;
};
- pinctrl_h100_reg_nvcc_sd2: h100-reg-nvcc-sd2 {
- fsl,pins = <
- MX6QDL_PAD_KEY_ROW1__GPIO4_IO09 0x1b0b0
- >;
- };
-
pinctrl_h100_sgtl5000: h100-sgtl5000 {
fsl,pins = <
MX6QDL_PAD_DISP0_DAT19__AUD5_RXD 0x130b0
@@ -316,6 +295,7 @@
MX6QDL_PAD_SD2_DAT1__SD2_DATA1 0x17059
MX6QDL_PAD_SD2_DAT2__SD2_DATA2 0x17059
MX6QDL_PAD_SD2_DAT3__SD2_DATA3 0x13059
+ MX6QDL_PAD_KEY_ROW1__SD2_VSELECT 0x1b0b0
>;
};
@@ -328,6 +308,7 @@
MX6QDL_PAD_SD2_DAT1__SD2_DATA1 0x170b9
MX6QDL_PAD_SD2_DAT2__SD2_DATA2 0x170b9
MX6QDL_PAD_SD2_DAT3__SD2_DATA3 0x170b9
+ MX6QDL_PAD_KEY_ROW1__SD2_VSELECT 0x1b0b0
>;
};
@@ -340,6 +321,7 @@
MX6QDL_PAD_SD2_DAT1__SD2_DATA1 0x170f9
MX6QDL_PAD_SD2_DAT2__SD2_DATA2 0x170f9
MX6QDL_PAD_SD2_DAT3__SD2_DATA3 0x170f9
+ MX6QDL_PAD_KEY_ROW1__SD2_VSELECT 0x1b0b0
>;
};
};
@@ -389,7 +371,6 @@
pinctrl-1 = <&pinctrl_h100_usdhc2_100mhz>;
pinctrl-2 = <&pinctrl_h100_usdhc2_200mhz>;
vmmc-supply = <®_3p3v>;
- vqmmc-supply = <®_nvcc_sd2>;
cd-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
status = "okay";
};
--
2.11.0
^ permalink raw reply related
* [RFC PATCH 2/7] gpio: gpiolib: split the gpiod_configure_flags function
From: Julien Thierry @ 2017-12-15 9:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214142138.23008-3-ludovic.desroches@microchip.com>
Hi Ludovic,
On 14/12/17 14:21, Ludovic Desroches wrote:
> The gpiod_configure_flags function doesn't only configure flags, it
> also performs some processing. It implies that it should be called
> after having requested the GPIO. Split configuration and processing
> to allow flags configuration before requesting the GPIO. It is
> needed if we want to set the pin configuration.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
> drivers/gpio/gpiolib.c | 49 +++++++++++++++++++++++++++++++------------------
> drivers/gpio/gpiolib.h | 7 ++++++-
> 2 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0621baddfddc..c887602ca0ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3564,23 +3564,9 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
> EXPORT_SYMBOL_GPL(gpiod_get_optional);
>
>
> -/**
> - * gpiod_configure_flags - helper function to configure a given GPIO
> - * @desc: gpio whose value will be assigned
> - * @con_id: function within the GPIO consumer
> - * @lflags: gpio_lookup_flags - returned from of_find_gpio() or
> - * of_get_gpio_hog()
> - * @dflags: gpiod_flags - optional GPIO initialization flags
> - *
> - * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> - */
> -int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> +void gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> unsigned long lflags, enum gpiod_flags dflags)
> {
> - int status;
> -
> if (lflags & GPIO_ACTIVE_LOW)
> set_bit(FLAG_ACTIVE_LOW, &desc->flags);
>
> @@ -3601,6 +3587,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> if (lflags & GPIO_OPEN_SOURCE)
> set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>
Small issue, I believe the patch is missing a '}' here to properly split
the function.
Otherwise:
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
Cheers,
--
Julien Thierry
^ permalink raw reply
* [PATCH 2/2] ARM: dts: imx6sx: Add support for PCI power domain
From: Lucas Stach @ 2017-12-15 9:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513304698-15169-2-git-send-email-festevam@gmail.com>
Am Freitag, den 15.12.2017, 00:24 -0200 schrieb Fabio Estevam:
> > From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Previously PCI support was working because the bootloader has previously
> powered up the PCI power domain.
>
> Represent the PCI power domain, so that PCI is functional without
> relying on the PCI support from the bootloader.
>
> Tested on a imx6sx-sdb board with no PCI support in the bootloader.
>
> > Reported-by: Abel Vesa <abel.vesa@nxp.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> ?arch/arm/boot/dts/imx6sx.dtsi | 14 ++++++++++++++
> ?1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index 40c6738c..c6ea6ec 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -750,6 +750,19 @@
> > ? #interrupt-cells = <3>;
> > ? interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > ? interrupt-parent = <&intc>;
> > + clocks = <&clks IMX6SX_CLK_IPG>;
> > + clock-names = "ipg";
> +
> > + pgc {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> +
> > > + pd_pci: power-domain at 3 {
> > + reg = <3>;
> > + #power-domain-cells = <0>;
> > + power-supply = <®_pcie>;
> > + };
> > + };
> > ? };
> ?
> > > ? iomuxc: iomuxc at 20e0000 {
> @@ -1328,6 +1341,7 @@
> > ? ?<&clks IMX6SX_CLK_PCIE_REF_125M>,
> > ? ?<&clks IMX6SX_CLK_DISPLAY_AXI>;
> > ? clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_inbound_axi";
> > + power-domains = <&pd_pci>;
> > ? status = "disabled";
> > ? };
> > ? };
^ permalink raw reply
* [PATCH] media: v4l: xilinx: Use SPDX-License-Identifier
From: Mauro Carvalho Chehab @ 2017-12-15 9:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOymtfJKzs=1xUeJOYqvxhb-o9r=aTUOLOM4-DMUj8xXdD2PUw@mail.gmail.com>
Em Fri, 15 Dec 2017 10:55:26 +0530
Dhaval Shah <dhaval23031987@gmail.com> escreveu:
> Hi Laurent/Mauro/Greg,
>
> On Fri, Dec 15, 2017 at 3:32 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Mauro,
> >
> > On Thursday, 14 December 2017 23:50:03 EET Mauro Carvalho Chehab wrote:
> >> Em Thu, 14 Dec 2017 21:57:06 +0100 Greg KH escreveu:
> >> > On Thu, Dec 14, 2017 at 10:44:16PM +0200, Laurent Pinchart wrote:
> >> >> On Thursday, 14 December 2017 22:08:51 EET Greg KH wrote:
> >> >>> On Thu, Dec 14, 2017 at 09:05:27PM +0200, Laurent Pinchart wrote:
> >> >>>> On Thursday, 14 December 2017 20:54:39 EET Joe Perches wrote:
> >> >>>>> On Thu, 2017-12-14 at 20:37 +0200, Laurent Pinchart wrote:
> >> >>>>>> On Thursday, 14 December 2017 20:32:20 EET Joe Perches wrote:
> >> >>>>>>> On Thu, 2017-12-14 at 20:28 +0200, Laurent Pinchart wrote:
> >> >>>>>>>> On Thursday, 14 December 2017 19:05:27 EET Mauro Carvalho Chehab
> >> >>>>>>>> wrote:
> >> >>>>>>>>> Em Fri, 8 Dec 2017 18:05:37 +0530 Dhaval Shah escreveu:
> >> >>>>>>>>>> SPDX-License-Identifier is used for the Xilinx Video IP and
> >> >>>>>>>>>> related drivers.
> >> >>>>>>>>>>
> >> >>>>>>>>>> Signed-off-by: Dhaval Shah <dhaval23031987@gmail.com>
> >> >>>>>>>>>
> >> >>>>>>>>> Hi Dhaval,
> >> >>>>>>>>>
> >> >>>>>>>>> You're not listed as one of the Xilinx driver maintainers. I'm
> >> >>>>>>>>> afraid that, without their explicit acks, sent to the ML, I
> >> >>>>>>>>> can't accept a patch touching at the driver's license tags.
> >> >>>>>>>>
> >> >>>>>>>> The patch doesn't change the license, I don't see why it would
> >> >>>>>>>> cause any issue. Greg isn't listed as the maintainer or copyright
> >> >>>>>>>> holder of any of the 10k+ files to which he added an SPDX license
> >> >>>>>>>> header in the last kernel release.
> >> >>>>>>>
> >> >>>>>>> Adding a comment line that describes an implicit or
> >> >>>>>>> explicit license is different than removing the license
> >> >>>>>>> text itself.
> >> >>>>>>
> >> >>>>>> The SPDX license header is meant to be equivalent to the license
> >> >>>>>> text.
> >> >>>>>
> >> >>>>> I understand that.
> >> >>>>> At a minimum, removing BSD license text is undesirable
> >> >>>>>
> >> >>>>> as that license states:
> >> >>>>> * * Redistributions of source code must retain the above copyright
> >> >>>>> * notice, this list of conditions and the following disclaimer.
> >> >>>>>
> >> >>>>> etc...
> >> >>>>
> >> >>>> But this patch only removes the following text:
> >> >>>>
> >> >>>> - * This program is free software; you can redistribute it and/or
> >> >>>> modify
> >> >>>> - * it under the terms of the GNU General Public License version 2 as
> >> >>>> - * published by the Free Software Foundation.
> >> >>>>
> >> >>>> and replaces it by the corresponding SPDX header.
> >> >>>>
> >> >>>>>> The only reason why the large SPDX patch didn't touch the whole
> >> >>>>>> kernel in one go was that it was easier to split in in multiple
> >> >>>>>> chunks.
> >> >>>>>
> >> >>>>> Not really, it was scripted.
> >> >>>>
> >> >>>> But still manually reviewed as far as I know.
> >> >>>>
> >> >>>>>> This is no different than not including the full GPL license in
> >> >>>>>> every header file but only pointing to it through its name and
> >> >>>>>> reference, as every kernel source file does.
> >> >>>>>
> >> >>>>> Not every kernel source file had a license text
> >> >>>>> or a reference to another license file.
> >> >>>>
> >> >>>> Correct, but the files touched by this patch do.
> >> >>>>
> >> >>>> This issue is in no way specific to linux-media and should be
> >> >>>> decided upon at the top level, not on a per-subsystem basis. Greg,
> >> >>>> could you comment on this ?
> >> >>>
> >> >>> Comment on what exactly? I don't understand the problem here, care to
> >> >>> summarize it?
> >> >>
> >> >> In a nutshell (if I understand it correctly), Dhaval Shah submitted
> >> >> https:// patchwork.kernel.org/patch/10102451/ which replaces
> >> >>
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> [...]
> >> >> - *
> >> >> - * This program is free software; you can redistribute it and/or modify
> >> >> - * it under the terms of the GNU General Public License version 2 as
> >> >> - * published by the Free Software Foundation.
> >> >>
> >> >> in all .c and .h files of the Xilinx V4L2 driver
> >> >> (drivers/media/platform/
> >> >> xilinx). I have reviewed the patch and acked it. Mauro then rejected it,
> >> >> stating that he can't accept a change to license text without an
> >> >> explicit ack from the official driver's maintainers. My position is
> >> >> that such a change doesn't change the license and thus doesn't need to
> >> >> track all copyright holders, and can be merged without an explicit ack
> >> >> from the respective maintainers.
> >> >
> >> > Yes, I agree with you, no license is being changed here, and no
> >> > copyright is either.
> >> >
> >> > BUT, I know that most major companies are reviewing this process right
> >> > now. We have gotten approval from almost all of the major kernel
> >> > developer companies to do this, which is great, and supports this work
> >> > as being acceptable.
> >> >
> >> > So it's nice to ask Xilinx if they object to this happening, which I
> >> > guess Mauro is trying to say here (in not so many words...) To at least
> >> > give them the heads-up that this is what is going to be going on
> >> > throughout the kernel tree soon, and if they object, it would be good to
> >> > speak up as to why (and if they do, I can put their lawyers in contact
> >> > with some lawyers to explain it all to them.)
> >>
> >> Yes, that's basically what I'm saying.
> >>
> >> I don't feel comfortable on signing a patch changing the license text
> >> without giving the copyright owners an opportunity and enough time
> >> to review it and approve, or otherwise comment about such changes.
> >
> > If I understand you and Greg correctly, you would like to get a general
> > approval from Xilinx for SPDX-related changes, but that would be a blanket
> > approval that would cover this and all subsequent similar patches. Is that
> > correct ? That is reasonable for me.
> >
> > In that case, could the fact that commit
> >
> > commit 5fd54ace4721fc5ce2bb5aef6318fcf17f421460
> > Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Date: Fri Nov 3 11:28:30 2017 +0100
> >
> > USB: add SPDX identifiers to all remaining files in drivers/usb/
> >
> > add SPDX headers to several Xilinx-authored source files constitute such a
> > blanket approval ?
> >
> I have to do anything here or Once, we get approval from the Michal
> Simek(michal.simek at xilinx.com) and Hyun.kwon at xilinx.com ACK this patch
> then it will go into mainline?
I would wait for their feedback.
Thanks,
Mauro
^ permalink raw reply
* [PATCH v4 2/4] ARM: PWM: add allwinner sun8i R40/V40/T3 pwm support.
From: Claudiu Beznea @ 2017-12-15 9:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2d5b5421-1c1a-f293-e197-7a1a673ddbaa@microchip.com>
On 14.12.2017 12:44, Claudiu Beznea wrote:
>
>
> On 13.12.2017 16:44, hao_zhang wrote:
>> This patch add allwinner sun8i R40/V40/T3 pwm support.
>>
>> Signed-off-by: hao_zhang <hao5781286@gmail.com>
>> ---
>> drivers/pwm/Kconfig | 10 +
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-sun8i-r40.c | 449 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 460 insertions(+)
>> create mode 100644 drivers/pwm/pwm-sun8i-r40.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 763ee50..cde5a70 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -444,6 +444,16 @@ config PWM_SUN4I
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sun4i.
>>
>> +config PWM_SUN8I_R40
>> + tristate "Allwinner PWM SUN8I R40 support"
>> + depends on ARCH_SUNXI || COMPILE_TEST
>> + depends on HAS_IOMEM && COMMON_CLK
>> + help
>> + Generic PWM framework driver for Allwinner SoCs R40, V40, T3.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-sun8i-r40.
>> +
>> config PWM_TEGRA
>> tristate "NVIDIA Tegra PWM support"
>> depends on ARCH_TEGRA
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 0258a74..026a55b 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
>> obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>> +obj-$(CONFIG_PWM_SUN8I_R40) += pwm-sun8i-r40.o
>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
>> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
>> diff --git a/drivers/pwm/pwm-sun8i-r40.c b/drivers/pwm/pwm-sun8i-r40.c
>> new file mode 100644
>> index 0000000..8df538d
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sun8i-r40.c
>> @@ -0,0 +1,449 @@
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/time.h>
>> +
>> +#define PWM_IRQ_ENABLE_REG 0x0000
>> +#define PCIE(ch) BIT(ch)
>> +
>> +#define PWM_IRQ_STATUS_REG 0x0004
>> +#define PIS(ch) BIT(ch)
>> +
>> +#define CAPTURE_IRQ_ENABLE_REG 0x0010
>> +#define CFIE(ch) BIT(ch << 1 + 1)
>> +#define CRIE(ch) BIT(ch << 1)
>> +
>> +#define CAPTURE_IRQ_STATUS_REG 0x0014
>> +#define CFIS(ch) BIT(ch << 1 + 1)
>> +#define CRIS(ch) BIT(ch << 1)
>> +
>> +#define CLK_CFG_REG(ch) (0x0020 + (ch >> 1) * 4)
>> +#define CLK_SRC BIT(7)
>> +#define CLK_SRC_BYPASS_SEC BIT(6)
>> +#define CLK_SRC_BYPASS_FIR BIT(5)
>> +#define CLK_GATING BIT(4)
>> +#define CLK_DIV_M GENMASK(3, 0)
>> +
>> +#define PWM_DZ_CTR_REG(ch) (0x0030 + (ch >> 1) * 4)
>> +#define PWM_DZ_INTV GENMASK(15, 8)
>> +#define PWM_DZ_EN BIT(0)
>> +
>> +#define PWM_ENABLE_REG 0x0040
>> +#define PWM_EN(ch) BIT(ch)
>> +
>> +#define CAPTURE_ENABLE_REG 0x0044
>> +#define CAP_EN(ch) BIT(ch)
>> +
>> +#define PWM_CTR_REG(ch) (0x0060 + ch * 0x20)
>> +#define PWM_PERIOD_RDY BIT(11)
>> +#define PWM_PUL_START BIT(10)
>> +#define PWM_MODE BIT(9)
>> +#define PWM_ACT_STA BIT(8)
>> +#define PWM_PRESCAL_K GENMASK(7, 0)
>> +
>> +#define PWM_PERIOD_REG(ch) (0x0064 + ch * 0x20)
>> +#define PWM_ENTIRE_CYCLE GENMASK(31, 16)
>> +#define PWM_ACT_CYCLE GENMASK(15, 0)
>> +
>> +#define PWM_CNT_REG(ch) (0x0068 + ch * 0x20)
>> +#define PWM_CNT_VAL GENMASK(15, 0)
>> +
>> +#define CAPTURE_CTR_REG(ch) (0x006c + ch * 0x20)
>> +#define CAPTURE_CRLF BIT(2)
>> +#define CAPTURE_CFLF BIT(1)
>> +#define CAPINV BIT(0)
>> +
>> +#define CAPTURE_RISE_REG(ch) (0x0070 + ch * 0x20)
>> +#define CAPTURE_CRLR GENMASK(15, 0)
>> +
>> +#define CAPTURE_FALL_REG(ch) (0x0074 + ch * 0x20)
>> +#define CAPTURE_CFLR GENMASK(15, 0)
>> +
>> +struct sunxi_pwm_data {
>> + bool has_prescaler_bypass;
>> + bool has_rdy;
>> + unsigned int npwm;
>> +};
>> +
>> +struct sunxi_pwm_chip {
>> + struct pwm_chip chip;
>> + struct clk *clk;
>> + void __iomem *base;
>> + spinlock_t ctrl_lock;
>> + const struct sunxi_pwm_data *data;
>> +};
>> +
>> +static const u16 div_m_table[] = {
>> + 1,
>> + 2,
>> + 4,
>> + 8,
>> + 16,
>> + 32,
>> + 64,
>> + 128,
>> + 256
>> +};
>> +
>> +static inline struct sunxi_pwm_chip *to_sunxi_pwm_chip(struct pwm_chip *chip)
>> +{
>> + return container_of(chip, struct sunxi_pwm_chip, chip);
>> +}
>> +
>> +static inline u32 sunxi_pwm_readl(struct sunxi_pwm_chip *chip,
>> + unsigned long offset)
>> +{
>> + return readl(chip->base + offset);
>> +}
>> +
>> +static inline void sunxi_pwm_writel(struct sunxi_pwm_chip *chip,
>> + u32 val, unsigned long offset)
>> +{
>> + writel(val, chip->base + offset);
>> +}
>> +
>> +static void sunxi_pwm_set_bit(struct sunxi_pwm_chip *sunxi_pwm,
>> + unsigned long reg, u32 bit)
>> +{
>> + u32 val;
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, reg);
>> + val |= bit;
>> + sunxi_pwm_writel(sunxi_pwm, val, reg);
>> +}
>> +
>> +static void sunxi_pwm_clear_bit(struct sunxi_pwm_chip *sunxi_pwm,
>> + unsigned long reg, u32 bit)
>> +{
>> + u32 val;
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, reg);
>> + val &= ~bit;
>> + sunxi_pwm_writel(sunxi_pwm, val, reg);
>> +}
>> +
>> +static void sunxi_pwm_set_value(struct sunxi_pwm_chip *sunxi_pwm,
>> + unsigned long reg, u32 mask, u32 val)
>> +{
>> + u32 tmp;
>> +
>> + tmp = sunxi_pwm_readl(sunxi_pwm, reg);
>> + tmp &= ~mask;
>> + tmp |= mask & val;
>> + sunxi_pwm_writel(sunxi_pwm, tmp, reg);
>> +}
>> +
>> +static void sunxi_pwm_set_polarity(struct sunxi_pwm_chip *chip, u32 ch,
>> + enum pwm_polarity polarity)
>> +{
>> + if (polarity == PWM_POLARITY_NORMAL)
>> + sunxi_pwm_set_bit(chip, PWM_CTR_REG(ch), PWM_ACT_STA);
>> + else
>> + sunxi_pwm_clear_bit(chip, PWM_CTR_REG(ch), PWM_ACT_STA);
>> +}
>> +
>> +static void sunxi_dump_reg(struct sunxi_pwm_chip *sunxi_pwm, u8 ch)
>> +{
>> + dev_dbg(sunxi_pwm->chip.dev, "ch: %d\n"
>> + "\tPWM_IRQ_ENABLE_REG(%04x): \t0x%08x\n"
>> + "\tPWM_IRQ_STATUS_REG(%04x): \t0x%08x\n"
>> + "\tCAPTURE_IRQ_ENABLE_REG(%04x): \t0x%08x\n"
>> + "\tCAPTURE_IRQ_STATUS_REG(%04x): \t0x%08x\n"
>> + "\tCLK_CFG_REG(%04x)(%d): \t0x%08x\n"
>> + "\tPWM_DZ_CTR_REG(%04x)(%d): \t0x%08x\n"
>> + "\tPWM_ENABLE_REG(%04x): \t0x%08x\n"
>> + "\tCAPTURE_ENABLE_REG(%04x): \t0x%08x\n"
>> + "\tPWM_CTR_REG(%04x)(%d): \t0x%08x\n"
>> + "\tPWM_PERIOD_REG(%04x)(%d): \t0x%08x\n"
>> + "\tPWM_CNT_REG(%04x)(%d): \t0x%08x\n"
>> + "\tCAPTURE_CTR_REG(%04x)(%d): \t0x%08x\n"
>> + "\tCAPTURE_RISE_REG(%04x)(%d): \t0x%08x\n"
>> + "\tCAPTURE_FALL_REG(%04x)(%d): \t0x%08x\n",
>> + ch,
>> + PWM_IRQ_ENABLE_REG,
>> + readl(sunxi_pwm->base + PWM_IRQ_ENABLE_REG),
>> + PWM_IRQ_STATUS_REG,
>> + readl(sunxi_pwm->base + PWM_IRQ_STATUS_REG),
>> + CAPTURE_IRQ_ENABLE_REG,
>> + readl(sunxi_pwm->base + CAPTURE_IRQ_ENABLE_REG),
>> + CAPTURE_IRQ_STATUS_REG,
>> + readl(sunxi_pwm->base + CAPTURE_IRQ_STATUS_REG),
>> + CLK_CFG_REG(ch),
>> + ch, readl(sunxi_pwm->base + CLK_CFG_REG(ch)),
>> + PWM_DZ_CTR_REG(ch),
>> + ch, readl(sunxi_pwm->base + PWM_DZ_CTR_REG(ch)),
>> + PWM_ENABLE_REG,
>> + readl(sunxi_pwm->base + PWM_ENABLE_REG),
>> + CAPTURE_ENABLE_REG,
>> + readl(sunxi_pwm->base + CAPTURE_ENABLE_REG),
>> + PWM_CTR_REG(ch),
>> + ch, readl(sunxi_pwm->base + PWM_CTR_REG(ch)),
>> + PWM_PERIOD_REG(ch),
>> + ch, readl(sunxi_pwm->base + PWM_PERIOD_REG(ch)),
>> + PWM_CNT_REG(ch),
>> + ch, readl(sunxi_pwm->base + PWM_CNT_REG(ch)),
>> + CAPTURE_CTR_REG(ch),
>> + ch, readl(sunxi_pwm->base + CAPTURE_CTR_REG(ch)),
>> + CAPTURE_RISE_REG(ch),
>> + ch, readl(sunxi_pwm->base + CAPTURE_RISE_REG(ch)),
>> + CAPTURE_FALL_REG(ch),
>> + ch, readl(sunxi_pwm->base + CAPTURE_FALL_REG(ch)));
>> +}
>> +
>> +static int sunxi_pwm_config(struct sunxi_pwm_chip *sunxi_pwm, u8 ch,
>> + struct pwm_state *state)
>> +{
>> + u64 clk_rate, clk_div, val;
>> + u16 prescaler = 0;
>> + u8 id = 0;
>> +
>> + clk_rate = clk_get_rate(sunxi_pwm->clk);
>> + dev_dbg(sunxi_pwm->chip.dev, "clock rate:%lld\n", clk_rate);
>> +
>> + if (clk_rate == 24000000)
>> + sunxi_pwm_clear_bit(sunxi_pwm, CLK_CFG_REG(ch), CLK_SRC);
>> + else
>> + sunxi_pwm_set_bit(sunxi_pwm, CLK_CFG_REG(ch), CLK_SRC);
>> +
>> + if (sunxi_pwm->data->has_prescaler_bypass) {
>> + /* pwm output bypass */
>> + if (ch % 2)
>> + sunxi_pwm_set_bit(sunxi_pwm, CLK_CFG_REG(ch),
>> + CLK_SRC_BYPASS_FIR);
>> + else
>> + sunxi_pwm_set_bit(sunxi_pwm, CLK_CFG_REG(ch),
>> + CLK_SRC_BYPASS_SEC);
>> + return 0;
>> + }
>> +
>> + val = state->period * clk_rate;
>> + do_div(val, NSEC_PER_SEC);
>> + if (val < 1) {
>> + dev_err(sunxi_pwm->chip.dev,
>> + "period expects a larger value\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* calculate and set prescalar, div table, pwn entrie cycle */
>> + clk_div = val;
>> +
>> + while (clk_div > 65535) {
>> + prescaler++;
>> + clk_div = val;
>> + do_div(clk_div, prescaler + 1);
>> + do_div(clk_div, div_m_table[id]);
>> +
>> + if (prescaler == 255) {
>> + prescaler = 0;
>> + id++;
>> + if (id == 9)
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + sunxi_pwm_set_value(sunxi_pwm, PWM_PERIOD_REG(ch),
>> + PWM_ENTIRE_CYCLE, clk_div << 16);
>> + sunxi_pwm_set_value(sunxi_pwm, PWM_CTR_REG(ch),
>> + PWM_PRESCAL_K, prescaler << 0);
>> + sunxi_pwm_set_value(sunxi_pwm, CLK_CFG_REG(ch),
>> + CLK_DIV_M, id << 0);
>> +
>> + /* set duty cycle */
>> + val = (prescaler + 1) * div_m_table[id] * clk_div;
>> + val = state->period;
>> + do_div(val, clk_div);
>> + clk_div = state->duty_cycle;
>> + do_div(clk_div, val);
>> +
>> + sunxi_pwm_set_value(sunxi_pwm, PWM_PERIOD_REG(ch),
>> + PWM_ACT_CYCLE, clk_div << 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int sunxi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + int ret;
>> + struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
>> + struct pwm_state cstate;
>> +
>> + sunxi_dump_reg(sunxi_pwm, pwm->hwpwm);
>> + pwm_get_state(pwm, &cstate);
>> + if (!cstate.enabled) {
>> + ret = clk_prepare_enable(sunxi_pwm->clk);
>> + if (ret) {
>> + dev_err(chip->dev, "failed to enable PWM clock\n");
>> + return ret;
>> + }
>> + }
>> +
>> + spin_lock(&sunxi_pwm->ctrl_lock);
>> +
>> + if (state->polarity != cstate.polarity)
>> + sunxi_pwm_set_polarity(sunxi_pwm, pwm->hwpwm, state->polarity);
>> +
>> + if ((cstate.period != state->period) ||
>> + (cstate.duty_cycle != state->duty_cycle)) {
>> + ret = sunxi_pwm_config(sunxi_pwm, pwm->hwpwm, state);
>> + if (ret) {
>> + dev_err(chip->dev, "failed to enable PWM clock\n");
>> + return ret;
> Here you might want to undo the previous polarity and do clk_disable_unprepare(),
> if any, in order to not reach inconsistency b/w software state of PWM and hardware
> state. Also, unlock the spinlock.
>> + }
>> + }
>> +
>> + if (state->enabled) {
>> + sunxi_pwm_set_bit(sunxi_pwm,
>> + CLK_CFG_REG(pwm->hwpwm), CLK_GATING);
>> +
>> + sunxi_pwm_set_bit(sunxi_pwm,
>> + PWM_ENABLE_REG, PWM_EN(pwm->hwpwm));
>> + } else {
>> + sunxi_pwm_clear_bit(sunxi_pwm,
>> + CLK_CFG_REG(pwm->hwpwm), CLK_GATING);
>> +
>> + sunxi_pwm_clear_bit(sunxi_pwm,
>> + PWM_ENABLE_REG, PWM_EN(pwm->hwpwm));
>> + }
>> +
>> + spin_unlock(&sunxi_pwm->ctrl_lock);
>> + sunxi_dump_reg(sunxi_pwm, pwm->hwpwm);
>> +
>> + return 0;
>> +}
>> +
>> +static void sunxi_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + struct sunxi_pwm_chip *sunxi_pwm = to_sunxi_pwm_chip(chip);
>> + u64 clk_rate, tmp;
>> + u32 val;
>> + u16 clk_div, act_cycle;
>> + u8 prescal, id;
>> +
>> + clk_rate = clk_get_rate(sunxi_pwm->clk);
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, PWM_CTR_REG(pwm->hwpwm));
>> + if (PWM_ACT_STA & val)
>> + state->polarity = PWM_POLARITY_NORMAL;
>> + else
>> + state->polarity = PWM_POLARITY_INVERSED;
>> +
>> + prescal = PWM_PRESCAL_K & val;
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, PWM_ENABLE_REG);
>> + if (PWM_EN(pwm->hwpwm) & val)
>> + state->enabled = true;
>> + else
>> + state->enabled = false;
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, PWM_PERIOD_REG(pwm->hwpwm));
>> + act_cycle = PWM_ACT_CYCLE & val;
>> + clk_div = val >> 16;
>> +
>> + val = sunxi_pwm_readl(sunxi_pwm, CLK_CFG_REG(pwm->hwpwm));
>> + id = CLK_DIV_M & val;
>> +
>> + tmp = act_cycle * prescal * div_m_table[id] * NSEC_PER_SEC;
>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
>> + tmp = clk_div * prescal * div_m_table[id] * NSEC_PER_SEC;
>> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
>> +}
>> +
>> +static const struct pwm_ops sunxi_pwm_ops = {
>> + .apply = sunxi_pwm_apply,
>> + .get_state = sunxi_pwm_get_state,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static const struct sunxi_pwm_data sunxi_pwm_data_r40 = {
>> + .has_prescaler_bypass = false,
>> + .has_rdy = true,
>> + .npwm = 8,
>> +};
>> +
>> +static const struct of_device_id sunxi_pwm_dt_ids[] = {
>> + {
>> + .compatible = "allwinner,sun8i-r40-pwm",
>> + .data = &sunxi_pwm_data_r40,
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sunxi_pwm_dt_ids);
>> +
>> +static int sunxi_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct sunxi_pwm_chip *pwm;
>> + struct resource *res;
>> + int ret;
>> + const struct of_device_id *match;
>> +
>> + match = of_match_device(sunxi_pwm_dt_ids, &pdev->dev);
>> +
>> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> check for res == NULL
My bad here, no need to check res == NULL since devm_ioremap_resource() will take
care of this case.
Thanks,
Claudiu
>> + pwm->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(pwm->base))
>> + return PTR_ERR(pwm->base);
>> +
>> + pwm->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(pwm->clk))
>> + return PTR_ERR(pwm->clk);
>> +
>> + pwm->data = match->data;
>> + pwm->chip.dev = &pdev->dev;
>> + pwm->chip.ops = &sunxi_pwm_ops;
>> + pwm->chip.base = -1;
>> + pwm->chip.npwm = pwm->data->npwm;
>> + pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> + pwm->chip.of_pwm_n_cells = 3;
>> +
>> + dev_dbg(pwm->chip.dev, "npwm: %d\n", pwm->chip.npwm);
>> +
>> + spin_lock_init(&pwm->ctrl_lock);
>> +
>> + ret = pwmchip_add(&pwm->chip);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int sunxi_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct sunxi_pwm_chip *pwm = platform_get_drvdata(pdev);
>> +
>> + return pwmchip_remove(&pwm->chip);
>> +}
>> +
>> +static struct platform_driver sunxi_pwm_driver = {
>> + .driver = {
>> + .name = "sun8i-r40-pwm",
>> + .of_match_table = sunxi_pwm_dt_ids,
>> + },
>> + .probe = sunxi_pwm_probe,
>> + .remove = sunxi_pwm_remove,
>> +};
>> +module_platform_driver(sunxi_pwm_driver);
>> +
>> +MODULE_ALIAS("platform:sun8i-r40-pwm");
>> +MODULE_AUTHOR("Hao Zhang <hao5781286@gmail.com>");
>> +MODULE_DESCRIPTION("Allwinner sun8i-r40 PWM driver");
>> +MODULE_LICENSE("GPL v2");
>>
^ permalink raw reply
* arm64 crashkernel fails to boot on acpi-only machines due to ACPI regions being no longer mapped as NOMAP
From: Ard Biesheuvel @ 2017-12-15 9:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171215085924.sqlcwm4copzba5ag@fireball>
On 15 December 2017 at 09:59, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
> On Wed, Dec 13, 2017 at 12:17:22PM +0000, Ard Biesheuvel wrote:
>> On 13 December 2017 at 12:16, AKASHI Takahiro
>> <takahiro.akashi@linaro.org> wrote:
>> > On Wed, Dec 13, 2017 at 10:49:27AM +0000, Ard Biesheuvel wrote:
>> >> On 13 December 2017 at 10:26, AKASHI Takahiro
>> >> <takahiro.akashi@linaro.org> wrote:
>> >> > Bhupesh, Ard,
>> >> >
>> >> > On Wed, Dec 13, 2017 at 03:21:59AM +0530, Bhupesh Sharma wrote:
>> >> >> Hi Ard, Akashi
>> >> >>
>> >> > (snip)
>> >> >
>> >> >> Looking deeper into the issue, since the arm64 kexec-tools uses the
>> >> >> 'linux,usable-memory-range' dt property to allow crash dump kernel to
>> >> >> identify its own usable memory and exclude, at its boot time, any
>> >> >> other memory areas that are part of the panicked kernel's memory.
>> >> >> (see https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
>> >> >> , for details)
>> >> >
>> >> > Right.
>> >> >
>> >> >> 1). Now when 'kexec -p' is executed, this node is patched up only
>> >> >> with the crashkernel memory range:
>> >> >>
>> >> >> /* add linux,usable-memory-range */
>> >> >> nodeoffset = fdt_path_offset(new_buf, "/chosen");
>> >> >> result = fdt_setprop_range(new_buf, nodeoffset,
>> >> >> PROP_USABLE_MEM_RANGE, &crash_reserved_mem,
>> >> >> address_cells, size_cells);
>> >> >>
>> >> >> (see https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/arm64/kexec-arm64.c#n465
>> >> >> , for details)
>> >> >>
>> >> >> 2). This excludes the ACPI reclaim regions irrespective of whether
>> >> >> they are marked as System RAM or as RESERVED. As,
>> >> >> 'linux,usable-memory-range' dt node is patched up only with
>> >> >> 'crash_reserved_mem' and not 'system_memory_ranges'
>> >> >>
>> >> >> 3). As a result when the crashkernel boots up it doesn't find this
>> >> >> ACPI memory and crashes while trying to access the same:
>> >> >>
>> >> >> # kexec -p /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname
>> >> >> -r`.img --reuse-cmdline -d
>> >> >>
>> >> >> [snip..]
>> >> >>
>> >> >> Reserved memory range
>> >> >> 000000000e800000-000000002e7fffff (0)
>> >> >>
>> >> >> Coredump memory ranges
>> >> >> 0000000000000000-000000000e7fffff (0)
>> >> >> 000000002e800000-000000003961ffff (0)
>> >> >> 0000000039d40000-000000003ed2ffff (0)
>> >> >> 000000003ed60000-000000003fbfffff (0)
>> >> >> 0000001040000000-0000001ffbffffff (0)
>> >> >> 0000002000000000-0000002ffbffffff (0)
>> >> >> 0000009000000000-0000009ffbffffff (0)
>> >> >> 000000a000000000-000000affbffffff (0)
>> >> >>
>> >> >> 4). So if we revert Ard's patch or just comment the fixing up of the
>> >> >> memory cap'ing passed to the crash kernel inside
>> >> >> 'arch/arm64/mm/init.c' (see below):
>> >> >>
>> >> >> static void __init fdt_enforce_memory_region(void)
>> >> >> {
>> >> >> struct memblock_region reg = {
>> >> >> .size = 0,
>> >> >> };
>> >> >>
>> >> >> of_scan_flat_dt(early_init_dt_scan_usablemem, ®);
>> >> >>
>> >> >> if (reg.size)
>> >> >> //memblock_cap_memory_range(reg.base, reg.size); /*
>> >> >> comment this out */
>> >> >> }
>> >> >
>> >> > Please just don't do that. It can cause a fatal damage on
>> >> > memory contents of the *crashed* kernel.
>> >> >
>> >> >> 5). Both the above temporary solutions fix the problem.
>> >> >>
>> >> >> 6). However exposing all System RAM regions to the crashkernel is not
>> >> >> advisable and may cause the crashkernel or some crashkernel drivers to
>> >> >> fail.
>> >> >>
>> >> >> 6a). I am trying an approach now, where the ACPI reclaim regions are
>> >> >> added to '/proc/iomem' separately as ACPI reclaim regions by the
>> >> >> kernel code and on the other hand the user-space 'kexec-tools' will
>> >> >> pick up the ACPI reclaim regions from '/proc/iomem' and add it to the
>> >> >> dt node 'linux,usable-memory-range'
>> >> >
>> >> > I still don't understand why we need to carry over the information
>> >> > about "ACPI Reclaim memory" to crash dump kernel. In my understandings,
>> >> > such regions are free to be reused by the kernel after some point of
>> >> > initialization. Why does crash dump kernel need to know about them?
>> >> >
>> >>
>> >> Not really. According to the UEFI spec, they can be reclaimed after
>> >> the OS has initialized, i.e., when it has consumed the ACPI tables and
>> >> no longer needs them. Of course, in order to be able to boot a kexec
>> >> kernel, those regions needs to be preserved, which is why they are
>> >> memblock_reserve()'d now.
>> >
>> > For my better understandings, who is actually accessing such regions
>> > during boot time, uefi itself or efistub?
>> >
>>
>> No, only the kernel. This is where the ACPI tables are stored. For
>> instance, on QEMU we have
>>
>> ACPI: RSDP 0x0000000078980000 000024 (v02 BOCHS )
>> ACPI: XSDT 0x0000000078970000 000054 (v01 BOCHS BXPCFACP 00000001
>> 01000013)
>> ACPI: FACP 0x0000000078930000 00010C (v05 BOCHS BXPCFACP 00000001
>> BXPC 00000001)
>> ACPI: DSDT 0x0000000078940000 0011DA (v02 BOCHS BXPCDSDT 00000001
>> BXPC 00000001)
>> ACPI: APIC 0x0000000078920000 000140 (v03 BOCHS BXPCAPIC 00000001
>> BXPC 00000001)
>> ACPI: GTDT 0x0000000078910000 000060 (v02 BOCHS BXPCGTDT 00000001
>> BXPC 00000001)
>> ACPI: MCFG 0x0000000078900000 00003C (v01 BOCHS BXPCMCFG 00000001
>> BXPC 00000001)
>> ACPI: SPCR 0x00000000788F0000 000050 (v02 BOCHS BXPCSPCR 00000001
>> BXPC 00000001)
>> ACPI: IORT 0x00000000788E0000 00007C (v00 BOCHS BXPCIORT 00000001
>> BXPC 00000001)
>>
>> covered by
>>
>> efi: 0x0000788e0000-0x00007894ffff [ACPI Reclaim Memory ...]
>> ...
>> efi: 0x000078970000-0x00007898ffff [ACPI Reclaim Memory ...]
>
> OK. I mistakenly understood those regions could be freed after exiting
> UEFI boot services.
>
>>
>> >> So it seems that kexec does not honour the memblock_reserve() table
>> >> when booting the next kernel.
>> >
>> > not really.
>> >
>> >> > (In other words, can or should we skip some part of ACPI-related init code
>> >> > on crash dump kernel?)
>> >> >
>> >>
>> >> I don't think so. And the change to the handling of ACPI reclaim
>> >> regions only revealed the bug, not created it (given that other
>> >> memblock_reserve regions may be affected as well)
>> >
>> > As whether we should honor such reserved regions over kexec'ing
>> > depends on each one's specific nature, we will have to take care one-by-one.
>> > As a matter of fact, no information about "reserved" memblocks is
>> > exposed to user space (via proc/iomem).
>> >
>>
>> That is why I suggested (somewhere in this thread?) to not expose them
>> as 'System RAM'. Do you think that could solve this?
>
> Memblock-reserv'ing them is necessary to prevent their corruption and
> marking them under another name in /proc/iomem would also be good in order
> not to allocate them as part of crash kernel's memory.
>
I agree. However, this may not be entirely trivial, since iterating
over the memblock_reserved table and creating iomem entries may result
in collisions.
> But I'm not still convinced that we should export them in useable-
> memory-range to crash dump kernel. They will be accessed through
> acpi_os_map_memory() and so won't be required to be part of system ram
> (or memblocks), I guess.
Agreed. They will be covered by the linear mapping in the boot kernel,
and be mapped explicitly via ioremap_cache() in the kexec kernel,
which is exactly what we want in this case.
> Just FYI, on x86, ACPI tables seems to be exposed to crash dump kernel
> via a kernel command line parameter, "memmap=".
>
^ permalink raw reply
* [PATCH 1/6] ARM: shmobile: defconfig: Enable PWM
From: Fabrizio Castro @ 2017-12-15 9:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdWUaQMgT4J0N+hrByAD12K-oO0+iNyDeVczS3nxr5ia9A@mail.gmail.com>
Hi Geert,
> Subject: Re: [PATCH 1/6] ARM: shmobile: defconfig: Enable PWM
>
> Hi Fabrizio,
>
> On Thu, Dec 14, 2017 at 11:56 AM, Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > RZ/G1 and R-Car platforms have PWM timers. This patch enables PWM support
> > by default.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
>
> Thanks for your patch!
Thank you for your feedback.
>
> If I'm not mistaken, there are no current users of this PWM block in the
> current R-Car Gen2 and RZ/G1 DTS files?
PWM support will be needed very soon as we are going to use pwm3 on the iwg20d platform to control the RGB display backlight.
The work to add display support is in progress, I hope it will be ready by next week, would you rather I sent this patch with the series to add display support?
Thanks,
Fab
> If that's correct, I don't think it makes much sense to enable it already.
>
> Simon: what do you think?
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
[https://www2.renesas.eu/media/email/unicef_2017.jpg]
This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* [PATCH 1/6] ARM: shmobile: defconfig: Enable PWM
From: Geert Uytterhoeven @ 2017-12-15 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <TY1PR06MB0895D343C77D0DB2E9F14680C00B0@TY1PR06MB0895.apcprd06.prod.outlook.com>
Hi Fabrizio,
On Fri, Dec 15, 2017 at 10:48 AM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
>> Subject: Re: [PATCH 1/6] ARM: shmobile: defconfig: Enable PWM
>> On Thu, Dec 14, 2017 at 11:56 AM, Fabrizio Castro
>> <fabrizio.castro@bp.renesas.com> wrote:
>> > RZ/G1 and R-Car platforms have PWM timers. This patch enables PWM support
>> > by default.
>> >
>> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
>>
>> Thanks for your patch!
>
> Thank you for your feedback.
>
>>
>> If I'm not mistaken, there are no current users of this PWM block in the
>> current R-Car Gen2 and RZ/G1 DTS files?
>
> PWM support will be needed very soon as we are going to use pwm3 on the iwg20d platform to control the RGB display backlight.
Good to know, thanks!
> The work to add display support is in progress, I hope it will be ready by next week, would you rather I sent this patch with the series to add display support?
That's up to Simon. But I guess it doesn't matter much, as defconfig updates
are queued in a separate branch anyway.
You may want to send a similar patch for multi_v7_defconfig.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Christoffer Dall @ 2017-12-15 10:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e5ccbf1c-6078-4c43-8aa3-fc226ed358f7@gmail.com>
On Fri, Dec 15, 2017 at 10:27:02AM +0800, Jia He wrote:
[...]
> >diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >index 73d262c4712b..544ed15fbbb3 100644
> >--- a/virt/kvm/arm/arch_timer.c
> >+++ b/virt/kvm/arm/arch_timer.c
> >@@ -46,7 +46,7 @@ static const struct kvm_irq_level default_vtimer_irq = {
> > .level = 1,
> > };
> >-static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx);
> >+static bool kvm_timer_irq_can_fire(u32 cnt_ctl);
> > static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> > struct arch_timer_context *timer_ctx);
> > static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
> >@@ -94,6 +94,7 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> > {
> > struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
> > struct arch_timer_context *vtimer;
> >+ u32 cnt_ctl;
> > if (!vcpu) {
> > pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
> >@@ -101,8 +102,8 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> > }
> > vtimer = vcpu_vtimer(vcpu);
> >- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> >- if (kvm_timer_irq_can_fire(vtimer))
> >+ cnt_ctl = read_sysreg_el0(cntv_ctl);
> >+ if (kvm_timer_irq_can_fire(cnt_ctl))
> > kvm_timer_update_irq(vcpu, true, vtimer);
> IIUC, your patch makes kvm_arch_timer_handler never changesvtimer->cnt_ctl
Yes, that's the idea.
Meanwhile, I think I thought of a cleaner way to do this. Could you
test the following two patches on your platform as well?
>From 3a594a3aa222bd64a86f6c6afcb209c9be20d5c5 Mon Sep 17 00:00:00 2001
From: Christoffer Dall <christoffer.dall@linaro.org>
Date: Thu, 14 Dec 2017 19:54:50 +0100
Subject: [PATCH 1/2] KVM: arm/arm64: Properly handle arch-timer IRQs after
vtimer_save_state
The recent timer rework was assuming that once the timer was disabled,
we should no longer see any interrupts from the timer. This assumption
turns out to not be true, and instead we have to handle the case when
the timer ISR runs even after the timer has been disabled.
This requires a couple of changes:
First, we should never overwrite the cached guest state of the timer
control register when the ISR runs, because KVM may have disabled its
timers when doing vcpu_put(), even though the guest still had the timer
enabled.
Second, we shouldn't assume that the timer is actually firing just
because we see an ISR, but we should check the ISTATUS field of the
timer control register to understand if the hardware timer is really
firing or not.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index aa9adfafe12b..792bcf6277b6 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -92,16 +92,21 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
{
struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
struct arch_timer_context *vtimer;
+ u32 cnt_ctl;
- if (!vcpu) {
- pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
- return IRQ_NONE;
- }
- vtimer = vcpu_vtimer(vcpu);
+ /*
+ * We may see a timer interrupt after vcpu_put() has been called which
+ * sets the CPU's vcpu pointer to NULL, because even though the timer
+ * has been disabled in vtimer_save_state(), the singal may not have
+ * been retired from the interrupt controller yet.
+ */
+ if (!vcpu)
+ return IRQ_HANDLED;
+ vtimer = vcpu_vtimer(vcpu);
if (!vtimer->irq.level) {
- vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
- if (kvm_timer_irq_can_fire(vtimer))
+ cnt_ctl = read_sysreg_el0(cntv_ctl);
+ if (cnt_ctl & ARCH_TIMER_CTRL_IT_STAT)
kvm_timer_update_irq(vcpu, true, vtimer);
}
>From ed96302b47d209024814df116994f64dc8f07c96 Mon Sep 17 00:00:00 2001
From: Christoffer Dall <christoffer.dall@linaro.org>
Date: Fri, 15 Dec 2017 00:30:12 +0100
Subject: [PATCH 2/2] KVM: arm/arm64: Fix timer enable flow
When enabling the timer on the first run, we fail to ever restore the
state and mark it as loaded. That means, that in the initial entry to
the VCPU ioctl, unless we exit to userspace for some reason such as a
pending signal, if the guest programs a timer and blocks, we will wait
forever, because we never read back the hardware state (the loaded flag
is not set), and so we think the timer is disabled, and we never
schedule a background soft timer.
The end result? The VCPU blocks forever, and the only solution is to
kill the thread.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
virt/kvm/arm/arch_timer.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 792bcf6277b6..8869658e6983 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -843,10 +843,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
no_vgic:
preempt_disable();
timer->enabled = 1;
- if (!irqchip_in_kernel(vcpu->kvm))
- kvm_timer_vcpu_load_user(vcpu);
- else
- kvm_timer_vcpu_load_vgic(vcpu);
+ kvm_timer_vcpu_load(vcpu);
preempt_enable();
return 0;
Thanks,
-Christoffer
^ permalink raw reply related
* [PATCH] KVM: arm/arm64: don't set vtimer->cnt_ctl in kvm_arch_timer_handler
From: Christoffer Dall @ 2017-12-15 10:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <519f0e33-4419-68be-32b4-11bb5e19cf17@arm.com>
On Fri, Dec 15, 2017 at 09:09:05AM +0000, Marc Zyngier wrote:
> On 15/12/17 02:27, Jia He wrote:
> >
> >
>
> [...]
>
> >> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu)
> >>
> >> /* Disable the virtual timer */
> >> write_sysreg_el0(0, cntv_ctl);
> >> + isb();
> > My only concern is whether this isb() is required here?
> > Sorryif this is a stupid question.I understand little about arm arch
> > memory barrier. But seems isb will flush all the instruction prefetch.Do
> > you think if an timer interrupt irq arrives, arm will use the previous
> > instruction prefetch?
>
>
> This barrier has little to do with prefetch. It just guarantees that the
> code after the isb() is now running with a disabled virtual timer.
> Otherwise, a CPU can freely reorder the write_sysreg() until the next
> context synchronization event.
>
> An interrupt coming between the write and the barrier will also act as a
> context synchronization event. For more details, see the ARMv8 ARM (the
> glossary has a section on the concept).
>
So since an ISB doesn't guarantee that the timer will actually be
disabled, is it a waste of energy to have it, or should we keep it as a
best effort kind of thing?
Thanks,
-Christoffer
^ 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