RE: [PATCH] irqchip: Enable compile-testing for Renesas drivers

[Chris Brandt]

On Fri, Jun 07, 2019, Geert Uytterhoeven wrote:
>  config RENESAS_RZA1_IRQC
> - bool
> + bool "Renesas RZ/A1 IRQC support" if COMPILE_TEST
>   select IRQ_DOMAIN_HIERARCHY
> + help
> +   Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
> +   to 8 external interrupts with configurable sense select.


Appreciated−by: Chris Brandt 

:)

RE: [PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Chris Brandt]

Hi Geert,

Thanks for this patch!

I've been hacking this support into the standard GIC driver in our BSPs 
for years now. :o

On Mon, Apr 29, 2019, Geert Uytterhoeven wrote:
> I expect this driver to be reusable for RZ/A2, after adding a match
> entry with .gic_spi_base = 4.

Yes, the same IP block is in RZ/A2.

So with that said, should we call this driver irq-renesas-rza1.c or just
irq-renesas-rza.c?
It doesn't really matter to me.
For an RZ/A3, we might just use the same IP again.

Side note, I've seen this interrupt pin HW in some older SH4A devices 
(like SH7724 and SH7757). So it's been around for a while.


Chris

RE: [PATCH v5 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

[Chris Brandt]

On Wednesday, January 09, 2019, Sergei Shtylyov wrote:
> > IIRC, this hardware block is also used on RZ/A, which is 32-bit.
> 
>   I'm not seeing it in the "RZ/A1H Group, RZ/A1M Group User’s Manual:
> Hardware"
> Rev 3.00. What SoC did you have in mind?

For the RZ/A series (and RZ/T series), it is called the
"SPI Multi I/O Bus Controller" (Chapter 17)

I have no idea why it has a different name for the same hardware (and
the same pages in the manual).

The HW version in RZ/A1 does not have HyperRAM.

But the HW version in RZ/A2 is the same as what is in R-Car Gen3.

Chris

RE: [PATCH 0/2] serial: sh-sci: Fix earlycon on Renesas ARM platforms

[Chris Brandt]

Hi Geert,

On Thursday, August 30, 2018, Geert Uytterhoeven wrote:
> Earlycon for RZ/A2 (which is a new platform) will be fixed later in a
> separate patch.

I assume something like this:
(which works for me)


diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ab3f6e91853d..426241da2e44 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3414,6 +3414,12 @@ static int __init scif_early_console_setup(struct 
earlycon_device *device,
 {
return early_console_setup(device, PORT_SCIF);
 }
+static int __init rzscifa_early_console_setup(struct earlycon_device *device,
+ const char *opt)
+{
+   port_cfg.regtype = SCIx_RZ_SCIFA_REGTYPE;
+   return early_console_setup(device, PORT_SCIF);
+}
 static int __init scifa_early_console_setup(struct earlycon_device *device,
  const char *opt)
 {
@@ -3432,6 +3438,7 @@ static int __init hscif_early_console_setup(struct 
earlycon_device *device,
 
 OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
 OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
+OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
 OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);


Chris

RE: [PATCH 0/2] serial: sh-sci: Fix earlycon on Renesas ARM platforms

[Chris Brandt]

Hi Geert,

On Thursday, August 30, 2018, Geert Uytterhoeven wrote:
> Earlycon for RZ/A2 (which is a new platform) will be fixed later in a
> separate patch.

I assume something like this:
(which works for me)


diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ab3f6e91853d..426241da2e44 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3414,6 +3414,12 @@ static int __init scif_early_console_setup(struct 
earlycon_device *device,
 {
return early_console_setup(device, PORT_SCIF);
 }
+static int __init rzscifa_early_console_setup(struct earlycon_device *device,
+ const char *opt)
+{
+   port_cfg.regtype = SCIx_RZ_SCIFA_REGTYPE;
+   return early_console_setup(device, PORT_SCIF);
+}
 static int __init scifa_early_console_setup(struct earlycon_device *device,
  const char *opt)
 {
@@ -3432,6 +3438,7 @@ static int __init hscif_early_console_setup(struct 
earlycon_device *device,
 
 OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
 OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
+OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
 OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);


Chris

RE: [PATCH] [RFC] arm: Replace "multiple platforms" by "common platform"

[Chris Brandt]

On Friday, June 22, 2018, Russell King - ARM Linux wrote:
> We've worked around the issues with "multi-platform" XIP/NOMMU by
> using things such as "ARM_SINGLE_V7M" to cover all V7M platforms
> (which must, by definition) have compatible physical layouts.
> Exactly the same approach should be adopted for other XIP/NOMMU
> platforms, and _not_ reusing ARCH_MULTIPLATFORM, which will lead
> to lots of non-bootable kernels.


FYI, I already submitted at patch like that:

https://patchwork.kernel.org/patch/9563975/


Chris

RE: [PATCH] [RFC] arm: Replace "multiple platforms" by "common platform"

[Chris Brandt]

On Friday, June 22, 2018, Russell King - ARM Linux wrote:
> We've worked around the issues with "multi-platform" XIP/NOMMU by
> using things such as "ARM_SINGLE_V7M" to cover all V7M platforms
> (which must, by definition) have compatible physical layouts.
> Exactly the same approach should be adopted for other XIP/NOMMU
> platforms, and _not_ reusing ARCH_MULTIPLATFORM, which will lead
> to lots of non-bootable kernels.


FYI, I already submitted at patch like that:

https://patchwork.kernel.org/patch/9563975/


Chris

RE: [PATCH v6 1/4] cramfs: direct memory access support

[Chris Brandt]

On Thursday, October 12, 2017, Nicolas Pitre wrote:
> Small embedded systems typically execute the kernel code in place (XIP)
> directly from flash to save on precious RAM usage. This adds the ability
> to consume filesystem data directly from flash to the cramfs filesystem
> as well. Cramfs is particularly well suited to this feature as it is
> very simple and its RAM usage is already very low, and with this feature
> it is possible to use it with no block device support and even lower RAM
> usage.
> 
> This patch was inspired by a similar patch from Shane Nay dated 17 years
> ago that used to be very popular in embedded circles but never made it
> into mainline. This is a cleaned-up implementation that uses far fewer
> ifdef's and gets the actual memory location for the filesystem image
> via MTD at run time. In the context of small IoT deployments, this
> functionality has become relevant and useful again.
> 
> Signed-off-by: Nicolas Pitre <n...@linaro.org>
> ---
>  fs/cramfs/Kconfig |  30 +++-
>  fs/cramfs/inode.c | 215 +++--
> -

Works!

I first applied the MTD patch series from here:

http://patchwork.ozlabs.org/project/linux-mtd/list/?series=7504

Then this v6 patch series on top of it.

I created a mtd-rom/direct-mapped partition and was able to both mount after 
boot, and also boot as the rootfs.

Log from booting as rootfs:

[1.586625] cramfs: checking physical address 0x1b00 for linear cramfs 
image
[1.594512] cramfs: linear cramfs image on mtd:rootfs_xipcramfs appears to 
be 15744 KB in size
[1.603619] VFS: Mounted root (cramfs filesystem) readonly on device 31:1.


$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 1f:01 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 1f:01 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e07000-b6ee r-xp  1f:01 766540 /lib/libc-2.18-2013.10.so
b6ee-b6ee8000 ---p 000d9000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6ee8000-b6eea000 r--p 000d9000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6eea000-b6eeb000 rw-p 000db000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6eeb000-b6eee000 rw-p  00:00 0
b6eee000-b6f05000 r-xp  1f:01 670372 /lib/ld-2.18-2013.10.so
b6f08000-b6f09000 rw-p  00:00 0
b6f0a000-b6f0c000 rw-p  00:00 0
b6f0c000-b6f0d000 r--p 00016000 1f:01 670372 /lib/ld-2.18-2013.10.so
b6f0d000-b6f0e000 rw-p 00017000 1f:01 670372 /lib/ld-2.18-2013.10.so
bedb-bedd1000 rw-p  00:00 0  [stack]
bedf4000-bedf5000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0      [vectors]

So far, so good.

Thank you!


Tested-by: Chris Brandt <chris.bra...@renesas.com>

RE: [PATCH v6 1/4] cramfs: direct memory access support

[Chris Brandt]

On Thursday, October 12, 2017, Nicolas Pitre wrote:
> Small embedded systems typically execute the kernel code in place (XIP)
> directly from flash to save on precious RAM usage. This adds the ability
> to consume filesystem data directly from flash to the cramfs filesystem
> as well. Cramfs is particularly well suited to this feature as it is
> very simple and its RAM usage is already very low, and with this feature
> it is possible to use it with no block device support and even lower RAM
> usage.
> 
> This patch was inspired by a similar patch from Shane Nay dated 17 years
> ago that used to be very popular in embedded circles but never made it
> into mainline. This is a cleaned-up implementation that uses far fewer
> ifdef's and gets the actual memory location for the filesystem image
> via MTD at run time. In the context of small IoT deployments, this
> functionality has become relevant and useful again.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/Kconfig |  30 +++-
>  fs/cramfs/inode.c | 215 +++--
> -

Works!

I first applied the MTD patch series from here:

http://patchwork.ozlabs.org/project/linux-mtd/list/?series=7504

Then this v6 patch series on top of it.

I created a mtd-rom/direct-mapped partition and was able to both mount after 
boot, and also boot as the rootfs.

Log from booting as rootfs:

[1.586625] cramfs: checking physical address 0x1b00 for linear cramfs 
image
[1.594512] cramfs: linear cramfs image on mtd:rootfs_xipcramfs appears to 
be 15744 KB in size
[1.603619] VFS: Mounted root (cramfs filesystem) readonly on device 31:1.


$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 1f:01 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 1f:01 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e07000-b6ee r-xp  1f:01 766540 /lib/libc-2.18-2013.10.so
b6ee-b6ee8000 ---p 000d9000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6ee8000-b6eea000 r--p 000d9000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6eea000-b6eeb000 rw-p 000db000 1f:01 766540 /lib/libc-2.18-2013.10.so
b6eeb000-b6eee000 rw-p  00:00 0
b6eee000-b6f05000 r-xp  1f:01 670372 /lib/ld-2.18-2013.10.so
b6f08000-b6f09000 rw-p  00:00 0
b6f0a000-b6f0c000 rw-p  00:00 0
b6f0c000-b6f0d000 r--p 00016000 1f:01 670372 /lib/ld-2.18-2013.10.so
b6f0d000-b6f0e000 rw-p 00017000 1f:01 670372 /lib/ld-2.18-2013.10.so
bedb-bedd1000 rw-p  00:00 0  [stack]
bedf4000-bedf5000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0      [vectors]

So far, so good.

Thank you!


Tested-by: Chris Brandt 

RE: [PATCH v5 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Friday, October 06, 2017, Christoph Hellwig wrote:
> This is still missing a proper API for accessing the file system,
> as said before specifying a physical address in the mount command
> line is a an absolute non-no.
> 
> Either work with the mtd folks to get the mtd core down to an absolute
> minimum suitable for you, or figure out a way to specify fs nodes
> through DT or similar.

On my system, the QSPI Flash is memory mapped and set up by the boot 
loader. In order to test the upstream kernel, I use a squashfs image and 
mtd-rom.

So, 0x1800 is the physical address of flash as it is seen by the 
CPU.

Is there any benefit to doing something similar to this?

/* File System */
/* Requires CONFIG_MTD_ROM=y */
qspi@1800 {
compatible = "mtd-rom";
probe-type = "map_rom";
reg = <0x1800 0x400>;   /* 64 MB*/
bank-width = <4>;
device-width = <1>;

#address-cells = <1>;
#size-cells = <1>;

partition@80 {
label ="user";
reg = <0x080 0x80>; /* 8MB @ 0x1880 */
read-only;
};
};


Of course this basically ioremaps the entire space on probe, but I think
what you really want to do is just ioremap pages at a time (maybe..I 
might not be following your code correctly)


Chris

RE: [PATCH v5 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Friday, October 06, 2017, Christoph Hellwig wrote:
> This is still missing a proper API for accessing the file system,
> as said before specifying a physical address in the mount command
> line is a an absolute non-no.
> 
> Either work with the mtd folks to get the mtd core down to an absolute
> minimum suitable for you, or figure out a way to specify fs nodes
> through DT or similar.

On my system, the QSPI Flash is memory mapped and set up by the boot 
loader. In order to test the upstream kernel, I use a squashfs image and 
mtd-rom.

So, 0x1800 is the physical address of flash as it is seen by the 
CPU.

Is there any benefit to doing something similar to this?

/* File System */
/* Requires CONFIG_MTD_ROM=y */
qspi@1800 {
compatible = "mtd-rom";
probe-type = "map_rom";
reg = <0x1800 0x400>;   /* 64 MB*/
bank-width = <4>;
device-width = <1>;

#address-cells = <1>;
#size-cells = <1>;

partition@80 {
label ="user";
reg = <0x080 0x80>; /* 8MB @ 0x1880 */
read-only;
};
};


Of course this basically ioremaps the entire space on probe, but I think
what you really want to do is just ioremap pages at a time (maybe..I 
might not be following your code correctly)


Chris

RE: [PATCH v4 4/5] cramfs: add mmap support

[Chris Brandt]

On Thursday, October 05, 2017, Nicolas Pitre wrote:
> Do you have the same amount of free memory once booted in both cases?

Yes, almost exactly the same, so obvious it must be working the same for
both cases. That's enough evidence for me.

Thanks.

Chris

RE: [PATCH v4 4/5] cramfs: add mmap support

[Chris Brandt]

On Thursday, October 05, 2017, Nicolas Pitre wrote:
> Do you have the same amount of free memory once booted in both cases?

Yes, almost exactly the same, so obvious it must be working the same for
both cases. That's enough evidence for me.

Thanks.

Chris

RE: [PATCH v4 4/5] cramfs: add mmap support

[Chris Brandt]

On Wednesday, October 04, 2017, Nicolas Pitre wrote:
> On Wed, 4 Oct 2017, Christoph Hellwig wrote:
> 
> > As said in my last mail: look at the VM_MIXEDMAP flag and how it is
> > used by DAX, and you'll get out of the vma splitting business in the
> > fault path.
> 
> Alright, it appears to work.
> 
> The only downside so far is the lack of visibility from user space to
> confirm it actually works as intended. With the vma splitting approach
> you clearly see what gets directly mapped in /proc/*/maps thanks to
> remap_pfn_range() storing the actual physical address in vma->vm_pgoff.
> With VM_MIXEDMAP things are no longer visible. Any opinion for the best
> way to overcome this?
> 
> Anyway, here's a replacement for patch 4/5 below:
> 
> - >8
> Subject: cramfs: add mmap support
> 
> When cramfs_physmem is used then we have the opportunity to map files
> directly from ROM, directly into user space, saving on RAM usage.
> This gives us Execute-In-Place (XIP) support.


Tested on my setup:
 * Cortex A9 (with MMU)
 * CONFIG_XIP_KERNEL=y
 * booted with XIP CRAMFS as my rootfs 
 * all apps and libraries marked as XIP in my cramfs image



So far, functionally it seems to work the same as [PATCH v4 4/5].

As Nicolas said, before you could easily see that all my apps and 
libraries were XIP from Flash:

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e69000-b6f42000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f42000-b6f4a000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4a000-b6f4c000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4c000-b6f4d000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4d000-b6f5 rw-p  00:00 0
b6f5-b6f67000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f6a000-b6f6b000 rw-p  00:00 0
b6f6c000-b6f6e000 rw-p  00:00 0
b6f6e000-b6f6f000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f6f000-b6f7 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
beac-beae1000 rw-p  00:00 0  [stack]
bebc9000-bebca000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]



But now just busybox looks like it's XIP:

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e4d000-b6f26000 r-xp  00:0c 766540 /lib/libc-2.18-2013.10.so
b6f26000-b6f2e000 ---p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f2e000-b6f3 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f3-b6f31000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f31000-b6f34000 rw-p  00:00 0
b6f34000-b6f4b000 r-xp  00:0c 670372 /lib/ld-2.18-2013.10.so
b6f4e000-b6f4f000 rw-p  00:00 0
b6f5-b6f52000 rw-p  00:00 0
b6f52000-b6f53000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f53000-b6f54000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
bec93000-becb4000 rw-p  00:00 0  [stack]
befad000-befae000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Regardless, from a functional standpoint:

Tested-by: Chris Brandt <chris.bra...@renesas.com>




Just FYI, the previous [PATCH v4 4/5] also included this (which was the 
only real difference between v3 and v4):


diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig
index 5b4e0b7e13..306549be25 100644
--- a/fs/cramfs/Kconfig
+++ b/fs/cramfs/Kconfig
@@ -30,7 +30,7 @@ config CRAMFS_BLOCKDEV
 
 config CRAMFS_PHYSMEM
bool "Support CramFs image directly mapped in physical memory"
-   depends on CRAMFS
+   depends on CRAMFS = y
default y if !CRAMFS_BLOCKDEV
help
  This option allows the CramFs driver to load data directly from


Chris

RE: [PATCH v4 4/5] cramfs: add mmap support

[Chris Brandt]

On Wednesday, October 04, 2017, Nicolas Pitre wrote:
> On Wed, 4 Oct 2017, Christoph Hellwig wrote:
> 
> > As said in my last mail: look at the VM_MIXEDMAP flag and how it is
> > used by DAX, and you'll get out of the vma splitting business in the
> > fault path.
> 
> Alright, it appears to work.
> 
> The only downside so far is the lack of visibility from user space to
> confirm it actually works as intended. With the vma splitting approach
> you clearly see what gets directly mapped in /proc/*/maps thanks to
> remap_pfn_range() storing the actual physical address in vma->vm_pgoff.
> With VM_MIXEDMAP things are no longer visible. Any opinion for the best
> way to overcome this?
> 
> Anyway, here's a replacement for patch 4/5 below:
> 
> - >8
> Subject: cramfs: add mmap support
> 
> When cramfs_physmem is used then we have the opportunity to map files
> directly from ROM, directly into user space, saving on RAM usage.
> This gives us Execute-In-Place (XIP) support.


Tested on my setup:
 * Cortex A9 (with MMU)
 * CONFIG_XIP_KERNEL=y
 * booted with XIP CRAMFS as my rootfs 
 * all apps and libraries marked as XIP in my cramfs image



So far, functionally it seems to work the same as [PATCH v4 4/5].

As Nicolas said, before you could easily see that all my apps and 
libraries were XIP from Flash:

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e69000-b6f42000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f42000-b6f4a000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4a000-b6f4c000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4c000-b6f4d000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f4d000-b6f5 rw-p  00:00 0
b6f5-b6f67000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f6a000-b6f6b000 rw-p  00:00 0
b6f6c000-b6f6e000 rw-p  00:00 0
b6f6e000-b6f6f000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f6f000-b6f7 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
beac-beae1000 rw-p  00:00 0  [stack]
bebc9000-bebca000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]



But now just busybox looks like it's XIP:

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e4d000-b6f26000 r-xp  00:0c 766540 /lib/libc-2.18-2013.10.so
b6f26000-b6f2e000 ---p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f2e000-b6f3 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f3-b6f31000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f31000-b6f34000 rw-p  00:00 0
b6f34000-b6f4b000 r-xp  00:0c 670372 /lib/ld-2.18-2013.10.so
b6f4e000-b6f4f000 rw-p  00:00 0
b6f5-b6f52000 rw-p  00:00 0
b6f52000-b6f53000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f53000-b6f54000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
bec93000-becb4000 rw-p  00:00 0  [stack]
befad000-befae000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Regardless, from a functional standpoint:

Tested-by: Chris Brandt 




Just FYI, the previous [PATCH v4 4/5] also included this (which was the 
only real difference between v3 and v4):


diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig
index 5b4e0b7e13..306549be25 100644
--- a/fs/cramfs/Kconfig
+++ b/fs/cramfs/Kconfig
@@ -30,7 +30,7 @@ config CRAMFS_BLOCKDEV
 
 config CRAMFS_PHYSMEM
bool "Support CramFs image directly mapped in physical memory"
-   depends on CRAMFS
+   depends on CRAMFS = y
default y if !CRAMFS_BLOCKDEV
help
  This option allows the CramFs driver to load data directly from


Chris

RE: [PATCH v4 1/5] cramfs: direct memory access support

[Chris Brandt]

On Tuesday, October 03, 2017 1, Rob Herring wrote:
> On Sun, Oct 1, 2017 at 3:29 AM, Christoph Hellwig 
> wrote:
> > On Wed, Sep 27, 2017 at 07:32:20PM -0400, Nicolas Pitre wrote:
> >> To distinguish between both access types, the cramfs_physmem filesystem
> >> type must be specified when using a memory accessible cramfs image, and
> >> the physaddr argument must provide the actual filesystem image's
> physical
> >> memory location.
> >
> > Sorry, but this still is a complete no-go.  A physical address is not a
> > proper interface.  You still need to have some interface for your NOR
> nand
> > or DRAM.  - usually that would be a mtd driver, but if you have a good
> > reason why that's not suitable for you (and please explain it well)
> > we'll need a little OF or similar layer to bind a thin driver.
> 
> I don't disagree that we may need DT binding here, but DT bindings are
> h/w description and not a mechanism bind Linux kernel drivers. It can
> be a subtle distinction, but it is an important one.
> 
> I can see the case where we have no driver. For RAM we don't have a
> driver, yet pretty much all hardware has a DRAM controller which we
> just rely on the firmware to setup. I could also envision that we have
> hardware we do need to configure in the kernel. Perhaps the boot
> settings are not optimal or we want/need to manage the clocks. That
> seems somewhat unlikely if the kernel is also XIP from the same flash
> as it is in Nico's case.
> 
> We do often describe the flash layout in DT when partitions are not
> discoverable. I don't know if that would be needed here. Would the ROM
> here ever be updateable from within Linux? If we're talking about a
> single address to pass the kernel, DT seems like an overkill and
> kernel cmdline is perfectly valid IMO.


As someone that's been using an XIP File system for a while now (AXFS, 
obviously not xip-cramfs), there is a way (in my system at least) to 
write to the same Flash that the kernel and file system are currently XIP 
executing (think jumping to RAM, doing a small flash operation, then 
jumping back to Flash).

The use case is if you've logically partitioned your flash such that you
keep your application in a separate file XIP filesystem image, you 
remotely download an updated version to some unused portion of Flash, then 
simply unmount what you have been using and mount the new image since you
can pass in the physical address of where you wrote your new image to.

So in that case, I guess you can do some type of DT overlay or 
something, but at the moment, just having the physical address as a parameter 
in 
mount command makes it pretty darn easy.

Chris

RE: [PATCH v4 1/5] cramfs: direct memory access support

[Chris Brandt]

On Tuesday, October 03, 2017 1, Rob Herring wrote:
> On Sun, Oct 1, 2017 at 3:29 AM, Christoph Hellwig 
> wrote:
> > On Wed, Sep 27, 2017 at 07:32:20PM -0400, Nicolas Pitre wrote:
> >> To distinguish between both access types, the cramfs_physmem filesystem
> >> type must be specified when using a memory accessible cramfs image, and
> >> the physaddr argument must provide the actual filesystem image's
> physical
> >> memory location.
> >
> > Sorry, but this still is a complete no-go.  A physical address is not a
> > proper interface.  You still need to have some interface for your NOR
> nand
> > or DRAM.  - usually that would be a mtd driver, but if you have a good
> > reason why that's not suitable for you (and please explain it well)
> > we'll need a little OF or similar layer to bind a thin driver.
> 
> I don't disagree that we may need DT binding here, but DT bindings are
> h/w description and not a mechanism bind Linux kernel drivers. It can
> be a subtle distinction, but it is an important one.
> 
> I can see the case where we have no driver. For RAM we don't have a
> driver, yet pretty much all hardware has a DRAM controller which we
> just rely on the firmware to setup. I could also envision that we have
> hardware we do need to configure in the kernel. Perhaps the boot
> settings are not optimal or we want/need to manage the clocks. That
> seems somewhat unlikely if the kernel is also XIP from the same flash
> as it is in Nico's case.
> 
> We do often describe the flash layout in DT when partitions are not
> discoverable. I don't know if that would be needed here. Would the ROM
> here ever be updateable from within Linux? If we're talking about a
> single address to pass the kernel, DT seems like an overkill and
> kernel cmdline is perfectly valid IMO.


As someone that's been using an XIP File system for a while now (AXFS, 
obviously not xip-cramfs), there is a way (in my system at least) to 
write to the same Flash that the kernel and file system are currently XIP 
executing (think jumping to RAM, doing a small flash operation, then 
jumping back to Flash).

The use case is if you've logically partitioned your flash such that you
keep your application in a separate file XIP filesystem image, you 
remotely download an updated version to some unused portion of Flash, then 
simply unmount what you have been using and mount the new image since you
can pass in the physical address of where you wrote your new image to.

So in that case, I guess you can do some type of DT overlay or 
something, but at the moment, just having the physical address as a parameter 
in 
mount command makes it pretty darn easy.

Chris

RE: [PATCH v2 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> This series brings a nice refresh to the cramfs filesystem, adding the
> following capabilities:
> 
> - Direct memory access, bypassing the block and/or MTD layers entirely.
> 
> - Ability to store individual data blocks uncompressed.
> 
> - Ability to locate individual data blocks anywhere in the filesystem.
> 
> The end result is a very tight filesystem that can be accessed directly
> from ROM without any other subsystem underneath. Also this allows for
> user space XIP which is a very important feature for tiny embedded
> systems.
> 
> Why cramfs?
> 
>   Because cramfs is very simple and small. With CONFIG_CRAMFS_BLOCK=n and
>   CONFIG_CRAMFS_PHYSMEM=y the cramfs driver may use as little as 3704
> bytes
>   of code. That's many times smaller than squashfs. And the runtime memory
>   usage is also much less with cramfs than squashfs. It packs very tightly
>   already compared to romfs which has no compression support. And the
> cramfs
>   format was simple to extend, allowing for both compressed and
> uncompressed
>   blocks within the same file.
> 
> Why not accessing ROM via MTD?
> 
>   The MTD layer is nice and flexible. It also represents a huge overhead
>   considering its core with no other enabled options weights 19KB.
>   That's many times the size of the cramfs code for something that
>   essentially boils down to a glorified argument parser and a call to
>   memremap() in this case.  And if someone still wants to use cramfs via
>   MTD then it is already possible with mtdblock.
> 
> Why not using DAX?
> 
>   DAX stands for "Direct Access" and is a generic kernel layer helping
>   with the necessary tasks involved with XIP. It is tailored for large
>   writable filesystems and relies on the presence of an MMU. It also has
>   the following shortcoming: "The DAX code does not work correctly on
>   architectures which have virtually mapped caches such as ARM, MIPS and
>   SPARC." That makes it unsuitable for a large portion of the intended
>   targets for this series. And due to the read-only nature of cramfs, it
> is
>   possible to achieve the intended result with a much simpler approach
> making
>   DAX somewhat overkill in this context.
> 
> The maximum size of a cramfs image can't exceed 272MB. In practice it is
> likely to be much much less. Given this series is concerned with small
> memory systems, even in the MMU case there is always plenty of vmalloc
> space left to map it all and even a 272MB memremap() wouldn't be a
> problem. If it is then maybe your system is big enough with large
> resources to manage already and you're pretty unlikely to be using cramfs
> in the first place.
> 
> Of course, while this cramfs remains backward compatible with existing
> filesystem images, a newer mkcramfs version is necessary to take advantage
> of the extended data layout. I created a version of mkcramfs that
> detects ELF files and marks text+rodata segments for XIP and compresses
> the
> rest of those ELF files automatically.
> 
> So here it is. I'm also willing to step up as cramfs maintainer given
> that no sign of any maintenance activities appeared for years.
> 
> This series is also available based on v4.13-rc4 via git here:
> 
>   http://git.linaro.org/people/nicolas.pitre/linux xipcramfs
> 
> 
> Changes from v1:
> 
> - Improved mmap() support by adding the ability to partially populate a
>   mapping and lazily split the non directly mapable pages to a separate
>   vma at fault time (thanks to Chris Brandt for testing).
> 
> - Clarified the documentation some more.
> 
> 
> diffstat:
> 
>  Documentation/filesystems/cramfs.txt |  42 ++
>  MAINTAINERS  |   4 +-
>  fs/cramfs/Kconfig|  39 +-
>  fs/cramfs/README     |  31 +-
>  fs/cramfs/inode.c| 621 +
>  include/uapi/linux/cramfs_fs.h   |  20 +-
>  init/do_mounts.c |   8 +
>  7 files changed, 688 insertions(+), 77 deletions(-)


For this whole series:

Tested-by: Chris Brandt <chris.bra...@renesas.com>

RE: [PATCH v2 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> This series brings a nice refresh to the cramfs filesystem, adding the
> following capabilities:
> 
> - Direct memory access, bypassing the block and/or MTD layers entirely.
> 
> - Ability to store individual data blocks uncompressed.
> 
> - Ability to locate individual data blocks anywhere in the filesystem.
> 
> The end result is a very tight filesystem that can be accessed directly
> from ROM without any other subsystem underneath. Also this allows for
> user space XIP which is a very important feature for tiny embedded
> systems.
> 
> Why cramfs?
> 
>   Because cramfs is very simple and small. With CONFIG_CRAMFS_BLOCK=n and
>   CONFIG_CRAMFS_PHYSMEM=y the cramfs driver may use as little as 3704
> bytes
>   of code. That's many times smaller than squashfs. And the runtime memory
>   usage is also much less with cramfs than squashfs. It packs very tightly
>   already compared to romfs which has no compression support. And the
> cramfs
>   format was simple to extend, allowing for both compressed and
> uncompressed
>   blocks within the same file.
> 
> Why not accessing ROM via MTD?
> 
>   The MTD layer is nice and flexible. It also represents a huge overhead
>   considering its core with no other enabled options weights 19KB.
>   That's many times the size of the cramfs code for something that
>   essentially boils down to a glorified argument parser and a call to
>   memremap() in this case.  And if someone still wants to use cramfs via
>   MTD then it is already possible with mtdblock.
> 
> Why not using DAX?
> 
>   DAX stands for "Direct Access" and is a generic kernel layer helping
>   with the necessary tasks involved with XIP. It is tailored for large
>   writable filesystems and relies on the presence of an MMU. It also has
>   the following shortcoming: "The DAX code does not work correctly on
>   architectures which have virtually mapped caches such as ARM, MIPS and
>   SPARC." That makes it unsuitable for a large portion of the intended
>   targets for this series. And due to the read-only nature of cramfs, it
> is
>   possible to achieve the intended result with a much simpler approach
> making
>   DAX somewhat overkill in this context.
> 
> The maximum size of a cramfs image can't exceed 272MB. In practice it is
> likely to be much much less. Given this series is concerned with small
> memory systems, even in the MMU case there is always plenty of vmalloc
> space left to map it all and even a 272MB memremap() wouldn't be a
> problem. If it is then maybe your system is big enough with large
> resources to manage already and you're pretty unlikely to be using cramfs
> in the first place.
> 
> Of course, while this cramfs remains backward compatible with existing
> filesystem images, a newer mkcramfs version is necessary to take advantage
> of the extended data layout. I created a version of mkcramfs that
> detects ELF files and marks text+rodata segments for XIP and compresses
> the
> rest of those ELF files automatically.
> 
> So here it is. I'm also willing to step up as cramfs maintainer given
> that no sign of any maintenance activities appeared for years.
> 
> This series is also available based on v4.13-rc4 via git here:
> 
>   http://git.linaro.org/people/nicolas.pitre/linux xipcramfs
> 
> 
> Changes from v1:
> 
> - Improved mmap() support by adding the ability to partially populate a
>   mapping and lazily split the non directly mapable pages to a separate
>   vma at fault time (thanks to Chris Brandt for testing).
> 
> - Clarified the documentation some more.
> 
> 
> diffstat:
> 
>  Documentation/filesystems/cramfs.txt |  42 ++
>  MAINTAINERS  |   4 +-
>  fs/cramfs/Kconfig|  39 +-
>  fs/cramfs/README     |  31 +-
>  fs/cramfs/inode.c| 621 +
>  include/uapi/linux/cramfs_fs.h   |  20 +-
>  init/do_mounts.c |   8 +
>  7 files changed, 688 insertions(+), 77 deletions(-)


For this whole series:

Tested-by: Chris Brandt 

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Tuesday, August 29, 2017, Chris Brandt wrote:
> On Tuesday, August 29, 2017, Nicolas Pitre wrote:
> > On Tue, 29 Aug 2017, Chris Brandt wrote:
> >
> > > On Monday, August 28, 2017, Nicolas Pitre wrote:
> > > > OK I moved the lock promotion right at the beginning _before_
> > validating
> > > > the split point. Also got a reference on the file to make sure that
> > > > hasn't changed too.
> > > >
> > > > > While we are at it, what happens if you mmap 120Kb, then munmap()
> > the
> > > > middle
> > > > > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.
> > Will
> > > > your
> > > > > ->vm_private_data be correct for both?
> > > >
> > > > It wouldn't, but I now changed it to contain absolute values so now
> it
> > > > will. And if the split point lands in the hole then the code just
> > > > readjusts the pgoff at the beginning of the remaining part.
> > > >
> > > > Here's the revised patch:
> > >
> > >
> > > For whatever it's worth, as soon as I moved to 4.13-rc7,
> > > CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can
> even
> > > get to any console output.
> > >
> > > (both the old patch and the new patch)
> > >
> > > If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.
> > >
> > > However, if I boot -rc7 as a uImage, the new patch works just as good
> as
> > > the old patch.
> >
> > When not a uImage, do you mean by that a XIP kernel?
> 
> Yes, CONFIG_XIP_KERNEL.
> 
> > If so you should
> > know by now from that other thread on LAK that the XIP linker script is
> > broken and probably just worked by luck till now. Still, if you could
> > bisect between -rc4 and -rc7 and pinpoint the change that makes it not
> > work that would be better than speculations.
> 
> Note that everything else seem OK when CONFIG_XIP_KERNEL=y. It's just
> when CONFIG_XIP_KERNEL=y CONFIG_CRAMFS_PHYSMEM=y which is odd. So
> hopefully
> that means it will be easy to track down.


Update:

My issue was caused by the XIP linker script (vmlinux-xip.lds.S).

Therefore, by applying the following patch series from the 
linux-arm-kernel mailing list, my system could boot normally.

 [PATCH v2 0/5] make XIP kernel .data compressed in ROM
 [PATCH v2 1/5] ARM: head-common.S: speed up startup code
 [PATCH v2 2/5] ARM: vmlinux*.lds.S: some decruftification
 [PATCH v2 3/5] ARM: vmlinux.lds.S: replace open coded .data sections with 
generic macros
 [PATCH v2 4/5] ARM: vmlinux-xip.lds.S: fix multiple issues
 [PATCH v2 5/5] ARM: XIP kernel: store .data compressed in ROM


Now that I could boot again, this cramfs series of patches operates as 
designed.

Notice that busybox, libc and ld have physical addresses in Flash (ie, XIP)

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6eed000-b6fc6000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fc6000-b6fce000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fce000-b6fd r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fd-b6fd1000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fd1000-b6fd4000 rw-p  00:00 0
b6fd4000-b6feb000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6fee000-b6fef000 rw-p  00:00 0
b6ff-b6ff2000 rw-p  00:00 0
b6ff2000-b6ff3000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6ff3000-b6ff4000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
bee27000-bee48000 rw-p  00:00 0  [stack]
beea4000-beea5000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]



Tested-by: Chris Brandt <chris.bra...@renesas.com>

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Tuesday, August 29, 2017, Chris Brandt wrote:
> On Tuesday, August 29, 2017, Nicolas Pitre wrote:
> > On Tue, 29 Aug 2017, Chris Brandt wrote:
> >
> > > On Monday, August 28, 2017, Nicolas Pitre wrote:
> > > > OK I moved the lock promotion right at the beginning _before_
> > validating
> > > > the split point. Also got a reference on the file to make sure that
> > > > hasn't changed too.
> > > >
> > > > > While we are at it, what happens if you mmap 120Kb, then munmap()
> > the
> > > > middle
> > > > > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.
> > Will
> > > > your
> > > > > ->vm_private_data be correct for both?
> > > >
> > > > It wouldn't, but I now changed it to contain absolute values so now
> it
> > > > will. And if the split point lands in the hole then the code just
> > > > readjusts the pgoff at the beginning of the remaining part.
> > > >
> > > > Here's the revised patch:
> > >
> > >
> > > For whatever it's worth, as soon as I moved to 4.13-rc7,
> > > CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can
> even
> > > get to any console output.
> > >
> > > (both the old patch and the new patch)
> > >
> > > If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.
> > >
> > > However, if I boot -rc7 as a uImage, the new patch works just as good
> as
> > > the old patch.
> >
> > When not a uImage, do you mean by that a XIP kernel?
> 
> Yes, CONFIG_XIP_KERNEL.
> 
> > If so you should
> > know by now from that other thread on LAK that the XIP linker script is
> > broken and probably just worked by luck till now. Still, if you could
> > bisect between -rc4 and -rc7 and pinpoint the change that makes it not
> > work that would be better than speculations.
> 
> Note that everything else seem OK when CONFIG_XIP_KERNEL=y. It's just
> when CONFIG_XIP_KERNEL=y CONFIG_CRAMFS_PHYSMEM=y which is odd. So
> hopefully
> that means it will be easy to track down.


Update:

My issue was caused by the XIP linker script (vmlinux-xip.lds.S).

Therefore, by applying the following patch series from the 
linux-arm-kernel mailing list, my system could boot normally.

 [PATCH v2 0/5] make XIP kernel .data compressed in ROM
 [PATCH v2 1/5] ARM: head-common.S: speed up startup code
 [PATCH v2 2/5] ARM: vmlinux*.lds.S: some decruftification
 [PATCH v2 3/5] ARM: vmlinux.lds.S: replace open coded .data sections with 
generic macros
 [PATCH v2 4/5] ARM: vmlinux-xip.lds.S: fix multiple issues
 [PATCH v2 5/5] ARM: XIP kernel: store .data compressed in ROM


Now that I could boot again, this cramfs series of patches operates as 
designed.

Notice that busybox, libc and ld have physical addresses in Flash (ie, XIP)

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6eed000-b6fc6000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fc6000-b6fce000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fce000-b6fd r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fd-b6fd1000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fd1000-b6fd4000 rw-p  00:00 0
b6fd4000-b6feb000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6fee000-b6fef000 rw-p  00:00 0
b6ff-b6ff2000 rw-p  00:00 0
b6ff2000-b6ff3000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6ff3000-b6ff4000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
bee27000-bee48000 rw-p  00:00 0  [stack]
beea4000-beea5000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]



Tested-by: Chris Brandt 

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Tuesday, August 29, 2017, Nicolas Pitre wrote:
> On Tue, 29 Aug 2017, Chris Brandt wrote:
> 
> > On Monday, August 28, 2017, Nicolas Pitre wrote:
> > > OK I moved the lock promotion right at the beginning _before_
> validating
> > > the split point. Also got a reference on the file to make sure that
> > > hasn't changed too.
> > >
> > > > While we are at it, what happens if you mmap 120Kb, then munmap()
> the
> > > middle
> > > > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.
> Will
> > > your
> > > > ->vm_private_data be correct for both?
> > >
> > > It wouldn't, but I now changed it to contain absolute values so now it
> > > will. And if the split point lands in the hole then the code just
> > > readjusts the pgoff at the beginning of the remaining part.
> > >
> > > Here's the revised patch:
> >
> >
> > For whatever it's worth, as soon as I moved to 4.13-rc7,
> > CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can even
> > get to any console output.
> >
> > (both the old patch and the new patch)
> >
> > If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.
> >
> > However, if I boot -rc7 as a uImage, the new patch works just as good as
> > the old patch.
> 
> When not a uImage, do you mean by that a XIP kernel?

Yes, CONFIG_XIP_KERNEL.

> If so you should
> know by now from that other thread on LAK that the XIP linker script is
> broken and probably just worked by luck till now. Still, if you could
> bisect between -rc4 and -rc7 and pinpoint the change that makes it not
> work that would be better than speculations.

Note that everything else seem OK when CONFIG_XIP_KERNEL=y. It's just 
when CONFIG_XIP_KERNEL=y CONFIG_CRAMFS_PHYSMEM=y which is odd. So hopefully
that means it will be easy to track down.


Chris

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Tuesday, August 29, 2017, Nicolas Pitre wrote:
> On Tue, 29 Aug 2017, Chris Brandt wrote:
> 
> > On Monday, August 28, 2017, Nicolas Pitre wrote:
> > > OK I moved the lock promotion right at the beginning _before_
> validating
> > > the split point. Also got a reference on the file to make sure that
> > > hasn't changed too.
> > >
> > > > While we are at it, what happens if you mmap 120Kb, then munmap()
> the
> > > middle
> > > > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.
> Will
> > > your
> > > > ->vm_private_data be correct for both?
> > >
> > > It wouldn't, but I now changed it to contain absolute values so now it
> > > will. And if the split point lands in the hole then the code just
> > > readjusts the pgoff at the beginning of the remaining part.
> > >
> > > Here's the revised patch:
> >
> >
> > For whatever it's worth, as soon as I moved to 4.13-rc7,
> > CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can even
> > get to any console output.
> >
> > (both the old patch and the new patch)
> >
> > If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.
> >
> > However, if I boot -rc7 as a uImage, the new patch works just as good as
> > the old patch.
> 
> When not a uImage, do you mean by that a XIP kernel?

Yes, CONFIG_XIP_KERNEL.

> If so you should
> know by now from that other thread on LAK that the XIP linker script is
> broken and probably just worked by luck till now. Still, if you could
> bisect between -rc4 and -rc7 and pinpoint the change that makes it not
> work that would be better than speculations.

Note that everything else seem OK when CONFIG_XIP_KERNEL=y. It's just 
when CONFIG_XIP_KERNEL=y CONFIG_CRAMFS_PHYSMEM=y which is odd. So hopefully
that means it will be easy to track down.


Chris

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Monday, August 28, 2017, Nicolas Pitre wrote:
> OK I moved the lock promotion right at the beginning _before_ validating
> the split point. Also got a reference on the file to make sure that
> hasn't changed too.
> 
> > While we are at it, what happens if you mmap 120Kb, then munmap() the
> middle
> > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.  Will
> your
> > ->vm_private_data be correct for both?
> 
> It wouldn't, but I now changed it to contain absolute values so now it
> will. And if the split point lands in the hole then the code just
> readjusts the pgoff at the beginning of the remaining part.
> 
> Here's the revised patch:


For whatever it's worth, as soon as I moved to 4.13-rc7,
CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can even 
get to any console output.

(both the old patch and the new patch)

If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.

However, if I boot -rc7 as a uImage, the new patch works just as good as
the old patch.

(mounting after boot, or booting with rootfstype=cramfs_physmem)


I guess I'll have to figure out what happened between -rc4 and -rc7.
Damn!


Chris

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Monday, August 28, 2017, Nicolas Pitre wrote:
> OK I moved the lock promotion right at the beginning _before_ validating
> the split point. Also got a reference on the file to make sure that
> hasn't changed too.
> 
> > While we are at it, what happens if you mmap 120Kb, then munmap() the
> middle
> > 40Kb.  Leaving two 40Kb VMAs with 40Kb gap between them, that is.  Will
> your
> > ->vm_private_data be correct for both?
> 
> It wouldn't, but I now changed it to contain absolute values so now it
> will. And if the split point lands in the hole then the code just
> readjusts the pgoff at the beginning of the remaining part.
> 
> Here's the revised patch:


For whatever it's worth, as soon as I moved to 4.13-rc7,
CONFIG_CRAMFS_PHYSMEM=y crashes my XIP_KERNEL system before it can even 
get to any console output.

(both the old patch and the new patch)

If CONFIG_CRAMFS_PHYSMEM is not set, my XIP system boots fine.

However, if I boot -rc7 as a uImage, the new patch works just as good as
the old patch.

(mounting after boot, or booting with rootfstype=cramfs_physmem)


I guess I'll have to figure out what happened between -rc4 and -rc7.
Damn!


Chris

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> When cramfs_physmem is used then we have the opportunity to map files
> directly from ROM, directly into user space, saving on RAM usage.
> This gives us Execute-In-Place (XIP) support.
> 
> For a file to be mmap()-able, the map area has to correspond to a range
> of uncompressed and contiguous blocks, and in the MMU case it also has
> to be page aligned. A version of mkcramfs with appropriate support is
> necessary to create such a filesystem image.
> 
> In the MMU case it may happen for a vma structure to extend beyond the
> actual file size. This is notably the case in binfmt_elf.c:elf_map().
> Or the file's last block is shared with other files and cannot be mapped
> as is. Rather than refusing to mmap it, we do a partial map and set up a
> special vm_ops fault handler that splits the vma in two: the direct
> mapping
> vma and the memory-backed vma populated by the readpage method.
> 
> In the non-MMU case it is the get_unmapped_area method that is responsible
> for providing the address where the actual data can be found. No mapping
> is necessary of course.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/inode.c | 270
> ++
>  1 file changed, 270 insertions(+)
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index b825ae162c..e3884c607b 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -49,6 +50,7 @@ static inline struct cramfs_sb_info *CRAMFS_SB(struct
> super_block *sb)
>  static const struct super_operations cramfs_ops;
>  static const struct inode_operations cramfs_dir_inode_operations;
>  static const struct file_operations cramfs_directory_operations;
> +static const struct file_operations cramfs_physmem_fops;
>  static const struct address_space_operations cramfs_aops;
> 
>  static DEFINE_MUTEX(read_mutex);
> @@ -96,6 +98,10 @@ static struct inode *get_cramfs_inode(struct
> super_block *sb,
>   case S_IFREG:
>   inode->i_fop = _ro_fops;
>   inode->i_data.a_ops = _aops;
> + if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) &&
> + CRAMFS_SB(sb)->flags & CRAMFS_FLAG_EXT_BLOCK_POINTERS &&
> + CRAMFS_SB(sb)->linear_phys_addr)
> + inode->i_fop = _physmem_fops;
>   break;
>   case S_IFDIR:
>   inode->i_op = _dir_inode_operations;
> @@ -277,6 +283,270 @@ static void *cramfs_read(struct super_block *sb,
> unsigned int offset,
>   return NULL;
>  }
> 
> +/*
> + * For a mapping to be possible, we need a range of uncompressed and
> + * contiguous blocks. Return the offset for the first block and number of
> + * valid blocks for which that is true, or zero otherwise.
> + */
> +static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32
> *pages)
> +{
> + struct super_block *sb = inode->i_sb;
> + struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
> + int i;
> + u32 *blockptrs, blockaddr;
> +
> + /*
> +  * We can dereference memory directly here as this code may be
> +  * reached only when there is a direct filesystem image mapping
> +  * available in memory.
> +  */
> + blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) +
> pgoff*4);
> + blockaddr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;
> + i = 0;
> + do {
> + u32 expect = blockaddr + i * (PAGE_SIZE >> 2);
> + expect |=
> CRAMFS_BLK_FLAG_DIRECT_PTR|CRAMFS_BLK_FLAG_UNCOMPRESSED;
> + if (blockptrs[i] != expect) {
> + pr_debug("range: block %d/%d got %#x expects %#x\n",
> +  pgoff+i, pgoff+*pages-1, blockptrs[i], expect);
> + if (i == 0)
> + return 0;
> + break;
> + }
> + } while (++i < *pages);
> +
> + *pages = i;
> +
> + /* stored "direct" block ptrs are shifted down by 2 bits */
> + return blockaddr << 2;
> +}
> +
> +/*
> + * It is possible for cramfs_physmem_mmap() to partially populate the
> mapping
> + * causing page faults in the unmapped area. When that happens, we need
> to
> + * split the vma so that the unmapped area gets its own vma that can be
> backed
> + * with actual memory pages and loaded normally. This is necessary
> because
> + * remap_pfn_range() overwrites vma->vm_pgoff with the pfn and
> filemap_fault()
> + * no longer works with it. Furthermore this makes /proc/x/maps right.
> + * Q: is there a way to do split vma at mmap() time?
> + */
> +static const struct vm_operations_struct cramfs_vmasplit_ops;
> +static int cramfs_vmasplit_fault(struct vm_fault *vmf)
> +{
> + struct mm_struct *mm = vmf->vma->vm_mm;
> + struct vm_area_struct *vma, *new_vma;
> + unsigned long split_val, split_addr;
> + unsigned int 

RE: [PATCH v2 4/5] cramfs: add mmap support

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> When cramfs_physmem is used then we have the opportunity to map files
> directly from ROM, directly into user space, saving on RAM usage.
> This gives us Execute-In-Place (XIP) support.
> 
> For a file to be mmap()-able, the map area has to correspond to a range
> of uncompressed and contiguous blocks, and in the MMU case it also has
> to be page aligned. A version of mkcramfs with appropriate support is
> necessary to create such a filesystem image.
> 
> In the MMU case it may happen for a vma structure to extend beyond the
> actual file size. This is notably the case in binfmt_elf.c:elf_map().
> Or the file's last block is shared with other files and cannot be mapped
> as is. Rather than refusing to mmap it, we do a partial map and set up a
> special vm_ops fault handler that splits the vma in two: the direct
> mapping
> vma and the memory-backed vma populated by the readpage method.
> 
> In the non-MMU case it is the get_unmapped_area method that is responsible
> for providing the address where the actual data can be found. No mapping
> is necessary of course.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/inode.c | 270
> ++
>  1 file changed, 270 insertions(+)
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index b825ae162c..e3884c607b 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -49,6 +50,7 @@ static inline struct cramfs_sb_info *CRAMFS_SB(struct
> super_block *sb)
>  static const struct super_operations cramfs_ops;
>  static const struct inode_operations cramfs_dir_inode_operations;
>  static const struct file_operations cramfs_directory_operations;
> +static const struct file_operations cramfs_physmem_fops;
>  static const struct address_space_operations cramfs_aops;
> 
>  static DEFINE_MUTEX(read_mutex);
> @@ -96,6 +98,10 @@ static struct inode *get_cramfs_inode(struct
> super_block *sb,
>   case S_IFREG:
>   inode->i_fop = _ro_fops;
>   inode->i_data.a_ops = _aops;
> + if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) &&
> + CRAMFS_SB(sb)->flags & CRAMFS_FLAG_EXT_BLOCK_POINTERS &&
> + CRAMFS_SB(sb)->linear_phys_addr)
> + inode->i_fop = _physmem_fops;
>   break;
>   case S_IFDIR:
>   inode->i_op = _dir_inode_operations;
> @@ -277,6 +283,270 @@ static void *cramfs_read(struct super_block *sb,
> unsigned int offset,
>   return NULL;
>  }
> 
> +/*
> + * For a mapping to be possible, we need a range of uncompressed and
> + * contiguous blocks. Return the offset for the first block and number of
> + * valid blocks for which that is true, or zero otherwise.
> + */
> +static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32
> *pages)
> +{
> + struct super_block *sb = inode->i_sb;
> + struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
> + int i;
> + u32 *blockptrs, blockaddr;
> +
> + /*
> +  * We can dereference memory directly here as this code may be
> +  * reached only when there is a direct filesystem image mapping
> +  * available in memory.
> +  */
> + blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) +
> pgoff*4);
> + blockaddr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;
> + i = 0;
> + do {
> + u32 expect = blockaddr + i * (PAGE_SIZE >> 2);
> + expect |=
> CRAMFS_BLK_FLAG_DIRECT_PTR|CRAMFS_BLK_FLAG_UNCOMPRESSED;
> + if (blockptrs[i] != expect) {
> + pr_debug("range: block %d/%d got %#x expects %#x\n",
> +  pgoff+i, pgoff+*pages-1, blockptrs[i], expect);
> + if (i == 0)
> + return 0;
> + break;
> + }
> + } while (++i < *pages);
> +
> + *pages = i;
> +
> + /* stored "direct" block ptrs are shifted down by 2 bits */
> + return blockaddr << 2;
> +}
> +
> +/*
> + * It is possible for cramfs_physmem_mmap() to partially populate the
> mapping
> + * causing page faults in the unmapped area. When that happens, we need
> to
> + * split the vma so that the unmapped area gets its own vma that can be
> backed
> + * with actual memory pages and loaded normally. This is necessary
> because
> + * remap_pfn_range() overwrites vma->vm_pgoff with the pfn and
> filemap_fault()
> + * no longer works with it. Furthermore this makes /proc/x/maps right.
> + * Q: is there a way to do split vma at mmap() time?
> + */
> +static const struct vm_operations_struct cramfs_vmasplit_ops;
> +static int cramfs_vmasplit_fault(struct vm_fault *vmf)
> +{
> + struct mm_struct *mm = vmf->vma->vm_mm;
> + struct vm_area_struct *vma, *new_vma;
> + unsigned long split_val, split_addr;
> + unsigned int split_pgoff, split_page;

RE: [PATCH v2 3/5] cramfs: implement uncompressed and arbitrary data block positioning

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> Two new capabilities are introduced here:
> 
> - The ability to store some blocks uncompressed.
> 
> - The ability to locate blocks anywhere.
> 
> Those capabilities can be used independently, but the combination
> opens the possibility for execute-in-place (XIP) of program text segments
> that must remain uncompressed, and in the MMU case, must have a specific
> alignment.  It is even possible to still have the writable data segments
> from the same file compressed as they have to be copied into RAM anyway.
> 
> This is achieved by giving special meanings to some unused block pointer
> bits while remaining compatible with legacy cramfs images.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/README   | 31 ++-
>  fs/cramfs/inode.c  | 87 +
> -
>  include/uapi/linux/cramfs_fs.h | 20 +-
>  3 files changed, 118 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/cramfs/README b/fs/cramfs/README
> index 9d4e7ea311..d71b27e0ff 100644
> --- a/fs/cramfs/README
> +++ b/fs/cramfs/README
> @@ -49,17 +49,46 @@ same as the start of the (i+1)'th  if there is
> one).  The first
>   immediately follows the last  for the file.
>  s are each 32 bits long.
> 
> +When the CRAMFS_FLAG_EXT_BLOCK_POINTERS capability bit is set, each
> +'s top bits may contain special flags as follows:
> +
> +CRAMFS_BLK_FLAG_UNCOMPRESSED (bit 31):
> + The block data is not compressed and should be copied verbatim.
> +
> +CRAMFS_BLK_FLAG_DIRECT_PTR (bit 30):
> + The  stores the actual block start offset and not
> + its end, shifted right by 2 bits. The block must therefore be
> + aligned to a 4-byte boundary. The block size is either blksize
> + if CRAMFS_BLK_FLAG_UNCOMPRESSED is also specified, otherwise
> + the compressed data length is included in the first 2 bytes of
> + the block data. This is used to allow discontiguous data layout
> + and specific data block alignments e.g. for XIP applications.
> +
> +
>  The order of 's is a depth-first descent of the directory
>  tree, i.e. the same order as `find -size +0 \( -type f -o -type l \)
>  -print'.
> 
> 
>  : The i'th  is the output of zlib's compress function
> -applied to the i'th blksize-sized chunk of the input data.
> +applied to the i'th blksize-sized chunk of the input data if the
> +corresponding CRAMFS_BLK_FLAG_UNCOMPRESSED  bit is not set,
> +otherwise it is the input data directly.
>  (For the last  of the file, the input may of course be smaller.)
>  Each  may be a different size.  (See  above.)
> +
>  s are merely byte-aligned, not generally u32-aligned.
> 
> +When CRAMFS_BLK_FLAG_DIRECT_PTR is specified then the corresponding
> + may be located anywhere and not necessarily contiguous with
> +the previous/next blocks. In that case it is minimally u32-aligned.
> +If CRAMFS_BLK_FLAG_UNCOMPRESSED is also specified then the size is always
> +blksize except for the last block which is limited by the file length.
> +If CRAMFS_BLK_FLAG_DIRECT_PTR is set and CRAMFS_BLK_FLAG_UNCOMPRESSED
> +is not set then the first 2 bytes of the block contains the size of the
> +remaining block data as this cannot be determined from the placement of
> +logically adjacent blocks.
> +
> 
>  Holes
>  -
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 393eb27ef4..b825ae162c 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -636,33 +636,84 @@ static int cramfs_readpage(struct file *file, struct
> page *page)
>   if (page->index < maxblock) {
>   struct super_block *sb = inode->i_sb;
>   u32 blkptr_offset = OFFSET(inode) + page->index*4;
> - u32 start_offset, compr_len;
> + u32 block_ptr, block_start, block_len;
> + bool uncompressed, direct;
> 
> - start_offset = OFFSET(inode) + maxblock*4;
>   mutex_lock(_mutex);
> - if (page->index)
> - start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4,
> - 4);
> - compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) -
> - start_offset);
> - mutex_unlock(_mutex);
> + block_ptr = *(u32 *) cramfs_read(sb, blkptr_offset, 4);
> + uncompressed = (block_ptr & CRAMFS_BLK_FLAG_UNCOMPRESSED);
> + direct = (block_ptr & CRAMFS_BLK_FLAG_DIRECT_PTR);
> + block_ptr &= ~CRAMFS_BLK_FLAGS;
> +
> + if (direct) {
> + /*
> +  * The block pointer is an absolute start pointer,
> +  * shifted by 2 bits. The size is included in the
> +  * first 2 bytes of the data block when compressed,
> +  * or PAGE_SIZE otherwise.
> +  */
> + block_start = block_ptr << 2;
> + if 

RE: [PATCH v2 3/5] cramfs: implement uncompressed and arbitrary data block positioning

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> Two new capabilities are introduced here:
> 
> - The ability to store some blocks uncompressed.
> 
> - The ability to locate blocks anywhere.
> 
> Those capabilities can be used independently, but the combination
> opens the possibility for execute-in-place (XIP) of program text segments
> that must remain uncompressed, and in the MMU case, must have a specific
> alignment.  It is even possible to still have the writable data segments
> from the same file compressed as they have to be copied into RAM anyway.
> 
> This is achieved by giving special meanings to some unused block pointer
> bits while remaining compatible with legacy cramfs images.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/README   | 31 ++-
>  fs/cramfs/inode.c  | 87 +
> -
>  include/uapi/linux/cramfs_fs.h | 20 +-
>  3 files changed, 118 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/cramfs/README b/fs/cramfs/README
> index 9d4e7ea311..d71b27e0ff 100644
> --- a/fs/cramfs/README
> +++ b/fs/cramfs/README
> @@ -49,17 +49,46 @@ same as the start of the (i+1)'th  if there is
> one).  The first
>   immediately follows the last  for the file.
>  s are each 32 bits long.
> 
> +When the CRAMFS_FLAG_EXT_BLOCK_POINTERS capability bit is set, each
> +'s top bits may contain special flags as follows:
> +
> +CRAMFS_BLK_FLAG_UNCOMPRESSED (bit 31):
> + The block data is not compressed and should be copied verbatim.
> +
> +CRAMFS_BLK_FLAG_DIRECT_PTR (bit 30):
> + The  stores the actual block start offset and not
> + its end, shifted right by 2 bits. The block must therefore be
> + aligned to a 4-byte boundary. The block size is either blksize
> + if CRAMFS_BLK_FLAG_UNCOMPRESSED is also specified, otherwise
> + the compressed data length is included in the first 2 bytes of
> + the block data. This is used to allow discontiguous data layout
> + and specific data block alignments e.g. for XIP applications.
> +
> +
>  The order of 's is a depth-first descent of the directory
>  tree, i.e. the same order as `find -size +0 \( -type f -o -type l \)
>  -print'.
> 
> 
>  : The i'th  is the output of zlib's compress function
> -applied to the i'th blksize-sized chunk of the input data.
> +applied to the i'th blksize-sized chunk of the input data if the
> +corresponding CRAMFS_BLK_FLAG_UNCOMPRESSED  bit is not set,
> +otherwise it is the input data directly.
>  (For the last  of the file, the input may of course be smaller.)
>  Each  may be a different size.  (See  above.)
> +
>  s are merely byte-aligned, not generally u32-aligned.
> 
> +When CRAMFS_BLK_FLAG_DIRECT_PTR is specified then the corresponding
> + may be located anywhere and not necessarily contiguous with
> +the previous/next blocks. In that case it is minimally u32-aligned.
> +If CRAMFS_BLK_FLAG_UNCOMPRESSED is also specified then the size is always
> +blksize except for the last block which is limited by the file length.
> +If CRAMFS_BLK_FLAG_DIRECT_PTR is set and CRAMFS_BLK_FLAG_UNCOMPRESSED
> +is not set then the first 2 bytes of the block contains the size of the
> +remaining block data as this cannot be determined from the placement of
> +logically adjacent blocks.
> +
> 
>  Holes
>  -
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 393eb27ef4..b825ae162c 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -636,33 +636,84 @@ static int cramfs_readpage(struct file *file, struct
> page *page)
>   if (page->index < maxblock) {
>   struct super_block *sb = inode->i_sb;
>   u32 blkptr_offset = OFFSET(inode) + page->index*4;
> - u32 start_offset, compr_len;
> + u32 block_ptr, block_start, block_len;
> + bool uncompressed, direct;
> 
> - start_offset = OFFSET(inode) + maxblock*4;
>   mutex_lock(_mutex);
> - if (page->index)
> - start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4,
> - 4);
> - compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) -
> - start_offset);
> - mutex_unlock(_mutex);
> + block_ptr = *(u32 *) cramfs_read(sb, blkptr_offset, 4);
> + uncompressed = (block_ptr & CRAMFS_BLK_FLAG_UNCOMPRESSED);
> + direct = (block_ptr & CRAMFS_BLK_FLAG_DIRECT_PTR);
> + block_ptr &= ~CRAMFS_BLK_FLAGS;
> +
> + if (direct) {
> + /*
> +  * The block pointer is an absolute start pointer,
> +  * shifted by 2 bits. The size is included in the
> +  * first 2 bytes of the data block when compressed,
> +  * or PAGE_SIZE otherwise.
> +  */
> + block_start = block_ptr << 2;
> + if (uncompressed) {

RE: [PATCH v2 1/5] cramfs: direct memory access support

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> Small embedded systems typically execute the kernel code in place (XIP)
> directly from flash to save on precious RAM usage. This adds the ability
> to consume filesystem data directly from flash to the cramfs filesystem
> as well. Cramfs is particularly well suited to this feature as it is
> very simple and its RAM usage is already very low, and with this feature
> it is possible to use it with no block device support and even lower RAM
> usage.
> 
> This patch was inspired by a similar patch from Shane Nay dated 17 years
> ago that used to be very popular in embedded circles but never made it
> into mainline. This is a cleaned-up implementation that uses far fewer
> memory address at run time when both methods are configured in. In the
> context of small IoT deployments, this functionality has become relevant
> and useful again.
> 
> To distinguish between both access types, the cramfs_physmem filesystem
> type must be specified when using a memory accessible cramfs image, and
> the physaddr argument must provide the actual filesystem image's physical
> memory location.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/Kconfig |  30 ++-
>  fs/cramfs/inode.c | 264 +++--
> -
>  2 files changed, 242 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig
> index 11b29d491b..5eed4ad2d5 100644
> --- a/fs/cramfs/Kconfig
> +++ b/fs/cramfs/Kconfig
> @@ -1,6 +1,5 @@
>  config CRAMFS
>   tristate "Compressed ROM file system support (cramfs) (OBSOLETE)"
> - depends on BLOCK
>   select ZLIB_INFLATE
>   help
> Saying Y here includes support for CramFs (Compressed ROM File
> @@ -20,3 +19,32 @@ config CRAMFS
> in terms of performance and features.
> 
> If unsure, say N.
> +
> +config CRAMFS_BLOCKDEV
> + bool "Support CramFs image over a regular block device" if EXPERT
> + depends on CRAMFS && BLOCK
> + default y
> + help
> +   This option allows the CramFs driver to load data from a regular
> +   block device such a disk partition or a ramdisk.
> +


trailing whitespace


> +config CRAMFS_PHYSMEM
> + bool "Support CramFs image directly mapped in physical memory"
> + depends on CRAMFS
> + default y if !CRAMFS_BLOCKDEV
> + help
> +   This option allows the CramFs driver to load data directly from
> +   a linear adressed memory range (usually non volatile memory
> +   like flash) instead of going through the block device layer.
> +   This saves some memory since no intermediate buffering is
> +   necessary.
> +
> +   The filesystem type for this feature is "cramfs_physmem".
> +   The location of the CramFs image in memory is board
> +   dependent. Therefore, if you say Y, you must know the proper
> +   physical address where to store the CramFs image and specify
> +   it using the physaddr=0x mount option (for example:
> +   "mount -t cramfs_physmem -o physaddr=0x10 none /mnt").
> +
> +   If unsure, say N.
> +


new blank line at EOF



-Chris

RE: [PATCH v2 1/5] cramfs: direct memory access support

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> Small embedded systems typically execute the kernel code in place (XIP)
> directly from flash to save on precious RAM usage. This adds the ability
> to consume filesystem data directly from flash to the cramfs filesystem
> as well. Cramfs is particularly well suited to this feature as it is
> very simple and its RAM usage is already very low, and with this feature
> it is possible to use it with no block device support and even lower RAM
> usage.
> 
> This patch was inspired by a similar patch from Shane Nay dated 17 years
> ago that used to be very popular in embedded circles but never made it
> into mainline. This is a cleaned-up implementation that uses far fewer
> memory address at run time when both methods are configured in. In the
> context of small IoT deployments, this functionality has become relevant
> and useful again.
> 
> To distinguish between both access types, the cramfs_physmem filesystem
> type must be specified when using a memory accessible cramfs image, and
> the physaddr argument must provide the actual filesystem image's physical
> memory location.
> 
> Signed-off-by: Nicolas Pitre 
> ---
>  fs/cramfs/Kconfig |  30 ++-
>  fs/cramfs/inode.c | 264 +++--
> -
>  2 files changed, 242 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig
> index 11b29d491b..5eed4ad2d5 100644
> --- a/fs/cramfs/Kconfig
> +++ b/fs/cramfs/Kconfig
> @@ -1,6 +1,5 @@
>  config CRAMFS
>   tristate "Compressed ROM file system support (cramfs) (OBSOLETE)"
> - depends on BLOCK
>   select ZLIB_INFLATE
>   help
> Saying Y here includes support for CramFs (Compressed ROM File
> @@ -20,3 +19,32 @@ config CRAMFS
> in terms of performance and features.
> 
> If unsure, say N.
> +
> +config CRAMFS_BLOCKDEV
> + bool "Support CramFs image over a regular block device" if EXPERT
> + depends on CRAMFS && BLOCK
> + default y
> + help
> +   This option allows the CramFs driver to load data from a regular
> +   block device such a disk partition or a ramdisk.
> +


trailing whitespace


> +config CRAMFS_PHYSMEM
> + bool "Support CramFs image directly mapped in physical memory"
> + depends on CRAMFS
> + default y if !CRAMFS_BLOCKDEV
> + help
> +   This option allows the CramFs driver to load data directly from
> +   a linear adressed memory range (usually non volatile memory
> +   like flash) instead of going through the block device layer.
> +   This saves some memory since no intermediate buffering is
> +   necessary.
> +
> +   The filesystem type for this feature is "cramfs_physmem".
> +   The location of the CramFs image in memory is board
> +   dependent. Therefore, if you say Y, you must know the proper
> +   physical address where to store the CramFs image and specify
> +   it using the physaddr=0x mount option (for example:
> +   "mount -t cramfs_physmem -o physaddr=0x10 none /mnt").
> +
> +   If unsure, say N.
> +


new blank line at EOF



-Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017 1, Nicolas Pitre wrote:
> > Just FYI,
> > I'm running an xipImage with all the RZ/A1 upstream drivers enabled and
> > only using about 4.5MB of total system RAM.
> > That's pretty good. Of course for a real application, you would trim off
> > the drivers and subsystems you don't plan on using, thus lowering your
> > RAM usage.
> 
> On my MMU-less test target I'm going under the 1MB mark now.

Show off  ;)


> Given that I also applied the device table patch to mkcramfs (that
> allows for the creation of device nodes and arbitrary
> user/group/permission without being root) it would be possible to extend
> this mechanism to implement other XIP patterns such as for
> uncompressible media files for example.

Good, I was going to ask about that.

I made an example once were all the graphics were RAW and uncompressed 
and marked as XIP in AXFS. The result was a large saving of RAM because 
as the graphics framework (DirectFB) would copy directly from Flash 
whenever it needed to do a background erase or image re-draw (button press 
animations).

Same went for playing MP3 files. The MP3 files were XIP in flash, so 
mpg123 pulled from flash directly.


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017 1, Nicolas Pitre wrote:
> > Just FYI,
> > I'm running an xipImage with all the RZ/A1 upstream drivers enabled and
> > only using about 4.5MB of total system RAM.
> > That's pretty good. Of course for a real application, you would trim off
> > the drivers and subsystems you don't plan on using, thus lowering your
> > RAM usage.
> 
> On my MMU-less test target I'm going under the 1MB mark now.

Show off  ;)


> Given that I also applied the device table patch to mkcramfs (that
> allows for the creation of device nodes and arbitrary
> user/group/permission without being root) it would be possible to extend
> this mechanism to implement other XIP patterns such as for
> uncompressible media files for example.

Good, I was going to ask about that.

I made an example once were all the graphics were RAW and uncompressed 
and marked as XIP in AXFS. The result was a large saving of RAM because 
as the graphics framework (DirectFB) would copy directly from Flash 
whenever it needed to do a background erase or image re-draw (button press 
animations).

Same went for playing MP3 files. The MP3 files were XIP in flash, so 
mpg123 pulled from flash directly.


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> > Yes, now I can boot with my rootfs being a XIP cramfs.
> >
> > However, like you said, libc is not XIP.
> 
> I think I have it working now. Probably learned more about the memory
> management internals than I ever wanted to know. Please try the patch
> below on top of all the previous ones. If it works for you as well then
> I'll rebase and repost the whole thing.
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 4c7f01fcd2..0b651f985c 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c


Yes, that worked. Very nice!

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e23000-b6efc000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6efc000-b6f04000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f04000-b6f06000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f06000-b6f07000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f07000-b6f0a000 rw-p  00:00 0
b6f0a000-b6f21000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f24000-b6f25000 rw-p  00:00 0
b6f26000-b6f28000 rw-p  00:00 0
b6f28000-b6f29000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f29000-b6f2a000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
be877000-be898000 rw-p  00:00 0  [stack]
beba9000-bebaa000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Just FYI,
I'm running an xipImage with all the RZ/A1 upstream drivers enabled and 
only using about 4.5MB of total system RAM.
That's pretty good. Of course for a real application, you would trim off
the drivers and subsystems you don't plan on using, thus lowering your
RAM usage.


> +/*
> + * It is possible for cramfs_physmem_mmap() to partially populate the
> mapping
> + * causing page faults in the unmapped area. When that happens, we need
> to
> + * split the vma so that the unmapped area gets its own vma that can be
> backed
> + * with actual memory pages and loaded normally. This is necessary
> because
> + * remap_pfn_range() overwrites vma->vm_pgoff with the pfn and
> filemap_fault()
> + * no longer works with it. Furthermore this makes /proc/x/maps right.
> + * Q: is there a way to do split vma at mmap() time?
> + */


So if I understand correctly, the issue is that sometimes you only have 
a partial PAGE worth that you need to map. Correct?

For the AXFS file system, XIP page mapping was done on a per page 
decision, not per file. So the mkfs.axfs tool would only mark a page as XIP if
the entire section would fit in a complete PAGE. If for example you had
a partial page at the end of a multi page code segment, it would put 
that partial page in a separate portion of the AXFS image and be marked as
'copy to RAM' instead of being marked as 'map as XIP'.
So in the AXFS case, it was a combination of the creation tool and file 
system driver features to fix the partial page issue.
Not sure if any of this info is relevant, but I thought I would mention 
anyway.


Thank you for your efforts on adding XIP to cramfs!


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Wednesday, August 16, 2017, Nicolas Pitre wrote:
> > Yes, now I can boot with my rootfs being a XIP cramfs.
> >
> > However, like you said, libc is not XIP.
> 
> I think I have it working now. Probably learned more about the memory
> management internals than I ever wanted to know. Please try the patch
> below on top of all the previous ones. If it works for you as well then
> I'll rebase and repost the whole thing.
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 4c7f01fcd2..0b651f985c 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c


Yes, that worked. Very nice!

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6e23000-b6efc000 r-xp 1b0bc000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6efc000-b6f04000 ---p 1b195000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f04000-b6f06000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f06000-b6f07000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6f07000-b6f0a000 rw-p  00:00 0
b6f0a000-b6f21000 r-xp 1b0a4000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f24000-b6f25000 rw-p  00:00 0
b6f26000-b6f28000 rw-p  00:00 0
b6f28000-b6f29000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6f29000-b6f2a000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
be877000-be898000 rw-p  00:00 0  [stack]
beba9000-bebaa000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Just FYI,
I'm running an xipImage with all the RZ/A1 upstream drivers enabled and 
only using about 4.5MB of total system RAM.
That's pretty good. Of course for a real application, you would trim off
the drivers and subsystems you don't plan on using, thus lowering your
RAM usage.


> +/*
> + * It is possible for cramfs_physmem_mmap() to partially populate the
> mapping
> + * causing page faults in the unmapped area. When that happens, we need
> to
> + * split the vma so that the unmapped area gets its own vma that can be
> backed
> + * with actual memory pages and loaded normally. This is necessary
> because
> + * remap_pfn_range() overwrites vma->vm_pgoff with the pfn and
> filemap_fault()
> + * no longer works with it. Furthermore this makes /proc/x/maps right.
> + * Q: is there a way to do split vma at mmap() time?
> + */


So if I understand correctly, the issue is that sometimes you only have 
a partial PAGE worth that you need to map. Correct?

For the AXFS file system, XIP page mapping was done on a per page 
decision, not per file. So the mkfs.axfs tool would only mark a page as XIP if
the entire section would fit in a complete PAGE. If for example you had
a partial page at the end of a multi page code segment, it would put 
that partial page in a separate portion of the AXFS image and be marked as
'copy to RAM' instead of being marked as 'map as XIP'.
So in the AXFS case, it was a combination of the creation tool and file 
system driver features to fix the partial page issue.
Not sure if any of this info is relevant, but I thought I would mention 
anyway.


Thank you for your efforts on adding XIP to cramfs!


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Tuesday, August 15, 2017 1, Nicolas Pitre wrote:
> I was able to reproduce. The following patch on top should partially fix
> it.  I'm trying to figure out how to split a vma and link it properly in
> the case the vma cannot be mapped entirely. In the mean time shared libs
> won't be XIP.
> 
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 5aedbd224e..4c7f01fcd2 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c


Yes, now I can boot with my rootfs being a XIP cramfs.

However, like you said, libc is not XIP.

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6ed8000-b6fb1000 r-xp  00:0c 766540 /lib/libc-2.18-2013.10.so
b6fb1000-b6fb9000 ---p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fb9000-b6fbb000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fbb000-b6fbc000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fbc000-b6fbf000 rw-p  00:00 0
b6fbf000-b6fd6000 r-xp  00:0c 670372 /lib/ld-2.18-2013.10.so
b6fd9000-b6fda000 rw-p  00:00 0
b6fdb000-b6fdd000 rw-p  00:00 0
b6fdd000-b6fde000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6fde000-b6fdf000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
be81f000-be84 rw-p  00:00 0  [stack]
beb19000-beb1a000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Tuesday, August 15, 2017 1, Nicolas Pitre wrote:
> I was able to reproduce. The following patch on top should partially fix
> it.  I'm trying to figure out how to split a vma and link it properly in
> the case the vma cannot be mapped entirely. In the mean time shared libs
> won't be XIP.
> 
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 5aedbd224e..4c7f01fcd2 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c


Yes, now I can boot with my rootfs being a XIP cramfs.

However, like you said, libc is not XIP.

$ cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:0c 18192  /bin/busybox
000a9000-000aa000 rw-p 00099000 00:0c 18192  /bin/busybox
000aa000-000ac000 rw-p  00:00 0  [heap]
b6ed8000-b6fb1000 r-xp  00:0c 766540 /lib/libc-2.18-2013.10.so
b6fb1000-b6fb9000 ---p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fb9000-b6fbb000 r--p 000d9000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fbb000-b6fbc000 rw-p 000db000 00:0c 766540 /lib/libc-2.18-2013.10.so
b6fbc000-b6fbf000 rw-p  00:00 0
b6fbf000-b6fd6000 r-xp  00:0c 670372 /lib/ld-2.18-2013.10.so
b6fd9000-b6fda000 rw-p  00:00 0
b6fdb000-b6fdd000 rw-p  00:00 0
b6fdd000-b6fde000 r--p 00016000 00:0c 670372 /lib/ld-2.18-2013.10.so
b6fde000-b6fdf000 rw-p 00017000 00:0c 670372 /lib/ld-2.18-2013.10.so
be81f000-be84 rw-p  00:00 0  [stack]
beb19000-beb1a000 r-xp  00:00 0  [sigpage]
-1000 r-xp  00:00 0  [vectors]


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Monday, August 14, 2017, Nicolas Pitre wrote:
> > However, now with your mkcramfs tool, I can no longer mount my cramfs
> > image as the rootfs on boot. I was able to do that before (ie, 30
> minutes
> > ago) when using the community mkcramfs (ie, 30 minutes ago).
> >
> > I get this:
> >
> > [1.712425] cramfs: checking physical address 0x1b00 for linear
> cramfs image
> > [1.720531] cramfs: linear cramfs image appears to be 15744 KB in
> size
> > [1.728656] VFS: Mounted root (cramfs_physmem filesystem) readonly on
> device 0:12.
> > [1.737062] devtmpfs: mounted
> > [1.741139] Freeing unused kernel memory: 48K
> > [1.745545] This architecture does not have kernel memory protection.
> > [1.760381] Starting init: /sbin/init exists but couldn't execute it
> (error -22)
> > [1.769685] Starting init: /bin/sh exists but couldn't execute it
> (error -14)
> 
> Is /sbin/init a link to busybox?

Yes.


> I suppose it just boots if you do mkcramfs without -X?

Correct. I just created another image and removed the "-X -X" when 
creating it. Now I can boot that image as my rootfs.
  (I'm using -X -X because I'm using a Cortex-A9 with MMU).


> If so could you share your non-working cramfs image with me?

I will send it (in a separate email)



Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Monday, August 14, 2017, Nicolas Pitre wrote:
> > However, now with your mkcramfs tool, I can no longer mount my cramfs
> > image as the rootfs on boot. I was able to do that before (ie, 30
> minutes
> > ago) when using the community mkcramfs (ie, 30 minutes ago).
> >
> > I get this:
> >
> > [1.712425] cramfs: checking physical address 0x1b00 for linear
> cramfs image
> > [1.720531] cramfs: linear cramfs image appears to be 15744 KB in
> size
> > [1.728656] VFS: Mounted root (cramfs_physmem filesystem) readonly on
> device 0:12.
> > [1.737062] devtmpfs: mounted
> > [1.741139] Freeing unused kernel memory: 48K
> > [1.745545] This architecture does not have kernel memory protection.
> > [1.760381] Starting init: /sbin/init exists but couldn't execute it
> (error -22)
> > [1.769685] Starting init: /bin/sh exists but couldn't execute it
> (error -14)
> 
> Is /sbin/init a link to busybox?

Yes.


> I suppose it just boots if you do mkcramfs without -X?

Correct. I just created another image and removed the "-X -X" when 
creating it. Now I can boot that image as my rootfs.
  (I'm using -X -X because I'm using a Cortex-A9 with MMU).


> If so could you share your non-working cramfs image with me?

I will send it (in a separate email)



Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Monday, August 14, 2017, Nicolas Pitre wrote:
> > I just applied the patches tried this simple test:
> >  - tested with a Renesas RZ/A1 (Cortex-A9...so it has an MMU).
> >  - I set the sticky bit for busybox before using mkcramfs
> 
> You need the newer mkcramfs I linked to in the documentation. With it
> you don't need to play tricks with the sticky bit anymore. However you
> need to specify -X twice (or just once for no-MMU targets) and it will
> make every ELF files XIPable automatically.

OK. Now I am getting bigger images that makes me think all the ELF files
are uncompressed.


> > However, at this point I'm not sure how I can confirm that the XIP
> > busybox actually executed as XIP or not.
> 
> Just use busybox's built-in cat command and dump the content of
> /proc/self/maps. You should see an offset that refers to a physical
> address within your cramfs image for those segments marked read-only and
> executable.

It works! Pretty cool.

$ /mnt/bin/busybox cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:10 18192  /mnt/bin/busybox

  (my cramfs flash image is at physical address 0x1B00)




However, now with your mkcramfs tool, I can no longer mount my cramfs 
image as the rootfs on boot. I was able to do that before (ie, 30 minutes 
ago) when using the community mkcramfs (ie, 30 minutes ago).

I get this:

[1.712425] cramfs: checking physical address 0x1b00 for linear cramfs 
image
[1.720531] cramfs: linear cramfs image appears to be 15744 KB in size
[1.728656] VFS: Mounted root (cramfs_physmem filesystem) readonly on device 
0:12.
[1.737062] devtmpfs: mounted
[1.741139] Freeing unused kernel memory: 48K
[1.745545] This architecture does not have kernel memory protection.
[1.760381] Starting init: /sbin/init exists but couldn't execute it (error 
-22)
[1.769685] Starting init: /bin/sh exists but couldn't execute it (error -14)
[1.776956] Kernel panic - not syncing: No working init found.  Try passing 
init= option to kernel. See Linux Documentation/admin-guide/init.rst for 
guidance.
[1.791192] CPU: 0 PID: 1 Comm: init Not tainted 
4.13.0-rc1-00014-g53182a0b7245 #667
[1.798959] Hardware name: Generic R7S72100 (Flattened Device Tree)
[1.805519] [] (unwind_backtrace) from [] 
(show_stack+0xb/0xc)
[1.813228] [] (show_stack) from [] (panic+0x6f/0x18c)
[1.820163] [] (panic) from [] (kernel_init+0x6b/0x98)
[1.827078] [] (kernel_init) from [] 
(ret_from_fork+0x11/0x20)
[1.834747] ---[ end Kernel panic - not syncing: No working init found.  Try 
passing init= option to kernel. See Linux Documentation/admin-guide/init.rst 
for guidance.


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Monday, August 14, 2017, Nicolas Pitre wrote:
> > I just applied the patches tried this simple test:
> >  - tested with a Renesas RZ/A1 (Cortex-A9...so it has an MMU).
> >  - I set the sticky bit for busybox before using mkcramfs
> 
> You need the newer mkcramfs I linked to in the documentation. With it
> you don't need to play tricks with the sticky bit anymore. However you
> need to specify -X twice (or just once for no-MMU targets) and it will
> make every ELF files XIPable automatically.

OK. Now I am getting bigger images that makes me think all the ELF files
are uncompressed.


> > However, at this point I'm not sure how I can confirm that the XIP
> > busybox actually executed as XIP or not.
> 
> Just use busybox's built-in cat command and dump the content of
> /proc/self/maps. You should see an offset that refers to a physical
> address within your cramfs image for those segments marked read-only and
> executable.

It works! Pretty cool.

$ /mnt/bin/busybox cat /proc/self/maps
8000-000a1000 r-xp 1b005000 00:10 18192  /mnt/bin/busybox

  (my cramfs flash image is at physical address 0x1B00)




However, now with your mkcramfs tool, I can no longer mount my cramfs 
image as the rootfs on boot. I was able to do that before (ie, 30 minutes 
ago) when using the community mkcramfs (ie, 30 minutes ago).

I get this:

[1.712425] cramfs: checking physical address 0x1b00 for linear cramfs 
image
[1.720531] cramfs: linear cramfs image appears to be 15744 KB in size
[1.728656] VFS: Mounted root (cramfs_physmem filesystem) readonly on device 
0:12.
[1.737062] devtmpfs: mounted
[1.741139] Freeing unused kernel memory: 48K
[1.745545] This architecture does not have kernel memory protection.
[1.760381] Starting init: /sbin/init exists but couldn't execute it (error 
-22)
[1.769685] Starting init: /bin/sh exists but couldn't execute it (error -14)
[1.776956] Kernel panic - not syncing: No working init found.  Try passing 
init= option to kernel. See Linux Documentation/admin-guide/init.rst for 
guidance.
[1.791192] CPU: 0 PID: 1 Comm: init Not tainted 
4.13.0-rc1-00014-g53182a0b7245 #667
[1.798959] Hardware name: Generic R7S72100 (Flattened Device Tree)
[1.805519] [] (unwind_backtrace) from [] 
(show_stack+0xb/0xc)
[1.813228] [] (show_stack) from [] (panic+0x6f/0x18c)
[1.820163] [] (panic) from [] (kernel_init+0x6b/0x98)
[1.827078] [] (kernel_init) from [] 
(ret_from_fork+0x11/0x20)
[1.834747] ---[ end Kernel panic - not syncing: No working init found.  Try 
passing init= option to kernel. See Linux Documentation/admin-guide/init.rst 
for guidance.


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Friday, August 11, 2017, Nicolas Pitre wrote:
> This series brings a nice refresh to the cramfs filesystem, adding the
> following capabilities:
> 
> - Direct memory access, bypassing the block and/or MTD layers entirely.
> 
> - Ability to store individual data blocks uncompressed.
> 
> - Ability to locate individual data blocks anywhere in the filesystem.
> 
> The end result is a very tight filesystem that can be accessed directly
> from ROM without any other subsystem underneath. Also this allows for
> user space XIP which is a very important feature for tiny embedded
> systems.



I just applied the patches tried this simple test:
 - tested with a Renesas RZ/A1 (Cortex-A9...so it has an MMU).
 - I set the sticky bit for busybox before using mkcramfs
 - booted (with squashfs) and mounted the cramfs image
 - confirmed that the sticky bit was still set on busybox
 - was able to execute busybox in the cramfs image


However, at this point I'm not sure how I can confirm that the XIP 
busybox actually executed as XIP or not.


Chris

RE: [PATCH 0/5] cramfs refresh for embedded usage

[Chris Brandt]

On Friday, August 11, 2017, Nicolas Pitre wrote:
> This series brings a nice refresh to the cramfs filesystem, adding the
> following capabilities:
> 
> - Direct memory access, bypassing the block and/or MTD layers entirely.
> 
> - Ability to store individual data blocks uncompressed.
> 
> - Ability to locate individual data blocks anywhere in the filesystem.
> 
> The end result is a very tight filesystem that can be accessed directly
> from ROM without any other subsystem underneath. Also this allows for
> user space XIP which is a very important feature for tiny embedded
> systems.



I just applied the patches tried this simple test:
 - tested with a Renesas RZ/A1 (Cortex-A9...so it has an MMU).
 - I set the sticky bit for busybox before using mkcramfs
 - booted (with squashfs) and mounted the cramfs image
 - confirmed that the sticky bit was still set on busybox
 - was able to execute busybox in the cramfs image


However, at this point I'm not sure how I can confirm that the XIP 
busybox actually executed as XIP or not.


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hello Jacopo and Linus,

On Monday, May 29, 2017, jmondi wrote:
> > > We can handle 'bi-directional' pins with static tables in our pin
> > > controller driver and not have it anywhere in DT.
> >
> > This sounds like a viable approach.
> >
> > I just want to know if "output-enable" is the right name?
> > "output-buffer-enable"?
> 
> Great! Thanks!
> 
> On naming: if we need "output-buffer-enable" should we add
> "input-buffer-enable" as well?
> 
> Currently we are using "input-enable" to pair with "output-enable",
> but as you said, just "output-enable" when "output-high" and
> "output-low" are there already seems a bit confusing.
> At the same time "input-buffer-enable" seems to actually be just
> electrically equivalent to "input-enable", so adding it is a bit of a
> waste as well.

Here is what I think:


In the case of this driver, after we remove the 'bi-directional'
properties and hide the other odd-ball pin configurations in an internal
table, we are left with the MTU2 timer pins that can be either input or
output depending on what you want to do with them.

 * If you want to use a MTU2 channel as a PWM, you set the pin as an
   output.
 * If you want to use a MTU2 channel as a input capture, you set the
   pin as an input.

They are simply "direction-input" and "direction-output" properties that
don't really need to talk about "buffers".


But, instead of making any new properties, for the Renesas driver, let's
just stick with what already exists today: 
 * If you want a MTU2 channel as a PWM: select "output-low"
 * If you want a MTU2 channel as a input capture: select "input-enable"


Side Note: You can also use output-high in addition to output-low
  because it doesn't matter (the driver can't set the pin level anyway
  because as soon as you assign the pin to MTU2, the MTU2 controls the
  pin, not the PFC). So the Renesas driver can check for both.



Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hello Jacopo and Linus,

On Monday, May 29, 2017, jmondi wrote:
> > > We can handle 'bi-directional' pins with static tables in our pin
> > > controller driver and not have it anywhere in DT.
> >
> > This sounds like a viable approach.
> >
> > I just want to know if "output-enable" is the right name?
> > "output-buffer-enable"?
> 
> Great! Thanks!
> 
> On naming: if we need "output-buffer-enable" should we add
> "input-buffer-enable" as well?
> 
> Currently we are using "input-enable" to pair with "output-enable",
> but as you said, just "output-enable" when "output-high" and
> "output-low" are there already seems a bit confusing.
> At the same time "input-buffer-enable" seems to actually be just
> electrically equivalent to "input-enable", so adding it is a bit of a
> waste as well.

Here is what I think:


In the case of this driver, after we remove the 'bi-directional'
properties and hide the other odd-ball pin configurations in an internal
table, we are left with the MTU2 timer pins that can be either input or
output depending on what you want to do with them.

 * If you want to use a MTU2 channel as a PWM, you set the pin as an
   output.
 * If you want to use a MTU2 channel as a input capture, you set the
   pin as an input.

They are simply "direction-input" and "direction-output" properties that
don't really need to talk about "buffers".


But, instead of making any new properties, for the Renesas driver, let's
just stick with what already exists today: 
 * If you want a MTU2 channel as a PWM: select "output-low"
 * If you want a MTU2 channel as a input capture: select "input-enable"


Side Note: You can also use output-high in addition to output-low
  because it doesn't matter (the driver can't set the pin level anyway
  because as soon as you assign the pin to MTU2, the MTU2 controls the
  pin, not the PFC). So the Renesas driver can check for both.



Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hi Geert,

On Friday, May 12, 2017, Geert Uytterhoeven wrote:
> 12 x 16 = 192, not 384.

Opps, my math was off!

  (I think I need another cup of coffee this morning)


> Do you need all possible combinations of input, output, and bi-dir?
> I assumed they're mutually exclusive. If not, you need 3 bits/pin/function

You're right, they are mutually exclusive, so 2 bits are fine.

Also, the other pinout variations chips (RZ/A1L,A1LU,A1LC) only have 10 ports:

 12 ports x 16 pins x 2bits-per-bit => 384 bytes
 10 ports x 16 pins x 2bits-per-bit => 320 bytes

 384 + 320 = 704 bytes (of const data)


> With packages, do you mean e.g. RZ/A1H vs. RZ/A1L?

Yes.


> These indeed differ,
> but
> should use different compatible values.

OK. Since r7s72100.dtsi has
compatible = "renesas,r7s72100-ports";

We'll just override that in say my-rza1l-board.dts

 {
compatible = "renesas,r7s72102-ports";
};

  and then look for that in the driver.


> Or do you mean QFP/BGA256 vs. BGA324? Isn't the former a subset of the
> latter?

The "package" doesn't matter, it's the pin assignments on the die:
  RZ/A1H = RZ/A1M = pin assignments #1
  RZ/A1L = RZ/A1LU = RZ/A1LC = pin assignments #2

