[PATCH] board/freescale/vid : move platform specific definitions

[PATCH] board/freescale/vid : move platform specific definitions

[Wasim Khan]

From: Wasim Khan <wasim.khan@nxp.com>

VID is a common driver. Move platform specific definitions
to platform specific header files

Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
---
 board/freescale/common/vid.h     | 10 ----------
 include/configs/lx2160a_common.h |  7 +++++++
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/board/freescale/common/vid.h b/board/freescale/common/vid.h
index b34c080b4b..32e68fceb2 100644
--- a/board/freescale/common/vid.h
+++ b/board/freescale/common/vid.h
@@ -59,16 +59,6 @@
 #define PMBUS_CMD_VOUT_COMMAND          0x21
 #define PMBUS_CMD_PAGE_PLUS_WRITE       0x05
 
-#if defined(CONFIG_TARGET_LX2160AQDS) || defined(CONFIG_TARGET_LX2162AQDS) || \
-defined(CONFIG_TARGET_LX2160ARDB)
-/* Voltage monitor on channel 2*/
-#define I2C_VOL_MONITOR_BUS_V_OFFSET	0x2
-#define I2C_VOL_MONITOR_BUS_V_OVF	0x1
-#define I2C_VOL_MONITOR_BUS_V_SHIFT	3
-#define I2C_VOL_MONITOR_ADDR            0x63
-#define I2C_MUX_CH_VOL_MONITOR		0xA
-#endif
-
 int adjust_vdd(ulong vdd_override);
 u16 soc_get_fuse_vid(int vid_index);
 
diff --git a/include/configs/lx2160a_common.h b/include/configs/lx2160a_common.h
index 1ae7d37dd9..c2c4a8c7a1 100644
--- a/include/configs/lx2160a_common.h
+++ b/include/configs/lx2160a_common.h
@@ -86,6 +86,13 @@
 #define CONFIG_SYS_LS_MC_DRAM_DPL_OFFSET	0x00F20000
 #define CONFIG_SYS_LS_MC_BOOT_TIMEOUT_MS	5000
 
+/* Voltage monitor on channel 2*/
+#define I2C_VOL_MONITOR_BUS_V_OFFSET	0x2
+#define I2C_VOL_MONITOR_BUS_V_OVF	0x1
+#define I2C_VOL_MONITOR_BUS_V_SHIFT	3
+#define I2C_VOL_MONITOR_ADDR		0x63
+#define I2C_MUX_CH_VOL_MONITOR		0xA
+
 /* Define phy_reset function to boot the MC based on mcinitcmd.
  * This happens late enough to properly fixup u-boot env MAC addresses.
  */
-- 
2.25.1

Re: [PATCH] board/freescale/vid : move platform specific definitions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

On Tue, Sep 21, 2021 at 04:24:57PM +0200, Wasim Khan wrote:

> From: Wasim Khan <wasim.khan@nxp.com>
> 
> VID is a common driver. Move platform specific definitions
> to platform specific header files
> 
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> ---
>  board/freescale/common/vid.h     | 10 ----------
>  include/configs/lx2160a_common.h |  7 +++++++
>  2 files changed, 7 insertions(+), 10 deletions(-)

NAK.  Things need to move out of include/configs/ and not in to them,
please find another common header file to use.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] board/freescale/vid : move platform specific definitions

[Wasim Khan (OSS)]

Hi Tom,

> -----Original Message-----
> From: Tom Rini <trini@konsulko.com>
> Sent: Tuesday, September 21, 2021 8:14 PM
> To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>
> Cc: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; u-
> boot@lists.denx.de; Wasim Khan <wasim.khan@nxp.com>
> Subject: Re: [PATCH] board/freescale/vid : move platform specific definitions
> 
> On Tue, Sep 21, 2021 at 04:24:57PM +0200, Wasim Khan wrote:
> 
> > From: Wasim Khan <wasim.khan@nxp.com>
> >
> > VID is a common driver. Move platform specific definitions to platform
> > specific header files
> >
> > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > ---
> >  board/freescale/common/vid.h     | 10 ----------
> >  include/configs/lx2160a_common.h |  7 +++++++
> >  2 files changed, 7 insertions(+), 10 deletions(-)
> 
> NAK.  Things need to move out of include/configs/ and not in to them, please
> find another common header file to use.
> 
> --
> Tom

