On Monday, 3 October 2016 15:49:33 GMT Simon Glass wrote:
Hi Paul,
On 1 October 2016 at 08:19, Paul Burton paul.burton@imgtec.com wrote:
Most architectures currently supported by U-Boot use trivial implementations of map_to_physmem & virt_to_phys which simply cast a physical address to a pointer for use a virtual address & vice-versa. This results in a lot of duplicate implementations of these mapping functions.
The functions provided by different architectures also differs, with some having implementations of phys_to_virt & others not. A later patch in this series will make use of phys_to_virt, so requires that it be provided for all architectures.
This patch introduces an asm-generic/io.h which provides generic implementations of address mapping functions, allowing the duplication of them between architectures to be removed. Once architectures are converted to make use of this generic header it will also ensure that all of phys_to_virt, virt_to_phys, map_physmem & unmap_physmem are provided. The 2 families of functions differ in that map_physmem may create dynamic mappings whilst phys_to_virt may not & therefore is more limited in scope but doesn't require information such as a length & flags.
This patch doesn't convert any architectures to make use of this generic header - later patches in the series will do so.
Signed-off-by: Paul Burton paul.burton@imgtec.com Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Alison Wang alison.wang@freescale.com Cc: Angelo Dureghello angelo@sysam.it Cc: Bin Meng bmeng.cn@gmail.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Francois Retief fgretief@spaceteq.co.za Cc: Macpaul Lin macpaul@andestech.com Cc: Michal Simek monstr@monstr.eu Cc: Mike Frysinger vapier@gentoo.org Cc: Nobuhiro Iwamatsu iwamatsu@nigauri.org Cc: Scott McNutt smcnutt@psyent.com Cc: Sonic Zhang sonic.adi@gmail.com Cc: Thomas Chou thomas@wytron.com.tw Cc: Wolfgang Denk wd@denx.de
include/asm-generic/io.h | 110 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 include/asm-generic/io.h
Reviewed-by: Simon Glass sjg@chromium.org
Question and nits below.
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h new file mode 100644 index 0000000..dd3a46d --- /dev/null +++ b/include/asm-generic/io.h @@ -0,0 +1,110 @@ +/*
- Generic I/O functions.
- Copyright (c) 2016 Imagination Technologies Ltd.
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef __ASM_GENERIC_IO_H__ +#define __ASM_GENERIC_IO_H__
+/*
- This file should be included at the end of each architecture-specific
- asm/io.h such that we may provide generic implementations without
- conflicting with architecture-specific code.
- */
+#ifndef __ASSEMBLY__
+/**
- phys_to_virt() - Return a virtual address mapped to a given physical
address + * @paddr: the physical address
@return
Hi Simon,
I was under the impression that we're following the kernel-doc style, both based on the style of existing comments & the statement from the CodingStyle page of the wiki:
U-Boot adopted the kernel-doc annotation style, this is the only exception from multi-line comment rule of Coding Style. While not mandatory, adding documentation is strongly advised. The Linux kernel kernel-doc document applies with no changes.
(From http://www.denx.de/wiki/U-Boot/CodingStyle)
The kernel-doc-nano-HOWTO.txt file linked to from that wiki paragraph & included in the Linux kernel source shows this example:
/** * foobar() - short function description of foobar * @arg1: Describe the first argument to foobar. * @arg2: Describe the second argument to foobar. * One can provide multiple line descriptions * for arguments. * * A longer description, with more discussion of the function foobar() * that might be useful to those using or modifying it. Begins with * empty comment line, and may include additional embedded empty * comment lines. * * The longer description can have multiple paragraphs. * * Return: Describe the return value of foobar. */
Nowhere does it use @return & that's not what's done in Linux, so my belief is that having a "Return:" line at the end of the comment is the right way.
- Returns a virtual address which the CPU can access that maps to the
physical + * address @paddr. This should only be used where it is known that no dynamic + * mapping is required. In general, map_physmem should be used instead. + *
- Returns: a virtual address which maps to @paddr
Why two Returns?
The first is just a part of the paragraph explaining what the function does. I could start it with "This function returns" if you really want, but I don't see as that makes it any clearer. The second is the kernel-doc style description of what the function returns, as described above.
- */
+#ifndef phys_to_virt +static inline void *phys_to_virt(phys_addr_t paddr) +{
return (void *)(unsigned long)paddr;+} +#endif
+/**
- virt_to_phys() - Return the physical address that a virtual address
maps to + * @vaddr: the virtual address
@return
Ditto.
- Returns the physical address which the CPU-accessible virtual address
@vaddr + * maps to.
- Returns: the physical address which @vaddr maps to
Why two Returns?
Ditto.
- */
+#ifndef virt_to_phys +static inline phys_addr_t virt_to_phys(void *vaddr) +{
return (phys_addr_t)((unsigned long)vaddr);+} +#endif
+/*
- Flags for use with map_physmem() & unmap_physmem(). Architectures need
not + * support all of these, in which case they will be defined as zero here & + * ignored. Callers that may run on multiple architectures should therefore + * treat them as hints rather than requirements.
- */
+#ifndef MAP_NOCACHE +# define MAP_NOCACHE 0 /* Produce an uncached mapping */ +#endif +#ifndef MAP_WRCOMBINE +# define MAP_WRCOMBINE 0 /* Allow write-combining on the mapping */ +#endif +#ifndef MAP_WRBACK +# define MAP_WRBACK 0 /* Map using write-back caching */ +#endif +#ifndef MAP_WRTHROUGH +# define MAP_WRTHROUGH 0 /* Map using write-through caching */ +#endif
It seems odd to make these 0 when not supported. How could an arch know that it was requested, and then complain when an unsupported flag is passed?
That's true, it couldn't unless it explicitly defined ones that it doesn't support. Given that architectures already #define each of the above to 0 in the cases that they don't support this doesn't change that problem though. I'd suggest that we improve this separately, otherwise we'd risk this change breaking anything which sets flags that some architectures might simply want to ignore.
+/**
- map_physmem() - Return a virtual address mapped to a given physical
address + * @paddr: the physical address
- @len: the length of the required mapping
- @flags: flags affecting the type of mapping
@return
See above.
- Return a virtual address through which the CPU may access the memory
at
- physical address @paddr. The mapping will be valid for at least @len
bytes, + * and may be affected by flags passed to the @flags argument. This function + * may create new mappings, so should generally be paired with a matching call + * to unmap_physmem once the caller is finished with the memory in question. + *
@return
Ditto.
- Returns: a virtual address suitably mapped to @paddr
- */
+#ifndef map_physmem +static inline void * +map_physmem(phys_addr_t paddr, unsigned long len, unsigned long flags)
Can you join those two lines and wrap the args if needed??
Sure.
Thanks, Paul
+{
return phys_to_virt(paddr);+} +#endif
+/**
- unmap_physmem() - Remove mappings created by a prior call to
map_physmem() + * @vaddr: the virtual address which map_physmem() previously returned + * @flags: flags matching those originally passed to map_physmem() + *
- Unmap memory which was previously mapped by a call to map_physmem().
If
- map_physmem() dynamically created a mapping for the memory in question
then + * unmap_physmem() will remove that mapping.
- */
+#ifndef unmap_physmem +static inline void unmap_physmem(void *vaddr, unsigned long flags) +{ +} +#endif
+#endif /* !__ASSEMBLY__ */
+#endif /* __ASM_GENERIC_IO_H__ */
2.10.0
REgards, Simon