soc: imx: Move imx_get_soc_revision from mach-imx

There are a few drivers which call imx_get_soc_revision in order to
enable errata workarounds but this is only available on 32-bit arm.

Move the current globals to drivers/soc/imx/revision.c so that they're
also accessible on all imx8 with same name and semantics.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
This is not very pretty.

 arch/arm/mach-imx/common.h    |  1 -
 arch/arm/mach-imx/cpu.c       | 17 ++++-------------
 drivers/soc/imx/Makefile      |  1 +
 drivers/soc/imx/revision.c    | 19 +++++++++++++++++++
 drivers/soc/imx/soc-imx-scu.c |  2 ++
 drivers/soc/imx/soc-imx8.c    |  5 ++++-
 include/soc/imx/revision.h    |  1 +
 7 files changed, 31 insertions(+), 15 deletions(-)
 create mode 100644 drivers/soc/imx/revision.c

> From: Leonard Crestez [mailto:leonard.crestez@nxp.com]
> Sent: Wednesday, June 12, 2019 2:52 AM
> 
> There are a few drivers which call imx_get_soc_revision in order to enable
> errata workarounds but this is only available on 32-bit arm.
> 
> Move the current globals to drivers/soc/imx/revision.c so that they're also
> accessible on all imx8 with same name and semantics.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

The idea looks good to me.

Only a few minor comments,
otherwise:
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

> 
> ---
> This is not very pretty.
> 
>  arch/arm/mach-imx/common.h    |  1 -
>  arch/arm/mach-imx/cpu.c       | 17 ++++-------------
>  drivers/soc/imx/Makefile      |  1 +
>  drivers/soc/imx/revision.c    | 19 +++++++++++++++++++
>  drivers/soc/imx/soc-imx-scu.c |  2 ++
>  drivers/soc/imx/soc-imx8.c    |  5 ++++-
>  include/soc/imx/revision.h    |  1 +
>  7 files changed, 31 insertions(+), 15 deletions(-)  create mode 100644
> drivers/soc/imx/revision.c
> 
> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> index c51764a85fd7..5c06224986f4 100644
> --- a/arch/arm/mach-imx/common.h
> +++ b/arch/arm/mach-imx/common.h
> @@ -49,11 +49,10 @@ void mxc_restart(enum reboot_mode, const char *);
> void mxc_arch_reset_init(void __iomem *);  void imx1_reset_init(void
> __iomem *);  void imx_set_aips(void __iomem *);  void
> imx_aips_allow_unprivileged_access(const char *compat);  int
> mxc_device_init(void); -void imx_set_soc_revision(unsigned int rev);  void
> imx_init_revision_from_anatop(void);
>  struct device *imx_soc_device_init(void);  void imx6_enable_rbc(bool
> enable);  void imx_gpc_check_dt(void);  void
> imx_gpc_set_arm_power_in_lpm(bool power_off); diff --git
> a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c index
> 0b137eeffb61..bfc84f5a1312 100644
> --- a/arch/arm/mach-imx/cpu.c
> +++ b/arch/arm/mach-imx/cpu.c
> @@ -9,27 +9,16 @@
> 
>  #include "hardware.h"
>  #include "common.h"
> 
>  unsigned int __mxc_cpu_type;
> -static unsigned int imx_soc_revision;
> 
>  void mxc_set_cpu_type(unsigned int type)  {
>  	__mxc_cpu_type = type;
>  }
> 
> -void imx_set_soc_revision(unsigned int rev) -{
> -	imx_soc_revision = rev;
> -}
> -
> -unsigned int imx_get_soc_revision(void) -{
> -	return imx_soc_revision;
> -}
> -
>  void imx_print_silicon_rev(const char *cpu, int srev)  {
>  	if (srev == IMX_CHIP_REVISION_UNKNOWN)
>  		pr_info("CPU identified as %s, unknown revision\n", cpu);
>  	else
> @@ -77,10 +66,11 @@ struct device * __init imx_soc_device_init(void)  {
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
>  	struct device_node *root;
>  	const char *soc_id;
> +	int soc_rev;
>  	int ret;
> 
>  	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>  	if (!soc_dev_attr)
>  		return NULL;
> @@ -151,13 +141,14 @@ struct device * __init imx_soc_device_init(void)
>  	default:
>  		soc_id = "Unknown";
>  	}
>  	soc_dev_attr->soc_id = soc_id;
> 
> +	soc_rev = imx_get_soc_revision();
>  	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d.%d",
> -					   (imx_soc_revision >> 4) & 0xf,
> -					   imx_soc_revision & 0xf);
> +					   (soc_rev >> 4) & 0xf,
> +					   soc_rev & 0xf);
>  	if (!soc_dev_attr->revision)
>  		goto free_soc;
> 
>  	soc_dev = soc_device_register(soc_dev_attr);
>  	if (IS_ERR(soc_dev))
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> cf9ca42ff739..293a771127dd 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-y += revision.o
>  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
>  obj-$(CONFIG_ARCH_MXC) += soc-imx8.o
>  obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o diff --git
> a/drivers/soc/imx/revision.c b/drivers/soc/imx/revision.c new file mode
> 100644 index 000000000000..fc4cea2f25bd
> --- /dev/null
> +++ b/drivers/soc/imx/revision.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP.
> + */
> +#include <linux/module.h>

Maybe replaced by <linux/export.h>?

> +#include <soc/imx/revision.h>
> +
> +static unsigned int imx_soc_revision = IMX_CHIP_REVISION_UNKNOWN;
> +
> +void imx_set_soc_revision(unsigned int rev) {
> +	imx_soc_revision = rev;
> +}
> +
> +unsigned int imx_get_soc_revision(void) {
> +	return imx_soc_revision;
> +}
> +EXPORT_SYMBOL(imx_get_soc_revision);
> diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c index
> 676f612f6488..7c4106ff3e0f 100644
> --- a/drivers/soc/imx/soc-imx-scu.c
> +++ b/drivers/soc/imx/soc-imx-scu.c
> @@ -7,10 +7,11 @@
>  #include <linux/firmware/imx/sci.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <soc/imx/revision.h>
> 
>  #define IMX_SCU_SOC_DRIVER_NAME		"imx-scu-soc"
> 
>  static struct imx_sc_ipc *soc_ipc_handle;
> 
> @@ -85,10 +86,11 @@ static int imx_scu_soc_probe(struct platform_device
> *pdev)
>  		return -ENOMEM;
> 
>  	/* format revision value passed from SCU firmware */
>  	val = (id >> 5) & 0xf;
>  	val = (((val >> 2) + 1) << 4) | (val & 0x3);
> +	imx_set_soc_revision(val);
>  	soc_dev_attr->revision = kasprintf(GFP_KERNEL,
>  					   "%d.%d",
>  					   (val >> 4) & 0xf,
>  					   val & 0xf);
>  	if (!soc_dev_attr->revision) {
> diff --git a/drivers/soc/imx/soc-imx8.c b/drivers/soc/imx/soc-imx8.c index
> 3842d096daf0..465d2c6c6905 100644
> --- a/drivers/soc/imx/soc-imx8.c
> +++ b/drivers/soc/imx/soc-imx8.c
> @@ -8,10 +8,11 @@
>  #include <linux/of_address.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <soc/imx/revision.h>
> 
>  #define REV_B1				0x21
> 
>  #define IMX8MQ_SW_INFO_B1		0x40
>  #define IMX8MQ_SW_MAGIC_B1		0xff0055aa
> @@ -118,12 +119,14 @@ static int __init imx8_soc_init(void)
>  	}
> 
>  	data = id->data;
>  	if (data) {
>  		soc_dev_attr->soc_id = data->name;
> -		if (data->soc_revision)
> +		if (data->soc_revision) {
>  			soc_rev = data->soc_revision();
> +			imx_set_soc_revision(soc_rev & 0xFF);

s/0xFF/0xff

BTW if we really need a mask, maybe move it into imx_set_soc_revision().

> +		}
>  	}
> 
>  	soc_dev_attr->revision = imx8_revision(soc_rev);
>  	if (!soc_dev_attr->revision) {
>  		ret = -ENOMEM;
> diff --git a/include/soc/imx/revision.h b/include/soc/imx/revision.h index
> 9ea346924c35..5e7e2aea10ff 100644
> --- a/include/soc/imx/revision.h
> +++ b/include/soc/imx/revision.h
> @@ -30,8 +30,9 @@ int mx31_revision(void);  int mx35_revision(void);  int
> mx51_revision(void);  int mx53_revision(void);
> 
>  unsigned int imx_get_soc_revision(void);
> +void imx_set_soc_revision(unsigned int rev);
>  void imx_print_silicon_rev(const char *cpu, int srev);
> 
>  #endif /* __SOC_IMX_REVISION_H__ */
> --
> 2.17.1

Hi,
On Tue, Jun 11, 2019 at 09:51:57PM +0300, Leonard Crestez wrote:
> There are a few drivers which call imx_get_soc_revision in order to
> enable errata workarounds but this is only available on 32-bit arm.
> 
> Move the current globals to drivers/soc/imx/revision.c so that they're
> also accessible on all imx8 with same name and semantics.

I thought soc_device_match() is preferred over imx_get_soc_revision()
nowadays?
Cheers,
 -- Guido

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> This is not very pretty.
> 
>  arch/arm/mach-imx/common.h    |  1 -
>  arch/arm/mach-imx/cpu.c       | 17 ++++-------------
>  drivers/soc/imx/Makefile      |  1 +
>  drivers/soc/imx/revision.c    | 19 +++++++++++++++++++
>  drivers/soc/imx/soc-imx-scu.c |  2 ++
>  drivers/soc/imx/soc-imx8.c    |  5 ++++-
>  include/soc/imx/revision.h    |  1 +
>  7 files changed, 31 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/soc/imx/revision.c
> 
> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> index c51764a85fd7..5c06224986f4 100644
> --- a/arch/arm/mach-imx/common.h
> +++ b/arch/arm/mach-imx/common.h
> @@ -49,11 +49,10 @@ void mxc_restart(enum reboot_mode, const char *);
>  void mxc_arch_reset_init(void __iomem *);
>  void imx1_reset_init(void __iomem *);
>  void imx_set_aips(void __iomem *);
>  void imx_aips_allow_unprivileged_access(const char *compat);
>  int mxc_device_init(void);
> -void imx_set_soc_revision(unsigned int rev);
>  void imx_init_revision_from_anatop(void);
>  struct device *imx_soc_device_init(void);
>  void imx6_enable_rbc(bool enable);
>  void imx_gpc_check_dt(void);
>  void imx_gpc_set_arm_power_in_lpm(bool power_off);
> diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c
> index 0b137eeffb61..bfc84f5a1312 100644
> --- a/arch/arm/mach-imx/cpu.c
> +++ b/arch/arm/mach-imx/cpu.c
> @@ -9,27 +9,16 @@
>  
>  #include "hardware.h"
>  #include "common.h"
>  
>  unsigned int __mxc_cpu_type;
> -static unsigned int imx_soc_revision;
>  
>  void mxc_set_cpu_type(unsigned int type)
>  {
>  	__mxc_cpu_type = type;
>  }
>  
> -void imx_set_soc_revision(unsigned int rev)
> -{
> -	imx_soc_revision = rev;
> -}
> -
> -unsigned int imx_get_soc_revision(void)
> -{
> -	return imx_soc_revision;
> -}
> -
>  void imx_print_silicon_rev(const char *cpu, int srev)
>  {
>  	if (srev == IMX_CHIP_REVISION_UNKNOWN)
>  		pr_info("CPU identified as %s, unknown revision\n", cpu);
>  	else
> @@ -77,10 +66,11 @@ struct device * __init imx_soc_device_init(void)
>  {
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
>  	struct device_node *root;
>  	const char *soc_id;
> +	int soc_rev;
>  	int ret;
>  
>  	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>  	if (!soc_dev_attr)
>  		return NULL;
> @@ -151,13 +141,14 @@ struct device * __init imx_soc_device_init(void)
>  	default:
>  		soc_id = "Unknown";
>  	}
>  	soc_dev_attr->soc_id = soc_id;
>  
> +	soc_rev = imx_get_soc_revision();
>  	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d.%d",
> -					   (imx_soc_revision >> 4) & 0xf,
> -					   imx_soc_revision & 0xf);
> +					   (soc_rev >> 4) & 0xf,
> +					   soc_rev & 0xf);
>  	if (!soc_dev_attr->revision)
>  		goto free_soc;
>  
>  	soc_dev = soc_device_register(soc_dev_attr);
>  	if (IS_ERR(soc_dev))
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index cf9ca42ff739..293a771127dd 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-y += revision.o
>  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
>  obj-$(CONFIG_ARCH_MXC) += soc-imx8.o
>  obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
> diff --git a/drivers/soc/imx/revision.c b/drivers/soc/imx/revision.c
> new file mode 100644
> index 000000000000..fc4cea2f25bd
> --- /dev/null
> +++ b/drivers/soc/imx/revision.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP.
> + */
> +#include <linux/module.h>
> +#include <soc/imx/revision.h>
> +
> +static unsigned int imx_soc_revision = IMX_CHIP_REVISION_UNKNOWN;
> +
> +void imx_set_soc_revision(unsigned int rev)
> +{
> +	imx_soc_revision = rev;
> +}
> +
> +unsigned int imx_get_soc_revision(void)
> +{
> +	return imx_soc_revision;
> +}
> +EXPORT_SYMBOL(imx_get_soc_revision);
> diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
> index 676f612f6488..7c4106ff3e0f 100644
> --- a/drivers/soc/imx/soc-imx-scu.c
> +++ b/drivers/soc/imx/soc-imx-scu.c
> @@ -7,10 +7,11 @@
>  #include <linux/firmware/imx/sci.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <soc/imx/revision.h>
>  
>  #define IMX_SCU_SOC_DRIVER_NAME		"imx-scu-soc"
>  
>  static struct imx_sc_ipc *soc_ipc_handle;
>  
> @@ -85,10 +86,11 @@ static int imx_scu_soc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	/* format revision value passed from SCU firmware */
>  	val = (id >> 5) & 0xf;
>  	val = (((val >> 2) + 1) << 4) | (val & 0x3);
> +	imx_set_soc_revision(val);
>  	soc_dev_attr->revision = kasprintf(GFP_KERNEL,
>  					   "%d.%d",
>  					   (val >> 4) & 0xf,
>  					   val & 0xf);
>  	if (!soc_dev_attr->revision) {
> diff --git a/drivers/soc/imx/soc-imx8.c b/drivers/soc/imx/soc-imx8.c
> index 3842d096daf0..465d2c6c6905 100644
> --- a/drivers/soc/imx/soc-imx8.c
> +++ b/drivers/soc/imx/soc-imx8.c
> @@ -8,10 +8,11 @@
>  #include <linux/of_address.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <soc/imx/revision.h>
>  
>  #define REV_B1				0x21
>  
>  #define IMX8MQ_SW_INFO_B1		0x40
>  #define IMX8MQ_SW_MAGIC_B1		0xff0055aa
> @@ -118,12 +119,14 @@ static int __init imx8_soc_init(void)
>  	}
>  
>  	data = id->data;
>  	if (data) {
>  		soc_dev_attr->soc_id = data->name;
> -		if (data->soc_revision)
> +		if (data->soc_revision) {
>  			soc_rev = data->soc_revision();
> +			imx_set_soc_revision(soc_rev & 0xFF);
> +		}
>  	}
>  
>  	soc_dev_attr->revision = imx8_revision(soc_rev);
>  	if (!soc_dev_attr->revision) {
>  		ret = -ENOMEM;
> diff --git a/include/soc/imx/revision.h b/include/soc/imx/revision.h
> index 9ea346924c35..5e7e2aea10ff 100644
> --- a/include/soc/imx/revision.h
> +++ b/include/soc/imx/revision.h
> @@ -30,8 +30,9 @@ int mx31_revision(void);
>  int mx35_revision(void);
>  int mx51_revision(void);
>  int mx53_revision(void);
>  
>  unsigned int imx_get_soc_revision(void);
> +void imx_set_soc_revision(unsigned int rev);
>  void imx_print_silicon_rev(const char *cpu, int srev);
>  
>  #endif /* __SOC_IMX_REVISION_H__ */
> -- 
> 2.17.1
>

On 12.06.2019 09:07, Guido Günther wrote:
> On Tue, Jun 11, 2019 at 09:51:57PM +0300, Leonard Crestez wrote:

>> There are a few drivers which call imx_get_soc_revision in order to
>> enable errata workarounds but this is only available on 32-bit arm.
>>
>> Move the current globals to drivers/soc/imx/revision.c so that they're
>> also accessible on all imx8 with same name and semantics.
>>
>> ---
>> This is not very pretty.
 >
 > I thought soc_device_match() is preferred over imx_get_soc_revision()
 > nowadays?

That looks like a much better solution, thanks for pointing it out.

--
Regards,
Leonard