diff mbox series

[v3] pinctrl: tegra: Add support to display pin function

Message ID 20230714113547.15384-1-pshete@nvidia.com
State Accepted
Headers show
Series [v3] pinctrl: tegra: Add support to display pin function | expand

Commit Message

Prathamesh Shete July 14, 2023, 11:35 a.m. UTC 
The current function for a given pin is not displayed via the debugfs.
Add support to display the current function that is set for each pin.

Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/pinctrl/tegra/pinctrl-tegra.c | 19 +++++++++++++++++--
 drivers/pinctrl/tegra/pinctrl-tegra.h |  2 ++
 2 files changed, 19 insertions(+), 2 deletions(-)

Comments

Linus Walleij July 20, 2023, 7:35 p.m. UTC | #1 
On Fri, Jul 14, 2023 at 1:36 PM Prathamesh Shete <pshete@nvidia.com> wrote:

> The current function for a given pin is not displayed via the debugfs.
> Add support to display the current function that is set for each pin.
>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Patch applied.

Yours,
Linus Walleij
Luca Ceresoli Sept. 25, 2023, 4:30 p.m. UTC | #2 
Hello Prathamesh Shete,

On Fri, 14 Jul 2023 17:05:47 +0530
Prathamesh Shete <pshete@nvidia.com> wrote:

> The current function for a given pin is not displayed via the debugfs.
> Add support to display the current function that is set for each pin.
> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

While testing a Tegra20-based custom board I found a regression which
according to my bisecting appears starting with this patch (commit
d1cd5b51bc91 upstream).

The symptom is that i2c3 is not working anymore, the I2C lines being
always high. No other known issues at the moment.

The board is built around an Avionic Design Tamonten SOM:
arch/arm/boot/dts/nvidia/tegra20-tamonten.dtsi, which has in the
&state_default node:

    dtf {
        nvidia,pins = "dtf";
        nvidia,function = "i2c3";
    };

But on top of that the board dts has:
 
&state_default {
       dtf {
               nvidia,pins = "dtf";
               nvidia,function = "i2c3";
               nvidia,pull = <TEGRA_PIN_PULL_NONE>;
               nvidia,tristate = <TEGRA_PIN_DISABLE>;
       };
};

> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 4547cf66d03b..cb1d67239cd0 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -96,6 +96,7 @@ static const struct cfg_param {
>  	{"nvidia,slew-rate-falling",	TEGRA_PINCONF_PARAM_SLEW_RATE_FALLING},
>  	{"nvidia,slew-rate-rising",	TEGRA_PINCONF_PARAM_SLEW_RATE_RISING},
>  	{"nvidia,drive-type",		TEGRA_PINCONF_PARAM_DRIVE_TYPE},
> +	{"nvidia,function",		TEGRA_PINCONF_PARAM_FUNCTION},

FYI, I reduced your patch to only this line plus the one in the
pinctrl-tegra.h and the problem appears as well.

This is all the info I have at the moment. Can you provide more info on
what could be going on or how to investigate?

I am available to share more info as needed.

Best regards,
Luca
Linus Walleij Sept. 27, 2023, 8:54 a.m. UTC | #3 
On Mon, Sep 25, 2023 at 6:30 PM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> The symptom is that i2c3 is not working anymore, the I2C lines being
> always high. No other known issues at the moment.

Hm....

>> +     {"nvidia,function",             TEGRA_PINCONF_PARAM_FUNCTION},
>
> FYI, I reduced your patch to only this line plus the one in the
> pinctrl-tegra.h and the problem appears as well.

I think there is a conflict now, that the pinconf is "stealing" the function
assignment from the pinmux call.

It's just a debugprint, I will revert the patch, Luca can investigate and you
can test a new patch then we will merge that.

Yours,
Linus Walleij
Luca Ceresoli Sept. 28, 2023, 6:53 a.m. UTC | #4 
Hello Linus, Prathamesh,

On Wed, 27 Sep 2023 10:54:15 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Sep 25, 2023 at 6:30 PM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > The symptom is that i2c3 is not working anymore, the I2C lines being
> > always high. No other known issues at the moment.  
> 
> Hm....
> 
> >> +     {"nvidia,function",             TEGRA_PINCONF_PARAM_FUNCTION},  
> >
> > FYI, I reduced your patch to only this line plus the one in the
> > pinctrl-tegra.h and the problem appears as well.  
> 
> I think there is a conflict now, that the pinconf is "stealing" the function
> assignment from the pinmux call.
> 
> It's just a debugprint, I will revert the patch, Luca can investigate and you

Thanks for the quick revert Linus.

> can test a new patch then we will merge that.

Prathamesh, if you send a new patch it would be great if you can Cc: me so I
can test it.

Best regards,
Luca
Thierry Reding Sept. 29, 2023, 12:26 p.m. UTC | #5 
On Thu, Sep 28, 2023 at 08:53:19AM +0200, Luca Ceresoli wrote:
> Hello Linus, Prathamesh,
> 
> On Wed, 27 Sep 2023 10:54:15 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > On Mon, Sep 25, 2023 at 6:30 PM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > 
> > > The symptom is that i2c3 is not working anymore, the I2C lines being
> > > always high. No other known issues at the moment.  
> > 
> > Hm....
> > 
> > >> +     {"nvidia,function",             TEGRA_PINCONF_PARAM_FUNCTION},  
> > >
> > > FYI, I reduced your patch to only this line plus the one in the
> > > pinctrl-tegra.h and the problem appears as well.  
> > 
> > I think there is a conflict now, that the pinconf is "stealing" the function
> > assignment from the pinmux call.
> > 
> > It's just a debugprint, I will revert the patch, Luca can investigate and you
> 
> Thanks for the quick revert Linus.
> 
> > can test a new patch then we will merge that.
> 
> Prathamesh, if you send a new patch it would be great if you can Cc: me so I
> can test it.

I was able to reproduce this on tegra20-trimslice, and it looks indeed
that the "shortcut" of supporting this through the pinconf "framework"
doesn't work. In addition to the pinmux now no longer getting applied
(exactly why that is I don't think I understand), it also leads to weird
things in other parts of the debugfs output. For example the code now
ends up trying to read a u32 from the nvidia,function property, which is
actually a string. That seems to be fine, but obviously it doesn't yield
the value that is expected for a function and messes up the config param
which will then later on fail to properly display.

Anyway, turns out the correct implementation is even shorter. I've sent
out a patch.

Thierry
diff mbox series

Patch

diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 4547cf66d03b..cb1d67239cd0 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -96,6 +96,7 @@  static const struct cfg_param {
 	{"nvidia,slew-rate-falling",	TEGRA_PINCONF_PARAM_SLEW_RATE_FALLING},
 	{"nvidia,slew-rate-rising",	TEGRA_PINCONF_PARAM_SLEW_RATE_RISING},
 	{"nvidia,drive-type",		TEGRA_PINCONF_PARAM_DRIVE_TYPE},
+	{"nvidia,function",		TEGRA_PINCONF_PARAM_FUNCTION},
 };
 
 static int tegra_pinctrl_dt_subnode_to_map(struct pinctrl_dev *pctldev,
@@ -470,6 +471,12 @@  static int tegra_pinconf_reg(struct tegra_pmx *pmx,
 		*bit = g->drvtype_bit;
 		*width = 2;
 		break;
+	case TEGRA_PINCONF_PARAM_FUNCTION:
+		*bank = g->mux_bank;
+		*reg = g->mux_reg;
+		*bit = g->mux_bit;
+		*width = 2;
+		break;
 	default:
 		dev_err(pmx->dev, "Invalid config param %04x\n", param);
 		return -ENOTSUPP;
@@ -633,8 +640,16 @@  static void tegra_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 		val >>= bit;
 		val &= (1 << width) - 1;
 
-		seq_printf(s, "\n\t%s=%u",
-			   strip_prefix(cfg_params[i].property), val);
+		if (cfg_params[i].param == TEGRA_PINCONF_PARAM_FUNCTION) {
+			u8 idx = pmx->soc->groups[group].funcs[val];
+
+			seq_printf(s, "\n\t%s=%s",
+				   strip_prefix(cfg_params[i].property),
+					 pmx->functions[idx].name);
+		} else {
+			seq_printf(s, "\n\t%s=%u",
+				   strip_prefix(cfg_params[i].property), val);
+		}
 	}
 }
 
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
index b3289bdf727d..e728efeaa4de 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.h
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
@@ -54,6 +54,8 @@  enum tegra_pinconf_param {
 	TEGRA_PINCONF_PARAM_SLEW_RATE_RISING,
 	/* argument: Integer, range is HW-dependant */
 	TEGRA_PINCONF_PARAM_DRIVE_TYPE,
+	/* argument: pinmux settings */
+	TEGRA_PINCONF_PARAM_FUNCTION,
 };
 
 enum tegra_pinconf_pull {