Re: [PATCH v2] pinctrl: qcom: sc7180: Make gpio28 non wakeup capable for google,lazor

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Tue, Jul 7, 2020 at 4:03 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Jul 6, 2020 at 9:52 PM Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
> >
> >
> > []..
> >
> > >>> @@ -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.
> > >
> > >> 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.
>
> 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?

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