[PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Rajendra Nayak]

From: Prasad Sodagudi <psodagud@codeaurora.org>

egpio is a scheme which allows special power Island Domain IOs
(LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
TLMM functions with Island Domain functions.
With this scheme, an IO can be controlled both by the cpu running
linux and the Island processor. This provides great flexibility to
re-purpose the Island IOs for regular TLMM usecases.

2 new bits are added to ctl_reg, egpio_present is a read only bit
which shows if egpio feature is available or not on a given gpio.
egpio_enable is the read/write bit and only effective if egpio_present
is 1. Once its set, the Island IO is controlled from Chip TLMM.
egpio_enable when set to 0 means the GPIO is used as Island Domain IO.

To support this we add a new function 'egpio' which can be used to
set the egpio_enable to 0, for any other TLMM controlled functions
we set the egpio_enable to 1.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 8476a8a..bfdba3a 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	unsigned int irq = irq_find_mapping(gc->irq.domain, group);
 	struct irq_data *d = irq_get_irq_data(irq);
 	unsigned int gpio_func = pctrl->soc->gpio_func;
+	unsigned int egpio_func = pctrl->soc->egpio_func;
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	u32 val, mask;
@@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_ctl(pctrl, g);
-	val &= ~mask;
-	val |= i << g->mux_bit;
+
+	if (egpio_func && i == egpio_func) {
+		if (val & BIT(g->egpio_present))
+			val &= ~BIT(g->egpio_enable);
+		else
+			return -EINVAL;
+	} else {
+		val &= ~mask;
+		val |= i << g->mux_bit;
+		/* Check if egpio present and enable that feature */
+		if (egpio_func && (val & BIT(g->egpio_present)))
+			val |= BIT(g->egpio_enable);
+	}
+
 	msm_writel_ctl(val, pctrl, g);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index e31a516..b7110ac 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -77,6 +77,8 @@ struct msm_pingroup {
 	unsigned drv_bit:5;
 
 	unsigned od_bit:5;
+	unsigned egpio_enable:5;
+	unsigned egpio_present:5;
 	unsigned oe_bit:5;
 	unsigned in_bit:5;
 	unsigned out_bit:5;
@@ -119,6 +121,7 @@ struct msm_gpio_wakeirq_map {
  *                            to be aware that their parent can't handle dual
  *                            edge interrupts.
  * @gpio_func: Which function number is GPIO (usually 0).
+ * @egpio_func: Which function number is eGPIO
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -136,6 +139,7 @@ struct msm_pinctrl_soc_data {
 	unsigned int nwakeirq_map;
 	bool wakeirq_dual_edge_errata;
 	unsigned int gpio_func;
+	unsigned int egpio_func;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/2] pinctrl: qcom: sc7280: Add egpio support

[Rajendra Nayak]

sc7280 supports the egpio feature, GPIOs ranging from 144 to 174
(31 GPIOs) support it, we define gpio_func to 9, which
is an unused function for all these pins on sc7280.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-sc7280.c | 75 ++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c
index 9017ede..31df55c 100644
--- a/drivers/pinctrl/qcom/pinctrl-sc7280.c
+++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c
@@ -43,6 +43,8 @@
 		.mux_bit = 2,			\
 		.pull_bit = 0,			\
 		.drv_bit = 6,			\
+		.egpio_enable = 12,		\
+		.egpio_present = 11,		\
 		.oe_bit = 9,			\
 		.in_bit = 0,			\
 		.out_bit = 1,			\
@@ -520,6 +522,7 @@ enum sc7280_functions {
 	msm_mux_dp_lcd,
 	msm_mux_edp_hot,
 	msm_mux_edp_lcd,
+	msm_mux_egpio,
 	msm_mux_gcc_gp1,
 	msm_mux_gcc_gp2,
 	msm_mux_gcc_gp3,
@@ -658,6 +661,14 @@ static const char * const gpio_groups[] = {
 	"gpio165", "gpio166", "gpio167", "gpio168", "gpio169", "gpio170",
 	"gpio171", "gpio172", "gpio173", "gpio174",
 };
+static const char * const egpio_groups[] = {
+	"gpio144", "gpio145", "gpio146", "gpio147", "gpio148", "gpio149",
+	"gpio150", "gpio151", "gpio152", "gpio153", "gpio154", "gpio155",
+	"gpio156", "gpio157", "gpio158", "gpio159", "gpio160", "gpio161",
+	"gpio162", "gpio163", "gpio164", "gpio165", "gpio166", "gpio167",
+	"gpio168", "gpio169", "gpio170", "gpio171", "gpio172", "gpio173",
+	"gpio174",
+};
 static const char * const atest_char_groups[] = {
 	"gpio81",
 };
@@ -1150,6 +1161,7 @@ static const struct msm_function sc7280_functions[] = {
 	FUNCTION(dp_lcd),
 	FUNCTION(edp_hot),
 	FUNCTION(edp_lcd),
+	FUNCTION(egpio),
 	FUNCTION(gcc_gp1),
 	FUNCTION(gcc_gp2),
 	FUNCTION(gcc_gp3),
@@ -1408,37 +1420,37 @@ static const struct msm_pingroup sc7280_groups[] = {
 	[141] = PINGROUP(141, _, _, _, _, _, _, _, _, _),
 	[142] = PINGROUP(142, _, _, _, _, _, _, _, _, _),
 	[143] = PINGROUP(143, _, _, _, _, _, _, _, _, _),
-	[144] = PINGROUP(144, _, _, _, _, _, _, _, _, _),
-	[145] = PINGROUP(145, _, _, _, _, _, _, _, _, _),
-	[146] = PINGROUP(146, _, _, _, _, _, _, _, _, _),
-	[147] = PINGROUP(147, _, _, _, _, _, _, _, _, _),
-	[148] = PINGROUP(148, _, _, _, _, _, _, _, _, _),
-	[149] = PINGROUP(149, _, _, _, _, _, _, _, _, _),
-	[150] = PINGROUP(150, qdss, _, _, _, _, _, _, _, _),
-	[151] = PINGROUP(151, qdss, _, _, _, _, _, _, _, _),
-	[152] = PINGROUP(152, qdss, _, _, _, _, _, _, _, _),
-	[153] = PINGROUP(153, qdss, _, _, _, _, _, _, _, _),
-	[154] = PINGROUP(154, _, _, _, _, _, _, _, _, _),
-	[155] = PINGROUP(155, _, _, _, _, _, _, _, _, _),
-	[156] = PINGROUP(156, qdss_cti, _, _, _, _, _, _, _, _),
-	[157] = PINGROUP(157, qdss_cti, _, _, _, _, _, _, _, _),
-	[158] = PINGROUP(158, _, _, _, _, _, _, _, _, _),
-	[159] = PINGROUP(159, _, _, _, _, _, _, _, _, _),
-	[160] = PINGROUP(160, _, _, _, _, _, _, _, _, _),
-	[161] = PINGROUP(161, _, _, _, _, _, _, _, _, _),
-	[162] = PINGROUP(162, _, _, _, _, _, _, _, _, _),
-	[163] = PINGROUP(163, _, _, _, _, _, _, _, _, _),
-	[164] = PINGROUP(164, _, _, _, _, _, _, _, _, _),
-	[165] = PINGROUP(165, qdss_cti, _, _, _, _, _, _, _, _),
-	[166] = PINGROUP(166, qdss_cti, _, _, _, _, _, _, _, _),
-	[167] = PINGROUP(167, _, _, _, _, _, _, _, _, _),
-	[168] = PINGROUP(168, _, _, _, _, _, _, _, _, _),
-	[169] = PINGROUP(169, _, _, _, _, _, _, _, _, _),
-	[170] = PINGROUP(170, _, _, _, _, _, _, _, _, _),
-	[171] = PINGROUP(171, qdss, _, _, _, _, _, _, _, _),
-	[172] = PINGROUP(172, qdss, _, _, _, _, _, _, _, _),
-	[173] = PINGROUP(173, qdss, _, _, _, _, _, _, _, _),
-	[174] = PINGROUP(174, qdss, _, _, _, _, _, _, _, _),
+	[144] = PINGROUP(144, _, _, _, _, _, _, _, _, egpio),
+	[145] = PINGROUP(145, _, _, _, _, _, _, _, _, egpio),
+	[146] = PINGROUP(146, _, _, _, _, _, _, _, _, egpio),
+	[147] = PINGROUP(147, _, _, _, _, _, _, _, _, egpio),
+	[148] = PINGROUP(148, _, _, _, _, _, _, _, _, egpio),
+	[149] = PINGROUP(149, _, _, _, _, _, _, _, _, egpio),
+	[150] = PINGROUP(150, qdss, _, _, _, _, _, _, _, egpio),
+	[151] = PINGROUP(151, qdss, _, _, _, _, _, _, _, egpio),
+	[152] = PINGROUP(152, qdss, _, _, _, _, _, _, _, egpio),
+	[153] = PINGROUP(153, qdss, _, _, _, _, _, _, _, egpio),
+	[154] = PINGROUP(154, _, _, _, _, _, _, _, _, egpio),
+	[155] = PINGROUP(155, _, _, _, _, _, _, _, _, egpio),
+	[156] = PINGROUP(156, qdss_cti, _, _, _, _, _, _, _, egpio),
+	[157] = PINGROUP(157, qdss_cti, _, _, _, _, _, _, _, egpio),
+	[158] = PINGROUP(158, _, _, _, _, _, _, _, _, egpio),
+	[159] = PINGROUP(159, _, _, _, _, _, _, _, _, egpio),
+	[160] = PINGROUP(160, _, _, _, _, _, _, _, _, egpio),
+	[161] = PINGROUP(161, _, _, _, _, _, _, _, _, egpio),
+	[162] = PINGROUP(162, _, _, _, _, _, _, _, _, egpio),
+	[163] = PINGROUP(163, _, _, _, _, _, _, _, _, egpio),
+	[164] = PINGROUP(164, _, _, _, _, _, _, _, _, egpio),
+	[165] = PINGROUP(165, qdss_cti, _, _, _, _, _, _, _, egpio),
+	[166] = PINGROUP(166, qdss_cti, _, _, _, _, _, _, _, egpio),
+	[167] = PINGROUP(167, _, _, _, _, _, _, _, _, egpio),
+	[168] = PINGROUP(168, _, _, _, _, _, _, _, _, egpio),
+	[169] = PINGROUP(169, _, _, _, _, _, _, _, _, egpio),
+	[170] = PINGROUP(170, _, _, _, _, _, _, _, _, egpio),
+	[171] = PINGROUP(171, qdss, _, _, _, _, _, _, _, egpio),
+	[172] = PINGROUP(172, qdss, _, _, _, _, _, _, _, egpio),
+	[173] = PINGROUP(173, qdss, _, _, _, _, _, _, _, egpio),
+	[174] = PINGROUP(174, qdss, _, _, _, _, _, _, _, egpio),
 	[175] = UFS_RESET(ufs_reset, 0xbe000),
 	[176] = SDC_QDSD_PINGROUP(sdc1_rclk, 0xb3004, 0, 6),
 	[177] = SDC_QDSD_PINGROUP(sdc1_clk, 0xb3000, 13, 6),
@@ -1481,6 +1493,7 @@ static const struct msm_pinctrl_soc_data sc7280_pinctrl = {
 	.ngpios = 176,
 	.wakeirq_map = sc7280_pdc_map,
 	.nwakeirq_map = ARRAY_SIZE(sc7280_pdc_map),
+	.egpio_func = 9,
 };
 
 static int sc7280_pinctrl_probe(struct platform_device *pdev)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 2/2] pinctrl: qcom: sc7280: Add egpio support

[Bjorn Andersson]

On Tue 26 Oct 05:07 PDT 2021, Rajendra Nayak wrote:

> sc7280 supports the egpio feature, GPIOs ranging from 144 to 174
> (31 GPIOs) support it, we define gpio_func to 9, which
> is an unused function for all these pins on sc7280.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Thanks for reworking these patches, I like the end result.

Regards,
Bjorn

> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-sc7280.c | 75 ++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c
> index 9017ede..31df55c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c
> @@ -43,6 +43,8 @@
>  		.mux_bit = 2,			\
>  		.pull_bit = 0,			\
>  		.drv_bit = 6,			\
> +		.egpio_enable = 12,		\
> +		.egpio_present = 11,		\
>  		.oe_bit = 9,			\
>  		.in_bit = 0,			\
>  		.out_bit = 1,			\
> @@ -520,6 +522,7 @@ enum sc7280_functions {
>  	msm_mux_dp_lcd,
>  	msm_mux_edp_hot,
>  	msm_mux_edp_lcd,
> +	msm_mux_egpio,
>  	msm_mux_gcc_gp1,
>  	msm_mux_gcc_gp2,
>  	msm_mux_gcc_gp3,
> @@ -658,6 +661,14 @@ static const char * const gpio_groups[] = {
>  	"gpio165", "gpio166", "gpio167", "gpio168", "gpio169", "gpio170",
>  	"gpio171", "gpio172", "gpio173", "gpio174",
>  };
> +static const char * const egpio_groups[] = {
> +	"gpio144", "gpio145", "gpio146", "gpio147", "gpio148", "gpio149",
> +	"gpio150", "gpio151", "gpio152", "gpio153", "gpio154", "gpio155",
> +	"gpio156", "gpio157", "gpio158", "gpio159", "gpio160", "gpio161",
> +	"gpio162", "gpio163", "gpio164", "gpio165", "gpio166", "gpio167",
> +	"gpio168", "gpio169", "gpio170", "gpio171", "gpio172", "gpio173",
> +	"gpio174",
> +};
>  static const char * const atest_char_groups[] = {
>  	"gpio81",
>  };
> @@ -1150,6 +1161,7 @@ static const struct msm_function sc7280_functions[] = {
>  	FUNCTION(dp_lcd),
>  	FUNCTION(edp_hot),
>  	FUNCTION(edp_lcd),
> +	FUNCTION(egpio),
>  	FUNCTION(gcc_gp1),
>  	FUNCTION(gcc_gp2),
>  	FUNCTION(gcc_gp3),
> @@ -1408,37 +1420,37 @@ static const struct msm_pingroup sc7280_groups[] = {
>  	[141] = PINGROUP(141, _, _, _, _, _, _, _, _, _),
>  	[142] = PINGROUP(142, _, _, _, _, _, _, _, _, _),
>  	[143] = PINGROUP(143, _, _, _, _, _, _, _, _, _),
> -	[144] = PINGROUP(144, _, _, _, _, _, _, _, _, _),
> -	[145] = PINGROUP(145, _, _, _, _, _, _, _, _, _),
> -	[146] = PINGROUP(146, _, _, _, _, _, _, _, _, _),
> -	[147] = PINGROUP(147, _, _, _, _, _, _, _, _, _),
> -	[148] = PINGROUP(148, _, _, _, _, _, _, _, _, _),
> -	[149] = PINGROUP(149, _, _, _, _, _, _, _, _, _),
> -	[150] = PINGROUP(150, qdss, _, _, _, _, _, _, _, _),
> -	[151] = PINGROUP(151, qdss, _, _, _, _, _, _, _, _),
> -	[152] = PINGROUP(152, qdss, _, _, _, _, _, _, _, _),
> -	[153] = PINGROUP(153, qdss, _, _, _, _, _, _, _, _),
> -	[154] = PINGROUP(154, _, _, _, _, _, _, _, _, _),
> -	[155] = PINGROUP(155, _, _, _, _, _, _, _, _, _),
> -	[156] = PINGROUP(156, qdss_cti, _, _, _, _, _, _, _, _),
> -	[157] = PINGROUP(157, qdss_cti, _, _, _, _, _, _, _, _),
> -	[158] = PINGROUP(158, _, _, _, _, _, _, _, _, _),
> -	[159] = PINGROUP(159, _, _, _, _, _, _, _, _, _),
> -	[160] = PINGROUP(160, _, _, _, _, _, _, _, _, _),
> -	[161] = PINGROUP(161, _, _, _, _, _, _, _, _, _),
> -	[162] = PINGROUP(162, _, _, _, _, _, _, _, _, _),
> -	[163] = PINGROUP(163, _, _, _, _, _, _, _, _, _),
> -	[164] = PINGROUP(164, _, _, _, _, _, _, _, _, _),
> -	[165] = PINGROUP(165, qdss_cti, _, _, _, _, _, _, _, _),
> -	[166] = PINGROUP(166, qdss_cti, _, _, _, _, _, _, _, _),
> -	[167] = PINGROUP(167, _, _, _, _, _, _, _, _, _),
> -	[168] = PINGROUP(168, _, _, _, _, _, _, _, _, _),
> -	[169] = PINGROUP(169, _, _, _, _, _, _, _, _, _),
> -	[170] = PINGROUP(170, _, _, _, _, _, _, _, _, _),
> -	[171] = PINGROUP(171, qdss, _, _, _, _, _, _, _, _),
> -	[172] = PINGROUP(172, qdss, _, _, _, _, _, _, _, _),
> -	[173] = PINGROUP(173, qdss, _, _, _, _, _, _, _, _),
> -	[174] = PINGROUP(174, qdss, _, _, _, _, _, _, _, _),
> +	[144] = PINGROUP(144, _, _, _, _, _, _, _, _, egpio),
> +	[145] = PINGROUP(145, _, _, _, _, _, _, _, _, egpio),
> +	[146] = PINGROUP(146, _, _, _, _, _, _, _, _, egpio),
> +	[147] = PINGROUP(147, _, _, _, _, _, _, _, _, egpio),
> +	[148] = PINGROUP(148, _, _, _, _, _, _, _, _, egpio),
> +	[149] = PINGROUP(149, _, _, _, _, _, _, _, _, egpio),
> +	[150] = PINGROUP(150, qdss, _, _, _, _, _, _, _, egpio),
> +	[151] = PINGROUP(151, qdss, _, _, _, _, _, _, _, egpio),
> +	[152] = PINGROUP(152, qdss, _, _, _, _, _, _, _, egpio),
> +	[153] = PINGROUP(153, qdss, _, _, _, _, _, _, _, egpio),
> +	[154] = PINGROUP(154, _, _, _, _, _, _, _, _, egpio),
> +	[155] = PINGROUP(155, _, _, _, _, _, _, _, _, egpio),
> +	[156] = PINGROUP(156, qdss_cti, _, _, _, _, _, _, _, egpio),
> +	[157] = PINGROUP(157, qdss_cti, _, _, _, _, _, _, _, egpio),
> +	[158] = PINGROUP(158, _, _, _, _, _, _, _, _, egpio),
> +	[159] = PINGROUP(159, _, _, _, _, _, _, _, _, egpio),
> +	[160] = PINGROUP(160, _, _, _, _, _, _, _, _, egpio),
> +	[161] = PINGROUP(161, _, _, _, _, _, _, _, _, egpio),
> +	[162] = PINGROUP(162, _, _, _, _, _, _, _, _, egpio),
> +	[163] = PINGROUP(163, _, _, _, _, _, _, _, _, egpio),
> +	[164] = PINGROUP(164, _, _, _, _, _, _, _, _, egpio),
> +	[165] = PINGROUP(165, qdss_cti, _, _, _, _, _, _, _, egpio),
> +	[166] = PINGROUP(166, qdss_cti, _, _, _, _, _, _, _, egpio),
> +	[167] = PINGROUP(167, _, _, _, _, _, _, _, _, egpio),
> +	[168] = PINGROUP(168, _, _, _, _, _, _, _, _, egpio),
> +	[169] = PINGROUP(169, _, _, _, _, _, _, _, _, egpio),
> +	[170] = PINGROUP(170, _, _, _, _, _, _, _, _, egpio),
> +	[171] = PINGROUP(171, qdss, _, _, _, _, _, _, _, egpio),
> +	[172] = PINGROUP(172, qdss, _, _, _, _, _, _, _, egpio),
> +	[173] = PINGROUP(173, qdss, _, _, _, _, _, _, _, egpio),
> +	[174] = PINGROUP(174, qdss, _, _, _, _, _, _, _, egpio),
>  	[175] = UFS_RESET(ufs_reset, 0xbe000),
>  	[176] = SDC_QDSD_PINGROUP(sdc1_rclk, 0xb3004, 0, 6),
>  	[177] = SDC_QDSD_PINGROUP(sdc1_clk, 0xb3000, 13, 6),
> @@ -1481,6 +1493,7 @@ static const struct msm_pinctrl_soc_data sc7280_pinctrl = {
>  	.ngpios = 176,
>  	.wakeirq_map = sc7280_pdc_map,
>  	.nwakeirq_map = ARRAY_SIZE(sc7280_pdc_map),
> +	.egpio_func = 9,
>  };
>  
>  static int sc7280_pinctrl_probe(struct platform_device *pdev)
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Bjorn Andersson]

On Tue 26 Oct 05:07 PDT 2021, Rajendra Nayak wrote:

> From: Prasad Sodagudi <psodagud@codeaurora.org>
> 
> egpio is a scheme which allows special power Island Domain IOs
> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
> TLMM functions with Island Domain functions.
> With this scheme, an IO can be controlled both by the cpu running
> linux and the Island processor. This provides great flexibility to
> re-purpose the Island IOs for regular TLMM usecases.
> 
> 2 new bits are added to ctl_reg, egpio_present is a read only bit
> which shows if egpio feature is available or not on a given gpio.
> egpio_enable is the read/write bit and only effective if egpio_present
> is 1. Once its set, the Island IO is controlled from Chip TLMM.
> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
> 
> To support this we add a new function 'egpio' which can be used to
> set the egpio_enable to 0, for any other TLMM controlled functions
> we set the egpio_enable to 1.
> 
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
>  drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 8476a8a..bfdba3a 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>  	struct irq_data *d = irq_get_irq_data(irq);
>  	unsigned int gpio_func = pctrl->soc->gpio_func;
> +	unsigned int egpio_func = pctrl->soc->egpio_func;
>  	const struct msm_pingroup *g;
>  	unsigned long flags;
>  	u32 val, mask;
> @@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	val = msm_readl_ctl(pctrl, g);
> -	val &= ~mask;
> -	val |= i << g->mux_bit;
> +
> +	if (egpio_func && i == egpio_func) {
> +		if (val & BIT(g->egpio_present))
> +			val &= ~BIT(g->egpio_enable);
> +		else
> +			return -EINVAL;

You're returning here with pctrl->lock held and irqs disabled.

> +	} else {
> +		val &= ~mask;
> +		val |= i << g->mux_bit;
> +		/* Check if egpio present and enable that feature */

I never remember if egpio_enable means apss or lpass, so I think this
comment would be better as:

		/* Claim ownership of pin if egpio capable */

> +		if (egpio_func && (val & BIT(g->egpio_present)))

Can't you drop the parenthesis around the second expression?

Regards,
Bjorn

> +			val |= BIT(g->egpio_enable);
> +	}
> +
>  	msm_writel_ctl(val, pctrl, g);
>  
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index e31a516..b7110ac 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -77,6 +77,8 @@ struct msm_pingroup {
>  	unsigned drv_bit:5;
>  
>  	unsigned od_bit:5;
> +	unsigned egpio_enable:5;
> +	unsigned egpio_present:5;
>  	unsigned oe_bit:5;
>  	unsigned in_bit:5;
>  	unsigned out_bit:5;
> @@ -119,6 +121,7 @@ struct msm_gpio_wakeirq_map {
>   *                            to be aware that their parent can't handle dual
>   *                            edge interrupts.
>   * @gpio_func: Which function number is GPIO (usually 0).
> + * @egpio_func: Which function number is eGPIO
>   */
>  struct msm_pinctrl_soc_data {
>  	const struct pinctrl_pin_desc *pins;
> @@ -136,6 +139,7 @@ struct msm_pinctrl_soc_data {
>  	unsigned int nwakeirq_map;
>  	bool wakeirq_dual_edge_errata;
>  	unsigned int gpio_func;
> +	unsigned int egpio_func;
>  };
>  
>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Rajendra Nayak]

On 10/29/2021 4:31 AM, Bjorn Andersson wrote:
> On Tue 26 Oct 05:07 PDT 2021, Rajendra Nayak wrote:
> 
>> From: Prasad Sodagudi <psodagud@codeaurora.org>
>>
>> egpio is a scheme which allows special power Island Domain IOs
>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
>> TLMM functions with Island Domain functions.
>> With this scheme, an IO can be controlled both by the cpu running
>> linux and the Island processor. This provides great flexibility to
>> re-purpose the Island IOs for regular TLMM usecases.
>>
>> 2 new bits are added to ctl_reg, egpio_present is a read only bit
>> which shows if egpio feature is available or not on a given gpio.
>> egpio_enable is the read/write bit and only effective if egpio_present
>> is 1. Once its set, the Island IO is controlled from Chip TLMM.
>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>>
>> To support this we add a new function 'egpio' which can be used to
>> set the egpio_enable to 0, for any other TLMM controlled functions
>> we set the egpio_enable to 1.
>>
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
>>   drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
>>   2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 8476a8a..bfdba3a 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>   	unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>   	struct irq_data *d = irq_get_irq_data(irq);
>>   	unsigned int gpio_func = pctrl->soc->gpio_func;
>> +	unsigned int egpio_func = pctrl->soc->egpio_func;
>>   	const struct msm_pingroup *g;
>>   	unsigned long flags;
>>   	u32 val, mask;
>> @@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>   	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>   
>>   	val = msm_readl_ctl(pctrl, g);
>> -	val &= ~mask;
>> -	val |= i << g->mux_bit;
>> +
>> +	if (egpio_func && i == egpio_func) {
>> +		if (val & BIT(g->egpio_present))
>> +			val &= ~BIT(g->egpio_enable);
>> +		else
>> +			return -EINVAL;
> 
> You're returning here with pctrl->lock held and irqs disabled.

argh, right. I will fix that and repost.
I wonder if I should just drop that error handling completely,
we wouldn't end up here unless the platform driver wrongly populates
a pin which does not support egpio with a egpio function.

> 
>> +	} else {
>> +		val &= ~mask;
>> +		val |= i << g->mux_bit;
>> +		/* Check if egpio present and enable that feature */
> 
> I never remember if egpio_enable means apss or lpass, so I think this
> comment would be better as:
> 
> 		/* Claim ownership of pin if egpio capable */

:) makes sense

> 
>> +		if (egpio_func && (val & BIT(g->egpio_present)))
> 
> Can't you drop the parenthesis around the second expression?

yes, will do, thanks for the review.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Stephen Boyd]

Quoting Rajendra Nayak (2021-10-26 05:07:35)
> From: Prasad Sodagudi <psodagud@codeaurora.org>
>
> egpio is a scheme which allows special power Island Domain IOs
> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
> TLMM functions with Island Domain functions.
> With this scheme, an IO can be controlled both by the cpu running
> linux and the Island processor. This provides great flexibility to
> re-purpose the Island IOs for regular TLMM usecases.
>
> 2 new bits are added to ctl_reg, egpio_present is a read only bit
> which shows if egpio feature is available or not on a given gpio.
> egpio_enable is the read/write bit and only effective if egpio_present
> is 1. Once its set, the Island IO is controlled from Chip TLMM.
> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>
> To support this we add a new function 'egpio' which can be used to
> set the egpio_enable to 0, for any other TLMM controlled functions
> we set the egpio_enable to 1.
>
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Does this supersede adding support for lpass pinctrl in this series[1]?

[1] https://lore.kernel.org/r/1635342097-2726-1-git-send-email-srivasam@codeaurora.org

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Rajendra Nayak]

On 10/29/2021 12:24 PM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2021-10-26 05:07:35)
>> From: Prasad Sodagudi <psodagud@codeaurora.org>
>>
>> egpio is a scheme which allows special power Island Domain IOs
>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
>> TLMM functions with Island Domain functions.
>> With this scheme, an IO can be controlled both by the cpu running
>> linux and the Island processor. This provides great flexibility to
>> re-purpose the Island IOs for regular TLMM usecases.
>>
>> 2 new bits are added to ctl_reg, egpio_present is a read only bit
>> which shows if egpio feature is available or not on a given gpio.
>> egpio_enable is the read/write bit and only effective if egpio_present
>> is 1. Once its set, the Island IO is controlled from Chip TLMM.
>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>>
>> To support this we add a new function 'egpio' which can be used to
>> set the egpio_enable to 0, for any other TLMM controlled functions
>> we set the egpio_enable to 1.
>>
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
> 
> Does this supersede adding support for lpass pinctrl in this series[1]?

No, the driver in [1] actually manages the LPASS TLMM instance, while this patch
makes it possible for the 'same' pins to be managed by the SoC TLMM instance.
On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174
maps to SSC-GPIO-0-15.

> [1] https://lore.kernel.org/r/1635342097-2726-1-git-send-email-srivasam@codeaurora.org
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Stephen Boyd]

Quoting Rajendra Nayak (2021-10-29 03:19:04)
>
>
> On 10/29/2021 12:24 PM, Stephen Boyd wrote:
> > Quoting Rajendra Nayak (2021-10-26 05:07:35)
> >> From: Prasad Sodagudi <psodagud@codeaurora.org>
> >>
> >> egpio is a scheme which allows special power Island Domain IOs
> >> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
> >> TLMM functions with Island Domain functions.
> >> With this scheme, an IO can be controlled both by the cpu running
> >> linux and the Island processor. This provides great flexibility to
> >> re-purpose the Island IOs for regular TLMM usecases.
> >>
> >> 2 new bits are added to ctl_reg, egpio_present is a read only bit
> >> which shows if egpio feature is available or not on a given gpio.
> >> egpio_enable is the read/write bit and only effective if egpio_present
> >> is 1. Once its set, the Island IO is controlled from Chip TLMM.
> >> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
> >>
> >> To support this we add a new function 'egpio' which can be used to
> >> set the egpio_enable to 0, for any other TLMM controlled functions
> >> we set the egpio_enable to 1.
> >>
> >> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> >> ---
> >
> > Does this supersede adding support for lpass pinctrl in this series[1]?
>
> No, the driver in [1] actually manages the LPASS TLMM instance, while this patch
> makes it possible for the 'same' pins to be managed by the SoC TLMM instance.
> On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174
> maps to SSC-GPIO-0-15.

How do we make sure that the LPASS pins are actually muxed out of the
SoC and not blocked by eGPIO in this driver muxing out the pin as a
gpio? Do they avoid conflicting with each other somehow?

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Rajendra Nayak]

On 10/30/2021 12:35 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2021-10-29 03:19:04)
>>
>>
>> On 10/29/2021 12:24 PM, Stephen Boyd wrote:
>>> Quoting Rajendra Nayak (2021-10-26 05:07:35)
>>>> From: Prasad Sodagudi <psodagud@codeaurora.org>
>>>>
>>>> egpio is a scheme which allows special power Island Domain IOs
>>>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
>>>> TLMM functions with Island Domain functions.
>>>> With this scheme, an IO can be controlled both by the cpu running
>>>> linux and the Island processor. This provides great flexibility to
>>>> re-purpose the Island IOs for regular TLMM usecases.
>>>>
>>>> 2 new bits are added to ctl_reg, egpio_present is a read only bit
>>>> which shows if egpio feature is available or not on a given gpio.
>>>> egpio_enable is the read/write bit and only effective if egpio_present
>>>> is 1. Once its set, the Island IO is controlled from Chip TLMM.
>>>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>>>>
>>>> To support this we add a new function 'egpio' which can be used to
>>>> set the egpio_enable to 0, for any other TLMM controlled functions
>>>> we set the egpio_enable to 1.
>>>>
>>>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>> ---
>>>
>>> Does this supersede adding support for lpass pinctrl in this series[1]?
>>
>> No, the driver in [1] actually manages the LPASS TLMM instance, while this patch
>> makes it possible for the 'same' pins to be managed by the SoC TLMM instance.
>> On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174
>> maps to SSC-GPIO-0-15.
> 
> How do we make sure that the LPASS pins are actually muxed out of the
> SoC and not blocked by eGPIO in this driver muxing out the pin as a
> gpio? Do they avoid conflicting with each other somehow?

No, currently they don't. The default value of egpio_enable is 0, so
if SoC TLMM grabs it first and sets it to 1 and then the LPASS TLMM tries
to grab it, perhaps it can find out and throw and error, though I don;t
think it does that today.
I am not sure how this can be caught when its the other way round (LPASS grabs it first)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Doug Anderson]

Hi,

On Tue, Oct 26, 2021 at 5:09 AM Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> From: Prasad Sodagudi <psodagud@codeaurora.org>
>
> egpio is a scheme which allows special power Island Domain IOs
> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
> TLMM functions with Island Domain functions.
> With this scheme, an IO can be controlled both by the cpu running
> linux and the Island processor. This provides great flexibility to
> re-purpose the Island IOs for regular TLMM usecases.
>
> 2 new bits are added to ctl_reg, egpio_present is a read only bit
> which shows if egpio feature is available or not on a given gpio.
> egpio_enable is the read/write bit and only effective if egpio_present
> is 1. Once its set, the Island IO is controlled from Chip TLMM.
> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>
> To support this we add a new function 'egpio' which can be used to
> set the egpio_enable to 0, for any other TLMM controlled functions
> we set the egpio_enable to 1.
>
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
>  drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 8476a8a..bfdba3a 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>         unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>         struct irq_data *d = irq_get_irq_data(irq);
>         unsigned int gpio_func = pctrl->soc->gpio_func;
> +       unsigned int egpio_func = pctrl->soc->egpio_func;
>         const struct msm_pingroup *g;
>         unsigned long flags;
>         u32 val, mask;
> @@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>         val = msm_readl_ctl(pctrl, g);
> -       val &= ~mask;
> -       val |= i << g->mux_bit;
> +
> +       if (egpio_func && i == egpio_func) {
> +               if (val & BIT(g->egpio_present))
> +                       val &= ~BIT(g->egpio_enable);
> +               else
> +                       return -EINVAL;
> +       } else {
> +               val &= ~mask;
> +               val |= i << g->mux_bit;
> +               /* Check if egpio present and enable that feature */
> +               if (egpio_func && (val & BIT(g->egpio_present)))
> +                       val |= BIT(g->egpio_enable);
> +       }
> +
>         msm_writel_ctl(val, pctrl, g);
>
>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index e31a516..b7110ac 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -77,6 +77,8 @@ struct msm_pingroup {
>         unsigned drv_bit:5;
>
>         unsigned od_bit:5;
> +       unsigned egpio_enable:5;
> +       unsigned egpio_present:5;
>         unsigned oe_bit:5;
>         unsigned in_bit:5;
>         unsigned out_bit:5;
> @@ -119,6 +121,7 @@ struct msm_gpio_wakeirq_map {
>   *                            to be aware that their parent can't handle dual
>   *                            edge interrupts.
>   * @gpio_func: Which function number is GPIO (usually 0).
> + * @egpio_func: Which function number is eGPIO

nit: in the above, document that this is actually a _virtual_ number.
In other words it doesn't actually map to any real hardware register
setting. Also maybe document 0 here means that eGPIO isn't supported
on this SoC. ...and lastly, all the other entries in this docstring
end with a ".". Something roughly like this:

 * @egpio_func: If non-zero then this SoC supports eGPIO. Even though in
                hardware this is a mux 1-level above the TLMM, we'll treat
                it as if this is just another mux state of the TLMM. Since
                it doesn't really map to hardware, we'll allocate a virtual
                function number for eGPIO and any time we see that function
                number used we'll treat it as a request to mux away from
                our TLMM towards another owner.

Thinking about this made me look a little closer at your virtual
function number, though. On sc7280 (in the next patch) you chose
function "9" as GPIO. Things smell a little strange, though.
Apparently sc7280 was already setup for a non-virtual "function 9"
since "nfuncs" was 10. Was this just a fortunate bug that kept you
from having to touch all the sc7280 PINGROUP definitions in the next
patch, or is there actually a true "function 9" somewhere in the
hardware that we might want to someday add to Linux? If so, should we
pick eGPIO as 10?

...and then, looking further, what would happen if we picked eGPIO 10?
Should "nfuncs" include this virtual number, or not? If "nfuncs"
_does_ include this number and it bumps you over to the next
"order_base_2" then the mask calculated by msm_pinmux_set_mux() will
be wrong. If "nfuncs" _doesn't_ include this number then we should
probably document that fact, and (I suppose) change sc7280's "nfuncs"
down to 9?


-Doug

Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support

[Rajendra Nayak]

On 11/2/2021 2:34 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 26, 2021 at 5:09 AM Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> From: Prasad Sodagudi <psodagud@codeaurora.org>
>>
>> egpio is a scheme which allows special power Island Domain IOs
>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular
>> TLMM functions with Island Domain functions.
>> With this scheme, an IO can be controlled both by the cpu running
>> linux and the Island processor. This provides great flexibility to
>> re-purpose the Island IOs for regular TLMM usecases.
>>
>> 2 new bits are added to ctl_reg, egpio_present is a read only bit
>> which shows if egpio feature is available or not on a given gpio.
>> egpio_enable is the read/write bit and only effective if egpio_present
>> is 1. Once its set, the Island IO is controlled from Chip TLMM.
>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO.
>>
>> To support this we add a new function 'egpio' which can be used to
>> set the egpio_enable to 0, for any other TLMM controlled functions
>> we set the egpio_enable to 1.
>>
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
>>   drivers/pinctrl/qcom/pinctrl-msm.h |  4 ++++
>>   2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 8476a8a..bfdba3a 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>          unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>          struct irq_data *d = irq_get_irq_data(irq);
>>          unsigned int gpio_func = pctrl->soc->gpio_func;
>> +       unsigned int egpio_func = pctrl->soc->egpio_func;
>>          const struct msm_pingroup *g;
>>          unsigned long flags;
>>          u32 val, mask;
>> @@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>          raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>          val = msm_readl_ctl(pctrl, g);
>> -       val &= ~mask;
>> -       val |= i << g->mux_bit;
>> +
>> +       if (egpio_func && i == egpio_func) {
>> +               if (val & BIT(g->egpio_present))
>> +                       val &= ~BIT(g->egpio_enable);
>> +               else
>> +                       return -EINVAL;
>> +       } else {
>> +               val &= ~mask;
>> +               val |= i << g->mux_bit;
>> +               /* Check if egpio present and enable that feature */
>> +               if (egpio_func && (val & BIT(g->egpio_present)))
>> +                       val |= BIT(g->egpio_enable);
>> +       }
>> +
>>          msm_writel_ctl(val, pctrl, g);
>>
>>          raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
>> index e31a516..b7110ac 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
>> @@ -77,6 +77,8 @@ struct msm_pingroup {
>>          unsigned drv_bit:5;
>>
>>          unsigned od_bit:5;
>> +       unsigned egpio_enable:5;
>> +       unsigned egpio_present:5;
>>          unsigned oe_bit:5;
>>          unsigned in_bit:5;
>>          unsigned out_bit:5;
>> @@ -119,6 +121,7 @@ struct msm_gpio_wakeirq_map {
>>    *                            to be aware that their parent can't handle dual
>>    *                            edge interrupts.
>>    * @gpio_func: Which function number is GPIO (usually 0).
>> + * @egpio_func: Which function number is eGPIO
> 
> nit: in the above, document that this is actually a _virtual_ number.
> In other words it doesn't actually map to any real hardware register
> setting. Also maybe document 0 here means that eGPIO isn't supported
> on this SoC. ...and lastly, all the other entries in this docstring
> end with a ".". Something roughly like this:
> 
>   * @egpio_func: If non-zero then this SoC supports eGPIO. Even though in
>                  hardware this is a mux 1-level above the TLMM, we'll treat
>                  it as if this is just another mux state of the TLMM. Since
>                  it doesn't really map to hardware, we'll allocate a virtual
>                  function number for eGPIO and any time we see that function
>                  number used we'll treat it as a request to mux away from
>                  our TLMM towards another owner.

Thanks Doug, this sounds perfect, I'll copy-paste it :)

> Thinking about this made me look a little closer at your virtual
> function number, though. On sc7280 (in the next patch) you chose
> function "9" as GPIO. Things smell a little strange, though.
> Apparently sc7280 was already setup for a non-virtual "function 9"
> since "nfuncs" was 10. Was this just a fortunate bug that kept you
> from having to touch all the sc7280 PINGROUP definitions in the next
> patch, or is there actually a true "function 9" somewhere in the
> hardware that we might want to someday add to Linux? If so, should we
> pick eGPIO as 10?

Right, I did start off thinking I would need to add a new function, and
deal with changing all the PINGROUP definitions, and worry about all the
stuff that you mentioned below, but then I realized function 9 was actually
unused on all sc7280 pins (and I did look at the couple other SoCs usptream
which support egpio which also had the same pattern) so decided to just reuse
function 9 and avoid dealing with all the mess that would result
with adding a new virtual function ¯\_(ツ)_/¯

> ...and then, looking further, what would happen if we picked eGPIO 10?
> Should "nfuncs" include this virtual number, or not? If "nfuncs"
> _does_ include this number and it bumps you over to the next
> "order_base_2" then the mask calculated by msm_pinmux_set_mux() will
> be wrong. If "nfuncs" _doesn't_ include this number then we should
> probably document that fact, and (I suppose) change sc7280's "nfuncs"
> down to 9?
> 
> 
> -Doug
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation