Re: [PATCH v2] thermal_hwmon: Fix the 'No sensors found' error after replacing the parameter NULL by the actual device

[Marc Zyngier]

On 03/10/18 08:25, Cao Van Dong wrote:

Formerly, when registering the hwmon device, we pass NULL as the device. It's 
not a problem.
Recently, the developer has replaced the parameter NULL as the device by the 
actual device.
This causes the "No sensors found" error. Therefore, instead of using the 
device we will use pass


What does report this error? Is it a userspace application?


the parent of that device as parameter. This will sync with the processor on 
the hwmon core.
This patch is to fix this error.

This patch is based on the v4.19-rc3 tag.

---


This patch has no SoB.


  drivers/thermal/thermal_hwmon.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index 40c69a5..a918ba9 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -143,7 +143,7 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
INIT_LIST_HEAD(>tz_list);
strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
strreplace(hwmon->type, '-', '_');
-   hwmon->device = hwmon_device_register_with_info(>device, 
hwmon->type,
+   hwmon->device = hwmon_device_register_with_info(tz->device.parent, 
hwmon->type,
hwmon, NULL, NULL);
if (IS_ERR(hwmon->device)) {
result = PTR_ERR(hwmon->device);



It is not clear to me that this is any better. What is the parent device 
in this case? Can you give an example of what breaks in the hierarchy?


Given how close we are to to 4.19, I'd rather we revert f6b6b52ef7a54160 
if there are userspace visible regressions.


Thanks,

M.
--
Jazz is not dead. It just smells funny...

Re: [PATCH] dt-bindings: irqchip: renesas-irqc: Document r8a7744 support

[Marc Zyngier]

On 25/09/18 17:56, Biju Das wrote:

Document RZ/G1N (R8A7744) SoC bindings.

Signed-off-by: Biju Das 
Reviewed-by: Chris Paterson 
---
  Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git 
a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt 
b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
index a046ed3..97a8c83 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
@@ -6,6 +6,7 @@ Required properties:
Examples with soctypes are:
  - "renesas,irqc-r8a73a4" (R-Mobile APE6)
  - "renesas,irqc-r8a7743" (RZ/G1M)
+- "renesas,irqc-r8a7744" (RZ/G1N)
  - "renesas,irqc-r8a7745" (RZ/G1E)
  - "renesas,irqc-r8a77470" (RZ/G1C)
  - "renesas,irqc-r8a7790" (R-Car H2)



Queued for 4.20.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

Re: [PATCH] dt-bindings: irqchip: renesas-irqc: Document R-Car E3 support

[Marc Zyngier]

On 28/09/18 12:32, Geert Uytterhoeven wrote:

Document support for the Interrupt Controller for External Devices
(INTC-EX) in the Renesas E3 (r8a77990) SoC.

No driver update is needed.

Signed-off-by: Geert Uytterhoeven 
---
  .../devicetree/bindings/interrupt-controller/renesas,irqc.txt | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt 
b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
index a046ed374d808d66..b8a86e30e8e67068 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
@@ -2,7 +2,8 @@ DT bindings for the R-Mobile/R-Car/RZ/G interrupt controller
  
  Required properties:
  
-- compatible: has to be "renesas,irqc-", "renesas,irqc" as fallback.

+- compatible: must be "renesas,irqc-" or "renesas,intc-ex-",
+ and "renesas,irqc" as fallback.
Examples with soctypes are:
  - "renesas,irqc-r8a73a4" (R-Mobile APE6)
  - "renesas,irqc-r8a7743" (RZ/G1M)
@@ -19,6 +20,7 @@ Required properties:
  - "renesas,intc-ex-r8a77965" (R-Car M3-N)
  - "renesas,intc-ex-r8a77970" (R-Car V3M)
  - "renesas,intc-ex-r8a77980" (R-Car V3H)
+- "renesas,intc-ex-r8a77990" (R-Car E3)
  - "renesas,intc-ex-r8a77995" (R-Car D3)
  - #interrupt-cells: has to be <2>: an interrupt index and flags, as defined in
interrupts.txt in this directory



Queued for 4.20.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

Re: [PATCH] dt-bindings: irqchip: renesas-irqc: Document r8a77470 support

[Marc Zyngier]

Hi Biju,

On 27/06/18 09:43, Biju Das wrote:
> Hello Marc,
> 
> Sorry to bother you.  Are you happy to merge the below patch with 4.18-rc3?
> https://www.spinics.net/lists/linux-renesas-soc/msg25788.html

This is odd. I queued that in April as part of a potential second set of
fixes for 4.17, but somehow failed to ever send the pull request (and
this was the only patch I had at that point).

Apologies for the delay, I'll sync up with Thomas for this patch to make
it into 4.18.

Thanks for the heads up,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v9 07/12] ARM: sunxi: Add initialization of CNTVOFF

[Marc Zyngier]

On 04/05/18 20:05, Mylène Josserand wrote:
> Add the initialization of CNTVOFF for sun8i-a83t.
> 
> For boot CPU, create a new machine that handles this
> function's call in an "init_early" callback. We need to initialize
> CNTVOFF before the arch timer's initialization otherwise, it will
> not be taken into account and fails to boot correctly.
> Because of that, this function can't be called in SMP's early_initcall
> function which is called after timer's init.
> 
> For secondary CPUs, add this function into secondary_startup
> assembly entry.
> 
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> Acked-by: Maxime Ripard <maxime.rip...@bootlin.com>

Acked-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v9 06/12] ARM: smp: Add initialization of CNTVOFF

[Marc Zyngier]

On 04/05/18 20:05, Mylène Josserand wrote:
> The CNTVOFF register from arch timer is uninitialized.
> It should be done by the bootloader but it is currently not the case,
> even for boot CPU because this SoC is booting in secure mode.
> It leads to an random offset value meaning that each CPU will have a
> different time, which isn't working very well.
> 
> Add assembly code used for boot CPU and secondary CPU cores to make
> sure that the CNTVOFF register is initialized. Because this code can
> be used by different platforms, add this assembly file in ARM's common
> folder.
> 
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>
> Tested-by: Geert Uytterhoeven <geert+rene...@glider.be>

Reviewed-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] dt-bindings: irqchip: renesas-irqc: Document r8a77470 support

[Marc Zyngier]

On 29/03/18 11:17, Biju Das wrote:
> Renesas RZ/G SoC have the R-Car gen2 compatible IRQC interrupt
> controllers. Document RZ/G1C (also known as R8A77470) SoC bindings.
> 
> Signed-off-by: Biju Das 
> Reviewed-by: Fabrizio Castro 
> ---
>  Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt 
> b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> index 20f121d..1d5891a 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> @@ -7,6 +7,7 @@ Required properties:
>  - "renesas,irqc-r8a73a4" (R-Mobile APE6)
>  - "renesas,irqc-r8a7743" (RZ/G1M)
>  - "renesas,irqc-r8a7745" (RZ/G1E)
> +- "renesas,irqc-r8a77470" (RZ/G1C)
>  - "renesas,irqc-r8a7790" (R-Car H2)
>  - "renesas,irqc-r8a7791" (R-Car M2-W)
>  - "renesas,irqc-r8a7792" (R-Car V2H)
> 

Queued with Simon and Geert's RB tags.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 0/3] renesas: irqchip: Use wakeup_path i.s.o. explicit clock handling

[Marc Zyngier]

On 12/02/18 13:55, Geert Uytterhoeven wrote:
>   Hi all,
> 
> If an interrupt controller in a Renesas ARM SoC is part of a Clock
> Domain, and it is part of the wakeup path, it must be kept active during
> system suspend.
> 
> Currently this is handled in all interrupt controller drivers by
> explicitly increasing the use count of the module clock when the device
> is part of the wakeup path.  However, this explicit clock handling is
> merely a workaround for a failure to properly communicate wakeup
> information to the device core.
> 
> Hence this series fixes the affected drivers by setting the devices'
> power.wakeup_path fields instead, to indicate they are part of the
> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
> the genpd core code will keep the device enabled (and the clock running)
> during system suspend when needed.
> 
> Target trees:
>   - Patches 1 and 2 are meant for the irqchip tree,
Patches queued for 4.17.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Marc Zyngier]

On 04/07/17 19:20, Geert Uytterhoeven wrote:
> Hi Marc,
> 
> On Tue, Jul 4, 2017 at 7:32 PM, Marc Zyngier <marc.zyng...@arm.com> wrote:
>> On 04/07/17 18:02, Geert Uytterhoeven wrote:
>>> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
>>> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>>>
>>> WARNING: Underflow in clocksource 'arch_sys_counter' observed, time 
>>> update ignored.
>>>Please report this, consider using a different clocksource, if 
>>> possible.
>>>Your kernel is probably still fine.
>>>
>>> To fix this, wrap the standard secondary_startup routine inside a
>>> routine which initializes CNTVOFF when running on a Cortex-A7.
>>> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
>>> nibble of the Primary Part Number is sufficient.
>>>
>>> The initialization is identical to what is already done for the boot CPU
>>> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
>>> arch_timer initialization for r8a7794").
>>
>> Humfff... Pretty horrible. Comments below.
> 
> I know.  I suppose this should be done by the firmware/boot loader?

That's what is normally expected.

> But we have to live with firmware/boot loaders in the wild...

Yeah, I can tell. It is a shame that firmware people didn't realize that
just because the old firmware seems to work on a new revision of the
architecture, it doesn't mean it does everything right for that
particular revision... Oh well.

>>> --- /dev/null
>>> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * SMP support for APMU based systems
>>> + *
>>> + * Copyright (C) 2014  Renesas Electronics Corporation
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include 
>>> +
>>> +ENTRY(shmobile_boot_apmu)
>>> + mrc p15, 0, r0, c0, c0, 0   /* Get Main ID */
>>> + ubfxr1, r0, #4, #4  /* r1=Lo 4bit of Primary Part 
>>> */
>>> + cmp r1, #0x7/* 0x7 = CA7, 0xf = CA15 */
>>> + bne 1f
>>
>> Why don't you deal with the A15 parts as well? And TBH, you'd be better
>> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
> 
> Do we have to deal with the A15 parts here?
> We're not having issues with the A15 parts, so that's why I left it out.

Well, A15 has the exact same features as A7 when it comes to the timer,
and CNTVOFF definitely resets as UNKNOWN.

> 
> I'm trying to understand what's different between A15 and A7, and why A7
> needs this code, while A15 apparently doesn't.
> I'm not seeing any obvious differences in the U-Boot sources handling
> e.g. r8a7791/koelsch (dual A15) vs. r8a7794/alt (dual A7).

The UNKNOWN above might well be 0 on A15, but I wouldn't bet on it. If
nothing sets CNTVOFF on these systems, consider yourself lucky!

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Marc Zyngier]

Hi Geert,

On 04/07/17 18:02, Geert Uytterhoeven wrote:
> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
> 
> WARNING: Underflow in clocksource 'arch_sys_counter' observed, time 
> update ignored.
>Please report this, consider using a different clocksource, if 
> possible.
>Your kernel is probably still fine.
> 
> To fix this, wrap the standard secondary_startup routine inside a
> routine which initializes CNTVOFF when running on a Cortex-A7.
> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
> nibble of the Primary Part Number is sufficient.
> 
> The initialization is identical to what is already done for the boot CPU
> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
> arch_timer initialization for r8a7794").

Humfff... Pretty horrible. Comments below.

