| Message ID | 20181219221105.3004-5-ilina@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: > SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO > routed to the PDC as interrupts that can be used to wake the system up > from deep low power modes and suspend. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > index 665aadb5ea28..a522ca46667d 100644 > --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > @@ -29,6 +29,11 @@ SDM845 platform. > Definition: must be 2. Specifying the pin number and flags, as defined > in <dt-bindings/interrupt-controller/irq.h> > > +- wakeup-parent: > + Usage: optional > + Value type: <phandle> > + Definition: A phandle to the wakeup interrupt controller for the SoC. Is this really necessary? Is there more than one possible wakeup-parent node? > + > - gpio-controller: > Usage: required > Value type: <none> > @@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the > mux function to select on those pin(s)/group(s), and various pin configuration > parameters, such as pull-up, drive strength, etc. > > - > PIN CONFIGURATION NODES: > > The name of each subnode is not important; all subnodes should be enumerated > @@ -160,6 +164,7 @@ Example: > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <2>; > + wakeup-parent = <&pdc>; > > qup9_active: qup9-active { > mux { > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote: >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO >> routed to the PDC as interrupts that can be used to wake the system up >> from deep low power modes and suspend. >> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> index 665aadb5ea28..a522ca46667d 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> @@ -29,6 +29,11 @@ SDM845 platform. >> Definition: must be 2. Specifying the pin number and flags, as defined >> in <dt-bindings/interrupt-controller/irq.h> >> >> +- wakeup-parent: >> + Usage: optional >> + Value type: <phandle> >> + Definition: A phandle to the wakeup interrupt controller for the SoC. > >Is this really necessary? Is there more than one possible wakeup-parent >node? > No. There is only one but depending on the architecture, the wakeup interrupt controller could be different device like PDC on SDM845 or MPM on SDM820. What do you have in mind? Let me know if you have a better idea than referencing in DT. Thanks, Lina >> + >> - gpio-controller: >> Usage: required >> Value type: <none> >> @@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the >> mux function to select on those pin(s)/group(s), and various pin configuration >> parameters, such as pull-up, drive strength, etc. >> >> - >> PIN CONFIGURATION NODES: >> >> The name of each subnode is not important; all subnodes should be enumerated >> @@ -160,6 +164,7 @@ Example: >> #gpio-cells = <2>; >> interrupt-controller; >> #interrupt-cells = <2>; >> + wakeup-parent = <&pdc>; >> >> qup9_active: qup9-active { >> mux { >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >>
On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote: > > On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote: > >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: > >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO > >> routed to the PDC as interrupts that can be used to wake the system up > >> from deep low power modes and suspend. > >> > >> Cc: devicetree@vger.kernel.org > >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> > >> --- > >> .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> index 665aadb5ea28..a522ca46667d 100644 > >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> @@ -29,6 +29,11 @@ SDM845 platform. > >> Definition: must be 2. Specifying the pin number and flags, as defined > >> in <dt-bindings/interrupt-controller/irq.h> > >> > >> +- wakeup-parent: > >> + Usage: optional > >> + Value type: <phandle> > >> + Definition: A phandle to the wakeup interrupt controller for the SoC. > > > >Is this really necessary? Is there more than one possible wakeup-parent > >node? > > > No. There is only one but depending on the architecture, the wakeup > interrupt controller could be different device like PDC on SDM845 or MPM > on SDM820. > > What do you have in mind? Let me know if you have a better idea than > referencing in DT. If there's only one possibility for a given platform, then you can just use of_find_compatible_node(). I don't think it matters that different platforms have a different device here. It's not going to be a large table and you may need to know the differences if there's not an abstracted interface to it (seems there is in your case). Alternatively, if the PDC/MPM code knows what interrupt controller it is associated with, then it could setup that relationship and the interrupt controller code could retrieve that. Maybe the stacked domain support doesn't work in that direction (I haven't looked at the irq code much since that was added). However, my main concern is documenting something genericish in a device specific binding. It looks like Tegra is trying to add the same thing, so this needs to be documented in a common place. One question is whether wakeup is the only use or if this should be more generally a secondary interrupt parent? Rob
On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote: >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote: >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO >> >> routed to the PDC as interrupts that can be used to wake the system up >> >> from deep low power modes and suspend. >> >> >> >> Cc: devicetree@vger.kernel.org >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> >> --- >> >> .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- >> >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> index 665aadb5ea28..a522ca46667d 100644 >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> @@ -29,6 +29,11 @@ SDM845 platform. >> >> Definition: must be 2. Specifying the pin number and flags, as defined >> >> in <dt-bindings/interrupt-controller/irq.h> >> >> >> >> +- wakeup-parent: >> >> + Usage: optional >> >> + Value type: <phandle> >> >> + Definition: A phandle to the wakeup interrupt controller for the SoC. >> > >> >Is this really necessary? Is there more than one possible wakeup-parent >> >node? >> > >> No. There is only one but depending on the architecture, the wakeup >> interrupt controller could be different device like PDC on SDM845 or MPM >> on SDM820. >> >> What do you have in mind? Let me know if you have a better idea than >> referencing in DT. > >If there's only one possibility for a given platform, then you can >just use of_find_compatible_node(). I don't think it matters that >different platforms have a different device here. It's not going to be >a large table and you may need to know the differences if there's not >an abstracted interface to it (seems there is in your case). The GPIO irqchip would be in hierarchy with the wakeup-parent irqchip and no device specific functions would be called directly. We could achieve this with compatible strings to the irqchip. >Alternatively, if the PDC/MPM code knows what interrupt controller it >is associated with, then it could setup that relationship and the >interrupt controller code could retrieve that. Maybe the stacked >domain support doesn't work in that direction (I haven't looked at the >irq code much since that was added). > The PDC/MPM do not know about the association. >However, my main concern is documenting something genericish in a >device specific binding. It looks like Tegra is trying to add the same >thing, so this needs to be documented in a common place. One question >is whether wakeup is the only use or if this should be more generally >a secondary interrupt parent? > Yes, wakeup is the only use of this interrupt parent. It is powered by an always-on rail and therefore can detect some interrupts that are routed to it even when the GIC is powered off. Though Tegra's implementation of the irqchip is a bit different from QCOM, the idea is generally the same. It would be helpful, if we could make this a generic enough binding. -- Lina
On Wed, Jan 9, 2019 at 11:31 AM Lina Iyer <ilina@codeaurora.org> wrote: > > On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote: > >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote: > >> > >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote: > >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: > >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO > >> >> routed to the PDC as interrupts that can be used to wake the system up > >> >> from deep low power modes and suspend. > >> >> > >> >> Cc: devicetree@vger.kernel.org > >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> > >> >> --- > >> >> .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- > >> >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> >> index 665aadb5ea28..a522ca46667d 100644 > >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt > >> >> @@ -29,6 +29,11 @@ SDM845 platform. > >> >> Definition: must be 2. Specifying the pin number and flags, as defined > >> >> in <dt-bindings/interrupt-controller/irq.h> > >> >> > >> >> +- wakeup-parent: > >> >> + Usage: optional > >> >> + Value type: <phandle> > >> >> + Definition: A phandle to the wakeup interrupt controller for the SoC. > >> > > >> >Is this really necessary? Is there more than one possible wakeup-parent > >> >node? > >> > > >> No. There is only one but depending on the architecture, the wakeup > >> interrupt controller could be different device like PDC on SDM845 or MPM > >> on SDM820. > >> > >> What do you have in mind? Let me know if you have a better idea than > >> referencing in DT. > > > >If there's only one possibility for a given platform, then you can > >just use of_find_compatible_node(). I don't think it matters that > >different platforms have a different device here. It's not going to be > >a large table and you may need to know the differences if there's not > >an abstracted interface to it (seems there is in your case). > The GPIO irqchip would be in hierarchy with the wakeup-parent > irqchip and no device specific functions would be called directly. > We could achieve this with compatible strings to the irqchip. > > >Alternatively, if the PDC/MPM code knows what interrupt controller it > >is associated with, then it could setup that relationship and the > >interrupt controller code could retrieve that. Maybe the stacked > >domain support doesn't work in that direction (I haven't looked at the > >irq code much since that was added). > > > The PDC/MPM do not know about the association. Neither does the main interrupt controller. The association is part of SoC integration. You can describe that association in either direction and that is sufficient from a DT standpoint. You've probably picked putting this in the GIC(?) based on what works more easily with the Linux irqdomain code. > >However, my main concern is documenting something genericish in a > >device specific binding. It looks like Tegra is trying to add the same > >thing, so this needs to be documented in a common place. One question > >is whether wakeup is the only use or if this should be more generally > >a secondary interrupt parent? > > > Yes, wakeup is the only use of this interrupt parent. Maybe for you, but I was wondering about this more generally. Should we encode what the function (e.g. wakeup) is in the property name or have something like aux-interrupt-controller? Maybe some platforms have some need for a secondary interrupt-controller which is not wakeup. Routing interrupts to other cores perhaps? > It is powered by > an always-on rail and therefore can detect some interrupts that are > routed to it even when the GIC is powered off. Though Tegra's > implementation of the irqchip is a bit different from QCOM, the idea is > generally the same. It would be helpful, if we could make this a > generic enough binding. > > -- Lina >
Quoting Rob Herring (2019-01-09 11:36:56) > > > >However, my main concern is documenting something genericish in a > > >device specific binding. It looks like Tegra is trying to add the same > > >thing, so this needs to be documented in a common place. One question > > >is whether wakeup is the only use or if this should be more generally > > >a secondary interrupt parent? > > > > > Yes, wakeup is the only use of this interrupt parent. > > Maybe for you, but I was wondering about this more generally. Should > we encode what the function (e.g. wakeup) is in the property name or > have something like aux-interrupt-controller? Maybe some platforms > have some need for a secondary interrupt-controller which is not > wakeup. Routing interrupts to other cores perhaps? > I'd say it's not the interrupt-parent, but a secondary-interrupt-parent, because it's another path that some GPIO interrupts will go through vs. the "normal" summary irq line that uses the interrupt-parent. Maybe that's similar to the interrupt partitioning that ARM is doing for PPIs that only go to some CPUs? We don't really specify that some GPIO is corresponding to the secondary or primary interrupt controller for the GPIO controller in DT. If we did, then we could do something like the interrupt-map binding and have the index of that property be the gpio number and the interrupt parent that it maps to (either summary from the GIC or MPM pin number). interrupt-map = <0 0 &gic GIC_SPI 208 0>, <1 0 &pdc 3 0>; interrupt-map-mask = <0xfffffff 0>; And then we would pass the 2-cell GPIO interrupt specifier (gpio# and flags) through the table and remap it to arbitrary domain parents. We could use this same design for the SSBI and SPMI gpio interrupt controller where we're currently looking at hardcoding the base interrupt number in the driver (0xc0) and then adding the GPIO number to that to get the parent interrupt specifier. It's sort of an abuse of interrupt-map, but I don't know if it really matters because there isn't a child of the gpio controller that is going to go through this table.
Quoting Stephen Boyd (2019-01-11 15:20:48) > Quoting Rob Herring (2019-01-09 11:36:56) > > > > > >However, my main concern is documenting something genericish in a > > > >device specific binding. It looks like Tegra is trying to add the same > > > >thing, so this needs to be documented in a common place. One question > > > >is whether wakeup is the only use or if this should be more generally > > > >a secondary interrupt parent? > > > > > > > Yes, wakeup is the only use of this interrupt parent. > > > > Maybe for you, but I was wondering about this more generally. Should > > we encode what the function (e.g. wakeup) is in the property name or > > have something like aux-interrupt-controller? Maybe some platforms > > have some need for a secondary interrupt-controller which is not > > wakeup. Routing interrupts to other cores perhaps? > > > > I'd say it's not the interrupt-parent, but a secondary-interrupt-parent, > because it's another path that some GPIO interrupts will go through vs. > the "normal" summary irq line that uses the interrupt-parent. Maybe > that's similar to the interrupt partitioning that ARM is doing for PPIs > that only go to some CPUs? > > We don't really specify that some GPIO is corresponding to the secondary > or primary interrupt controller for the GPIO controller in DT. If we > did, then we could do something like the interrupt-map binding and have > the index of that property be the gpio number and the interrupt parent > that it maps to (either summary from the GIC or MPM pin number). > > interrupt-map = <0 0 &gic GIC_SPI 208 0>, > <1 0 &pdc 3 0>; > interrupt-map-mask = <0xfffffff 0>; > > And then we would pass the 2-cell GPIO interrupt specifier (gpio# and > flags) through the table and remap it to arbitrary domain parents. We > could use this same design for the SSBI and SPMI gpio interrupt > controller where we're currently looking at hardcoding the base > interrupt number in the driver (0xc0) and then adding the GPIO number to > that to get the parent interrupt specifier. > > It's sort of an abuse of interrupt-map, but I don't know if it really > matters because there isn't a child of the gpio controller that is going > to go through this table. > Rob, can you please respond?
Quoting Stephen Boyd (2019-01-23 12:52:09) > Quoting Stephen Boyd (2019-01-11 15:20:48) > > Quoting Rob Herring (2019-01-09 11:36:56) > > > > > > > >However, my main concern is documenting something genericish in a > > > > >device specific binding. It looks like Tegra is trying to add the same > > > > >thing, so this needs to be documented in a common place. One question > > > > >is whether wakeup is the only use or if this should be more generally > > > > >a secondary interrupt parent? > > > > > > > > > Yes, wakeup is the only use of this interrupt parent. > > > > > > Maybe for you, but I was wondering about this more generally. Should > > > we encode what the function (e.g. wakeup) is in the property name or > > > have something like aux-interrupt-controller? Maybe some platforms > > > have some need for a secondary interrupt-controller which is not > > > wakeup. Routing interrupts to other cores perhaps? > > > > > > > I'd say it's not the interrupt-parent, but a secondary-interrupt-parent, > > because it's another path that some GPIO interrupts will go through vs. > > the "normal" summary irq line that uses the interrupt-parent. Maybe > > that's similar to the interrupt partitioning that ARM is doing for PPIs > > that only go to some CPUs? > > > > We don't really specify that some GPIO is corresponding to the secondary > > or primary interrupt controller for the GPIO controller in DT. If we > > did, then we could do something like the interrupt-map binding and have > > the index of that property be the gpio number and the interrupt parent > > that it maps to (either summary from the GIC or MPM pin number). > > > > interrupt-map = <0 0 &gic GIC_SPI 208 0>, > > <1 0 &pdc 3 0>; > > interrupt-map-mask = <0xfffffff 0>; > > > > And then we would pass the 2-cell GPIO interrupt specifier (gpio# and > > flags) through the table and remap it to arbitrary domain parents. We > > could use this same design for the SSBI and SPMI gpio interrupt > > controller where we're currently looking at hardcoding the base > > interrupt number in the driver (0xc0) and then adding the GPIO number to > > that to get the parent interrupt specifier. > > > > It's sort of an abuse of interrupt-map, but I don't know if it really > > matters because there isn't a child of the gpio controller that is going > > to go through this table. > > > > Rob, can you please respond? > Thinking more about this it doesn't seem too beneficial to use the interrupt-map binding to figure out the parent domain. When we create the irqdomain in the gpio controller, we have to specify the parent domain at the same time, and there can only be one parent of an irqdomain. Furthermore, we can have many irqdomains per device node, and the DT binding doesn't indicate which irqdomain we want to parent to, just the device node for the parent. The nice part about using a DT binding to map the incoming irq specifier to the parent specifier is that we don't have to put that mapping somewhere in the driver. Instead, we can look it up in DT. This is especially helpful with these qcom devices where they seem to randomly assign gpios to PDC pins, and change it every time they make a new SoC. Having those data tables in the kernel is annoying to maintain, and having the child to parent hwirq numbers in DT isn't too great either when it's just a bunch of numbers: <0 208>, <1 3>, etc. It makes more sense when we at least have the parent phandle and can assume the incoming irq specifier is for the current node. <[#interrupt-cells] [phandle to parent to irq remap] [parents #interrupt-cells]> <0 0 &pdc 208 0>, <1 0 &pdc 3 0>, etc. But then, the parent phandle is always the same because a domain can't have more than one parent. In theory, we could also do simple trigger type inverting or collapsing if we wanted to while translating the irq specifier to the parent. Currently, drivers do that with some code to translate the specifier to a hwirq and flags and then overwrite the incoming flags from the child to be what the parent can accept. So should we make some new binding like 'irqdomain-<foo>-map' that maps the irq specifiers coming into the containing node's 'foo' irqdomain to a parent's irq specifier, instead of writing that in each irqchip driver? Or is this too verbose because each irq needs to be specified in the mapping table? 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.
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. 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. -----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 @@ -1213,6 +1213,83 @@ #interrupt-cells = <2>; gpio-ranges = <&tlmm 0 0 150>; wakeup-parent = <&pdc_intc>; + irqdomain-map = <1 0 &pdc_intc 30 0>, + <3 0 &pdc_intc 31 0>, + <5 0 &pdc_intc 32 0>, + <10 0 &pdc_intc 33 0>, + <11 0 &pdc_intc 34 0>, + <20 0 &pdc_intc 35 0>, + <22 0 &pdc_intc 36 0>, + <24 0 &pdc_intc 37 0>, + <26 0 &pdc_intc 38 0>, + <30 0 &pdc_intc 39 0>, + <31 0 &pdc_intc 117 0>, + <32 0 &pdc_intc 41 0>, + <34 0 &pdc_intc 42 0>, + <36 0 &pdc_intc 43 0>, + <37 0 &pdc_intc 44 0>, + <38 0 &pdc_intc 45 0>, + <39 0 &pdc_intc 46 0>, + <40 0 &pdc_intc 47 0>, + <41 0 &pdc_intc 115 0>, + <43 0 &pdc_intc 49 0>, + <44 0 &pdc_intc 50 0>, + <46 0 &pdc_intc 51 0>, + <48 0 &pdc_intc 52 0>, + <49 0 &pdc_intc 118 0>, + <52 0 &pdc_intc 54 0>, + <53 0 &pdc_intc 55 0>, + <54 0 &pdc_intc 56 0>, + <56 0 &pdc_intc 57 0>, + <57 0 &pdc_intc 58 0>, + <58 0 &pdc_intc 59 0>, + <59 0 &pdc_intc 60 0>, + <60 0 &pdc_intc 61 0>, + <61 0 &pdc_intc 62 0>, + <62 0 &pdc_intc 63 0>, + <63 0 &pdc_intc 64 0>, + <64 0 &pdc_intc 65 0>, + <66 0 &pdc_intc 66 0>, + <68 0 &pdc_intc 67 0>, + <71 0 &pdc_intc 68 0>, + <73 0 &pdc_intc 69 0>, + <77 0 &pdc_intc 70 0>, + <78 0 &pdc_intc 71 0>, + <79 0 &pdc_intc 72 0>, + <80 0 &pdc_intc 73 0>, + <84 0 &pdc_intc 74 0>, + <85 0 &pdc_intc 75 0>, + <86 0 &pdc_intc 76 0>, + <88 0 &pdc_intc 77 0>, + <89 0 &pdc_intc 116 0>, + <91 0 &pdc_intc 79 0>, + <92 0 &pdc_intc 80 0>, + <95 0 &pdc_intc 81 0>, + <96 0 &pdc_intc 82 0>, + <97 0 &pdc_intc 83 0>, + <101 0 &pdc_intc 84 0>, + <103 0 &pdc_intc 85 0>, + <104 0 &pdc_intc 86 0>, + <115 0 &pdc_intc 90 0>, + <116 0 &pdc_intc 91 0>, + <117 0 &pdc_intc 92 0>, + <118 0 &pdc_intc 93 0>, + <119 0 &pdc_intc 94 0>, + <120 0 &pdc_intc 95 0>, + <121 0 &pdc_intc 96 0>, + <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>; 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); +int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out) +{ + char *stem_name; + char *cells_name, *map_name = NULL, *mask_name = NULL; + char *pass_name = NULL; + struct device_node *cur, *new = NULL; + const __be32 *map, *mask, *pass; + static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 }; + static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 }; + __be32 initial_match_array[MAX_PHANDLE_ARGS]; + const __be32 *match_array = initial_match_array; + int i, ret, map_len, match; + u32 in_size, out_size; + + stem_name = ""; + cells_name = "#interrupt-cells"; + + ret = -ENOMEM; + map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name); + if (!map_name) + goto free; + + mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name); + if (!mask_name) + goto free; + + pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru", stem_name); + if (!pass_name) + goto free; + + /* Get the #interrupt-cells property */ + cur = to_of_node(in->fwnode); + ret = of_property_read_u32(cur, cells_name, &in_size); + if (ret < 0) + goto put; + + /* Precalculate the match array - this simplifies match loop */ + for (i = 0; i < in_size; i++) + initial_match_array[i] = cpu_to_be32(in->param[i]); + + ret = -EINVAL; + /* Get the irqdomain-map property */ + map = of_get_property(cur, map_name, &map_len); + if (!map) { + ret = 0; + goto free; + } + map_len /= sizeof(u32); + + /* Get the irqdomain-map-mask property (optional) */ + mask = of_get_property(cur, mask_name, NULL); + if (!mask) + mask = dummy_mask; + /* Iterate through irqdomain-map property */ + match = 0; + 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; + } + 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]; + val |= cpu_to_be32(out->param[i]) & pass[i]; + } + + out->param[i] = be32_to_cpu(val); + } + out->param_count = in_size = out_size; + out->fwnode = of_node_to_fwnode(new); + } +put: + of_node_put(new); +free: + kfree(mask_name); + kfree(map_name); + kfree(pass_name); + + return ret; +} +EXPORT_SYMBOL(of_irq_domain_map); + /** * of_irq_parse_one - Resolve an interrupt for a device * @device: the device whose interrupt is to be resolved diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 9b45219893bd..0473b180dc8d 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -1011,6 +1011,12 @@ static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq, if (!domain->parent) return 0; + ret = of_irq_domain_map(fwspec, &parent.fwspec); + if (ret) + return ret; + + pr_info("incoming is %d %#x and outgoing is %d %#x\n", fwspec->param[0], fwspec->param[1], parent.fwspec.param[0], parent.fwspec.param[1]); + parent.fwspec.fwnode = domain->parent->fwnode; parent.fwspec.param_count = 2; parent.fwspec.param[0] = hwirq; diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h index 1214cabb2247..80ba1e9be68d 100644 --- a/include/linux/of_irq.h +++ b/include/linux/of_irq.h @@ -32,6 +32,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index, } #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */ +extern int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out); + extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq); extern int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_args *out_irq);
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? >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. > >-----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 >@@ -1213,6 +1213,83 @@ > #interrupt-cells = <2>; > gpio-ranges = <&tlmm 0 0 150>; > wakeup-parent = <&pdc_intc>; >+ irqdomain-map = <1 0 &pdc_intc 30 0>, >+ <3 0 &pdc_intc 31 0>, >+ <5 0 &pdc_intc 32 0>, >+ <10 0 &pdc_intc 33 0>, >+ <11 0 &pdc_intc 34 0>, >+ <20 0 &pdc_intc 35 0>, >+ <22 0 &pdc_intc 36 0>, >+ <24 0 &pdc_intc 37 0>, >+ <26 0 &pdc_intc 38 0>, >+ <30 0 &pdc_intc 39 0>, >+ <31 0 &pdc_intc 117 0>, >+ <32 0 &pdc_intc 41 0>, >+ <34 0 &pdc_intc 42 0>, >+ <36 0 &pdc_intc 43 0>, >+ <37 0 &pdc_intc 44 0>, >+ <38 0 &pdc_intc 45 0>, >+ <39 0 &pdc_intc 46 0>, >+ <40 0 &pdc_intc 47 0>, >+ <41 0 &pdc_intc 115 0>, >+ <43 0 &pdc_intc 49 0>, >+ <44 0 &pdc_intc 50 0>, >+ <46 0 &pdc_intc 51 0>, >+ <48 0 &pdc_intc 52 0>, >+ <49 0 &pdc_intc 118 0>, >+ <52 0 &pdc_intc 54 0>, >+ <53 0 &pdc_intc 55 0>, >+ <54 0 &pdc_intc 56 0>, >+ <56 0 &pdc_intc 57 0>, >+ <57 0 &pdc_intc 58 0>, >+ <58 0 &pdc_intc 59 0>, >+ <59 0 &pdc_intc 60 0>, >+ <60 0 &pdc_intc 61 0>, >+ <61 0 &pdc_intc 62 0>, >+ <62 0 &pdc_intc 63 0>, >+ <63 0 &pdc_intc 64 0>, >+ <64 0 &pdc_intc 65 0>, >+ <66 0 &pdc_intc 66 0>, >+ <68 0 &pdc_intc 67 0>, >+ <71 0 &pdc_intc 68 0>, >+ <73 0 &pdc_intc 69 0>, >+ <77 0 &pdc_intc 70 0>, >+ <78 0 &pdc_intc 71 0>, >+ <79 0 &pdc_intc 72 0>, >+ <80 0 &pdc_intc 73 0>, >+ <84 0 &pdc_intc 74 0>, >+ <85 0 &pdc_intc 75 0>, >+ <86 0 &pdc_intc 76 0>, >+ <88 0 &pdc_intc 77 0>, >+ <89 0 &pdc_intc 116 0>, >+ <91 0 &pdc_intc 79 0>, >+ <92 0 &pdc_intc 80 0>, >+ <95 0 &pdc_intc 81 0>, >+ <96 0 &pdc_intc 82 0>, >+ <97 0 &pdc_intc 83 0>, >+ <101 0 &pdc_intc 84 0>, >+ <103 0 &pdc_intc 85 0>, >+ <104 0 &pdc_intc 86 0>, >+ <115 0 &pdc_intc 90 0>, >+ <116 0 &pdc_intc 91 0>, >+ <117 0 &pdc_intc 92 0>, >+ <118 0 &pdc_intc 93 0>, >+ <119 0 &pdc_intc 94 0>, >+ <120 0 &pdc_intc 95 0>, >+ <121 0 &pdc_intc 96 0>, >+ <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? > > 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? >+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out) >+{ >+ char *stem_name; >+ char *cells_name, *map_name = NULL, *mask_name = NULL; >+ char *pass_name = NULL; >+ struct device_node *cur, *new = NULL; >+ const __be32 *map, *mask, *pass; >+ static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 }; >+ static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 }; >+ __be32 initial_match_array[MAX_PHANDLE_ARGS]; >+ const __be32 *match_array = initial_match_array; >+ int i, ret, map_len, match; >+ u32 in_size, out_size; >+ >+ stem_name = ""; >+ cells_name = "#interrupt-cells"; >+ >+ ret = -ENOMEM; >+ map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name); >+ if (!map_name) >+ goto free; >+ >+ mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name); >+ if (!mask_name) >+ goto free; >+ >+ pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru", stem_name); >+ if (!pass_name) >+ goto free; >+ >+ /* Get the #interrupt-cells property */ >+ cur = to_of_node(in->fwnode); >+ ret = of_property_read_u32(cur, cells_name, &in_size); >+ if (ret < 0) >+ goto put; >+ >+ /* Precalculate the match array - this simplifies match loop */ >+ for (i = 0; i < in_size; i++) >+ initial_match_array[i] = cpu_to_be32(in->param[i]); >+ >+ ret = -EINVAL; >+ /* Get the irqdomain-map property */ >+ map = of_get_property(cur, map_name, &map_len); >+ if (!map) { >+ ret = 0; >+ goto free; >+ } >+ map_len /= sizeof(u32); >+ >+ /* Get the irqdomain-map-mask property (optional) */ >+ mask = of_get_property(cur, mask_name, NULL); >+ if (!mask) >+ mask = dummy_mask; >+ /* Iterate through irqdomain-map property */ >+ match = 0; >+ 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. >+ } >+ 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? >+ val |= cpu_to_be32(out->param[i]) & pass[i]; >+ } >+ >+ out->param[i] = be32_to_cpu(val); >+ } >+ out->param_count = in_size = out_size; >+ out->fwnode = of_node_to_fwnode(new); >+ } >+put: >+ of_node_put(new); >+free: >+ kfree(mask_name); >+ kfree(map_name); >+ kfree(pass_name); >+ >+ return ret; >+} >+EXPORT_SYMBOL(of_irq_domain_map); >+ > /** > * of_irq_parse_one - Resolve an interrupt for a device > * @device: the device whose interrupt is to be resolved >diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >index 9b45219893bd..0473b180dc8d 100644 >--- a/drivers/pinctrl/qcom/pinctrl-msm.c >+++ b/drivers/pinctrl/qcom/pinctrl-msm.c >@@ -1011,6 +1011,12 @@ static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq, > if (!domain->parent) > return 0; > >+ ret = of_irq_domain_map(fwspec, &parent.fwspec); >+ if (ret) >+ return ret; >+ >+ pr_info("incoming is %d %#x and outgoing is %d %#x\n", fwspec->param[0], fwspec->param[1], parent.fwspec.param[0], parent.fwspec.param[1]); >+ > parent.fwspec.fwnode = domain->parent->fwnode; > parent.fwspec.param_count = 2; > parent.fwspec.param[0] = hwirq; >diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h >index 1214cabb2247..80ba1e9be68d 100644 >--- a/include/linux/of_irq.h >+++ b/include/linux/of_irq.h >@@ -32,6 +32,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index, > } > #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */ > >+extern int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out); >+ > extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq); > extern int of_irq_parse_one(struct device_node *device, int index, > struct of_phandle_args *out_irq);
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]; > >+ } > >+
On Wed, Jan 09 2019 at 12:37 -0700, Rob Herring wrote: >On Wed, Jan 9, 2019 at 11:31 AM Lina Iyer <ilina@codeaurora.org> wrote: >> >> On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote: >> >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> >> >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote: >> >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote: >> >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO >> >> >> routed to the PDC as interrupts that can be used to wake the system up >> >> >> from deep low power modes and suspend. >> >> >> >> >> >> Cc: devicetree@vger.kernel.org >> >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> >> >> --- >> >> >> .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- >> >> >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> >> index 665aadb5ea28..a522ca46667d 100644 >> >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt >> >> >> @@ -29,6 +29,11 @@ SDM845 platform. >> >> >> Definition: must be 2. Specifying the pin number and flags, as defined >> >> >> in <dt-bindings/interrupt-controller/irq.h> >> >> >> >> >> >> +- wakeup-parent: >> >> >> + Usage: optional >> >> >> + Value type: <phandle> >> >> >> + Definition: A phandle to the wakeup interrupt controller for the SoC. >> >> > >> >> >Is this really necessary? Is there more than one possible wakeup-parent >> >> >node? >> >> > >> >> No. There is only one but depending on the architecture, the wakeup >> >> interrupt controller could be different device like PDC on SDM845 or MPM >> >> on SDM820. >> >> >> >> What do you have in mind? Let me know if you have a better idea than >> >> referencing in DT. >> > >> >If there's only one possibility for a given platform, then you can >> >just use of_find_compatible_node(). I don't think it matters that >> >different platforms have a different device here. It's not going to be >> >a large table and you may need to know the differences if there's not >> >an abstracted interface to it (seems there is in your case). >> The GPIO irqchip would be in hierarchy with the wakeup-parent >> irqchip and no device specific functions would be called directly. >> We could achieve this with compatible strings to the irqchip. >> >> >Alternatively, if the PDC/MPM code knows what interrupt controller it >> >is associated with, then it could setup that relationship and the >> >interrupt controller code could retrieve that. Maybe the stacked >> >domain support doesn't work in that direction (I haven't looked at the >> >irq code much since that was added). >> > >> The PDC/MPM do not know about the association. > >Neither does the main interrupt controller. The association is part of >SoC integration. You can describe that association in either direction >and that is sufficient from a DT standpoint. You've probably picked >putting this in the GIC(?) based on what works more easily with the >Linux irqdomain code. > >> >However, my main concern is documenting something genericish in a >> >device specific binding. It looks like Tegra is trying to add the same >> >thing, so this needs to be documented in a common place. One question >> >is whether wakeup is the only use or if this should be more generally >> >a secondary interrupt parent? >> > >> Yes, wakeup is the only use of this interrupt parent. > >Maybe for you, but I was wondering about this more generally. Should >we encode what the function (e.g. wakeup) is in the property name or >have something like aux-interrupt-controller? Maybe some platforms >have some need for a secondary interrupt-controller which is not >wakeup. Routing interrupts to other cores perhaps? > Rob, Would like to know your opinion on Stephen's idea. Could you take a look at this thread again please? Thanks, Lina >> It is powered by >> an always-on rail and therefore can detect some interrupts that are >> routed to it even when the GIC is powered off. Though Tegra's >> implementation of the irqchip is a bit different from QCOM, the idea is >> generally the same. It would be helpful, if we could make this a >> generic enough binding. >> >> -- Lina >>
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt index 665aadb5ea28..a522ca46667d 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt @@ -29,6 +29,11 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h> +- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + - gpio-controller: Usage: required Value type: <none> @@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the mux function to select on those pin(s)/group(s), and various pin configuration parameters, such as pull-up, drive strength, etc. - PIN CONFIGURATION NODES: The name of each subnode is not important; all subnodes should be enumerated @@ -160,6 +164,7 @@ Example: #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; + wakeup-parent = <&pdc>; qup9_active: qup9-active { mux {
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend. Cc: devicetree@vger.kernel.org Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)