Thank you so much for review.
Header files 'include/configs/' are auto picked for platform we are using. 
I find it useful especially for common drivers like VID to auto pick required values for underneath platform from 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h' and we don't need changes in common driver.

(arch/Kconfig)
config SYS_CONFIG_NAME
        string
        help
          This option should contain the base name of board header file.
          The header file include/configs/<CONFIG_SYS_CONFIG_NAME>.h
          should be included from include/config.h.


(Same is recommended for add/remove boards: doc/README.kconfig)
         Define CONFIG_SYS_CONFIG_NAME="target" to include
         include/configs/<target>.h


Currently all NXP platforms (except LX2 series) are using CONFIG_SYS_CONFIG_NAME to include platform specific header file for VID driver. I extended the support for LX2 and because the changes are common for lx2160ardb, lx2160aqds and lx2162aqds , I added them to lx2160a_common.h. 

Do you want me to move changes from ' include/configs/lx2160a_common.h' to ' include/configs/lx2160ardb.h',  ' include/configs/lx2160aqds.h' and ' include/configs/lx2162aqds.h'  ?
 
Or you want me to avoid adding anything to 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h ' files ? Is there any reason to do so ?

Re: [PATCH] board/freescale/vid : move platform specific definitions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

On Fri, Sep 24, 2021 at 08:36:45AM +0000, Wasim Khan (OSS) wrote:
> Hi Tom,
> 
> > -----Original Message-----
> > From: Tom Rini <trini@konsulko.com>
> > Sent: Tuesday, September 21, 2021 8:14 PM
> > To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>
> > Cc: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; u-
> > boot@lists.denx.de; Wasim Khan <wasim.khan@nxp.com>
> > Subject: Re: [PATCH] board/freescale/vid : move platform specific definitions
> > 
> > On Tue, Sep 21, 2021 at 04:24:57PM +0200, Wasim Khan wrote:
> > 
> > > From: Wasim Khan <wasim.khan@nxp.com>
> > >
> > > VID is a common driver. Move platform specific definitions to platform
> > > specific header files
> > >
> > > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > > ---
> > >  board/freescale/common/vid.h     | 10 ----------
> > >  include/configs/lx2160a_common.h |  7 +++++++
> > >  2 files changed, 7 insertions(+), 10 deletions(-)
> > 
> > NAK.  Things need to move out of include/configs/ and not in to them, please
> > find another common header file to use.
> > 
> > --
> > Tom
> 
> Thank you so much for review.
> Header files 'include/configs/' are auto picked for platform we are using. 
> I find it useful especially for common drivers like VID to auto pick required values for underneath platform from 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h' and we don't need changes in common driver.
> 
> (arch/Kconfig)
> config SYS_CONFIG_NAME
>         string
>         help
>           This option should contain the base name of board header file.
>           The header file include/configs/<CONFIG_SYS_CONFIG_NAME>.h
>           should be included from include/config.h.
> 
> 
> (Same is recommended for add/remove boards: doc/README.kconfig)
>          Define CONFIG_SYS_CONFIG_NAME="target" to include
>          include/configs/<target>.h
> 
> 
> Currently all NXP platforms (except LX2 series) are using CONFIG_SYS_CONFIG_NAME to include platform specific header file for VID driver. I extended the support for LX2 and because the changes are common for lx2160ardb, lx2160aqds and lx2162aqds , I added them to lx2160a_common.h. 
> 
> Do you want me to move changes from ' include/configs/lx2160a_common.h' to ' include/configs/lx2160ardb.h',  ' include/configs/lx2160aqds.h' and ' include/configs/lx2162aqds.h'  ?
>  
> Or you want me to avoid adding anything to 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h ' files ? Is there any reason to do so ?

I want you to find a place outside of include/config/ for these values
to reside, yes.  The long term goal of moving everything to Kconfig
means that we will not have include/config/ headers anymore.  Further,
values like this which are not actually user-configurable should neither
be CONFIG-prefixed (which these are not, good!) nor reside in
include/config/ at all.  There should be some appropriate header perhaps
under arch/arm/include/asm/arch-fsl-layerscape/ to put these values.
And if they need to be shared a bit wider, a fsl-layerscape-common or
similar directory could be used (similar to
arch/arm/include/asm/ti-common/).  Does this make sense?  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] board/freescale/vid : move platform specific definitions