> 
> Based on patches by Hisashi Nakamura in the BSP.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  arch/arm/mach-shmobile/Makefile   |  1 +
>  arch/arm/mach-shmobile/common.h   |  1 +
>  arch/arm/mach-shmobile/headsmp-apmu.S | 38 
> +++
>  arch/arm/mach-shmobile/platsmp-apmu.c |  2 +-
>  4 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S
> 
> diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
> index 64611a1b4276517b..de735444e9f715fd 100644
> --- a/arch/arm/mach-shmobile/Makefile
> +++ b/arch/arm/mach-shmobile/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_ARCH_R8A7793)  += regulator-quirk-rcar-gen2.o
>  
>  # SMP objects
>  smp-y:= $(cpu-y)
> +smp-$(CONFIG_ARCH_RCAR_GEN2) += headsmp-apmu.o
>  smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7779)   += smp-r8a7779.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7790)   += smp-r8a7790.o
> diff --git a/arch/arm/mach-shmobile/common.h b/arch/arm/mach-shmobile/common.h
> index 1a8f7b3ab449db56..6792a07bce761aab 100644
> --- a/arch/arm/mach-shmobile/common.h
> +++ b/arch/arm/mach-shmobile/common.h
> @@ -11,6 +11,7 @@ extern void shmobile_smp_hook(unsigned int cpu, unsigned 
> long fn,
> unsigned long arg);
>  extern bool shmobile_smp_cpu_can_disable(unsigned int cpu);
>  extern bool shmobile_smp_init_fallback_ops(void);
> +extern void shmobile_boot_apmu(void);
>  extern void shmobile_boot_scu(void);
>  extern void shmobile_smp_scu_prepare_cpus(phys_addr_t scu_base_phys,
> unsigned int max_cpus);
> diff --git a/arch/arm/mach-shmobile/headsmp-apmu.S 
> b/arch/arm/mach-shmobile/headsmp-apmu.S
> new file mode 100644
> index ..52516d81ce98384c
> --- /dev/null
> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
> @@ -0,0 +1,38 @@
> +/*
> + * SMP support for APMU based systems
> + *
> + * Copyright (C) 2014  Renesas Electronics Corporation
> + *
> + * 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.
> + */
> +
> +#include 
> +
> +ENTRY(shmobile_boot_apmu)
> + mrc p15, 0, r0, c0, c0, 0   /* Get Main ID */
> + ubfxr1, r0, #4, #4  /* r1=Lo 4bit of Primary Part */
> + cmp r1, #0x7/* 0x7 = CA7, 0xf = CA15 */
> + bne 1f

Why don't you deal with the A15 parts as well? And TBH, you'd be better
off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.

> + /*
> +  * CA7 setup
> +  * CNTVOFF has to be initialized either from non-secure Hypervisor
> +  * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> +  * then it should be handled by the secure code
> +  */
> + cps 0x16

It'd be worth adding a MONITOR_MODE macro instead of this raw value.

> + mrc p15, 0, r1, c1, c1, 0   /* get Secure Config */
> + orr r0, r1, #1
> + mcr p15, 0, r0, c1, c1, 0   /* Set Non Secure bit */
> + isb
> + mov r0, #0
> + mcrrp15, 4, r0, r0, c14 /* CNTVOFF = 0 */
> + isb
> + mcr p15, 0, r1, c1, c1, 0   /* Set Secure bit */
> + isb
> + cps 0x13

and use SVC_MODE here.

