Re: [PATCH] ARM: i.MX: Kamstrup mx7 concentrator board support

[Lars Pedersen <lapeddk@gmail.com>]

Hello again. Thanks for the quick response :)

On Mon, 12 Apr 2021 at 13:24, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Lars,
>
> On 12.04.21 12:45, Lars Pedersen wrote:
> >>> +#define BOARD_RESTART_GPIO IMX_GPIO_NR(7, 12)
> >>> +#define TPM_RESET_GPIO IMX_GPIO_NR(3, 8)
> >>> +
> >>> +static void kamstrup_mx7_tpm_reset(void)
> >>> +{
> >>> +     imx7_setup_pad(MX7D_PAD_LCD_DATA03__GPIO3_IO8);
> >>> +
> >>> +     gpio_request(TPM_RESET_GPIO, "tpm-reset");
> >>> +     gpio_direction_output(TPM_RESET_GPIO, 1);
> >>> +     mdelay(100);
> >>> +     gpio_set_value(TPM_RESET_GPIO, 0);
> >>> +     mdelay(100);
> >>> +     gpio_set_value(TPM_RESET_GPIO, 1);
> >>> +     gpio_free(TPM_RESET_GPIO);
> >>
> >> We are trying to cut down on code that doesn't use the driver model.
> >> Couldn't this be represented as a gpio-hog in the device tree or
> >> a reset line for the SPI device?
> >
> > I can't find anything in the DT binding for the SPI/TPM driver to use
> > a reset line.
>
> Proper way would be for this to be added into Linux then,
> but that's out of scope for the patch here.
>
> > Can a DT gpio-hog toggle the pin? Don't you need a
> > driver for this?
>
> You can't pulse with a gpio-hog. I assumed the TPM is in reset by
> default.
>
> > I see the following options:
> >
> > 1) gpio-hog with a label and use new gpiolib in board.c.
> > 2) gpio-hog and control the pin in a boot script.
>
> If there's no straight-forward way to do it without board code,
> it's ok the way it is with one change though: I missed it the first
> time, but your board breaks multi-image support because you don't
> check whether the initcall is indeed running on your board (See for
> example imx_v7_defconfig, which builds over a hundred boards at once).
>
> Easiest way to get this right is to write a board driver.
> See arch/arm/boards/lxa-mc1/board.c for an example.

In arch/arm/boards/freescale-mx7-sabresd/board.c they do something like:

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

Can't I just do the same?

if (!of_machine_is_compatible("kam,imx7d-flex-concentrator-mfg"))
  return 0;

The board driver in arch/arm/boards/lxa-mc1/board.c seems more complicated.

>
> >>> +}
> >>> +
> >>> +static void __noreturn kamstrup_mx7_board_restart_gpio(struct restart_handler *rst)
> >>> +{
> >>> +     imx7_setup_pad(MX7D_PAD_ENET1_TX_CLK__GPIO7_IO12);
> >>> +
> >>> +     gpio_direction_output(BOARD_RESTART_GPIO, 0);
> >>> +     mdelay(1);
> >>> +     gpio_set_value(BOARD_RESTART_GPIO, 0);
> >>
> >> I just sent out a patch[1] with a driver implementing the "gpio-restart" device
> >> tree binding. Could you test that one and use it here instead?
> >>
> >
> > I have applied the patches on a master branch (Last patch failed to
> > apply but was only watchdog) and the gpio-restart works perfectly.
>
> Thanks for testing! You can add your Tested-By on the patch in question
> if you like.
>
> >
> >>> +
> >>> +     hang();
> >>> +}
> >>> +
> >>> +static int kamstrup_mx7_concentrator_coredevices_init(void)
> >>> +{
> >>> +     kamstrup_mx7_tpm_reset();
> >>> +     restart_handler_register_fn("board", kamstrup_mx7_board_restart_gpio);
> >>> +     barebox_set_model("Kamstrup OMNIA Concentrator");
> >>
> >> The default model name is "Kamstrup OMNIA Flex Concentrator".
> >> If that's too long, you could override /model in the barebox device tree.
> >> With the changes suggested above, you could drop board.c then altogether.
> >>
> >
> > I'll move this into the device tree or delete it entirely.
>
> Great. :-)
>
>
> Cheers,
> Ahmad
>
> --
> 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 |

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