Hi Thierry,
Thanks for reviewing the changes.
Please find the replies inline.
All replies can be found with prefix - [Reply]
Please help reviewing the next version patch: Patch-V2 (which addresses all review comments).
Thanks & Regards,
Sandipan
-----Original Message-----
From: Thierry Reding <thierry.reding@xxxxxxxxx>
Sent: Tuesday, September 25, 2018 5:45 PM
To: Sandipan Patra <spatra@xxxxxxxxxx>
Cc: Thierry Reding <treding@xxxxxxxxxx>; Jonathan Hunter <jonathanh@xxxxxxxxxx>; Mikko Perttunen <mperttunen@xxxxxxxxxx>; linux-tegra@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] soc/tegra: pmc: Add sysfs entry to get reset info
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?
[Reply] Presently there is no support to get the information at user space. User may need it to know the reason then we are providing it through sysfs.
> 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?
[Reply] Checked with Tegra20 trm, could not find any information on pmc reset information. Considering not to add support for Tegra20 pmc reset status. In the new parameterized structure, all corresponding values to be filled with NULL.
> +#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.
[Reply] Done.
> +
> +
> /* 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.
[Reply] Done
> +
> 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.
[Reply] Done
>
> 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.
[Reply] Done
> +{
> + 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...
[Reply] Done. Changed to value instead of using val.
> +
> + 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.
[Reply] Done. I have modified it as below.
Tegra20 : pmc reset status not used
Tegra30 till Tegra210 : named as legacy version
Tegra186 and + : named as Tegra186
> + 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.
[Reply] Done. Taken care to not to populate the sysfs node if SoC does not support.
> +
> +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",
...
};
[Reply] Done. Used caps format in string values to highlight the information while reading nodes.
> +
> 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?
[Reply] Done. Removed.
> +
> +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.
[Reple] Agree. Removed tegra_ prefix.
> +
> +static void tegra_pmc_reset_sysfs_init(struct device *dev) {
> + int error;
We use "int err" elsewhere, please stick to that.
[Reply] Done. Int err is no longer used.
> +
> + 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()?
[Reply] Agree. Used 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.
[Reply] Done. Taken care.
> +
> 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
