Re: [PATCH 9/9] ARM: i.MX: Add support for NXP i.MX7 SABRESD board

From: Sam Ravnborg <sam@ravnborg.org>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 9/9] ARM: i.MX: Add support for NXP i.MX7 SABRESD board
Date: Mon, 24 Jul 2017 17:59:15 +0200	[thread overview]
Message-ID: <20170724155915.GB18294@ravnborg.org> (raw)
In-Reply-To: <20170724145400.2279-10-andrew.smirnov@gmail.com>

Hi Andrey.

On Mon, Jul 24, 2017 at 07:54:00AM -0700, Andrey Smirnov wrote:
> Add minimal code to support NXP i.MX7 SABRESD board. Tested to have
> working SD card and first Ethernet port as well as being able to boot
> upstream Linux kernel (4.12+).
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/boards/Makefile                           |  1 +
>  arch/arm/boards/freescale-mx7-sabresd/Makefile     |  3 +
>  arch/arm/boards/freescale-mx7-sabresd/board.c      | 59 ++++++++++++++++
>  .../flash-header-mx7-sabresd.imxcfg                | 79 ++++++++++++++++++++++
>  arch/arm/boards/freescale-mx7-sabresd/lowlevel.c   | 46 +++++++++++++
>  arch/arm/dts/Makefile                              |  2 +-
>  arch/arm/dts/imx7d-sdb.dts                         | 70 +++++++++++++++++++
>  arch/arm/mach-imx/Kconfig                          |  7 ++
>  images/Makefile.imx                                |  5 ++
>  9 files changed, 271 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boards/freescale-mx7-sabresd/Makefile
>  create mode 100644 arch/arm/boards/freescale-mx7-sabresd/board.c
>  create mode 100644 arch/arm/boards/freescale-mx7-sabresd/flash-header-mx7-sabresd.imxcfg
>  create mode 100644 arch/arm/boards/freescale-mx7-sabresd/lowlevel.c
>  create mode 100644 arch/arm/dts/imx7d-sdb.dts
> 
> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
> index 9bbdd684f..295a362bd 100644
> --- a/arch/arm/boards/Makefile
> +++ b/arch/arm/boards/Makefile
> @@ -148,3 +148,4 @@ obj-$(CONFIG_MACH_WARP7)			+= element14-warp7/
>  obj-$(CONFIG_MACH_VF610_TWR)			+= freescale-vf610-twr/
>  obj-$(CONFIG_MACH_ZII_RDU2)			+= zii-imx6q-rdu2/
>  obj-$(CONFIG_MACH_ZII_VF610_DEV)		+= zii-vf610-dev/
> +obj-$(CONFIG_MACH_FREESCALE_MX7_SABRESD)	+= freescale-mx7-sabresd/
Other entries in this file looks like they are sorted after the CONFIG_ name,
so add this entry also in alphabetical order.

> +++ b/arch/arm/boards/freescale-mx7-sabresd/board.c
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (C) 2016 Zodiac Inflight Innovation
Nitpick - 2016 or 2017?

> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
Should this file use the SPDX thingy used for licenses?

> +
> +#include <common.h>
> +#include <init.h>
> +#include <io.h>
> +#include <mach/imx7-regs.h>
> +#include <linux/phy.h>
> +#include <mfd/imx7-iomuxc-gpr.h>
> +
> +#define PHY_ID_BCM54220 0x600d8589
This define belongs in a header file as it may be used by more than one board.
In the kernel it is in: arch/arm/mach-imx/mach-imx7d.c
Not a header file, but a common imx7d file.

> +static int mx7_sabresd_coredevices_init(void)
> +{
> +	if (!of_machine_is_compatible("fsl,imx7d-sdb"))
> +		return 0;
> +
> +	phy_register_fixup_for_uid(PHY_ID_BCM54220, 0xffffffff,
> +				   bcm54220_phy_fixup);
> +
> +	return 0;
> +}
> +coredevice_initcall(mx7_sabresd_coredevices_init);

I fail to grasp why we shall test for:

> +     if (!of_machine_is_compatible("fsl,imx7d-sdb"))
> +             return 0;

It will in this case prevent us from calling
phy_register_fixup_for_uid() but not the other board specific setup.

And "return 0" will not signal any error.
The documentation suggest to add the above in a function
called by device_initcall(foo) - but I assume this is not relevant.
I likely just shows my ignorance towards multi-image support...

> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index d8abe452b..603d88922 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -99,6 +99,6 @@ pbl-dtb-$(CONFIG_MACH_ZII_VF610_DEV) += \
>  	vf610-zii-scu4-aib-rev-c.dtb.o
>  
>  pbl-dtb-$(CONFIG_MACH_AT91SAM9X5EK) += at91sam9x5ek.dtb.o
> -
> +pbl-dtb-$(CONFIG_MACH_FREESCALE_MX7_SABRESD) += imx7d-sdb.dtb.o
In this file entries are also sorted alpabetically by their CONFIG_ names.
The last one (CONFIG_MACH_AT91SAM9X5EK) is not, but blame whoever
reviewed the patch that added this entry.

	Sam
>  
> +config MACH_FREESCALE_MX7_SABRESD
> +	bool "NXP i.MX7 SabreSD Board"
> +	select ARCH_IMX7
> +	# Nedded to de-assert reset on Ethernet PHY
> +	select DRIVER_SPI_GPIO if DRIVER_NET_FEC_IMX
> +	select GPIO_74164 if DRIVER_NET_FEC_IMX

A small help text that introduces the board (maybe with an URL) would be nice.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox