[U-Boot] [PATCH 01/27] Provide a generic io.h & address mapping functions
Paul Burton
paul.burton at imgtec.com
Thu Nov 17 16:32:50 CET 2016
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 at 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 at imgtec.com>
> > Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> > Cc: Alexey Brodkin <alexey.brodkin at synopsys.com>
> > Cc: Alison Wang <alison.wang at freescale.com>
> > Cc: Angelo Dureghello <angelo at sysam.it>
> > Cc: Bin Meng <bmeng.cn at gmail.com>
> > Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> > Cc: Francois Retief <fgretief at spaceteq.co.za>
> > Cc: Macpaul Lin <macpaul at andestech.com>
> > Cc: Michal Simek <monstr at monstr.eu>
> > Cc: Mike Frysinger <vapier at gentoo.org>
> > Cc: Nobuhiro Iwamatsu <iwamatsu at nigauri.org>
> > Cc: Scott McNutt <smcnutt at psyent.com>
> > Cc: Sonic Zhang <sonic.adi at gmail.com>
> > Cc: Thomas Chou <thomas at wytron.com.tw>
> > Cc: Wolfgang Denk <wd at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161117/27cd0ef4/attachment.sig>
More information about the U-Boot
mailing list