> +1:
> +
> + b   secondary_startup
> +ENDPROC(shmobile_boot_apmu)
> diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c 
> b/arch/arm/mach-shmobile/platsmp-apmu.c
> index 3ca2c13346f0cbc3..4422b615a6ee6045 100644
> --- a/arch/arm/mach-shmobile/platsmp-apmu.c
> +++ b/arch/arm/mach-shmobile/platsmp-apmu.c
> @@ -204,7 +204,7 @@ void __init shmobile_smp_apmu_prepare_cpus(unsigned int 
> max_cpus,
>  int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct 
> *idle)
>  {
>   /* For this particular CPU register boot 

Re: [PATCH v2] irqchip/renesas-irqc: Postpone driver initialization

[Marc Zyngier]

Hi Geert,

On 08/11/16 19:35, Geert Uytterhoeven wrote:
> Currently the renesas-irqc driver uses postcore_initcall().
> 
> However, the new CPG/MSSR driver uses subsys_initcall(). Hence the
> IRQC's probe will be deferred, which causes the Micrel Ethernet PHY to
> not find its interrupt on R-Car Gen2 and RZ/G, as the of_mdio subsystem
> does not support deferred probe yet.
> 
> Replace postcore_initcall() by device_initcall() to work around this.
> 
> Note that on R-Mobile APE6, where the PFC/GPIO combo uses the IRQC as
> its parent interrupt controller, this does cause a few additional probe
> deferrals (for SCIFA0, SD0, SD1, and MMC). But the affected drivers
> handle that fine.
> 
> Signed-off-by: Geert Uytterhoeven 
> Tested-by: Sergei Shtylyov 
> ---
> v2:
>   - Drop RFC state,
>   - Add Tested-by,
>   - Improved description.
> ---
>  drivers/irqchip/irq-renesas-irqc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-renesas-irqc.c 
> b/drivers/irqchip/irq-renesas-irqc.c
> index 52304b139aa46a60..992849e54d00ea77 100644
> --- a/drivers/irqchip/irq-renesas-irqc.c
> +++ b/drivers/irqchip/irq-renesas-irqc.c
> @@ -295,7 +295,7 @@ static int __init irqc_init(void)
>  {
>   return platform_driver_register(_device_driver);
>  }
> -postcore_initcall(irqc_init);
> +device_initcall(irqc_init);

Overall, I'm not keen on these hacks (by moving from one initcall to
another, you're as likely to fix something than to break something else).

What should really be done is to either teach the various drivers to
handle deferred probing, or to teach the kernel to handle proper
dependencies (vastly more ambitious).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] drivers/perf: arm-pmu: Handle per-interrupt affinity mask

[Marc Zyngier]

Hi Geert,

On 19/07/16 14:25, Geert Uytterhoeven wrote:
> Hi Marc, Catalin, Will,
> 
> On Wed, Jul 6, 2016 at 4:33 PM, Marc Zyngier <marc.zyng...@arm.com> wrote:
>> On a big-little system, PMUs can be wired to CPUs using per CPU
>> interrups (PPI). In this case, it is important to make sure that
>> the enable/disable do happen on the right set of CPUs.
>>
>> So instead of relying on the interrupt-affinity property, we can
>> use the actual percpu affinity that DT exposes as part of the
>> interrupt specifier. The DT binding is also updated to reflect
>> the fact that the interrupt-affinity property shouldn't be used
>> in that case.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
>> ---
>> * From v1:
>>   - propagate the error if irq_get_percpu_devid_partition fails
> 
> This patch, which is commit 19a469a58720ea96 in arm64/for-next/core, broke
> the PMU on r8a7740/armadillo800eva:
> 
> -hw perfevents: enabled with armv7_cortex_a9 PMU driver, 7
> counters available
> +hw perfevents: /pmu: failed to probe PMU!
> +hw perfevents: /pmu: failed to register PMU devices!
> +armv7-pmu: probe of pmu failed with error -22
> 
> This is a single-core Cortex A9.
> 
>>  Documentation/devicetree/bindings/arm/pmu.txt |  4 +++-
>>  drivers/perf/arm_pmu.c| 27 
>> ++-
>>  2 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/pmu.txt 
>> b/Documentation/devicetree/bindings/arm/pmu.txt
>> index 74d5417..61c8b46 100644
>> --- a/Documentation/devicetree/bindings/arm/pmu.txt
>> +++ b/Documentation/devicetree/bindings/arm/pmu.txt
>> @@ -39,7 +39,9 @@ Optional properties:
>> When using a PPI, specifies a list of phandles to CPU
>>nodes corresponding to the set of CPUs which have
>>a PMU of this type signalling the PPI listed in the
>> -  interrupts property.
>> +  interrupts property, unless this is already specified
>> +  by the PPI interrupt specifier itself (in which case
>> +  the interrupt-affinity property shouldn't be present).
>>
>> This property should be present when there is more 
>> than
>>a single SPI.
> 
> On a single core, there's only a single SPI, hence there's no need for an
> "interrupt-affinity" property.
> 
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index 140436a..8e4d7f5 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -961,9 +964,23 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
>> i++;
>> } while (1);
>>
>> -   /* If we didn't manage to parse anything, claim to support all CPUs 
>> */
>> -   if (cpumask_weight(>supported_cpus) == 0)
>> -   cpumask_setall(>supported_cpus);
>> +   /* If we didn't manage to parse anything, try the interrupt affinity 
>> */
>> +   if (cpumask_weight(>supported_cpus) == 0) {
>> +   if (!using_spi) {
> 
> However, using_spi is never set to true in the absence of that property,
> causing the wrong branch to be taken...
> 
>> +   /* If using PPIs, check the affinity of the 
>> partition */
>> +   int ret, irq;
>> +
>> +   irq = platform_get_irq(pdev, 0);
>> +   ret = irq_get_percpu_devid_partition(irq, 
>> >supported_cpus);
> 
> ... and ret to become -22 here.

Thanks for the thorough analysis. Could you please give the following
patchlet a go:

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 2513365..9275e08 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -958,11 +958,12 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 
/* If we didn't manage to parse anything, try the interrupt affinity */
if (cpumask_weight(>supported_cpus) == 0) {
-   if (!using_spi) {
+   int irq = platform_get_irq(pdev, 0);
+
+   if (irq_is_percpu(irq)) {
/* If using PPIs, check the affinity of the partition */
-   int ret, irq;
+   int ret;
 
-   irq = platform_get_irq(pdev, 0);
ret = irq_get_percpu_devid_partition(irq, 
>supported_cpus);
if (ret) {
kfree(irqs);


and let me know if that helps?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: dts: r8a7795: Increase the size of GIC-400 mapped registers

[Marc Zyngier]

On 10/05/16 16:29, Dirk Behme wrote:
> On 10.05.2016 16:17, Marc Zyngier wrote:
>> On 10/05/16 14:33, Geert Uytterhoeven wrote:
>>> CC Marc, lakml
>>>
>>> On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme <dirk.be...@de.bosch.com> wrote:
>>>> From: Pooya Keshavarzi <pooya.keshava...@de.bosch.com>
>>>>
>>>> There are some requirements about the GIC-400 memory layout and its
>>>> mapping if using 64k aligned base addresses like on r8a7795.
>>>>
>>>> See e.g.
>>>>
>>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9
>>>>
>>>> Map the whole memory range instead of only 0x2000. This will fix
>>>> the issue that some hypervisors, e.g. Xen, fail to handle the
>>>> interrupts correctly.
>>>>
>>>> Signed-off-by: Pooya Keshavarzi <pooya.keshava...@de.bosch.com>
>>>> Signed-off-by: Dirk Behme <dirk.be...@de.bosch.com>
>>>
>>> Based on my understanding below
>>> Acked-by: Geert Uytterhoeven <geert+rene...@glider.be>
>>>
>>>> ---
>>>> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3
>>>>
>>>>   arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
>>>> b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> index 8be9424..d880fd4 100644
>>>> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> @@ -160,9 +160,9 @@
>>>>  #address-cells = <0>;
>>>>  interrupt-controller;
>>>>  reg = <0x0 0xf101 0 0x1000>,
>>>> - <0x0 0xf102 0 0x2000>,
>>>> + <0x0 0xf102 0 0x2>,
>>>><0x0 0xf104 0 0x2>,
>>>> - <0x0 0xf106 0 0x2000>;
>>>> + <0x0 0xf106 0 0x2>;
>>>>  interrupts = >>>  (GIC_CPU_MASK_SIMPLE(4) | 
>>>> IRQ_TYPE_LEVEL_HIGH)>;
>>>>  };
>>>
>>> Region 0:
>>>  4 KiB-pages 0xf1011000-0xf101 are aliased to 0xf101-0xf1010fff,
>>>  but we need the first 4 KiB only.
>>>
>>> Region 1:
>>>  4 KiB-pages 0xf1021000-0xf102 are aliased to 0xf102-0xf1020fff,
>>>  4 KiB-pages 0xf103-0xf103 are all zeroes, probably due to
>>>  non-secure mode?
>>
>> No. This 4kB page only contain a single register (GICC_DIR), which is
>> WO/RAZ.
>>
>>>
>>> Region 2:
>>>  4 KiB-pages 0xf1041000-0xf104 are aliased to 0xf104-0xf1040fff,
>>>  4 KiB-pages 0xf105-0xf105 are all zeroes, probably due to
>>>  non-secure mode?
>>
>> Neither. The aliases are an unused feature of GIC400 exposing the other
>> CPUs view of the same registers...
>>
>>>
>>> Region 3:
>>>  4 KiB-pages 0xf1061000-0xf106 are aliased to 0xf106-0xf1060fff,
>>>  4 KiB-pages 0xf107-0xf107 are all zeroes, probably due to
>>>  non-secure mode?
>>
>> Same as region 1.
>>
>>>
>>> Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at
>>> 0xf104f000.
>>
>> No. This region (GICH) only needs the first 256 bytes or so. The rest is
>> either RAZ/WI or useless stuff.
>>
>>>
>>> An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this
>>> covered two identical (aliased) 4 KiB pages, instead of two different pages
>>> at offset 0xf000.
>>
>> While we're at it, adding a pointer to the documentation (GIC400 and
>> SBSA) would be tremendously useful, as it'd avoid misinterpreting the
>> various bits.
> 
> 
> If anybody could give a short description I could copy & paste into 
> the commit message that would be quite helpful :)

Well, there is my reply to an earlier email in this very thread, as well
as the xen commit which quotes my original commit (which you could also
refer to).

I'll leave it to you to eradicate the swear words (or to leave them in
place as a testimony of what 4 years of GIC hacking can do to an
otherwise sane person).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: dts: r8a7795: Increase the size of GIC-400 mapped registers

[Marc Zyngier]

On 10/05/16 14:33, Geert Uytterhoeven wrote:
> CC Marc, lakml
> 
> On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme  wrote:
>> From: Pooya Keshavarzi 
>>
>> There are some requirements about the GIC-400 memory layout and its
>> mapping if using 64k aligned base addresses like on r8a7795.
>>
>> See e.g.
>>
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9
>>
>> Map the whole memory range instead of only 0x2000. This will fix
>> the issue that some hypervisors, e.g. Xen, fail to handle the
>> interrupts correctly.
>>
>> Signed-off-by: Pooya Keshavarzi 
>> Signed-off-by: Dirk Behme 
> 
> Based on my understanding below
> Acked-by: Geert Uytterhoeven 
> 
>> ---
>> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3
>>
>>  arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
>> b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> index 8be9424..d880fd4 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> @@ -160,9 +160,9 @@
>> #address-cells = <0>;
>> interrupt-controller;
>> reg = <0x0 0xf101 0 0x1000>,
>> - <0x0 0xf102 0 0x2000>,
>> + <0x0 0xf102 0 0x2>,
>>   <0x0 0xf104 0 0x2>,
>> - <0x0 0xf106 0 0x2000>;
>> + <0x0 0xf106 0 0x2>;
>> interrupts = > (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_HIGH)>;
>> };
> 
> Region 0:
> 4 KiB-pages 0xf1011000-0xf101 are aliased to 0xf101-0xf1010fff,
> but we need the first 4 KiB only.
> 
> Region 1:
> 4 KiB-pages 0xf1021000-0xf102 are aliased to 0xf102-0xf1020fff,
> 4 KiB-pages 0xf103-0xf103 are all zeroes, probably due to
> non-secure mode?

No. This 4kB page only contain a single register (GICC_DIR), which is
WO/RAZ.

> 
> Region 2:
> 4 KiB-pages 0xf1041000-0xf104 are aliased to 0xf104-0xf1040fff,
> 4 KiB-pages 0xf105-0xf105 are all zeroes, probably due to
> non-secure mode?

Neither. The aliases are an unused feature of GIC400 exposing the other
CPUs view of the same registers...

> 
> Region 3:
> 4 KiB-pages 0xf1061000-0xf106 are aliased to 0xf106-0xf1060fff,
> 4 KiB-pages 0xf107-0xf107 are all zeroes, probably due to
> non-secure mode?

Same as region 1.

> 
> Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at
> 0xf104f000.

No. This region (GICH) only needs the first 256 bytes or so. The rest is
either RAZ/WI or useless stuff.

> 
> An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this
> covered two identical (aliased) 4 KiB pages, instead of two different pages
> at offset 0xf000.

While we're at it, adding a pointer to the documentation (GIC400 and
SBSA) would be tremendously useful, as it'd avoid misinterpreting the
various bits.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: dts: r8a7795: Increase the size of GIC-400 mapped registers

[Marc Zyngier]

On Fri, 29 Apr 2016 09:43:45 +1000
Simon Horman  wrote:

> [Cc Mark Zyngier, linux-arm-kernel]
> 
> Hi Dirk,
> 
> On Thu, Apr 28, 2016 at 07:41:57AM +0200, Dirk Behme wrote:
> > Hi Simon,
> > 
> > On 28.04.2016 01:30, Simon Horman wrote:
> > >Hi Dirk,
> > >
> > >I understand that there is an issue here but I'm not yet able
> > >to convince myself that this is the correct solution.
> > >
> > >In revision r0p1 of the CoreLink GIC-400 Generic Interrupt Controller
> > >Technical Reference Manual[1] I see in Section 3.2. "GIC-400 register map"
> > >that the size of both the CPU interfaces and Virtual CPU interfaces are
> > >0x2000 bytes. And assuming that the hardware follows the specification it
> > >appears that DT is correctly describing the hardware.
> > 
> > 
> > I think you are missing the details described by ARM in
> > 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9
> > 
> > 
> > Maybe Julien could help if you have some more doubts?
> 
> I guess I am confused.
> 
> I see that there is now handling of the case where the region size is
> 128Kbytes. But I'm still not seeing the bit which describes that the
> GIC-400  has a region size of 128Kbytes. Perhaps the later is somehow
> implied by the former. Or perhaps I need to check with the hw team.

Please have a look at the SBSA document, and in particular the
Appendix-F (registration and selling your soul required - only kidding):

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029/index.html

This requires that, in order for the two halves of GICV to be trappable
*separately* by a hypervisor using 64kB pages at Stage-2, the two 4kB
pages that describe that region are aliased as such:
- the first 4kB page is aliased 16 times over a 64kB region
- the second 4kB page is aliased 16 times over another contiguous 64kB
  region

This means that your GIC is indeed covering a 128kB region, with the
mapping corresponding to the GICv2 memory map located at offset 0xf000
from the base of that 128kB region. Also, this GICV requirement also
applies to GICC (most likely because the two regions use the same
decoding logic).

The OS must of course be aware of this (see gic_check_eoimode in the
GIC driver). Of course, almost nobody got that right (I only know of
the APM Xgene-1 so far). If you actually did, great!

Also, the ACPI spec fails to recognize this by not providing the length
of the region, meaning that those who got it right with DT are likely
to get it wrong with ACPI, and vice-versa.

It's a wonderful world.

M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH/RFC v2] gpio: rcar: Add Runtime PM handling for interrupts

[Marc Zyngier]

On 11/04/16 17:26, Laurent Pinchart wrote:
> On Friday 19 Feb 2016 11:59:40 Marc Zyngier wrote:
>> On 19/02/16 09:18, Linus Walleij wrote:
>>> Top-quoting so everyone on the new To:-line gets the context.
>>>
>>> I definately need an indication from an irqchip maintainer like tglx or
>>> Marc Z before I merge this. Also, as in reply to the previous letter,
>>> coordinate efforts with Jon Hunters similar problem space.
>>
>> Seems pretty straightforward to me.
>>
>> Acked-by: Marc Zyngier <marc.zyng...@arm.com>
> 
> Too straightforward to be correct :-/
> 
> [6.232681] BUG: sleeping function called from invalid context at 
> /home/laurent/src/iob/renesas/linux/drivers/base/power/runtime.c:955
> [6.244795] in_atomic(): 1, irqs_disabled(): 128, pid: 658, name: udevd
> [6.251429] CPU: 3 PID: 658 Comm: udevd Tainted: P
> 4.6.0-rc3 #756
> [6.258844] Hardware name: Generic R8A7790 (Flattened Device Tree)

[...]

Ah! That will teach me a lesson.

> The .irq_request_resources() handler is called with a spinlock held, it thus
> can't call the synchronous version of the PM runtime functions.

OK, so we're back to square one. Is that just a matter of calling the
non-synchronous version? My hunch is that it is not that simple...

Geert?

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH/RFC v2] gpio: rcar: Add Runtime PM handling for interrupts

[Marc Zyngier]

Hi Linus,

On 19/02/16 09:18, Linus Walleij wrote:
> Top-quoting so everyone on the new To:-line gets the context.
> 
> I definately need an indication from an irqchip maintainer like tglx or Marc Z
> before I merge this. Also, as in reply to the previous letter, coordinate
> efforts with Jon Hunters similar problem space.

Seems pretty straightforward to me.

Acked-by: Marc Zyngier <marc.zyng...@arm.com>

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: ARM GIC DT binding reg block mismatch? (Re: [PATCH v11 1/8] arm64: renesas: r8a7795: Add Renesas R8A7795 SoC support)

[Marc Zyngier]

On 15/02/16 18:53, Dirk Behme wrote:
> On 15.02.2016 11:55, Marc Zyngier wrote:
>> On 15/02/16 10:35, Geert Uytterhoeven wrote:
>>> Hi Marc,
>>>
>>> On Mon, Feb 15, 2016 at 9:45 AM, Marc Zyngier <marc.zyng...@arm.com> wrote:
>>>> On 15/02/16 08:16, Geert Uytterhoeven wrote:
>>>>> On Wed, Dec 9, 2015 at 9:23 AM, Geert Uytterhoeven <ge...@linux-m68k.org> 
>>>>> wrote:
>>>>>> On Tue, Nov 3, 2015 at 3:28 PM, Mark Rutland <mark.rutl...@arm.com> 
>>>>>> wrote:
>>>>>>> On Wed, Oct 21, 2015 at 03:34:39PM +0200, Geert Uytterhoeven wrote:
>>>>>>>> On Thu, Oct 15, 2015 at 12:58 PM, Mark Rutland <mark.rutl...@arm.com> 
>>>>>>>> wrote:
>>>>>>>>>>> +   gic: interrupt-controller@0xf101 {
>>>>>>>>>> + compatible = "arm,gic-400";
>>>>>>>>>> + #interrupt-cells = <3>;
>>>>>>>>>> + #address-cells = <0>;
>>>>>>>>>> + interrupt-controller;
>>>>>>>>>> + reg = <0x0 0xf101 0 0x1000>,
>>>>>>>>>> +   <0x0 0xf102 0 0x2000>;
>>>>>>>>>> + interrupts = >>>>>>>>> + (GIC_CPU_MASK_SIMPLE(1) | 
>>>>>>>>>> IRQ_TYPE_LEVEL_HIGH)>;
>>>>>>>>>> + };
>>>>>>>>>
>>>>>>>>> No GICH and GICV?
>>>>>>>>
>>>>>>>> These seem to be defined in the "arm,gic-v3" DT bindings only, while 
>>>>>>>> this is
>>>>>>>> an "arm,gic-400" (GICD_IIDR 0x0200043b)?
>>>>>>>
>>>>>>> See the "GIC virtualization extensions (VGIC)" section in
>>>>>>> Documentation/devicetree/bindings/arm/gic.txt
>>>>>>
>>>>>> DDI0471B_gic400_r0p1_trm.pdf says:
>>>>>>
>>>>>>  Address range GIC-400 functional block
>>>>>>  A. 0x - 0x0FFF Reserved
>>>>>>  B. 0x1000 - 0x1FFF Distributor
>>>>>>  C. 0x2000 - 0x3FFF CPU interfaces
>>>>>>  D. 0x4000 - 0x4FFF Virtual interface control block, for the 
>>>>>> processor that
>>>>>> is performing the access
>>>>>>  E. 0x5000 - 0x5FFF Virtual interface control block, for the 
>>>>>> processor
>>>>>> selected by address bits [11:9]
>>>>>>  F. 0x6000 - 0x7FFF Virtual CPU interfaces
>>>>>>
>>>>>> The DT binding document says:
>>>>>>1. The  first region is the GIC distributor register base and size.
>>>>>>2. The 2nd region is the GIC cpu interface register base and size.
>>>>>>3. The first additional region is the GIC virtual interface control 
>>>>>> register
>>>>>>   base and size.
>>>>>>4. The 2nd additional region is the GIC virtual cpu interface 
>>>>>> register base
>>>>>>   and size.
>>>>>>
>>>>>> Matching with the example:
>>>>>>
>>>>>>  interrupt-controller@2c001000 {
>>>>>>  compatible = "arm,cortex-a15-gic";
>>>>>>  #interrupt-cells = <3>;
>>>>>>  interrupt-controller;
>>>>>>  reg = <0x2c001000 0x1000>,
>>>>>><0x2c002000 0x1000>,
>>>>>><0x2c004000 0x2000>,
>>>>>><0x2c006000 0x2000>;
>>>>>>  interrupts = <1 9 0xf04>;
>>>>>>  };
>>>>>>
>>>>>> This means:
>>>>>>- reg entry 1. covers address range B,
>>>>>>- reg entry 2. covers address range C,
>>>>>>- reg entry 3. covers address ranges D _and_ E,
>>>>>>- reg entry 4. covers address range F.
>>>>

