| Message ID | 20240522002122.50796-1-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Fabio Estevam |
| Headers | show |
| Series | [v2] ARM: imx: verdin-imx8mm: Set CAN oscillator frequency based on model | expand |
On Wed, May 22, 2024 at 02:21:14AM +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@denx.de> > --- > Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Cc: Martyn Welch <martyn.welch@collabora.com> > Cc: Stefano Babic <sbabic@denx.de> > Cc: u-boot@lists.denx.de > --- > V2: - Update the comment in conditional statement > - Update ver_assembly check for 0055 rev.A/B/C/D > --- > 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..ef632d95f0a 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. Special case 20 MHz variant > + * here: > + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator > + * - 0059, V1.1A and V1.1B, use a 20MHz oscillator > + */ Any reason why you ignored my suggestion here? The variants you list here are the only one with a 20MHz oscillator, and this is correct. What is not correct is that 0055/0059 are the only variant with CAN functionality. We have other "prodid" with CAN functionality. With that said, the code is correct, thanks. I appreciate you taking care of this. Francesco
On 5/22/24 8:39 AM, Francesco Dolcini wrote: Hi, >> diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c >> index 55c02653da6..ef632d95f0a 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. Special case 20 MHz variant >> + * here: >> + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator >> + * - 0059, V1.1A and V1.1B, use a 20MHz oscillator >> + */ > > Any reason why you ignored my suggestion here? The variants you list > here are the only one with a 20MHz oscillator, and this is correct. > > What is not correct is that 0055/0059 are the only variant with CAN > functionality. We have other "prodid" with CAN functionality. > > With that said, the code is correct, thanks. I appreciate you taking care > of this. So ... what should I change for V3 ? Maybe just create me a diff I can squash into the patch before resend ? (I think I am a bit confused, I thought I addressed all the V1 feedback)
On Wed, May 22, 2024 at 03:23:51PM +0200, Marek Vasut wrote: > On 5/22/24 8:39 AM, Francesco Dolcini wrote: > > > diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c > > > index 55c02653da6..ef632d95f0a 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. Special case 20 MHz variant > > > + * here: > > > + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator > > > + * - 0059, V1.1A and V1.1B, use a 20MHz oscillator > > > + */ > > > > Any reason why you ignored my suggestion here? The variants you list > > here are the only one with a 20MHz oscillator, and this is correct. > > > > What is not correct is that 0055/0059 are the only variant with CAN > > functionality. We have other "prodid" with CAN functionality. > > > > With that said, the code is correct, thanks. I appreciate you taking care > > of this. > > So ... what should I change for V3 ? > > Maybe just create me a diff I can squash into the patch before resend ? (I > think I am a bit confused, I thought I addressed all the V1 feedback) Sorry for the confusion, here the diff. With that please add Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c index ef632d95f0a5..59cc28f652f1 100644 --- a/board/toradex/verdin-imx8mm/verdin-imx8mm.c +++ b/board/toradex/verdin-imx8mm/verdin-imx8mm.c @@ -134,11 +134,10 @@ int ft_board_setup(void *blob, struct bd_info *bd) goto exit; /* - * The actual "prodid" (PID4 in Toradex naming) that have the CAN - * functionality are 0055 and 0059. Special case 20 MHz variant - * here: - * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator - * - 0059, V1.1A and V1.1B, use a 20MHz oscillator + * The followings "prodid" (PID4 in Toradex naming) use + * a 20MHz CAN oscillator: + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D + * - 0059, V1.1A and V1.1B */ if ((tdx_hw_tag.ver_major == 1 && tdx_hw_tag.ver_minor == 1) && ((tdx_hw_tag.prodid == VERDIN_IMX8MMQ_IT &&
On 5/23/24 5:03 PM, Francesco Dolcini wrote: > On Wed, May 22, 2024 at 03:23:51PM +0200, Marek Vasut wrote: >> On 5/22/24 8:39 AM, Francesco Dolcini wrote: >>>> diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c >>>> index 55c02653da6..ef632d95f0a 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. Special case 20 MHz variant >>>> + * here: >>>> + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator >>>> + * - 0059, V1.1A and V1.1B, use a 20MHz oscillator >>>> + */ >>> >>> Any reason why you ignored my suggestion here? The variants you list >>> here are the only one with a 20MHz oscillator, and this is correct. >>> >>> What is not correct is that 0055/0059 are the only variant with CAN >>> functionality. We have other "prodid" with CAN functionality. >>> >>> With that said, the code is correct, thanks. I appreciate you taking care >>> of this. >> >> So ... what should I change for V3 ? >> >> Maybe just create me a diff I can squash into the patch before resend ? (I >> think I am a bit confused, I thought I addressed all the V1 feedback) > > Sorry for the confusion, here the diff. > With that please add Thank you. I missed this mail and now I found it. Fixed in V3.
diff --git a/board/toradex/verdin-imx8mm/verdin-imx8mm.c b/board/toradex/verdin-imx8mm/verdin-imx8mm.c index 55c02653da6..ef632d95f0a 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. Special case 20 MHz variant + * here: + * - 0055, V1.1A, V1.1B, V1.1C and V1.1D, use a 20MHz oscillator + * - 0059, V1.1A and V1.1B, use a 20MHz oscillator + */ + 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 */ + freq = 20000000; + } + + ret = fdt_setprop_u32(blob, canoscoff, "clock-frequency", freq); + if (ret < 0) { + printf("Failed to set CAN oscillator clock-frequency, ret=%d\n", + ret); + } + +exit: return ft_common_board_setup(blob, bd); } #endif
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@denx.de> --- Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com> Cc: Fabio Estevam <festevam@gmail.com> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> Cc: Marcel Ziswiler <marcel.ziswiler@toradex.com> Cc: Martyn Welch <martyn.welch@collabora.com> Cc: Stefano Babic <sbabic@denx.de> Cc: u-boot@lists.denx.de --- V2: - Update the comment in conditional statement - Update ver_assembly check for 0055 rev.A/B/C/D --- board/toradex/verdin-imx8mm/verdin-imx8mm.c | 30 +++++++++++++++++++++ 1 file changed, 30 insertions(+)