Re: [PATCH 6/7] bus: ti-sysc: Implement SoC revision handling

[Adam Ford]

On Fri, Feb 21, 2020 at 1:53 PM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
> We need to know SoC type and features for cases where the same SoC
> may be installed in various versions on the same board and would need
> a separate dts file otherwise for the different variants.
>
> For example, am3703 is pin compatible with omap3630, but has sgx and
> iva accelerators disabled. We must not try to access the sgx or iva
> module registers on am3703, and need to set the unavailable devices
> disabled early.
>
> Let's also detect omap3430 as that is needed for display subsystem
> (DSS) reset later on, and GP vs EMU or HS devices. Further SoC
> specific disabled device detection can be added as needed, such as
> dra71x vs dra76x rtc and usb4.
>
> Cc: Adam Ford <aford173@xxxxxxxxx>
> Cc: André Hentschel <nerv@xxxxxxxxxxx>
> Cc: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
> Cc: Keerthy <j-keerthy@xxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>

Tony,

I apologize for digging up an old thread, but I finally managed to get
my hands on an OMAP3503.  It seems like older kernels do not boot at
all or hang somewhere in the boot process.  I am still seeing a
difference in behavior between OMAP3503 and OMAP3530, where 3505
throws a bunch of splat and a kernel panic, while the 3530 appears to
boot happily.

Using the latest 5.17-rc6, I had to remove some IVA and SGX references
from omap_l3_smx.h to get the 3503 to stop crashing on boot.

Do you have any ideas how we can make the omap3_l3_app_bases and
omap3_l3_debug_bases more cleanly remove the IVA and SGX references
if/when OMAP3503 is detected?  I assume the same algorithm would have
to detect a AM3703 as well.  I'm trying to get my hands on an AM3703
to see if there is similar behavior as what I'm seeing on the
OMAP3503.

Thanks,

adam