Re: ARM GIC DT binding reg block mismatch? (Re: [PATCH v11 1/8] arm64: renesas: r8a7795: Add Renesas R8A7795 SoC support)

[Marc Zyngier]

On 15/02/16 10:35, Geert Uytterhoeven wrote:
> Hi Marc,
> 
> On Mon, Feb 15, 2016 at 9:45 AM, Marc Zyngier <marc.zyng...@arm.com> wrote:
>> On 15/02/16 08:16, Geert Uytterhoeven wrote:
>>> On Wed, Dec 9, 2015 at 9:23 AM, Geert Uytterhoeven <ge...@linux-m68k.org> 
>>> wrote:
>>>> On Tue, Nov 3, 2015 at 3:28 PM, Mark Rutland <mark.rutl...@arm.com> wrote:
>>>>> On Wed, Oct 21, 2015 at 03:34:39PM +0200, Geert Uytterhoeven wrote:
>>>>>> On Thu, Oct 15, 2015 at 12:58 PM, Mark Rutland <mark.rutl...@arm.com> 
>>>>>> wrote:
>>>>>>>>> +   gic: interrupt-controller@0xf101 {
>>>>>>>> + compatible = "arm,gic-400";
>>>>>>>> + #interrupt-cells = <3>;
>>>>>>>> + #address-cells = <0>;
>>>>>>>> + interrupt-controller;
>>>>>>>> + reg = <0x0 0xf101 0 0x1000>,
>>>>>>>> +   <0x0 0xf102 0 0x2000>;
>>>>>>>> + interrupts = >>>>>>> + (GIC_CPU_MASK_SIMPLE(1) | 
>>>>>>>> IRQ_TYPE_LEVEL_HIGH)>;
>>>>>>>> + };
>>>>>>>
>>>>>>> No GICH and GICV?
>>>>>>
>>>>>> These seem to be defined in the "arm,gic-v3" DT bindings only, while 
>>>>>> this is
>>>>>> an "arm,gic-400" (GICD_IIDR 0x0200043b)?
>>>>>
>>>>> See the "GIC virtualization extensions (VGIC)" section in
>>>>> Documentation/devicetree/bindings/arm/gic.txt
>>>>
>>>> DDI0471B_gic400_r0p1_trm.pdf says:
>>>>
>>>> Address range GIC-400 functional block
>>>> A. 0x - 0x0FFF Reserved
>>>> B. 0x1000 - 0x1FFF Distributor
>>>> C. 0x2000 - 0x3FFF CPU interfaces
>>>> D. 0x4000 - 0x4FFF Virtual interface control block, for the processor 
>>>> that
>>>>is performing the access
>>>> E. 0x5000 - 0x5FFF Virtual interface control block, for the processor
>>>>selected by address bits [11:9]
>>>> F. 0x6000 - 0x7FFF Virtual CPU interfaces
>>>>
>>>> The DT binding document says:
>>>>   1. The  first region is the GIC distributor register base and size.
>>>>   2. The 2nd region is the GIC cpu interface register base and size.
>>>>   3. The first additional region is the GIC virtual interface control 
>>>> register
>>>>  base and size.
>>>>   4. The 2nd additional region is the GIC virtual cpu interface register 
>>>> base
>>>>  and size.
>>>>
>>>> Matching with the example:
>>>>
>>>> interrupt-controller@2c001000 {
>>>> compatible = "arm,cortex-a15-gic";
>>>> #interrupt-cells = <3>;
>>>> interrupt-controller;
>>>> reg = <0x2c001000 0x1000>,
>>>>   <0x2c002000 0x1000>,
>>>>   <0x2c004000 0x2000>,
>>>>   <0x2c006000 0x2000>;
>>>> interrupts = <1 9 0xf04>;
>>>> };
>>>>
>>>> This means:
>>>>   - reg entry 1. covers address range B,
>>>>   - reg entry 2. covers address range C,
>>>>   - reg entry 3. covers address ranges D _and_ E,
>>>>   - reg entry 4. covers address range F.
>>>>
>>>> On R-Car Gen3, the base addresses are:
>>>>
>>>> Distributor : 0xF101_
>>>> CPU interfaces  : 0xF102_
>>>> Virtual interfaces  : 0xF104_
>>>> Virtual interfaces  : 0xF105_
>>>> Virtual CPU interfaces  : 0xF106_
>>>>
>>>> Note the additional multiplication factor of 16 in the offsets relative to
>>>> the base address 0xf100 (e.g. 0x5 instead of 0x5000).
>>>>
>>>> As address ranges D and E are merged in a single reg entry, how is the GIC
>>>> driver supposed to know about this multiplication factor?
>>
>> The answer is very simple, the GIC driver doesn't give a damn about the
>> second part of the GICH region, because it is absolutely unusable for
>> any realistic use-case. Only the banked version of GICH is of any
>> relevance (the first 512 bytes, in essence).
>>
>> Aligning the GIC regions on 64kB boundaries is documented in the SBSA
>> specification, independently of the GIC400 documentation.
> 
> If I understand the SBSA spec correctly (BTW, arm,gic.txt doesn't use the
> "GICC" terminology, unlike arm,gic-v3.txt), that means reg entry 3 should be
> "<0xf104f000 0x2000>", so it covers the aliased last 4 KiB of address range D,
> and the first 4 KiB of address range E. I.e.
> 
> reg = <0x0 0xf101 0 0x1000>,
>  <0x0 0xf102 0 0x2000>,
>  <0x0 0xf104f000 0 0x2000>,
>  <0x0 0xf106 0 0x2000>;
> 
> Is that correct?

