* [PATCH 7/8] x86: tsc: Register the persistent clock
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>
Register the tsc as one persistent clock to compensate the suspend time
if the tsc clocksource is always available.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
arch/x86/Kconfig | 1 +
arch/x86/kernel/tsc.c | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 297789a..549dd01 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -200,6 +200,7 @@ config X86
select USER_STACKTRACE_SUPPORT
select VIRT_TO_BUS
select X86_FEATURE_NAMES if PROC_FS
+ select PERSISTENT_CLOCK
config INSTRUCTION_DECODER
def_bool y
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..cb4f495 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/clocksource.h>
#include <linux/percpu.h>
+#include <linux/persistent_clock.h>
#include <linux/timex.h>
#include <linux/static_key.h>
@@ -1032,6 +1033,11 @@ static u64 read_tsc(struct clocksource *cs)
return (u64)rdtsc_ordered();
}
+static u64 notrace tsc_read_persistent_clock(void)
+{
+ return (u64)rdtsc_ordered();
+}
+
static void tsc_cs_mark_unstable(struct clocksource *cs)
{
if (tsc_unstable)
@@ -1300,6 +1306,14 @@ static void tsc_refine_calibration_work(struct work_struct *work)
if (boot_cpu_has(X86_FEATURE_ART))
art_related_clocksource = &clocksource_tsc;
clocksource_register_khz(&clocksource_tsc, tsc_khz);
+
+ if (clocksource_tsc.flags & CLOCK_SOURCE_SUSPEND_NONSTOP) {
+ persistent_clock_init_and_register(tsc_read_persistent_clock,
+ CLOCKSOURCE_MASK(64),
+ tsc_khz * 1000, 0);
+ persistent_clock_start_alarmtimer();
+ }
+
unreg:
clocksource_unregister(&clocksource_tsc_early);
}
@@ -1327,6 +1341,13 @@ static int __init init_tsc_clocksource(void)
if (boot_cpu_has(X86_FEATURE_ART))
art_related_clocksource = &clocksource_tsc;
clocksource_register_khz(&clocksource_tsc, tsc_khz);
+
+ if (clocksource_tsc.flags & CLOCK_SOURCE_SUSPEND_NONSTOP) {
+ persistent_clock_init_and_register(tsc_read_persistent_clock,
+ CLOCKSOURCE_MASK(64),
+ tsc_khz * 1000, 0);
+ persistent_clock_start_alarmtimer();
+ }
unreg:
clocksource_unregister(&clocksource_tsc_early);
return 0;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 8/8] time: timekeeping: Remove time compensating from nonstop clocksources
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>
Since we have converted all nonstop clocksources to use persistent clock,
thus we can simplify the time compensating by removing the nonstop
clocksources. Now we can compensate the suspend time for the OS time from
the persistent clock or rtc device.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
kernel/time/timekeeping.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df9..3026d98 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1666,7 +1666,6 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta)
void timekeeping_resume(void)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *clock = tk->tkr_mono.clock;
unsigned long flags;
struct timespec64 ts_new, ts_delta;
u64 cycle_now;
@@ -1682,27 +1681,17 @@ void timekeeping_resume(void)
/*
* After system resumes, we need to calculate the suspended time and
- * compensate it for the OS time. There are 3 sources that could be
- * used: Nonstop clocksource during suspend, persistent clock and rtc
- * device.
+ * compensate it for the OS time. There are 2 sources that could be
+ * used: persistent clock and rtc device.
*
* One specific platform may have 1 or 2 or all of them, and the
* preference will be:
- * suspend-nonstop clocksource -> persistent clock -> rtc
+ * persistent clock -> rtc
* The less preferred source will only be tried if there is no better
* usable source. The rtc part is handled separately in rtc core code.
*/
cycle_now = tk_clock_read(&tk->tkr_mono);
- if ((clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP) &&
- cycle_now > tk->tkr_mono.cycle_last) {
- u64 nsec, cyc_delta;
-
- cyc_delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last,
- tk->tkr_mono.mask);
- nsec = mul_u64_u32_shr(cyc_delta, clock->mult, clock->shift);
- ts_delta = ns_to_timespec64(nsec);
- sleeptime_injected = true;
- } else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
+ if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
sleeptime_injected = true;
}
--
1.7.9.5
^ permalink raw reply related
* [GIT PULL] Renesas ARM Based SoC Fixes for v4.18
From: Simon Horman @ 2018-06-13 11:41 UTC (permalink / raw)
To: linux-arm-kernel
Hi Olof, Hi Kevin, Hi Arnd,
Please consider these Renesas ARM based SoC fixes for v4.18.
This pull request is based on renesas-soc-for-v4.18,
which you have already pulled.
The following changes since commit 086b399965a7ee7e50c3b3c57f2dba30ff0334b0:
soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B} (2018-05-16 10:57:44 +0200)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git tags/renesas-fixes-for-v4.18
for you to fetch changes up to 977d5ba4507dfe5b1346597ee57750262d8d2b19:
soc: renesas: rcar-sysc: Make PM domain initialization more robust (2018-06-08 10:04:25 +0200)
----------------------------------------------------------------
Renesas ARM Based SoC Fixes for v4.18
Make PM domain initialization more robust in Renesas R-Car SYSC driver.
This resolves a regression due to re-parenting of PM domains by
086b399965a7ee7e ("soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}").
----------------------------------------------------------------
Geert Uytterhoeven (1):
soc: renesas: rcar-sysc: Make PM domain initialization more robust
drivers/soc/renesas/rcar-sysc.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
^ permalink raw reply
* [PATCH 1/1] soc: renesas: rcar-sysc: Make PM domain initialization more robust
From: Simon Horman @ 2018-06-13 11:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528889592.git.horms+renesas@verge.net.au>
From: Geert Uytterhoeven <geert+renesas@glider.be>
The quirk for R-Car E3 ES1.0 added in commit 086b399965a7ee7e ("soc:
renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}") makes the 3DG-A
PM domain a subdomain of the 3DG-B PM domain. However, registering
3DG-A with its parent fails silently, as the 3DG-B PM domain hasn't been
registered yet, and such failures are never reported.
Fix this by:
1. Splitting PM Domain initialization in two steps, so all PM domains
are registered before any child-parent links are established,
2. Reporting any failures in establishing child-parent relations.
Check for and report pm_genpd_init() failures, too, as that function
gained a return value in commit 7eb231c337e00735 ("PM / Domains: Convert
pm_genpd_init() to return an error code").
Fixes: 086b399965a7ee7e ("soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
drivers/soc/renesas/rcar-sysc.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
index 95120acc4d80..50d03d8b4f9a 100644
--- a/drivers/soc/renesas/rcar-sysc.c
+++ b/drivers/soc/renesas/rcar-sysc.c
@@ -194,11 +194,12 @@ static int rcar_sysc_pd_power_on(struct generic_pm_domain *genpd)
static bool has_cpg_mstp;
-static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
+static int __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
{
struct generic_pm_domain *genpd = &pd->genpd;
const char *name = pd->genpd.name;
struct dev_power_governor *gov = &simple_qos_governor;
+ int error;
if (pd->flags & PD_CPU) {
/*
@@ -251,7 +252,11 @@ static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
rcar_sysc_power_up(&pd->ch);
finalize:
- pm_genpd_init(genpd, gov, false);
+ error = pm_genpd_init(genpd, gov, false);
+ if (error)
+ pr_err("Failed to init PM domain %s: %d\n", name, error);
+
+ return error;
}
static const struct of_device_id rcar_sysc_matches[] __initconst = {
@@ -375,6 +380,9 @@ static int __init rcar_sysc_pd_init(void)
pr_debug("%pOF: syscier = 0x%08x\n", np, syscier);
iowrite32(syscier, base + SYSCIER);
+ /*
+ * First, create all PM domains
+ */
for (i = 0; i < info->num_areas; i++) {
const struct rcar_sysc_area *area = &info->areas[i];
struct rcar_sysc_pd *pd;
@@ -397,14 +405,29 @@ static int __init rcar_sysc_pd_init(void)
pd->ch.isr_bit = area->isr_bit;
pd->flags = area->flags;
- rcar_sysc_pd_setup(pd);
- if (area->parent >= 0)
- pm_genpd_add_subdomain(domains->domains[area->parent],
- &pd->genpd);
+ error = rcar_sysc_pd_setup(pd);
+ if (error)
+ goto out_put;
domains->domains[area->isr_bit] = &pd->genpd;
}
+ /*
+ * Second, link all PM domains to their parents
+ */
+ for (i = 0; i < info->num_areas; i++) {
+ const struct rcar_sysc_area *area = &info->areas[i];
+
+ if (!area->name || area->parent < 0)
+ continue;
+
+ error = pm_genpd_add_subdomain(domains->domains[area->parent],
+ domains->domains[area->isr_bit]);
+ if (error)
+ pr_warn("Failed to add PM subdomain %s to parent %u\n",
+ area->name, area->parent);
+ }
+
error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
out_put:
--
2.11.0
^ permalink raw reply related
* [PATCH v1 2/4] dt-bindings: mailbox: provide imx-mailbox documentation
From: Oleksij Rempel @ 2018-06-13 11:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAA+hA=RNxUHro_92YdORw7FOHba=ZSv4q697npCNKwoMG-_HMw@mail.gmail.com>
On 13.06.2018 13:05, Dong Aisheng wrote:
> Hi Oleksij,
>
> Some more comments besides Rob's:
>
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>> .../bindings/mailbox/imx-mailbox.txt | 35 +++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> new file mode 100644
>> index 000000000000..a45604b33039
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> @@ -0,0 +1,35 @@
>> +i.MX Messaging Unit
>> +===================
>> +
>> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most cases
>> +they are accessible from all Processor Units. On one hand, at least for mailbox functionality,
>> +it makes no difference which application or processor is using which set of the MU. On
>> +other hand, the register sets for each of the MU parts are not identical.
>> +
>> +Required properties:
>> +- compatible : Shell be one of:
>> + "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
>
> What's current requirement to distinguish Side A and B?
> I see current code, side A only does extra clear of xCR register but none for B.
> Is this a generic approach or something customized?
A and B side have more then one BIT difference, and there is no way to
see the difference by software. Current driver do not make use of it,
but in devicetree we describe HW not SW. If HW is different, it should
have different compatibles.
> BTW can we name it more generically?e.g. fsl,imx7s-mu.
> And using a property to indicate whether it's side A or B if really required.
>
> Furthermore, AFAIK MX7 MU is derived from MX6SX.
> Should we add and use fsl,imx6sx-mu instead?
As soon as some one will test this driver on imx6sx and confirm it
working, i'll add fsl,imx6sx-mu-a/b as well.
> e.g.
> - compatible : Shell be one of:
> "fsl,imx6sx-mu" and "fsl,imx7s-mu".
>
> Regards
> Dong Aisheng
>
>> +- reg : physical base address of the mailbox and length of
>> + memory mapped region.
>> +- #mbox-cells: Common mailbox binding property to identify the number
>> + of cells required for the mailbox specifier. Should be 1.
>> +- interrupts : interrupt number. The interrupt specifier format
>> + depends on the interrupt controller parent.
>> +- clocks : phandle to the input clock.
>> +
>> +Example:
>> + mu0a: mu at 30aa0000 {
>> + compatible = "fsl,imx7s-mu-a";
>> + reg = <0x30aa0000 0x28>;
>> + interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> + #mbox-cells = <1>;
>> + };
>> +
>> + mu0b: mu at 30ab0000 {
>> + compatible = "fsl,imx7s-mu-b";
>> + reg = <0x30ab0000 0x28>;
>> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> + #mbox-cells = <1>;
>> + };
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/a91c9c57/attachment-0001.sig>
^ permalink raw reply
* [PATCH v1 2/4] dt-bindings: mailbox: provide imx-mailbox documentation
From: Oleksij Rempel @ 2018-06-13 11:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180612194142.GA16521@rob-hp-laptop>
On 12.06.2018 21:41, Rob Herring wrote:
> On Fri, Jun 01, 2018 at 08:58:19AM +0200, Oleksij Rempel wrote:
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>> .../bindings/mailbox/imx-mailbox.txt | 35 +++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> new file mode 100644
>> index 000000000000..a45604b33039
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> @@ -0,0 +1,35 @@
>> +i.MX Messaging Unit
>> +===================
>> +
>> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most cases
>> +they are accessible from all Processor Units. On one hand, at least for mailbox functionality,
>> +it makes no difference which application or processor is using which set of the MU. On
>
> Please wrap lines correctly (<80).
>
>> +other hand, the register sets for each of the MU parts are not identical.
>> +
>> +Required properties:
>> +- compatible : Shell be one of:
>> + "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
>> +- reg : physical base address of the mailbox and length of
>
> Mix of space and tab.
>
>> + memory mapped region.
>> +- #mbox-cells: Common mailbox binding property to identify the number
>> + of cells required for the mailbox specifier. Should be 1.
>> +- interrupts : interrupt number. The interrupt specifier format
>> + depends on the interrupt controller parent.
>
> Just need to say how many interrupts and what they are if more than 1.
>
>> +- clocks : phandle to the input clock.
>> +
>> +Example:
>> + mu0a: mu at 30aa0000 {
>
> mailbox at ...
>
>> + compatible = "fsl,imx7s-mu-a";
>> + reg = <0x30aa0000 0x28>;
>> + interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> + #mbox-cells = <1>;
>> + };
>> +
>> + mu0b: mu at 30ab0000 {
>
> mailbox at ...
>
>> + compatible = "fsl,imx7s-mu-b";
>> + reg = <0x30ab0000 0x28>;
>> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> + #mbox-cells = <1>;
>> + };
thx. will fix it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/3d9a7f9d/attachment.sig>
^ permalink raw reply
* [PATCH v7 5/6] regulator: pfuze100-regulator: provide pm_power_off_prepare handler
From: Oleksij Rempel @ 2018-06-13 12:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517055014.6607-6-o.rempel@pengutronix.de>
Hi Mark,
Rob ACKed DT bindings, Rafael ACKed export of pm_power_off_prepare. I
need your OK for regulator patch.
On 17.05.2018 07:50, Oleksij Rempel wrote:
> On some boards the SoC can use one pin "PMIC_STBY_REQ" to notify th PMIC
> about state changes. In this case internal state of PMIC must be
> preconfigured for upcomming state change.
> It works fine with the current regulator framework, except with the
> power-off case.
>
> This patch is providing an optional pm_power_off_prepare handler
> which will configure standby state of the PMIC to disable all power lines.
>
> In my power consumption test on RIoTBoard, I got the following results:
> power off without this patch: 320 mA
> power off with this patch: 2 mA
> suspend to ram: 40 mA
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> drivers/regulator/pfuze100-regulator.c | 92 ++++++++++++++++++++++++++
> 1 file changed, 92 insertions(+)
>
> diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
> index 63922a2167e5..f6c276ed91d8 100644
> --- a/drivers/regulator/pfuze100-regulator.c
> +++ b/drivers/regulator/pfuze100-regulator.c
> @@ -28,6 +28,7 @@
> #include <linux/regulator/pfuze100.h>
> #include <linux/i2c.h>
> #include <linux/slab.h>
> +#include <linux/kallsyms.h>
> #include <linux/regmap.h>
>
> #define PFUZE_NUMREGS 128
> @@ -42,11 +43,17 @@
>
> #define PFUZE100_COINVOL 0x1a
> #define PFUZE100_SW1ABVOL 0x20
> +#define PFUZE100_SW1ABMODE 0x23
> #define PFUZE100_SW1CVOL 0x2e
> +#define PFUZE100_SW1CMODE 0x31
> #define PFUZE100_SW2VOL 0x35
> +#define PFUZE100_SW2MODE 0x38
> #define PFUZE100_SW3AVOL 0x3c
> +#define PFUZE100_SW3AMODE 0x3f
> #define PFUZE100_SW3BVOL 0x43
> +#define PFUZE100_SW3BMODE 0x46
> #define PFUZE100_SW4VOL 0x4a
> +#define PFUZE100_SW4MODE 0x4d
> #define PFUZE100_SWBSTCON1 0x66
> #define PFUZE100_VREFDDRCON 0x6a
> #define PFUZE100_VSNVSVOL 0x6b
> @@ -57,6 +64,13 @@
> #define PFUZE100_VGEN5VOL 0x70
> #define PFUZE100_VGEN6VOL 0x71
>
> +#define PFUZE100_SWxMODE_MASK 0xf
> +#define PFUZE100_SWxMODE_APS_APS 0x8
> +#define PFUZE100_SWxMODE_APS_OFF 0x4
> +
> +#define PFUZE100_VGENxLPWR BIT(6)
> +#define PFUZE100_VGENxSTBY BIT(5)
> +
> enum chips { PFUZE100, PFUZE200, PFUZE3000 = 3 };
>
> struct pfuze_regulator {
> @@ -489,6 +503,69 @@ static inline struct device_node *match_of_node(int index)
> }
> #endif
>
> +static struct pfuze_chip *syspm_pfuze_chip;
> +
> +static void pfuze_power_off_prepare(void)
> +{
> + dev_info(syspm_pfuze_chip->dev, "Configure standy mode for power off");
> +
> + /* Switch from default mode: APS/APS to APS/Off */
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1ABMODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1CMODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW2MODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3AMODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3BMODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW4MODE,
> + PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN1VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN2VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN3VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN4VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN5VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> + regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN6VOL,
> + PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> + PFUZE100_VGENxSTBY);
> +}
> +
> +static int pfuze_power_off_prepare_init(struct pfuze_chip *pfuze_chip)
> +{
> + if (pfuze_chip->chip_id != PFUZE100) {
> + dev_warn(pfuze_chip->dev, "Requested pm_power_off_prepare handler for not supported chip\n");
> + return -ENODEV;
> + }
> +
> + if (pm_power_off_prepare) {
> + dev_warn(pfuze_chip->dev, "pm_power_off_prepare is already registered.\n");
> + return -EBUSY;
> + }
> +
> + if (syspm_pfuze_chip) {
> + dev_warn(pfuze_chip->dev, "syspm_pfuze_chip is already set.\n");
> + return -EBUSY;
> + }
> +
> + syspm_pfuze_chip = pfuze_chip;
> + pm_power_off_prepare = pfuze_power_off_prepare;
> +
> + return 0;
> +}
> +
> static int pfuze_identify(struct pfuze_chip *pfuze_chip)
> {
> unsigned int value;
> @@ -659,6 +736,20 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
> }
> }
>
> + if (of_property_read_bool(client->dev.of_node,
> + "fsl,pmic-stby-poweroff"))
> + return pfuze_power_off_prepare_init(pfuze_chip);
> +
> + return 0;
> +}
> +
> +static int pfuze100_regulator_remove(struct i2c_client *client)
> +{
> + if (syspm_pfuze_chip) {
> + syspm_pfuze_chip = NULL;
> + pm_power_off_prepare = NULL;
> + }
> +
> return 0;
> }
>
> @@ -669,6 +760,7 @@ static struct i2c_driver pfuze_driver = {
> .of_match_table = pfuze_dt_ids,
> },
> .probe = pfuze100_regulator_probe,
> + .remove = pfuze100_regulator_remove,
> };
> module_i2c_driver(pfuze_driver);
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/358050e0/attachment.sig>
^ permalink raw reply
* [PATCH] clk: meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL
From: Neil Armstrong @ 2018-06-13 12:20 UTC (permalink / raw)
To: linux-arm-kernel
On Amlogic Meson GXBB & GXL platforms, the SCPI Cortex-M4 Co-Processor
seems to be dependent on the FCLK_DIV2 to be operationnal.
The issue occured since v4.17-rc1 by freezing the kernel boot when
the 'schedutil' cpufreq governor was selected as default :
[ 12.071837] scpi_protocol scpi: SCP Protocol 0.0 Firmware 0.0.0 version
domain-0 init dvfs: 4
[ 12.087757] hctosys: unable to open rtc device (rtc0)
[ 12.087907] cfg80211: Loading compiled-in X.509 certificates for regulatory database
[ 12.102241] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
But when disabling the MMC driver, the boot finished but cpufreq failed to
change the CPU frequency :
[ 12.153045] cpufreq: __target_index: Failed to change cpu frequency: -5
A bisect between v4.16 and v4.16-rc1 gave the 05f814402d61 commit to be
the first bad commit.
This commit added support for the missing clock gates before the fixed PLL
fixed dividers (FCLK_DIVx) and the clock framework basically disabled
all the unused fixed dividers, thus disabled a critical clock path for
the SCPI Co-Processor.
This patch simply sets the FCLK_DIV2 gate as critical to ensure
nobody can disable it.
Fixes: 05f814402d61 ("clk: meson: add fdiv clock gates")
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/clk/meson/gxbb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index b1e4d95..0e053c1 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -511,6 +511,7 @@ static struct clk_regmap gxbb_fclk_div2 = {
.ops = &clk_regmap_gate_ops,
.parent_names = (const char *[]){ "fclk_div2_div" },
.num_parents = 1,
+ .flags = CLK_IS_CRITICAL,
},
};
--
2.7.4
^ permalink raw reply related
* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Dong Aisheng @ 2018-06-13 12:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180601065821.28234-5-o.rempel@pengutronix.de>
Hi Oleksij,
On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
Could we really be able to send up to 4 42bit words with this driver?
It looks to me the current Mailbox framework is more designed for share mem
transfer which does not fit i.MX MU well.
>
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
Would you please provide a guide on how to test it quickly?
I may want to give a test.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> drivers/mailbox/Kconfig | 6 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/imx-mailbox.c | 289 ++++++++++++++++++++++++++++++++++
> 3 files changed, 297 insertions(+)
> create mode 100644 drivers/mailbox/imx-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
> The controller has 3 mailbox channels, the last of which can be
> used in Secure mode only.
>
> +config IMX_MBOX
> + tristate "iMX Mailbox"
> + depends on SOC_IMX7D || COMPILE_TEST
> + help
> + Mailbox implementation for iMX7D Messaging Unit (MU).
> +
> config PLATFORM_MHU
> tristate "Platform MHU Mailbox"
> depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
>
> obj-$(CONFIG_ARM_MHU) += arm_mhu.o
>
> +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> +
> obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o
>
> obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644
> index 000000000000..2bc9f11393b1
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x) (0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x) (0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR 0x20
> +#define IMX_MU_xSR_TEn(x) BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x) BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR 0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x) BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x) BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS 4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> + unsigned int chans;
> + void (*init_hw)(struct imx_mu_priv *priv);
> +};
> +
> +struct imx_mu_con_priv {
> + int irq;
> + unsigned int bidx;
> + unsigned int idx;
> +};
> +
> +struct imx_mu_priv {
> + struct device *dev;
> + const struct imx_mu_cfg *dcfg;
> + void __iomem *base;
> +
> + struct mbox_controller mbox;
> + struct mbox_chan mbox_chans[IMX_MU_MAX_CHANS];
> +
> + struct imx_mu_con_priv con_priv[IMX_MU_MAX_CHANS];
> + struct clk *clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> + return container_of(mbox, struct imx_mu_priv, mbox);
> +}
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
> +{
> + iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
> +{
> + return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
> +{
> + u32 val;
> +
> + val = imx_mu_read(priv, offs);
> + val &= ~clr;
> + val |= set;
> + imx_mu_write(priv, val, offs);
> +
> + return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p)
> +{
> + struct mbox_chan *chan = p;
> + struct imx_mu_con_priv *cp = chan->con_priv;
> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
Please do in reversed order from long to short
> +
> + u32 val, dat;
> +
> + val = imx_mu_read(priv, IMX_MU_xSR);
> + val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> + if (!val)
> + return IRQ_NONE;
> +
> + if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
> + mbox_chan_txdone(chan, 0);
> + }
> +
> + if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> + mbox_chan_received_data(chan, (void *)&dat);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
> +{
> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> + struct imx_mu_con_priv *cp = chan->con_priv;
> + u32 val;
> +
> + val = imx_mu_read(priv, IMX_MU_xSR);
> + /* test if transmit register is empty */
> + return (!(val & IMX_MU_xSR_TEn(cp->bidx)));
> +}
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> + struct imx_mu_con_priv *cp = chan->con_priv;
> + u32 *arg = data;
> +
> + if (imx_mu_last_tx_done(chan))
return true for tx_done?
Or maybe better imx_mu_is_busy?
> + return -EBUSY;
> +
> + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> + return 0;
> +}
> +
> +static int imx_mu_startup(struct mbox_chan *chan)
> +{
> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> + struct imx_mu_con_priv *cp = chan->con_priv;
> + int ret;
> +
> + ret = request_irq(cp->irq, imx_mu_isr,
> + IRQF_SHARED, "imx_mu_chan", chan);
This looks me to a bit strange as all virtual channels interrupts
line actually are the same. And we request that same irq line
repeatedly here with the same irq handler.
> + if (ret) {
> + dev_err(chan->mbox->dev,
> + "Unable to acquire IRQ %d\n", cp->irq);
> + return ret;
> + }
> +
> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> + return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan)
> +{
> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> + struct imx_mu_con_priv *cp = chan->con_priv;
> +
> + imx_mu_rmw(priv, IMX_MU_xCR, 0,
> + IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
> +
> + free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> + .send_data = imx_mu_send_data,
> + .startup = imx_mu_startup,
> + .shutdown = imx_mu_shutdown,
> + .last_tx_done = imx_mu_last_tx_done,
Do we really need this?
Looking at the code, it seems .last_tx_done() is only called for polling mode.
But what you set below is:
priv->mbox.txdone_irq = true;
Or am i missed something?
> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *iomem;
> + struct imx_mu_priv *priv;
> + const struct imx_mu_cfg *dcfg;
> + unsigned int i, chans;
> + int irq, ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dcfg = of_device_get_match_data(dev);
> + if (!dcfg)
> + return -EINVAL;
> +
> + priv->dcfg = dcfg;
> + priv->dev = dev;
> +
> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0)
> + return irq < 0 ? irq : -EINVAL;
Is it possible == 0?
> +
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + if (PTR_ERR(priv->clk) == -ENOENT) {
> + priv->clk = NULL;
> + } else {
> + dev_err(dev, "Failed to get clock\n");
The line looks not be quite meaningful as it may be defer probe.
> + return PTR_ERR(priv->clk);
> + }
> + }
> +
> + ret = clk_prepare_enable(priv->clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> + /* Initialize channel identifiers */
> + for (i = 0; i < chans; i++) {
> + struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> + cp->bidx = 3 - i;
> + cp->idx = i;
> + cp->irq = irq;
> + priv->mbox_chans[i].con_priv = cp;
> + }
> +
> + priv->mbox.dev = dev;
> + priv->mbox.ops = &imx_mu_ops;
> + priv->mbox.chans = priv->mbox_chans;
> + priv->mbox.num_chans = chans;
> + priv->mbox.txdone_irq = true;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + if (priv->dcfg->init_hw)
> + priv->dcfg->init_hw(priv);
> +
> + return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev)
> +{
> + struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> + mbox_controller_unregister(&priv->mbox);
> + clk_disable_unprepare(priv->clk);
> +
> + return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
> +{
> + /* Set default config */
> + imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> + .chans = IMX_MU_MAX_CHANS,
> + .init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> + .chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> + { .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> + { .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> + .probe = imx_mu_probe,
> + .remove = imx_mu_remove,
> + .driver = {
> + .name = "imx_mu",
> + .of_match_table = imx_mu_dt_ids,
> + },
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX7");
s/i.MX7/i.MX
Regards
Dong Aisheng
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Matthew Wilcox @ 2018-06-13 12:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com>
On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.
I think the documentation for this function needs improving. For example,
GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
At the very least, the kernel-doc needs:
* Context: Process context (may sleep even if GFP flags indicate otherwise).
Unless someone wants to rework this allocator to use spinlocks instead
of mutexes ...
^ permalink raw reply
* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Dong Aisheng @ 2018-06-13 12:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAA+hA=RpoRXUK+jB18F4OeYE9PG73SDa_NLYeN5Q2G3B1bC-JA@mail.gmail.com>
Copy linux-imx at nxp.com and more related guys to comment
On Wed, Jun 13, 2018 at 8:21 PM, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Oleksij,
>
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> The Mailbox controller is able to send messages (up to 4 32 bit words)
>> between the endpoints.
>
> Could we really be able to send up to 4 42bit words with this driver?
>
> It looks to me the current Mailbox framework is more designed for share mem
> transfer which does not fit i.MX MU well.
>
>>
>> This driver was tested using the mailbox-test driver sending messages
>> between the Cortex-A7 and the Cortex-M4.
>
> Would you please provide a guide on how to test it quickly?
> I may want to give a test.
>
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>> drivers/mailbox/Kconfig | 6 +
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/imx-mailbox.c | 289 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 297 insertions(+)
>> create mode 100644 drivers/mailbox/imx-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index a2bb27446dce..e1d2738a2e4c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -15,6 +15,12 @@ config ARM_MHU
>> The controller has 3 mailbox channels, the last of which can be
>> used in Secure mode only.
>>
>> +config IMX_MBOX
>> + tristate "iMX Mailbox"
>> + depends on SOC_IMX7D || COMPILE_TEST
>> + help
>> + Mailbox implementation for iMX7D Messaging Unit (MU).
>> +
>> config PLATFORM_MHU
>> tristate "Platform MHU Mailbox"
>> depends on OF
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index cc23c3a43fcd..ba2fe1b6dd62 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
>>
>> obj-$(CONFIG_ARM_MHU) += arm_mhu.o
>>
>> +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>> +
>> obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o
>>
>> obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>> new file mode 100644
>> index 000000000000..2bc9f11393b1
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,289 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* Transmit Register */
>> +#define IMX_MU_xTRn(x) (0x00 + 4 * (x))
>> +/* Receive Register */
>> +#define IMX_MU_xRRn(x) (0x10 + 4 * (x))
>> +/* Status Register */
>> +#define IMX_MU_xSR 0x20
>> +#define IMX_MU_xSR_TEn(x) BIT(20 + (x))
>> +#define IMX_MU_xSR_RFn(x) BIT(24 + (x))
>> +#define IMX_MU_xSR_BRDIP BIT(9)
>> +
>> +/* Control Register */
>> +#define IMX_MU_xCR 0x24
>> +/* Transmit Interrupt Enable */
>> +#define IMX_MU_xCR_TIEn(x) BIT(20 + (x))
>> +/* Receive Interrupt Enable */
>> +#define IMX_MU_xCR_RIEn(x) BIT(24 + (x))
>> +
>> +#define IMX_MU_MAX_CHANS 4u
>> +
>> +struct imx_mu_priv;
>> +
>> +struct imx_mu_cfg {
>> + unsigned int chans;
>> + void (*init_hw)(struct imx_mu_priv *priv);
>> +};
>> +
>> +struct imx_mu_con_priv {
>> + int irq;
>> + unsigned int bidx;
>> + unsigned int idx;
>> +};
>> +
>> +struct imx_mu_priv {
>> + struct device *dev;
>> + const struct imx_mu_cfg *dcfg;
>> + void __iomem *base;
>> +
>> + struct mbox_controller mbox;
>> + struct mbox_chan mbox_chans[IMX_MU_MAX_CHANS];
>> +
>> + struct imx_mu_con_priv con_priv[IMX_MU_MAX_CHANS];
>> + struct clk *clk;
>> +};
>> +
>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>> +{
>> + return container_of(mbox, struct imx_mu_priv, mbox);
>> +}
>> +
>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
>> +{
>> + iowrite32(val, priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
>> +{
>> + return ioread32(priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
>> +{
>> + u32 val;
>> +
>> + val = imx_mu_read(priv, offs);
>> + val &= ~clr;
>> + val |= set;
>> + imx_mu_write(priv, val, offs);
>> +
>> + return val;
>> +}
>> +
>> +static irqreturn_t imx_mu_isr(int irq, void *p)
>> +{
>> + struct mbox_chan *chan = p;
>> + struct imx_mu_con_priv *cp = chan->con_priv;
>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>
> Please do in reversed order from long to short
>
>> +
>> + u32 val, dat;
>> +
>> + val = imx_mu_read(priv, IMX_MU_xSR);
>> + val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>> + if (!val)
>> + return IRQ_NONE;
>> +
>> + if (val & IMX_MU_xSR_TEn(cp->bidx)) {
>> + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
>> + mbox_chan_txdone(chan, 0);
>> + }
>> +
>> + if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>> + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>> + mbox_chan_received_data(chan, (void *)&dat);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
>> +{
>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> + struct imx_mu_con_priv *cp = chan->con_priv;
>> + u32 val;
>> +
>> + val = imx_mu_read(priv, IMX_MU_xSR);
>> + /* test if transmit register is empty */
>> + return (!(val & IMX_MU_xSR_TEn(cp->bidx)));
>> +}
>> +
>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data)
>> +{
>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> + struct imx_mu_con_priv *cp = chan->con_priv;
>> + u32 *arg = data;
>> +
>> + if (imx_mu_last_tx_done(chan))
>
> return true for tx_done?
> Or maybe better imx_mu_is_busy?
>
>> + return -EBUSY;
>> +
>> + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int imx_mu_startup(struct mbox_chan *chan)
>> +{
>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> + struct imx_mu_con_priv *cp = chan->con_priv;
>> + int ret;
>> +
>> + ret = request_irq(cp->irq, imx_mu_isr,
>> + IRQF_SHARED, "imx_mu_chan", chan);
>
> This looks me to a bit strange as all virtual channels interrupts
> line actually are the same. And we request that same irq line
> repeatedly here with the same irq handler.
>
>> + if (ret) {
>> + dev_err(chan->mbox->dev,
>> + "Unable to acquire IRQ %d\n", cp->irq);
>> + return ret;
>> + }
>> +
>> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void imx_mu_shutdown(struct mbox_chan *chan)
>> +{
>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> + struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> + imx_mu_rmw(priv, IMX_MU_xCR, 0,
>> + IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
>> +
>> + free_irq(cp->irq, chan);
>> +}
>> +
>> +static const struct mbox_chan_ops imx_mu_ops = {
>> + .send_data = imx_mu_send_data,
>> + .startup = imx_mu_startup,
>> + .shutdown = imx_mu_shutdown,
>> + .last_tx_done = imx_mu_last_tx_done,
>
> Do we really need this?
> Looking at the code, it seems .last_tx_done() is only called for polling mode.
> But what you set below is:
> priv->mbox.txdone_irq = true;
>
> Or am i missed something?
>
>> +};
>> +
>> +static int imx_mu_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *iomem;
>> + struct imx_mu_priv *priv;
>> + const struct imx_mu_cfg *dcfg;
>> + unsigned int i, chans;
>> + int irq, ret;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + dcfg = of_device_get_match_data(dev);
>> + if (!dcfg)
>> + return -EINVAL;
>> +
>> + priv->dcfg = dcfg;
>> + priv->dev = dev;
>> +
>> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>> + if (IS_ERR(priv->base))
>> + return PTR_ERR(priv->base);
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq <= 0)
>> + return irq < 0 ? irq : -EINVAL;
>
> Is it possible == 0?
>
>> +
>> + priv->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(priv->clk)) {
>> + if (PTR_ERR(priv->clk) == -ENOENT) {
>> + priv->clk = NULL;
>> + } else {
>> + dev_err(dev, "Failed to get clock\n");
>
> The line looks not be quite meaningful as it may be defer probe.
>
>> + return PTR_ERR(priv->clk);
>> + }
>> + }
>> +
>> + ret = clk_prepare_enable(priv->clk);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>> + /* Initialize channel identifiers */
>> + for (i = 0; i < chans; i++) {
>> + struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> + cp->bidx = 3 - i;
>> + cp->idx = i;
>> + cp->irq = irq;
>> + priv->mbox_chans[i].con_priv = cp;
>> + }
>> +
>> + priv->mbox.dev = dev;
>> + priv->mbox.ops = &imx_mu_ops;
>> + priv->mbox.chans = priv->mbox_chans;
>> + priv->mbox.num_chans = chans;
>> + priv->mbox.txdone_irq = true;
>> +
>> + platform_set_drvdata(pdev, priv);
>> +
>> + if (priv->dcfg->init_hw)
>> + priv->dcfg->init_hw(priv);
>> +
>> + return mbox_controller_register(&priv->mbox);
>> +}
>> +
>> +static int imx_mu_remove(struct platform_device *pdev)
>> +{
>> + struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>> +
>> + mbox_controller_unregister(&priv->mbox);
>> + clk_disable_unprepare(priv->clk);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
>> +{
>> + /* Set default config */
>> + imx_mu_write(priv, 0, IMX_MU_xCR);
>> +}
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>> + .chans = IMX_MU_MAX_CHANS,
>> + .init_hw = imx_mu_init_imx7d_a,
>> +};
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>> + .chans = IMX_MU_MAX_CHANS,
>> +};
>> +
>> +static const struct of_device_id imx_mu_dt_ids[] = {
>> + { .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>> + { .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>> +
>> +static struct platform_driver imx_mu_driver = {
>> + .probe = imx_mu_probe,
>> + .remove = imx_mu_remove,
>> + .driver = {
>> + .name = "imx_mu",
>> + .of_match_table = imx_mu_dt_ids,
>> + },
>> +};
>> +module_platform_driver(imx_mu_driver);
>> +
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>> +MODULE_DESCRIPTION("Message Unit driver for i.MX7");
>
> s/i.MX7/i.MX
>
> Regards
> Dong Aisheng
>
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] clk: meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL
From: Jerome Brunet @ 2018-06-13 12:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528892421-12180-1-git-send-email-narmstrong@baylibre.com>
On Wed, 2018-06-13 at 14:20 +0200, Neil Armstrong wrote:
> On Amlogic Meson GXBB & GXL platforms, the SCPI Cortex-M4 Co-Processor
> seems to be dependent on the FCLK_DIV2 to be operationnal.
>
> The issue occured since v4.17-rc1 by freezing the kernel boot when
> the 'schedutil' cpufreq governor was selected as default :
>
> [ 12.071837] scpi_protocol scpi: SCP Protocol 0.0 Firmware 0.0.0 version
> domain-0 init dvfs: 4
> [ 12.087757] hctosys: unable to open rtc device (rtc0)
> [ 12.087907] cfg80211: Loading compiled-in X.509 certificates for regulatory database
> [ 12.102241] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
>
> But when disabling the MMC driver, the boot finished but cpufreq failed to
> change the CPU frequency :
>
> [ 12.153045] cpufreq: __target_index: Failed to change cpu frequency: -5
>
> A bisect between v4.16 and v4.16-rc1 gave the 05f814402d61 commit to be
> the first bad commit.
> This commit added support for the missing clock gates before the fixed PLL
> fixed dividers (FCLK_DIVx) and the clock framework basically disabled
> all the unused fixed dividers, thus disabled a critical clock path for
> the SCPI Co-Processor.
>
> This patch simply sets the FCLK_DIV2 gate as critical to ensure
> nobody can disable it.
>
> Fixes: 05f814402d61 ("clk: meson: add fdiv clock gates")
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Good catch !
We'll probably have to check the axg family as well
> ---
> drivers/clk/meson/gxbb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index b1e4d95..0e053c1 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -511,6 +511,7 @@ static struct clk_regmap gxbb_fclk_div2 = {
> .ops = &clk_regmap_gate_ops,
> .parent_names = (const char *[]){ "fclk_div2_div" },
> .num_parents = 1,
> + .flags = CLK_IS_CRITICAL,
> },
> };
>
^ permalink raw reply
* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Marek Szyprowski @ 2018-06-13 12:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613122359.GA8695@bombadil.infradead.org>
Hi Matthew,
On 2018-06-13 14:24, Matthew Wilcox wrote:
> On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
>> cma_alloc() function has gfp mask parameter, so users expect that it
>> honors typical memory allocation related flags. The most imporant from
>> the security point of view is handling of __GFP_ZERO flag, because memory
>> allocated by this function usually can be directly remapped to userspace
>> by device drivers as a part of multimedia processing and ignoring this
>> flag might lead to leaking some kernel structures to userspace.
>> Some callers of this function (for example arm64 dma-iommu glue code)
>> already assumed that the allocated buffers are cleared when this flag
>> is set. To avoid such issues, add simple code for clearing newly
>> allocated buffer when __GFP_ZERO flag is set. Callers will be then
>> updated to skip implicit clearing or adjust passed gfp flags.
> I think the documentation for this function needs improving. For example,
> GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
> At the very least, the kernel-doc needs:
>
> * Context: Process context (may sleep even if GFP flags indicate otherwise).
>
> Unless someone wants to rework this allocator to use spinlocks instead
> of mutexes ...
It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
by the
memory compaction code, which is used in alloc_contig_range(). Right, this
should be also noted in the documentation.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Sascha Hauer @ 2018-06-13 12:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAA+hA=RpoRXUK+jB18F4OeYE9PG73SDa_NLYeN5Q2G3B1bC-JA@mail.gmail.com>
On Wed, Jun 13, 2018 at 08:21:10PM +0800, Dong Aisheng wrote:
> Hi Oleksij,
>
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
>
> Could we really be able to send up to 4 42bit words with this driver?
>
> It looks to me the current Mailbox framework is more designed for share mem
> transfer which does not fit i.MX MU well.
The mailbox framework just defines channels and messages. A message is a
void * which may contain arbitrary data or even no data at all; some
drivers simply ignore the message pointer, so in fact they act as a
doorbell unit only.
There's nothing about shared memory in the mailbox framework, but of
course you can combine a mailbox driver and shared memory to a remote
message mechanism. That could be done with the i.MX MU aswell and would
indeed be a good match for the hardware.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
From: Matt Sealey @ 2018-06-13 12:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a1c0749b-9c97-b45f-716f-a62ae07af278@arm.com>
Suzuki,
Why not use ?unit??
I believe we had this discussion years ago about numbering serial ports and sdhci (i.e. how do you know it?s UART0 or UART1 from just the address? Some SoC?s don?t address sequentially *or* in a forward direction) - I believe it?s not exactly codified in ePAPR, not am I sure where it may be otherwise, but it exists.
I agree with Rob on the slave-mode nonsense, this is an SPI controller concept weirdly stuffed into a directed graph which implicitly tells you the data direction - it?s a rooted tree (just like DT!).
For the case of a funnel each device supplying trace should end up into an input node - numbered with a unit - and all those nodes should point to the output node as endpoints. Describing the hardware as a black box is probably less of a good idea than showing that it?s a funnel, or replicator by showing the internal paths. You wouldn?t need to ?number? ports with a unit except where the HW needs to differentiate between them, and you don?t need reg or a node address to do it.
If you really need to parse full graphs in both directions (find root, find leaf) then could we simply introduce properties which list the phandles of all uplink sources, as linked lists point to the list head?
This gives a way to validate that the graph starts and ends the way we expect, and also allows every port to be associated with being a required path between any two devices without parsing the *whole* graph (although you still need to do that to find the route to sinks).
Ta,
Matt
Sent from my iPhone
> On Jun 13, 2018, at 04:45, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>
> Hi Rob,
>
>> On 12/06/18 21:48, Rob Herring wrote:
>>> On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
>>> The coresight drivers relied on default bindings for graph
>>> in DT, while reusing the "reg" field of the "ports" to indicate
>>> the actual hardware port number for the connections. However,
>>> with the rules getting stricter w.r.t to the address mismatch
>>> with the label, it is no longer possible to use the port address
>>> field for the hardware port number. Hence, we add an explicit
>>> property to denote the hardware port number, "coresight,hwid"
>>> which must be specified for each "endpoint".
>>>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>> .../devicetree/bindings/arm/coresight.txt | 26 +++++++++---
>>> drivers/hwtracing/coresight/of_coresight.c | 46 ++++++++++++++++------
>>> 2 files changed, 54 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>>> index bd36e40..385581a 100644
>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>>> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
>>> "slave-mode"
>>> * Hardware Port number at the component:
>>> - - The hardware port number is assumed to be the address of the "port" component.
>>> + - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
>>> + - Each "endpoint" must define the hardware port of the local end of the
>>> + connection using the following property:
>>> + "coresight,hwid" - 32bit integer, hardware port number at the local end.
>> "coresight" is not a vendor and properties are in the form
>> [<vendor>,]<prop-name>.
>
> OK. The issue here is that a coresight component could be an Arm IP or
> a custom partner IP. So, the vendor could be either arm or the partner id.
> However, this property is kind of a generic one for the Coresight family,
> which is why we opted for "coresight". What is the guideline for such
> cases ?
>
> Or in other words I see the following possible options :
>
> 1) coresight,hwid - coresight generic
> 2) arm,coresight-hwid - arm vendor, however the device could be from any vendor.
> 3) hwid - Generic
> 4) none of the above, something completely different.
>
> What do you recommend from the above ?
>
>>> +
>>> Example:
>>> @@ -120,6 +124,7 @@ Example:
>>> etb_in_port: endpoint at 0 {
>> There shouldn't be a unit address here because there is no reg property.
>>> slave-mode;
>>> remote-endpoint = <&replicator_out_port0>;
>>> + coresight,hwid = <0>;
>> It doesn't make sense for these to be in the endpoint. If you had
>> multiple endpoints, then you would have to duplicate it. "ports" are
>> a single data stream. "endpoints" are connections to that stream. So if
>> you have a muxed (input) or fanout/1-to-many (output) connection, then
>> you have multiple endpoints.
>
> We do have many-to-1 input (e.g, funnels) and 1-to-many outputs
> (e.g replicators). However, we have (so far) used only one endpoint per
> port.
>
> Also we could potentially have multiple data streams flowing through
> the ports, which gets filtered to different ports in 1-to-many components
> (read programmable-replicator).
>
> So the point is we have a shared path which carries different data
> streams with mux/demux components. I am open for suggestions based on
> the above facts.
>
>> The same applied to the slave-mode property, but that ship has sailed.
>> No reason to continue that though.
>>> };
>>> };
>>> };
>>> @@ -134,6 +139,7 @@ Example:
>>> tpiu_in_port: endpoint at 0 {
>>> slave-mode;
>>> remote-endpoint = <&replicator_out_port1>;
>>> + coresight,hwid = <0>;
>>> };
>>> };
>>> };
>>> @@ -154,6 +160,7 @@ Example:
>>> reg = <0>;
>>> replicator_out_port0: endpoint {
>>> remote-endpoint = <&etb_in_port>;
>>> + coresight,hwid = <0>;
>>> };
>>> };
>>> @@ -161,15 +168,17 @@ Example:
>>> reg = <1>;
>>> replicator_out_port1: endpoint {
>>> remote-endpoint = <&tpiu_in_port>;
>>> + coresight,hwid = <1>;
>>> };
>>> };
>>> /* replicator input port */
>>> port at 2 {
>>> - reg = <0>;
>>> + reg = <1>;
>> This will still get flagged as an error. reg must be 2 here.
>
> Sorry, thats a mistake. I will fix it.
>
> Cheers
> Suzuki
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply
* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Christoph Hellwig @ 2018-06-13 12:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com>
On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.
dma mapping implementations need to zero all memory returned anyway
(even if a few implementation don't do that yet).
I'd rather keep the zeroing in the common callers.
^ permalink raw reply
* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Christoph Hellwig @ 2018-06-13 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613124001eucas1p2422f7916367ce19fecd40d6131990383~3uKFrT3ML1977219772eucas1p2G@eucas1p2.samsung.com>
On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
> by the
> memory compaction code, which is used in alloc_contig_range(). Right, this
> should be also noted in the documentation.
Documentation is good, asserts are better. The code should reject any
flag not explicitly supported, or even better have its own flags type
with the few actually supported flags.
^ permalink raw reply
* [PATCH] ARM64: dts: rockchip: add some pins to rk3399
From: Heiko Stübner @ 2018-06-13 12:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <B649911B-28E0-4466-B3F0-15875FFB9D43@theobroma-systems.com>
Am Dienstag, 12. Juni 2018, 20:21:06 CEST schrieb klaus.goger at theobroma-
systems.com:
> Hi Randy,
>
> > On 12.06.2018, at 17:25, Randy Li <ayaka@soulik.info> wrote:
> >
> > Those pins would be used by many boards.
> >
> > Signed-off-by: Randy Li <ayaka@soulik.info>
agree to everything Klaus said ;-) .
[...]
> > + pcie_clkreqn: pci-clkreqn {
> > + rockchip,pins =
> > + <2 26 RK_FUNC_2 &pcfg_pull_none>;
> > + };
> > +
> > + pcie_clkreqnb: pci-clkreqnb {
> > + rockchip,pins =
> > + <4 24 RK_FUNC_1 &pcfg_pull_none>;
> > + };
> > +
>
> I?m not sure if pci-clkreqn is functional at all. If not I?m not sure if we
> should add it to the dtsi. Shawn may know more about it.
Yep, wasn't there a big change away from clkreqn, due it
not being functional?
> > pcie_clkreqnb_cpm: pci-clkreqnb-cpm {
> >
> > rockchip,pins =
> >
> > - <4 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
> > + <4 24 RK_FUNC_GPIO &pcfg_pull_none>;
> >
> > };
> >
> > };
>
> Could we actually use RK_Pxx for all new pin definitions? Would increase
> readability a lot.
Especially as the above change really only seems to change RK_PD0 back
to 24, so this block (and some others) will go away entirely.
Heiko
^ permalink raw reply
* [RFC V2 3/3] perf: qcom: Add Falkor CPU PMU IMPLEMENTATION DEFINED event support
From: Marc Zyngier @ 2018-06-13 12:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613103555.GC26280@arm.com>
On 13/06/18 11:35, Will Deacon wrote:
> On Tue, Jun 12, 2018 at 04:41:32PM -0400, Agustin Vega-Frias wrote:
>> Hi Mark,
>>
>> On 2018-06-12 10:40, Mark Rutland wrote:
>>> Hi,
>>>
>>> On Thu, Jun 07, 2018 at 09:56:48AM -0400, Agustin Vega-Frias wrote:
>>>> Selection of these events can be envisioned as indexing them from
>>>> a 3D matrix:
>>>> - the first index selects a Region Event Selection Register
>>>> (PMRESRx_EL0)
>>>> - the second index selects a group from which only one event at a time
>>>> can be selected
>>>> - the third index selects the event
>>>>
>>>> The event is encoded into perf_event_attr.config as 0xPRCCG, where:
>>>> P [config:16 ] = prefix (flag that indicates a matrix-based
>>>> event)
>>>> R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
>>>> G [config:0-3 ] = group (specifies the event group)
>>>> CC [config:4-11 ] = code (specifies the event)
>>>>
>>>> Events with the P flag set to zero are treated as common PMUv3 events
>>>> and are directly programmed into PMXEVTYPERx_EL0.
>>>>
>>>> The first two indexes are set combining the RESR and group number with
>>>> a base number and writing it into the architected PMXEVTYPER_EL0
>>>> register.
>>>> The third index is set by writing the code into the bits corresponding
>>>> with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
>>>> register.
>>>
>>> When are the IMP DEF registers accessible at EL0? Are those goverend by
>>> the same controls as the architected registers?
>>
>> No, there is a separate IMP DEF register to control access.
>
> Great :( We need to make sure we disable EL0 access during boot then, but
> that means we need to prove for the existence of this thing in head.S
> (since the PMU driver might not get loaded).
>
> Also, what's the kvm story here so that we don't accidentally open up a
> VM-VM side-channel via these registers? How do the EL1 trapping controls
> work?
We'd trap the IMPDEF register access and inject an UNDEF (assuming that
the IMPDEF trapping works correctly). I have strictly no plan to support
this in a guest.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [RFC V2 3/3] perf: qcom: Add Falkor CPU PMU IMPLEMENTATION DEFINED event support
From: Will Deacon @ 2018-06-13 13:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <06c81a64-904f-59fa-5751-62818d6d2107@arm.com>
On Wed, Jun 13, 2018 at 01:59:58PM +0100, Marc Zyngier wrote:
> On 13/06/18 11:35, Will Deacon wrote:
> > On Tue, Jun 12, 2018 at 04:41:32PM -0400, Agustin Vega-Frias wrote:
> >> On 2018-06-12 10:40, Mark Rutland wrote:
> >>> On Thu, Jun 07, 2018 at 09:56:48AM -0400, Agustin Vega-Frias wrote:
> >>>> Selection of these events can be envisioned as indexing them from
> >>>> a 3D matrix:
> >>>> - the first index selects a Region Event Selection Register
> >>>> (PMRESRx_EL0)
> >>>> - the second index selects a group from which only one event at a time
> >>>> can be selected
> >>>> - the third index selects the event
> >>>>
> >>>> The event is encoded into perf_event_attr.config as 0xPRCCG, where:
> >>>> P [config:16 ] = prefix (flag that indicates a matrix-based
> >>>> event)
> >>>> R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
> >>>> G [config:0-3 ] = group (specifies the event group)
> >>>> CC [config:4-11 ] = code (specifies the event)
> >>>>
> >>>> Events with the P flag set to zero are treated as common PMUv3 events
> >>>> and are directly programmed into PMXEVTYPERx_EL0.
> >>>>
> >>>> The first two indexes are set combining the RESR and group number with
> >>>> a base number and writing it into the architected PMXEVTYPER_EL0
> >>>> register.
> >>>> The third index is set by writing the code into the bits corresponding
> >>>> with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
> >>>> register.
> >>>
> >>> When are the IMP DEF registers accessible at EL0? Are those goverend by
> >>> the same controls as the architected registers?
> >>
> >> No, there is a separate IMP DEF register to control access.
> >
> > Great :( We need to make sure we disable EL0 access during boot then, but
> > that means we need to prove for the existence of this thing in head.S
> > (since the PMU driver might not get loaded).
> >
> > Also, what's the kvm story here so that we don't accidentally open up a
> > VM-VM side-channel via these registers? How do the EL1 trapping controls
> > work?
>
> We'd trap the IMPDEF register access and inject an UNDEF (assuming that
> the IMPDEF trapping works correctly). I have strictly no plan to support
> this in a guest.
Ah, so we could actually configure that in el2_setup and solve the host
problem if we're entered at EL2. Agustin -- does that work, and what do we
need to do if the host is entered at EL1?
Will
^ permalink raw reply
* [RFC V2 3/3] perf: qcom: Add Falkor CPU PMU IMPLEMENTATION DEFINED event support
From: Mark Rutland @ 2018-06-13 13:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9957219b64252d3b2e19724db04a1179@codeaurora.org>
On Tue, Jun 12, 2018 at 04:41:32PM -0400, Agustin Vega-Frias wrote:
> On 2018-06-12 10:40, Mark Rutland wrote:
> > On Thu, Jun 07, 2018 at 09:56:48AM -0400, Agustin Vega-Frias wrote:
> > > +/*
> > > + * Qualcomm Technologies CPU PMU IMPLEMENTATION DEFINED extensions
> > > support
> > > + *
> > > + * Current extensions supported:
> > > + *
> > > + * - Matrix-based microarchitectural events support
> > > + *
> > > + * Selection of these events can be envisioned as indexing them
> > > from
> > > + * a 3D matrix:
> > > + * - the first index selects a Region Event Selection Register
> > > (PMRESRx_EL0)
> > > + * - the second index selects a group from which only one event
> > > at a time
> > > + * can be selected
> > > + * - the third index selects the event
> > > + *
> > > + * The event is encoded into perf_event_attr.config as 0xPRCCG,
> > > where:
> > > + * P [config:16 ] = prefix (flag that indicates a
> > > matrix-based event)
> > > + * R [config:12-15] = register (specifies the PMRESRx_EL0
> > > instance)
> > > + * G [config:0-3 ] = group (specifies the event group)
> > > + * CC [config:4-11 ] = code (specifies the event)
> > > + *
> > > + * Events with the P flag set to zero are treated as common PMUv3
> > > events
> > > + * and are directly programmed into PMXEVTYPERx_EL0.
> >
> > When PMUv3 is given a raw event code, the config fields should be the
> > PMU event number, and this conflicts with RESERVED encodings.
> >
> > I'd rather we used a separate field for the QC extension events. e.g.
> > turn config1 into a flags field, and move the P flag there.
> >
> > We *should* add code to sanity check those fields are zero in the PMUv3
> > driver, even though it's a potential ABI break to start now.
>
> I should have stated clearly that in this case the event code is directly
> programmed into PMXEVTYPERx_EL0.evtCount, not by this code, but by the PMUv3
> code, which will do the masking and ensure reserved bits are not touched.
I understand this may be fine on the HW side; it's more to do with the
userspace ABI expected with PMUv3. The config encoding should be the
(PMUv3) type field, and I don't want a potential clash if/when PMUv3
gets extended in future beyond the current 16 bits.
I'd very much like to keep the QC extension bits separate from that.
> IOW, that case is no different from the raw event or a common event case.
>
> I would prefer to keep the flag in config because it allows the use of
> raw code encodings to access these events more easily, and given that
> the flag is never propagated to any register I believe it is safe.
You can easily add sysfs fields to make this easy, e.g. have reg map to
config1:x-y, and the user can specify the event based on that, e.g.
perf ${cmd} -e qcom_pmuv3/reg=0xf,.../
Which means they're *explicitly* asking for the QC extension bits, and
we can be sure they're not asking for something from baseline PMUv3.
We *might* want to namespace the qc fields, in case we want to add
anything similar to PMUv3 in future.
[...]
> > > +/*
> > > + * Check if e1 and e2 conflict with each other
> > > + *
> > > + * e1 is a matrix-based microarchitectural event we are checking
> > > against e2.
> > > + * A conflict exists if the events use the same reg, group, and a
> > > different
> > > + * code. Events with the same code are allowed because they could
> > > be using
> > > + * different filters (e.g. one to count user space and the other to
> > > count
> > > + * kernel space events).
> > > + */
> > Does the filter matter at all? When happens if I open two identical
> > events, both counting the same reg, group, and code, with the same
> > filter?
>
> That is possible and allowed, similar to counting the same common event
> in two configurable counters. Only problem is wasting a counter resource.
Ok. Please drop the mention of filtering from the comment -- it makes it
sound like there's a potential problem, and it isn't relevant.
[...]
> > > + /* Matrix event, program the appropriate PMRESRx_EL0 */
> > > + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> > > + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> > > + u64 reg = QC_EVT_REG(event->attr.config);
> > > + u64 code = QC_EVT_CODE(event->attr.config);
> > > + u64 group = QC_EVT_GROUP(event->attr.config);
> > > + unsigned long flags;
> > > +
> > > + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> > > + falkor_set_resr(reg, group, code);
> > > + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> >
> > Why is the spinlock required?
> >
> > AFACIT this should only ever be called in contexts where IRQs are
> > disabled already.
> >
>
> falkor_set_resr is a read-modify-write operation. The PMUv3 code uses
> the spinlock to protect the counter selection too (armv8pmu_enable_event).
As I mention, that only happens in contexts where IRQs are disabled, so
that shouldn't be a problem.
> I believe this is to deal with event rotation which can potentially
> be active when we are creating new events.
Event rotation happens off the back of a hrtimer callback, with IRQs
disabled. There's a lockdep assert to that effect in
perf_mux_hrtimer_handler(), before it calls perf_rotate_context().
> > > + }
> > > +
> > > + /* Let the original op handle the rest */
> > > + def_ops->enable(event);
> > > +}
> > > +
> > > +/*
> > > + * Disable the given event
> > > + */
> > > +static void falkor_disable(struct perf_event *event)
> > > +{
> > > + /* Use the original op to disable the counter and interrupt */
> > > + def_ops->enable(event);
> > > +
> > > + if (!!(event->attr.config & QC_EVT_PFX_MASK)) {
> > > + /* Matrix event, de-program the appropriate PMRESRx_EL0 */
> > > + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> > > + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> > > + u64 reg = QC_EVT_REG(event->attr.config);
> > > + u64 group = QC_EVT_GROUP(event->attr.config);
> > > + unsigned long flags;
> > > +
> > > + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> > > + falkor_clear_resr(reg, group);
> > > + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> > > + }
> > > +}
> >
> > Same comments as with falkor_enable().
> >
> > > +
> > > +PMU_FORMAT_ATTR(event, "config:0-15");
> > > +PMU_FORMAT_ATTR(prefix, "config:16");
> > > +PMU_FORMAT_ATTR(reg, "config:12-15");
> > > +PMU_FORMAT_ATTR(code, "config:4-11");
> > > +PMU_FORMAT_ATTR(group, "config:0-3");
> >
> > What sort of events are available? Do you plan to add anything to the
> > userspace event database in tools/perf/pmu-events/ ?
> >
>
> Yes, we are still doing some internal work to see what we can put in
> the driver or as JSON events.
Please put them in userspace. Events living in the kernel is really a
legacy thing, and we've placed all other IMP-DEF events in userspace for
ACPI systems.
[...]
> > > + pmu->name = "qcom_pmuv3";
> >
> > All the other CPU PMUs on an ARM ACPI system will have an index suffix,
> > e.g. "armv8_pmuv3_0". I can see why we might want to change the name to
> > indicate the QC extensions, but I think we should keep the existing
> > pattern, with a '_0' suffix here.
>
> This overrides the name before the suffix is added, so the PMU name will be
> qcom_pmuv3_0 for Centriq 2400 which has only Falkor CPUs.
Ok.
[...]
> > > + /* Override the necessary ops */
> > > + pmu->map_event = falkor_map_event;
> > > + pmu->get_event_idx = falkor_get_event_idx;
> > > + pmu->reset = falkor_reset;
> > > + pmu->enable = falkor_enable;
> > > + pmu->disable = falkor_disable;
> >
> > I'm somewhat concerned by hooking into the existing PMU code at this
> > level, but I don't currently have a better suggestion.
> >
>
> IMO this is no different from other PMUs implemented on top of the arm_pmu
> framework. The difference is of course that I'm calling back into the base
> PMUv3 ops,
Yup, the latter is what I'm concerned about. e.g. when you take the
pmu_lock, if PMUv3 code has already taken this, there'll be a deadlock,
and it means that we can't consider the PMUv3 code in isolation.
As long as we can keep this *simple*, that'll just leave me uneasy.
Thanks,
Mark.
^ permalink raw reply
* [U-Boot] [RFC PATCH 0/2] ARM: v7: Enable basic framework for supporting bits for CVE-2017-5715
From: Nishanth Menon @ 2018-06-13 13:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613100803.GB17671@n2100.armlinux.org.uk>
On 10:08-20180613, Russell King - ARM Linux wrote:
> On Tue, Jun 12, 2018 at 04:58:34PM -0500, Nishanth Menon wrote:
> > On 21:40-20180612, Russell King - ARM Linux wrote:
> > [...]
> > > > I started respinning the series, while there is definitely a use of
> > > > implementing in u-boot,
> > > > I am starting to wonder if we should also be doing this in kernel.
> > >
> > > How does the kernel set the bit when the kernel is running in non-secure
> > > mode, when the ACTLR is read-only in that mode?
> >
> > For OMAP5/DRA7 SMP systems, I just posted a patch that seems to resolve
> > it:
> > https://patchwork.kernel.org/patch/10461273/
> >
> > This'd be similar in implementation to ARM erratum 801819 workaround
> > that needs two pieces (u-boot + kernel). I am not really worried about
> > OMAP5/DRA7 since they should'nt loose context in Low power modes.
> > Other SoCs need to be aware of the constraints.
> >
> > /me wishes PSCI was a standard during ARMv7, but it was'nt... So
> > legacy v7 SoCs have implementations that are kind of different (even
> > smc calling conventions vary).
>
> It may seem to be an easy way out (do everything in the kernel) but
> have you considered that the secure world is also vulnerable?
Yes, we have.
> If the IBE bit is not set in the secure world, then the secure world
> is not implementing the workarounds, and therefore the non-secure
> world has the possibility to use the Spectre vulnerabilities to
> exploit the secure world with enough effort and knowledge.
>
> You really need to _also_ fix these vulnerabilities in the secure
> world, which includes setting the IBE bit there.
Correct, but, unfortunately, this is NOT always the case / not necessary in
some cases. Example:
On General purpose (GP) devices such as in OMAP, ROM owns the secure APIs,
there is no override of secure world APIs possible. In such cases, we
have to see if there is anything that needs to be protected. in case of
GP devices, there are no secure assets to protect and any SMC calls are
just support services provided by ROM.
a) updating secure side is not possible
b) secure side updates is not necessary since there are no critical run time
services provided by ROM.
On devices such as Keystone 2 (TI) generation, yes, we do have ability
to update secure side and must be done as well.
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH] ARM: DRA7/OMAP5: Enable ACTLR[0] (Enable invalidates of BTB) for secondary cores
From: Nishanth Menon @ 2018-06-13 13:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180613101153.GD6920@n2100.armlinux.org.uk>
On 10:11-20180613, Russell King - ARM Linux wrote:
> On Tue, Jun 12, 2018 at 04:36:11PM -0500, Nishanth Menon wrote:
> > Call secure services to enable ACTLR[0] (Enable invalidates of BTB with
> > ICIALLU) when branch hardening is enabled for kernel.
>
> As mentioned elsewhere, I don't think this is a good idea - if the secure
> world is not implementing the Spectre workarounds, then the _system_ is
> exploitable.
>
> If the secure world is implementing the spectre workarounds, it will
> already have enabled the IBE bit (which is r/w from secure, read only
> from non-secure.)
>
> So, basically, lack of the IBE bit being set is basically telling the
> kernel that it's running on a vulnerable platform _even if the kernel
> were to set it through some means_.
On GP devices OMAP5/DRA7, there is no possibility to update secure side
since "secure world" is ROM and there are no override mechanisms possible.
on HS devices, I agree, appropriate PPA will do the workarounds as well.
However, this patch is to enable the IBE enable on GP device for _a_
core can only be done via SMC services that ROM provides for
specifically the reasons you have already stated. u-boot will only
enable the IBE for the boot core, by the time the secondary cores start
up, u-boot is long gone.. so someone has to invoke the SMC call to
enable the IBE bit for the secondary core.
This is what the patch does.
If the above explanation makes sense, I will add that to the commit log
as well.
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 2/4] ARM: Introduce ability to enable invalidate of BTB with ICIALLU on Cortex-A15 for CVE-2017-5715
From: Nishanth Menon @ 2018-06-13 13:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8fac2f1c-ecc4-1aa1-0620-8ac6e2efdbf1@gmail.com>
On 23:05-20180612, Marek Vasut wrote:
> On 06/12/2018 10:24 PM, Nishanth Menon wrote:
[..]
> > +#ifdef CONFIG_ARM_CORTEX_A15_CVE_2017_5715
> > + mrc p15, 0, r0, c1, c0, 1 @ read auxilary control register
> > + orr r0, r0, #1 << 0 @ Enable invalidates of BTB
>
> Can we use BIT() macro in the assembler code too ?
Probably, but just following convention in the rest of the file. Do we
want to change from existing code?
--
Regards,
Nishanth Menon
^ permalink raw reply
* [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
From: Suzuki K Poulose @ 2018-06-13 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5448BBB7-93FE-400F-9D87-FABF5DE0539C@arm.com>
Hi Matt,
Thanks for your comments, responses inline.
On 13/06/18 13:49, Matt Sealey wrote:
> Suzuki,
>
> Why not use ?unit??
>
> I believe we had this discussion years ago about numbering serial ports and sdhci (i.e. how do you know it?s UART0 or UART1 from just the address? Some SoC?s don?t address sequentially *or* in a forward direction) - I believe it?s not exactly codified in ePAPR, not am I sure where it may be otherwise, but it exists.
We have different situation here. We need to know *the port number* as understood by the
hardware, so that we can enable *the specific* port for a given path.
>
> I agree with Rob on the slave-mode nonsense, this is an SPI controller concept weirdly stuffed into a directed graph which implicitly tells you the data direction - it?s a rooted tree (just like DT!).
Btw, the "slave-mode" is not a standard DT graph binding. It is not part of the
generic DT graph binding. In fact the generic bindings stay away from the direction
aspect and explicitly mentions the same.
>
> For the case of a funnel each device supplying trace should end up into an input node - numbered with a unit - and all those nodes should point to the output node as endpoints. Describing the hardware as a black box is probably less of a good idea than showing that it?s a funnel, or replicator by showing the internal paths. You wouldn?t need to ?number? ports with a unit except where the HW needs to differentiate between them, and you don?t need reg or a node address to do it.
>
As I mentioned above, we need the hardware numbers to enable the "specific" port.
E.g, :
static void funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
{
u32 functl;
CS_UNLOCK(drvdata->base);
functl = readl_relaxed(drvdata->base + FUNNEL_FUNCTL);
functl &= ~FUNNEL_HOLDTIME_MASK;
functl |= FUNNEL_HOLDTIME;
functl |= (1 << port);
writel_relaxed(functl, drvdata->base + FUNNEL_FUNCTL);
writel_relaxed(drvdata->priority, drvdata->base + FUNNEL_PRICTL);
CS_LOCK(drvdata->base);
}
> If you really need to parse full graphs in both directions (find root, find leaf) then could we simply introduce properties which list the phandles of all uplink sources, as linked lists point to the list head?
No we don't need to parse it in both ways, up and down. Btw, the trace paths
are not statically created. They are done at runtime, as configured by the
user. So all we need to do is have a list of the ports and the devices it
is connected to (of course with direction information). I would stay
away from duplicating the platform code when something already does
a good job.
>
> This gives a way to validate that the graph starts and ends the way we expect, and also allows every port to be associated with being a required path between any two devices without parsing the *whole* graph (although you still need to do that to find the route to sinks).
Coming back to your suggestion of "unit", what does it imply ?
Its too generic a term for something as concrete as a port number.
Cheers
Suzuki
>
> Ta,
> Matt
>
> Sent from my iPhone
>
>> On Jun 13, 2018, at 04:45, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>>
>> Hi Rob,
>>
>>> On 12/06/18 21:48, Rob Herring wrote:
>>>> On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
>>>> The coresight drivers relied on default bindings for graph
>>>> in DT, while reusing the "reg" field of the "ports" to indicate
>>>> the actual hardware port number for the connections. However,
>>>> with the rules getting stricter w.r.t to the address mismatch
>>>> with the label, it is no longer possible to use the port address
>>>> field for the hardware port number. Hence, we add an explicit
>>>> property to denote the hardware port number, "coresight,hwid"
>>>> which must be specified for each "endpoint".
>>>>
>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> ---
>>>> .../devicetree/bindings/arm/coresight.txt | 26 +++++++++---
>>>> drivers/hwtracing/coresight/of_coresight.c | 46 ++++++++++++++++------
>>>> 2 files changed, 54 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>>>> index bd36e40..385581a 100644
>>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>>>> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
>>>> "slave-mode"
>>>> * Hardware Port number at the component:
>>>> - - The hardware port number is assumed to be the address of the "port" component.
>>>> + - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
>>>> + - Each "endpoint" must define the hardware port of the local end of the
>>>> + connection using the following property:
>>>> + "coresight,hwid" - 32bit integer, hardware port number at the local end.
>>> "coresight" is not a vendor and properties are in the form
>>> [<vendor>,]<prop-name>.
>>
>> OK. The issue here is that a coresight component could be an Arm IP or
>> a custom partner IP. So, the vendor could be either arm or the partner id.
>> However, this property is kind of a generic one for the Coresight family,
>> which is why we opted for "coresight". What is the guideline for such
>> cases ?
>>
>> Or in other words I see the following possible options :
>>
>> 1) coresight,hwid - coresight generic
>> 2) arm,coresight-hwid - arm vendor, however the device could be from any vendor.
>> 3) hwid - Generic
>> 4) none of the above, something completely different.
>>
>> What do you recommend from the above ?
>>
>>>> +
>>>> Example:
>>>> @@ -120,6 +124,7 @@ Example:
>>>> etb_in_port: endpoint at 0 {
>>> There shouldn't be a unit address here because there is no reg property.
>>>> slave-mode;
>>>> remote-endpoint = <&replicator_out_port0>;
>>>> + coresight,hwid = <0>;
>>> It doesn't make sense for these to be in the endpoint. If you had
>>> multiple endpoints, then you would have to duplicate it. "ports" are
>>> a single data stream. "endpoints" are connections to that stream. So if
>>> you have a muxed (input) or fanout/1-to-many (output) connection, then
>>> you have multiple endpoints.
>>
>> We do have many-to-1 input (e.g, funnels) and 1-to-many outputs
>> (e.g replicators). However, we have (so far) used only one endpoint per
>> port.
>>
>> Also we could potentially have multiple data streams flowing through
>> the ports, which gets filtered to different ports in 1-to-many components
>> (read programmable-replicator).
>>
>> So the point is we have a shared path which carries different data
>> streams with mux/demux components. I am open for suggestions based on
>> the above facts.
>>
>>> The same applied to the slave-mode property, but that ship has sailed.
>>> No reason to continue that though.
>>>> };
>>>> };
>>>> };
>>>> @@ -134,6 +139,7 @@ Example:
>>>> tpiu_in_port: endpoint at 0 {
>>>> slave-mode;
>>>> remote-endpoint = <&replicator_out_port1>;
>>>> + coresight,hwid = <0>;
>>>> };
>>>> };
>>>> };
>>>> @@ -154,6 +160,7 @@ Example:
>>>> reg = <0>;
>>>> replicator_out_port0: endpoint {
>>>> remote-endpoint = <&etb_in_port>;
>>>> + coresight,hwid = <0>;
>>>> };
>>>> };
>>>> @@ -161,15 +168,17 @@ Example:
>>>> reg = <1>;
>>>> replicator_out_port1: endpoint {
>>>> remote-endpoint = <&tpiu_in_port>;
>>>> + coresight,hwid = <1>;
>>>> };
>>>> };
>>>> /* replicator input port */
>>>> port at 2 {
>>>> - reg = <0>;
>>>> + reg = <1>;
>>> This will still get flagged as an error. reg must be 2 here.
>>
>> Sorry, thats a mistake. I will fix it.
>>
>> Cheers
>> Suzuki
^ 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