This series is another attempt on adding wakeup capable GPIOs for QCOM
SoC. This patchset is based on Linus's support for hierarchical GPIOs
merged into linux-next [1]. The essense of the idea remains the same as
the previous submission [2]. GPIO irqchip TLMM is setup in hierarchy with
the PDC as the wakeup-parent. PDC's interrupt parent is the GIC. GPIOs
in QCOM SoC that are wakeup capable (when TLMM is powered off) are
routed to the PDC as well and can be detected at the always-on interrupt
controller (PDC). The idea is setup the irqchips in hierarchy and if the
interrupt is handled at the PDC, then TLMM relinquishes control and
configuration of the interrupt to the PDC.
There are few new additions in this submission. The first is the
additional SPI configuration that needs to be done to setup the GPIO
type in a register interface between the PDC and the GIC. This is needed
only for GPIOs. This registers in some QCOM SoCs is access restricted
and has to be written from the TZ. The DT bindings are also updated for
this new requirement. The second change is that with the new
hierarchical support in gpiolib, we could remove the .alloc and
.translate functions from the pinctrl driver. But to distinguish the
case where a wakeup interrupt controller needs the TLMM to configure the
GPIO interrupts (in the case of MPM interrupt controller), irqdomain
flags have been added. The third change is ensure the interrupt
controllers' interrupt pending bits are cleared when the GPIO is enabled
as an interrupt.
Please consider reviewing these patches.
Thanks,
Lina
Lina Iyer (12):
irqdomain: add bus token DOMAIN_BUS_WAKEUP
drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
of: irq: document properties for wakeup interrupt parent
dt-bindings/interrupt-controller: pdc: add SPI config register
drivers: irqchip: pdc: additionally set type in SPI config registers
drivers: pinctrl: msm: fix use of deprecated gpiolib APIs
drivers: pinctrl: msm: setup GPIO chip in hierarchy
drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs
arm64: dts: qcom: add PDC interrupt controller for SDM845
arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845
arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845
Maulik Shah (2):
genirq: Introduce irq_chip_get/set_parent_state calls
drivers: irqchip: pdc: Add irqchip set/get state calls
.../interrupt-controller/interrupts.txt | 13 +
.../interrupt-controller/qcom,pdc.txt | 9 +-
arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +
arch/arm64/configs/defconfig | 1 +
drivers/irqchip/qcom-pdc.c | 234 +++++++++++++++++-
drivers/pinctrl/qcom/pinctrl-msm.c | 142 +++++++++--
drivers/pinctrl/qcom/pinctrl-msm.h | 16 ++
drivers/pinctrl/qcom/pinctrl-sdm845.c | 83 ++++++-
include/linux/irq.h | 6 +
include/linux/irqdomain.h | 1 +
include/linux/soc/qcom/irq.h | 34 +++
kernel/irq/chip.c | 44 ++++
12 files changed, 566 insertions(+), 28 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
GPIOs that can be configured as wakeup are routed to the PDC wakeup
interrupt controller and from there to the GIC interrupt controller. On
some QCOM SoCs, the interface to the GIC for wakeup capable GPIOs have
additional hardware registers that need to be configured as well to
match the trigger type of the GPIO. This register interfaces the PDC to
the GIC and therefore updated from the PDC driver.
Typically, the firmware intializes the interface registers for the
wakeup capable GPIOs with commonly used GPIO trigger type, but it is
possible that a platform may want to use the GPIO differently. So, in
addition to configuring the PDC, configure the interface registers as
well.
Signed-off-by: Lina Iyer <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 93 ++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index ad1faf634bcf..bf5f98bb4d2b 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -18,6 +18,8 @@
#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/qcom_scm.h>
+
#define PDC_MAX_IRQS 126
#define PDC_MAX_GPIO_IRQS 256
@@ -35,10 +37,20 @@ struct pdc_pin_region {
u32 cnt;
};
+struct spi_cfg_regs {
+ union {
+ u64 start;
+ void __iomem *base;
+ };
+ resource_size_t size;
+ bool scm_io;
+};
+
static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
static int pdc_region_cnt;
+static struct spi_cfg_regs *spi_cfg;
static void pdc_reg_write(int reg, u32 i, u32 val)
{
@@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
irq_chip_unmask_parent(d);
}
+static u32 __spi_pin_read(unsigned int pin)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io) {
+ unsigned int val;
+
+ qcom_scm_io_readl(scm_cfg_reg, &val);
+ return val;
+ } else {
+ return readl(cfg_reg);
+ }
+}
+
+static void __spi_pin_write(unsigned int pin, unsigned int val)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io)
+ qcom_scm_io_writel(scm_cfg_reg, val);
+ else
+ writel(val, cfg_reg);
+}
+
+static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
+{
+ int spi = hwirq - 32;
+ u32 pin = spi / 32;
+ u32 mask = BIT(spi % 32);
+ u32 val;
+ unsigned long flags;
+
+ if (!spi_cfg)
+ return 0;
+
+ if (pin * 4 > spi_cfg->size)
+ return -EFAULT;
+
+ raw_spin_lock_irqsave(&pdc_lock, flags);
+ val = __spi_pin_read(pin);
+ val &= ~mask;
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ val |= mask;
+ __spi_pin_write(pin, val);
+ raw_spin_unlock_irqrestore(&pdc_lock, flags);
+
+ return 0;
+}
+
/*
* GIC does not handle falling edge or active low. To allow falling edge and
* active low interrupts to be handled at GIC, PDC has an inverter that inverts
@@ -137,7 +200,9 @@ enum pdc_irq_config_bits {
static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
{
int pin_out = d->hwirq;
+ int parent_hwirq = d->parent_data->hwirq;
enum pdc_irq_config_bits pdc_type;
+ int ret;
if (pin_out == GPIO_NO_WAKE_IRQ)
return 0;
@@ -168,6 +233,11 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
+ /* Additionally, configure (only) the GPIO in the f/w */
+ ret = spi_configure_type(parent_hwirq, type);
+ if (ret)
+ return ret;
+
return irq_chip_set_type_parent(d, type);
}
@@ -354,6 +424,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
{
struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
+ struct resource res;
int ret;
pdc_base = of_iomap(node, 0);
@@ -384,6 +455,27 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
goto fail;
}
+ ret = of_address_to_resource(node, 1, &res);
+ if (!ret) {
+ spi_cfg = kcalloc(1, sizeof(*spi_cfg), GFP_KERNEL);
+ if (!spi_cfg) {
+ ret = -ENOMEM;
+ goto remove;
+ }
+ spi_cfg->scm_io = of_find_property(node,
+ "qcom,scm-spi-cfg", NULL);
+ spi_cfg->size = resource_size(&res);
+ if (spi_cfg->scm_io) {
+ spi_cfg->start = res.start;
+ } else {
+ spi_cfg->base = ioremap(res.start, spi_cfg->size);
+ if (!spi_cfg->base) {
+ ret = -ENOMEM;
+ goto remove;
+ }
+ }
+ }
+
pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
PDC_MAX_GPIO_IRQS,
@@ -401,6 +493,7 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
remove:
irq_domain_remove(pdc_domain);
+ kfree(spi_cfg);
fail:
kfree(pdc_region);
iounmap(pdc_base);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
In addition to configuring the PDC, additional registers that interface
the GIC have to be configured to match the GPIO type. The registers on
some QCOM SoCs are access restricted, while on other SoCs are not. They
SoCs with access restriction to these SPI registers need to be written
from the firmware using the SCM interface. Add a flag to indicate if the
register is to be written using SCM interface.
Cc: [email protected]
Signed-off-by: Lina Iyer <[email protected]>
---
.../bindings/interrupt-controller/qcom,pdc.txt | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
index 8e0797cb1487..852fcba98ea6 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -50,15 +50,22 @@ Properties:
The second element is the GIC hwirq number for the PDC port.
The third element is the number of interrupts in sequence.
+- qcom,scm-spi-cfg:
+ Usage: optional
+ Value type: <bool>
+ Definition: Specifies if the SPI configuration registers have to be
+ written from the firmware.
+
Example:
pdc: interrupt-controller@b220000 {
compatible = "qcom,sdm845-pdc";
- reg = <0xb220000 0x30000>;
+ reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
#interrupt-cells = <2>;
interrupt-parent = <&intc>;
interrupt-controller;
+ qcom,scm-spi-cfg;
};
DT binding of a device that wants to use the GIC SPI 514 as a wakeup
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 07ec8b390161..cc846abeff28 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -83,6 +83,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
DOMAIN_BUS_TI_SCI_INTA_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> In addition to configuring the PDC, additional registers that interface
> the GIC have to be configured to match the GPIO type. The registers on
> some QCOM SoCs are access restricted, while on other SoCs are not. They
> SoCs with access restriction to these SPI registers need to be written
Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
> from the firmware using the SCM interface. Add a flag to indicate if the
> register is to be written using SCM interface.
>
> Cc: [email protected]
> Signed-off-by: Lina Iyer <[email protected]>
> ---
> .../bindings/interrupt-controller/qcom,pdc.txt | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> index 8e0797cb1487..852fcba98ea6 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> @@ -50,15 +50,22 @@ Properties:
> The second element is the GIC hwirq number for the PDC port.
> The third element is the number of interrupts in sequence.
>
> +- qcom,scm-spi-cfg:
> + Usage: optional
> + Value type: <bool>
> + Definition: Specifies if the SPI configuration registers have to be
> + written from the firmware.
> +
> Example:
>
> pdc: interrupt-controller@b220000 {
> compatible = "qcom,sdm845-pdc";
> - reg = <0xb220000 0x30000>;
> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
There needs to be a description for reg updated. These aren't GIC
registers are they? Because those go in the GIC node.
> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
> #interrupt-cells = <2>;
> interrupt-parent = <&intc>;
> interrupt-controller;
> + qcom,scm-spi-cfg;
> };
>
> DT binding of a device that wants to use the GIC SPI 514 as a wakeup
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On 02/09/2019 14:38, Rob Herring wrote:
> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> In addition to configuring the PDC, additional registers that interface
>> the GIC have to be configured to match the GPIO type. The registers on
>> some QCOM SoCs are access restricted, while on other SoCs are not. They
>> SoCs with access restriction to these SPI registers need to be written
>
> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
>
>> from the firmware using the SCM interface. Add a flag to indicate if the
>> register is to be written using SCM interface.
>>
>> Cc: [email protected]
>> Signed-off-by: Lina Iyer <[email protected]>
>> ---
>> .../bindings/interrupt-controller/qcom,pdc.txt | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> index 8e0797cb1487..852fcba98ea6 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>> @@ -50,15 +50,22 @@ Properties:
>> The second element is the GIC hwirq number for the PDC port.
>> The third element is the number of interrupts in sequence.
>>
>> +- qcom,scm-spi-cfg:
>> + Usage: optional
>> + Value type: <bool>
>> + Definition: Specifies if the SPI configuration registers have to be
>> + written from the firmware.
>> +
>> Example:
>>
>> pdc: interrupt-controller@b220000 {
>> compatible = "qcom,sdm845-pdc";
>> - reg = <0xb220000 0x30000>;
>> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>
> There needs to be a description for reg updated. These aren't GIC
> registers are they? Because those go in the GIC node.
This is completely insane. Why are the GIC registers configured as
secure the first place, if they are expected to be in control of the
non-secure?
M.
--
Jazz is not dead. It just smells funny...
On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>On 02/09/2019 14:38, Rob Herring wrote:
>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>>> In addition to configuring the PDC, additional registers that interface
>>> the GIC have to be configured to match the GPIO type. The registers on
>>> some QCOM SoCs are access restricted, while on other SoCs are not. They
>>> SoCs with access restriction to these SPI registers need to be written
>>
>> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
>>
>>> from the firmware using the SCM interface. Add a flag to indicate if the
>>> register is to be written using SCM interface.
>>>
>>> Cc: [email protected]
>>> Signed-off-by: Lina Iyer <[email protected]>
>>> ---
>>> .../bindings/interrupt-controller/qcom,pdc.txt | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> index 8e0797cb1487..852fcba98ea6 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
>>> @@ -50,15 +50,22 @@ Properties:
>>> The second element is the GIC hwirq number for the PDC port.
>>> The third element is the number of interrupts in sequence.
>>>
>>> +- qcom,scm-spi-cfg:
>>> + Usage: optional
>>> + Value type: <bool>
>>> + Definition: Specifies if the SPI configuration registers have to be
>>> + written from the firmware.
>>> +
>>> Example:
>>>
>>> pdc: interrupt-controller@b220000 {
>>> compatible = "qcom,sdm845-pdc";
>>> - reg = <0xb220000 0x30000>;
>>> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>>
>> There needs to be a description for reg updated. These aren't GIC
>> registers are they? Because those go in the GIC node.
>
They are not GIC registers. I will update this documentation.
>This is completely insane. Why are the GIC registers configured as
>secure the first place, if they are expected to be in control of the
>non-secure?
These are not GIC registers but located on the PDC interface to the GIC.
They may or may not be secure access controlled, depending on the SoC.
Thanks,
Lina
Quoting Lina Iyer (2019-09-03 10:07:22)
> On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
> >On 02/09/2019 14:38, Rob Herring wrote:
> >> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> >>> In addition to configuring the PDC, additional registers that interface
> >>> the GIC have to be configured to match the GPIO type. The registers on
> >>> some QCOM SoCs are access restricted, while on other SoCs are not. They
> >>> SoCs with access restriction to these SPI registers need to be written
> >>
> >> Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
> >>
> >>> from the firmware using the SCM interface. Add a flag to indicate if the
> >>> register is to be written using SCM interface.
> >>>
> >>> Cc: [email protected]
> >>> Signed-off-by: Lina Iyer <[email protected]>
> >>> ---
> >>> .../bindings/interrupt-controller/qcom,pdc.txt | 9 ++++++++-
> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> index 8e0797cb1487..852fcba98ea6 100644
> >>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
> >>> @@ -50,15 +50,22 @@ Properties:
> >>> The second element is the GIC hwirq number for the PDC port.
> >>> The third element is the number of interrupts in sequence.
> >>>
> >>> +- qcom,scm-spi-cfg:
> >>> + Usage: optional
> >>> + Value type: <bool>
> >>> + Definition: Specifies if the SPI configuration registers have to be
> >>> + written from the firmware.
> >>> +
> >>> Example:
> >>>
> >>> pdc: interrupt-controller@b220000 {
> >>> compatible = "qcom,sdm845-pdc";
> >>> - reg = <0xb220000 0x30000>;
> >>> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
> >>
> >> There needs to be a description for reg updated. These aren't GIC
> >> registers are they? Because those go in the GIC node.
> >
> They are not GIC registers. I will update this documentation.
>
> >This is completely insane. Why are the GIC registers configured as
> >secure the first place, if they are expected to be in control of the
> >non-secure?
> These are not GIC registers but located on the PDC interface to the GIC.
> They may or may not be secure access controlled, depending on the SoC.
>
It looks like it falls under this "mailbox" device which is really the
catch all bucket for bits with no home besides they're related to the
apps CPUs/subsystem.
apss_shared: mailbox@17990000 {
compatible = "qcom,sdm845-apss-shared";
reg = <0 0x17990000 0 0x1000>;
#mbox-cells = <1>;
};
Can you point to this node with a phandle and then parse the reg
property out of it to use in the scm readl/writel APIs? Maybe it can be
a two cell property with <&apps_shared 0xf0> to indicate the offset to
the registers to read/write? In non-secure mode presumably we need to
also write these registers? Good news is that there's a regmap for this
driver already, so maybe that can be acquired from the pdc driver.
Quoting Lina Iyer (2019-08-29 11:11:55)
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index ad1faf634bcf..bf5f98bb4d2b 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
> irq_chip_unmask_parent(d);
> }
>
> +static u32 __spi_pin_read(unsigned int pin)
> +{
> + void __iomem *cfg_reg = spi_cfg->base + pin * 4;
> + u64 scm_cfg_reg = spi_cfg->start + pin * 4;
> +
> + if (spi_cfg->scm_io) {
> + unsigned int val;
> +
> + qcom_scm_io_readl(scm_cfg_reg, &val);
> + return val;
> + } else {
> + return readl(cfg_reg);
> + }
Please remove the else and just return readl() result instead.
> +}
> +
> +static void __spi_pin_write(unsigned int pin, unsigned int val)
> +{
> + void __iomem *cfg_reg = spi_cfg->base + pin * 4;
> + u64 scm_cfg_reg = spi_cfg->start + pin * 4;
> +
> + if (spi_cfg->scm_io)
> + qcom_scm_io_writel(scm_cfg_reg, val);
> + else
> + writel(val, cfg_reg);
> +}
> +
> +static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
> +{
> + int spi = hwirq - 32;
> + u32 pin = spi / 32;
> + u32 mask = BIT(spi % 32);
> + u32 val;
> + unsigned long flags;
> +
> + if (!spi_cfg)
> + return 0;
> +
> + if (pin * 4 > spi_cfg->size)
> + return -EFAULT;
> +
> + raw_spin_lock_irqsave(&pdc_lock, flags);
Ah I don't think the regmap would use a raw spinlock, so that's another
hurdle to get over here.
> + val = __spi_pin_read(pin);
> + val &= ~mask;
> + if (type & IRQ_TYPE_LEVEL_MASK)
> + val |= mask;
> + __spi_pin_write(pin, val);
Does monitoring level triggered interrupts matter? I'm asking if the
whole thing can be configured to monitor for edges regardless of trigger
type and then let the level handling be done by the GIC after the wakeup
or when the device is active.
> + raw_spin_unlock_irqrestore(&pdc_lock, flags);
> +
> + return 0;
> +}
> +
> /*
> * GIC does not handle falling edge or active low. To allow falling edge and
> * active low interrupts to be handled at GIC, PDC has an inverter that inverts
On Thu, Aug 29, 2019 at 8:47 PM Lina Iyer <[email protected]> wrote:
> +- qcom,scm-spi-cfg:
> + Usage: optional
> + Value type: <bool>
> + Definition: Specifies if the SPI configuration registers have to be
> + written from the firmware.
> +
> Example:
>
> pdc: interrupt-controller@b220000 {
> compatible = "qcom,sdm845-pdc";
> - reg = <0xb220000 0x30000>;
> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
> #interrupt-cells = <2>;
> interrupt-parent = <&intc>;
> interrupt-controller;
> + qcom,scm-spi-cfg;
You can probably drop this bool if you just give names to the registers.
Like
reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
reg-names = "gic", "pdc";
Then jus check explicitly for a "pdc" register and in that case
initialize the PDC.
Yours,
Linus Walleij
On Wed, Sep 11 2019 at 04:05 -0600, Linus Walleij wrote:
>On Thu, Aug 29, 2019 at 8:47 PM Lina Iyer <[email protected]> wrote:
>
>> +- qcom,scm-spi-cfg:
>> + Usage: optional
>> + Value type: <bool>
>> + Definition: Specifies if the SPI configuration registers have to be
>> + written from the firmware.
>> +
>> Example:
>>
>> pdc: interrupt-controller@b220000 {
>> compatible = "qcom,sdm845-pdc";
>> - reg = <0xb220000 0x30000>;
>> + reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>> #interrupt-cells = <2>;
>> interrupt-parent = <&intc>;
>> interrupt-controller;
>> + qcom,scm-spi-cfg;
>
>You can probably drop this bool if you just give names to the registers.
>
>Like
>reg = <0xb220000 0x30000>, <0x179900f0 0x60>;
>reg-names = "gic", "pdc";
>
>Then jus check explicitly for a "pdc" register and in that case
>initialize the PDC.
>
Well the address remains the same. The bool defines how to access that
register address - from linux or from the firmware using SCM calls. But
I get your point, I could have different register namess - pdc-linux or
pdc-scm and request by name. I can then use that to determine the mode
for accessing the register.
Thanks,
Lina
Sorry, I couldn't get to this earlier.
On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-09-03 10:07:22)
>> On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>> >On 02/09/2019 14:38, Rob Herring wrote:
>> >> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> These are not GIC registers but located on the PDC interface to the GIC.
>> They may or may not be secure access controlled, depending on the SoC.
>>
>
>It looks like it falls under this "mailbox" device which is really the
>catch all bucket for bits with no home besides they're related to the
>apps CPUs/subsystem.
>
Thanks for pointing to this.
> apss_shared: mailbox@17990000 {
> compatible = "qcom,sdm845-apss-shared";
> reg = <0 0x17990000 0 0x1000>;
But this doesn't seem correct. The registers in this page are all not
mailbox door bell registers. We should restrict the space allocated to
the mbox to 0xC or something, definitely, not the whole page. They all
cannot be treated as a mailbox registers.
> #mbox-cells = <1>;
> };
>
>Can you point to this node with a phandle and then parse the reg
>property out of it to use in the scm readl/writel APIs? Maybe it can be
>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>the registers to read/write? In non-secure mode presumably we need to
>also write these registers? Good news is that there's a regmap for this
>driver already, so maybe that can be acquired from the pdc driver.
>
The register space collection seems to be mix of different types of
application processor registers that should probably not be grouped up
under one subsystem. A single regmap doesn't seem correct either.
-- Lina
Adding Sibi
On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
>Sorry, I couldn't get to this earlier.
>
>On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>>Quoting Lina Iyer (2019-09-03 10:07:22)
>>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>>>>On 02/09/2019 14:38, Rob Herring wrote:
>>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>>>These are not GIC registers but located on the PDC interface to the GIC.
>>>They may or may not be secure access controlled, depending on the SoC.
>>>
>>
>>It looks like it falls under this "mailbox" device which is really the
>>catch all bucket for bits with no home besides they're related to the
>>apps CPUs/subsystem.
>>
>Thanks for pointing to this.
>> apss_shared: mailbox@17990000 {
>> compatible = "qcom,sdm845-apss-shared";
>> reg = <0 0x17990000 0 0x1000>;
>But this doesn't seem correct. The registers in this page are all not
>mailbox door bell registers. We should restrict the space allocated to
>the mbox to 0xC or something, definitely, not the whole page. They all
>cannot be treated as a mailbox registers.
>> #mbox-cells = <1>;
>> };
>>
>>Can you point to this node with a phandle and then parse the reg
>>property out of it to use in the scm readl/writel APIs? Maybe it can be
>>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>>the registers to read/write? In non-secure mode presumably we need to
>>also write these registers? Good news is that there's a regmap for this
>>driver already, so maybe that can be acquired from the pdc driver.
>>
>The register space collection seems to be mix of different types of
>application processor registers that should probably not be grouped up
>under one subsystem. A single regmap doesn't seem correct either.
>
>-- Lina
Quoting Lina Iyer (2019-09-17 14:50:20)
> On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
> >On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
> >>Quoting Lina Iyer (2019-09-03 10:07:22)
> >>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
> >>>>On 02/09/2019 14:38, Rob Herring wrote:
> >>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
> >>>These are not GIC registers but located on the PDC interface to the GIC.
> >>>They may or may not be secure access controlled, depending on the SoC.
> >>>
> >>
> >>It looks like it falls under this "mailbox" device which is really the
> >>catch all bucket for bits with no home besides they're related to the
> >>apps CPUs/subsystem.
> >>
> >Thanks for pointing to this.
> >> apss_shared: mailbox@17990000 {
> >> compatible = "qcom,sdm845-apss-shared";
> >> reg = <0 0x17990000 0 0x1000>;
> >But this doesn't seem correct. The registers in this page are all not
> >mailbox door bell registers. We should restrict the space allocated to
> >the mbox to 0xC or something, definitely, not the whole page. They all
> >cannot be treated as a mailbox registers.
Well the binding is already done and this is the compatible string for
this node and register region. Sounds like this node is a mailbox plus
some more stuff in the same page.
> >> #mbox-cells = <1>;
> >> };
> >>
> >>Can you point to this node with a phandle and then parse the reg
> >>property out of it to use in the scm readl/writel APIs? Maybe it can be
> >>a two cell property with <&apps_shared 0xf0> to indicate the offset to
> >>the registers to read/write? In non-secure mode presumably we need to
> >>also write these registers? Good news is that there's a regmap for this
> >>driver already, so maybe that can be acquired from the pdc driver.
> >>
> >The register space collection seems to be mix of different types of
> >application processor registers that should probably not be grouped up
> >under one subsystem. A single regmap doesn't seem correct either.
Why isn't a single regmap correct? The PDC driver should be able to use
it to read/write into this register space. The lock on the regmap will
need to be changed to a raw lock though for RT. Otherwise it looks OK to
me.
On 2019-09-21 03:50, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-09-17 14:50:20)
>> On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
>> >On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
>> >>Quoting Lina Iyer (2019-09-03 10:07:22)
>> >>>On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>> >>>>On 02/09/2019 14:38, Rob Herring wrote:
>> >>>>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
>> >>>These are not GIC registers but located on the PDC interface to the GIC.
>> >>>They may or may not be secure access controlled, depending on the SoC.
>> >>>
>> >>
>> >>It looks like it falls under this "mailbox" device which is really the
>> >>catch all bucket for bits with no home besides they're related to the
>> >>apps CPUs/subsystem.
>> >>
>> >Thanks for pointing to this.
>> >> apss_shared: mailbox@17990000 {
>> >> compatible = "qcom,sdm845-apss-shared";
>> >> reg = <0 0x17990000 0 0x1000>;
>> >But this doesn't seem correct. The registers in this page are all not
>> >mailbox door bell registers. We should restrict the space allocated to
>> >the mbox to 0xC or something, definitely, not the whole page. They all
>> >cannot be treated as a mailbox registers.
>
> Well the binding is already done and this is the compatible string for
> this node and register region. Sounds like this node is a mailbox plus
> some more stuff in the same page.
>
Bjorn already noticed ^^ during the
original review. Hence the compatible
was correctly named "apss-shared"
instead of following the older bindings.
>> >> #mbox-cells = <1>;
>> >> };
>> >>
>> >>Can you point to this node with a phandle and then parse the reg
>> >>property out of it to use in the scm readl/writel APIs? Maybe it can be
>> >>a two cell property with <&apps_shared 0xf0> to indicate the offset to
>> >>the registers to read/write? In non-secure mode presumably we need to
>> >>also write these registers? Good news is that there's a regmap for this
>> >>driver already, so maybe that can be acquired from the pdc driver.
>> >>
>> >The register space collection seems to be mix of different types of
>> >application processor registers that should probably not be grouped up
>> >under one subsystem. A single regmap doesn't seem correct either.
>
> Why isn't a single regmap correct? The PDC driver should be able to use
> it to read/write into this register space. The lock on the regmap will
> need to be changed to a raw lock though for RT. Otherwise it looks OK
> to
> me.
--
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.