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
[PATCH] pinctrl: Add pinctrl-s3c24xx driver
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 +++ b/drivers/pinctrl/pinctrl-s3c24xx.c @@ -0,0 +1,603 @@ +/* + * Exynos specific support for Samsung pinctrl/gpiolib driver with eint support. + * + * Copyright (c) 2012 Samsung Electronics Co., Ltd. + * http://www.samsung.com + *