This patch series adds hierarchical IRQ chip support to ssbi-gpio so
that device tree consumers can request an IRQ directly from the GPIO
block rather than having to request an IRQ from the underlying PMIC.
For more background information, see the email thread with Linus
Walleij's excellent description of the problem at
https://www.spinics.net/lists/linux-gpio/msg34655.html.
These changes were tested by Linus Walleij on an APQ8060 DragonBoard.
See the notes on the individual patches for the changelog.
Brian Masney (9):
pinctrl: qcom: ssbi-gpio: hardcode IRQ counts
genirq: introduce irq_domain_translate_twocell
mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ
chips
mfd: pm8xxx: disassociate old virq if hwirq mapping already exists
qcom: ssbi-gpio: add support for hierarchical IRQ chip
arm: dts: qcom: apq8064: add interrupt controller properties
arm: dts: qcom: msm8660: add interrupt controller properties
arm: dts: qcom: mdm9615: add interrupt controller properties
mfd: pm8xxx: revert "disassociate old virq if hwirq mapping already
exists"
Linus Walleij (2):
genirq: introduce irq_chip_mask_ack_parent()
ARM: dts: qcom-apq8060: Fix up interrupt parents
.../arm/boot/dts/qcom-apq8060-dragonboard.dts | 21 +--
arch/arm/boot/dts/qcom-apq8064.dtsi | 46 +-----
arch/arm/boot/dts/qcom-mdm9615.dtsi | 9 +-
arch/arm/boot/dts/qcom-msm8660.dtsi | 47 +-----
drivers/mfd/qcom-pm8xxx.c | 75 ++++-----
drivers/pinctrl/qcom/Kconfig | 1 +
drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 156 +++++++++++++++---
include/linux/irq.h | 1 +
include/linux/irqdomain.h | 5 +
kernel/irq/chip.c | 11 ++
kernel/irq/irqdomain.c | 45 +++--
11 files changed, 234 insertions(+), 183 deletions(-)
--
2.17.2
Now that ssbi-gpio is a proper hierarchical IRQ chip, and all in-tree
users of device tree have been updated, we can now drop the hack that
was introduced to disassociate the old Linux virq if a hwirq mapping
already exists. That patch was introduced to not break git bisect for
any existing boards.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- None
drivers/mfd/qcom-pm8xxx.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index 2f99a98ccee5..8eb2528793f9 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -380,12 +380,6 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
struct irq_domain *domain, unsigned int irq,
irq_hw_number_t hwirq, unsigned int type)
{
- unsigned int old_virq;
-
- old_virq = irq_find_mapping(domain, hwirq);
- if (old_virq)
- irq_domain_disassociate(domain, old_virq);
-
irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
chip, handle_level_irq, NULL, NULL);
irq_set_noprobe(irq);
--
2.17.2
Check to see if the hwirq is already associated with another virq on
this IRQ domain. If so, then disassociate it before associating the
hwirq with the new virq.
This is a temporary hack that is needed in order to not break git
bisect for existing boards. The next patch in this series converts
ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
to update all of the device tree files, and finally this patch will be
reverted within the same patch series.
IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
this driver is probed due to the interrupts property in device tree.
Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
and that will use the new IRQ chip code in ssbi-gpio that sets up the
IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
virqs and interrupts will not work as expected. This patch corrects
that issue.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- None
drivers/mfd/qcom-pm8xxx.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index 8eb2528793f9..2f99a98ccee5 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
struct irq_domain *domain, unsigned int irq,
irq_hw_number_t hwirq, unsigned int type)
{
+ unsigned int old_virq;
+
+ old_virq = irq_find_mapping(domain, hwirq);
+ if (old_virq)
+ irq_domain_disassociate(domain, old_virq);
+
irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
chip, handle_level_irq, NULL, NULL);
irq_set_noprobe(irq);
--
2.17.2
Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.
Note that the IRQs started at 24 instead of 192 like all of the other
PMICs. This is the same IRQs as the MPP for this board. qcom-pm8xxx.c
doesn't set the shared IRQs so this is highly likely to be a copy and
paste error.
Signed-off-by: Brian Masney <[email protected]>
---
Changes since v1:
- None
arch/arm/boot/dts/qcom-mdm9615.dtsi | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-mdm9615.dtsi b/arch/arm/boot/dts/qcom-mdm9615.dtsi
index e49f67ad5dbc..02afc6a42005 100644
--- a/arch/arm/boot/dts/qcom-mdm9615.dtsi
+++ b/arch/arm/boot/dts/qcom-mdm9615.dtsi
@@ -323,13 +323,8 @@
pmicgpio: gpio@150 {
compatible = "qcom,pm8018-gpio", "qcom,ssbi-gpio";
- interrupt-parent = <&pmicintc>;
- interrupts = <24 IRQ_TYPE_NONE>,
- <25 IRQ_TYPE_NONE>,
- <26 IRQ_TYPE_NONE>,
- <27 IRQ_TYPE_NONE>,
- <28 IRQ_TYPE_NONE>,
- <29 IRQ_TYPE_NONE>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
gpio-controller;
#gpio-cells = <2>;
};
--
2.17.2
From: Linus Walleij <[email protected]>
Before we fixed up the interrupt hierarchy for the SSBI
GPIO controller, we had to use the PM8058 directly to pick
interrupts. After making the interrupt controller work properly,
we can reference the real interrupt parent.
Signed-off-by: Linus Walleij <[email protected]>
Signed-off-by: Brian Masney <[email protected]>
---
This is a new patch introduced in v2.
.../arm/boot/dts/qcom-apq8060-dragonboard.dts | 21 +++++++------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-apq8060-dragonboard.dts b/arch/arm/boot/dts/qcom-apq8060-dragonboard.dts
index 497bb065eb9d..4e6c50d45cb2 100644
--- a/arch/arm/boot/dts/qcom-apq8060-dragonboard.dts
+++ b/arch/arm/boot/dts/qcom-apq8060-dragonboard.dts
@@ -93,9 +93,8 @@
vdd-supply = <&pm8058_l14>; // 2.85V
aset-gpios = <&pm8058_gpio 35 GPIO_ACTIVE_LOW>;
capella,aset-resistance-ohms = <100000>;
- /* GPIO34 has interrupt 225 on the PM8058 */
/* Trig on both edges - getting close or far away */
- interrupts-extended = <&pm8058 225 IRQ_TYPE_EDGE_BOTH>;
+ interrupts-extended = <&pm8058_gpio 34 IRQ_TYPE_EDGE_BOTH>;
/* MPP05 analog input to the XOADC */
io-channels = <&xoadc 0x00 0x05>;
io-channel-names = "aout";
@@ -515,9 +514,8 @@
ak8975@c {
compatible = "asahi-kasei,ak8975";
reg = <0x0c>;
- /* FIXME: GPIO33 has interrupt 224 on the PM8058 */
- interrupt-parent = <&pm8058>;
- interrupts = <224 IRQ_TYPE_EDGE_RISING>;
+ interrupt-parent = <&pm8058_gpio>;
+ interrupts = <33 IRQ_TYPE_EDGE_RISING>;
pinctrl-names = "default";
pinctrl-0 = <&dragon_ak8975_gpios>;
vid-supply = <&pm8058_lvs0>; // 1.8V
@@ -526,9 +524,8 @@
bmp085@77 {
compatible = "bosch,bmp085";
reg = <0x77>;
- /* FIXME: GPIO16 has interrupt 207 on the PM8058 */
- interrupt-parent = <&pm8058>;
- interrupts = <207 IRQ_TYPE_EDGE_RISING>;
+ interrupt-parent = <&pm8058_gpio>;
+ interrupts = <16 IRQ_TYPE_EDGE_RISING>;
reset-gpios = <&tlmm 86 GPIO_ACTIVE_LOW>;
pinctrl-names = "default";
pinctrl-0 = <&dragon_bmp085_gpios>;
@@ -539,12 +536,11 @@
compatible = "invensense,mpu3050";
reg = <0x68>;
/*
- * GPIO17 has interrupt 208 on the
- * PM8058, it is pulled high by a 10k
+ * GPIO17 is pulled high by a 10k
* resistor to VLOGIC so needs to be
* active low/falling edge.
*/
- interrupts-extended = <&pm8058 208 IRQ_TYPE_EDGE_FALLING>;
+ interrupts-extended = <&pm8058_gpio 17 IRQ_TYPE_EDGE_FALLING>;
pinctrl-names = "default";
pinctrl-0 = <&dragon_mpu3050_gpios>;
vlogic-supply = <&pm8058_lvs0>; // 1.8V
@@ -589,11 +585,10 @@
compatible = "smsc,lan9221", "smsc,lan9115";
reg = <2 0x0 0x100>;
/*
- * GPIO7 has interrupt 198 on the PM8058
* The second interrupt is the PME interrupt
* for network wakeup, connected to the TLMM.
*/
- interrupts-extended = <&pm8058 198 IRQ_TYPE_EDGE_FALLING>,
+ interrupts-extended = <&pm8058_gpio 7 IRQ_TYPE_EDGE_FALLING>,
<&tlmm 29 IRQ_TYPE_EDGE_RISING>;
reset-gpios = <&tlmm 30 GPIO_ACTIVE_LOW>;
vdd33a-supply = <&dragon_veth>;
--
2.17.2
Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order
to support hierarchical IRQ chips. This is necessary so that ssbi-gpio
can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ
chips in device tree should be usable from the start without having to
make an additional call to gpio[d]_to_irq() to get the proper IRQ on the
parent.
pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this
patch since the irq_chip is now contained in the pm_irq_data struct, and
that allows us to use a common IRQ mapping function.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- Put u8 config[0] at end along with a comment
- Hardcode the IRQ flow handler as handle_level_irq like the original
code.
- Don't set IRQ handler in pm8xxx_irq_set_type() since this is already
done in pm8xxx_irq_domain_map(). Linus: This is different than what
you tested but this *should* be fine.
drivers/mfd/qcom-pm8xxx.c | 75 ++++++++++++++++++++-------------------
1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index e6e8d81c15fd..8eb2528793f9 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -70,22 +70,23 @@
#define PM8XXX_NR_IRQS 256
#define PM8821_NR_IRQS 112
+struct pm_irq_data {
+ int num_irqs;
+ struct irq_chip *irq_chip;
+ void (*irq_handler)(struct irq_desc *desc);
+};
+
struct pm_irq_chip {
struct regmap *regmap;
spinlock_t pm_irq_lock;
struct irq_domain *irqdomain;
- unsigned int num_irqs;
unsigned int num_blocks;
unsigned int num_masters;
+ const struct pm_irq_data *pm_irq_data;
+ /* MUST BE AT THE END OF THIS STRUCT */
u8 config[0];
};
-struct pm_irq_data {
- int num_irqs;
- const struct irq_domain_ops *irq_domain_ops;
- void (*irq_handler)(struct irq_desc *desc);
-};
-
static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
unsigned int *ip)
{
@@ -375,21 +376,38 @@ static struct irq_chip pm8xxx_irq_chip = {
.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
};
-static int pm8xxx_irq_domain_map(struct irq_domain *d, unsigned int irq,
- irq_hw_number_t hwirq)
+static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
+ struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq, unsigned int type)
{
- struct pm_irq_chip *chip = d->host_data;
-
- irq_set_chip_and_handler(irq, &pm8xxx_irq_chip, handle_level_irq);
- irq_set_chip_data(irq, chip);
+ irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
+ chip, handle_level_irq, NULL, NULL);
irq_set_noprobe(irq);
+}
+
+static int pm8xxx_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct pm_irq_chip *chip = domain->host_data;
+ struct irq_fwspec *fwspec = data;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret, i;
+
+ ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < nr_irqs; i++)
+ pm8xxx_irq_domain_map(chip, domain, virq + i, hwirq + i, type);
return 0;
}
static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
- .xlate = irq_domain_xlate_twocell,
- .map = pm8xxx_irq_domain_map,
+ .alloc = pm8xxx_irq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+ .translate = irq_domain_translate_twocell,
};
static void pm8821_irq_mask_ack(struct irq_data *d)
@@ -473,23 +491,6 @@ static struct irq_chip pm8821_irq_chip = {
.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
};
-static int pm8821_irq_domain_map(struct irq_domain *d, unsigned int irq,
- irq_hw_number_t hwirq)
-{
- struct pm_irq_chip *chip = d->host_data;
-
- irq_set_chip_and_handler(irq, &pm8821_irq_chip, handle_level_irq);
- irq_set_chip_data(irq, chip);
- irq_set_noprobe(irq);
-
- return 0;
-}
-
-static const struct irq_domain_ops pm8821_irq_domain_ops = {
- .xlate = irq_domain_xlate_twocell,
- .map = pm8821_irq_domain_map,
-};
-
static const struct regmap_config ssbi_regmap_config = {
.reg_bits = 16,
.val_bits = 8,
@@ -501,13 +502,13 @@ static const struct regmap_config ssbi_regmap_config = {
static const struct pm_irq_data pm8xxx_data = {
.num_irqs = PM8XXX_NR_IRQS,
- .irq_domain_ops = &pm8xxx_irq_domain_ops,
+ .irq_chip = &pm8xxx_irq_chip,
.irq_handler = pm8xxx_irq_handler,
};
static const struct pm_irq_data pm8821_data = {
.num_irqs = PM8821_NR_IRQS,
- .irq_domain_ops = &pm8821_irq_domain_ops,
+ .irq_chip = &pm8821_irq_chip,
.irq_handler = pm8821_irq_handler,
};
@@ -571,14 +572,14 @@ static int pm8xxx_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, chip);
chip->regmap = regmap;
- chip->num_irqs = data->num_irqs;
- chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
+ chip->num_blocks = DIV_ROUND_UP(data->num_irqs, 8);
chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
+ chip->pm_irq_data = data;
spin_lock_init(&chip->pm_irq_lock);
chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
data->num_irqs,
- data->irq_domain_ops,
+ &pm8xxx_irq_domain_ops,
chip);
if (!chip->irqdomain)
return -ENODEV;
--
2.17.2
Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.
This change was not tested on any hardware but the same change was
tested on an APQ8060 DragonBoard with no issues.
Signed-off-by: Brian Masney <[email protected]>
---
Changes since v1:
- None
arch/arm/boot/dts/qcom-apq8064.dtsi | 46 ++---------------------------
1 file changed, 2 insertions(+), 44 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 1374c2e52c20..bd6907db615b 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -711,50 +711,8 @@
compatible = "qcom,pm8921-gpio",
"qcom,ssbi-gpio";
reg = <0x150>;
- interrupts = <192 IRQ_TYPE_NONE>,
- <193 IRQ_TYPE_NONE>,
- <194 IRQ_TYPE_NONE>,
- <195 IRQ_TYPE_NONE>,
- <196 IRQ_TYPE_NONE>,
- <197 IRQ_TYPE_NONE>,
- <198 IRQ_TYPE_NONE>,
- <199 IRQ_TYPE_NONE>,
- <200 IRQ_TYPE_NONE>,
- <201 IRQ_TYPE_NONE>,
- <202 IRQ_TYPE_NONE>,
- <203 IRQ_TYPE_NONE>,
- <204 IRQ_TYPE_NONE>,
- <205 IRQ_TYPE_NONE>,
- <206 IRQ_TYPE_NONE>,
- <207 IRQ_TYPE_NONE>,
- <208 IRQ_TYPE_NONE>,
- <209 IRQ_TYPE_NONE>,
- <210 IRQ_TYPE_NONE>,
- <211 IRQ_TYPE_NONE>,
- <212 IRQ_TYPE_NONE>,
- <213 IRQ_TYPE_NONE>,
- <214 IRQ_TYPE_NONE>,
- <215 IRQ_TYPE_NONE>,
- <216 IRQ_TYPE_NONE>,
- <217 IRQ_TYPE_NONE>,
- <218 IRQ_TYPE_NONE>,
- <219 IRQ_TYPE_NONE>,
- <220 IRQ_TYPE_NONE>,
- <221 IRQ_TYPE_NONE>,
- <222 IRQ_TYPE_NONE>,
- <223 IRQ_TYPE_NONE>,
- <224 IRQ_TYPE_NONE>,
- <225 IRQ_TYPE_NONE>,
- <226 IRQ_TYPE_NONE>,
- <227 IRQ_TYPE_NONE>,
- <228 IRQ_TYPE_NONE>,
- <229 IRQ_TYPE_NONE>,
- <230 IRQ_TYPE_NONE>,
- <231 IRQ_TYPE_NONE>,
- <232 IRQ_TYPE_NONE>,
- <233 IRQ_TYPE_NONE>,
- <234 IRQ_TYPE_NONE>,
- <235 IRQ_TYPE_NONE>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
gpio-controller;
#gpio-cells = <2>;
--
2.17.2
ssbi-gpio did not have any irqchip support so consumers of this in
device tree would need to call gpio[d]_to_irq() in order to get the
proper IRQ on the underlying PMIC. IRQ chips in device tree should
be usable from the start without the consumer having to make an
additional call to get the proper IRQ on the parent. This patch adds
hierarchical IRQ chip support to the ssbi-gpio code to correct this
issue.
The constant PM8XXX_GPIO_PHYSICAL_OFFSET is introduced to replace the
hardcoded '1' that previously existed in two places in this driver to
improve code readability.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- Change irq_ack / irq_mask with irq_mask_ack to match what is on the
parent interrupt controller.
- Select IRQ_DOMAIN_HIERARCHY in Kconfig
- Hardcode the IRQ handler as handle_level_irq since this is what the
parent expects. Linus: This is different than what you tested but this
*should* be fine.
drivers/pinctrl/qcom/Kconfig | 1 +
drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 135 +++++++++++++++++++++--
2 files changed, 124 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index 93efbffcd3a4..2e66ab72c10b 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -150,6 +150,7 @@ config PINCTRL_QCOM_SSBI_PMIC
select PINMUX
select PINCONF
select GENERIC_PINCONF
+ select IRQ_DOMAIN_HIERARCHY
help
This is the pinctrl, pinmux, pinconf and gpiolib driver for the
Qualcomm GPIO and MPP blocks found in the Qualcomm PMIC's chips,
diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index 23641e4d4521..84a232450000 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -55,6 +55,8 @@
#define PM8XXX_MAX_GPIOS 44
+#define PM8XXX_GPIO_PHYSICAL_OFFSET 1
+
/* custom pinconf parameters */
#define PM8XXX_QCOM_DRIVE_STRENGH (PIN_CONFIG_END + 1)
#define PM8XXX_QCOM_PULL_UP_STRENGTH (PIN_CONFIG_END + 2)
@@ -99,6 +101,9 @@ struct pm8xxx_gpio {
struct pinctrl_desc desc;
unsigned npins;
+
+ struct fwnode_handle *fwnode;
+ struct irq_domain *domain;
};
static const struct pinconf_generic_params pm8xxx_gpio_bindings[] = {
@@ -499,11 +504,12 @@ static int pm8xxx_gpio_get(struct gpio_chip *chip, unsigned offset)
if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
ret = pin->output_value;
- } else {
+ } else if (pin->irq >= 0) {
ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
if (!ret)
ret = !!state;
- }
+ } else
+ ret = -EINVAL;
return ret;
}
@@ -533,16 +539,39 @@ static int pm8xxx_gpio_of_xlate(struct gpio_chip *chip,
if (flags)
*flags = gpio_desc->args[1];
- return gpio_desc->args[0] - 1;
+ return gpio_desc->args[0] - PM8XXX_GPIO_PHYSICAL_OFFSET;
}
static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ struct pm8xxx_gpio *pctrl = gpiochip_get_data(chip);
+ struct pm8xxx_pin_data *pin = pctrl->desc.pins[offset].drv_data;
+ struct irq_fwspec fwspec;
+ int ret;
+
+ fwspec.fwnode = pctrl->fwnode;
+ fwspec.param_count = 2;
+ fwspec.param[0] = offset + PM8XXX_GPIO_PHYSICAL_OFFSET;
+ fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+
+ ret = irq_create_fwspec_mapping(&fwspec);
+
+ /*
+ * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
+ * line level.
+ */
+ pin->irq = ret;
+
+ return ret;
+}
+
+static void pm8xxx_gpio_free(struct gpio_chip *chip, unsigned int offset)
{
struct pm8xxx_gpio *pctrl = gpiochip_get_data(chip);
struct pm8xxx_pin_data *pin = pctrl->desc.pins[offset].drv_data;
- return pin->irq;
+ pin->irq = -1;
}
#ifdef CONFIG_DEBUG_FS
@@ -571,7 +600,7 @@ static void pm8xxx_gpio_dbg_show_one(struct seq_file *s,
"no", "high", "medium", "low"
};
- seq_printf(s, " gpio%-2d:", offset + 1);
+ seq_printf(s, " gpio%-2d:", offset + PM8XXX_GPIO_PHYSICAL_OFFSET);
if (pin->disable) {
seq_puts(s, " ---");
} else {
@@ -603,6 +632,7 @@ static void pm8xxx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
#endif
static const struct gpio_chip pm8xxx_gpio_template = {
+ .free = pm8xxx_gpio_free,
.direction_input = pm8xxx_gpio_direction_input,
.direction_output = pm8xxx_gpio_direction_output,
.get = pm8xxx_gpio_get,
@@ -664,6 +694,68 @@ static int pm8xxx_pin_populate(struct pm8xxx_gpio *pctrl,
return 0;
}
+static struct irq_chip pm8xxx_irq_chip = {
+ .name = "ssbi-gpio",
+ .irq_mask_ack = irq_chip_mask_ack_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_set_type = irq_chip_set_type_parent,
+ .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int pm8xxx_domain_translate(struct irq_domain *domain,
+ struct irq_fwspec *fwspec,
+ unsigned long *hwirq,
+ unsigned int *type)
+{
+ struct pm8xxx_gpio *pctrl = container_of(domain->host_data,
+ struct pm8xxx_gpio, chip);
+
+ if (fwspec->param_count != 2 || fwspec->param[0] >= pctrl->chip.ngpio)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[0] - PM8XXX_GPIO_PHYSICAL_OFFSET;
+ *type = fwspec->param[1];
+
+ return 0;
+}
+
+static int pm8xxx_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct pm8xxx_gpio *pctrl = container_of(domain->host_data,
+ struct pm8xxx_gpio, chip);
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+ int ret, i;
+
+ ret = pm8xxx_domain_translate(domain, fwspec, &hwirq, &type);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_info(domain, virq + i, hwirq + i,
+ &pm8xxx_irq_chip, pctrl, handle_level_irq,
+ NULL, NULL);
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 2;
+ parent_fwspec.param[0] = hwirq + 0xc0;
+ parent_fwspec.param[1] = fwspec->param[1];
+
+ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+ &parent_fwspec);
+}
+
+static const struct irq_domain_ops pm8xxx_domain_ops = {
+ .activate = gpiochip_irq_domain_activate,
+ .alloc = pm8xxx_domain_alloc,
+ .deactivate = gpiochip_irq_domain_deactivate,
+ .free = irq_domain_free_irqs_common,
+ .translate = pm8xxx_domain_translate,
+};
+
static const struct of_device_id pm8xxx_gpio_of_match[] = {
{ .compatible = "qcom,pm8018-gpio", .data = (void *) 6 },
{ .compatible = "qcom,pm8038-gpio", .data = (void *) 12 },
@@ -677,6 +769,8 @@ MODULE_DEVICE_TABLE(of, pm8xxx_gpio_of_match);
static int pm8xxx_gpio_probe(struct platform_device *pdev)
{
struct pm8xxx_pin_data *pin_data;
+ struct irq_domain *parent_domain;
+ struct device_node *parent_node;
struct pinctrl_pin_desc *pins;
struct pm8xxx_gpio *pctrl;
int ret, i;
@@ -713,12 +807,7 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
for (i = 0; i < pctrl->desc.npins; i++) {
pin_data[i].reg = SSBI_REG_ADDR_GPIO(i);
- pin_data[i].irq = platform_get_irq(pdev, i);
- if (pin_data[i].irq < 0) {
- dev_err(&pdev->dev,
- "missing interrupts for pin %d\n", i);
- return pin_data[i].irq;
- }
+ pin_data[i].irq = -1;
ret = pm8xxx_pin_populate(pctrl, &pin_data[i]);
if (ret)
@@ -749,10 +838,29 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
pctrl->chip.of_gpio_n_cells = 2;
pctrl->chip.label = dev_name(pctrl->dev);
pctrl->chip.ngpio = pctrl->npins;
+
+ parent_node = of_irq_find_parent(pctrl->dev->of_node);
+ if (!parent_node)
+ return -ENXIO;
+
+ parent_domain = irq_find_host(parent_node);
+ of_node_put(parent_node);
+ if (!parent_domain)
+ return -ENXIO;
+
+ pctrl->fwnode = of_node_to_fwnode(pctrl->dev->of_node);
+ pctrl->domain = irq_domain_create_hierarchy(parent_domain, 0,
+ pctrl->chip.ngpio,
+ pctrl->fwnode,
+ &pm8xxx_domain_ops,
+ &pctrl->chip);
+ if (!pctrl->domain)
+ return -ENODEV;
+
ret = gpiochip_add_data(&pctrl->chip, pctrl);
if (ret) {
dev_err(&pdev->dev, "failed register gpiochip\n");
- return ret;
+ goto err_chip_add_data;
}
/*
@@ -782,6 +890,8 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
unregister_gpiochip:
gpiochip_remove(&pctrl->chip);
+err_chip_add_data:
+ irq_domain_remove(pctrl->domain);
return ret;
}
@@ -791,6 +901,7 @@ static int pm8xxx_gpio_remove(struct platform_device *pdev)
struct pm8xxx_gpio *pctrl = platform_get_drvdata(pdev);
gpiochip_remove(&pctrl->chip);
+ irq_domain_remove(pctrl->domain);
return 0;
}
--
2.17.2
Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- None
arch/arm/boot/dts/qcom-msm8660.dtsi | 47 ++---------------------------
1 file changed, 2 insertions(+), 45 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-msm8660.dtsi b/arch/arm/boot/dts/qcom-msm8660.dtsi
index 993107ed1476..65a994f0e09b 100644
--- a/arch/arm/boot/dts/qcom-msm8660.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8660.dtsi
@@ -292,51 +292,8 @@
compatible = "qcom,pm8058-gpio",
"qcom,ssbi-gpio";
reg = <0x150>;
- interrupt-parent = <&pm8058>;
- interrupts = <192 IRQ_TYPE_NONE>,
- <193 IRQ_TYPE_NONE>,
- <194 IRQ_TYPE_NONE>,
- <195 IRQ_TYPE_NONE>,
- <196 IRQ_TYPE_NONE>,
- <197 IRQ_TYPE_NONE>,
- <198 IRQ_TYPE_NONE>,
- <199 IRQ_TYPE_NONE>,
- <200 IRQ_TYPE_NONE>,
- <201 IRQ_TYPE_NONE>,
- <202 IRQ_TYPE_NONE>,
- <203 IRQ_TYPE_NONE>,
- <204 IRQ_TYPE_NONE>,
- <205 IRQ_TYPE_NONE>,
- <206 IRQ_TYPE_NONE>,
- <207 IRQ_TYPE_NONE>,
- <208 IRQ_TYPE_NONE>,
- <209 IRQ_TYPE_NONE>,
- <210 IRQ_TYPE_NONE>,
- <211 IRQ_TYPE_NONE>,
- <212 IRQ_TYPE_NONE>,
- <213 IRQ_TYPE_NONE>,
- <214 IRQ_TYPE_NONE>,
- <215 IRQ_TYPE_NONE>,
- <216 IRQ_TYPE_NONE>,
- <217 IRQ_TYPE_NONE>,
- <218 IRQ_TYPE_NONE>,
- <219 IRQ_TYPE_NONE>,
- <220 IRQ_TYPE_NONE>,
- <221 IRQ_TYPE_NONE>,
- <222 IRQ_TYPE_NONE>,
- <223 IRQ_TYPE_NONE>,
- <224 IRQ_TYPE_NONE>,
- <225 IRQ_TYPE_NONE>,
- <226 IRQ_TYPE_NONE>,
- <227 IRQ_TYPE_NONE>,
- <228 IRQ_TYPE_NONE>,
- <229 IRQ_TYPE_NONE>,
- <230 IRQ_TYPE_NONE>,
- <231 IRQ_TYPE_NONE>,
- <232 IRQ_TYPE_NONE>,
- <233 IRQ_TYPE_NONE>,
- <234 IRQ_TYPE_NONE>,
- <235 IRQ_TYPE_NONE>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
gpio-controller;
#gpio-cells = <2>;
--
2.17.2
The probing of this driver calls platform_irq_count, which will
setup all of the IRQs that are configured in device tree. In
preparation for converting this driver to be a hierarchical IRQ
chip, hardcode the IRQ count based on the hardware type so that all
the IRQs are not configured immediately and are configured on an
as-needed basis later in the boot process. This change will also
allow for the removal of the interrupts property later in this
patch series once the hierarchical IRQ chip support is in.
This patch also removes the generic qcom,ssbi-gpio OF match since we
don't know the number of pins. All of the existing upstream bindings
already include the more-specific binding.
This change was tested on an APQ8060 DragonBoard.
Signed-off-by: Brian Masney <[email protected]>
Tested-by: Linus Walleij <[email protected]>
---
Changes since v1:
- Added pctrl->dev back that was mistakenly removed
- Removed npins local variable from _probe(); populate pctrl->npins
directly.
drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index ded7d765af2e..23641e4d4521 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -665,12 +665,11 @@ static int pm8xxx_pin_populate(struct pm8xxx_gpio *pctrl,
}
static const struct of_device_id pm8xxx_gpio_of_match[] = {
- { .compatible = "qcom,pm8018-gpio" },
- { .compatible = "qcom,pm8038-gpio" },
- { .compatible = "qcom,pm8058-gpio" },
- { .compatible = "qcom,pm8917-gpio" },
- { .compatible = "qcom,pm8921-gpio" },
- { .compatible = "qcom,ssbi-gpio" },
+ { .compatible = "qcom,pm8018-gpio", .data = (void *) 6 },
+ { .compatible = "qcom,pm8038-gpio", .data = (void *) 12 },
+ { .compatible = "qcom,pm8058-gpio", .data = (void *) 44 },
+ { .compatible = "qcom,pm8917-gpio", .data = (void *) 38 },
+ { .compatible = "qcom,pm8921-gpio", .data = (void *) 44 },
{ },
};
MODULE_DEVICE_TABLE(of, pm8xxx_gpio_of_match);
@@ -680,20 +679,14 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
struct pm8xxx_pin_data *pin_data;
struct pinctrl_pin_desc *pins;
struct pm8xxx_gpio *pctrl;
- int ret;
- int i, npins;
+ int ret, i;
pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
if (!pctrl)
return -ENOMEM;
pctrl->dev = &pdev->dev;
- npins = platform_irq_count(pdev);
- if (!npins)
- return -EINVAL;
- if (npins < 0)
- return npins;
- pctrl->npins = npins;
+ pctrl->npins = (uintptr_t) device_get_match_data(&pdev->dev);
pctrl->regmap = dev_get_regmap(pdev->dev.parent, NULL);
if (!pctrl->regmap) {
--
2.17.2
From: Linus Walleij <[email protected]>
The hierarchical irqchip never before ran into a situation
where the parent is not "simple", i.e. does not implement
.irq_ack() and .irq_mask() like most, but the qcom-pm8xxx.c
happens to implement only .irq_mask_ack().
Since we want to make ssbi-gpio a hierarchical child of this
irqchip, it must *also* only implement .irq_mask_ack()
and call down to the parent, and for this we of course
need irq_chip_mask_ack_parent().
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
Acked-by: Marc Zyngier <[email protected]>
Signed-off-by: Brian Masney <[email protected]>
---
This is a new patch introduced in v2.
include/linux/irq.h | 1 +
kernel/irq/chip.c | 11 +++++++++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index def2b2aac8b1..9a1a67d2e07d 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -605,6 +605,7 @@ extern void irq_chip_disable_parent(struct irq_data *data);
extern void irq_chip_ack_parent(struct irq_data *data);
extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
extern void irq_chip_mask_parent(struct irq_data *data);
+extern void irq_chip_mask_ack_parent(struct irq_data *data);
extern void irq_chip_unmask_parent(struct irq_data *data);
extern void irq_chip_eoi_parent(struct irq_data *data);
extern int irq_chip_set_affinity_parent(struct irq_data *data,
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 086d5a34b5a0..0aefc2e69cf5 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1277,6 +1277,17 @@ void irq_chip_mask_parent(struct irq_data *data)
}
EXPORT_SYMBOL_GPL(irq_chip_mask_parent);
+/**
+ * irq_chip_mask_ack_parent - Mask and acknowledge the parent interrupt
+ * @data: Pointer to interrupt specific data
+ */
+void irq_chip_mask_ack_parent(struct irq_data *data)
+{
+ data = data->parent_data;
+ data->chip->irq_mask_ack(data);
+}
+EXPORT_SYMBOL_GPL(irq_chip_mask_ack_parent);
+
/**
* irq_chip_unmask_parent - Unmask the parent interrupt
* @data: Pointer to interrupt specific data
--
2.17.2
Add a new function irq_domain_translate_twocell() that is to be used as
the translate function in struct irq_domain_ops for the v2 IRQ API.
This patch also changes irq_domain_xlate_twocell() from the v1 IRQ API
to call irq_domain_translate_twocell() in the v2 IRQ API. This required
changes to of_phandle_args_to_fwspec()'s arguments so that it can be
called from multiple places.
Cc: Marc Zyngier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Brian Masney <[email protected]>
---
Changes since v1:
- Included Marc Zyngier's changes to irq_domain_xlate_twocell() and
of_phandle_args_to_fwspec().
include/linux/irqdomain.h | 5 +++++
kernel/irq/irqdomain.c | 45 +++++++++++++++++++++++++++++----------
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 35965f41d7be..fcefe0c7263f 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -419,6 +419,11 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
const u32 *intspec, unsigned int intsize,
irq_hw_number_t *out_hwirq, unsigned int *out_type);
+int irq_domain_translate_twocell(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type);
+
/* IPI functions */
int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest);
int irq_destroy_ipi(unsigned int irq, const struct cpumask *dest);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 45c74373c7a4..c35434f70e71 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -729,16 +729,17 @@ static int irq_domain_translate(struct irq_domain *d,
return 0;
}
-static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
+static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
+ unsigned int count,
struct irq_fwspec *fwspec)
{
int i;
- fwspec->fwnode = irq_data->np ? &irq_data->np->fwnode : NULL;
- fwspec->param_count = irq_data->args_count;
+ fwspec->fwnode = np ? &np->fwnode : NULL;
+ fwspec->param_count = count;
- for (i = 0; i < irq_data->args_count; i++)
- fwspec->param[i] = irq_data->args[i];
+ for (i = 0; i < count; i++)
+ fwspec->param[i] = args[i];
}
unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
@@ -836,7 +837,9 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
{
struct irq_fwspec fwspec;
- of_phandle_args_to_fwspec(irq_data, &fwspec);
+ of_phandle_args_to_fwspec(irq_data->np, irq_data->args,
+ irq_data->args_count, &fwspec);
+
return irq_create_fwspec_mapping(&fwspec);
}
EXPORT_SYMBOL_GPL(irq_create_of_mapping);
@@ -928,11 +931,10 @@ int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
const u32 *intspec, unsigned int intsize,
irq_hw_number_t *out_hwirq, unsigned int *out_type)
{
- if (WARN_ON(intsize < 2))
- return -EINVAL;
- *out_hwirq = intspec[0];
- *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
- return 0;
+ struct irq_fwspec fwspec;
+
+ of_phandle_args_to_fwspec(ctrlr, intspec, intsize, &fwspec);
+ return irq_domain_translate_twocell(d, &fwspec, out_hwirq, out_type);
}
EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
@@ -968,6 +970,27 @@ const struct irq_domain_ops irq_domain_simple_ops = {
};
EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
+/**
+ * irq_domain_translate_twocell() - Generic translate for direct two cell
+ * bindings
+ *
+ * Device Tree IRQ specifier translation function which works with two cell
+ * bindings where the cell values map directly to the hwirq number
+ * and linux irq flags.
+ */
+int irq_domain_translate_twocell(struct irq_domain *d,
+ struct irq_fwspec *fwspec,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (WARN_ON(fwspec->param_count < 2))
+ return -EINVAL;
+ *out_hwirq = fwspec->param[0];
+ *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
+
int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq,
int node, const struct irq_affinity_desc *affinity)
{
--
2.17.2
On 08/02/2019 02:16, Brian Masney wrote:
> Add a new function irq_domain_translate_twocell() that is to be used as
> the translate function in struct irq_domain_ops for the v2 IRQ API.
>
> This patch also changes irq_domain_xlate_twocell() from the v1 IRQ API
> to call irq_domain_translate_twocell() in the v2 IRQ API. This required
> changes to of_phandle_args_to_fwspec()'s arguments so that it can be
> called from multiple places.
>
> Cc: Marc Zyngier <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Signed-off-by: Brian Masney <[email protected]>
> ---
> Changes since v1:
> - Included Marc Zyngier's changes to irq_domain_xlate_twocell() and
> of_phandle_args_to_fwspec().
Thanks for having folded this in.
Reviewed-by: Marc Zyngier <[email protected]>
M.
--
Jazz is not dead. It just smells funny...
On Fri, Feb 8, 2019 at 3:16 AM Brian Masney <[email protected]> wrote:
> This patch series adds hierarchical IRQ chip support to ssbi-gpio so
> that device tree consumers can request an IRQ directly from the GPIO
> block rather than having to request an IRQ from the underlying PMIC.
This looks good and work for me, I'd like to get a nod from one of the
Qualcomm maintainers if possible, then I can merge the whole
thing through the GPIO tree like with the previous patch set.
Yours,
Linus Walleij
On Thu, 07 Feb 2019, Brian Masney wrote:
> Now that ssbi-gpio is a proper hierarchical IRQ chip, and all in-tree
> users of device tree have been updated, we can now drop the hack that
> was introduced to disassociate the old Linux virq if a hwirq mapping
> already exists. That patch was introduced to not break git bisect for
> any existing boards.
>
> This change was tested on an APQ8060 DragonBoard.
>
> Signed-off-by: Brian Masney <[email protected]>
> Tested-by: Linus Walleij <[email protected]>
> ---
> Changes since v1:
> - None
>
> drivers/mfd/qcom-pm8xxx.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index 2f99a98ccee5..8eb2528793f9 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -380,12 +380,6 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> struct irq_domain *domain, unsigned int irq,
> irq_hw_number_t hwirq, unsigned int type)
> {
> - unsigned int old_virq;
> -
> - old_virq = irq_find_mapping(domain, hwirq);
> - if (old_virq)
> - irq_domain_disassociate(domain, old_virq);
> -
For Linus:
Acked-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 07 Feb 2019, Brian Masney wrote:
> Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order
> to support hierarchical IRQ chips. This is necessary so that ssbi-gpio
> can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ
> chips in device tree should be usable from the start without having to
> make an additional call to gpio[d]_to_irq() to get the proper IRQ on the
> parent.
>
> pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this
> patch since the irq_chip is now contained in the pm_irq_data struct, and
> that allows us to use a common IRQ mapping function.
>
> This change was tested on an APQ8060 DragonBoard.
>
> Signed-off-by: Brian Masney <[email protected]>
> Tested-by: Linus Walleij <[email protected]>
> ---
> Changes since v1:
> - Put u8 config[0] at end along with a comment
> - Hardcode the IRQ flow handler as handle_level_irq like the original
> code.
> - Don't set IRQ handler in pm8xxx_irq_set_type() since this is already
> done in pm8xxx_irq_domain_map(). Linus: This is different than what
> you tested but this *should* be fine.
>
> drivers/mfd/qcom-pm8xxx.c | 75 ++++++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 37 deletions(-)
For Linus:
Acked-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 07 Feb 2019, Brian Masney wrote:
> Check to see if the hwirq is already associated with another virq on
> this IRQ domain. If so, then disassociate it before associating the
> hwirq with the new virq.
>
> This is a temporary hack that is needed in order to not break git
> bisect for existing boards. The next patch in this series converts
> ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
> to update all of the device tree files, and finally this patch will be
> reverted within the same patch series.
>
> IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
> this driver is probed due to the interrupts property in device tree.
> Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
> patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
> and that will use the new IRQ chip code in ssbi-gpio that sets up the
> IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
> virqs and interrupts will not work as expected. This patch corrects
> that issue.
>
> This change was tested on an APQ8060 DragonBoard.
>
> Signed-off-by: Brian Masney <[email protected]>
> Tested-by: Linus Walleij <[email protected]>
> ---
> Changes since v1:
> - None
>
> drivers/mfd/qcom-pm8xxx.c | 6 ++++++
> 1 file changed, 6 insertions(+)
For Linus:
Acked-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Feb 8, 2019 at 3:16 AM Brian Masney <[email protected]> wrote:
> This patch series adds hierarchical IRQ chip support to ssbi-gpio so
> that device tree consumers can request an IRQ directly from the GPIO
> block rather than having to request an IRQ from the underlying PMIC.
>
> For more background information, see the email thread with Linus
> Walleij's excellent description of the problem at
> https://www.spinics.net/lists/linux-gpio/msg34655.html.
>
> These changes were tested by Linus Walleij on an APQ8060 DragonBoard.
>
> See the notes on the individual patches for the changelog.
SInce we have definitive ACKs from the irqchip and
MFD maintainers I have queued this series on an immutable branch
that I will merge for v5.1 and offer to the MFD and ARM SoC
once it builds fine on the test servers.
I don't think the Qcom maintainers will complain.
Yours,
Linus Walleij
Quoting Brian Masney (2019-02-07 18:16:25)
> Check to see if the hwirq is already associated with another virq on
> this IRQ domain. If so, then disassociate it before associating the
> hwirq with the new virq.
>
> This is a temporary hack that is needed in order to not break git
> bisect for existing boards. The next patch in this series converts
> ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
> to update all of the device tree files, and finally this patch will be
> reverted within the same patch series.
>
> IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
> this driver is probed due to the interrupts property in device tree.
> Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
> patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
> and that will use the new IRQ chip code in ssbi-gpio that sets up the
> IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
> virqs and interrupts will not work as expected. This patch corrects
> that issue.
>
> This change was tested on an APQ8060 DragonBoard.
>
> Signed-off-by: Brian Masney <[email protected]>
> Tested-by: Linus Walleij <[email protected]>
> ---
> Changes since v1:
> - None
>
> drivers/mfd/qcom-pm8xxx.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index 8eb2528793f9..2f99a98ccee5 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> struct irq_domain *domain, unsigned int irq,
> irq_hw_number_t hwirq, unsigned int type)
> {
> + unsigned int old_virq;
> +
> + old_virq = irq_find_mapping(domain, hwirq);
> + if (old_virq)
> + irq_domain_disassociate(domain, old_virq);
Is it possible to pass 'true' for the 'realloc' argument to
__irq_domain_alloc_irqs() and then this disassociate change isn't
needed?
> +
> irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
> chip, handle_level_irq, NULL, NULL);
> irq_set_noprobe(irq);
On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > index 8eb2528793f9..2f99a98ccee5 100644
> > --- a/drivers/mfd/qcom-pm8xxx.c
> > +++ b/drivers/mfd/qcom-pm8xxx.c
> > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > struct irq_domain *domain, unsigned int irq,
> > irq_hw_number_t hwirq, unsigned int type)
> > {
> > + unsigned int old_virq;
> > +
> > + old_virq = irq_find_mapping(domain, hwirq);
> > + if (old_virq)
> > + irq_domain_disassociate(domain, old_virq);
>
> Is it possible to pass 'true' for the 'realloc' argument to
> __irq_domain_alloc_irqs() and then this disassociate change isn't
> needed?
The kernel doc for __irq_domain_alloc_irqs() says that the realloc
parameter is mainly to support legacy IRQs. I don't think its a good
idea to add new code that'll stay past the end of this patch series
on top of that legacy interface.
Brian
Quoting Brian Masney (2019-02-15 05:47:33)
> On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > index 8eb2528793f9..2f99a98ccee5 100644
> > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > struct irq_domain *domain, unsigned int irq,
> > > irq_hw_number_t hwirq, unsigned int type)
> > > {
> > > + unsigned int old_virq;
> > > +
> > > + old_virq = irq_find_mapping(domain, hwirq);
> > > + if (old_virq)
> > > + irq_domain_disassociate(domain, old_virq);
> >
> > Is it possible to pass 'true' for the 'realloc' argument to
> > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > needed?
>
> The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> parameter is mainly to support legacy IRQs. I don't think its a good
> idea to add new code that'll stay past the end of this patch series
> on top of that legacy interface.
>
Ok. The other side of the argument is that this is the only user of
irq_domain_disassociate(), which may also be some sort of legacy
interface that isn't supposed to be used. Looking at the commit that
exposed it, it seems to be that it's there for legacy reasons.
commit 43a775916d63d1c822107b39987192ca5ced445c
Author: Jiang Liu <[email protected]>
Date: Mon Jun 9 16:20:05 2014 +0800
genirq: Export irq_domain_disassociate() to architecture interrupt drivers
Export irq_domain_disassociate() to architecture interrupt drivers,
so it could be used to handle legacy IRQ descriptors on x86.
So maybe we should just use the realloc argument and bury the
disassociate API in irqdomain.c because it's not supposed to be used?
Or does the realloc path not work for some reason?
On Fri, Feb 15, 2019 at 01:28:02PM -0800, Stephen Boyd wrote:
> Quoting Brian Masney (2019-02-15 05:47:33)
> > On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > > index 8eb2528793f9..2f99a98ccee5 100644
> > > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > > struct irq_domain *domain, unsigned int irq,
> > > > irq_hw_number_t hwirq, unsigned int type)
> > > > {
> > > > + unsigned int old_virq;
> > > > +
> > > > + old_virq = irq_find_mapping(domain, hwirq);
> > > > + if (old_virq)
> > > > + irq_domain_disassociate(domain, old_virq);
> > >
> > > Is it possible to pass 'true' for the 'realloc' argument to
> > > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > > needed?
> >
> > The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> > parameter is mainly to support legacy IRQs. I don't think its a good
> > idea to add new code that'll stay past the end of this patch series
> > on top of that legacy interface.
> >
>
> Ok. The other side of the argument is that this is the only user of
> irq_domain_disassociate(), which may also be some sort of legacy
> interface that isn't supposed to be used. Looking at the commit that
> exposed it, it seems to be that it's there for legacy reasons.
>
> commit 43a775916d63d1c822107b39987192ca5ced445c
> Author: Jiang Liu <[email protected]>
> Date: Mon Jun 9 16:20:05 2014 +0800
>
> genirq: Export irq_domain_disassociate() to architecture interrupt drivers
>
> Export irq_domain_disassociate() to architecture interrupt drivers,
> so it could be used to handle legacy IRQ descriptors on x86.
>
> So maybe we should just use the realloc argument and bury the
> disassociate API in irqdomain.c because it's not supposed to be used?
> Or does the realloc path not work for some reason?
I haven't tried the realloc path yet. Let's back up further so that we
are both starting with the same assumptions. Do you want to keep either
your proposed change (realloc argument) or the existing
irq_domain_disassociate change in mainline past the end of this patch
series? If it is going to continue to be a temporary shim that will be
reverted at the end of the patch series (like I did here), then I don't
see the point in this extra work since this patch is only here to keep
it bisectable and works. We're not using any legacy interfaces by the
end of this patch series.
Brian
On Fri, Feb 8, 2019 at 3:16 AM Brian Masney <[email protected]> wrote:
> This patch series adds hierarchical IRQ chip support to ssbi-gpio so
> that device tree consumers can request an IRQ directly from the GPIO
> block rather than having to request an IRQ from the underlying PMIC.
I have merged this series (+ associated fix) for the v5.1 merge window.
If some disagrees you can shout now.
Yours,
Linus Walleij
Quoting Brian Masney (2019-02-15 16:23:59)
> On Fri, Feb 15, 2019 at 01:28:02PM -0800, Stephen Boyd wrote:
> > Quoting Brian Masney (2019-02-15 05:47:33)
> > > On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > > > index 8eb2528793f9..2f99a98ccee5 100644
> > > > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > > > struct irq_domain *domain, unsigned int irq,
> > > > > irq_hw_number_t hwirq, unsigned int type)
> > > > > {
> > > > > + unsigned int old_virq;
> > > > > +
> > > > > + old_virq = irq_find_mapping(domain, hwirq);
> > > > > + if (old_virq)
> > > > > + irq_domain_disassociate(domain, old_virq);
> > > >
> > > > Is it possible to pass 'true' for the 'realloc' argument to
> > > > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > > > needed?
> > >
> > > The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> > > parameter is mainly to support legacy IRQs. I don't think its a good
> > > idea to add new code that'll stay past the end of this patch series
> > > on top of that legacy interface.
> > >
> >
> > Ok. The other side of the argument is that this is the only user of
> > irq_domain_disassociate(), which may also be some sort of legacy
> > interface that isn't supposed to be used. Looking at the commit that
> > exposed it, it seems to be that it's there for legacy reasons.
> >
> > commit 43a775916d63d1c822107b39987192ca5ced445c
> > Author: Jiang Liu <[email protected]>
> > Date: Mon Jun 9 16:20:05 2014 +0800
> >
> > genirq: Export irq_domain_disassociate() to architecture interrupt drivers
> >
> > Export irq_domain_disassociate() to architecture interrupt drivers,
> > so it could be used to handle legacy IRQ descriptors on x86.
> >
> > So maybe we should just use the realloc argument and bury the
> > disassociate API in irqdomain.c because it's not supposed to be used?
> > Or does the realloc path not work for some reason?
>
> I haven't tried the realloc path yet. Let's back up further so that we
> are both starting with the same assumptions. Do you want to keep either
> your proposed change (realloc argument) or the existing
> irq_domain_disassociate change in mainline past the end of this patch
> series? If it is going to continue to be a temporary shim that will be
> reverted at the end of the patch series (like I did here), then I don't
> see the point in this extra work since this patch is only here to keep
> it bisectable and works. We're not using any legacy interfaces by the
> end of this patch series.
The main concern I have is that we're changing the binding to avoid a
larger discussion we could have about whether or not the binding is
wrong to list out the hierarchical irqs as part of an 'interrupts'
property. We have one case where hardware irq numbers are remapped to
other hardware irq number spaces and it seems perfectly valid to use the
'interrupts' property to indicate this, i.e. chained interrupt
controllers.
Those controllers typically have a single 'interrupts' specifier for the
chained irq of the parent interrupt controller, but when it comes to
hierarchical interrupts we seem to have various different ways of
expressing the mapping from one number space to another. I've seen
approaches varying between hardcoding everything in the kernel to
hardcoding everything in DT.
I'm mostly wondering if having an interrupts property listing all the
parent interrupts in corresponding specifier slots for the interrupt
controller's hardware irq space is wrong. It seems to describe the
mapping between the two number spaces already, so maybe it would make
sense to use the realloc argument and keep listing them as interrupts in
DT. Obviously things are already moving forward with the new DT binding,
so maybe I just need to be told that having an interrupts property there
is wrong. If it is wrong, then nothing needs to be kept around and the
binding can easily be changed.
On Thu, 21 Feb 2019 10:47:03 -0800
Stephen Boyd <[email protected]> wrote:
> Quoting Brian Masney (2019-02-15 16:23:59)
> > On Fri, Feb 15, 2019 at 01:28:02PM -0800, Stephen Boyd wrote:
> > > Quoting Brian Masney (2019-02-15 05:47:33)
> > > > On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > > > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > > > > index 8eb2528793f9..2f99a98ccee5 100644
> > > > > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > > > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > > > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > > > > struct irq_domain *domain, unsigned int irq,
> > > > > > irq_hw_number_t hwirq, unsigned int type)
> > > > > > {
> > > > > > + unsigned int old_virq;
> > > > > > +
> > > > > > + old_virq = irq_find_mapping(domain, hwirq);
> > > > > > + if (old_virq)
> > > > > > + irq_domain_disassociate(domain, old_virq);
> > > > >
> > > > > Is it possible to pass 'true' for the 'realloc' argument to
> > > > > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > > > > needed?
> > > >
> > > > The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> > > > parameter is mainly to support legacy IRQs. I don't think its a good
> > > > idea to add new code that'll stay past the end of this patch series
> > > > on top of that legacy interface.
> > > >
> > >
> > > Ok. The other side of the argument is that this is the only user of
> > > irq_domain_disassociate(), which may also be some sort of legacy
> > > interface that isn't supposed to be used. Looking at the commit that
> > > exposed it, it seems to be that it's there for legacy reasons.
> > >
> > > commit 43a775916d63d1c822107b39987192ca5ced445c
> > > Author: Jiang Liu <[email protected]>
> > > Date: Mon Jun 9 16:20:05 2014 +0800
> > >
> > > genirq: Export irq_domain_disassociate() to architecture interrupt drivers
> > >
> > > Export irq_domain_disassociate() to architecture interrupt drivers,
> > > so it could be used to handle legacy IRQ descriptors on x86.
> > >
> > > So maybe we should just use the realloc argument and bury the
> > > disassociate API in irqdomain.c because it's not supposed to be used?
> > > Or does the realloc path not work for some reason?
> >
> > I haven't tried the realloc path yet. Let's back up further so that we
> > are both starting with the same assumptions. Do you want to keep either
> > your proposed change (realloc argument) or the existing
> > irq_domain_disassociate change in mainline past the end of this patch
> > series? If it is going to continue to be a temporary shim that will be
> > reverted at the end of the patch series (like I did here), then I don't
> > see the point in this extra work since this patch is only here to keep
> > it bisectable and works. We're not using any legacy interfaces by the
> > end of this patch series.
>
> The main concern I have is that we're changing the binding to avoid a
> larger discussion we could have about whether or not the binding is
> wrong to list out the hierarchical irqs as part of an 'interrupts'
> property. We have one case where hardware irq numbers are remapped to
> other hardware irq number spaces and it seems perfectly valid to use the
> 'interrupts' property to indicate this, i.e. chained interrupt
> controllers.
>
> Those controllers typically have a single 'interrupts' specifier for the
> chained irq of the parent interrupt controller, but when it comes to
> hierarchical interrupts we seem to have various different ways of
> expressing the mapping from one number space to another. I've seen
> approaches varying between hardcoding everything in the kernel to
> hardcoding everything in DT.
>
> I'm mostly wondering if having an interrupts property listing all the
> parent interrupts in corresponding specifier slots for the interrupt
> controller's hardware irq space is wrong. It seems to describe the
> mapping between the two number spaces already, so maybe it would make
> sense to use the realloc argument and keep listing them as interrupts in
> DT. Obviously things are already moving forward with the new DT binding,
> so maybe I just need to be told that having an interrupts property there
> is wrong. If it is wrong, then nothing needs to be kept around and the
> binding can easily be changed.
The issue I've had in the past about the use of 'proper' interrupt
specifiers in DT for stuff that is represented as a hierarchy comes
mainly from the way Linux deals with those (unsurprisingly).
The DT parsing code will create a Linux interrupt each time it probes
an device that has these descriptors. That's all fine, except that for
a hierarchy, these output lines are not independent IRQs. We end-up
with double the interrupt descriptors at each level of the hierarchy,
which is both a waste of memory and a pain to debug. It also creates
all kind of bad problems as the interrupt is now registered in the
irqdomain, and I think this is what this patch is trying to work around.
Short of being able to tell the DT parser "don't map interrupts for this
device", we have two options: undo the mess when mapping a hierarchy,
or sidestep the issue altogether, which is what I've recommended in the
past (though Rob did disagree with me on that, and he has good
arguments too).
To be honest, I'd like to make progress on that too, if only to put
something in core code so that individual drivers don't have to play
that kind of game.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, Feb 22, 2019 at 9:57 AM Marc Zyngier <[email protected]> wrote:
> To be honest, I'd like to make progress on that too, if only to put
> something in core code so that individual drivers don't have to play
> that kind of game.
I am trying to pull hierarchical IRQ into the gpiolib core by refactoring
based on these and some other patches (like the IXP4xx GPIO driver).
I am working under the assumptions of what compatible strings
indicating the hierarchy topology and hard coded ranges of
offsets from parent to child in the driver only associated with
the compatible string, so no IRQ range mapping in the device
tree.
AFAICT that is how hierarchical irqdomain is engineered as of
today.
Yours,
Linus Walleij