[Wasim Khan (OSS)]

Hi Tom,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Tom Rini
> Sent: Friday, September 24, 2021 5:24 PM
> To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>
> Cc: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; u-
> boot@lists.denx.de
> Subject: Re: [PATCH] board/freescale/vid : move platform specific definitions
> 
> On Fri, Sep 24, 2021 at 08:36:45AM +0000, Wasim Khan (OSS) wrote:
> > Hi Tom,
> >
> > > -----Original Message-----
> > > From: Tom Rini <trini@konsulko.com>
> > > Sent: Tuesday, September 21, 2021 8:14 PM
> > > To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>
> > > Cc: Priyanka Jain <priyanka.jain@nxp.com>; Varun Sethi
> > > <V.Sethi@nxp.com>; u- boot@lists.denx.de; Wasim Khan
> > > <wasim.khan@nxp.com>
> > > Subject: Re: [PATCH] board/freescale/vid : move platform specific
> > > definitions
> > >
> > > On Tue, Sep 21, 2021 at 04:24:57PM +0200, Wasim Khan wrote:
> > >
> > > > From: Wasim Khan <wasim.khan@nxp.com>
> > > >
> > > > VID is a common driver. Move platform specific definitions to
> > > > platform specific header files
> > > >
> > > > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > > > ---
> > > >  board/freescale/common/vid.h     | 10 ----------
> > > >  include/configs/lx2160a_common.h |  7 +++++++
> > > >  2 files changed, 7 insertions(+), 10 deletions(-)
> > >
> > > NAK.  Things need to move out of include/configs/ and not in to
> > > them, please find another common header file to use.
> > >
> > > --
> > > Tom
> >
> > Thank you so much for review.
> > Header files 'include/configs/' are auto picked for platform we are using.
> > I find it useful especially for common drivers like VID to auto pick required
> values for underneath platform from
> 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h' and we don't need changes
> in common driver.
> >
> > (arch/Kconfig)
> > config SYS_CONFIG_NAME
> >         string
> >         help
> >           This option should contain the base name of board header file.
> >           The header file include/configs/<CONFIG_SYS_CONFIG_NAME>.h
> >           should be included from include/config.h.
> >
> >
> > (Same is recommended for add/remove boards: doc/README.kconfig)
> >          Define CONFIG_SYS_CONFIG_NAME="target" to include
> >          include/configs/<target>.h
> >
> >
> > Currently all NXP platforms (except LX2 series) are using
> CONFIG_SYS_CONFIG_NAME to include platform specific header file for VID
> driver. I extended the support for LX2 and because the changes are common for
> lx2160ardb, lx2160aqds and lx2162aqds , I added them to lx2160a_common.h.
> >
> > Do you want me to move changes from ' include/configs/lx2160a_common.h'
> to ' include/configs/lx2160ardb.h',  ' include/configs/lx2160aqds.h' and '
> include/configs/lx2162aqds.h'  ?
> >
> > Or you want me to avoid adding anything to
> 'include/configs/<CONFIG_SYS_CONFIG_NAME>.h ' files ? Is there any reason to
> do so ?
> 
> I want you to find a place outside of include/config/ for these values to reside,
> yes.  The long term goal of moving everything to Kconfig means that we will not
> have include/config/ headers anymore.  Further, values like this which are not
> actually user-configurable should neither be CONFIG-prefixed (which these are
> not, good!) nor reside in include/config/ at all.  There should be some
> appropriate header perhaps under arch/arm/include/asm/arch-fsl-layerscape/
> to put these values.
> And if they need to be shared a bit wider, a fsl-layerscape-common or similar
> directory could be used (similar to arch/arm/include/asm/ti-common/).  Does
> this make sense?  Thanks.
> 
> --
> Tom


Thanks for explaining it to me. 
Because VID driver is applicable for arm/x86 platforms, I need to check what would be best place for the header file , but I understood your point. 

Thanks,
Wasim