> ---
>  arch/arm/mach-omap2/pdata-quirks.c    |   6 +
>  drivers/bus/ti-sysc.c                 | 194 +++++++++++++++++++++++++-
>  include/linux/platform_data/ti-sysc.h |   1 +
>  3 files changed, 200 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
> --- a/arch/arm/mach-omap2/pdata-quirks.c
> +++ b/arch/arm/mach-omap2/pdata-quirks.c
> @@ -397,10 +397,16 @@ static int ti_sysc_shutdown_module(struct device *dev,
>         return omap_hwmod_shutdown(cookie->data);
>  }
>
> +static bool ti_sysc_soc_type_gp(void)
> +{
> +       return omap_type() == OMAP2_DEVICE_TYPE_GP;
> +}
> +
>  static struct of_dev_auxdata omap_auxdata_lookup[];
>
>  static struct ti_sysc_platform_data ti_sysc_pdata = {
>         .auxdata = omap_auxdata_lookup,
> +       .soc_type_gp = ti_sysc_soc_type_gp,
>         .init_clockdomain = ti_sysc_clkdm_init,
>         .clkdm_deny_idle = ti_sysc_clkdm_deny_idle,
>         .clkdm_allow_idle = ti_sysc_clkdm_allow_idle,
> diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> --- a/drivers/bus/ti-sysc.c
> +++ b/drivers/bus/ti-sysc.c
> @@ -7,6 +7,7 @@
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
>  #include <linux/delay.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> @@ -15,15 +16,47 @@
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/slab.h>
> +#include <linux/sys_soc.h>
>  #include <linux/iopoll.h>
>
>  #include <linux/platform_data/ti-sysc.h>
>
>  #include <dt-bindings/bus/ti-sysc.h>
>
> +#define DIS_ISP                BIT(2)
> +#define DIS_IVA                BIT(1)
> +#define DIS_SGX                BIT(0)
> +
> +#define SOC_FLAG(match, flag)  { .machine = match, .data = (void *)(flag), }
> +
>  #define MAX_MODULE_SOFTRESET_WAIT              10000
>
> -static const char * const reg_names[] = { "rev", "sysc", "syss", };
> +enum sysc_soc {
> +       SOC_UNKNOWN,
> +       SOC_2420,
> +       SOC_2430,
> +       SOC_3430,
> +       SOC_3630,
> +       SOC_4430,
> +       SOC_4460,
> +       SOC_4470,
> +       SOC_5430,
> +       SOC_AM3,
> +       SOC_AM4,
> +       SOC_DRA7,
> +};
> +
> +struct sysc_address {
> +       unsigned long base;
> +       struct list_head node;
> +};
> +
> +struct sysc_soc_info {
> +       unsigned long general_purpose:1;
> +       enum sysc_soc soc;
> +       struct mutex list_lock;                 /* disabled modules list lock */
> +       struct list_head disabled_modules;
> +};
>
>  enum sysc_clocks {
>         SYSC_FCK,
> @@ -39,6 +72,8 @@ enum sysc_clocks {
>         SYSC_MAX_CLOCKS,
>  };
>
> +static struct sysc_soc_info *sysc_soc;
> +static const char * const reg_names[] = { "rev", "sysc", "syss", };
>  static const char * const clock_names[SYSC_MAX_CLOCKS] = {
>         "fck", "ick", "opt0", "opt1", "opt2", "opt3", "opt4",
>         "opt5", "opt6", "opt7",
> @@ -2382,6 +2417,154 @@ static void ti_sysc_idle(struct work_struct *work)
>                 pm_runtime_put_sync(ddata->dev);
>  }
>
> +/*
> + * SoC model and features detection. Only needed for SoCs that need
> + * special handling for quirks, no need to list others.
> + */
> +static const struct soc_device_attribute sysc_soc_match[] = {
> +       SOC_FLAG("OMAP242*", SOC_2420),
> +       SOC_FLAG("OMAP243*", SOC_2430),
> +       SOC_FLAG("OMAP3[45]*", SOC_3430),
> +       SOC_FLAG("OMAP3[67]*", SOC_3630),
> +       SOC_FLAG("OMAP443*", SOC_4430),
> +       SOC_FLAG("OMAP446*", SOC_4460),
> +       SOC_FLAG("OMAP447*", SOC_4470),
> +       SOC_FLAG("OMAP54*", SOC_5430),
> +       SOC_FLAG("AM433", SOC_AM3),
> +       SOC_FLAG("AM43*", SOC_AM4),
> +       SOC_FLAG("DRA7*", SOC_DRA7),
> +
> +       { /* sentinel */ },
> +};
> +
> +/*
> + * List of SoCs variants with disabled features. By default we assume all
> + * devices in the device tree are available so no need to list those SoCs.
> + */
> +static const struct soc_device_attribute sysc_soc_feat_match[] = {
> +       /* OMAP3430/3530 and AM3517 variants with some accelerators disabled */
> +       SOC_FLAG("AM3505", DIS_SGX),
> +       SOC_FLAG("OMAP3525", DIS_SGX),
> +       SOC_FLAG("OMAP3515", DIS_IVA | DIS_SGX),
> +       SOC_FLAG("OMAP3503", DIS_ISP | DIS_IVA | DIS_SGX),
> +
> +       /* OMAP3630/DM3730 variants with some accelerators disabled */
> +       SOC_FLAG("AM3703", DIS_IVA | DIS_SGX),
> +       SOC_FLAG("DM3725", DIS_SGX),
> +       SOC_FLAG("OMAP3611", DIS_ISP | DIS_IVA | DIS_SGX),
> +       SOC_FLAG("OMAP3615/AM3715", DIS_IVA),
> +       SOC_FLAG("OMAP3621", DIS_ISP),
> +
> +       { /* sentinel */ },
> +};
> +
> +static int sysc_add_disabled(unsigned long base)
> +{
> +       struct sysc_address *disabled_module;
> +
> +       disabled_module = kzalloc(sizeof(*disabled_module), GFP_KERNEL);
> +       if (!disabled_module)
> +               return -ENOMEM;
> +
> +       disabled_module->base = base;
> +
> +       mutex_lock(&sysc_soc->list_lock);
> +       list_add(&disabled_module->node, &sysc_soc->disabled_modules);
> +       mutex_unlock(&sysc_soc->list_lock);
> +
> +       return 0;
> +}
> +
> +/*
> + * One time init to detect the booted SoC and disable unavailable features.
> + * Note that we initialize static data shared across all ti-sysc instances
> + * so ddata is only used for SoC type. This can be called from module_init
> + * once we no longer need to rely on platform data.
> + */
> +static int sysc_init_soc(struct sysc *ddata)
> +{
> +       const struct soc_device_attribute *match;
> +       struct ti_sysc_platform_data *pdata;
> +       unsigned long features = 0;
> +
> +       if (sysc_soc)
> +               return 0;
> +
> +       sysc_soc = kzalloc(sizeof(*sysc_soc), GFP_KERNEL);
> +       if (!sysc_soc)
> +               return -ENOMEM;
> +
> +       mutex_init(&sysc_soc->list_lock);
> +       INIT_LIST_HEAD(&sysc_soc->disabled_modules);
> +       sysc_soc->general_purpose = true;
> +
> +       pdata = dev_get_platdata(ddata->dev);
> +       if (pdata && pdata->soc_type_gp)
> +               sysc_soc->general_purpose = pdata->soc_type_gp();
> +
> +       match = soc_device_match(sysc_soc_match);
> +       if (match && match->data)
> +               sysc_soc->soc = (int)match->data;
> +
> +       match = soc_device_match(sysc_soc_feat_match);
> +       if (!match)
> +               return 0;
> +
> +       if (match->data)
> +               features = (unsigned long)match->data;
> +
> +       /*
> +        * Add disabled devices to the list based on the module base.
> +        * Note that this must be done before we attempt to access the
> +        * device and have module revision checks working.
> +        */
> +       if (features & DIS_ISP)
> +               sysc_add_disabled(0x480bd400);
> +       if (features & DIS_IVA)
> +               sysc_add_disabled(0x5d000000);
> +       if (features & DIS_SGX)
> +               sysc_add_disabled(0x50000000);
> +
> +       return 0;
> +}
> +
> +static void sysc_cleanup_soc(void)
> +{
> +       struct sysc_address *disabled_module;
> +       struct list_head *pos, *tmp;
> +
> +       if (!sysc_soc)
> +               return;
> +
> +       mutex_lock(&sysc_soc->list_lock);
> +       list_for_each_safe(pos, tmp, &sysc_soc->disabled_modules) {
> +               disabled_module = list_entry(pos, struct sysc_address, node);
> +               list_del(pos);
> +               kfree(disabled_module);
> +       }
> +       mutex_unlock(&sysc_soc->list_lock);
> +}
> +
> +static int sysc_check_disabled_devices(struct sysc *ddata)
> +{
> +       struct sysc_address *disabled_module;
> +       struct list_head *pos;
> +       int error = 0;
> +
> +       mutex_lock(&sysc_soc->list_lock);
> +       list_for_each(pos, &sysc_soc->disabled_modules) {
> +               disabled_module = list_entry(pos, struct sysc_address, node);
> +               if (ddata->module_pa == disabled_module->base) {
> +                       dev_dbg(ddata->dev, "module disabled for this SoC\n");
> +                       error = -ENODEV;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&sysc_soc->list_lock);
> +
> +       return error;
> +}
> +
>  static const struct of_device_id sysc_match_table[] = {
>         { .compatible = "simple-bus", },
>         { /* sentinel */ },
> @@ -2400,6 +2583,10 @@ static int sysc_probe(struct platform_device *pdev)
>         ddata->dev = &pdev->dev;
>         platform_set_drvdata(pdev, ddata);
>
> +       error = sysc_init_soc(ddata);
> +       if (error)
> +               return error;
> +
>         error = sysc_init_match(ddata);
>         if (error)
>                 return error;
> @@ -2430,6 +2617,10 @@ static int sysc_probe(struct platform_device *pdev)
>
>         sysc_init_early_quirks(ddata);
>
> +       error = sysc_check_disabled_devices(ddata);
> +       if (error)
> +               return error;
> +
>         error = sysc_get_clocks(ddata);
>         if (error)
>                 return error;
> @@ -2560,6 +2751,7 @@ static void __exit sysc_exit(void)
>  {
>         bus_unregister_notifier(&platform_bus_type, &sysc_nb);
>         platform_driver_unregister(&sysc_driver);
> +       sysc_cleanup_soc();
>  }
>  module_exit(sysc_exit);
>
> diff --git a/include/linux/platform_data/ti-sysc.h b/include/linux/platform_data/ti-sysc.h
> --- a/include/linux/platform_data/ti-sysc.h
> +++ b/include/linux/platform_data/ti-sysc.h
> @@ -141,6 +141,7 @@ struct clk;
>
>  struct ti_sysc_platform_data {
>         struct of_dev_auxdata *auxdata;
> +       bool (*soc_type_gp)(void);
>         int (*init_clockdomain)(struct device *dev, struct clk *fck,
>                                 struct clk *ick, struct ti_sysc_cookie *cookie);
>         void (*clkdm_deny_idle)(struct device *dev,
> --
> 2.25.1