RE: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Heiko Stübner wrote: The s3c24xx pins follow a similar pattern as the other Samsung SoCs and can therefore reuse the already introduced infrastructure. The s3c24xx SoCs have one design oddity in that the first 4 external interrupts do not reside in the eint pending register but in the main interrupt controller instead. We solve this by forwarding the external interrupt from the main controller into the irq domain of the pin bank. The masking/acking of these interrupts is handled in the same way. Furthermore the S3C2412/2413 SoCs contain another oddity in that they keep the same 4 eints in the main interrupt controller and eintpend register and requiring ack operations to happen in both. To solve this a ctrl_type enum is introduced which can keep the type of controller in the samsung_pin_ctrl struct for later retrieval. The ctrl_type enum contains only S3C24XX and S3C2412 types, as the eint-speciality is currently the only use-case. But it can be expaned if other SoCs gain special handling requirements later on. Signed-off-by: Heiko Stuebner he...@sntech.de Looks good to me, need to implement more for other s3c24xx though. Linus, if you want, please add: Acked-by: Kukjin Kim kgene@samsung.com Thanks. - Kukjin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Hi Heiko,
Basically looks good to me, but please see my inline comments about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung SoCs and
can therefore reuse the already introduced infrastructure.
The s3c24xx SoCs have one design oddity in that the first 4 external
interrupts do not reside in the eint pending register but in the main
interrupt controller instead. We solve this by forwarding the external
interrupt from the main controller into the irq domain of the pin bank.
The masking/acking of these interrupts is handled in the same way.
Furthermore the S3C2412/2413 SoCs contain another oddity in that they
keep the same 4 eints in the main interrupt controller and eintpend
register and requiring ack operations to happen in both. To solve this
a ctrl_type enum is introduced which can keep the type of controller
in the samsung_pin_ctrl struct for later retrieval.
The ctrl_type enum contains only S3C24XX and S3C2412 types, as the
eint-speciality is currently the only use-case. But it can be expaned
if other SoCs gain special handling requirements later on.
Signed-off-by: Heiko Stuebner he...@sntech.de
---
Depends on the s3c64xx pinctrl work from Tomasz Figa.
It also does not yet contain the pin-definitions for all s3c24xx SoCs,
as I don't have datasheets for them.
Tested on a s3c2416 based board.
.../bindings/pinctrl/samsung-pinctrl.txt |4 +
drivers/gpio/gpio-samsung.c|4 +
drivers/pinctrl/Kconfig|5 +
drivers/pinctrl/Makefile |1 +
drivers/pinctrl/pinctrl-s3c24xx.c | 603
drivers/pinctrl/pinctrl-samsung.c
| 10 +
drivers/pinctrl/pinctrl-samsung.h | 16 +
7 files changed, 643 insertions(+), 0 deletions(-)
create mode 100644 drivers/pinctrl/pinctrl-s3c24xx.c
diff --git
a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt index
c70fca1..1d8fc3c 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -7,6 +7,10 @@ on-chip controllers onto these pads.
Required Properties:
- compatible: should be one of the following.
+ - samsung,s3c2413-pinctrl: for S3C64xx-compatible pin-controller,
+ - samsung,s3c2416-pinctrl: for S3C64xx-compatible pin-controller,
+ - samsung,s3c2440-pinctrl: for S3C64xx-compatible pin-controller,
+ - samsung,s3c2450-pinctrl: for S3C64xx-compatible pin-controller,
- samsung,s3c64xx-pinctrl: for S3C64xx-compatible pin-controller,
- samsung,exynos4210-pinctrl: for Exynos4210 compatible
pin-controller. - samsung,exynos4x12-pinctrl: for Exynos4x12
compatible pin-controller. diff --git a/drivers/gpio/gpio-samsung.c
b/drivers/gpio/gpio-samsung.c index dc06a6f..73017b9 100644
--- a/drivers/gpio/gpio-samsung.c
+++ b/drivers/gpio/gpio-samsung.c
@@ -3026,6 +3026,10 @@ static __init int samsung_gpiolib_init(void)
*/
struct device_node *pctrl_np;
static const struct of_device_id exynos_pinctrl_ids[] = {
+ { .compatible = samsung,s3c2413-pinctrl, },
+ { .compatible = samsung,s3c2416-pinctrl, },
+ { .compatible = samsung,s3c2440-pinctrl, },
+ { .compatible = samsung,s3c2450-pinctrl, },
{ .compatible = samsung,s3c64xx-pinctrl, },
{ .compatible = samsung,exynos4210-pinctrl, },
{ .compatible = samsung,exynos4x12-pinctrl, },
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7402ac9..58d73ac 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -226,6 +226,11 @@ config PINCTRL_EXYNOS5440
select PINMUX
select PINCONF
+config PINCTRL_S3C24XX
+ bool Samsung S3C24XX SoC pinctrl driver
+ depends on ARCH_S3C24XX
+ select PINCTRL_SAMSUNG
+
config PINCTRL_S3C64XX
bool Samsung S3C64XX SoC pinctrl driver
depends on ARCH_S3C64XX
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 21d34c2..1ccdfd8 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_COH901)+= pinctrl-
coh901.o
obj-$(CONFIG_PINCTRL_SAMSUNG)+= pinctrl-samsung.o
obj-$(CONFIG_PINCTRL_EXYNOS) += pinctrl-exynos.o
obj-$(CONFIG_PINCTRL_EXYNOS5440) += pinctrl-exynos5440.o
+obj-$(CONFIG_PINCTRL_S3C24XX)+= pinctrl-s3c24xx.o
obj-$(CONFIG_PINCTRL_S3C64XX)+= pinctrl-s3c64xx.o
obj-$(CONFIG_PINCTRL_XWAY) += pinctrl-xway.o
obj-$(CONFIG_PINCTRL_LANTIQ) += pinctrl-lantiq.o
diff --git a/drivers/pinctrl/pinctrl-s3c24xx.c
b/drivers/pinctrl/pinctrl-s3c24xx.c new file mode 100644
index 000..6b05519
--- /dev/null
+++
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung SoCs and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would store the
information about the s3c24xx-specific quirk in 24xx-specific data
structure, without the need to add another field to the generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in interrupt
handling code.
ok, sounds good, especially gathering the type from the wakeup-int property
+};
[...]
Now I would split the following 3 functions into two sets of 3 functions,
one set for s3c2412 and other for remaining SoCs and make separate EINT0-3
IRQ chips for both cases.
Not doing the decision every time, might bring some very slight speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the irq-handler should
stay in charge of when to ack it might be better this way.
Generic s3c24xx SoCs need acking in the main controller only, while s3c2412
needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data = irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack callback of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an unmapped
EINT
+* was unmasked...
+*/
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it would limit the
eints to a level style handling. With the way it's currently the whole
determination of when to ack,mask and unmask is completely for the real
handler (edge or level) to decide, as the original interrupt gets completely
forwarded into the irq-domain without further constraints.
So, after the change on regular s3c24xx SoCs when the real irq handler wants
to ack the irq, it would be a no-op, as it would already have been acked by
chained_irq_enter.
Masking might be even more interesting. Currently control is transfered
completely to the pinctrl irq-domain, which then controls the masking of the
interrupt thru the parent-calls - on regular s3c24xx the masking of these is
also only done in the main controller.
When using chained_irq_* it also wants to mask the interrupt which might
conflict with regular enable_irq/disable_irq calls being done for example in
driver code.
So in short I agree with the earlier split of the irqchip, but would keep the
irq operations themself centralized in the pinctrl driver, instead of using
chained_irq_* functions.
Heiko
+}
+
+static inline void s3c24xx_demux_eint(unsigned int irq, struct irq_desc
*desc, + u32 offset, u32 range)
+{
+ struct irq_chip *chip = irq_get_chip(irq);
+ struct s3c24xx_eint_data *data = irq_get_handler_data(irq);
+ struct samsung_pinctrl_drv_data *d = data-drvdata;
+ unsigned int pend, mask;
+
+ chained_irq_enter(chip, desc);
+
+ pend = readl(d-virt_base + EINTPEND_REG);
+ mask = readl(d-virt_base + EINTMASK_REG);
+
+ pend = ~mask;
+ pend = range;
+
+ while (pend) {
+ unsigned
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would store the
information about the s3c24xx-specific quirk in 24xx-specific data
structure, without the need to add another field to the generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs and make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the irq-handler
should stay in charge of when to ack it might be better this way.
Generic s3c24xx SoCs need acking in the main controller only, while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data = irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack callback of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an unmapped
EINT
+ * was unmasked...
+ */
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it would limit
the eints to a level style handling. With the way it's currently the
whole determination of when to ack,mask and unmask is completely for
the real handler (edge or level) to decide, as the original interrupt
gets completely forwarded into the irq-domain without further
constraints.
So, after the change on regular s3c24xx SoCs when the real irq handler
wants to ack the irq, it would be a no-op, as it would already have
been acked by chained_irq_enter.
Masking might be even more interesting. Currently control is transfered
completely to the pinctrl irq-domain, which then controls the masking of
the interrupt thru the parent-calls - on regular s3c24xx the masking of
these is also only done in the main controller.
When using chained_irq_* it also wants to mask the interrupt which might
conflict with regular enable_irq/disable_irq calls being done for
example in driver code.
So in short I agree with the earlier split of the irqchip, but would
keep the irq operations themself centralized in the pinctrl driver,
instead of using chained_irq_* functions.
Right, my solution wouldn't work properly in case of regular s3c24xx and
edge triggered interrupts.
However I'm still wondering if it's OK to manually call parent chip
operations in case of s3c2416. This would imply the same operation calling
order as imposed by flow handler of the chained EINT (which can be
handle_edge_irq), while the parent chip is probably level triggered, isn't
it?
Best regards,
Tomasz
Heiko
+}
+
+static inline void
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would store the
information about the s3c24xx-specific quirk in 24xx-specific data
structure, without the need to add another field to the generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs and make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank =
irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the irq-handler
should stay in charge of when to ack it might be better this way.
Generic s3c24xx SoCs need acking in the main controller only, while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data = irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack callback of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an unmapped
EINT
+* was unmasked...
+*/
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it would limit
the eints to a level style handling. With the way it's currently the
whole determination of when to ack,mask and unmask is completely for
the real handler (edge or level) to decide, as the original interrupt
gets completely forwarded into the irq-domain without further
constraints.
So, after the change on regular s3c24xx SoCs when the real irq handler
wants to ack the irq, it would be a no-op, as it would already have
been acked by chained_irq_enter.
Masking might be even more interesting. Currently control is transfered
completely to the pinctrl irq-domain, which then controls the masking of
the interrupt thru the parent-calls - on regular s3c24xx the masking of
these is also only done in the main controller.
When using chained_irq_* it also wants to mask the interrupt which might
conflict with regular enable_irq/disable_irq calls being done for
example in driver code.
So in short I agree with the earlier split of the irqchip, but would
keep the irq operations themself centralized in the pinctrl driver,
instead of using chained_irq_* functions.
Right, my solution wouldn't work properly in case of regular s3c24xx and
edge triggered interrupts.
However I'm still
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
On Wednesday 10 of April 2013 15:45:48 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments
about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung
SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would store
the
information about the s3c24xx-specific quirk in 24xx-specific data
structure, without the need to add another field to the generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs and
make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight
speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank =
irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-
irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the
irq-handler
should stay in charge of when to ack it might be better this way.
Generic s3c24xx SoCs need acking in the main controller only, while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct
irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data =
irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack callback
of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an
unmapped
EINT
+ * was unmasked...
+ */
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it would
limit the eints to a level style handling. With the way it's
currently the whole determination of when to ack,mask and unmask is
completely for the real handler (edge or level) to decide, as the
original interrupt gets completely forwarded into the irq-domain
without further
constraints.
So, after the change on regular s3c24xx SoCs when the real irq
handler
wants to ack the irq, it would be a no-op, as it would already have
been acked by chained_irq_enter.
Masking might be even more interesting. Currently control is
transfered
completely to the pinctrl irq-domain, which then controls the
masking of the interrupt thru the parent-calls - on regular s3c24xx
the masking of these is also only done in the main controller.
When using chained_irq_* it also wants to mask the interrupt which
might conflict with regular enable_irq/disable_irq calls being done
for example in driver code.
So in short I agree with the earlier split of the irqchip, but would
keep the irq operations themself
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Am Mittwoch, 10. April 2013, 21:51:11 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 15:45:48 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments
about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other Samsung
SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would store
the
information about the s3c24xx-specific quirk in 24xx-specific data
structure, without the need to add another field to the generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs and
make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight
speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank =
irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-
irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the
irq-handler
should stay in charge of when to ack it might be better this way.
Generic s3c24xx SoCs need acking in the main controller only, while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct
irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data =
irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack callback
of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an
unmapped
EINT
+* was unmasked...
+*/
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it would
limit the eints to a level style handling. With the way it's
currently the whole determination of when to ack,mask and unmask is
completely for the real handler (edge or level) to decide, as the
original interrupt gets completely forwarded into the irq-domain
without further
constraints.
So, after the change on regular s3c24xx SoCs when the real irq
handler
wants to ack the irq, it would be a no-op, as it would already have
been acked by chained_irq_enter.
Masking might be even more interesting. Currently control is
transfered
completely to the pinctrl irq-domain, which then controls the
masking of the interrupt thru the parent-calls - on regular s3c24xx
the masking of these is also only done in the main controller.
When using chained_irq_* it also wants to mask the interrupt which
might conflict with regular enable_irq/disable_irq calls being done
for
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
On Wednesday 10 of April 2013 22:11:03 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 21:51:11 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 15:45:48 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments
about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other
Samsung
SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would
store
the
information about the s3c24xx-specific quirk in 24xx-specific
data
structure, without the need to add another field to the
generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the
wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs
and
make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight
speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank =
irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata = bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-
irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the
irq-handler
should stay in charge of when to ack it might be better this
way.
Generic s3c24xx SoCs need acking in the main controller only,
while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct
irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data =
irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack
callback
of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq = irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an
unmapped
EINT
+ * was unmasked...
+ */
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it
would
limit the eints to a level style handling. With the way it's
currently the whole determination of when to ack,mask and unmask
is
completely for the real handler (edge or level) to decide, as
the
original interrupt gets completely forwarded into the irq-domain
without further
constraints.
So, after the change on regular s3c24xx SoCs when the real irq
handler
wants to ack the irq, it would be a no-op, as it would already
have
been acked by chained_irq_enter.
Masking might be even more interesting. Currently control is
transfered
completely to the pinctrl irq-domain, which then controls the
masking of the interrupt thru the parent-calls - on regular
Re: [PATCH] pinctrl: Add pinctrl-s3c24xx driver
Am Mittwoch, 10. April 2013, 22:17:43 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 22:11:03 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 21:51:11 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 15:45:48 Heiko Stübner wrote:
Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
Hi Tomasz,
thanks for your comments, more inline.
Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
Hi Heiko,
Basically looks good to me, but please see my inline comments
about
handling of EINT0-3.
On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
The s3c24xx pins follow a similar pattern as the other
Samsung
SoCs
and
can therefore reuse the already introduced infrastructure.
[...]
+struct s3c24xx_eint_data {
+ struct samsung_pinctrl_drv_data *drvdata;
+ struct irq_domain *domains[NUM_EINT];
+ int parents[NUM_EINT_IRQ];
+};
+
+struct s3c24xx_eint_domain_data {
+ struct samsung_pin_bank *bank;
+ struct s3c24xx_eint_data *eint_data;
What about:
+ bool eint0_3_parent_only;
(or whatever name would be more appropriate), which would
store
the
information about the s3c24xx-specific quirk in 24xx-specific
data
structure, without the need to add another field to the
generic
samsung_pinctrl_drv_data structure?
See my further comments on how I would see using this field in
interrupt handling code.
ok, sounds good, especially gathering the type from the
wakeup-int
property
+};
[...]
Now I would split the following 3 functions into two sets of 3
functions, one set for s3c2412 and other for remaining SoCs
and
make
separate EINT0-3 IRQ chips for both cases.
Not doing the decision every time, might bring some very slight
speed
improvements, so is probably the right way to go.
+
+static void s3c24xx_eint0_3_ack(struct irq_data *data)
+{
+ struct samsung_pin_bank *bank =
irq_data_get_irq_chip_data(data);
+ struct samsung_pinctrl_drv_data *d = bank-drvdata;
+ struct s3c24xx_eint_domain_data *ddata =
bank-irq_domain-
host_data;
+ struct s3c24xx_eint_data *eint_data = ddata-eint_data;
+ int parent_irq = eint_data-parents[data-hwirq];
+ struct irq_chip *parent_chip = irq_get_chip(parent_irq);
+
+ if (d-ctrl-type == S3C2412) {
+ unsigned long bitval = 1UL data-hwirq;
+ writel(bitval, d-virt_base + EINTPEND_REG);
+ }
+
+ if (parent_chip-irq_ack)
+ parent_chip-
irq_ack(irq_get_irq_data(parent_irq));
Btw. Is this parent level acking really needed here?
Depends. If using chained_irq_* of course not, but if the
irq-handler
should stay in charge of when to ack it might be better this
way.
Generic s3c24xx SoCs need acking in the main controller only,
while
s3c2412 needs acking in both the main controller and eintpend.
+}
[...]
+static void s3c24xx_demux_eint0_3(unsigned int irq, struct
irq_desc
*desc) +{
+ struct irq_data *data = irq_desc_get_irq_data(desc);
+ struct s3c24xx_eint_data *eint_data =
irq_get_handler_data(irq);
+ unsigned int virq;
+
Instead of acking the interrupt at parent chip from ack
callback
of
EINT0_3 chip, I would rather use chained_irq_enter() here...
+ /* the first 4 eints have a simple 1 to 1 mapping */
+ virq =
irq_linear_revmap(eint_data-domains[data-hwirq],
data-hwirq); + /* Something must be really wrong if an
unmapped
EINT
+* was unmasked...
+*/
+ BUG_ON(!virq);
+
+ generic_handle_irq(virq);
...and chained_irq_exit() here.
If I understand it correctly, the way chained_irq_* works it
would
limit the eints to a level style handling. With the way it's
currently the whole determination of when to ack,mask and unmask
is
completely for the real handler (edge or level) to decide, as
the
original interrupt gets completely forwarded into the irq-domain
without further
constraints.
So, after the change on regular s3c24xx SoCs
