Re: [PATCH v2 02/18] ARM: i.MX8M: bundle imx-scratch code

[Sascha Hauer <sha@pengutronix.de>]

On Thu, Jan 18, 2024 at 10:11:18AM +0100, Marco Felsch wrote:
> On 24-01-17, Sascha Hauer wrote:
> > On Tue, Jan 16, 2024 at 06:07:22PM +0100, Marco Felsch wrote:
> > > The scatch code spread over the code base which is ok as long as we use
> > > it for the bootrom-log only.
> > > 
> > > This commit bundles the scratch code into a own module to make it easier
> > > to adapt later on and to make the scratch space independent of
> > > CONFIG_ARCH_IMX_ROMAPI.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  arch/arm/mach-imx/Makefile       |  2 +-
> > >  arch/arm/mach-imx/romapi.c       |  8 --------
> > >  arch/arm/mach-imx/scratch.c      | 25 +++++++++++++++++++++++++
> > >  arch/arm/mach-imx/xload-common.c |  8 --------
> > >  include/mach/imx/romapi.h        |  2 +-
> > >  include/mach/imx/scratch.h       | 17 +++++++++++++++++
> > >  include/mach/imx/xload.h         | 11 -----------
> > >  7 files changed, 44 insertions(+), 29 deletions(-)
> > >  create mode 100644 arch/arm/mach-imx/scratch.c
> > >  create mode 100644 include/mach/imx/scratch.h
> > > 
> > > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> > > index 3a51ae87813a..9505cffa8111 100644
> > > --- a/arch/arm/mach-imx/Makefile
> > > +++ b/arch/arm/mach-imx/Makefile
> > > @@ -17,7 +17,7 @@ CFLAGS_imx6.o := -march=armv7-a
> > >  lwl-$(CONFIG_ARCH_IMX6) += imx6-mmdc.o
> > >  obj-$(CONFIG_ARCH_IMX7) += imx7.o
> > >  obj-$(CONFIG_ARCH_VF610) += vf610.o
> > > -obj-pbl-$(CONFIG_ARCH_IMX8M) += imx8m.o
> > > +obj-pbl-$(CONFIG_ARCH_IMX8M) += imx8m.o scratch.o
> > >  obj-$(CONFIG_ARCH_IMX9) += imx9.o
> > >  lwl-$(CONFIG_ARCH_IMX_ATF) += atf.o
> > >  obj-pbl-$(CONFIG_ARCH_IMX8M) += tzasc.o
> > > diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
> > > index 6d47f3c6d4af..e11f38202461 100644
> > > --- a/arch/arm/mach-imx/romapi.c
> > > +++ b/arch/arm/mach-imx/romapi.c
> > > @@ -228,14 +228,6 @@ const u32 *imx8m_get_bootrom_log(void)
> > >  	return NULL;
> > >  }
> > >  
> > > -static int imx8m_reserve_scratch_area(void)
> > > -{
> > > -	return PTR_ERR_OR_ZERO(request_sdram_region("scratch area",
> > > -				    (ulong)arm_mem_scratch_get(),
> > > -				    sizeof(struct imx_scratch_space)));
> > > -}
> > > -device_initcall(imx8m_reserve_scratch_area);
> > > -
> > >  void imx8m_save_bootrom_log(void *dest)
> > >  {
> > >  	const u32 *rom_log;
> > > diff --git a/arch/arm/mach-imx/scratch.c b/arch/arm/mach-imx/scratch.c
> > > new file mode 100644
> > > index 000000000000..0828b55ff8e5
> > > --- /dev/null
> > > +++ b/arch/arm/mach-imx/scratch.c
> > > @@ -0,0 +1,25 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +#include <asm/barebox-arm.h>
> > > +#include <init.h>
> > > +#include <linux/err.h>
> > > +#include <mach/imx/imx8m-regs.h>
> > > +#include <mach/imx/esdctl.h>
> > > +#include <mach/imx/scratch.h>
> > > +#include <memory.h>
> > > +
> > > +struct imx_scratch_space *__imx8m_scratch_space(int ddr_buswidth)
> > > +{
> > > +	ulong endmem = MX8M_DDR_CSD1_BASE_ADDR +
> > > +		imx8m_barebox_earlymem_size(ddr_buswidth);
> > > +
> > > +	return (void *)arm_mem_scratch(endmem);
> > > +}
> > > +
> > > +static int imx8m_reserve_scratch_area(void)
> > > +{
> > > +	return PTR_ERR_OR_ZERO(request_sdram_region("scratch area",
> > > +				    (ulong)arm_mem_scratch_get(),
> > > +				    sizeof(struct imx_scratch_space)));
> > > +}
> > > +device_initcall(imx8m_reserve_scratch_area);
> > 
> > This reservation doesn't seem to be i.MX specific. You could move this
> > to a more generic place.
> 
> You mean to use it for Rockchip,... as well or just for all i.MX SoCs? I
> wanted to keep this commit as simple as possible by just shifting the
> code from placeA to placeB without changing the logic nor the naming.

I didn't notice that this code was just moved, so it's ok for this
commit.

What I really meant is that this could be ARM generic as the scratch
area exists on all ARM machines. It's size is 32KiB, I am not sure why
sizeof(struct imx_scratch_space) is used here. It could be used to
request some other offset in the scratch area for somehing else, but do
we really want to go that path?
If this reservation is only meant to see something in the iomem output,
then we could make this reservation ARM generic.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |