From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:00
This patch series is a work on improving usability and extensibiltiy of the
pinctrl-samsung driver. It consists of three main parts:
- moving SoC-specific data to device tree
- converting the driver to use one GPIO chip and one IRQ domain per pin bank
- introducing generic wake-up interrupt capability description
What the first part does is replacing static platform-specific data in
pinctrl-samsung driver with data dynamically parsed from device tree.
It aims at reducing the SoC-specific part of the driver and thus the
amount of modifications to driver sources when adding support for next
SoCs (like Exynos4x12).
In addition, it extends the description of SoC-specific attributes to cover all
mainlined Samsung SoCs, starting from s3c24xx and ending on latest Exynos5
series, with the exception of 4-bit banks with more than 8 pins on S3C64xx,
which will be covered by futher patch(es).
The second part attempts to simplify usage of the driver and fix several
problems of current implementation, in particular:
- Simplifies GPIO pin specification in device tree by using pin
namespace local to pin bank instead of local to pin controller, e.g.
gpios = <&gpj0 3 0>;
instead of
gpios = <&pinctrl0 115 0>;
- Simplifies GPIO interrupt specification in device tree by using
namespace local to pin bank (and equal to GPIO namespace), e.g.
interrupt-parent = <&gpj0>;
interrupts = <3 0>;
instead of
interrupt-parent = <&pinctrl0>;
interrupts = <115 0>;
- Simplifies internal GPIO pin to bank translation thanks to
correspondence of particular GPIO chips to pin banks. This allows
to remove the (costly in case of GPIO bit-banging drivers) lookup
over all banks to find the one that the pin is from.
Third part is focused on removing the static, hard-coded description of wake-up
interrupt controller and wake-up interrupt layout.
It defines a (mostly) generic (in scope of targetted SoCs) wake-up interrupt
layout specification format that allows to specify which pin banks support
wake-up interrupts and how they are handled (direct or multiplexed/chained).
See particular patches for more detailed descriptions and the last patch for
updated device tree bindings.
Tomasz Figa (16):
ARM: dts: exynos4210: Replace legacy GPIO bank nodes with pinctrl
bank nodes
pinctrl: exynos: Parse wakeup-eint parameters from DT
pinctrl: samsung: Detect and handle unsupported configuration types
pinctrl: samsung: Parse pin banks from DT
pinctrl: exynos: Remove static SoC-specific data
pinctrl: samsung: Parse bank-specific eint offset from DT
pinctrl: samsung: Hold OF node of pin bank in bank struct
pinctrl: samsung: Hold pointer to driver data in bank struct
pinctrl: exynos: Use one IRQ domain per pin bank
pinctrl: samsung: Do not pass gpio_chip to pin_to_reg_bank
pinctrl: samsung: Use one GPIO chip per pin bank
pinctrl: samsung: Use per-bank IRQ domain for wake-up interrupts
pinctrl: exynos: Set pin function to EINT in irq_set_type of wake-up
EINT
pinctrl: samsung: Parse offsets of particular registers from DT
pinctrl: samsung: Add GPIO to IRQ translation
Documentation: Update samsung-pinctrl device tree bindings
documentation
.../bindings/pinctrl/samsung-pinctrl.txt | 212 ++++++++--
arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi | 459 +++++++++++++++++++++
arch/arm/boot/dts/exynos4210-pinctrl.dtsi | 2 +
arch/arm/boot/dts/exynos4210.dtsi | 254 +-----------
drivers/pinctrl/pinctrl-exynos.c | 399 ++++++++----------
drivers/pinctrl/pinctrl-exynos.h | 184 +--------
drivers/pinctrl/pinctrl-samsung.c | 392 ++++++++++++++----
drivers/pinctrl/pinctrl-samsung.h | 87 ++--
8 files changed, 1205 insertions(+), 784 deletions(-)
create mode 100644 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
--
1.7.12
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:01
Seuqential patches from this series introduce SoC-specific data parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
Signed-off-by: Tomasz Figa <redacted>
---
arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi | 459 ++++++++++++++++++++++++
arch/arm/boot/dts/exynos4210-pinctrl.dtsi | 2 +
arch/arm/boot/dts/exynos4210.dtsi | 254 +------------
3 files changed, 474 insertions(+), 241 deletions(-)
create mode 100644 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:02
This patch converts the pinctrl-exynos driver to parse wakeup interrupt
count and register offsets from device tree. It reduces the amount of
static platform-specific data and facilitates adding further SoC
variants to pinctrl-samsung driver.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:03
This patch modifies the pinctrl-samsung driver to detect when width of a
bit field is set to zero (which means that such configuraton type is not
supported) and return an error instead of trying to modify an inexistent
register.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 3 +++
1 file changed, 3 insertions(+)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:04
Currently SoC-specific properties such as list of pin banks, register
offsets and bitfield sizes are being taken from static data structures
residing in pinctrl-exynos.c.
This patch modifies the pinctrl-samsung driver to parse all SoC-specific
data from device tree, which will allow to remove the static data
structures and facilitate adding of further SoC variants to the
pinctrl-samsung driver.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 5 ++
drivers/pinctrl/pinctrl-samsung.c | 169 +++++++++++++++++++++++++++++++++++++-
drivers/pinctrl/pinctrl-samsung.h | 17 +++-
3 files changed, 187 insertions(+), 4 deletions(-)
@@ -46,6 +46,8 @@ struct pin_config {{"samsung,pin-pud-pdn",PINCFG_TYPE_PUD_PDN},};+staticunsignedintpin_base=0;+/* check if the selector is a valid pin group selector */staticintsamsung_get_group_count(structpinctrl_dev*pctldev){
@@ -602,6 +604,8 @@ static int __init samsung_pinctrl_parse_dt(struct platform_device *pdev,u32function;if(of_find_property(cfg_np,"interrupt-controller",NULL))continue;+if(of_find_property(cfg_np,"gpio-controller",NULL))+continue;ret=samsung_pinctrl_parse_dt_pins(pdev,cfg_np,&drvdata->pctl,&pin_list,&npins);
@@ -778,6 +782,86 @@ static int __init samsung_gpiolib_unregister(struct platform_device *pdev,staticconststructof_device_idsamsung_pinctrl_dt_match[];+staticintsamsung_pinctrl_parse_dt_bank_type(structsamsung_pin_bank*bank,+structdevice_node*np)+{+structsamsung_pin_bank*type=np->data;+intret;+u32val;++if(type){+*bank=*type;+return0;+}++type=kzalloc(sizeof(*type),GFP_KERNEL);+if(!type)+return-ENOMEM;++ret=of_property_read_u32(np,"samsung,func-width",&val);+if(ret)+returnret;+type->func_width=val;++ret=of_property_read_u32(np,"samsung,pud-width",&val);+if(!ret)+type->pud_width=val;++ret=of_property_read_u32(np,"samsung,drv-width",&val);+if(!ret)+type->drv_width=val;++ret=of_property_read_u32(np,"samsung,conpdn-width",&val);+if(!ret)+type->conpdn_width=val;++ret=of_property_read_u32(np,"samsung,pudpdn-width",&val);+if(!ret)+type->pudpdn_width=val;++*bank=*type;+np->data=type;++return0;+}++staticintsamsung_pinctrl_parse_dt_bank(structsamsung_pin_bank*bank,+structdevice_node*np)+{+intret;+u32val;+structdevice_node*type_np;++type_np=of_parse_phandle(np,"samsung,bank-type",0);+if(!type_np)+return-EINVAL;++ret=samsung_pinctrl_parse_dt_bank_type(bank,type_np);+if(ret)+returnret;++ret=of_property_read_u32(np,"samsung,pctl-offset",&val);+if(ret)+returnret;+bank->pctl_offset=val;++ret=of_property_read_u32(np,"samsung,pin-count",&val);+if(ret)+returnret;+bank->nr_pins=val;++bank->name=np->name;++if(!of_find_property(np,"interrupt-controller",NULL)){+bank->eint_type=EINT_TYPE_NONE;+return0;+}++bank->eint_type=EINT_TYPE_GPIO;++return0;+}+/* retrieve the soc specific data */staticstructsamsung_pin_ctrl*samsung_pinctrl_get_soc_data(structplatform_device*pdev)
@@ -235,5 +247,6 @@ struct samsung_pmx_func {/* list of all exported SoC specific data */externstructsamsung_pin_ctrlexynos4210_pin_ctrl[];+externstructsamsung_pin_ctrl_variantexynos4_pin_ctrl;#endif /* __PINCTRL_SAMSUNG_H */
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:05
The patch "pinctrl: samsung: Parse pin banks from DT" introduced parsing
SoC-specific data from device tree, so there is no need to keep the
previously used definitions in headers and source files.
This patch cleans up the pinctrl-exynos driver from unused
SoC-specific data.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 96 -----------------------
drivers/pinctrl/pinctrl-exynos.h | 157 --------------------------------------
drivers/pinctrl/pinctrl-samsung.h | 1 -
3 files changed, 254 deletions(-)
@@ -246,7 +246,6 @@ struct samsung_pmx_func {};/* list of all exported SoC specific data */-externstructsamsung_pin_ctrlexynos4210_pin_ctrl[];externstructsamsung_pin_ctrl_variantexynos4_pin_ctrl;#endif /* __PINCTRL_SAMSUNG_H */
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:06
Some SoCs, like Exynos4x12, have non-sequential layout of EINT control
registers and so current way of calculating register addresses does not
work correctly for them.
This patch adds parsing of samsung,eint-offset property from bank nodes
and uses the read values instead of calculating the offsets from bank
index.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 5 ++---
drivers/pinctrl/pinctrl-samsung.c | 5 +++++
drivers/pinctrl/pinctrl-samsung.h | 1 +
3 files changed, 8 insertions(+), 3 deletions(-)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:07
The node pointer will be used in extensions added by patches that will
follow.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 1 +
drivers/pinctrl/pinctrl-samsung.h | 3 +++
2 files changed, 4 insertions(+)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:08
The pointer will be used by further extensions added to the driver.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 20 +++++++++++---------
drivers/pinctrl/pinctrl-samsung.h | 2 ++
2 files changed, 13 insertions(+), 9 deletions(-)
@@ -870,11 +870,12 @@ static int samsung_pinctrl_parse_dt_bank(struct samsung_pin_bank *bank,/* retrieve the soc specific data */staticstructsamsung_pin_ctrl*samsung_pinctrl_get_soc_data(-structplatform_device*pdev)+structplatform_device*pdev,+structsamsung_pinctrl_drv_data*d){intid;conststructof_device_id*match;-conststructdevice_node*node=pdev->dev.of_node;+structdevice_node*node=pdev->dev.of_node;structdevice_node*bank_np;structsamsung_pin_ctrl*ctrl;structsamsung_pin_bank*banks,*b;
@@ -884,7 +885,7 @@ static struct samsung_pin_ctrl *samsung_pinctrl_get_soc_data(u32val;intret;-id=of_alias_get_id(pdev->dev.of_node,"pinctrl");+id=of_alias_get_id(node,"pinctrl");if(id<0){dev_err(&pdev->dev,"failed to get alias id\n");returnNULL;
@@ -979,18 +981,18 @@ static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)return-ENODEV;}-ctrl=samsung_pinctrl_get_soc_data(pdev);-if(!ctrl){-dev_err(&pdev->dev,"driver data not available\n");-return-EINVAL;-}-drvdata=devm_kzalloc(dev,sizeof(*drvdata),GFP_KERNEL);if(!drvdata){dev_err(dev,"failed to allocate memory for driver's ""private data\n");return-ENOMEM;}++ctrl=samsung_pinctrl_get_soc_data(pdev,drvdata);+if(!ctrl){+dev_err(&pdev->dev,"driver data not available\n");+return-EINVAL;+}drvdata->ctrl=ctrl;drvdata->dev=dev;
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:09
Instead of registering one IRQ domain for all pin banks of a pin
controller, this patch implements registration of per-bank domains.
At a cost of a little memory overhead (~2.5KiB for all GPIO interrupts
of Exynos4x12) it simplifies driver code and device tree sources,
because GPIO interrupts can be now specified per banks.
Example:
device {
/* ... */
interrupt-parent = <&gpa1>;
interrupts = <3 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 117 +++++++++++---------------------------
drivers/pinctrl/pinctrl-exynos.h | 12 ----
drivers/pinctrl/pinctrl-samsung.c | 5 +-
drivers/pinctrl/pinctrl-samsung.h | 7 +--
4 files changed, 36 insertions(+), 105 deletions(-)
@@ -114,7 +114,7 @@ static int exynos_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)writel(con,d->virt_base+reg_con);reg_con=bank->pctl_offset;-shift=edata->pin*bank->func_width;+shift=pin*bank->func_width;mask=(1<<bank->func_width)-1;con=readl(d->virt_base+reg_con);
@@ -136,81 +136,23 @@ static struct irq_chip exynos_gpio_irq_chip = {.irq_set_type=exynos_gpio_irq_set_type,};-/*-*givenacontroller-localexternalgpiointerruptnumber,preparethehandler-*dataforit.-*/-staticstructexynos_geint_data*exynos_get_eint_data(irq_hw_number_thw,-structsamsung_pinctrl_drv_data*d)-{-structsamsung_pin_bank*bank=d->ctrl->pin_banks;-structexynos_geint_data*eint_data;-unsignedintnr_banks=d->ctrl->nr_banks,idx;-unsignedintirq_base=0;--if(hw>=d->ctrl->nr_gint){-dev_err(d->dev,"unsupported ext-gpio interrupt\n");-returnNULL;-}--for(idx=0;idx<nr_banks;idx++,bank++){-if(bank->eint_type!=EINT_TYPE_GPIO)-continue;-if((hw>=irq_base)&&(hw<(irq_base+bank->nr_pins)))-break;-irq_base+=bank->nr_pins;-}--if(idx==nr_banks){-dev_err(d->dev,"pin bank not found for ext-gpio interrupt\n");-returnNULL;-}--eint_data=devm_kzalloc(d->dev,sizeof(*eint_data),GFP_KERNEL);-if(!eint_data){-dev_err(d->dev,"no memory for eint-gpio data\n");-returnNULL;-}--eint_data->bank=bank;-eint_data->pin=hw-irq_base;-eint_data->eint_offset=bank->eint_offset;-returneint_data;-}-staticintexynos_gpio_irq_map(structirq_domain*h,unsignedintvirq,irq_hw_number_thw){-structsamsung_pinctrl_drv_data*d=h->host_data;-structexynos_geint_data*eint_data;--eint_data=exynos_get_eint_data(hw,d);-if(!eint_data)-return-EINVAL;+structsamsung_pin_bank*b=h->host_data;-irq_set_handler_data(virq,eint_data);-irq_set_chip_data(virq,h->host_data);+irq_set_chip_data(virq,b);irq_set_chip_and_handler(virq,&exynos_gpio_irq_chip,handle_level_irq);set_irq_flags(virq,IRQF_VALID);return0;}-staticvoidexynos_gpio_irq_unmap(structirq_domain*h,unsignedintvirq)-{-structsamsung_pinctrl_drv_data*d=h->host_data;-structexynos_geint_data*eint_data;--eint_data=irq_get_handler_data(virq);-devm_kfree(d->dev,eint_data);-}-/**irqdomaincallbacksforexternalgpiointerruptcontroller.*/staticconststructirq_domain_opsexynos_gpio_irqd_ops={.map=exynos_gpio_irq_map,-.unmap=exynos_gpio_irq_unmap,.xlate=irq_domain_xlate_twocell,};
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:10
The pointer to gpio_chip passed to pin_to_reg_bank utility function is
used only to retrieve a pointer to samsung_pinctrl_drv_data structure.
This patch modifies the function and its users to pass a pointer to
samsung_pinctrl_drv_data directly.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:11
This patch modifies the pinctrl-samsung driver to register one GPIO chip
per pin bank, instead of a single chip for all pin banks of the
controller.
It simplifies GPIO accesses a lot (constant time instead of looping
through the list of banks to find the right one) and should have a good
effect on performance of any bit-banging driver.
In addition it allows to reference GPIO pins by a phandle to the bank
node and a local pin offset inside of the bank (similar to previous
gpiolib driver), which is more clear and readable than using indices
relative to the whole pin controller.
Example:
device {
/* ... */
gpios = <&gpk0 4 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 119 ++++++++++++++++++++++++--------------
drivers/pinctrl/pinctrl-samsung.h | 12 ++--
2 files changed, 81 insertions(+), 50 deletions(-)
@@ -48,6 +48,11 @@ struct pin_config {staticunsignedintpin_base=0;+staticinlinestructsamsung_pin_bank*gc_to_pin_bank(structgpio_chip*gc)+{+returncontainer_of(gc,structsamsung_pin_bank,gpio_chip);+}+/* check if the selector is a valid pin group selector */staticintsamsung_get_group_count(structpinctrl_dev*pctldev){
@@ -333,9 +338,12 @@ static int samsung_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,void__iomem*reg;u32data,pin_offset,mask,shift;+bank=gc_to_pin_bank(range->gc);drvdata=pinctrl_dev_get_drvdata(pctldev);-pin_to_reg_bank(drvdata,offset,®,&pin_offset,&bank);+pin_offset=offset-bank->pin_base;+reg=drvdata->virt_base+bank->pctl_offset;+mask=(1<<bank->func_width)-1;shift=pin_offset*bank->func_width;
@@ -23,6 +23,8 @@#include<linux/pinctrl/consumer.h>#include<linux/pinctrl/machine.h>+#include<linux/gpio.h>+/* register offsets within a pin bank */#define DAT_REG 0x4#define PUD_REG 0x8
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:12
This patch reworks wake-up interrupt handling in pinctrl-exynos driver,
so each pin bank, which provides wake-up interrupts, has its own IRQ
domain.
Information about whether given pin bank provides wake-up interrupts,
how many and whether they are separate or muxed are parsed from device
tree.
It gives following advantages:
- interrupts can be specified in device tree in a more readable way,
e.g. :
device {
/* ... */
interrupt-parent = <&gpx2>;
interrupts = <4 0>;
/* ... */
};
- the amount and layout of interrupts is not hardcoded in the driver
anymore
- bank and pin of each wake-up interrupt can be easily identified, to
allow operations, such as setting the pin to EINT function, from
irq_set_type() callback
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 159 +++++++++++++++++++++++---------------
drivers/pinctrl/pinctrl-exynos.h | 17 +++-
drivers/pinctrl/pinctrl-samsung.c | 9 ++-
drivers/pinctrl/pinctrl-samsung.h | 6 +-
4 files changed, 119 insertions(+), 72 deletions(-)
@@ -381,7 +383,11 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)structdevice*dev=d->dev;structdevice_node*wkup_np=NULL;structdevice_node*np;+structsamsung_pin_bank*bank;structexynos_weint_data*weint_data;+structexynos_muxed_weint_data*muxed_data;+unsignedintmuxed_banks=0;+unsignedinti;intidx,irq;u32val;intret;
@@ -395,11 +401,6 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)if(!wkup_np)return-ENODEV;-ret=of_property_read_u32(wkup_np,"samsung,weint-count",&val);-if(ret)-return-EINVAL;-d->ctrl->nr_wint=val;-ret=of_property_read_u32(wkup_np,"samsung,weint-con",&val);if(ret)return-EINVAL;
@@ -415,40 +416,74 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)return-EINVAL;d->ctrl->weint_pend=val;-d->wkup_irqd=irq_domain_add_linear(wkup_np,d->ctrl->nr_wint,-&exynos_wkup_irqd_ops,d);-if(!d->wkup_irqd){-dev_err(dev,"wakeup irq domain allocation failed\n");-return-ENXIO;-}+bank=d->ctrl->pin_banks;+for(i=0;i<d->ctrl->nr_banks;++i,++bank){+if(bank->eint_type!=EINT_TYPE_WKUP)+continue;-weint_data=devm_kzalloc(dev,sizeof(*weint_data)*17,GFP_KERNEL);-if(!weint_data){-dev_err(dev,"could not allocate memory for weint_data\n");-return-ENOMEM;-}+bank->irq_domain=irq_domain_add_linear(bank->of_node,+bank->nr_pins,&exynos_wkup_irqd_ops,bank);+if(!bank->irq_domain){+dev_err(dev,"wkup irq domain add failed\n");+return-ENXIO;+}-irq=irq_of_parse_and_map(wkup_np,16);-if(irq){-weint_data[16].domain=d->wkup_irqd;-irq_set_chained_handler(irq,exynos_irq_demux_eint16_31);-irq_set_handler_data(irq,&weint_data[16]);-}else{-dev_err(dev,"irq number for EINT16-32 not found\n");-}+if(!of_find_property(bank->of_node,"interrupts",NULL)){+bank->eint_type=EINT_TYPE_WKUP_MUX;+++muxed_banks;+continue;+}-for(idx=0;idx<16;idx++){-weint_data[idx].domain=d->wkup_irqd;-weint_data[idx].irq=idx;+weint_data=devm_kzalloc(dev,bank->nr_pins+*sizeof(*weint_data),GFP_KERNEL);+if(!weint_data){+dev_err(dev,"could not allocate memory for weint_data\n");+return-ENOMEM;+}-irq=irq_of_parse_and_map(wkup_np,idx);-if(irq){+for(idx=0;idx<bank->nr_pins;++idx){+irq=irq_of_parse_and_map(bank->of_node,idx);+if(!irq){+dev_err(dev,"irq number for eint-%s-%d not found\n",+bank->name,idx);+continue;+}+weint_data[idx].irq=idx;+weint_data[idx].bank=bank;irq_set_handler_data(irq,&weint_data[idx]);irq_set_chained_handler(irq,exynos_irq_eint0_15);-}else{-dev_err(dev,"irq number for eint-%x not found\n",idx);}}++if(!muxed_banks)+return0;++irq=irq_of_parse_and_map(wkup_np,0);+if(!irq){+dev_err(dev,"irq number for muxed EINTs not found\n");+return0;+}++muxed_data=devm_kzalloc(dev,sizeof(*muxed_data)++muxed_banks*sizeof(structsamsung_pin_bank*),GFP_KERNEL);+if(!muxed_data){+dev_err(dev,"could not allocate memory for muxed_data\n");+return-ENOMEM;+}++irq_set_chained_handler(irq,exynos_irq_demux_eint16_31);+irq_set_handler_data(irq,muxed_data);++bank=d->ctrl->pin_banks;+idx=0;+for(i=0;i<d->ctrl->nr_banks;++i,++bank){+if(bank->eint_type!=EINT_TYPE_WKUP_MUX)+continue;++muxed_data->banks[idx++]=bank;+}+muxed_data->nr_banks=muxed_banks;+return0;}
@@ -78,6 +79,7 @@ enum eint_type {EINT_TYPE_NONE,EINT_TYPE_GPIO,EINT_TYPE_WKUP,+EINT_TYPE_WKUP_MUX,};/* maximum length of a pin in pin descriptor (example: "gpa0-0") */
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:13
Pins used as wake-up interrupts need to be configured as EINTs. This
patch adds the required configuration code to exynos_wkup_irq_set_type,
to set the pin as EINT when its interrupt trigger type is configured.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 11 +++++++++++
1 file changed, 11 insertions(+)
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:14
The order and availability of pin control registers vary with SoC.
This patch modifies the driver to parse register offsets from device
tree as a part of bank type definition.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-exynos.c | 12 ++---
drivers/pinctrl/pinctrl-samsung.c | 93 +++++++++++++++++++++------------------
drivers/pinctrl/pinctrl-samsung.h | 38 +++++++++-------
3 files changed, 78 insertions(+), 65 deletions(-)
@@ -273,10 +273,6 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,*offset=pin-b->pin_base;if(bank)*bank=b;--/* some banks have two config registers in a single bank */-if(*offset*b->func_width>BITS_PER_LONG)-*reg+=4;}/* enable or disable a pinmux function */
@@ -25,12 +25,16 @@#include<linux/gpio.h>-/* register offsets within a pin bank */-#define DAT_REG 0x4-#define PUD_REG 0x8-#define DRV_REG 0xC-#define CONPDN_REG 0x10-#define PUDPDN_REG 0x14+enumpincfg_reg{+REG_FUNC=0,+REG_DAT,+REG_PUD,+REG_DRV,+REG_CONPDN,+REG_PUDPDN,++REG_NUM+};/* pinmux function number for pin as gpio output line */#define FUNC_OUTPUT 0x1
From: Tomasz Figa <hidden> Date: 2012-10-08 08:39:15
Some drivers require a way to translate GPIO pins to their IRQ numbers.
This patch adds the .to_irq() gpiolib callback to pinctrl-samsung
driver, which creates (if not present yet) and returns an IRQ mapping
for given GPIO pin.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
@@ -13,8 +13,35 @@ Required Properties: - reg: Base address of the pin controller hardware module and length of the address space it occupies.-- interrupts: interrupt specifier for the controller. The format and value of- the interrupt specifier depends on the interrupt parent for the controller.+- Pin bank types: Pin bank nodes reference pin bank types by their phandles to+ determine low level bank parameters such as bit field availability and widths.+ There is no restriction for placement of such nodes. A bank type node must+ contain following properties:++ - samsung,reg-names: names of registers specified in samsung,reg-params+ property. Allowed register names are:+ - "func" - function configuration register (GPxCON)+ - "dat" - input/output value register (GPxDAT)+ - "pud" - pull-up/-down control register (GPxPUD)+ - "drv" - driver strength control register (GPxDRV)+ - "conpdn" - power-down state control register (GPxCONPDN)+ - "pudpdn" - power-down pull-up/-down control register (GPxPUDPDN)+ - samsung,reg-params: register specifiers for registers named by+ samsung,reg-names property. Each specifier contains two cells:+ - first cell: offset in bytes from first register of the bank+ - second cell: bits in register used for single pin.++- Pin banks as child nodes: Pin banks of the controller are represented by child+ nodes of the controller node. Bank name is taken from name of the node. Each+ bank node must contain following properties:++ - gpio-controller: identifies the node as a gpio controller and pin bank.+ - samsung,pctrl-offset: offset to pin control registers of the bank.+ - samsung,pin-count: number of pins in the bank.+ - samsung,bank-type: phandle to a node defining bank type.+ - #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO+ binding is used, the amount of cells must be specified as 2. See generic+ GPIO binding documentation for description of particular cells. - Pin mux/config groups as child nodes: The pin mux (selecting pin function mode) and pin config (pull up/down, driver strength) settings are represented
@@ -72,16 +99,30 @@ used as system wakeup events. A. External GPIO Interrupts: For supporting external gpio interrupts, the following properties should be specified in the pin-controller device node.-- interrupt-controller: identifies the controller node as interrupt-parent.-- #interrupt-cells: the value of this property should be 2.- - First Cell: represents the external gpio interrupt number local to the- external gpio interrupt space of the controller.- - Second Cell: flags to identify the type of the interrupt- - 1 = rising edge triggered- - 2 = falling edge triggered- - 3 = rising and falling edge triggered- - 4 = high level triggered- - 8 = low level triggered+ - samsung,geint-con: offset of first EXT_INTxx_CON register.+ - samsung,geint-mask: offset of first EXT_INTxx_MASK register.+ - samsung,geint-pend: offset of first EXT_INTxx_PEND register.+ - samsung,svc: offset of EXT_INT_SERVICE register.+ - interrupt-parent: phandle of the interrupt parent to which the external+ GPIO interrupts are forwarded to.+ - interrupts: interrupt specifier for the controller. The format and value of+ the interrupt specifier depends on the interrupt parent for the controller.++ In addition, following properties must be present in node of every bank+ of pins supporting GPIO interrupts:++ - interrupt-controller: identifies the controller node as interrupt-parent.+ - samsung,eint-offset: offset of register related to this bank from first+ CON/MASK/PEND register.+ - #interrupt-cells: the value of this property should be 2.+ - First Cell: represents the external gpio interrupt number local to the+ external gpio interrupt space of the controller.+ - Second Cell: flags to identify the type of the interrupt+ - 1 = rising edge triggered+ - 2 = falling edge triggered+ - 3 = rising and falling edge triggered+ - 4 = high level triggered+ - 8 = low level triggered B. External Wakeup Interrupts: For supporting external wakeup interrupts, a child node representing the external wakeup interrupt controller should be
@@ -94,7 +135,18 @@ B. External Wakeup Interrupts: For supporting external wakeup interrupts, a found on Samsung Exynos4210 SoC. - interrupt-parent: phandle of the interrupt parent to which the external wakeup interrupts are forwarded to.+ - interrupts: interrupt used by multiplexed wakeup interrupts.+ - samsung,weint-con: offset of first wake-up EXT_INTxx_CON register.+ - samsung,weint-mask: offset of first wake-up EXT_INTxx_MASK register.+ - samsung,weint-pend: offset of first wake-up EXT_INTxx_PEND register.++ In addition, following properties must be present in node of every bank+ of pins supporting wake-up interrupts:+ - interrupt-controller: identifies the node as interrupt-parent.+ - samsung,wkup-eint: marks the bank as supporting wake-up interrupts.+ - samsung,eint-offset: offset of register related to this bank from first+ wake-up CON/MASK/PEND register. - #interrupt-cells: the value of this property should be 2 - First Cell: represents the external wakeup interrupt number local to the external wakeup interrupt space of the controller.
@@ -105,17 +157,125 @@ B. External Wakeup Interrupts: For supporting external wakeup interrupts, a - 4 = high level triggered - 8 = low level triggered+ Node of every bank of pins supporting direct wake-up interrupts (without+ multiplexing) must contain following properties:++ - interrupt-parent: phandle of the interrupt parent to which the external+ wakeup interrupts are forwarded to.+ - interrupts: interrupts of the interrupt parent which are used for external+ wakeup interrupts from pins of the bank, must contain interrupts for all+ pins of the bank.+ Aliases: All the pin controller nodes should be represented in the aliases node using the following format 'pinctrl{n}' where n is a unique number for the alias.+Example: Nodes for pin bank types:++ pinctrl-bank-types {+ bank_off: bank-off {+ samsung,reg-names = "func", "dat", "pud",+ "drv", "conpdn", "pudpdn";+ samsung,reg-params = <0x00 4>, <0x04 1>, <0x08 2>,+ <0x0C 2>, <0x10 2>, <0x14 2>;+ };++ bank_alive: bank-alive {+ samsung,reg-names = "func", "dat", "pud",+ "drv";+ samsung,reg-params = <0x00 4>, <0x04 1>, <0x08 2>,+ <0x0C 2>;+ };+ };++Example: A pin-controller node with pin banks:++ pinctrl_0: pinctrl at 11400000 {+ compatible = "samsung,pinctrl-exynos4210";+ reg = <0x11400000 0x1000>;+ interrupts = <0 47 0>;+ samsung,geint-con = <0x700>;+ samsung,geint-mask = <0x900>;+ samsung,geint-pend = <0xA00>;+ samsung,svc = <0xB08>;++ /* ... */++ /* Pin bank without external interrupts */+ gpy0: gpy0 {+ gpio-controller;+ samsung,pctl-offset = <0x120>;+ samsung,pin-count = <6>;+ samsung,bank-type = <&bank_off>;+ #gpio-cells = <2>;+ };++ /* ... */++ /* Pin bank with external GPIO interrupts */+ gpj0: gpj0 {+ gpio-controller;+ samsung,pctl-offset = <0x000>;+ samsung,pin-count = <8>;+ samsung,bank-type = <&bank_off>;+ #gpio-cells = <2>;++ interrupt-controller;+ samsung,eint-offset = <0x00>;+ #interrupt-cells = <2>;+ };++ /* ... */++ /* Pin bank with external direct wake-up interrupts */+ gpx0: gpx0 {+ gpio-controller;+ samsung,pctl-offset = <0xC00>;+ samsung,pin-count = <8>;+ samsung,bank-type = <&bank_alive>;+ #gpio-cells = <2>;++ interrupt-controller;+ samsung,wkup-eint;+ interrupt-parent = <&gic>;+ interrupts = <0 16 0>, <0 17 0>, <0 18 0>, <0 19 0>,+ <0 20 0>, <0 21 0>, <0 22 0>, <0 23 0>;+ samsung,eint-offset = <0x00>;+ #interrupt-cells = <2>;+ };++ /* ... */++ /* Pin bank with external multiplexed wake-up interrupts */+ gpx2: gpx2 {+ gpio-controller;+ samsung,pctl-offset = <0xC40>;+ samsung,pin-count = <8>;+ samsung,bank-type = <&bank_alive>;+ #gpio-cells = <2>;++ interrupt-controller;+ samsung,wkup-eint;+ samsung,eint-offset = <0x08>;+ #interrupt-cells = <2>;+ };++ /* ... */+ };+ Example 1: A pin-controller node with pin groups. pinctrl_0: pinctrl at 11400000 { compatible = "samsung,pinctrl-exynos4210"; reg = <0x11400000 0x1000>; interrupts = <0 47 0>;+ samsung,geint-con = <0x700>;+ samsung,geint-mask = <0x900>;+ samsung,geint-pend = <0xA00>;+ samsung,svc = <0xB08>;++ /* ... */ uart0_data: uart0-data { samsung,pins = "gpa0-0", "gpa0-1";
@@ -190,7 +351,8 @@ Example 4: Set up the default pin state for uart controller. static int s3c24xx_serial_probe(struct platform_device *pdev) { struct pinctrl *pinctrl;- ...- ...++ /* ... */+ pinctrl = devm_pinctrl_get_select_default(&pdev->dev); }
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
This patch converts the pinctrl-exynos driver to parse wakeup interrupt
count and register offsets from device tree. It reduces the amount of
static platform-specific data and facilitates adding further SoC
variants to pinctrl-samsung driver.
So these are:
+ ret = of_property_read_u32(wkup_np, "samsung,weint-count", &val);
+ ret = of_property_read_u32(wkup_np, "samsung,weint-con", &val);
+ ret = of_property_read_u32(wkup_np, "samsung,weint-mask", &val);
+ ret = of_property_read_u32(wkup_np, "samsung,weint-pend", &val);
Are these all four register offsets?
I don't think it's proper for the device tree to contain register offsets.
Base address, "regs" property, yes. Individual registers, no. That just
makes the code hard to read and compare to the datasheet.
Or what are you aiming at here?
Linus Walleij
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
Seuqential patches from this series introduce SoC-specific data parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
We really need to discuss this, Grant has already NACK:ed
such approaches once.
If you're still going to do this, it is mandatory
to NOT use magic hex numbers anymore, because Stephen has
merged preprocessor support to the DTC compiler so you
can use #defined macros.
See commit:
cd296721a9645f9f28800a072490fa15458d1fb7
This property is *NOT* OK. IMHO the driver should know these
offsets, not the device tree. The driver only needs the offset to
the register range, what registers there are and their names
should be #defined.
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
Currently SoC-specific properties such as list of pin banks, register
offsets and bitfield sizes are being taken from static data structures
residing in pinctrl-exynos.c.
This patch modifies the pinctrl-samsung driver to parse all SoC-specific
data from device tree, which will allow to remove the static data
structures and facilitate adding of further SoC variants to the
pinctrl-samsung driver.
So why? Two approaches:
- Put as much info as possible into the device tree
- Put as much info as possible into the driver
The first approach is currently only used by pinctrl-single.c.
That driver is designed for the case where all info about
the hardware arrives in some description language that
can be translated into a simple DT description.
If you want to use that approach, you should use that
driver. If that driver does not work for you, then it's not
fulfilling it's purpose as a one-stop shop for simple
pin controllers entirely contained within the device tree,
and should be renamed or redesigned.
If you will end up with a hybrid approach with some
stuff in the device tree and some stuff in the code,
it's better to keep the old driver.
Yours,
Linus Walleij
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted hunk
This patch modifies the pinctrl-samsung driver to detect when width of a
bit field is set to zero (which means that such configuraton type is not
supported) and return an error instead of trying to modify an inexistent
register.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 3 +++
1 file changed, 3 insertions(+)
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
Instead of registering one IRQ domain for all pin banks of a pin
controller, this patch implements registration of per-bank domains.
At a cost of a little memory overhead (~2.5KiB for all GPIO interrupts
of Exynos4x12) it simplifies driver code and device tree sources,
because GPIO interrupts can be now specified per banks.
Example:
device {
/* ... */
interrupt-parent = <&gpa1>;
interrupts = <3 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
This looks like a very good patch!
Can it be applied in isolation from the other patches?
Thomas A: can you ACK this?
Yours,
Linus Walleij
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
The pointer to gpio_chip passed to pin_to_reg_bank utility function is
used only to retrieve a pointer to samsung_pinctrl_drv_data structure.
This patch modifies the function and its users to pass a pointer to
samsung_pinctrl_drv_data directly.
Signed-off-by: Tomasz Figa <redacted>
Looks good, can it be applied without the others?
Maybe you can make a patch series without all the
stuff moving register offsets to the DT so I can begin
with merging that and we can discuss the movement
of register info separately?
Thomas A: is this ACK:able?
Yours,
Linus Walleij
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
This patch modifies the pinctrl-samsung driver to register one GPIO chip
per pin bank, instead of a single chip for all pin banks of the
controller.
It simplifies GPIO accesses a lot (constant time instead of looping
through the list of banks to find the right one) and should have a good
effect on performance of any bit-banging driver.
In addition it allows to reference GPIO pins by a phandle to the bank
node and a local pin offset inside of the bank (similar to previous
gpiolib driver), which is more clear and readable than using indices
relative to the whole pin controller.
Example:
device {
/* ... */
gpios = <&gpk0 4 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
This also looks good (and I think it has been discussed before)
so needs to be applied in isolation from the regs-to-DT stuff.
Yours,
Linus Walleij
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
This patch series is a work on improving usability and extensibiltiy of the
pinctrl-samsung driver. It consists of three main parts:
- moving SoC-specific data to device tree
- converting the driver to use one GPIO chip and one IRQ domain per pin bank
- introducing generic wake-up interrupt capability description
So can you prepare a patch series which does all but the first
bullet to begin with, and a SoC-and register offset patch
on top of that as a separate series, because it is controversial?
I don't like that these two things are mingled together like this
in an all-or-nothing manner.
So I'm OK with a patch series for bulle (2) and (3) but not (1).
And I'd like to have Thomas A:s ACK on the series too.
Yours,
Linus Walleij
From: Tomasz Figa <hidden> Date: 2012-10-10 08:20:07
On Wednesday 10 of October 2012 09:26:51 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
Seuqential patches from this series introduce SoC-specific data parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
I'm not sure if I understood you correctly, so let me explain how this
works.
Each specifier defines register offset inside bank registers and how many
bits are used for one pin in this register to specify configuration value.
E.g. func register is available at offset 0x00 and pin 0 occupies bits 0-3,
pin 1 bit 4-7, etc.
We really need to discuss this, Grant has already NACK:ed
such approaches once.
If you're still going to do this, it is mandatory
to NOT use magic hex numbers anymore, because Stephen has
merged preprocessor support to the DTC compiler so you
can use #defined macros.
See commit:
cd296721a9645f9f28800a072490fa15458d1fb7
That's definitely nice. I have seen the work going on this before, but
haven't followed it recently. Good to know that now it can be used.
This property is *NOT* OK. IMHO the driver should know these
offsets, not the device tree. The driver only needs the offset to
the register range, what registers there are and their names
should be #defined.
This is an offset inside of EINT register group. EINT registers are
organized in groups as following:
EINT_CON_0
EINT_CON_1
...
EINT_CON_N
...
EINT_MASK_0
EINT_MASK_1
...
EINT_MASK_N
...
EINT_PEND_0
EINT_PEND_1
...
EINT_PEND_N
With arbitrary order of particular groups, arbitrary space between groups
and arbitrary mapping of particular registers to pin banks, although the
mapping is the same for all groups of registers, that's why there is only
one eint-offset property. Also holes (reserved/unused registers) inside
groups might exist.
So if we want to access EINT_MASK register of bank A0 (of pinctrl 0), we
must construct the address as following:
eint_mask_a0 = pinctrl_0_base + pinctrl_0_geint_mask + bank_a0_eint_offset
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 08:23:53
On Wednesday 10 of October 2012 09:18:51 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
This patch converts the pinctrl-exynos driver to parse wakeup interrupt
count and register offsets from device tree. It reduces the amount of
static platform-specific data and facilitates adding further SoC
variants to pinctrl-samsung driver.
So these are:
quoted
+ ret = of_property_read_u32(wkup_np, "samsung,weint-count",
&val); + ret = of_property_read_u32(wkup_np,
"samsung,weint-con", &val); + ret =
of_property_read_u32(wkup_np, "samsung,weint-mask", &val); + ret
= of_property_read_u32(wkup_np, "samsung,weint-pend", &val);
Are these all four register offsets?
I don't think it's proper for the device tree to contain register
offsets.
Base address, "regs" property, yes. Individual registers, no. That just
makes the code hard to read and compare to the datasheet.
Or what are you aiming at here?
See my reply to your comments for patch 1. I think it should explain how
these values are used.
One thing worth mentioning is that registers for GPIO interrupts and wakeup
interrupts can be located at different areas of pin controller address
space, so these offsets have to be specified for both (using geint-* and
weint-* properties).
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 08:25:06
On Wednesday 10 of October 2012 09:37:42 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
This patch modifies the pinctrl-samsung driver to detect when width of
a
bit field is set to zero (which means that such configuraton type is
not
supported) and return an error instead of trying to modify an
inexistent
register.
Signed-off-by: Tomasz Figa <redacted>
---
drivers/pinctrl/pinctrl-samsung.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-samsung.c
b/drivers/pinctrl/pinctrl-samsung.c index dd108a9..c660fa5 100644
From: Tomasz Figa <hidden> Date: 2012-10-10 08:39:30
On Wednesday 10 of October 2012 09:34:05 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
Currently SoC-specific properties such as list of pin banks, register
offsets and bitfield sizes are being taken from static data structures
residing in pinctrl-exynos.c.
This patch modifies the pinctrl-samsung driver to parse all
SoC-specific
data from device tree, which will allow to remove the static data
structures and facilitate adding of further SoC variants to the
pinctrl-samsung driver.
So why? Two approaches:
- Put as much info as possible into the device tree
- Put as much info as possible into the driver
The first approach is currently only used by pinctrl-single.c.
That driver is designed for the case where all info about
the hardware arrives in some description language that
can be translated into a simple DT description.
If you want to use that approach, you should use that
driver. If that driver does not work for you, then it's not
fulfilling it's purpose as a one-stop shop for simple
pin controllers entirely contained within the device tree,
and should be renamed or redesigned.
If you will end up with a hybrid approach with some
stuff in the device tree and some stuff in the code,
it's better to keep the old driver.
This will allow us to cover all the existing Samsung SoCs, starting from
S3C24xx, through S3C64xx, S5P*, all supported Exynos SoCs and ending on any
future SoCs using this kind of pin controller, without bloating the driver
with hardly readable macros, lots of (often duplicated) static data and
similar.
If there are some serious problems with this approach, just let me know and
I will reconsider it, but if not, I'd like to keep it, because of the
benefits it gives.
(Even if I moved all those SoC-specific data to static structures located
in the driver, to keep the most readable way of GPIO specification in DT, I
would have to create nodes for all banks in DT anyway and the driver would
have to match particular nodes with their static data. I don't like this
kind of approach.)
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 08:45:20
On Wednesday 10 of October 2012 09:40:16 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
Instead of registering one IRQ domain for all pin banks of a pin
controller, this patch implements registration of per-bank domains.
At a cost of a little memory overhead (~2.5KiB for all GPIO interrupts
of Exynos4x12) it simplifies driver code and device tree sources,
because GPIO interrupts can be now specified per banks.
Example:
device {
/* ... */
interrupt-parent = <&gpa1>;
interrupts = <3 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
This looks like a very good patch!
Can it be applied in isolation from the other patches?
This is heavily dependent on previous patches, because each pin bank must
have its own node that can be bound to the IRQ domain and used as an
interrupt-controller in interrupt-parent property.
I can imagine kind of hybrid solution, where bank nodes contain almost no
data, other than gpio-controller, interrupt-controller and #*-cells
properties, but this would introduce the need of matching bank nodes with
banks statically defined in the driver.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 08:49:09
On Wednesday 10 of October 2012 09:43:25 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
This patch modifies the pinctrl-samsung driver to register one GPIO
chip
per pin bank, instead of a single chip for all pin banks of the
controller.
It simplifies GPIO accesses a lot (constant time instead of looping
through the list of banks to find the right one) and should have a good
effect on performance of any bit-banging driver.
In addition it allows to reference GPIO pins by a phandle to the bank
node and a local pin offset inside of the bank (similar to previous
gpiolib driver), which is more clear and readable than using indices
relative to the whole pin controller.
Example:
device {
/* ... */
gpios = <&gpk0 4 0>;
/* ... */
};
Signed-off-by: Tomasz Figa <redacted>
This also looks good (and I think it has been discussed before)
so needs to be applied in isolation from the regs-to-DT stuff.
Please see my reply for patch 9.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 08:51:37
On Wednesday 10 of October 2012 09:42:10 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
The pointer to gpio_chip passed to pin_to_reg_bank utility function is
used only to retrieve a pointer to samsung_pinctrl_drv_data structure.
This patch modifies the function and its users to pass a pointer to
samsung_pinctrl_drv_data directly.
Signed-off-by: Tomasz Figa <redacted>
Looks good, can it be applied without the others?
Yes, I think this one should apply fine.
Maybe you can make a patch series without all the
stuff moving register offsets to the DT so I can begin
with merging that and we can discuss the movement
of register info separately?
I already considered this approach, but it introduces some problems, as I
mentioned in my other replies.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Tomasz Figa <hidden> Date: 2012-10-10 15:22:17
On Wednesday 10 of October 2012 09:46:28 Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
This patch series is a work on improving usability and extensibiltiy of
the>
pinctrl-samsung driver. It consists of three main parts:
- moving SoC-specific data to device tree
- converting the driver to use one GPIO chip and one IRQ domain per
pin bank - introducing generic wake-up interrupt capability
description
So can you prepare a patch series which does all but the first
bullet to begin with, and a SoC-and register offset patch
on top of that as a separate series, because it is controversial?
I don't like that these two things are mingled together like this
in an all-or-nothing manner.
So I'm OK with a patch series for bulle (2) and (3) but not (1).
And I'd like to have Thomas A:s ACK on the series too.
I have managed to rework the changes to drop (1). I will send next version
of patches tomorrow. It would be nice to have them merged for 3.7, as they
are rather important for further work.
Moving data from the driver to device tree is not as important, so it might
be discussed later.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
From: Stephen Warren <hidden> Date: 2012-10-10 16:27:27
On 10/10/2012 01:26 AM, Linus Walleij wrote:
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
Seuqential patches from this series introduce SoC-specific data parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
We really need to discuss this, Grant has already NACK:ed
such approaches once.
Well, I don't think he NACK'd Tony Lindgren's generic pinctrl driver,
which is doing this exact same thing. I did raise the same point about
Tony's driver when he posted it, but nobody seemed inclined to NACK it
based on that at the time, IIRC...
BTW, the idea here is IIRC to create a generic Samsung pinctrl driver
that works across N different Samsung SoCs, each with different register
layout, without having to encode the register layout into tables in the
kernel.
If you're still going to do this, it is mandatory
to NOT use magic hex numbers anymore, because Stephen has
merged preprocessor support to the DTC compiler so you
can use #defined macros.
See commit:
cd296721a9645f9f28800a072490fa15458d1fb7
That feature isn't enabled yet. While dtc has been modified to be able
to accept input that's been generated/processed by cpp, there is still
ongoing discussion about how/whether to actually enable *.dts to use
that feature.
From: Tony Lindgren <tony@atomide.com> Date: 2012-10-10 18:12:53
* Stephen Warren [off-list ref] [121010 09:36]:
On 10/10/2012 01:26 AM, Linus Walleij wrote:
quoted
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
quoted
Seuqential patches from this series introduce SoC-specific data parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
We really need to discuss this, Grant has already NACK:ed
such approaches once.
Well, I don't think he NACK'd Tony Lindgren's generic pinctrl driver,
which is doing this exact same thing. I did raise the same point about
Tony's driver when he posted it, but nobody seemed inclined to NACK it
based on that at the time, IIRC...
To summarize, using reg value pairs in DT makes sense if the amount
of data is huge. Otherwise we'll be describing indidual hardware bits
as properties in DT, or have to have huge amounts of static data in
the kernel.
Where it does not make sense is if there's a sequence of reads
and writes with test loops in between.. But that's does not look
to be the case here.
The reg value pairs will be readable when the DT preprocessing is
available, and that allows the values to be orred together while
DT properties don't. The alternative is to describe hardware register
bits as DT properties, which is very bloated.
But considering all this.. Are the samsung,reg-names really needed
by the kernel?
The pinctrl named modes actually are more generic from the pinctrl
client driver point of view as you can set up multiple states for
runtime PM.
BTW, the idea here is IIRC to create a generic Samsung pinctrl driver
that works across N different Samsung SoCs, each with different register
layout, without having to encode the register layout into tables in the
kernel.
quoted
If you're still going to do this, it is mandatory
to NOT use magic hex numbers anymore, because Stephen has
merged preprocessor support to the DTC compiler so you
can use #defined macros.
See commit:
cd296721a9645f9f28800a072490fa15458d1fb7
That feature isn't enabled yet. While dtc has been modified to be able
to accept input that's been generated/processed by cpp, there is still
ongoing discussion about how/whether to actually enable *.dts to use
that feature.
From: Tomasz Figa <hidden> Date: 2012-10-10 18:22:51
Dnia ?roda, 10 pa?dziernika 2012 11:12:53 Tony Lindgren pisze:
* Stephen Warren [off-list ref] [121010 09:36]:
quoted
On 10/10/2012 01:26 AM, Linus Walleij wrote:
quoted
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref]
wrote:
quoted
quoted
quoted
Seuqential patches from this series introduce SoC-specific data
parsing
quoted
quoted
quoted
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
We really need to discuss this, Grant has already NACK:ed
such approaches once.
Well, I don't think he NACK'd Tony Lindgren's generic pinctrl driver,
which is doing this exact same thing. I did raise the same point about
Tony's driver when he posted it, but nobody seemed inclined to NACK it
based on that at the time, IIRC...
To summarize, using reg value pairs in DT makes sense if the amount
of data is huge. Otherwise we'll be describing indidual hardware bits
as properties in DT, or have to have huge amounts of static data in
the kernel.
Where it does not make sense is if there's a sequence of reads
and writes with test loops in between.. But that's does not look
to be the case here.
The reg value pairs will be readable when the DT preprocessing is
available, and that allows the values to be orred together while
DT properties don't. The alternative is to describe hardware register
bits as DT properties, which is very bloated.
But considering all this.. Are the samsung,reg-names really needed
by the kernel?
They are used to specify which registers are defined in reg-params property
and in which order. Most of the registers are not mandatory and this is
needed to be able to specify only those that are present. At least I
couldn't think of a better solution for this. Do you have some suggestions?
Best regards,
Tomasz Figa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121010/8a94d561/attachment-0001.html>
From: Tomasz Figa <hidden> Date: 2012-10-10 18:26:25
Dnia ?roda, 10 pa?dziernika 2012 11:12:53 Tony Lindgren pisze:
* Stephen Warren [off-list ref] [121010 09:36]:
quoted
On 10/10/2012 01:26 AM, Linus Walleij wrote:
quoted
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref]
wrote:
quoted
quoted
quoted
Seuqential patches from this series introduce SoC-specific data
parsing
from device tree.
This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.
This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:
"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?
We really need to discuss this, Grant has already NACK:ed
such approaches once.
Well, I don't think he NACK'd Tony Lindgren's generic pinctrl driver,
which is doing this exact same thing. I did raise the same point about
Tony's driver when he posted it, but nobody seemed inclined to NACK it
based on that at the time, IIRC...
To summarize, using reg value pairs in DT makes sense if the amount
of data is huge. Otherwise we'll be describing indidual hardware bits
as properties in DT, or have to have huge amounts of static data in
the kernel.
Where it does not make sense is if there's a sequence of reads
and writes with test loops in between.. But that's does not look
to be the case here.
The reg value pairs will be readable when the DT preprocessing is
available, and that allows the values to be orred together while
DT properties don't. The alternative is to describe hardware register
bits as DT properties, which is very bloated.
But considering all this.. Are the samsung,reg-names really needed
by the kernel?
They are used to specify which registers are defined in reg-params property
and in which order. Most of the registers are not mandatory and this is
needed to be able to specify only those that are present. At least I
couldn't think of a better solution for this. Do you have some suggestions?
Best regards,
Tomasz Figa
On Wed, Oct 10, 2012 at 5:22 PM, Tomasz Figa [off-list ref] wrote:
I have managed to rework the changes to drop (1). I will send next version
of patches tomorrow. It would be nice to have them merged for 3.7, as they
are rather important for further work.
Moving data from the driver to device tree is not as important, so it might
be discussed later.
Thanks! :-D
I'll look at it now... I still would really like to see Mr. Abraham's
ACK on this before I merge any of it.
Yours,
Linus Walleij
On Wed, Oct 10, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:
On Wednesday 10 of October 2012 09:34:05 Linus Walleij wrote:
quoted
If you will end up with a hybrid approach with some
stuff in the device tree and some stuff in the code,
it's better to keep the old driver.
This will allow us to cover all the existing Samsung SoCs, starting from
S3C24xx, through S3C64xx, S5P*, all supported Exynos SoCs and ending on any
future SoCs using this kind of pin controller, without bloating the driver
with hardly readable macros, lots of (often duplicated) static data and
similar.
I do not agree with this, as you probably have realized by now...
I think it's better to use the compatible string to choose the offset
variable directly in the driver. But hey, it's just me, still.
If there are some serious problems with this approach, just let me know and
I will reconsider it, but if not, I'd like to keep it, because of the
benefits it gives.
I'd like some input from Thomas Abraham before I make up my
mind about it.
Yours,
Linus Walleij