* ioremap() use with specific mailbox drivers and SCMI @ 2018-04-19 16:25 Florian Fainelli 2018-04-19 16:35 ` Florian Fainelli 0 siblings, 1 reply; 5+ messages in thread From: Florian Fainelli @ 2018-04-19 16:25 UTC (permalink / raw) To: linux-arm-kernel Hi Sudeep, I am looking at the following files: arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts: memory at 0 { device_type = "memory"; reg = <0x00000000 0x00000000 0x00000000 0x05e00000>, <0x00000000 0x05f00000 0x00000000 0x00001000>, <0x00000000 0x05f02000 0x00000000 0x00efd000>, <0x00000000 0x06e00000 0x00000000 0x0060f000>, <0x00000000 0x07410000 0x00000000 0x1aaf0000>, <0x00000000 0x22000000 0x00000000 0x1c000000>; }; arch/arm64/boot/dts/hisilicon/hi6220.dtsi: mailbox: mailbox at f7510000 { compatible = "hisilicon,hi6220-mbox"; reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */ <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */ interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; #mbox-cells = <3>; }; and this is quite an interesting approach, although it appears to be quite fragile and simplistic to me for a number of reasons: - if the mailbox at f7510000 was ever to be relocated within a parent "bus" node that had a "ranges" property mapping the parent space from 0xf000_0000 to produce offset relative nodes, the translation for its "reg" properties would apply relative to both cells, which means that now, the second cell may no longer point to DRAM anymore. This can be worked around by keeping that particular node outside of the "bus" node, but it is essentially means we mixed address spaces within the "reg" property - punching holes in DRAM by omitting this mailbox's shared buffer from the top-level memory "reg" property is extremely error prone, and seems to be a shortcut/hack to allow the ARM/ARM64 implementations to successfully call ioremap() against these regions. I am not commenting about the other regions mentioned in the DTS, but pstore is possibly another example where using ioremap() is convenient, but still not quite correct. Using a reserved-memory entry would be far less error prone, would clearly show the intents of the specific regions being declared, and could, through the use of additional properties (e.g: "map", as opposed to "no-map") indicate a possible need to map that DRAM region into kernel virtual memory and using e.g: memremap() APIs. Also the semantics of ioremap() are not particularly portable from one architecture to another whereas memremap() is IMHO. Thank you -- Florian ^ permalink raw reply [flat|nested] 5+ messages in thread
* ioremap() use with specific mailbox drivers and SCMI 2018-04-19 16:25 ioremap() use with specific mailbox drivers and SCMI Florian Fainelli @ 2018-04-19 16:35 ` Florian Fainelli 2018-04-19 17:09 ` Sudeep Holla 0 siblings, 1 reply; 5+ messages in thread From: Florian Fainelli @ 2018-04-19 16:35 UTC (permalink / raw) To: linux-arm-kernel +Sudeep as originally intended On 04/19/2018 09:25 AM, Florian Fainelli wrote: > Hi Sudeep, > > I am looking at the following files: > > arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts: > memory at 0 { > device_type = "memory"; > reg = <0x00000000 0x00000000 0x00000000 0x05e00000>, > <0x00000000 0x05f00000 0x00000000 0x00001000>, > <0x00000000 0x05f02000 0x00000000 0x00efd000>, > <0x00000000 0x06e00000 0x00000000 0x0060f000>, > <0x00000000 0x07410000 0x00000000 0x1aaf0000>, > <0x00000000 0x22000000 0x00000000 0x1c000000>; > }; > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi: > mailbox: mailbox at f7510000 { > compatible = "hisilicon,hi6220-mbox"; > reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */ > <0x0 0x06dff800 0x0 0x0800>; /* Mailbox > buffer */ > interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > #mbox-cells = <3>; > }; > > and this is quite an interesting approach, although it appears to be > quite fragile and simplistic to me for a number of reasons: > > - if the mailbox at f7510000 was ever to be relocated within a parent "bus" > node that had a "ranges" property mapping the parent space from > 0xf000_0000 to produce offset relative nodes, the translation for its > "reg" properties would apply relative to both cells, which means that > now, the second cell may no longer point to DRAM anymore. This can be > worked around by keeping that particular node outside of the "bus" node, > but it is essentially means we mixed address spaces within the "reg" > property > > - punching holes in DRAM by omitting this mailbox's shared buffer from > the top-level memory "reg" property is extremely error prone, and seems > to be a shortcut/hack to allow the ARM/ARM64 implementations to > successfully call ioremap() against these regions. I am not commenting > about the other regions mentioned in the DTS, but pstore is possibly > another example where using ioremap() is convenient, but still not quite > correct. > > Using a reserved-memory entry would be far less error prone, would > clearly show the intents of the specific regions being declared, and > could, through the use of additional properties (e.g: "map", as opposed > to "no-map") indicate a possible need to map that DRAM region into > kernel virtual memory and using e.g: memremap() APIs. > > Also the semantics of ioremap() are not particularly portable from one > architecture to another whereas memremap() is IMHO. > > Thank you > -- Florian ^ permalink raw reply [flat|nested] 5+ messages in thread
* ioremap() use with specific mailbox drivers and SCMI 2018-04-19 16:35 ` Florian Fainelli @ 2018-04-19 17:09 ` Sudeep Holla 2018-04-19 21:05 ` Arnd Bergmann 0 siblings, 1 reply; 5+ messages in thread From: Sudeep Holla @ 2018-04-19 17:09 UTC (permalink / raw) To: linux-arm-kernel +Arnd, On 19/04/18 17:35, Florian Fainelli wrote: > +Sudeep as originally intended > > On 04/19/2018 09:25 AM, Florian Fainelli wrote: >> Hi Sudeep, >> >> I am looking at the following files: >> >> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts: >> memory at 0 { >> device_type = "memory"; >> reg = <0x00000000 0x00000000 0x00000000 0x05e00000>, >> <0x00000000 0x05f00000 0x00000000 0x00001000>, >> <0x00000000 0x05f02000 0x00000000 0x00efd000>, >> <0x00000000 0x06e00000 0x00000000 0x0060f000>, >> <0x00000000 0x07410000 0x00000000 0x1aaf0000>, >> <0x00000000 0x22000000 0x00000000 0x1c000000>; >> }; >> >> arch/arm64/boot/dts/hisilicon/hi6220.dtsi: >> mailbox: mailbox at f7510000 { >> compatible = "hisilicon,hi6220-mbox"; >> reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */ >> <0x0 0x06dff800 0x0 0x0800>; /* Mailbox >> buffer */ >> interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; >> #mbox-cells = <3>; >> }; >> Yes I did indeed point Jim Quinlan to look at the above DTS as I roughly knew that it used DRAM for mailbox communication. >> and this is quite an interesting approach, although it appears to be >> quite fragile and simplistic to me for a number of reasons: >> >> - if the mailbox at f7510000 was ever to be relocated within a parent "bus" >> node that had a "ranges" property mapping the parent space from >> 0xf000_0000 to produce offset relative nodes, the translation for its >> "reg" properties would apply relative to both cells, which means that >> now, the second cell may no longer point to DRAM anymore. This can be >> worked around by keeping that particular node outside of the "bus" node, >> but it is essentially means we mixed address spaces within the "reg" >> property >> I am not suggesting that. The memory remains where ever it belong in the bus and just marked as reserved for use for mailbox. The SCMI binding just refers by phandle into that. In that respect, the above example is wrong as it has mixed the shared memory with the mailbox controller address range. >> - punching holes in DRAM by omitting this mailbox's shared buffer from >> the top-level memory "reg" property is extremely error prone, and seems >> to be a shortcut/hack to allow the ARM/ARM64 implementations to >> successfully call ioremap() against these regions. I am not commenting >> about the other regions mentioned in the DTS, but pstore is possibly >> another example where using ioremap() is convenient, but still not quite >> correct. >> OK new to me and sorry for lack of my knowledge. >> Using a reserved-memory entry would be far less error prone, would >> clearly show the intents of the specific regions being declared, and >> could, through the use of additional properties (e.g: "map", as opposed >> to "no-map") indicate a possible need to map that DRAM region into >> kernel virtual memory and using e.g: memremap() APIs. >> Sorry for missing this. For some reasons, I had assumed memremap was specifically for DRAM and when I wrote the driver, I was testing on our internal Juno dev platform which has dedicated SRAM for the communication with remote control processor. However having a quick look at the API, I see no reasons to not use that as it takes care of using ioremap if required. But I am wondering why the generic SRAM driver in drivers/misc/sram.c uses ioremap. >> Also the semantics of ioremap() are not particularly portable from one >> architecture to another whereas memremap() is IMHO. >> Again, wasn't aware of that. Arnd, Do you see any issues moving from ioremap to memremap for SCMI ? I am asking you as you would have seen various platforms having multiple options for shared IPC memory and wanted to get you feedback. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 5+ messages in thread
* ioremap() use with specific mailbox drivers and SCMI 2018-04-19 17:09 ` Sudeep Holla @ 2018-04-19 21:05 ` Arnd Bergmann 2018-08-10 18:13 ` Florian Fainelli 0 siblings, 1 reply; 5+ messages in thread From: Arnd Bergmann @ 2018-04-19 21:05 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 19, 2018 at 7:09 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 19/04/18 17:35, Florian Fainelli wrote: > >>> Using a reserved-memory entry would be far less error prone, would >>> clearly show the intents of the specific regions being declared, and >>> could, through the use of additional properties (e.g: "map", as opposed >>> to "no-map") indicate a possible need to map that DRAM region into >>> kernel virtual memory and using e.g: memremap() APIs. >>> > > Sorry for missing this. For some reasons, I had assumed memremap was > specifically for DRAM and when I wrote the driver, I was testing on > our internal Juno dev platform which has dedicated SRAM for the > communication with remote control processor. However having a quick look > at the API, I see no reasons to not use that as it takes care of using > ioremap if required. But I am wondering why the generic SRAM driver in > drivers/misc/sram.c uses ioremap. > >>> Also the semantics of ioremap() are not particularly portable from one >>> architecture to another whereas memremap() is IMHO. >>> > > Again, wasn't aware of that. > > Arnd, > > Do you see any issues moving from ioremap to memremap for SCMI ? > I am asking you as you would have seen various platforms having multiple > options for shared IPC memory and wanted to get you feedback. Good question. The important point here is that whatever way the kernel maps the buffer must match whatever the other end does: - If you have regular RAM that is accessed using the CPU cache from firmware running on the same CPU, or using a cache-coherent method from a remote processor, you need to use memremap(). - If you have device registers somewhere, you must use ioremap() or possibly ioremap_wc(). - A dedicated SRAM is a bit tricky, usually that kind of buffer is accessed without going through the cache, so memremap() would be wrong. ioremap() is probably safe here. - The last case would get you into real trouble: a memory buffer at a fixed location that is physicall in system RAM and excluded from the memory visible to the OS, but accessed from a remote processor that is not cache-coherent with your CPU caches. Here, you can't use memremap() since you want an uncached mapping, but ioremap() is probably also wrong. You could construct something using dma_map_single() that might work, but that's not what this API is meant for either. Arnd ^ permalink raw reply [flat|nested] 5+ messages in thread
* ioremap() use with specific mailbox drivers and SCMI 2018-04-19 21:05 ` Arnd Bergmann @ 2018-08-10 18:13 ` Florian Fainelli 0 siblings, 0 replies; 5+ messages in thread From: Florian Fainelli @ 2018-08-10 18:13 UTC (permalink / raw) To: linux-arm-kernel On 04/19/2018 02:05 PM, Arnd Bergmann wrote: > On Thu, Apr 19, 2018 at 7:09 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 19/04/18 17:35, Florian Fainelli wrote: >> >>>> Using a reserved-memory entry would be far less error prone, would >>>> clearly show the intents of the specific regions being declared, and >>>> could, through the use of additional properties (e.g: "map", as opposed >>>> to "no-map") indicate a possible need to map that DRAM region into >>>> kernel virtual memory and using e.g: memremap() APIs. >>>> >> >> Sorry for missing this. For some reasons, I had assumed memremap was >> specifically for DRAM and when I wrote the driver, I was testing on >> our internal Juno dev platform which has dedicated SRAM for the >> communication with remote control processor. However having a quick look >> at the API, I see no reasons to not use that as it takes care of using >> ioremap if required. But I am wondering why the generic SRAM driver in >> drivers/misc/sram.c uses ioremap. >> >>>> Also the semantics of ioremap() are not particularly portable from one >>>> architecture to another whereas memremap() is IMHO. >>>> >> >> Again, wasn't aware of that. >> >> Arnd, >> >> Do you see any issues moving from ioremap to memremap for SCMI ? >> I am asking you as you would have seen various platforms having multiple >> options for shared IPC memory and wanted to get you feedback. > > Good question. The important point here is that whatever way the > kernel maps the buffer must match whatever the other end does: > > - If you have regular RAM that is accessed using the CPU cache from > firmware running on the same CPU, or using a cache-coherent method > from a remote processor, you need to use memremap(). > > - If you have device registers somewhere, you must use ioremap() > or possibly ioremap_wc(). > > - A dedicated SRAM is a bit tricky, usually that kind of buffer is accessed > without going through the cache, so memremap() would be wrong. > ioremap() is probably safe here. > > - The last case would get you into real trouble: a memory buffer at > a fixed location that is physicall in system RAM and excluded from > the memory visible to the OS, but accessed from a remote processor > that is not cache-coherent with your CPU caches. Here, you can't > use memremap() since you want an uncached mapping, but ioremap() > is probably also wrong. You could construct something using > dma_map_single() that might work, but that's not what this API is > meant for either. I suppose as long as you have a struct page created for that memory area you should be fine with the DMA-API, right? Would have to that to happen, otherwise the reserved memory region is flagged with "nomap" and then you won't have any, and it's truly invisible to the Linux. Part of the problem is that, when using a "shmem" phandle, we have no clue about what address space we might be come from, we might be able to figure it out somehow, like, are within a node, within a bus, which appears to map to register space, or are we within a reserved-memory node, in which case, we likely point to RAM. ioremap() is actually quite conservative but what the HiSilicon DTS file I referred to is doing seems like a hack because it carves out the mailbox area from it's memory node's "reg" property, and besides silencing the warning in arch/arm*/mmm/ioremap.c about remapping RAM, I am not sure how this is any safer than ignoring the warning and still doing ioremap() against a region of RAM? -- Florian ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-10 18:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-19 16:25 ioremap() use with specific mailbox drivers and SCMI Florian Fainelli 2018-04-19 16:35 ` Florian Fainelli 2018-04-19 17:09 ` Sudeep Holla 2018-04-19 21:05 ` Arnd Bergmann 2018-08-10 18:13 ` Florian Fainelli
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.