irqchip/bcm-6345-l1: show MMIO address

PATCH In review Álvaro Fernández Rojas 4 participants 9 messages Mar 16, 2023 – Apr 8, 2023

Subsystems (3)

MIPS irqchip drivers Broadcom bmips MIPS architecture

lore

Share & tools

lore

Álvaro Fernández Rojas

author

2023-03-16 18:07 UTC #0 lore raw

PATCH 1/1 irqchip/bcm-6345-l1: show MMIO address

It's safe to show MMIO address.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
 drivers/irqchip/irq-bcm6345-l1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-bcm6345-l1.c +1 -1

index 6899e37810a8..55a2d9b31597 100644
@@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
 	for_each_cpu(idx, &intc->cpumask) {
 		struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
 
-		pr_info("  CPU%u at MMIO 0x%p (irq = %d)\n", idx,
+		pr_info("  CPU%u at MMIO 0x%px (irq = %d)\n", idx,
 				cpu->map_base, cpu->parent_irq);
 	}

Signature · 1 line

2.30.2

Florian Fainelli

2023-03-16 18:13 UTC #1 lore raw

Quoted 3 lines · Álvaro Fernández Rojas

It's safe to show MMIO address.
Signed-off-by Álvaro Fernández Rojas <[email protected]>

This is going to be the kernel virtual address, and while on MIPS it is easy to resolve to the physical address because these platforms map registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel virtual addresses are pretty meaningless unless what you want to debug is how ioremap() mapped the address.

I would rather do the following change:

drivers/irqchip/irq-bcm6345-l1.c +2 -0

index 1bd0621c4ce2..832957d363a4 100644
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct device_node *dn,
        if (!cpu->map_base)
                return -ENOMEM;
 
+       request_mem_region(res.start, sz, res.name);
+
        for (i = 0; i < n_words; i++) {
                cpu->enable_cache[i] = 0;
                __raw_writel(0, cpu->map_base + reg_enable(intc, i));

such that this shows up in /proc/iomem. WDYT?

Quoted 17 lines

---
  drivers/irqchip/irq-bcm6345-l1.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-bcm6345-l1.c +1 -1

index 6899e37810a8..55a2d9b31597 100644
@@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
  	for_each_cpu(idx, &intc->cpumask) {
  		struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
 
-		pr_info("  CPU%u at MMIO 0x%p (irq = %d)\n", idx,
+		pr_info("  CPU%u at MMIO 0x%px (irq = %d)\n", idx,
  				cpu->map_base, cpu->parent_irq);
  	}

Signature · 1 line

Florian

Marc Zyngier

2023-03-16 18:25 UTC #2 lore raw

Quoted 28 lines · Florian Fainelli

Quoted 3 lines · Álvaro Fernández Rojas
It's safe to show MMIO address.
Signed-off-by Álvaro Fernández Rojas <[email protected]>
This is going to be the kernel virtual address, and while on MIPS it is easy to resolve to the physical address because these platforms map registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel virtual addresses are pretty meaningless unless what you want to debug is how ioremap() mapped the address.
I would rather do the following change:
drivers/irqchip/irq-bcm6345-l1.c +2 -0
index 1bd0621c4ce2..832957d363a4 100644
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
 device_node *dn,
        if (!cpu->map_base)
                return -ENOMEM;
 
+       request_mem_region(res.start, sz, res.name);
+
        for (i = 0; i < n_words; i++) {
                cpu->enable_cache[i] = 0;
                __raw_writel(0, cpu->map_base + reg_enable(intc, i));

Quoted 1 line

such that this shows up in /proc/iomem. WDYT?

That's certainly much more useful in general.

Also, the current pr_info() is probably pretty useless, given that the OP was trying to circumvent the obfuscation. Either printing the PA or removing the message altogether would be good.

Thanks,

M.

Signature · 1 line

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

Álvaro Fernández Rojas

author

2023-03-16 19:05 UTC #3 lore raw

In reply to Florian Fainelli

Quoted 30 lines · Florian Fainelli

Quoted 3 lines · Álvaro Fernández Rojas
It's safe to show MMIO address.
Signed-off-by Álvaro Fernández Rojas <[email protected]>
This is going to be the kernel virtual address, and while on MIPS it is easy to resolve to the physical address because these platforms map registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel virtual addresses are pretty meaningless unless what you want to debug is how ioremap() mapped the address.
I would rather do the following change:
drivers/irqchip/irq-bcm6345-l1.c +2 -0
index 1bd0621c4ce2..832957d363a4 100644
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
 device_node *dn,
         if (!cpu->map_base)
                 return -ENOMEM;
 
+       request_mem_region(res.start, sz, res.name);
+
         for (i = 0; i < n_words; i++) {
                 cpu->enable_cache[i] = 0;
                 __raw_writel(0, cpu->map_base + reg_enable(intc, i));

Quoted 1 line

such that this shows up in /proc/iomem. WDYT?

I tried doing it that way, but it still shows (ptrval):

[    0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
[    0.000000] irq_bcm6345_l1:   CPU0 at MMIO 0x(ptrval) (irq = 2)

I checked /proc/iomem and it's shown:

root@OpenWrt:/# cat /proc/iomem
00000000-03ffffff : System RAM
  00010000-0068e96f : Kernel code
  0068e970-008834ff : Kernel data
  01610000-016458e7 : Kernel bss
08000000-0800ffff : BCM6348 PCI IO space
1e000000-1fffffff : 1e000000.nor nor@1e000000
30000000-37ffffff : pci@fffe1000
  30000000-3000ffff : 0000:00:01.0
    30000000-3000ffff : ath9k
fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
fffe000c-fffe0013 : interrupt-controller@fffe000c
fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
fffe1000-fffe11ff : fffe1000.pci pci
fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500

Any idea why this could be hapenning?

Quoted 13 lines

---
  drivers/irqchip/irq-bcm6345-l1.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-bcm6345-l1.c +0 -0

index 6899e37810a8..55a2d9b31597 100644
@@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
      for_each_cpu(idx, &intc->cpumask) {
              struct bcm6345_l1_cpu *cpu = intc->cpus[idx];

Quoted 9 lines

drivers/irqchip/irq-bcm6345-l1.c static int __init bcm6345_l1_of_init(struct device_node *dn, · line 341
Quoted 5 lines
drivers/irqchip/irq-bcm6345-l1.c +1 -1
-             pr_info("  CPU%u at MMIO 0x%p (irq = %d)\n", idx,
+             pr_info("  CPU%u at MMIO 0x%px (irq = %d)\n", idx,
                              cpu->map_base, cpu->parent_irq);
      }
 
--
Florian

Álvaro

Florian Fainelli

2023-03-16 19:10 UTC #4 lore raw

Quoted 32 lines · Álvaro Fernández Rojas

Quoted 30 lines · Florian Fainelli
Quoted 3 lines · Álvaro Fernández Rojas
It's safe to show MMIO address.
Signed-off-by Álvaro Fernández Rojas <[email protected]>
This is going to be the kernel virtual address, and while on MIPS it is easy to resolve to the physical address because these platforms map registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel virtual addresses are pretty meaningless unless what you want to debug is how ioremap() mapped the address.
I would rather do the following change:
drivers/irqchip/irq-bcm6345-l1.c +2 -0
index 1bd0621c4ce2..832957d363a4 100644
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
 device_node *dn,
          if (!cpu->map_base)
                  return -ENOMEM;
 
+       request_mem_region(res.start, sz, res.name);
+
          for (i = 0; i < n_words; i++) {
                  cpu->enable_cache[i] = 0;
                  __raw_writel(0, cpu->map_base + reg_enable(intc, i));

Quoted 5 lines

Quoted 1 line
such that this shows up in /proc/iomem. WDYT?
I tried doing it that way, but it still shows (ptrval):
[    0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
[    0.000000] irq_bcm6345_l1:   CPU0 at MMIO 0x(ptrval) (irq = 2)

Well yes, if you don't remove the pr_info() you are still going to be printing it, and because map_base is the return of ioremap() which is a kernel virtual address, it is still hashed, also see Marc's message that came in. I guess I should have been way more explicit and also provide a tentative patch that also took out the pr_info().

Quoted 22 lines

I checked /proc/iomem and it's shown:

root@OpenWrt:/# cat /proc/iomem
00000000-03ffffff : System RAM
   00010000-0068e96f : Kernel code
   0068e970-008834ff : Kernel data
   01610000-016458e7 : Kernel bss
08000000-0800ffff : BCM6348 PCI IO space
1e000000-1fffffff : 1e000000.nor nor@1e000000
30000000-37ffffff : pci@fffe1000
   30000000-3000ffff : 0000:00:01.0
     30000000-3000ffff : ath9k
fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
fffe000c-fffe0013 : interrupt-controller@fffe000c
fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
fffe1000-fffe11ff : fffe1000.pci pci
fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500

Quoted 1 line

Any idea why this could be hapenning?

We now have the desired resource listed using its physical address:

fffe000c-fffe0013 : interrupt-controller@fffe000c

There could be a variety of improvements to how request_mem_region() is called if you want to provide a break down of each resource on a per-CPU basis.

Signature · 1 line

Florian

Álvaro Fernández Rojas

author

2023-03-16 19:34 UTC #5 lore raw

Quoted 40 lines · Florian Fainelli

Quoted 32 lines · Álvaro Fernández Rojas
Quoted 30 lines · Florian Fainelli
Quoted 3 lines · Álvaro Fernández Rojas
It's safe to show MMIO address.
Signed-off-by Álvaro Fernández Rojas <[email protected]>
This is going to be the kernel virtual address, and while on MIPS it is easy to resolve to the physical address because these platforms map registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel virtual addresses are pretty meaningless unless what you want to debug is how ioremap() mapped the address.
I would rather do the following change:
drivers/irqchip/irq-bcm6345-l1.c +2 -0
index 1bd0621c4ce2..832957d363a4 100644
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
 device_node *dn,
          if (!cpu->map_base)
                  return -ENOMEM;
 
+       request_mem_region(res.start, sz, res.name);
+
          for (i = 0; i < n_words; i++) {
                  cpu->enable_cache[i] = 0;
                  __raw_writel(0, cpu->map_base + reg_enable(intc, i));
Quoted 5 lines
Quoted 1 line
such that this shows up in /proc/iomem. WDYT?
I tried doing it that way, but it still shows (ptrval):
[    0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
[    0.000000] irq_bcm6345_l1:   CPU0 at MMIO 0x(ptrval) (irq = 2)

Quoted 5 lines

Well yes, if you don't remove the pr_info() you are still going to be printing it, and because map_base is the return of ioremap() which is a kernel virtual address, it is still hashed, also see Marc's message that came in. I guess I should have been way more explicit and also provide a tentative patch that also took out the pr_info().

Ah, sorry for that, I didn't get your point...
However, I'd rather keep the pr_info, so I just removed the MMIO address in v2.

Quoted 27 lines

Quoted 23 lines

I checked /proc/iomem and it's shown:

root@OpenWrt:/# cat /proc/iomem
00000000-03ffffff : System RAM
   00010000-0068e96f : Kernel code
   0068e970-008834ff : Kernel data
   01610000-016458e7 : Kernel bss
08000000-0800ffff : BCM6348 PCI IO space
1e000000-1fffffff : 1e000000.nor nor@1e000000
30000000-37ffffff : pci@fffe1000
   30000000-3000ffff : 0000:00:01.0
     30000000-3000ffff : ath9k
fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
fffe000c-fffe0013 : interrupt-controller@fffe000c
fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
fffe1000-fffe11ff : fffe1000.pci pci
fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500

Quoted 1 line

Any idea why this could be hapenning?

We now have the desired resource listed using its physical address:

Quoted 8 lines

fffe000c-fffe0013 : interrupt-controller@fffe000c
There could be a variety of improvements to how request_mem_region() is called if you want to provide a break down of each resource on a per-CPU basis.
--
Florian

Álvaro

Álvaro Fernández Rojas

author

2023-03-16 19:28 UTC #6 lore raw

PATCH v2 1/1 irqchip/bcm-6345-l1: request memory region

In reply to Álvaro Fernández Rojas

Request memory region in order to display it in /proc/iomem. Also stop printing the MMIO address since it just displays (ptrval).

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: request memory region and stop displaying MMIO address.

 drivers/irqchip/irq-bcm6345-l1.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/irqchip/irq-bcm6345-l1.c +4 -2

index 6899e37810a8..fa113cb2529a 100644
@@ -257,6 +257,9 @@ static int __init bcm6345_l1_init_one(struct device_node *dn,
 	if (!cpu->map_base)
 		return -ENOMEM;
 
+	if (!request_mem_region(res.start, sz, res.name))
+		pr_err("failed to request intc memory");
+
 	for (i = 0; i < n_words; i++) {
 		cpu->enable_cache[i] = 0;
 		__raw_writel(0, cpu->map_base + reg_enable(intc, i));

@@ -335,8 +338,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
 	for_each_cpu(idx, &intc->cpumask) {
 		struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
 
-		pr_info("  CPU%u at MMIO 0x%p (irq = %d)\n", idx,
-				cpu->map_base, cpu->parent_irq);
+		pr_info("  CPU%u (irq = %d)\n", idx, cpu->parent_irq);
 	}
 
 	return 0;

Signature · 1 line

2.30.2

Florian Fainelli

Acked-by on v2 1/1

2023-03-16 19:40 UTC #7 lore raw

Re: [PATCH v2] irqchip/bcm-6345-l1: request memory region

Quoted 4 lines · Álvaro Fernández Rojas

Request memory region in order to display it in /proc/iomem. Also stop printing the MMIO address since it just displays (ptrval).
Signed-off-by Álvaro Fernández Rojas <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Thanks!

Signature · 1 line

Florian

irqchip-bot for Álvaro Fernández Rojas

merge bot Applied

2023-04-08 10:50 UTC #8 lore raw

[irqchip: irq/irqchip-next] irqchip/bcm-6345-l1: Request memory region

In reply to Álvaro Fernández Rojas

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     23c7ff129cf33dee5f1f4fd9fa729ab440e8f1c5
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/23c7ff129cf33dee5f1f4fd9fa729ab440e8f1c5
Author:        Álvaro Fernández Rojas <[email protected]>
AuthorDate:    Thu, 16 Mar 2023 20:28:33 +01:00
Committer:     Marc Zyngier <[email protected]>
CommitterDate: Sat, 08 Apr 2023 11:07:43 +01:00

irqchip/bcm-6345-l1: Request memory region

Request memory region in order to display it in /proc/iomem. Also stop printing the MMIO address since it just displays (ptrval).

Signed-off-by Álvaro Fernández Rojas <[email protected]>

Acked-by Florian Fainelli <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/irqchip/irq-bcm6345-l1.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/irqchip/irq-bcm6345-l1.c +4 -2

index 6899e37..fa113cb 100644
@@ -257,6 +257,9 @@ static int __init bcm6345_l1_init_one(struct device_node *dn,
 	if (!cpu->map_base)
 		return -ENOMEM;
 
+	if (!request_mem_region(res.start, sz, res.name))
+		pr_err("failed to request intc memory");
+
 	for (i = 0; i < n_words; i++) {
 		cpu->enable_cache[i] = 0;
 		__raw_writel(0, cpu->map_base + reg_enable(intc, i));

@@ -335,8 +338,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
 	for_each_cpu(idx, &intc->cpumask) {
 		struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
 
-		pr_info("  CPU%u at MMIO 0x%p (irq = %d)\n", idx,
-				cpu->map_base, cpu->parent_irq);
+		pr_info("  CPU%u (irq = %d)\n", idx, cpu->parent_irq);
 	}
 
 	return 0;