On Tue, Sep 25, 2018 at 04:52:46PM +0530, Sandipan Patra wrote:
> 1. Implementation of:
> tegra_pmc_get_system_reset_reason and
> tegra_pmc_get_system_reset_level
> These APIs provide information about tegra reset reason and level
> respectively.
>
> 2. sysfs entries:
> /sys/devices/platform/<address>.pmc/tegra_reset_reason and
> /sys/devices/platform/<address>.pmc/tegra_reset_level
> are implemented in readonly mode to fetch tegra reset reason and
> tegra reset level information.
>
> Signed-off-by: Sandipan Patra <spatra@xxxxxxxxxx>
> ---
> drivers/soc/tegra/pmc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 131 insertions(+), 1 deletion(-)
Hi Sandipan,
Overall I think this is pretty nice. We've never had a use for checking
the reset reason before, so could you detail (in the commit message) how
you intend this to be used?
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index ab719fa..ac74fee 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -2,6 +2,7 @@
> * drivers/soc/tegra/pmc.c
> *
> * Copyright (c) 2010 Google, Inc
> + * Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
> *
> * Author:
> * Colin Cross <ccross@xxxxxxxxxx>
> @@ -92,7 +93,8 @@
> #define PMC_SENSOR_CTRL_SCRATCH_WRITE BIT(2)
> #define PMC_SENSOR_CTRL_ENABLE_RST BIT(1)
>
> -#define PMC_RST_STATUS 0x1b4
> +#define PMC_RST_STATUS_T30 0x1b4
What about Tegra20? We've always used PMC_RST_STATUS on all generations,
so I would expect PMC_RST_STATUS_T30 to actually still work on Tegra20?
> +#define PMC_RST_STATUS_T186 0x70
> #define PMC_RST_STATUS_POR 0
> #define PMC_RST_STATUS_WATCHDOG 1
> #define PMC_RST_STATUS_SENSOR 2
> @@ -125,6 +127,13 @@
>
> #define GPU_RG_CNTRL 0x2d4
>
> +#define PMC_RST_LEVEL_SHIFT_T186 0x0
> +#define PMC_RST_LEVEL_MASK_T186 0x3
> +#define PMC_RST_SOURCE_MASK_T186 0x3C
> +#define PMC_RST_SOURCE_SHIFT_T186 0x2
> +#define PMC_RST_LEVEL_MASK_T30 0x7
Also please fully parameterize these. We already have a couple of
registers that differ between SoC generations and we keep those in a
struct tegra_pmc_regs. I would suggest something along the lines of:
struct tegra_pmc_regs {
...
unsigned int rst_status;
unsigned int rst_source_shift;
unsigned int rst_source_mask;
unsigned int rst_level_shift;
unsigned int rst_level_mask;
};
Then all the code accessing these fields can be unconditionally written
based on these parameters and the differentiation happens in these
structures.
> +
> +
> /* Tegra186 and later */
> #define WAKE_AOWAKE_CTRL 0x4f4
> #define WAKE_AOWAKE_CTRL_INTR_POLARITY BIT(0)
> @@ -153,6 +162,12 @@ struct tegra_pmc_regs {
> unsigned int dpd2_status;
> };
>
> +enum tegra_pmc_reset_status_type {
> + PMC_RESET_STATUS_TYPE_NONE,
> + PMC_RESET_STATUS_TYPE_T30,
> + PMC_RESET_STATUS_TYPE_T186
> +};
This can then go away as well.
> +
> struct tegra_pmc_soc {
> unsigned int num_powergates;
> const char *const *powergates;
> @@ -163,6 +178,7 @@ struct tegra_pmc_soc {
> bool has_gpu_clamps;
> bool needs_mbist_war;
> bool has_impl_33v_pwr;
> + enum tegra_pmc_reset_status_type pmc_reset_status;
And this, too.
>
> const struct tegra_io_pad_soc *io_pads;
> unsigned int num_io_pads;
> @@ -662,6 +678,71 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
> }
> #endif /* CONFIG_SMP */
>
> +/**
> + * tegra_pmc_get_system_reset_reason() - last reset reason status
> + */
> +static char *tegra_pmc_get_system_reset_reason(void)
This should return const char * and also take a struct tegra_pmc * as
argument for consistency.
> +{
> + u32 val, rst_src;
All other code in this file uses "u32 value", so please stick with that.
You should also be able to get away without the rst_src by just using
val & PMC_RST_LEVEL_MASK_* in the switch statements. Or better yet...
> +
> + if (pmc->soc->pmc_reset_status == PMC_RESET_STATUS_TYPE_T30) {
> + val = tegra_pmc_readl(PMC_RST_STATUS_T30);
> + rst_src = val & PMC_RST_LEVEL_MASK_T30;
Was this supposed to be PMC_RST_SOURCE_MASK_T30? Looks like it isn't,
but then it is certainly badly named. This should go away if you switch
to parameterized registers.
> + switch (rst_src) {
> + case 0: return "TEGRA_POWER_ON_RESET";
> + case 1: return "TEGRA_WATCHDOG";
> + case 2: return "TEGRA_SENSOR";
> + case 3: return "TEGRA_SOFTWARE_RESET";
> + case 4: return "TEGRA_LP0";
> + case 5: return "TEGRA_AOTAG";
> + default: return "TEGRA_RESET_REASON_UNSPECIFIED";
> + }
> + } else if (pmc->soc->pmc_reset_status == PMC_RESET_STATUS_TYPE_T186) {
> + val = tegra_pmc_readl(PMC_RST_STATUS_T186);
> + rst_src = (val & PMC_RST_SOURCE_MASK_T186) >>
> + PMC_RST_SOURCE_SHIFT_T186;
> + switch (rst_src) {
> + case 0: return "TEGRA_POWER_ON_RESET";
> + case 1: return "TEGRA_AO_WATCHDOG";
> + case 2: return "TEGRA_DENVER_WATCHDOG";
> + case 3: return "TEGRA_BPMP_WATCHDOG";
> + case 4: return "TEGRA_SCE_WATCHDOG";
> + case 5: return "TEGRA_SPE_WATCHDOG";
> + case 6: return "TEGRA_APE_WATCHDOG";
> + case 7: return "TEGRA_A57_WATCHDOG";
> + case 8: return "TEGRA_SENSOR";
> + case 9: return "TEGRA_AOTAG";
> + case 10: return "TEGRA_VFSENSOR";
> + case 11: return "TEGRA_SOFTWARE_RESET";
> + case 12: return "TEGRA_SC7";
> + case 13: return "TEGRA_HSM";
> + case 14: return "TEGRA_CSITE";
> + default: return "TEGRA_RESET_REASON_UNSPECIFIED";
> + }
> + } else {
> + return "TEGRA_RESET_REASON_NOT_SUPPORTED";
> + }
> +}
I think we should either return an unsigned integer as the reason and
put that into sysfs. If we really need to have a string, the table of
strings should be part of struct tegra_pmc_soc as well, something along
the lines of:
struct tegra_pmc_soc {
...
const char * const *reset_sources;
unsigned int num_reset_sources;
};
Then the above function can simply read the value using the
parameterized registers and use the value to index into the table. While
at it, we can check that the value is within the allowed range for the
specific SoC.
Also, if the SoC doesn't have a way of reporting the reset reason, I
don't think we should expose the corresponding sysfs file in the first
place.
> +
> +static char *tegra_pmc_get_system_reset_level(void)
> +{
> + u32 val, rst_lvl;
> +
> + if (pmc->soc->pmc_reset_status == PMC_RESET_STATUS_TYPE_T186) {
> + val = tegra_pmc_readl(PMC_RST_STATUS_T186);
> + rst_lvl = (val & PMC_RST_LEVEL_MASK_T186) >>
> + PMC_RST_LEVEL_SHIFT_T186;
> + switch (rst_lvl) {
> + case 0: return "TEGRA_RESET_LEVEL_L0";
> + case 1: return "TEGRA_RESET_LEVEL_L1";
> + case 2: return "TEGRA_RESET_LEVEL_L2";
> + default: return "TEGRA_RESET_LEVEL_UNSPECIFIED";
> + }
> + } else {
> + return "TEGRA_RESET_LEVEL_NOT_SUPPORTED";
> + }
> +}
Mostly the same comments as for tegra_pmc_get_system_reset_reason().
Oh, and for the strings, let's go with something less redundant. For
example when we read these files we already know that we're running on
Tegra, so TEGRA_ is completely redundant. For the level, we also know
that it is a level, so RESET_LEVEL_ is redundant as well. Now it's also
custom for strings in sysfs to be lowercase, so I suggest the following
for reset levels:
static const char * const tegra186_reset_levels[] = {
"l0", "l1", "l2", "unspecified"
};
According to the hardware manuals, "unspecified" is actually "warm". Any
reason for the difference here?
For the reset sources I'd also drop the TEGRA_ prefix and maybe stick
with a lowercase variant of what's listed in the TRM:
static const char * const tegra186_reset_sources[] = {
"sys-reset-n",
"aowdt",
"bccplexwdt",
...
};
> +
> static int tegra_pmc_restart_notify(struct notifier_block *this,
> unsigned long action, void *data)
> {
> @@ -1543,6 +1624,46 @@ static int tegra_pmc_pinctrl_init(struct tegra_pmc *pmc)
> return err;
> }
>
> +static void tegra_pmc_show_reset_status(struct device *dev)
> +{
> + dev_info(dev, "reset source: %s\n",
> + tegra_pmc_get_system_reset_reason());
> + dev_info(dev, "reset level: %s\n",
> + tegra_pmc_get_system_reset_level());
> +}
Is this really useful? This is something that can be found out via
sysfs, so why also have it spam the boot log?
> +
> +static ssize_t tegra_reset_reason_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", tegra_pmc_get_system_reset_reason());
> +}
> +
> +static DEVICE_ATTR_RO(tegra_reset_reason);
> +
> +static ssize_t tegra_reset_level_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", tegra_pmc_get_system_reset_level());
> +}
> +
> +static DEVICE_ATTR_RO(tegra_reset_level);
I think these attributes should also omit the tegra_ prefix because that
is implied already by the system that we're running on.
> +
> +static void tegra_pmc_reset_sysfs_init(struct device *dev)
> +{
> + int error;
We use "int err" elsewhere, please stick to that.
> +
> + error = device_create_file(dev, &dev_attr_tegra_reset_reason);
> + if (error) {
> + dev_err(dev,
> + "Failed to create sysfs node - tegra_reset_reason\n");
> + }
If these aren't fatal errors, maybe turn them into dev_warn()?
> + error = device_create_file(dev, &dev_attr_tegra_reset_level);
> + if (error) {
> + dev_err(dev,
> + "Failed to create sysfs node - tegra_reset_level\n");
> + }
> +}
As mentioned earlier, I think these should be added conditionally based
on whether or not they exist on a given SoC. For example if the reset
source is not reported on an SoC, just leave the corresponding register
fields empty (= 0) in struct tegra_pmc_soc and then you can check in
this function whether to export the attribute or not. It's probably best
to make the decision based on the mask, because the offset and shift can
be zero, whereas a mask of zero doesn't make sense.
> +
> static int tegra_pmc_probe(struct platform_device *pdev)
> {
> void __iomem *base;
> @@ -1612,6 +1733,9 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>
> tegra_pmc_init_tsense_reset(pmc);
>
> + tegra_pmc_show_reset_status(&pdev->dev);
> + tegra_pmc_reset_sysfs_init(&pdev->dev);
> +
> if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> err = tegra_powergate_debugfs_init();
> if (err < 0)
> @@ -1728,6 +1852,7 @@ static const struct tegra_pmc_soc tegra20_pmc_soc = {
> .cpu_powergates = NULL,
> .has_tsense_reset = false,
> .has_gpu_clamps = false,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_NONE,
> .num_io_pads = 0,
> .io_pads = NULL,
> .num_pin_descs = 0,
For Tegra20, you'd have to leave all the values for the new registers at
0.
> @@ -1769,6 +1894,7 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = {
> .has_tsense_reset = true,
> .has_gpu_clamps = false,
> .has_impl_33v_pwr = false,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_T30,
> .num_io_pads = 0,
> .io_pads = NULL,
> .num_pin_descs = 0,
For Tegra30, make a copy of the Tegra20 registers and set the proper
values for the new fields. This structure can now be shared on Tegra114,
Tegra124 and Tegra210.
> @@ -1814,6 +1940,7 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = {
> .has_tsense_reset = true,
> .has_gpu_clamps = false,
> .has_impl_33v_pwr = false,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_T30,
> .num_io_pads = 0,
> .io_pads = NULL,
> .num_pin_descs = 0,
> @@ -1919,6 +2046,7 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = {
> .has_tsense_reset = true,
> .has_gpu_clamps = true,
> .has_impl_33v_pwr = false,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_T30,
> .num_io_pads = ARRAY_SIZE(tegra124_io_pads),
> .io_pads = tegra124_io_pads,
> .num_pin_descs = ARRAY_SIZE(tegra124_pin_descs),
> @@ -2019,6 +2147,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
> .has_tsense_reset = true,
> .has_gpu_clamps = true,
> .has_impl_33v_pwr = false,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_T30,
> .needs_mbist_war = true,
> .num_io_pads = ARRAY_SIZE(tegra210_io_pads),
> .io_pads = tegra210_io_pads,
> @@ -2129,6 +2258,7 @@ static const struct tegra_pmc_soc tegra186_pmc_soc = {
> .has_tsense_reset = false,
> .has_gpu_clamps = false,
> .has_impl_33v_pwr = true,
> + .pmc_reset_status = PMC_RESET_STATUS_TYPE_T186,
> .num_io_pads = ARRAY_SIZE(tegra186_io_pads),
> .io_pads = tegra186_io_pads,
> .num_pin_descs = ARRAY_SIZE(tegra186_pin_descs),
For this you can just extend the existing tegra186_pmc_regs with the new
values.
Thierry
Attachment:
signature.asc
Description: PGP signature
