Skip to contentSkip to conversation

pinctrl: qcom: sc7180: Make gpio28 non wakeup capable for google,lazor

PATCH In review Rajendra Nayak 4 participants 9 messages Jul 3, 2020 – Jul 8, 2020

Subsystems (5)
Arm Qualcomm mailing list GPIO Pin control Pin controller - Qualcomm +1 more subsystems
lore
Share & tools
lore

Rajendra Nayak

author
#0 lore raw

PATCH v2 1/1 pinctrl: qcom: sc7180: Make gpio28 non wakeup capable for google,lazor

The PDC irqchip driver currently does not handle dual-edge interrupts, and we have google,lazor board with sc7180 designed to configure gpio28 as a dual-edge interrupt. This interrupt is however not expected to be wakeup capable on this board, so an easy way to fix this, seems to be to make this gpio non wakeup capable and let TLMM handle it (which is capable of handling dual-edge irqs)

To be able to do so only on this board, so other boards designed with this SoC can continue to use gpio28 as a wakeup capable one, make a copy of msm_gpio_wakeirq_map for lazor and remove gpio28 from the list.

Reported-by Jimmy Cheng-Yi Chiang <[email protected]>
Signed-off-by Rajendra Nayak <[email protected]>
---
 drivers/pinctrl/qcom/pinctrl-sc7180.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/pinctrl/qcom/pinctrl-sc7180.c +22 -1
index 1b6465a..0668933 100644
@@ -1135,7 +1135,24 @@ static const struct msm_gpio_wakeirq_map sc7180_pdc_map[] = {
 	{117, 114}, {118, 119},
 };
 