Then each of those parts above have multiple 'package options', but of
course doesn't change the actual pin/function assignments (that's part
of the silicon die)

Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hi Geert,

On Friday, May 12, 2017, Geert Uytterhoeven wrote:
> 12 x 16 = 192, not 384.

Opps, my math was off!

  (I think I need another cup of coffee this morning)


> Do you need all possible combinations of input, output, and bi-dir?
> I assumed they're mutually exclusive. If not, you need 3 bits/pin/function

You're right, they are mutually exclusive, so 2 bits are fine.

Also, the other pinout variations chips (RZ/A1L,A1LU,A1LC) only have 10 ports:

 12 ports x 16 pins x 2bits-per-bit => 384 bytes
 10 ports x 16 pins x 2bits-per-bit => 320 bytes

 384 + 320 = 704 bytes (of const data)


> With packages, do you mean e.g. RZ/A1H vs. RZ/A1L?

Yes.


> These indeed differ,
> but
> should use different compatible values.

OK. Since r7s72100.dtsi has
compatible = "renesas,r7s72100-ports";

We'll just override that in say my-rza1l-board.dts

 {
compatible = "renesas,r7s72102-ports";
};

  and then look for that in the driver.


> Or do you mean QFP/BGA256 vs. BGA324? Isn't the former a subset of the
> latter?

The "package" doesn't matter, it's the pin assignments on the die:
  RZ/A1H = RZ/A1M = pin assignments #1
  RZ/A1L = RZ/A1LU = RZ/A1LC = pin assignments #2

Then each of those parts above have multiple 'package options', but of
course doesn't change the actual pin/function assignments (that's part
of the silicon die)

Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hi Geert and Linus,

On Friday, May 12, 2017, Geert Uytterhoeven wrote:
> Jacopo, Chris: Would two bits per pin/function (none, input, output,
> bidir)
> be sufficient?
> That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes.
> Plus code to handle it. After all not that bad...

OK...I give up!
If that's what it takes to get it, I'm fine.

NOTE, your math is a little off, the issue is that depending on the
function that you use, you might need to do extra settings, so you'd
have to have a lookup table for every pin & function.
Each pin can have 1 of 8 functions (which is good because a 'byte' has
8 bits).

So,
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
bi-dir is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
input is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
input is needed)
 
 1,152 bytes

But then...there are package variations so you need another entire
table for those parts.
   1,152 bytes x 2 = 2,304 bytes

However, if these tables are constants, they will reside in flash for the
XIP_KERNEL systems, so that's OK.

#What we should really do is just make a look-up table (tables) for the
'special' ones. But, we can have that discussion in a different thread.



One final note:

There is still a need for "input-enable" and "output-enable" for the timer
pins. Because, when you choose the pin to be connected to the MTU2 timer,
the pin can be used as either input-capture/output-compare/PWM and that's
the user's choice. So that's probably a valid usage of the generic pin
properties for configuration.


Sorry Jacopo, but we'll need another round of patches.
It sounds like for sure the bi-direction needs to get ripped back out.


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hi Geert and Linus,

On Friday, May 12, 2017, Geert Uytterhoeven wrote:
> Jacopo, Chris: Would two bits per pin/function (none, input, output,
> bidir)
> be sufficient?
> That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes.
> Plus code to handle it. After all not that bad...

OK...I give up!
If that's what it takes to get it, I'm fine.

NOTE, your math is a little off, the issue is that depending on the
function that you use, you might need to do extra settings, so you'd
have to have a lookup table for every pin & function.
Each pin can have 1 of 8 functions (which is good because a 'byte' has
8 bits).

So,
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
bi-dir is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
input is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if 
input is needed)
 
 1,152 bytes

But then...there are package variations so you need another entire
table for those parts.
   1,152 bytes x 2 = 2,304 bytes

However, if these tables are constants, they will reside in flash for the
XIP_KERNEL systems, so that's OK.

#What we should really do is just make a look-up table (tables) for the
'special' ones. But, we can have that discussion in a different thread.



One final note:

There is still a need for "input-enable" and "output-enable" for the timer
pins. Because, when you choose the pin to be connected to the MTU2 timer,
the pin can be used as either input-capture/output-compare/PWM and that's
the user's choice. So that's probably a valid usage of the generic pin
properties for configuration.


Sorry Jacopo, but we'll need another round of patches.
It sounds like for sure the bi-direction needs to get ripped back out.


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, May 12, 2017, linux-renesas-soc-ow...@vger.kernel.org wrote:
> As you say this is actually fixing hardware bugs, we can expect these
> quirky tables to be gone in the next hardware generation, right?

I see this particular pin controller as a one shot deal. For the next
part in this series we are moving to a completely different controller
which you could probably get away with simply using pinctrl-single.


> Then the right place for it is in the quirky driver for the quirky
> first-generation
> hardware.

Maybe to be more clear, I would say the design is not a 'bug', but
rather an under sight of the original designers where are they stared
to need pins that would be both in and out, they started tacking on
more hardware.


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, May 12, 2017, linux-renesas-soc-ow...@vger.kernel.org wrote:
> As you say this is actually fixing hardware bugs, we can expect these
> quirky tables to be gone in the next hardware generation, right?

I see this particular pin controller as a one shot deal. For the next
part in this series we are moving to a completely different controller
which you could probably get away with simply using pinctrl-single.


> Then the right place for it is in the quirky driver for the quirky
> first-generation
> hardware.

Maybe to be more clear, I would say the design is not a 'bug', but
rather an under sight of the original designers where are they stared
to need pins that would be both in and out, they started tacking on
more hardware.


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hello Andy,

On Monday, May 08, 2017, Andy Shevchenko wrote:
> > Bi-Directional:
> >
> > For any pin that needs to drive (send) or sense (receive) signals, the
> pin
> > mux controller can only enable 1 direction of buffers (in this case, it
> > only the output buffers). Therefore the appropriate bit in the
> > 'bi-directional' register is set in order to enable the signal path in
> both
> > directions (ie, enable the input buffers).
> 
> So, I see this way how it can be enabled:
> 1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set
> 2. BiDi bit is set and BUFOE is set
> 
> Now the question is what the real use case for 2?

For #1, IP_ENI and IP_ENO are controlled by PFC/PFCE/PFCAE.
Those basically equate to the pin function register (as in,
what IP block gets wired up to each pin.) However, it
seems that PFC/PFCE/PFCAE cannot enable both IP_ENI and
IP_ENO signals at the same time (the diagram doesn't really
show you that piece of info), hence they had to make an
'override' register can called it PBDC (bir-dir register)
to manually turn the input buffers on when needed.

Seems like a HW hack if you ask me.


> If we find one we need to rename and fix a description of the pin
> control configuration property.
> 
> like:
> @PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.
> ...
> Note: As long as it's enabled the pin's input enabled as well and vice
> versa.

So your suggestion is to rename PIN_CONFIG_OUTPUT to
PIN_CONFIG_OUTPUT_INPUT_ENABLE?

I would say the description for @PIN_CONFIG_INPUT_ENABLE is probably
'technically' correct for our bi-dir needs, but I didn't like it
because it might confuse users making a DT file for their board (unless
of course they studied the hardware manual in detail to finally come
to the conclusion of the screwy PFC hardware).


> >> >  and SWIO_[INPUT|OUTPUT] directly there?
> >
> > In the hardware manual, there is a list of pin functions that if you
> want
> > to use, you cannot use the stand pinmux register method that you use for
> > all the other pins. Instead, you are instructed to do a different
> > procedure. If course electrically, input/output buffers are still turned
> > on/off or whatever, but the root reason of why you need to do this
> > differently for these specific pin/function is not known.
> >
> > The "SWIO_" portion of the naming comes from the hardware manual which
> > refers to this as "Software I/O Control Alternative Mode" (which in my
> > opinion means the HW guys couldn't get the pin directions/buffers to be
> set
> > automatically for some reason, so they decided it's the SW guys problem
> now
> > for those pins)
> 
> Okay, these are related to pin muxing explicitly.
> So, you have 10 functions overall?
> What prevents you describe them accordingly and hide this
> implementation detail in the driver?

The one issue I was trying to avoid by hiding things in the driver
with some type of look-up table was that this series of parts comes
in different subsets/packages and sometimes the functions comes out
on different port numbers. So now you need multiple look-up tables
and then also a way to signal what part/package you have so you use
the correct look-up table. It seemed easier (and safer) to just pass
the info in (if you needed it) in the Device Tree for the board.

Of these 'special' pins (Table 54.7):

For 16 of them, they truly can operate as input or output, so
the user needs to specify that direction they need in the DT.

For LVDS, Sound and WDT, those will be fixed, so they would be
the only ones you could do a table for, but as I mentioned, Sound
and WDT don't always come out in the same place so a lookup table
isn't so cut and dry.


> >> Second question, what makes it differ to what already exists?
> >
> > We have 3 different flags (properties) that need to be specified for
> some
> > pins in some circumstances.
> > At first, we just tried to pass this additional information in when
> > defining what function the pin should be just for those pins whose
> > direction (ie, buffers) would not be set up automatically.
> > However, this was rejected and we were told to pick from the existing
> set
> > generic properties.
> > But, 3 different generic pinctrl properties that fit what we needed
> didn't
> > exist. So, we used the existing "input-enable" and "output-enable", but
> > then created "bi-directional".
> 
> Yes, that figure helped me a lot to understand.

I see from your other email you sent to Jacopo, it looks like the link
I sent you only brought you to the shorter 'data sheet' version of the
hardware manual, not the full manual that includes 'Figure 54.1'.
Sorry about that.



Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

Hello Andy,

On Monday, May 08, 2017, Andy Shevchenko wrote:
> > Bi-Directional:
> >
> > For any pin that needs to drive (send) or sense (receive) signals, the
> pin
> > mux controller can only enable 1 direction of buffers (in this case, it
> > only the output buffers). Therefore the appropriate bit in the
> > 'bi-directional' register is set in order to enable the signal path in
> both
> > directions (ie, enable the input buffers).
> 
> So, I see this way how it can be enabled:
> 1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set
> 2. BiDi bit is set and BUFOE is set
> 
> Now the question is what the real use case for 2?

For #1, IP_ENI and IP_ENO are controlled by PFC/PFCE/PFCAE.
Those basically equate to the pin function register (as in,
what IP block gets wired up to each pin.) However, it
seems that PFC/PFCE/PFCAE cannot enable both IP_ENI and
IP_ENO signals at the same time (the diagram doesn't really
show you that piece of info), hence they had to make an
'override' register can called it PBDC (bir-dir register)
to manually turn the input buffers on when needed.

Seems like a HW hack if you ask me.


> If we find one we need to rename and fix a description of the pin
> control configuration property.
> 
> like:
> @PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.
> ...
> Note: As long as it's enabled the pin's input enabled as well and vice
> versa.

So your suggestion is to rename PIN_CONFIG_OUTPUT to
PIN_CONFIG_OUTPUT_INPUT_ENABLE?

I would say the description for @PIN_CONFIG_INPUT_ENABLE is probably
'technically' correct for our bi-dir needs, but I didn't like it
because it might confuse users making a DT file for their board (unless
of course they studied the hardware manual in detail to finally come
to the conclusion of the screwy PFC hardware).


> >> >  and SWIO_[INPUT|OUTPUT] directly there?
> >
> > In the hardware manual, there is a list of pin functions that if you
> want
> > to use, you cannot use the stand pinmux register method that you use for
> > all the other pins. Instead, you are instructed to do a different
> > procedure. If course electrically, input/output buffers are still turned
> > on/off or whatever, but the root reason of why you need to do this
> > differently for these specific pin/function is not known.
> >
> > The "SWIO_" portion of the naming comes from the hardware manual which
> > refers to this as "Software I/O Control Alternative Mode" (which in my
> > opinion means the HW guys couldn't get the pin directions/buffers to be
> set
> > automatically for some reason, so they decided it's the SW guys problem
> now
> > for those pins)
> 
> Okay, these are related to pin muxing explicitly.
> So, you have 10 functions overall?
> What prevents you describe them accordingly and hide this
> implementation detail in the driver?

The one issue I was trying to avoid by hiding things in the driver
with some type of look-up table was that this series of parts comes
in different subsets/packages and sometimes the functions comes out
on different port numbers. So now you need multiple look-up tables
and then also a way to signal what part/package you have so you use
the correct look-up table. It seemed easier (and safer) to just pass
the info in (if you needed it) in the Device Tree for the board.

Of these 'special' pins (Table 54.7):

For 16 of them, they truly can operate as input or output, so
the user needs to specify that direction they need in the DT.

For LVDS, Sound and WDT, those will be fixed, so they would be
the only ones you could do a table for, but as I mentioned, Sound
and WDT don't always come out in the same place so a lookup table
isn't so cut and dry.


> >> Second question, what makes it differ to what already exists?
> >
> > We have 3 different flags (properties) that need to be specified for
> some
> > pins in some circumstances.
> > At first, we just tried to pass this additional information in when
> > defining what function the pin should be just for those pins whose
> > direction (ie, buffers) would not be set up automatically.
> > However, this was rejected and we were told to pick from the existing
> set
> > generic properties.
> > But, 3 different generic pinctrl properties that fit what we needed
> didn't
> > exist. So, we used the existing "input-enable" and "output-enable", but
> > then created "bi-directional".
> 
> Yes, that figure helped me a lot to understand.

I see from your other email you sent to Jacopo, it looks like the link
I sent you only brought you to the shorter 'data sheet' version of the
hardware manual, not the full manual that includes 'Figure 54.1'.
Sorry about that.



Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Monday, May 08, 2017, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 7:01 PM, jmondi  wrote:
> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >>  wrote:
> >>
> >> > Linus, for me it looks like better to revert that change, until we
> >> > will have clear picture why existing configuration parameters can't
> >> > work.
> >>
> >> Yeah I'll revert the binding for fixes.
> 
> > As it seems we won't be able to proceed with the currently proposed
> solution,
> > would that be acceptable now that we use the "pinmux" property to add
> > flags as BIDIR
> 
> Can you explain what does this *electrically* mean?

Bi-Directional:

For any pin that needs to drive (send) or sense (receive) signals, the pin
mux controller can only enable 1 direction of buffers (in this case, it
only the output buffers). Therefore the appropriate bit in the
'bi-directional' register is set in order to enable the signal path in both
directions (ie, enable the input buffers).


> >  and SWIO_[INPUT|OUTPUT] directly there?

In the hardware manual, there is a list of pin functions that if you want
to use, you cannot use the stand pinmux register method that you use for
all the other pins. Instead, you are instructed to do a different
procedure. If course electrically, input/output buffers are still turned
on/off or whatever, but the root reason of why you need to do this
differently for these specific pin/function is not known.

The "SWIO_" portion of the naming comes from the hardware manual which
refers to this as "Software I/O Control Alternative Mode" (which in my
opinion means the HW guys couldn't get the pin directions/buffers to be set
automatically for some reason, so they decided it's the SW guys problem now
for those pins)


> Second question, what makes it differ to what already exists?

We have 3 different flags (properties) that need to be specified for some
pins in some circumstances.
At first, we just tried to pass this additional information in when
defining what function the pin should be just for those pins whose
direction (ie, buffers) would not be set up automatically.
However, this was rejected and we were told to pick from the existing set
generic properties.
But, 3 different generic pinctrl properties that fit what we needed didn't
exist. So, we used the existing "input-enable" and "output-enable", but
then created "bi-directional".


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Monday, May 08, 2017, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 7:01 PM, jmondi  wrote:
> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >>  wrote:
> >>
> >> > Linus, for me it looks like better to revert that change, until we
> >> > will have clear picture why existing configuration parameters can't
> >> > work.
> >>
> >> Yeah I'll revert the binding for fixes.
> 
> > As it seems we won't be able to proceed with the currently proposed
> solution,
> > would that be acceptable now that we use the "pinmux" property to add
> > flags as BIDIR
> 
> Can you explain what does this *electrically* mean?

Bi-Directional:

For any pin that needs to drive (send) or sense (receive) signals, the pin
mux controller can only enable 1 direction of buffers (in this case, it
only the output buffers). Therefore the appropriate bit in the
'bi-directional' register is set in order to enable the signal path in both
directions (ie, enable the input buffers).


> >  and SWIO_[INPUT|OUTPUT] directly there?

In the hardware manual, there is a list of pin functions that if you want
to use, you cannot use the stand pinmux register method that you use for
all the other pins. Instead, you are instructed to do a different
procedure. If course electrically, input/output buffers are still turned
on/off or whatever, but the root reason of why you need to do this
differently for these specific pin/function is not known.

The "SWIO_" portion of the naming comes from the hardware manual which
refers to this as "Software I/O Control Alternative Mode" (which in my
opinion means the HW guys couldn't get the pin directions/buffers to be set
automatically for some reason, so they decided it's the SW guys problem now
for those pins)


> Second question, what makes it differ to what already exists?

We have 3 different flags (properties) that need to be specified for some
pins in some circumstances.
At first, we just tried to pass this additional information in when
defining what function the pin should be just for those pins whose
direction (ie, buffers) would not be set up automatically.
However, this was rejected and we were told to pick from the existing set
generic properties.
But, 3 different generic pinctrl properties that fit what we needed didn't
exist. So, we used the existing "input-enable" and "output-enable", but
then created "bi-directional".


Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, May 05, 2017, Linus Walleij wrote:
> > This is the part of the whole "DT is for hardware description only" that
> doesn't really make sense to me.
> 
> OK yeah we do hardware description AND configuration.
> 
> And we never do interpreted languages.
> 
> And then there is a bunch of grayzone things. For example we have a
> linux,input binding for connecting keypresses to certain Linux input codes.
> That is really grayzone, but very useful.

Ah

compatible = "linux,grayzone";


Thanks for the reply. I'll stop ranting now.
Of course, I'll still probably screw it up again on a future patch somehow...

Cheers

Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, May 05, 2017, Linus Walleij wrote:
> > This is the part of the whole "DT is for hardware description only" that
> doesn't really make sense to me.
> 
> OK yeah we do hardware description AND configuration.
> 
> And we never do interpreted languages.
> 
> And then there is a bunch of grayzone things. For example we have a
> linux,input binding for connecting keypresses to certain Linux input codes.
> That is really grayzone, but very useful.

Ah

compatible = "linux,grayzone";


Thanks for the reply. I'll stop ranting now.
Of course, I'll still probably screw it up again on a future patch somehow...

Cheers

Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > We were using "input-enable" to signal when the pin function that we set
> also needs to be forcible set to input by the software (once again,
> because the HW is not smart enough to do it on its own), but is different
> than the bi-directional functionality (ie, a different register setting).
> 
> You are trying to introduce an abstraction, called BiDi, which is
> *not* a separate thing from a set of pin properties.

Note, I'm talking about 2 different issues we had:

1) Pins that need input and output buffers enabled during normal use. We 
created "bi-directional" for that.

2) For whatever reason, the HW manual points out that the PFC hardware can't 
really automatically set buffers enables correctly for some pin instances, and 
we have to manually assign the pin as input or output using another register. 
For that, we were using "input-enable" and "output-enable".


For #2:

> > Note that we added a enable-output for the same reason.
> > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that
> PIPCn.PIPCnm Bit Should be Set to 0"
> 
> Perhaps needs to be revisited as well.

Sorry, we didn't 'add' anything new. The property "output-enable", (ie, 
PIN_CONFIG_OUTPUT) already existed and describes what we are doing in the case 
for output.

But, we still have the issue that we have 2 cases that need the input enabled, 
but they are not the same situation, so we can't just use "input-enable" for 
both.


My only suggestion is (and I'm not sure this is possible in the driver):

"input-enable"  : case #2 where you need the pin to be forced as an input
"output-enable" : case #2 where you need the pin to be forced as an output
"input-enable" + "output-enable" : case #1 (replaces "bi-directional").

For example:

i2c2_pins: i2c2 {
pinmux = , ;
input-enable;
output-enable;
};

So in the SW driver, if we see both, that will signal to us what is going on 
and what to do about it (as in, set the bi-directional register and not the 
input direction register).


Thoughts?


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > We were using "input-enable" to signal when the pin function that we set
> also needs to be forcible set to input by the software (once again,
> because the HW is not smart enough to do it on its own), but is different
> than the bi-directional functionality (ie, a different register setting).
> 
> You are trying to introduce an abstraction, called BiDi, which is
> *not* a separate thing from a set of pin properties.

Note, I'm talking about 2 different issues we had:

1) Pins that need input and output buffers enabled during normal use. We 
created "bi-directional" for that.

2) For whatever reason, the HW manual points out that the PFC hardware can't 
really automatically set buffers enables correctly for some pin instances, and 
we have to manually assign the pin as input or output using another register. 
For that, we were using "input-enable" and "output-enable".


For #2:

> > Note that we added a enable-output for the same reason.
> > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that
> PIPCn.PIPCnm Bit Should be Set to 0"
> 
> Perhaps needs to be revisited as well.

Sorry, we didn't 'add' anything new. The property "output-enable", (ie, 
PIN_CONFIG_OUTPUT) already existed and describes what we are doing in the case 
for output.

But, we still have the issue that we have 2 cases that need the input enabled, 
but they are not the same situation, so we can't just use "input-enable" for 
both.


My only suggestion is (and I'm not sure this is possible in the driver):

"input-enable"  : case #2 where you need the pin to be forced as an input
"output-enable" : case #2 where you need the pin to be forced as an output
"input-enable" + "output-enable" : case #1 (replaces "bi-directional").

For example:

i2c2_pins: i2c2 {
pinmux = , ;
input-enable;
output-enable;
};

So in the SW driver, if we see both, that will signal to us what is going on 
and what to do about it (as in, set the bi-directional register and not the 
input direction register).


Thoughts?


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> Had you read the following, esp. Note there?
> 
> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
> not
> *  affect the pin's ability to drive output.  1 enables input, 0
> disables
> *  input.
> 
> For me manual is clearly tells about this settings:
> "This register enables or disables the input buffer while the output
> buffer is enabled."


But, then if we use "input-enable" to get bi-directional functionality, now we 
need something to replace what we were using "input-enable" for.
We were using "input-enable" to signal when the pin function that we set also 
needs to be forcible set to input by the software (once again, because the HW 
is not smart enough to do it on its own), but is different than the 
bi-directional functionality (ie, a different register setting).


So, if we replace "bi-directional" with "input-enable" (since logically 
internally that is what is going on), what do we use for the special pins that 
the HW manual says "hey, you need to manually set these pins to input with SW 
because the pin selection HW can't do it correctly)". Note that we added a 
enable-output for the same reason.
See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that 
PIPCn.PIPCnm Bit Should be Set to 0"


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> Had you read the following, esp. Note there?
> 
> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
> not
> *  affect the pin's ability to drive output.  1 enables input, 0
> disables
> *  input.
> 
> For me manual is clearly tells about this settings:
> "This register enables or disables the input buffer while the output
> buffer is enabled."


But, then if we use "input-enable" to get bi-directional functionality, now we 
need something to replace what we were using "input-enable" for.
We were using "input-enable" to signal when the pin function that we set also 
needs to be forcible set to input by the software (once again, because the HW 
is not smart enough to do it on its own), but is different than the 
bi-directional functionality (ie, a different register setting).


So, if we replace "bi-directional" with "input-enable" (since logically 
internally that is what is going on), what do we use for the special pins that 
the HW manual says "hey, you need to manually set these pins to input with SW 
because the pin selection HW can't do it correctly)". Note that we added a 
enable-output for the same reason.
See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that 
PIPCn.PIPCnm Bit Should be Set to 0"


Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Friday, April 28, 2017, Geert Uytterhoeven wrote:
> >> Shouldn't the interrupt (connected to P1_15) be described?
> >
> > That interrupt pin from the PHY is not used. It did not need to be
> connected.
> 
> But it is connected, according to the schematics.

P1_15 can be configured as:
 * GPIO_IN
 * AN7
 * AVB_CAPTURE

So...I guess you would 'describe' it as an GPIO-input.


> DT describes hardware, not software limitations.

Describing things is fine, but the kernel code takes the DT and then start 
configuring things based on it.

For example, on the RSK board, that line is connected to P4_14. P4_14 can be 
configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might 
want to use one of those other choices, so I don't want to describe P4_14 as an 
interrupt.

If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of 
the RZ/A1H", that's fine because it's hardware connection that's not going to 
change.
But...the DT also defines the pin muxing...which is a software decision (do I 
want to get interrupt or just manually poll or simple ignore it).
This is the part of the whole "DT is for hardware description only" that 
doesn't really make sense to me.


Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Friday, April 28, 2017, Geert Uytterhoeven wrote:
> >> Shouldn't the interrupt (connected to P1_15) be described?
> >
> > That interrupt pin from the PHY is not used. It did not need to be
> connected.
> 
> But it is connected, according to the schematics.

P1_15 can be configured as:
 * GPIO_IN
 * AN7
 * AVB_CAPTURE

So...I guess you would 'describe' it as an GPIO-input.


> DT describes hardware, not software limitations.

Describing things is fine, but the kernel code takes the DT and then start 
configuring things based on it.

For example, on the RSK board, that line is connected to P4_14. P4_14 can be 
configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might 
want to use one of those other choices, so I don't want to describe P4_14 as an 
interrupt.

If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of 
the RZ/A1H", that's fine because it's hardware connection that's not going to 
change.
But...the DT also defines the pin muxing...which is a software decision (do I 
want to get interrupt or just manually poll or simple ignore it).
This is the part of the whole "DT is for hardware description only" that 
doesn't really make sense to me.


Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, April 28, 2017, Linus Walleij wrote:
> > Add pin configuration subnode for ETHER ethernet controller.
> >
> > Signed-off-by: Jacopo Mondi 
> (...)
> > +   pins_bidir {
> > +   pinmux = ;/* P3_3 =
> ET_MDIO  */
> > +   bi-directional;
> > +   };
> 
> So I'm against merging this until someone explains what "bi-directional"
> actually means, electrically speaking. What happens physically on this
> pin?
> 
> I think this just means open drain.
> 
> It is dangerous to merge things we don't understand.
> 
> Surely someone inside Renesas can answer this question.

I don't think this has anything to do with open drain because you need it for 
any pin that the peripheral IP block needs to transmit and receive over the 
same line, regardless of if it's a SDHI, I2C, Ethernet MDIO, etc...
It's more about of allowing the internal IP block signals to get hooked up to 
the IO pad signals.

Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, April 28, 2017, Linus Walleij wrote:
> > Add pin configuration subnode for ETHER ethernet controller.
> >
> > Signed-off-by: Jacopo Mondi 
> (...)
> > +   pins_bidir {
> > +   pinmux = ;/* P3_3 =
> ET_MDIO  */
> > +   bi-directional;
> > +   };
> 
> So I'm against merging this until someone explains what "bi-directional"
> actually means, electrically speaking. What happens physically on this
> pin?
> 
> I think this just means open drain.
> 
> It is dangerous to merge things we don't understand.
> 
> Surely someone inside Renesas can answer this question.

I don't think this has anything to do with open drain because you need it for 
any pin that the peripheral IP block needs to transmit and receive over the 
same line, regardless of if it's a SDHI, I2C, Ethernet MDIO, etc...
It's more about of allowing the internal IP block signals to get hooked up to 
the IO pad signals.

Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
> Logical Diagram (but that wasn't obvious to me at first).
> 
> Please, post a link to it or copy essential parts.


This board the RZ/A1 GENMAI board.
https://www.renesas.com/en-us/products/software-tools/boards-and-kits/evaluation-demo-solution-boards/genmai-cpu-board-rtk772100bc0br.html


The schematic is included in the "User's manual"
https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf


The RZ/A1H Hardware manual is here:
https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true=186374=RZ%252FA1H=document-prd-search=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf=54f335753742b5add524d4725b7242e6


Chapter 54 is the port/pin controller.

"54.18 Port Control Logical Diagram" is the diagram I was talking about. Note 
that is says "Note: This figure shows the logic for reference, not the circuit."

"54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register 
needed to get some pins to work.


> I'm quite skeptical  that cheap hardware can implement something more
> costable than simplest open-source / open-drain + bias

I don't think this is an open-source / open-drain + bias issue. It's a "the 
internal signal paths are not getting hooked up correctly" issue.


Regardless, on this part, we needed a way to flag that some pins when put in 
some function modes needed 'an extra register setting'. At first we tried to 
sneak that info in with a simple #define in the pin/pinmux DT node properties. 
But, Linus didn't want it there so we had to make up a new generic property 
called "bi-directional".

What is your end goal here? Get "bi-directional" changed to something else?


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
> Logical Diagram (but that wasn't obvious to me at first).
> 
> Please, post a link to it or copy essential parts.


This board the RZ/A1 GENMAI board.
https://www.renesas.com/en-us/products/software-tools/boards-and-kits/evaluation-demo-solution-boards/genmai-cpu-board-rtk772100bc0br.html


The schematic is included in the "User's manual"
https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf


The RZ/A1H Hardware manual is here:
https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true=186374=RZ%252FA1H=document-prd-search=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf=54f335753742b5add524d4725b7242e6


Chapter 54 is the port/pin controller.

"54.18 Port Control Logical Diagram" is the diagram I was talking about. Note 
that is says "Note: This figure shows the logic for reference, not the circuit."

"54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register 
needed to get some pins to work.


> I'm quite skeptical  that cheap hardware can implement something more
> costable than simplest open-source / open-drain + bias

I don't think this is an open-source / open-drain + bias issue. It's a "the 
internal signal paths are not getting hooked up correctly" issue.


Regardless, on this part, we needed a way to flag that some pins when put in 
some function modes needed 'an extra register setting'. At first we tried to 
sneak that info in with a simple #define in the pin/pinmux DT node properties. 
But, Linus didn't want it there so we had to make up a new generic property 
called "bi-directional".

What is your end goal here? Get "bi-directional" changed to something else?


Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Linus Walleij wrote:
> > For me it looks like you are trying to alias open-drain + bias or
> > alike. Don't actually see the benefit of it.
> 
> Andy is bringing up a valid point. And I remember asking about this before.
> 
> What does "bi-directional" really mean, electrically speaking?
> 
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of how open
> drain/source works.)
> 
> When you set an output without setting a value, what happens electrically?
> 
> Isn't this bias-high-impedance / High-Z?
> 
> Hopefully you can find the answer from Renesas hardware dept.
> 
> You can certainly call it whatever the datasheet calls it in your driver
> #defines but for the DT bindings we would ideally have the physical world
> things.

The main reason is this pin controller is too dumb to do what it's supposed to 
with 1 register setting.


Take the SDHI data pins. You send AND receive data over those pins (and they 
are not open drain). The issue is that the PFC HW that enables the connections 
between the SDHI IP block and the I/O pad buffers can only enable one 
path/signal/direction to the buffer enables (in or out). So for a pin that 
needs both directions, the PFC enables output and the "bidirectional register" 
is used to enable the input buffer as well.
In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical 
Diagram (but that wasn't obvious to me at first).

# side note, the way the registers are arranged is also ridiculous in my 
opinion. I'm not a fan of this particular IP.

The good news is the RZ/A1 is the only chip series I've seen with this PFC IP 
(I can't even figure out where it came from internally). And as far as I know, 
it will not appear in any other RZ series chips.



Chris

RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Chris Brandt]

On Friday, April 28, 2017, Linus Walleij wrote:
> > For me it looks like you are trying to alias open-drain + bias or
> > alike. Don't actually see the benefit of it.
> 
> Andy is bringing up a valid point. And I remember asking about this before.
> 
> What does "bi-directional" really mean, electrically speaking?
> 
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of how open
> drain/source works.)
> 
> When you set an output without setting a value, what happens electrically?
> 
> Isn't this bias-high-impedance / High-Z?
> 
> Hopefully you can find the answer from Renesas hardware dept.
> 
> You can certainly call it whatever the datasheet calls it in your driver
> #defines but for the DT bindings we would ideally have the physical world
> things.

The main reason is this pin controller is too dumb to do what it's supposed to 
with 1 register setting.


Take the SDHI data pins. You send AND receive data over those pins (and they 
are not open drain). The issue is that the PFC HW that enables the connections 
between the SDHI IP block and the I/O pad buffers can only enable one 
path/signal/direction to the buffer enables (in or out). So for a pin that 
needs both directions, the PFC enables output and the "bidirectional register" 
is used to enable the input buffer as well.
In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical 
Diagram (but that wasn't obvious to me at first).

# side note, the way the registers are arranged is also ridiculous in my 
opinion. I'm not a fan of this particular IP.

The good news is the RZ/A1 is the only chip series I've seen with this PFC IP 
(I can't even figure out where it came from internally). And as far as I know, 
it will not appear in any other RZ series chips.



Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > + {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_pins>;
> > +
> > +   status = "okay";
> > +
> > +   renesas,no-ether-link;
> > +   phy-handle = <>;
> > +   phy0: ethernet-phy@0 {
> > +   reg = <0>;
> 
> Shouldn't the interrupt (connected to P1_15) be described?


That interrupt pin from the PHY is not used. It did not need to be connected.


Chris

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > + {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_pins>;
> > +
> > +   status = "okay";
> > +
> > +   renesas,no-ether-link;
> > +   phy-handle = <>;
> > +   phy0: ethernet-phy@0 {
> > +   reg = <0>;
> 
> Shouldn't the interrupt (connected to P1_15) be described?


That interrupt pin from the PHY is not used. It did not need to be connected.


Chris

RE: [PATCH v4 0/9] Renesas RZ/A1 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

On Wednesday, April 05, 2017, Jacopo Mondi wrote:
> v3 -> v4:
> - use "pinmux" property in pmx sub-nodes in place of "renesas,pins"
> - use pinconf standard properties to set pin mux additional flags
> - add "bi-directional" and "output-enable" to pinconf generic properties
> - perform pmx function parsing at dt_node_to_map() time
> - change DT bindings to use GENERIC_PINCONF
> - change DT bindings to allow sub-nodes to have "pinmux" property
> specified
> - several renames (register names, DT parse functions, set_mux() function)


I just tested this driver on the RZ/A1 RSK board.
The following worked good.

   SCIF2, I2C, SDHI, Ethernet


SDHI also has bi-direction pins. For your reference, here was my DT:


/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
pins {
pinmux = ,/* SD_CD_1 */
 ,/* SD_WP_1 */
 ,   /* SD_CLK_1 */
 ;   /* SD_CMD_1 */
};

pins_bidir {
pinmux = ,   /* SD_D1_1 */
 ,   /* SD_D0_1 */
 ,   /* SD_D3_1 */
 ;   /* SD_D2_1 */
bi-directional;
};
};


Thanks,
Chris

RE: [PATCH v4 0/9] Renesas RZ/A1 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

On Wednesday, April 05, 2017, Jacopo Mondi wrote:
> v3 -> v4:
> - use "pinmux" property in pmx sub-nodes in place of "renesas,pins"
> - use pinconf standard properties to set pin mux additional flags
> - add "bi-directional" and "output-enable" to pinconf generic properties
> - perform pmx function parsing at dt_node_to_map() time
> - change DT bindings to use GENERIC_PINCONF
> - change DT bindings to allow sub-nodes to have "pinmux" property
> specified
> - several renames (register names, DT parse functions, set_mux() function)


I just tested this driver on the RZ/A1 RSK board.
The following worked good.

   SCIF2, I2C, SDHI, Ethernet


SDHI also has bi-direction pins. For your reference, here was my DT:


/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
pins {
pinmux = ,/* SD_CD_1 */
 ,/* SD_WP_1 */
 ,   /* SD_CLK_1 */
 ;   /* SD_CMD_1 */
};

pins_bidir {
pinmux = ,   /* SD_D1_1 */
 ,   /* SD_D0_1 */
 ,   /* SD_D3_1 */
 ;   /* SD_D2_1 */
bi-directional;
};
};


Thanks,
Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Thursday, March 30, 2017, Linus Walleij wrote:
> >> > +/*
> >> > + * Pin is bi-directional.
> >> > + * An alternate function that needs both input/output
> >> > +functionalities shall
> >> > + * be configured as bidirectional.
> >> > + * Eg. SDA/SCL pins of an I2c interface.
> >> > + */
> >> > +#define BI_DIR (1 << 3)
> >>
> >> Any specific reason why this should not simply be added to
> >> include/linux/pinctrl/pinconf-generic.h
> >> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> >> generic.c from the (new) DT property "bidirectional" simply?
> >
> > I see your point. It would cut down from every driver out there
> > inventing some new property or config instead of everyone just sharing
> > a fixed set.
> > Maybe someone else out there will end up having a need for a
> > "bidirectional" option.
> 
> I was thinking about that one. It is a bit weird electrically, like what
> kind of electronics is really bidirectional?
> 
> It seems like a fancy name for open drain/open source, what we call
> "single ended" configuration. (See docs in
> Documentation/gpio/driver.txt)
> 
> It would be great if you could check if that is what they mean, actually.

Here is the definition of the register in the hardware manual:

---
54.3.13 Port Bidirection Control Register (PBDCn)

This register enables or disables the input buffer while the output buffer is 
enabled. When the input buffer is enabled
while the output buffer is enabled, the bidirectional mode is entered, allowing 
the level of the Pn_m pin to always be read
via the PPRn.PPRnm bit.
---


But...what they don't really tell you is that any peripheral that needs to TX 
and RX data over a pin needs this set.


In the hardware manual, there is a pretty picture (Figure 54.1 Logical Diagram 
of Port Control) that shows a detailed logic diagram of the interface between 
the peripheral bus and the pin pad.
As far as I can tell, the "function mux" portion of this controller only knows 
how to set a pin as direction=in or direction=out.
So, in the case of I2C where each the SCL and SDA pins needs to be driven and 
read...it can't do that.
Therefore, there is an additional register to manually enable the input buffer 
and let the mux enable the output buffer.

So while yes, I2C is open-drain, this is also the case for the data pins for 
the SDHI. And the MDIO pin from Ethernet. It really has to do with the fact 
that this pin controller wasn't designed to enable both the input and output 
buffers at the same time.

The situation is similar for our SWIO_IN and SWIO_OUT flags. For example, to 
use the SSI sound interface, you have to set the TX signals to "input" 
(SWIO_IN). Makes sense, right??

I could argue that all of these "FLAGS" settings should have been incorporated 
in the HW logic of the pin controller...but for whatever reason, the they had 
to make them separate registers and make SW do it.
So, I could argue that these registers settings are really part of pin muxing, 
not really user configand hence belong in the "pinmux" property.


How about this compromise: Instead of BI_DIR, we use "TYPE_I2C", "TYPE_SDDATA", 
"TYPE_MDIO", "TYPE_LVDS", etc... to describe the 'special' ones. That way it 
can go back under "pinmux". Like Jacopo said, these register settings are 
really supposed to be set when you are selecting the pin-mux option.


Of course behind the scenes, it's really:
  #define TYPE_I2CBI_DIR 
  #define TYPE_SDDATA BI_DIR
  #define TYPE_SDCMD  BI_DIR
  #define TYPE_LVDS   SWIO_OUT

Examples:

i2c3_pins: i2c3 {
pinmux = ,
 ;
};

/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
/* SHDI ch1 on Port 3 */
pinmux = ,  /* SDHI1 CD */
,/* 
SDHI1 WP */
, /* 
SDHI1 DAT1 */
, /* 
SDHI1 DAT0 */
,   /* 
SDHI1 CLK */
,  /* 
SDHI1 CMD */
, /* 
SDHI1 DAT3 */
; /* 
SDHI1 DAT2 */
};



# Honestly, I have no idea where this pin controller came from. I have not seen 
it in any other Renesas part (Mitsubishi/Hitachi/NEC).


Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Thursday, March 30, 2017, Linus Walleij wrote:
> >> > +/*
> >> > + * Pin is bi-directional.
> >> > + * An alternate function that needs both input/output
> >> > +functionalities shall
> >> > + * be configured as bidirectional.
> >> > + * Eg. SDA/SCL pins of an I2c interface.
> >> > + */
> >> > +#define BI_DIR (1 << 3)
> >>
> >> Any specific reason why this should not simply be added to
> >> include/linux/pinctrl/pinconf-generic.h
> >> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> >> generic.c from the (new) DT property "bidirectional" simply?
> >
> > I see your point. It would cut down from every driver out there
> > inventing some new property or config instead of everyone just sharing
> > a fixed set.
> > Maybe someone else out there will end up having a need for a
> > "bidirectional" option.
> 
> I was thinking about that one. It is a bit weird electrically, like what
> kind of electronics is really bidirectional?
> 
> It seems like a fancy name for open drain/open source, what we call
> "single ended" configuration. (See docs in
> Documentation/gpio/driver.txt)
> 
> It would be great if you could check if that is what they mean, actually.

Here is the definition of the register in the hardware manual:

---
54.3.13 Port Bidirection Control Register (PBDCn)

This register enables or disables the input buffer while the output buffer is 
enabled. When the input buffer is enabled
while the output buffer is enabled, the bidirectional mode is entered, allowing 
the level of the Pn_m pin to always be read
via the PPRn.PPRnm bit.
---


But...what they don't really tell you is that any peripheral that needs to TX 
and RX data over a pin needs this set.


In the hardware manual, there is a pretty picture (Figure 54.1 Logical Diagram 
of Port Control) that shows a detailed logic diagram of the interface between 
the peripheral bus and the pin pad.
As far as I can tell, the "function mux" portion of this controller only knows 
how to set a pin as direction=in or direction=out.
So, in the case of I2C where each the SCL and SDA pins needs to be driven and 
read...it can't do that.
Therefore, there is an additional register to manually enable the input buffer 
and let the mux enable the output buffer.

So while yes, I2C is open-drain, this is also the case for the data pins for 
the SDHI. And the MDIO pin from Ethernet. It really has to do with the fact 
that this pin controller wasn't designed to enable both the input and output 
buffers at the same time.

The situation is similar for our SWIO_IN and SWIO_OUT flags. For example, to 
use the SSI sound interface, you have to set the TX signals to "input" 
(SWIO_IN). Makes sense, right??

I could argue that all of these "FLAGS" settings should have been incorporated 
in the HW logic of the pin controller...but for whatever reason, the they had 
to make them separate registers and make SW do it.
So, I could argue that these registers settings are really part of pin muxing, 
not really user configand hence belong in the "pinmux" property.


How about this compromise: Instead of BI_DIR, we use "TYPE_I2C", "TYPE_SDDATA", 
"TYPE_MDIO", "TYPE_LVDS", etc... to describe the 'special' ones. That way it 
can go back under "pinmux". Like Jacopo said, these register settings are 
really supposed to be set when you are selecting the pin-mux option.


Of course behind the scenes, it's really:
  #define TYPE_I2CBI_DIR 
  #define TYPE_SDDATA BI_DIR
  #define TYPE_SDCMD  BI_DIR
  #define TYPE_LVDS   SWIO_OUT

Examples:

i2c3_pins: i2c3 {
pinmux = ,
 ;
};

/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
/* SHDI ch1 on Port 3 */
pinmux = ,  /* SDHI1 CD */
,/* 
SDHI1 WP */
, /* 
SDHI1 DAT1 */
, /* 
SDHI1 DAT0 */
,   /* 
SDHI1 CLK */
,  /* 
SDHI1 CMD */
, /* 
SDHI1 DAT3 */
; /* 
SDHI1 DAT2 */
};



# Honestly, I have no idea where this pin controller came from. I have not seen 
it in any other Renesas part (Mitsubishi/Hitachi/NEC).


Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Wednesday, March 29, 2017, Chris Brandt wrote:
> On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > > But, what do we do for Ethernet? All the pins are "normal" except
> > > just the MDIO pin needs to be bidirectional.
> > > That's the part I'm confused by.
> > > How do we flag that just the ET_MDIO needs "bidirectional"?
> >
> > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-
> x.dts:
> >
> > avb_pins: avb {
> > mux {
> > groups = "avb_link", "avb_phy_int", "avb_mdc",
> >  "avb_mii";
> > function = "avb";
> > };
> >
> > pins_mdc {
> > groups = "avb_mdc";
> > drive-strength = <24>;
> > };
> >
> > pins_mii_tx {
> > pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> > "PIN_AVB_TD0",
> >"PIN_AVB_TD1", "PIN_AVB_TD2",
> > "PIN_AVB_TD3";
> > drive-strength = <12>;
> > };
> > };
> 
> Oh, so there is a way!
> 
> Thank you.
> 
> 
> So, for clarity:


Actually, Linus's request was to use "pinmux", not "pins".

So, it should be:

/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pinmux = <PIN(1, 6) | FUNC_1>,
 <PIN(1, 7) | FUNC_1>;
bidirectional;
};

 
/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pinmux = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
<PIN(5, 9) | FUNC_2)>,  /* P5_9 = ET_MDC */
<PIN(3, 4) | FUNC_2)>,  /* P3_4 = ET_RXCLK */
<PIN(3, 5) | FUNC_2)>,  /* P3_5 = ET_RXER */
<PIN(3, 6) | FUNC_2)>,  /* P3_6 = ET_RXDV */
<PIN(2, 0) | FUNC_2)>,  /* P2_0 = ET_TXCLK */
<PIN(2, 1) | FUNC_2)>,  /* P2_1 = ET_TXER */
<PIN(2, 2) | FUNC_2)>,  /* P2_2 = ET_TXEN */
<PIN(2, 3) | FUNC_2)>,  /* P2_3 = ET_CRS */
<PIN(2, 4) | FUNC_2)>,  /* P2_4 = ET_TXD0 */
<PIN(2, 5) | FUNC_2)>,  /* P2_5 = ET_TXD1 */
<PIN(2, 6) | FUNC_2)>,  /* P2_6 = ET_TXD2 */
<PIN(2, 7) | FUNC_2)>,  /* P2_7 = ET_TXD3 */
<PIN(2, 8) | FUNC_2)>,  /* P2_8 = ET_RXD0 */
<PIN(2, 9) | FUNC_2)>,  /* P2_9 = ET_RXD1 */
<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pinmux = <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO */
bidirectional;
};
};


  NOTE: "FUNC_2" can just be "2" as per Geert's request.



Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Wednesday, March 29, 2017, Chris Brandt wrote:
> On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > > But, what do we do for Ethernet? All the pins are "normal" except
> > > just the MDIO pin needs to be bidirectional.
> > > That's the part I'm confused by.
> > > How do we flag that just the ET_MDIO needs "bidirectional"?
> >
> > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-
> x.dts:
> >
> > avb_pins: avb {
> > mux {
> > groups = "avb_link", "avb_phy_int", "avb_mdc",
> >  "avb_mii";
> > function = "avb";
> > };
> >
> > pins_mdc {
> > groups = "avb_mdc";
> > drive-strength = <24>;
> > };
> >
> > pins_mii_tx {
> > pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> > "PIN_AVB_TD0",
> >"PIN_AVB_TD1", "PIN_AVB_TD2",
> > "PIN_AVB_TD3";
> > drive-strength = <12>;
> > };
> > };
> 
> Oh, so there is a way!
> 
> Thank you.
> 
> 
> So, for clarity:


Actually, Linus's request was to use "pinmux", not "pins".

So, it should be:

/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pinmux = ,
 ;
bidirectional;
};

 
/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pinmux = , /* P1_14 = ET_COL */
,  /* P5_9 = ET_MDC */
,  /* P3_4 = ET_RXCLK */
,  /* P3_5 = ET_RXER */
,  /* P3_6 = ET_RXDV */
,  /* P2_0 = ET_TXCLK */
,  /* P2_1 = ET_TXER */
,  /* P2_2 = ET_TXEN */
,  /* P2_3 = ET_CRS */
,  /* P2_4 = ET_TXD0 */
,  /* P2_5 = ET_TXD1 */
,  /* P2_6 = ET_TXD2 */
,  /* P2_7 = ET_TXD3 */
,  /* P2_8 = ET_RXD0 */
,  /* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pinmux = , /* P3_3 = ET_MDIO */
bidirectional;
};
};


  NOTE: "FUNC_2" can just be "2" as per Geert's request.



Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

Hi Geert,

On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > But, what do we do for Ethernet? All the pins are "normal" except just
> > the MDIO pin needs to be bidirectional.
> > That's the part I'm confused by.
> > How do we flag that just the ET_MDIO needs "bidirectional"?
> 
> You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:
> 
> avb_pins: avb {
> mux {
> groups = "avb_link", "avb_phy_int", "avb_mdc",
>  "avb_mii";
> function = "avb";
> };
> 
> pins_mdc {
> groups = "avb_mdc";
> drive-strength = <24>;
> };
> 
> pins_mii_tx {
> pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> "PIN_AVB_TD0",
>"PIN_AVB_TD1", "PIN_AVB_TD2",
> "PIN_AVB_TD3";
> drive-strength = <12>;
> };
> };

Oh, so there is a way!

Thank you.


So, for clarity:

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pins = , /* P1_14 = ET_COL */
,  /* P5_9 = ET_MDC */
,  /* P3_4 = ET_RXCLK */
,  /* P3_5 = ET_RXER */
,  /* P3_6 = ET_RXDV */
,  /* P2_0 = ET_TXCLK */
,  /* P2_1 = ET_TXER */
,  /* P2_2 = ET_TXEN */
,  /* P2_3 = ET_CRS */
,  /* P2_4 = ET_TXD0 */
,  /* P2_5 = ET_TXD1 */
,  /* P2_6 = ET_TXD2 */
,  /* P2_7 = ET_TXD3 */
,  /* P2_8 = ET_RXD0 */
,  /* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pins = ,   /* P3_3 = ET_MDIO */
bidirectional;
};
};



Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

Hi Geert,

On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > But, what do we do for Ethernet? All the pins are "normal" except just
> > the MDIO pin needs to be bidirectional.
> > That's the part I'm confused by.
> > How do we flag that just the ET_MDIO needs "bidirectional"?
> 
> You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:
> 
> avb_pins: avb {
> mux {
> groups = "avb_link", "avb_phy_int", "avb_mdc",
>  "avb_mii";
> function = "avb";
> };
> 
> pins_mdc {
> groups = "avb_mdc";
> drive-strength = <24>;
> };
> 
> pins_mii_tx {
> pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> "PIN_AVB_TD0",
>"PIN_AVB_TD1", "PIN_AVB_TD2",
> "PIN_AVB_TD3";
> drive-strength = <12>;
> };
> };

Oh, so there is a way!

Thank you.


So, for clarity:

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pins = , /* P1_14 = ET_COL */
,  /* P5_9 = ET_MDC */
,  /* P3_4 = ET_RXCLK */
,  /* P3_5 = ET_RXER */
,  /* P3_6 = ET_RXDV */
,  /* P2_0 = ET_TXCLK */
,  /* P2_1 = ET_TXER */
,  /* P2_2 = ET_TXEN */
,  /* P2_3 = ET_CRS */
,  /* P2_4 = ET_TXD0 */
,  /* P2_5 = ET_TXD1 */
,  /* P2_6 = ET_TXD2 */
,  /* P2_7 = ET_TXD3 */
,  /* P2_8 = ET_RXD0 */
,  /* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pins = ,   /* P3_3 = ET_MDIO */
bidirectional;
};
};



Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Wednesday, March 29, 2017, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi 
> wrote:
> 
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi 
> 
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities
> > +shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR (1 << 3)
> 
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> generic.c from the (new) DT property "bidirectional" simply?

I see your point. It would cut down from every driver out there
inventing some new property or config instead of everyone just sharing
a fixed set.
Maybe someone else out there will end up having a need for a
"bidirectional" option.


> > +/*
> > + * Flags used to ask software to drive the pin I/O direction
> > +overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction
> > +of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN(1 << 4)
> > +#define SWIO_OUT   (1 << 5)
> 
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
> 
> In the device tree use input-enable and add a new output-enable (with
> unspecified value) with proper description and DT bindings?

Again, that's probably fine. It seems we are still doing the same thing
which is using the DT to pass extra config information to the driver.
And, we can do whatever we want with that info.


> And if you think these have no general applicability, by the end of the
> day they are *still* pin config, not magic flags we can choose to toss in
> with the muxing, so you can do what the Qualcomm driver does and add
> custom pin configurations extending the generic pin config, see
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.

But, it seems that when you set a config option, it applies to everything
in "pins"?

I2C Example: (seem OK)
/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pins = ,
   ;
bidirectional;
};


But, what do we do for Ethernet? All the pins are "normal" except just
the MDIO pin needs to be bidirectional.
That's the part I'm confused by.
How do we flag that just the ET_MDIO needs "bidirectional"?

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
pins = , /* P1_14 = ET_COL */
,  /* P5_9 = ET_MDC */
,  /* P3_3 = ET_MDIO  (bi dir) 
!! */
,  /* P3_4 = ET_RXCLK */
,  /* P3_5 = ET_RXER */
,  /* P3_6 = ET_RXDV */
,  /* P2_0 = ET_TXCLK */
,  /* P2_1 = ET_TXER */
,  /* P2_2 = ET_TXEN */
,  /* P2_3 = ET_CRS */
,  /* P2_4 = ET_TXD0 */
,  /* P2_5 = ET_TXD1 */
,  /* P2_6 = ET_TXD2 */
,  /* P2_7 = ET_TXD3 */
,  /* P2_8 = ET_RXD0 */
,  /* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};


Chris

RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Wednesday, March 29, 2017, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi 
> wrote:
> 
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi 
> 
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities
> > +shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR (1 << 3)
> 
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> generic.c from the (new) DT property "bidirectional" simply?

I see your point. It would cut down from every driver out there
inventing some new property or config instead of everyone just sharing
a fixed set.
Maybe someone else out there will end up having a need for a
"bidirectional" option.


> > +/*
> > + * Flags used to ask software to drive the pin I/O direction
> > +overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction
> > +of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN(1 << 4)
> > +#define SWIO_OUT   (1 << 5)
> 
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
> 
> In the device tree use input-enable and add a new output-enable (with
> unspecified value) with proper description and DT bindings?

Again, that's probably fine. It seems we are still doing the same thing
which is using the DT to pass extra config information to the driver.
And, we can do whatever we want with that info.


> And if you think these have no general applicability, by the end of the
> day they are *still* pin config, not magic flags we can choose to toss in
> with the muxing, so you can do what the Qualcomm driver does and add
> custom pin configurations extending the generic pin config, see
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.

But, it seems that when you set a config option, it applies to everything
in "pins"?

I2C Example: (seem OK)
/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pins = ,
   ;
bidirectional;
};


But, what do we do for Ethernet? All the pins are "normal" except just
the MDIO pin needs to be bidirectional.
That's the part I'm confused by.
How do we flag that just the ET_MDIO needs "bidirectional"?

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
pins = , /* P1_14 = ET_COL */
,  /* P5_9 = ET_MDC */
,  /* P3_3 = ET_MDIO  (bi dir) 
!! */
,  /* P3_4 = ET_RXCLK */
,  /* P3_5 = ET_RXER */
,  /* P3_6 = ET_RXDV */
,  /* P2_0 = ET_TXCLK */
,  /* P2_1 = ET_TXER */
,  /* P2_2 = ET_TXEN */
,  /* P2_3 = ET_CRS */
,  /* P2_4 = ET_TXD0 */
,  /* P2_5 = ET_TXD1 */
,  /* P2_6 = ET_TXD2 */
,  /* P2_7 = ET_TXD3 */
,  /* P2_8 = ET_RXD0 */
,  /* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};


Chris

RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Chris Brandt]

On Wednesday, March 29, 2017, Linus Walleij:
> If you prefer to use preprocessor macros or whatever to make the bitmasks
> or how you want to organize the constants in your include files is not my
> concern, do whatever you seem fit, just pack it into a 32bit thing somehow
> which makes sense from a maintenance point of view.

OK, I think everyone agrees that a single 32-bit value is fine because macros
will also for good readability and maintenance.


> >> Not only because it will save use from having a loong list(*) of
> >> macros that has to be kept up to date when/if new RZ hardware will
> >> arrive, but also because of readability and simplicity for down-stream
> and BSP users.
> >> Speaking of which, I would like to know what does Chris think of this.
> >
> > The list of macros would be very long, especially against the
> > different packaging version of the RZ/A1 series. 11 ports, 16-pins for
> > each port, 8 different function options for each pin2 different
> package/pin variations.
> >
> > And at the end of the daythere is no benefit for the user over
> > just using a macro.
> 
> I don't know who has this idea that you could not use macros, certainly
> not me. Some misunderstanding must be going on. For what I'm concerned you
> can write hex numbers in the pinmux = <0x12345678>;
> 
> > The reason for the "FLAGS" is to work around a quirky hardware design
> (in my opinion).
> 
> The flags I don't like at all, and think they should be converted to
> generic pin config because they have nothing to do with muxing.
> 
> But I will point that out in the specific patch adding them.

OK, I think I understand your issue a little better of mixing user-defined 
config
with generic pinmux.

From our perspective, the FLAGS (BI_DIR, SWIO_IN, and SWIO_OUT ) were not really
optional when selecting what you want the pin to doso we considered it part
of the pin-mux.

In the hardware manual, there are tables that say that for 'some' certain
pins/functions "just setting the pin to a function is not enough...you need to 
make
another register setting (that may or may not make sense), otherwise it's not 
going
to work".

So, trying to get the driver to be that smart across all the different 
pin/package
variations seems to be way too ugly (and a maintenance nightmare). Simply 
putting
that in the DT binding was much more cleaner.


As for how to pass this HW info into the driver, I'll move over to the other 
email
thread where you started to give some suggestions...


Chris

RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Chris Brandt]

On Wednesday, March 29, 2017, Linus Walleij:
> If you prefer to use preprocessor macros or whatever to make the bitmasks
> or how you want to organize the constants in your include files is not my
> concern, do whatever you seem fit, just pack it into a 32bit thing somehow
> which makes sense from a maintenance point of view.

OK, I think everyone agrees that a single 32-bit value is fine because macros
will also for good readability and maintenance.


> >> Not only because it will save use from having a loong list(*) of
> >> macros that has to be kept up to date when/if new RZ hardware will
> >> arrive, but also because of readability and simplicity for down-stream
> and BSP users.
> >> Speaking of which, I would like to know what does Chris think of this.
> >
> > The list of macros would be very long, especially against the
> > different packaging version of the RZ/A1 series. 11 ports, 16-pins for
> > each port, 8 different function options for each pin2 different
> package/pin variations.
> >
> > And at the end of the daythere is no benefit for the user over
> > just using a macro.
> 
> I don't know who has this idea that you could not use macros, certainly
> not me. Some misunderstanding must be going on. For what I'm concerned you
> can write hex numbers in the pinmux = <0x12345678>;
> 
> > The reason for the "FLAGS" is to work around a quirky hardware design
> (in my opinion).
> 
> The flags I don't like at all, and think they should be converted to
> generic pin config because they have nothing to do with muxing.
> 
> But I will point that out in the specific patch adding them.

OK, I think I understand your issue a little better of mixing user-defined 
config
with generic pinmux.

From our perspective, the FLAGS (BI_DIR, SWIO_IN, and SWIO_OUT ) were not really
optional when selecting what you want the pin to doso we considered it part
of the pin-mux.

In the hardware manual, there are tables that say that for 'some' certain
pins/functions "just setting the pin to a function is not enough...you need to 
make
another register setting (that may or may not make sense), otherwise it's not 
going
to work".

So, trying to get the driver to be that smart across all the different 
pin/package
variations seems to be way too ugly (and a maintenance nightmare). Simply 
putting
that in the DT binding was much more cleaner.


As for how to pass this HW info into the driver, I'll move over to the other 
email
thread where you started to give some suggestions...


Chris

RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Chris Brandt]

Adding my 2 cents here:

On Wednesday, March 29, 2017, jacopo wrote:
> > > If you really do we may need to go for u64 but ... really? Is there
> > > a rational reason for that other than "we did it like this first"?
> > >
> > > I do not understand the notion of "flags" here. I hope that is not
> > > referring
> >
> > Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
> > https://patchwork.kernel.org/patch/9643047/
> >
> > 32-bit should be enough to cover pins, function, and flags.
> >
> 
> Geert already replied, but to avoid any confusion I'll try to remove from
> driver the use of "pin config" when referring to this three flags, which
> are just additional informations the pin controller needs to perform pin
> muxing properly. They're not related the standard pin config properties
> (pull-up/down, bias etc.. actually our hardware does not even support
> these natively)
> 
> > > to pin config, because I expect that to use the standard pin config
> > > bindings outside of the pinmux value which should just define the
> > > pin+function combo:
> > >
> > > node {
> > > pinmux = ;
> > > GENERIC_PINCONFIG;
> > > };
> > >
> > > Example from Mediatek:
> > >
> > > i2c1_pins_a: i2c1@0 {
> > > pins {
> > > pinmux = ,
> > > ;
> >
> > If we follow this example, then we can list all combinations in
> > include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating
> > the value by combining the bits using a macro where we need it in the
> DTS.
> >
> > It's gonna be a long list, though...
> >
> 
> I'm strongly in favour of something like
> pinmux = ,  ;
> 
> opposed to
> pinmux = , ... ;


I agree. I like "".



> Not only because it will save use from having a loong list(*) of macros
> that has to be kept up to date when/if new RZ hardware will arrive, but
> also because of readability and simplicity for down-stream and BSP users.
> Speaking of which, I would like to know what does Chris think of this.

The list of macros would be very long, especially against the different
packaging version of the RZ/A1 series. 11 ports, 16-pins for each port, 8 
different
function options for each pin2 different package/pin variations.

And at the end of the daythere is no benefit for the user over just using
a macro.


A little about "this controller" for the RZ/A1: In my opinion it's a one-shot 
usage.
I don't foresee future RZ/A SoCs using this exact pin controller.
The reason for the "FLAGS" is to work around a quirky hardware design (in my 
opinion).

However, future RZ/A SoCs will use a 'similar' type controller where each pin 
has 8 function
options (but the FLAGs won't be needed anymore...as far as I can see...).

So I foresee the DT interface staying more or less the same, but the underlying 
register
setup in the driver Will change (become more simple).


Now...if we can only convince the R-Car guys to move to a more simple pin-mux 
controller...


Chris

RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Chris Brandt]

Adding my 2 cents here:

On Wednesday, March 29, 2017, jacopo wrote:
> > > If you really do we may need to go for u64 but ... really? Is there
> > > a rational reason for that other than "we did it like this first"?
> > >
> > > I do not understand the notion of "flags" here. I hope that is not
> > > referring
> >
> > Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
> > https://patchwork.kernel.org/patch/9643047/
> >
> > 32-bit should be enough to cover pins, function, and flags.
> >
> 
> Geert already replied, but to avoid any confusion I'll try to remove from
> driver the use of "pin config" when referring to this three flags, which
> are just additional informations the pin controller needs to perform pin
> muxing properly. They're not related the standard pin config properties
> (pull-up/down, bias etc.. actually our hardware does not even support
> these natively)
> 
> > > to pin config, because I expect that to use the standard pin config
> > > bindings outside of the pinmux value which should just define the
> > > pin+function combo:
> > >
> > > node {
> > > pinmux = ;
> > > GENERIC_PINCONFIG;
> > > };
> > >
> > > Example from Mediatek:
> > >
> > > i2c1_pins_a: i2c1@0 {
> > > pins {
> > > pinmux = ,
> > > ;
> >
> > If we follow this example, then we can list all combinations in
> > include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating
> > the value by combining the bits using a macro where we need it in the
> DTS.
> >
> > It's gonna be a long list, though...
> >
> 
> I'm strongly in favour of something like
> pinmux = ,  ;
> 
> opposed to
> pinmux = , ... ;


I agree. I like "".



> Not only because it will save use from having a loong list(*) of macros
> that has to be kept up to date when/if new RZ hardware will arrive, but
> also because of readability and simplicity for down-stream and BSP users.
> Speaking of which, I would like to know what does Chris think of this.

The list of macros would be very long, especially against the different
packaging version of the RZ/A1 series. 11 ports, 16-pins for each port, 8 
different
function options for each pin2 different package/pin variations.

And at the end of the daythere is no benefit for the user over just using
a macro.


A little about "this controller" for the RZ/A1: In my opinion it's a one-shot 
usage.
I don't foresee future RZ/A SoCs using this exact pin controller.
The reason for the "FLAGS" is to work around a quirky hardware design (in my 
opinion).

However, future RZ/A SoCs will use a 'similar' type controller where each pin 
has 8 function
options (but the FLAGs won't be needed anymore...as far as I can see...).

So I foresee the DT interface staying more or less the same, but the underlying 
register
setup in the driver Will change (become more simple).


Now...if we can only convince the R-Car guys to move to a more simple pin-mux 
controller...


Chris

RE: [PATCH v3 4/7] arm: dts: r7s72100: Add pin controller node

[Chris Brandt]

Hi Jacopo,


On Friday, March 24, 2017, Jacopo Mondi
> + pinctrl: pinctrl@fcfe3000 {
> + compatible = "renesas,r7s72100-ports";
> +
> + #pinctrl-cells = <1>;
> +
> + reg = <0xfcfe3000 0x42C0>;

Out of curiosity, why did you change this from 0x4248 to 0x42C0?

In your update for renesas,rza1-pinctrl.txt, for the 'Example', you changed it 
from 0x4248 to 0x4230 (which Geert pointed out makes more sense), but then in 
the actual DT file you changed it to 0x42C0. Typo?


Chris

RE: [PATCH v3 4/7] arm: dts: r7s72100: Add pin controller node

[Chris Brandt]

Hi Jacopo,


On Friday, March 24, 2017, Jacopo Mondi
> + pinctrl: pinctrl@fcfe3000 {
> + compatible = "renesas,r7s72100-ports";
> +
> + #pinctrl-cells = <1>;
> +
> + reg = <0xfcfe3000 0x42C0>;

Out of curiosity, why did you change this from 0x4248 to 0x42C0?

In your update for renesas,rza1-pinctrl.txt, for the 'Example', you changed it 
from 0x4248 to 0x4230 (which Geert pointed out makes more sense), but then in 
the actual DT file you changed it to 0x42C0. Typo?


Chris

RE: [PATCH v2 0/7] Renesas RZ/A1 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

On Monday, March 20, 2017, Jacopo Mondi wrote:
> Chris: it would be great if you could give this another spin on RSK board.


I tested these patches on an RZ/A1H RSK board after modifying the DT for
the RSK vs the GENMAI board.

The following worked fine:
 * SCIF
 * I2C
 * SDHI
 * Ethernet

I see Geert has already responded with mostly text (grammar) changes.
As for the "API" of the driver from a user perspective, I think it should
cover all the use cases of the peripherals.


For your reference, here was my pin config for the RSK board testing. Once
this driver is (hopefully) accepted, I will update the upstream rskrza1 DT. 


--- RSK BOARD PIN SETUP ---

 {

scif2_pins: serial2 {
/* P3_0 as TxD2; P3_2 as RxD2 */
renesas,pins = ,
   ;
};

/* RIIC Ch 3 */
i2c3_pins: i2c3 {
/* RIIC3: P1_6 as SCL, P1_7 as SDA */
renesas,pins = ,
   ;
};

/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
/* SHDI ch1 on Port 3 */
renesas,pins = , /* SDHI1 CD */
,/* SDHI1 WP */
,  /* SDHI1 DAT1 */
,  /* SDHI1 DAT0 */
,   /* SDHI1 CLK */
,  /* SDHI1 CMD */
,  /* SDHI1 DAT3 */
;  /* SDHI1 DAT2 */
};

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
renesas,pins = , /* P1_14 = ET_COL */
,/* P5_9 = ET_MDC */
,   /* P3_3 = ET_MDIO (bi 
dir) */
,/* P3_4 = ET_RXCLK */
,/* P3_5 = ET_RXER */
,/* P3_6 = ET_RXDV */
,/* P2_0 = ET_TXCLK */
,/* P2_1 = ET_TXER */
,/* P2_2 = ET_TXEN */
,/* P2_3 = ET_CRS */
,/* P2_4 = ET_TXD0 */
,/* P2_5 = ET_TXD1 */
,/* P2_6 = ET_TXD2 */
,/* P2_7 = ET_TXD3 */
,/* P2_8 = ET_RXD0 */
,/* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

/ {
leds {
status = "okay";
compatible = "gpio-leds";

led1 {
gpios = < 1 GPIO_ACTIVE_LOW>;
};
};
};



Thank you,
Chris

RE: [PATCH v2 0/7] Renesas RZ/A1 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

On Monday, March 20, 2017, Jacopo Mondi wrote:
> Chris: it would be great if you could give this another spin on RSK board.


I tested these patches on an RZ/A1H RSK board after modifying the DT for
the RSK vs the GENMAI board.

The following worked fine:
 * SCIF
 * I2C
 * SDHI
 * Ethernet

I see Geert has already responded with mostly text (grammar) changes.
As for the "API" of the driver from a user perspective, I think it should
cover all the use cases of the peripherals.


For your reference, here was my pin config for the RSK board testing. Once
this driver is (hopefully) accepted, I will update the upstream rskrza1 DT. 


--- RSK BOARD PIN SETUP ---

 {

scif2_pins: serial2 {
/* P3_0 as TxD2; P3_2 as RxD2 */
renesas,pins = ,
   ;
};

/* RIIC Ch 3 */
i2c3_pins: i2c3 {
/* RIIC3: P1_6 as SCL, P1_7 as SDA */
renesas,pins = ,
   ;
};

/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
/* SHDI ch1 on Port 3 */
renesas,pins = , /* SDHI1 CD */
,/* SDHI1 WP */
,  /* SDHI1 DAT1 */
,  /* SDHI1 DAT0 */
,   /* SDHI1 CLK */
,  /* SDHI1 CMD */
,  /* SDHI1 DAT3 */
;  /* SDHI1 DAT2 */
};

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
renesas,pins = , /* P1_14 = ET_COL */
,/* P5_9 = ET_MDC */
,   /* P3_3 = ET_MDIO (bi 
dir) */
,/* P3_4 = ET_RXCLK */
,/* P3_5 = ET_RXER */
,/* P3_6 = ET_RXDV */
,/* P2_0 = ET_TXCLK */
,/* P2_1 = ET_TXER */
,/* P2_2 = ET_TXEN */
,/* P2_3 = ET_CRS */
,/* P2_4 = ET_TXD0 */
,/* P2_5 = ET_TXD1 */
,/* P2_6 = ET_TXD2 */
,/* P2_7 = ET_TXD3 */
,/* P2_8 = ET_RXD0 */
,/* P2_9 = ET_RXD1 */
, /* P2_10 = ET_RXD2 */
; /* P2_11 = ET_RXD3 */
};
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

/ {
leds {
status = "okay";
compatible = "gpio-leds";

led1 {
gpios = < 1 GPIO_ACTIVE_LOW>;
};
};
};



Thank you,
Chris

RE: [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END

[Chris Brandt]

On 12/6/2016, Florian Fainelli wrote:
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4001dd15818d..18ef688a796e 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1437,12 +1437,8 @@ static void __init kmap_init(void)
>  static void __init map_lowmem(void)
>  {
>   struct memblock_region *reg;
> -#ifdef CONFIG_XIP_KERNEL
> - phys_addr_t kernel_x_start = round_down(__pa(_sdata), SECTION_SIZE);
> -#else
> - phys_addr_t kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
> -#endif
> - phys_addr_t kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
> + phys_addr_t kernel_x_start = round_down(__pa(KERNEL_START),
> SECTION_SIZE);
> + phys_addr_t kernel_x_end = round_down(__pa(_end), SECTION_SIZE);

Why are you changing the end of executable kernel (hence the 'x' in
kernel_x_end) from __init_end to _end which basically maps the entire
kernel image including text and data?

Doing so would then change data from MT_MEMORY_RW into MT_MEMORY_RWX.

I would think it would create some type of security risk to allow
data to be executable.

RE: [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END

[Chris Brandt]

On 12/6/2016, Florian Fainelli wrote:
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 4001dd15818d..18ef688a796e 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1437,12 +1437,8 @@ static void __init kmap_init(void)
>  static void __init map_lowmem(void)
>  {
>   struct memblock_region *reg;
> -#ifdef CONFIG_XIP_KERNEL
> - phys_addr_t kernel_x_start = round_down(__pa(_sdata), SECTION_SIZE);
> -#else
> - phys_addr_t kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
> -#endif
> - phys_addr_t kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
> + phys_addr_t kernel_x_start = round_down(__pa(KERNEL_START),
> SECTION_SIZE);
> + phys_addr_t kernel_x_end = round_down(__pa(_end), SECTION_SIZE);

Why are you changing the end of executable kernel (hence the 'x' in
kernel_x_end) from __init_end to _end which basically maps the entire
kernel image including text and data?

Doing so would then change data from MT_MEMORY_RW into MT_MEMORY_RWX.

I would think it would create some type of security risk to allow
data to be executable.

RE: [PATCH] spi: rspi: avoid uninitialized variable access

[Chris Brandt]

Since I was CC-ed, I'll add in my opinion:


While Geert already pointed out the spelling mistake (_in_or_our >> 
_in_or_out), since that function is only just for qspi versions, a better 
function name should have been "qspi_pio_transfer_in_or_out"



However

On 11/8/2016, Arnd Bergmann wrote:
> This simplifies it again by keeping the two separate, which then ends up
> avoiding that warning.

I agree with Arnd's method of NOT adding a new "rspi_pio_transfer_in_or_our" 
function and instead just doing it in the existing qspi_transfer_ functions.



Side note: The RSPI in the RZ/A1 devices also have FIFOs which can be used to 
reduce the number of interrupts in pio transfers, so maybe someday I'll make a 
similar change for non-qspi devices as well.


Chris

RE: [PATCH] spi: rspi: avoid uninitialized variable access

[Chris Brandt]

Since I was CC-ed, I'll add in my opinion:


While Geert already pointed out the spelling mistake (_in_or_our >> 
_in_or_out), since that function is only just for qspi versions, a better 
function name should have been "qspi_pio_transfer_in_or_out"



However

On 11/8/2016, Arnd Bergmann wrote:
> This simplifies it again by keeping the two separate, which then ends up
> avoiding that warning.

I agree with Arnd's method of NOT adding a new "rspi_pio_transfer_in_or_our" 
function and instead just doing it in the existing qspi_transfer_ functions.



Side note: The RSPI in the RZ/A1 devices also have FIFOs which can be used to 
reduce the number of interrupts in pio transfers, so maybe someday I'll make a 
similar change for non-qspi devices as well.


Chris

RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

[Chris Brandt]

On 19 Feb 2016, Russell King wrote:

> Using the DTB location on XIP platforms is a no-goer - the flattened
> DTB information can be fixed, so on an XIP platform it makes sense
> for this to also be in flash, not in RAM (the whole point of XIP is
> to remove constant data from RAM after all, so why would you want to
> copy the FDT to RAM?)

I was under the impression that the DTB had a limited life span, and once 
booted, it could be clobbered. Hence, RAM would not really be wasted (post boot 
that is).

For an XIP system (with MMU), if you have your DTB in ROM someplace, you run 
the risk of getting it cut off after the MMU setup is done because it would 
fall someplace after the _exiprom marker.


> Passing it in a register to the kernel image is also not possible:
> we've been out of spare registers for some time now for passing
> additional information into the kernel.  It wouldn't be a problem
> had folk not had this "eww, I don't like ATAGs, lets get rid of them
> and use DT instead" attitude: had we kept ATAGs, then we'd have an
> in-memory format to pass this kind of information to the kernel.
> That would solve soo many problems today it's untrue: stuff like
> where a debugging UART is located and the type of it...

Too bad we just couldn't have added an ATAG to point to a DTB. Sigh.


Chris

RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

[Chris Brandt]

On 19 Feb 2016, Russell King wrote:

> Using the DTB location on XIP platforms is a no-goer - the flattened
> DTB information can be fixed, so on an XIP platform it makes sense
> for this to also be in flash, not in RAM (the whole point of XIP is
> to remove constant data from RAM after all, so why would you want to
> copy the FDT to RAM?)

I was under the impression that the DTB had a limited life span, and once 
booted, it could be clobbered. Hence, RAM would not really be wasted (post boot 
that is).

For an XIP system (with MMU), if you have your DTB in ROM someplace, you run 
the risk of getting it cut off after the MMU setup is done because it would 
fall someplace after the _exiprom marker.


> Passing it in a register to the kernel image is also not possible:
> we've been out of spare registers for some time now for passing
> additional information into the kernel.  It wouldn't be a problem
> had folk not had this "eww, I don't like ATAGs, lets get rid of them
> and use DT instead" attitude: had we kept ATAGs, then we'd have an
> in-memory format to pass this kind of information to the kernel.
> That would solve soo many problems today it's untrue: stuff like
> where a debugging UART is located and the type of it...

Too bad we just couldn't have added an ATAG to point to a DTB. Sigh.


Chris

RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

[Chris Brandt]

On 19 Feb 2016, Arnd Bergmann wrote:

> On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > 
> > Acked-by: Nicolas Pitre 
> > 
> > Is there a way to provide a default for defaults?
> 
> We could have something like
> 
> config PHYS_OFFSET_0
>   bool
> 
> config PHYS_OFFSET_1
>   bool
> 
> config PHYS_OFFSET_2
>   bool
> 
> ... (we need 8 of the 16 possible addresses)
> 
> 
> config PHYS_OFFSET
>   hex "Physical address of main memory" if MMU
>   default DRAM_BASE if !MMU
>   default 0x if PHYS_OFFSET_0
>   default 0x1000 if PHYS_OFFSET_1
>   default 0x2000 if PHYS_OFFSET_2
>   default 0x3000 if PHYS_OFFSET_3
>   default 0x7000 if PHYS_OFFSET_7
>   default 0x8000 if PHYS_OFFSET_8
>   default 0xa000 if PHYS_OFFSET_A
>   default 0xc000 if PHYS_OFFSET_C
> 
> 
> and then select one of the bool symbols from each platform.
> Would that address your question?


Here's a question:

Can we just get rid of PHYS_OFFSET???

If it's only used at boot for XIP systems, we could:

A) pass it in via an unused register like atags and DT
 or
B) just assume that atags or DT is in RAM, so round down to the nearest section 
and assume that is the start of your RAM

If it is needed after initial boot, then on first boot we save what was passed 
in from the boot loader for later use.


Chris

RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

[Chris Brandt]

On 19 Feb 2016, Arnd Bergmann wrote:

> On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > 
> > Acked-by: Nicolas Pitre 
> > 
> > Is there a way to provide a default for defaults?
> 
> We could have something like
> 
> config PHYS_OFFSET_0
>   bool
> 
> config PHYS_OFFSET_1
>   bool
> 
> config PHYS_OFFSET_2
>   bool
> 
> ... (we need 8 of the 16 possible addresses)
> 
> 
> config PHYS_OFFSET
>   hex "Physical address of main memory" if MMU
>   default DRAM_BASE if !MMU
>   default 0x if PHYS_OFFSET_0
>   default 0x1000 if PHYS_OFFSET_1
>   default 0x2000 if PHYS_OFFSET_2
>   default 0x3000 if PHYS_OFFSET_3
>   default 0x7000 if PHYS_OFFSET_7
>   default 0x8000 if PHYS_OFFSET_8
>   default 0xa000 if PHYS_OFFSET_A
>   default 0xc000 if PHYS_OFFSET_C
> 
> 
> and then select one of the bool symbols from each platform.
> Would that address your question?


Here's a question:

Can we just get rid of PHYS_OFFSET???

If it's only used at boot for XIP systems, we could:

A) pass it in via an unused register like atags and DT
 or
B) just assume that atags or DT is in RAM, so round down to the nearest section 
and assume that is the start of your RAM

If it is needed after initial boot, then on first boot we save what was passed 
in from the boot loader for later use.


Chris

RE: [PATCH v12 10/20] dax: Replace XIP documentation with DAX documentation

[Chris Brandt]

I believe the motivation for the new DAX code was being able to read/write data 
directly to specific physical memory. However, with the AXFS file system, XIP 
file mapping was mostly beneficial for direct access to executable code pages, 
not data. Code pages were XIP-ed, and data pages were copied to RAM as normal. 
This results in a significant reduction in system RAM, especially when used 
with an XIP_KERNEL. In some systems, most of your RAM is eaten up by lots of 
code pages from big bloated shared libraries, not R/W data. (of course I'm 
talking about smaller embedded system here)


Also, it's up to the file system decide to decide what should be XIP/DAX or 
not. If your motivation is to DAX/XIP code pages to save RAM, then you don't 
have to worry about '/etc/password' cache issues, because that file would be 
handled in a traditional manner.

I think it comes down to what your motivation to DAX is: DAX data or DAX code


Chris



-Original Message-
From: Wilcox, Matthew R [mailto:matthew.r.wil...@intel.com] 
Sent: Friday, January 22, 2016 8:08 AM
To: Jared Hulbert 
Cc: Linux FS Devel ; LKML 
; Linux Memory Management List 
; Matthew Wilcox ; Andrew Morton 
; Carsten Otte ; Chris Brandt 

Subject: RE: [PATCH v12 10/20] dax: Replace XIP documentation with DAX 
documentation

Hi Jared,

The old filemap_xip code was living in a state of sin ;-)  It was writing to 
the kernel's mapping of an address, and then not flushing the cache before 
telling userspace that the data was updated.  That left userspace able to read 
stale data, which might actually have been a security hole (had that page 
previously contained, say, /etc/passwd).

We don't have cache flushing functions that work without a struct page.  So we 
need to come up with a new solution.  My preferred solution is to explicitly 
map the memory before using it.  On ARM, MIPS & SPARC, each page should be 
mapped to an address that is at a multiple of SHMLBA from the address that the 
user has the page mapped at.  On other architectures, there is no d-cache flush 
problem, so they can use an identity map.

Or you can just enable the DAX code and continue living in the state of sin 
that you were in before.  It probably won't bite you ... maybe ...

-Original Message-
From: Jared Hulbert [mailto:jare...@gmail.com]
Sent: Thursday, January 21, 2016 10:38 AM
To: Wilcox, Matthew R
Cc: Linux FS Devel; LKML; Linux Memory Management List; Matthew Wilcox; Andrew 
Morton; Carsten Otte; Chris Brandt
Subject: Re: [PATCH v12 10/20] dax: Replace XIP documentation with DAX 
documentation

HI!  I've been out of the community for a while, but I'm trying to step back in 
here and catch up with some of my old areas of specialty.
Couple questions, sorry to drag up such old conversations.

The DAX documentation that made it into kernel 4.0 has the following line  "The 
DAX code does not work correctly on architectures which have virtually mapped 
caches such as ARM, MIPS and SPARC."

1) It really doesn't support ARM.?  I never had problems with the old 
filemap_xip.c stuff on ARM, what changed?
2) Is there a thread discussing this?

On Fri, Oct 24, 2014 at 2:20 PM, Matthew Wilcox  
wrote:
> From: Matthew Wilcox 
>
> Based on the original XIP documentation, this documents the current 
> state of affairs, and includes instructions on how users can enable 
> DAX if their devices and kernel support it.
>
> Signed-off-by: Matthew Wilcox 
> Reviewed-by: Randy Dunlap 
> ---
>  Documentation/filesystems/00-INDEX |  5 ++-  
> Documentation/filesystems/dax.txt  | 89 
> ++
>  Documentation/filesystems/xip.txt  | 71 
> --
>  3 files changed, 92 insertions(+), 73 deletions(-)  create mode 
> 100644 Documentation/filesystems/dax.txt  delete mode 100644 
> Documentation/filesystems/xip.txt
>
> diff --git a/Documentation/filesystems/00-INDEX 
> b/Documentation/filesystems/00-INDEX
> index ac28149..9922939 100644
> --- a/Documentation/filesystems/00-INDEX
> +++ b/Documentation/filesystems/00-INDEX
> @@ -34,6 +34,9 @@ configfs/
> - directory containing configfs documentation and example code.
>  cramfs.txt
> - info on the cram filesystem for small storage (ROMs etc).
> +dax.txt
> +   - info on avoiding the page cache for files stored on CPU-addressable
> + storage devices.
>  debugfs.txt
> - info on the debugfs filesystem.
>  devpts.txt
> @@ -154,5 +157,3 @@ xfs-self-describing-metadata.txt
> - info on XFS Self Describing Metadata.
>  xfs.txt
> - info and mount options for the XFS filesystem.
> -xip.txt
> -   - info on execute-in-place for file mappings.
> diff --git a/Documentation/filesystems/dax.txt 
> b/Documentation/filesystems/dax.txt
> new file mode 100644
> index 000..635adaa
> 

RE: [PATCH v12 10/20] dax: Replace XIP documentation with DAX documentation

[Chris Brandt]

I believe the motivation for the new DAX code was being able to read/write data 
directly to specific physical memory. However, with the AXFS file system, XIP 
file mapping was mostly beneficial for direct access to executable code pages, 
not data. Code pages were XIP-ed, and data pages were copied to RAM as normal. 
This results in a significant reduction in system RAM, especially when used 
with an XIP_KERNEL. In some systems, most of your RAM is eaten up by lots of 
code pages from big bloated shared libraries, not R/W data. (of course I'm 
talking about smaller embedded system here)


Also, it's up to the file system decide to decide what should be XIP/DAX or 
not. If your motivation is to DAX/XIP code pages to save RAM, then you don't 
have to worry about '/etc/password' cache issues, because that file would be 
handled in a traditional manner.

I think it comes down to what your motivation to DAX is: DAX data or DAX code


Chris



-Original Message-
From: Wilcox, Matthew R [mailto:matthew.r.wil...@intel.com] 
Sent: Friday, January 22, 2016 8:08 AM
To: Jared Hulbert <jare...@gmail.com>
Cc: Linux FS Devel <linux-fsde...@vger.kernel.org>; LKML 
<linux-kernel@vger.kernel.org>; Linux Memory Management List 
<linux...@kvack.org>; Matthew Wilcox <wi...@linux.intel.com>; Andrew Morton 
<a...@linux-foundation.org>; Carsten Otte <co...@de.ibm.com>; Chris Brandt 
<chris.bra...@renesas.com>
Subject: RE: [PATCH v12 10/20] dax: Replace XIP documentation with DAX 
documentation

Hi Jared,

The old filemap_xip code was living in a state of sin ;-)  It was writing to 
the kernel's mapping of an address, and then not flushing the cache before 
telling userspace that the data was updated.  That left userspace able to read 
stale data, which might actually have been a security hole (had that page 
previously contained, say, /etc/passwd).

We don't have cache flushing functions that work without a struct page.  So we 
need to come up with a new solution.  My preferred solution is to explicitly 
map the memory before using it.  On ARM, MIPS & SPARC, each page should be 
mapped to an address that is at a multiple of SHMLBA from the address that the 
user has the page mapped at.  On other architectures, there is no d-cache flush 
problem, so they can use an identity map.

Or you can just enable the DAX code and continue living in the state of sin 
that you were in before.  It probably won't bite you ... maybe ...

-Original Message-
From: Jared Hulbert [mailto:jare...@gmail.com]
Sent: Thursday, January 21, 2016 10:38 AM
To: Wilcox, Matthew R
Cc: Linux FS Devel; LKML; Linux Memory Management List; Matthew Wilcox; Andrew 
Morton; Carsten Otte; Chris Brandt
Subject: Re: [PATCH v12 10/20] dax: Replace XIP documentation with DAX 
documentation

HI!  I've been out of the community for a while, but I'm trying to step back in 
here and catch up with some of my old areas of specialty.
Couple questions, sorry to drag up such old conversations.

The DAX documentation that made it into kernel 4.0 has the following line  "The 
DAX code does not work correctly on architectures which have virtually mapped 
caches such as ARM, MIPS and SPARC."

1) It really doesn't support ARM.?  I never had problems with the old 
filemap_xip.c stuff on ARM, what changed?
2) Is there a thread discussing this?

On Fri, Oct 24, 2014 at 2:20 PM, Matthew Wilcox <matthew.r.wil...@intel.com> 
wrote:
> From: Matthew Wilcox <wi...@linux.intel.com>
>
> Based on the original XIP documentation, this documents the current 
> state of affairs, and includes instructions on how users can enable 
> DAX if their devices and kernel support it.
>
> Signed-off-by: Matthew Wilcox <wi...@linux.intel.com>
> Reviewed-by: Randy Dunlap <rdun...@infradead.org>
> ---
>  Documentation/filesystems/00-INDEX |  5 ++-  
> Documentation/filesystems/dax.txt  | 89 
> ++
>  Documentation/filesystems/xip.txt  | 71 
> --
>  3 files changed, 92 insertions(+), 73 deletions(-)  create mode 
> 100644 Documentation/filesystems/dax.txt  delete mode 100644 
> Documentation/filesystems/xip.txt
>
> diff --git a/Documentation/filesystems/00-INDEX 
> b/Documentation/filesystems/00-INDEX
> index ac28149..9922939 100644
> --- a/Documentation/filesystems/00-INDEX
> +++ b/Documentation/filesystems/00-INDEX
> @@ -34,6 +34,9 @@ configfs/
> - directory containing configfs documentation and example code.
>  cramfs.txt
> - info on the cram filesystem for small storage (ROMs etc).
> +dax.txt
> +   - info on avoiding the page cache for files stored on CPU-addressable
> + storage devices.
>  debugfs.txt
> - info on the debugfs filesystem.
>  devpts.txt
> @@ -154,5 +157,3 @@ xfs-self-describing-metadata.t

RE: [PATCH] ARM: xip: Use correct symbol for end of ROM marker

[Chris Brandt]

> I think below is required as well.
>
> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -34,7 +36,7 @@
>   * recompiling the whole kernel when CONFIG_XIP_KERNEL is turned on/off.
>   */
>  #undef MODULES_VADDR
> -#define MODULES_VADDR(((unsigned long)_etext + ~PMD_MASK) & PMD_MASK)
> +#define MODULES_VADDR(((unsigned long)_edata_loc + ~PMD_MASK) & 
> PMD_MASK)
>  #endif


Thank you!
Somehow I missed that file when I was moving from one tree to another when 
preparing my commits.

I'll update and resubmit the patch.

Chris