Message-ID: <154947886779.115909.17337770809316468217@swboyd.mtv.corp.google.com>
Date:   Wed, 06 Feb 2019 10:47:47 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Lina Iyer <ilina@...eaurora.org>
Cc:     Rob Herring <robh@...nel.org>, Evan Green <evgreen@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Raju P.L.S.S.S.N" <rplsssn@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        devicetree@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

Quoting Lina Iyer (2019-02-06 09:07:30)
> Thanks for the patch Stephen. Sorry, it took a while to get to this and
> understand how this works.
> 
> On Thu, Jan 31 2019 at 00:10 -0700, Stephen Boyd wrote:
> >Quoting Stephen Boyd (2019-01-31 13:53:42)
> >>
> >> I'm prototyping out some code to do the remapping based on this type of
> >> DT property, because it will make the irqdomain::alloc function a little
> >> simpler to implement by passing in a struct irq_fwspec and getting out a
> >> parent irq_fwspec and it will make the patches to add these mapping
> >> tables in C irrelevant. Plus, I think Lina will be happy that the
> >> pinctrl driver will know if some pin maps to the PDC or not without
> >> having to see if it fails to allocate in the parent irqdomain. But there
> >> will still need to be a property for 'wakeup-parent' unless we do
> >> something to expose irqdomain tokens into DT.
> >>
> >
> >And here's the code to do this remapping idea, heavily based on the
> >phandle remapping code. I didn't properly fix up the irq alloc function
> >for the pinctrl driver here, but it should work out properly without
> >much more diff. I realize now that the pass-thru mechanism will fail
> >horribly when a specifier changes size types.
> Could you explain?

The pass-thru code maps an incoming specifier directly to an outgoing
specifier, so it won't work well if the incoming specifier size is
different than the outgoing specifier. For example, converting a GPIO
two cell to a GIC 3 cell specifier doesn't work well.

	incoming: <GPIO# flags>
	outgoing: <GIC_SPI SPI# flags>

And the pass-thru and mask properties can't "shift" or swap a u32
element of the specifier. All they can do is copy from the incoming to
the outgoing specifier. Furthermore, we only copy over how ever many
cells there are in the incoming specifier so we don't do anything with
the last cell. So if GPIO32 corresponds to GIC SPI 14 we can't copy over
the flags.

	<32 IRQ_TYPE_LEVEL_HIGH> becomes <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>

but the code only sees <32 4> and needs to copy each element over one by
one to <0 14 4>. One solution is to match on the incoming flags and then
remap them to the outgoing flags, but then pass-thru property is
unusable and we have to list all possible flag combinations on the
incoming specifier side of the mapping.


> >I guess we'll need to keep
> >that in mind if we convert the PDC driver to this too. Or we can leave
> >the PDC driver as is and not worry about listing out the individual
> >pins.
> The PDC pins are more sequential and it looks clean as it. I would
> prefer that it be left as is.

Sure. I don't see any problems keeping the status quo in the PDC driver.

> >
> >-----8<-----
> >diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >index a2241dd9c185..6b3a4227f433 100644
> >--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
[...]
> >+                                      <122 0 &pdc_intc 97 0>,
> >+                                      <123 0 &pdc_intc 98 0>,
> >+                                      <124 0 &pdc_intc 99 0>,
> >+                                      <125 0 &pdc_intc 100 0>,
> >+                                      <127 0 &pdc_intc 102 0>,
> >+                                      <128 0 &pdc_intc 103 0>,
> >+                                      <129 0 &pdc_intc 104 0>,
> >+                                      <130 0 &pdc_intc 105 0>,
> >+                                      <132 0 &pdc_intc 106 0>,
> >+                                      <133 0 &pdc_intc 107 0>,
> >+                                      <145 0 &pdc_intc 108 0>;
> >+                      irqdomain-map-mask = <0xff 0>;
> >+                      irqdomain-map-pass-thru = <0 0xff>;
> Where do we document these bindings?

In the DT spec itself or at
Documentation/devicetree/booting-without-of.txt I guess.

> >
> >                       qspi_clk: qspi-clk {
> >                               pinmux {
> >diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> >index 02ad93a304a4..b37f4cdfda53 100644
> >--- a/drivers/of/irq.c
> >+++ b/drivers/of/irq.c
> >@@ -274,6 +274,130 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
> > }
> > EXPORT_SYMBOL_GPL(of_irq_parse_raw);
> >
> I would think this would be a separate patch and I presume, I can add
> you as the author with your signed-off-by?

Sure, but I'd rather see if Rob has any views or opinions here.

> 
> >+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
> >+{
[...]
> >+      while (map_len > (in_size + 1) && !match) {
> >+              /* Compare specifiers */
> >+              match = 1;
> >+              for (i = 0; i < in_size; i++, map_len--)
> >+                      match &= !((match_array[i] ^ *map++) & mask[i]);
> >+
> >+              of_node_put(new);
> >+              new = of_find_node_by_phandle(be32_to_cpup(map));
> >+              map++;
> >+              map_len--;
> >+
> >+              /* Check if not found */
> >+              if (!new)
> >+                      goto put;
> >+
> >+              if (!of_device_is_available(new))
> >+                      match = 0;
> >+
> >+              ret = of_property_read_u32(new, cells_name, &out_size);
> >+              if (ret)
> >+                      goto put;
> >+
> >+              /* Check for malformed properties */
> >+              if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
> >+                      goto put;
> >+              if (map_len < out_size)
> >+                      goto put;
> >+
> >+              /* Move forward by new node's #interrupt-cells amount */
> >+              map += out_size;
> >+              map_len -= out_size;
> Nit: Could make this a bit simpler to understand if you broke out the
> loop on match instead of continuing the loop.

Instead of just doing that in the loop condition?

> >+      }
> >+      if (match) {
> >+              /* Get the irqdomain-map-pass-thru property (optional) */
> >+              pass = of_get_property(cur, pass_name, NULL);
> >+              if (!pass)
> >+                      pass = dummy_pass;
> >+
> >+              /*
> >+               * Successfully parsed a irqdomain-map translation; copy new
> >+               * specifier into the out structure, keeping the
> >+               * bits specified in irqdomain-map-pass-thru.
> >+               */
> >+              match_array = map - out_size;
> >+              for (i = 0; i < out_size; i++) {
> >+                      __be32 val = *(map - out_size + i);
> >+
> >+                      out->param[i] = in->param[i];
> >+                      if (i < in_size) {
> >+                              val &= ~pass[i];
> I don't get why the mask is inverted and reversed here again?

The mask is inverted to clear out the bits in the outgoing specifier for
whatever is there on the incoming side, per what the pass-thru mask
indicates should be copied over from incoming to outgoing.

> >+                              val |= cpu_to_be32(out->param[i]) & pass[i];
> >+                      }
> >+

Powered by blists - more mailing lists