My preference would be to expose the full 128kB of the region - assuming
your HW actually supports the SBSA aliasing.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: ARM GIC DT binding reg block mismatch? (Re: [PATCH v11 1/8] arm64: renesas: r8a7795: Add Renesas R8A7795 SoC support)

[Marc Zyngier]

On 15/02/16 08:16, Geert Uytterhoeven wrote:
> Cc Marz Zyngier
> Cc Dirk Behme
> Cc devicetree
> Cc linux-renesas-soc
> Drop linux-sh
> 
> On Wed, Dec 9, 2015 at 9:23 AM, Geert Uytterhoeven  
> wrote:
>> On Tue, Nov 3, 2015 at 3:28 PM, Mark Rutland  wrote:
>>> On Wed, Oct 21, 2015 at 03:34:39PM +0200, Geert Uytterhoeven wrote:
 On Thu, Oct 15, 2015 at 12:58 PM, Mark Rutland  
 wrote:
>>> +   gic: interrupt-controller@0xf101 {
>> + compatible = "arm,gic-400";
>> + #interrupt-cells = <3>;
>> + #address-cells = <0>;
>> + interrupt-controller;
>> + reg = <0x0 0xf101 0 0x1000>,
>> +   <0x0 0xf102 0 0x2000>;
>> + interrupts = > + (GIC_CPU_MASK_SIMPLE(1) | 
>> IRQ_TYPE_LEVEL_HIGH)>;
>> + };
>
> No GICH and GICV?

 These seem to be defined in the "arm,gic-v3" DT bindings only, while this 
 is
 an "arm,gic-400" (GICD_IIDR 0x0200043b)?
>>>
>>> See the "GIC virtualization extensions (VGIC)" section in
>>> Documentation/devicetree/bindings/arm/gic.txt
>>
>> DDI0471B_gic400_r0p1_trm.pdf says:
>>
>> Address range GIC-400 functional block
>> A. 0x - 0x0FFF Reserved
>> B. 0x1000 - 0x1FFF Distributor
>> C. 0x2000 - 0x3FFF CPU interfaces
>> D. 0x4000 - 0x4FFF Virtual interface control block, for the processor 
>> that
>>is performing the access
>> E. 0x5000 - 0x5FFF Virtual interface control block, for the processor
>>selected by address bits [11:9]
>> F. 0x6000 - 0x7FFF Virtual CPU interfaces
>>
>> The DT binding document says:
>>   1. The  first region is the GIC distributor register base and size.
>>   2. The 2nd region is the GIC cpu interface register base and size.
>>   3. The first additional region is the GIC virtual interface control 
>> register
>>  base and size.
>>   4. The 2nd additional region is the GIC virtual cpu interface register base
>>  and size.
>>
>> Matching with the example:
>>
>> interrupt-controller@2c001000 {
>> compatible = "arm,cortex-a15-gic";
>> #interrupt-cells = <3>;
>> interrupt-controller;
>> reg = <0x2c001000 0x1000>,
>>   <0x2c002000 0x1000>,
>>   <0x2c004000 0x2000>,
>>   <0x2c006000 0x2000>;
>> interrupts = <1 9 0xf04>;
>> };
>>
>> This means:
>>   - reg entry 1. covers address range B,
>>   - reg entry 2. covers address range C,
>>   - reg entry 3. covers address ranges D _and_ E,
>>   - reg entry 4. covers address range F.
>>
>> On R-Car Gen3, the base addresses are:
>>
>> Distributor : 0xF101_
>> CPU interfaces  : 0xF102_
>> Virtual interfaces  : 0xF104_
>> Virtual interfaces  : 0xF105_
>> Virtual CPU interfaces  : 0xF106_
>>
>> Note the additional multiplication factor of 16 in the offsets relative to
>> the base address 0xf100 (e.g. 0x5 instead of 0x5000).
>>
>> As address ranges D and E are merged in a single reg entry, how is the GIC
>> driver supposed to know about this multiplication factor?

The answer is very simple, the GIC driver doesn't give a damn about the
second part of the GICH region, because it is absolutely unusable for
any realistic use-case. Only the banked version of GICH is of any
relevance (the first 512 bytes, in essence).

Aligning the GIC regions on 64kB boundaries is documented in the SBSA
specification, independently of the GIC400 documentation.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...