On Wed, Oct 03, 2018 at 03:54:50PM +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.
>
> These sysfs nodes provide reset reason and reset level information
> on production software.
>
> Signed-off-by: Sandipan Patra <spatra@xxxxxxxxxx>
> ---
> Changes since V1:
> 1. Fully parameterized the registers for SoC generations
> to take the path unconditionally.
> 2. Changed the string to const char *
> 3. Instead of using val, changed them to value to comply with format.
> 4. Tegra20 : pmc reset status not used
> Tegra30 till Tegra210 : named as legacy version
> Tegra186 and + : named as Tegra186
> 5. No sysfs node to be exposed, when soc does not support.
> 6. Changed the log level to dev_warn as they are not fatal errors.
>
> drivers/soc/tegra/pmc.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 138 insertions(+), 1 deletion(-)
Hi Sandipan,
this looks almost ready now. Just a couple of cosmetic comments below.
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index ab719fa..18ee90b 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,6 @@
> #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_POR 0
> #define PMC_RST_STATUS_WATCHDOG 1
> #define PMC_RST_STATUS_SENSOR 2
> @@ -151,6 +151,11 @@ struct tegra_pmc_regs {
> unsigned int dpd_status;
> unsigned int dpd2_req;
> unsigned int dpd2_status;
> + unsigned int rst_status;
> + unsigned int rst_source_shift;
> + unsigned int rst_source_mask;
> + unsigned int rst_level_shift;
> + unsigned int rst_level_mask;
> };
>
> struct tegra_pmc_soc {
> @@ -175,6 +180,42 @@ struct tegra_pmc_soc {
> void (*setup_irq_polarity)(struct tegra_pmc *pmc,
> struct device_node *np,
> bool invert);
> +
> + const char *const *reset_sources;
> + unsigned int num_reset_sources;
> + const char *const *reset_levels;
> + unsigned int num_reset_levels;
While both variants are used in the kernel, the most common and
therefore idiomatic version is to put a space between '*' and
"const".
> +};
> +
> +static const char *const tegra186_reset_sources[] = {
Same here.
> + "SYS_RESET",
> + "AOWDT",
> + "MCCPLEXWDT",
> + "BPMPWDT",
> + "SCEWDT",
> + "SPEWDT",
> + "APEWDT",
> + "BCCPLEXWDT",
> + "SENSOR",
> + "AOTAG",
> + "VFSENSOR",
> + "SWREST",
> + "SC7",
> + "HSM",
> + "CORESIGHT"
> +};
> +
> +static const char *const tegra186_reset_levels[] = {
And here.
> + "L0", "L1", "L2", "WARM"
> +};
> +
> +static const char *const tegra_legacy_reset_sources[] = {
And here.
Also, in these kinds of situations we usually use a prefix that
identifies the oldest generation that supports a given interface or
certain properties. In this particular case that would make this:
tegra30_reset_sources.
That's more explicit than "legacy".
> + "POWER_ON_RESET",
> + "WATCHDOG",
> + "SENSOR",
> + "SW_MAIN",
> + "LP0",
> + "AOTAG"
> };
>
> /**
> @@ -662,6 +703,31 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
> }
> #endif /* CONFIG_SMP */
>
> +/**
> + * tegra_pmc_get_system_reset_reason() - last reset reason status
> + */
> +static const char *tegra_pmc_get_system_reset_reason(void)
> +{
> + u32 value, rst_src;
> +
> + value = tegra_pmc_readl(pmc->soc->regs->rst_status);
> + rst_src = (value & pmc->soc->regs->rst_source_mask) >>
> + pmc->soc->regs->rst_source_shift;
> +
> + return pmc->soc->reset_sources[rst_src];
> +}
> +
> +static const char *tegra_pmc_get_system_reset_level(void)
> +{
> + u32 value, rst_lvl;
> +
> + value = tegra_pmc_readl(pmc->soc->regs->rst_status);
> + rst_lvl = (value & pmc->soc->regs->rst_level_mask) >>
> + pmc->soc->regs->rst_level_shift;
> +
> + return pmc->soc->reset_levels[rst_lvl];
> +}
These functions serve no other purpose than to be called by the sysfs
attributes, right? Couldn't we move the implementation into the
attributes to avoid these extra functions?
> +
> static int tegra_pmc_restart_notify(struct notifier_block *this,
> unsigned long action, void *data)
> {
> @@ -1543,6 +1609,40 @@ static int tegra_pmc_pinctrl_init(struct tegra_pmc *pmc)
> return err;
> }
>
> +static ssize_t 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(reset_reason);
> +
> +static ssize_t reset_level_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s\n", tegra_pmc_get_system_reset_level());
> +}
If you move the body of tegra_pmc_get_system_reset_level() into this
function, you can get at pmc via dev_get_drvdata(dev). That way you
don't rely on the global "pmc" variable.
> +static DEVICE_ATTR_RO(reset_level);
> +
> +static void tegra_pmc_reset_sysfs_init(struct device *dev)
I think it'd be better if this took a struct tegra_pmc * as a parameter.
You can still get at the struct device * from that, and you don't rely
on the global "pmc" variable.
> +{
> +
Gratuitous blank line.
> + if (pmc->soc->reset_sources) {
> + if (device_create_file(dev, &dev_attr_reset_reason)) {
> + dev_warn(dev,
> + "Failed to create sysfs node - tegra_reset_reason\n");
> + }
> + }
> +
> + if (pmc->soc->reset_levels) {
> + if (device_create_file(dev, &dev_attr_reset_level)) {
> + dev_warn(dev,
> + "Failed to create sysfs node - tegra_reset_level\n");
> + }
> + }
Do we want these failures to be ignored, or do we want to propagate the
error code and fail probe if these attribute file can't be created?
In either case I think the error or warning messages should contain the
error code. Also, other error and warning messages in this file start
with a lower case letter, so please stick with that for consistency. The
attributes are now also called "reset_reason" and "reset_level", so the
messages should reflect that.
Taking into account all of the above, perhaps make this something like:
err = device_create_file(...);
if (err < 0)
dev_warn(dev, "failed to create \"reset_reason\" attribute: %d\n", err);
> +}
> +
> static int tegra_pmc_probe(struct platform_device *pdev)
> {
> void __iomem *base;
> @@ -1612,6 +1712,8 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>
> tegra_pmc_init_tsense_reset(pmc);
>
> + tegra_pmc_reset_sysfs_init(&pdev->dev);
> +
> if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> err = tegra_powergate_debugfs_init();
> if (err < 0)
> @@ -1678,6 +1780,11 @@ static const struct tegra_pmc_regs tegra20_pmc_regs = {
> .dpd_status = 0x1bc,
> .dpd2_req = 0x1c0,
> .dpd2_status = 0x1c4,
> + .rst_status = 0x1b4,
> + .rst_source_shift = 0x0,
> + .rst_source_mask = 0x7,
> + .rst_level_shift = 0x0,
> + .rst_level_mask = 0x0,
> };
>
> static void tegra20_pmc_init(struct tegra_pmc *pmc)
> @@ -1735,6 +1842,10 @@ static const struct tegra_pmc_soc tegra20_pmc_soc = {
> .regs = &tegra20_pmc_regs,
> .init = tegra20_pmc_init,
> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> + .reset_sources = NULL,
> + .num_reset_sources = 0,
> + .reset_levels = NULL,
> + .num_reset_levels = 0,
> };
>
> static const char * const tegra30_powergates[] = {
> @@ -1776,6 +1887,10 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = {
> .regs = &tegra20_pmc_regs,
> .init = tegra20_pmc_init,
> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> + .reset_sources = tegra_legacy_reset_sources,
> + .num_reset_sources = 5,
> + .reset_levels = NULL,
> + .num_reset_levels = 0,
> };
>
> static const char * const tegra114_powergates[] = {
> @@ -1821,6 +1936,10 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = {
> .regs = &tegra20_pmc_regs,
> .init = tegra20_pmc_init,
> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> + .reset_sources = tegra_legacy_reset_sources,
> + .num_reset_sources = 5,
> + .reset_levels = NULL,
> + .num_reset_levels = 0,
> };
>
> static const char * const tegra124_powergates[] = {
> @@ -1926,6 +2045,10 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = {
> .regs = &tegra20_pmc_regs,
> .init = tegra20_pmc_init,
> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> + .reset_sources = tegra_legacy_reset_sources,
> + .num_reset_sources = 5,
> + .reset_levels = NULL,
> + .num_reset_levels = 0,
> };
>
> static const char * const tegra210_powergates[] = {
> @@ -2027,6 +2150,10 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
> .regs = &tegra20_pmc_regs,
> .init = tegra20_pmc_init,
> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> + .reset_sources = tegra_legacy_reset_sources,
> + .num_reset_sources = 5,
> + .reset_levels = NULL,
> + .num_reset_levels = 0,
> };
>
> #define TEGRA186_IO_PAD_TABLE(_pad) \
> @@ -2084,6 +2211,12 @@ static const struct tegra_pmc_regs tegra186_pmc_regs = {
> .dpd_status = 0x78,
> .dpd2_req = 0x7c,
> .dpd2_status = 0x80,
> + .rst_status = 0x70,
> + .rst_source_shift = 0x2,
> + .rst_source_mask = 0x3C,
> + .rst_level_shift = 0x0,
> + .rst_level_mask = 0x3,
> +
Gratuitous blank line.
One last thing: can the above Tegra186 code be used as-is for Tegra194
support as well? In most other places the PMC driver upstream uses the
Tegra186 parameters on Tegra194, so can we continue to do that or will
Tegra194 require different parameters for these reset bits?
Thierry
Attachment:
signature.asc
Description: PGP signature