-static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
+/* Dropped gpio28 from the map for the google,lazor board */
+static const struct msm_gpio_wakeirq_map sc7180_lazor_pdc_map[] = {
+	{0, 40}, {3, 50}, {4, 42}, {5, 70}, {6, 41}, {9, 35},
+	{10, 80}, {11, 51}, {16, 20}, {21, 55}, {22, 90}, {23, 21},
+	{24, 61}, {26, 52}, {30, 100}, {31, 33}, {32, 81},
+	{33, 62}, {34, 43}, {36, 91}, {37, 53}, {38, 63}, {39, 72},
+	{41, 101}, {42, 7}, {43, 34}, {45, 73}, {47, 82}, {49, 17},
+	{52, 109}, {53, 102}, {55, 92}, {56, 56}, {57, 57}, {58, 83},
+	{59, 37}, {62, 110}, {63, 111}, {64, 74}, {65, 44}, {66, 93},
+	{67, 58}, {68, 112}, {69, 32}, {70, 54}, {72, 59}, {73, 64},
+	{74, 71}, {78, 31}, {82, 30}, {85, 103}, {86, 38}, {87, 39},
+	{88, 45}, {89, 46}, {90, 47}, {91, 48}, {92, 60}, {93, 49},
+	{94, 84}, {95, 94}, {98, 65}, {101, 66}, {104, 67}, {109, 104},
+	{110, 68}, {113, 69}, {114, 113}, {115, 108}, {116, 121},
+	{117, 114}, {118, 119},
+};
+
+static struct msm_pinctrl_soc_data sc7180_pinctrl = {
 	.pins = sc7180_pins,
 	.npins = ARRAY_SIZE(sc7180_pins),
 	.functions = sc7180_functions,

@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
 static int sc7180_pinctrl_probe(struct platform_device *pdev)
 {
+	if (of_machine_is_compatible("google,lazor")) {
+		sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+		sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+	}
 	return msm_pinctrl_probe(pdev, &sc7180_pinctrl);
 }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#1 lore raw

Hi,

Quoted 50 lines · Rajendra Nayak

The PDC irqchip driver currently does not handle dual-edge interrupts, and we have google,lazor board with sc7180 designed to configure gpio28 as a dual-edge interrupt. This interrupt is however not expected to be wakeup capable on this board, so an easy way to fix this, seems to be to make this gpio non wakeup capable and let TLMM handle it (which is capable of handling dual-edge irqs)

To be able to do so only on this board, so other boards designed with this SoC can continue to use gpio28 as a wakeup capable one, make a copy of msm_gpio_wakeirq_map for lazor and remove gpio28 from the list.

Reported-by Jimmy Cheng-Yi Chiang <[email protected]>
Signed-off-by Rajendra Nayak <[email protected]>
---
 drivers/pinctrl/qcom/pinctrl-sc7180.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/pinctrl/qcom/pinctrl-sc7180.c +18 -1
index 1b6465a..0668933 100644
@@ -1135,7 +1135,24 @@ static const struct msm_gpio_wakeirq_map sc7180_pdc_map[] = {
        {117, 114}, {118, 119},
 };
 
-static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
+/* Dropped gpio28 from the map for the google,lazor board */
+static const struct msm_gpio_wakeirq_map sc7180_lazor_pdc_map[] = {
+       {0, 40}, {3, 50}, {4, 42}, {5, 70}, {6, 41}, {9, 35},
+       {10, 80}, {11, 51}, {16, 20}, {21, 55}, {22, 90}, {23, 21},
+       {24, 61}, {26, 52}, {30, 100}, {31, 33}, {32, 81},
+       {33, 62}, {34, 43}, {36, 91}, {37, 53}, {38, 63}, {39, 72},
+       {41, 101}, {42, 7}, {43, 34}, {45, 73}, {47, 82}, {49, 17},
+       {52, 109}, {53, 102}, {55, 92}, {56, 56}, {57, 57}, {58, 83},
+       {59, 37}, {62, 110}, {63, 111}, {64, 74}, {65, 44}, {66, 93},
+       {67, 58}, {68, 112}, {69, 32}, {70, 54}, {72, 59}, {73, 64},
+       {74, 71}, {78, 31}, {82, 30}, {85, 103}, {86, 38}, {87, 39},
+       {88, 45}, {89, 46}, {90, 47}, {91, 48}, {92, 60}, {93, 49},
+       {94, 84}, {95, 94}, {98, 65}, {101, 66}, {104, 67}, {109, 104},
+       {110, 68}, {113, 69}, {114, 113}, {115, 108}, {116, 121},
+       {117, 114}, {118, 119},
+};
+
+static struct msm_pinctrl_soc_data sc7180_pinctrl = {
        .pins = sc7180_pins,
        .npins = ARRAY_SIZE(sc7180_pins),
        .functions = sc7180_functions,

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 7 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
 static int sc7180_pinctrl_probe(struct platform_device *pdev)
 {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly. As we have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

...so of the 5 boards you'll only match one of them.

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts

...and if you look at those two commits they refer to other examples. The mediatek one says:

Quoted 1 line

This follows an example of drivers/gpio/gpio-mxc.c.

...and the Rockchip one says:

Quoted 1 line

implement a solution similar to pinctrl-coh901

That means you have at least 4 samples to look at?

-Doug

#2 lore raw
Quoted 54 lines · Doug Anderson

Hi,

Quoted 51 lines · Rajendra Nayak

The PDC irqchip driver currently does not handle dual-edge interrupts, and we have google,lazor board with sc7180 designed to configure gpio28 as a dual-edge interrupt. This interrupt is however not expected to be wakeup capable on this board, so an easy way to fix this, seems to be to make this gpio non wakeup capable and let TLMM handle it (which is capable of handling dual-edge irqs)

To be able to do so only on this board, so other boards designed with this SoC can continue to use gpio28 as a wakeup capable one, make a copy of msm_gpio_wakeirq_map for lazor and remove gpio28 from the list.

Reported-by Jimmy Cheng-Yi Chiang <[email protected]>
Signed-off-by Rajendra Nayak <[email protected]>
---
 drivers/pinctrl/qcom/pinctrl-sc7180.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/pinctrl/qcom/pinctrl-sc7180.c +18 -1
index 1b6465a..0668933 100644
@@ -1135,7 +1135,24 @@ static const struct msm_gpio_wakeirq_map sc7180_pdc_map[] = {
        {117, 114}, {118, 119},
 };
 
-static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
+/* Dropped gpio28 from the map for the google,lazor board */
+static const struct msm_gpio_wakeirq_map sc7180_lazor_pdc_map[] = {
+       {0, 40}, {3, 50}, {4, 42}, {5, 70}, {6, 41}, {9, 35},
+       {10, 80}, {11, 51}, {16, 20}, {21, 55}, {22, 90}, {23, 21},
+       {24, 61}, {26, 52}, {30, 100}, {31, 33}, {32, 81},
+       {33, 62}, {34, 43}, {36, 91}, {37, 53}, {38, 63}, {39, 72},
+       {41, 101}, {42, 7}, {43, 34}, {45, 73}, {47, 82}, {49, 17},
+       {52, 109}, {53, 102}, {55, 92}, {56, 56}, {57, 57}, {58, 83},
+       {59, 37}, {62, 110}, {63, 111}, {64, 74}, {65, 44}, {66, 93},
+       {67, 58}, {68, 112}, {69, 32}, {70, 54}, {72, 59}, {73, 64},
+       {74, 71}, {78, 31}, {82, 30}, {85, 103}, {86, 38}, {87, 39},
+       {88, 45}, {89, 46}, {90, 47}, {91, 48}, {92, 60}, {93, 49},
+       {94, 84}, {95, 94}, {98, 65}, {101, 66}, {104, 67}, {109, 104},
+       {110, 68}, {113, 69}, {114, 113}, {115, 108}, {116, 121},
+       {117, 114}, {118, 119},
+};
+
+static struct msm_pinctrl_soc_data sc7180_pinctrl = {
        .pins = sc7180_pins,
        .npins = ARRAY_SIZE(sc7180_pins),
        .functions = sc7180_functions,

@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 6 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
 static int sc7180_pinctrl_probe(struct platform_device *pdev)
 {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

I second that.

Quoted 22 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

...so of the 5 boards you'll only match one of them.

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

Quoted 4 lines
$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts

pinctrl-msm already supports emulating dual edge, but my understanding was that the problem lies in that somehow this emulation would have to be tied to or affect the PDC driver?

Regards,
Bjorn

Quoted 13 lines

...and if you look at those two commits they refer to other examples. The mediatek one says:

Quoted 1 line

This follows an example of drivers/gpio/gpio-mxc.c.

...and the Rockchip one says:

Quoted 1 line

implement a solution similar to pinctrl-coh901

That means you have at least 4 samples to look at?

-Doug

author
#3 lore raw

[]..

Quoted 13 lines
Quoted 12 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
  static int sc7180_pinctrl_probe(struct platform_device *pdev)
  {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

Quoted 20 lines

I second that.

Quoted 18 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

...so of the 5 boards you'll only match one of them.

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or

FWIK, this is really a PDC hardware issue (with the specific IP rev that exists on sc7180) so working it around in SW could get ugly.

Quoted 4 lines
Quoted 4 lines

maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

Quoted 8 lines
Quoted 4 lines
$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts

pinctrl-msm already supports emulating dual edge, but my understanding was that the problem lies in that somehow this emulation would have to be tied to or affect the PDC driver?

yes, thats correct, pinctrl-msm already supports it, the problem lies in the fact that PDC does not. This patch, infact was trying to fix the issue by removing all PDC involvement for gpio28 and making pinctrl-msm in charge of it.

Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#4 lore raw

Hi,

Quoted 34 lines · Rajendra Nayak

[]..

Quoted 30 lines
Quoted 12 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
  static int sc7180_pinctrl_probe(struct platform_device *pdev)
  {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

I second that.

Quoted 15 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

...so of the 5 boards you'll only match one of them.

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or

FWIK, this is really a PDC hardware issue (with the specific IP rev that exists on sc7180) so working it around in SW could get ugly.

Ugh. I guess it's ugly because the workaround would need to be in the PDC driver but to properly do the workaround you need to be able to read the state of the pin from the PDC driver? ...and I guess you can't do that with the PDC register space so you'd either need to violate a layer or 3 of abstraction and snarf into the GPIO register space from the PDC driver or you'd have to provide some sort of API access from the PDC back down to the GPIO driver?

--

Actually, though, I'm still not sure why this would need to be in the PDC driver. Sure, you can't just magically re-use the existing dual-edge emulation in pinctrl-msm.c, but you can add some new dual-edge emulation for when your parent handles your interrupts, can't you? As per usually, I'm talking out of my rear end, but I sorta imagine:

1. At the head of msm_gpio_irq_set_type() if you detect that "skip_wake_irqs" is set and you're on an SoC with this hardware errata then you do a loop much like the one in msm_gpio_update_dual_edge_pos() except that instead of changing the polarity with msm_writel_intr_cfg() you change the polarity with "irq_chip_set_type_parent()".

2. At the head of msm_gpio_irq_ack() you make the same function call if "skip_wake_irqs" is set and you're on an SoC with this hardware errata.

It doesn't feel all that ugly to me, assuming I'm understanding it correctly. ...or maybe you can tell me why it'd be harder than that?

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts
Quoted 8 lines
Quoted 3 lines

pinctrl-msm already supports emulating dual edge, but my understanding was that the problem lies in that somehow this emulation would have to be tied to or affect the PDC driver?

yes, thats correct, pinctrl-msm already supports it, the problem lies in the fact that PDC does not. This patch, infact was trying to fix the issue by removing all PDC involvement for gpio28 and making pinctrl-msm in charge of it.

If we're going to try to do this, I think we're stuck with one of these:

1. A really really long list that we keep jamming more boards into.

2. Add an entry at the top-level device tree compatible to all affected boards _just_ for this purpose. Seems ugly since we don't need it for any other reasons.

3. Add some sort of property to the pinctrl node on these boards. Seems ugly since conceivably this _could_ be worked around in software.

I don't really like any of those options, so I'm really hoping we can find out how to get a workaround in...

-Doug

#5 lore raw

Hi,

Quoted 69 lines · Doug Anderson

Hi,

Quoted 34 lines · Rajendra Nayak

[]..

Quoted 30 lines
Quoted 12 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
  static int sc7180_pinctrl_probe(struct platform_device *pdev)
  {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

I second that.

Quoted 15 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

...so of the 5 boards you'll only match one of them.

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or

FWIK, this is really a PDC hardware issue (with the specific IP rev that exists on sc7180) so working it around in SW could get ugly.

Ugh. I guess it's ugly because the workaround would need to be in the PDC driver but to properly do the workaround you need to be able to read the state of the pin from the PDC driver? ...and I guess you can't do that with the PDC register space so you'd either need to violate a layer or 3 of abstraction and snarf into the GPIO register space from the PDC driver or you'd have to provide some sort of API access from the PDC back down to the GPIO driver?

--

Actually, though, I'm still not sure why this would need to be in the PDC driver. Sure, you can't just magically re-use the existing dual-edge emulation in pinctrl-msm.c, but you can add some new dual-edge emulation for when your parent handles your interrupts, can't you? As per usually, I'm talking out of my rear end, but I sorta imagine:

1. At the head of msm_gpio_irq_set_type() if you detect that "skip_wake_irqs" is set and you're on an SoC with this hardware errata then you do a loop much like the one in msm_gpio_update_dual_edge_pos() except that instead of changing the polarity with msm_writel_intr_cfg() you change the polarity with "irq_chip_set_type_parent()".

Quoted 6 lines

2. At the head of msm_gpio_irq_ack() you make the same function call if "skip_wake_irqs" is set and you're on an SoC with this hardware errata.

It doesn't feel all that ugly to me, assuming I'm understanding it correctly. ...or maybe you can tell me why it'd be harder than that?

So I REALLY don't know what I'm doing and this is very rough and probably all sorts of illegal (maybe?), but in the limited testing I was able to do it seemed to work:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2285815
WIP: proof of concept workaround for PDC wakeup problem

If there's anything worthwhile to steal from that, please feel free and post your own patch based on it. :-)

-Doug

#6 lore raw
In reply to Doug Anderson

Hi,

Quoted 94 lines · Doug Anderson

Hi,

Quoted 28 lines · Rajendra Nayak

[]..

Quoted 25 lines
Quoted 11 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
   static int sc7180_pinctrl_probe(struct platform_device *pdev)
   {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

I second that.

Quoted 12 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

...so of the 5 boards you'll only match one of them.

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or

FWIK, this is really a PDC hardware issue (with the specific IP rev that exists on sc7180) so working it around in SW could get ugly.

Ugh. I guess it's ugly because the workaround would need to be in the PDC driver but to properly do the workaround you need to be able to read the state of the pin from the PDC driver? ...and I guess you can't do that with the PDC register space so you'd either need to violate a layer or 3 of abstraction and snarf into the GPIO register space from the PDC driver or you'd have to provide some sort of API access from the PDC back down to the GPIO driver?

--

Actually, though, I'm still not sure why this would need to be in the PDC driver. Sure, you can't just magically re-use the existing dual-edge emulation in pinctrl-msm.c, but you can add some new dual-edge emulation for when your parent handles your interrupts, can't you? As per usually, I'm talking out of my rear end, but I sorta imagine:

1. At the head of msm_gpio_irq_set_type() if you detect that "skip_wake_irqs" is set and you're on an SoC with this hardware errata then you do a loop much like the one in msm_gpio_update_dual_edge_pos() except that instead of changing the polarity with msm_writel_intr_cfg() you change the polarity with "irq_chip_set_type_parent()".

2. At the head of msm_gpio_irq_ack() you make the same function call if "skip_wake_irqs" is set and you're on an SoC with this hardware errata.

It doesn't feel all that ugly to me, assuming I'm understanding it correctly. ...or maybe you can tell me why it'd be harder than that?

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts
Quoted 7 lines
Quoted 3 lines

pinctrl-msm already supports emulating dual edge, but my understanding was that the problem lies in that somehow this emulation would have to be tied to or affect the PDC driver?

yes, thats correct, pinctrl-msm already supports it, the problem lies in the fact that PDC does not. This patch, infact was trying to fix the issue by removing all PDC involvement for gpio28 and making pinctrl-msm in charge of it.

If we're going to try to do this, I think we're stuck with one of these:

1. A really really long list that we keep jamming more boards into.

2. Add an entry at the top-level device tree compatible to all affected boards _just_ for this purpose. Seems ugly since we don't need it for any other reasons.

Quoted 6 lines

3. Add some sort of property to the pinctrl node on these boards. Seems ugly since conceivably this _could_ be worked around in software.

I don't really like any of those options, so I'm really hoping we can find out how to get a workaround in...

Hi Doug,

The client driver here never uses/needs both the edges at a given time.
Another option (clean & correct IMO) is to update the driver to request proper irq type its expecting.

Lets take SD card detect GPIO for example, which uses dual edge interrupt.
one edge (rising type) can be used as a card insert detect interrupt and another edge (falling type) may be used for card removal detect.

The sequence of operations, IMO should be..
1. Driver request a rising type irq to start with (the one that detects card insertion)
2. once card insertion irq comes in, the driver should change the type to falling which is expected (to detect the card removal)
3. once card removal irq comes in, it can change type to rising edge (to detect another insertion)
4. above steps (2,3) continues

if above sequence is followed from drivers using dual edge IRQ, we don't need any workaround.

Thanks,
Maulik

Quoted 2 lines

-Doug

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#7 lore raw

Hi,

Quoted 123 lines · Maulik Shah

Hi,

Quoted 94 lines · Doug Anderson

Hi,

Quoted 28 lines · Rajendra Nayak

[]..

Quoted 25 lines
Quoted 11 lines

drivers/pinctrl/qcom/pinctrl-sc7180.c static const struct msm_pinctrl_soc_data sc7180_pinctrl = { · line 1174

Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-sc7180.c +4 -0
@@ -1151,6 +1168,10 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 
   static int sc7180_pinctrl_probe(struct platform_device *pdev)
   {
+       if (of_machine_is_compatible("google,lazor")) {
+               sc7180_pinctrl.wakeirq_map = sc7180_lazor_pdc_map;
+               sc7180_pinctrl.nwakeirq_map = ARRAY_SIZE(sc7180_lazor_pdc_map);
+       }

As much as I want patches landed and things working, the above just doesn't feel like a viable solution. I guess it could work as a short term hack but it's going to become untenable pretty quickly.

I second that.

Quoted 12 lines

As we
have more variants of this we're going to have to just keep piling more machines in here, right? ...this is also already broken for us because not all boards will have the "google,lazor" compatible. From the current Chrome OS here are the compatibles for various revs/SKUs

compatible = "google,lazor-rev0", "qcom,sc7180"; compatible = "google,lazor-rev0-sku0", "qcom,sc7180"; compatible = "google,lazor", "qcom,sc7180"; compatible = "google,lazor-sku0", "qcom,sc7180"; compatible = "google,lazor-rev2", "qcom,sc7180";

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

...so of the 5 boards you'll only match one of them.

Maybe I'm jumping into a situation again where I'm ignorant since I haven't followed all the prior conversation, but is it really that hard to just add dual edge support to the PDC irqchip driver? ...or

FWIK, this is really a PDC hardware issue (with the specific IP rev that exists on sc7180) so working it around in SW could get ugly.

Ugh. I guess it's ugly because the workaround would need to be in the PDC driver but to properly do the workaround you need to be able to read the state of the pin from the PDC driver? ...and I guess you can't do that with the PDC register space so you'd either need to violate a layer or 3 of abstraction and snarf into the GPIO register space from the PDC driver or you'd have to provide some sort of API access from the PDC back down to the GPIO driver?

--

Actually, though, I'm still not sure why this would need to be in the PDC driver. Sure, you can't just magically re-use the existing dual-edge emulation in pinctrl-msm.c, but you can add some new dual-edge emulation for when your parent handles your interrupts, can't you? As per usually, I'm talking out of my rear end, but I sorta imagine:

1. At the head of msm_gpio_irq_set_type() if you detect that "skip_wake_irqs" is set and you're on an SoC with this hardware errata then you do a loop much like the one in msm_gpio_update_dual_edge_pos() except that instead of changing the polarity with msm_writel_intr_cfg() you change the polarity with "irq_chip_set_type_parent()".

2. At the head of msm_gpio_irq_ack() you make the same function call if "skip_wake_irqs" is set and you're on an SoC with this hardware errata.

It doesn't feel all that ugly to me, assuming I'm understanding it correctly. ...or maybe you can tell me why it'd be harder than that?

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

maybe it's just easier to change the pinctrl driver to emulate dual edge itself and that can work around the problem in the PDC? There seem to be a few samples you could copy from:

$ git log --oneline --no-merges --grep=emulate drivers/pinctrl/
3221f40b7631 pinctrl: mediatek: emulate GPIO interrupt on both-edges
5a92750133ff pinctrl: rockchip: emulate both edge triggered interrupts
Quoted 7 lines
Quoted 3 lines

pinctrl-msm already supports emulating dual edge, but my understanding was that the problem lies in that somehow this emulation would have to be tied to or affect the PDC driver?

yes, thats correct, pinctrl-msm already supports it, the problem lies in the fact that PDC does not. This patch, infact was trying to fix the issue by removing all PDC involvement for gpio28 and making pinctrl-msm in charge of it.

If we're going to try to do this, I think we're stuck with one of these:

1. A really really long list that we keep jamming more boards into.

2. Add an entry at the top-level device tree compatible to all affected boards _just_ for this purpose. Seems ugly since we don't need it for any other reasons.

Quoted 6 lines

3. Add some sort of property to the pinctrl node on these boards. Seems ugly since conceivably this _could_ be worked around in software.

I don't really like any of those options, so I'm really hoping we can find out how to get a workaround in...

Hi Doug,

The client driver here never uses/needs both the edges at a given time. Another option (clean & correct IMO) is to update the driver to request proper irq type its expecting.

Lets take SD card detect GPIO for example, which uses dual edge interrupt. one edge (rising type) can be used as a card insert detect interrupt and another edge (falling type) may be used for card removal detect.

The sequence of operations, IMO should be..
1. Driver request a rising type irq to start with (the one that detects card insertion)
2. once card insertion irq comes in, the driver should change the type to falling which is expected (to detect the card removal)
3. once card removal irq comes in, it can change type to rising edge (to detect another insertion)
4. above steps (2,3) continues

Quoted 2 lines

if above sequence is followed from drivers using dual edge IRQ, we don't need any workaround.

You're saying that we should make the rest of Linux change to work around the bug in your pinctrl driver? I don't think that'll fly too well with the maintainers of the other subsystems. The IRQ_TYPE_EDGE_BOTH define exists and we should support it.

Was there something wrong with my proof of concept patch? Unless I get swamped with other things, my plan was to try to make that more real and post it unless someone told me why it was wrong...

-Doug