[PATCH] ARM: imx: verdin-imx8mm: Set CAN oscillator frequency based on model

[Francesco Dolcini]

On Tue, May 21, 2024 at 01:19:24PM +0200, Francesco Dolcini wrote:
> On Tue, May 21, 2024 at 11:39:05AM +0200, Marek Vasut wrote:
> > The older i.MX8M Mini Verdin SoMs may came with 20 MHz SPI CAN controller
> > oscillator, the newer SoMs always use 40 MHz oscillator. Handle both by
> > overriding the oscillator frequency just before booting the kernel.
> > 
> > These are the known variants with 20 MHz oscillator:
> > - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator
> > - 0059, V1.1A and V1.1B, use a 20MHz oscillator
> > 
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > ---
> > Cc: "NXP i.MX U-Boot Team" <uboot-imx at nxp.com>
> > Cc: Fabio Estevam <festevam at gmail.com>
> > Cc: Francesco Dolcini <francesco.dolcini at toradex.com>
> > Cc: Marcel Ziswiler <marcel.ziswiler at toradex.com>
> > Cc: Philippe Schenker <philippe.schenker at toradex.com>
> > Cc: Martyn Welch <martyn.welch at collabora.com>
> > Cc: Stefano Babic <sbabic at denx.de>
> > Cc: u-boot at lists.denx.de
> > ---
> >  board/toradex/verdin-imx8mm/verdin-imx8mm.c | 30 +++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c
> > index 55c02653da6..b4a443ebfb0 100644
> > --- a/board/toradex/verdin-imx8mm/verdin-imx8mm.c
> > +++ b/board/toradex/verdin-imx8mm/verdin-imx8mm.c
> > @@ -125,6 +125,36 @@ int board_phys_sdram_size(phys_size_t *size)
> >  #if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
> >  int ft_board_setup(void *blob, struct bd_info *bd)
> >  {
> > +	const char *canoscpath = "/oscillator";
> > +	int freq = 40000000;	/* 40 MHz is used on most variants */
> > +	int canoscoff, ret;
> > +
> > +	canoscoff = fdt_path_offset(blob, canoscpath);
> > +	if (canoscoff < 0)	/* No CAN oscillator found. */
> > +		goto exit;
> > +
> > +	/*
> > +	 * The actual "prodid" (PID4 in Toradex naming) that have the CAN
> > +	 * functionality are 0055 and 0059.
> This is nor correct, we have more, they just always use 40MHz.
> 
> I would rephrase this:
> 
> /*
>  * 20MHz CAN oscillator product variants:
>  * - 0055, V1.1A, V1.1B, V1.1C and V1.1D
>  * - 0059, V1.1A and V1.1B
>  */
> 
> or in a different way, if you prefer, but you got the point.
> 
> > +	if ((tdx_hw_tag.ver_major == 1 && tdx_hw_tag.ver_minor == 1) &&
> > +	    ((tdx_hw_tag.prodid == VERDIN_IMX8MMQ_IT &&
> > +	      tdx_hw_tag.ver_assembly <= 1) ||	/* 0055 rev. A or B */
> > +	     (tdx_hw_tag.prodid == VERDIN_IMX8MMQ_WIFI_BT_IT &&
> > +	      tdx_hw_tag.ver_assembly <= 4))) {	/* 0059 rev. A/B/C/D */
> 
> ver_assembly is the hardware revision - 'A', e.g. 'A' is 0, 'D' is 3.
> 
> VERDIN_IMX8MMQ_IT is 0059
> VERDIN_IMX8MMQ_WIFI_BT_IT is 0055
> 
> with that said, it should be
> 
> 	if ((tdx_hw_tag.ver_major == 1 && tdx_hw_tag.ver_minor == 1) &&
> 	    ((tdx_hw_tag.prodid == VERDIN_IMX8MMQ_WIFI_BT_IT &&
> 	      tdx_hw_tag.ver_assembly <= 1) ||	/* 0055 rev. A or B */
> 	     (tdx_hw_tag.prodid == VERDIN_IMX8MMQ_IT &&
> 	      tdx_hw_tag.ver_assembly <= 3))) {	/* 0059 rev. A/B/C/D */

whoops. I confused myself.
What is correct is the table you have in the comment and the commit
message.

What is wrong is the comment in this if branch, and this confused me
(and probably also you, sorry).

	if ((tdx_hw_tag.ver_major == 1 && tdx_hw_tag.ver_minor == 1) &&
	    ((tdx_hw_tag.prodid == VERDIN_IMX8MMQ_IT &&
	      tdx_hw_tag.ver_assembly <= 1) ||	/* 0059 rev. A or B */
	     (tdx_hw_tag.prodid == VERDIN_IMX8MMQ_WIFI_BT_IT &&
	      tdx_hw_tag.ver_assembly <= 3))) {	/* 0055 rev. A/B/C/D */

Francesco