| Message ID | 1348131197-25506-7-git-send-email-t.figa@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/20/2012 02:53 AM, Tomasz Figa wrote: > The patch "pinctrl: samsung: Parse pin banks from DT" introduced > platform-specific data parsing from DT. > > This patch adds all necessary nodes and properties to exynos4210 device > tree sources. > +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > +/ { > + pinctrl@11400000 { > + gpa0: pin-bank@0 { If you're going to put a unit address ("@0")into the DT node name, the node should have a reg property containing the same value, and the parent node should have #address-cells and #size-cells properties. However, I believe you can actually get unique node names without using a unit address; instead name the nodes after the object they represent, so e.g. s/pin-bank@0/gpa0/ above. > + gpio-controller; You need a #gpio-cells property too. > + samsung,pctl-offset = <0x000>; > + samsung,pin-bank = "gpa0"; > + samsung,pin-count = <8>; > + samsung,func-width = <4>; > + samsung,pud-width = <2>; > + samsung,drv-width = <2>; > + samsung,conpdn-width = <2>; > + samsung,pudpdn-width = <2>; The properties above represent the width of the fields. Must all fields always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order of the fields in the register changes? I think you want to add either a samsung,func-bit/samsung,func-position property for each of the fields, or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC, the generic pinctrl binding used a mask for this purpose. What if a future SoC variant adds more fields to the register? At that point, you'd need to edit the driver anyway in order to define a new DT property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields: samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; That would allow a completely arbitrary number of fields and layouts to be described. I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now. > + interrupt-controller; You need a #interrupt-cells property too. > + gpd0: pin-bank@5 { > + gpio-controller; > + samsung,pctl-offset = <0x0A0>; I think hex number are usually lower-case in DT, but I may be extrapolating a generality from a limited set of examples. > + gpy5: pin-bank@19{ Missing a space before the { there. > diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi > index ecbc707..0e93717 100644 > --- a/arch/arm/boot/dts/exynos4210.dtsi > +++ b/arch/arm/boot/dts/exynos4210.dtsi > @@ -59,6 +59,10 @@ > reg = <0x11400000 0x1000>; > interrupts = <0 47 0>; > interrupt-controller; > + samsung,geint-con = <0x700>; > + samsung,geint-mask = <0x900>; > + samsung,geint-pend = <0xA00>; > + samsung,svc = <0xB08>; I assume those new properties represent register addresses within the block. If not, I don't understand what they are. It's unclear to me why those properties aren't all part of exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where the register addresses for the pinctrl registers are the same (hence can be in a shared exynos4210-pinctrl-banks.dtsi), yet the register addresses for the geint registers are different (hence must be in non-shared exynos4210.dtsi)? Do these properties interact with samsung,eint-offset at all? Oh, perhaps these properties are defining a top-level interrupt controller that aggregates all the banks together, whereas samsung,eint-offset is per-bank?
Hi Stephen, Thanks for your comments. On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: > On 09/20/2012 02:53 AM, Tomasz Figa wrote: > > The patch "pinctrl: samsung: Parse pin banks from DT" introduced > > platform-specific data parsing from DT. > > > > This patch adds all necessary nodes and properties to exynos4210 device > > tree sources. > > > > +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > > > > +/ { > > + pinctrl@11400000 { > > + gpa0: pin-bank@0 { > > If you're going to put a unit address ("@0")into the DT node name, the > node should have a reg property containing the same value, and the > parent node should have #address-cells and #size-cells properties. > > However, I believe you can actually get unique node names without using > a unit address; instead name the nodes after the object they represent, > so e.g. s/pin-bank@0/gpa0/ above. Good point. I wasn't aware of the relation between unit address and reg property. I thought it was just for readability. > > > + gpio-controller; > > You need a #gpio-cells property too. It will be added in further patches that add per-bank GPIO addressing. In this RFC, this property is just used to distinguish pin bank nodes from pin group nodes. > > + samsung,pctl-offset = <0x000>; > > + samsung,pin-bank = "gpa0"; > > + samsung,pin-count = <8>; > > + samsung,func-width = <4>; > > + samsung,pud-width = <2>; > > + samsung,drv-width = <2>; > > + samsung,conpdn-width = <2>; > > + samsung,pudpdn-width = <2>; > > The properties above represent the width of the fields. Must all fields > always be packed together into say the LSB of the registers? What if > there are gaps between the fields in a future SoC variant, or the order > of the fields in the register changes? I think you want to add either a > samsung,func-bit/samsung,func-position property for each of the fields, > or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC, > the generic pinctrl binding used a mask for this purpose. > > What if a future SoC variant adds more fields to the register? At that > point, you'd need to edit the driver anyway in order to define a new DT > property to represent the new field. Perhaps instead of having an > explicit named property per field in the register, you want a simple > list of fields: > > samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; > samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; > > That would allow a completely arbitrary number of fields and layouts to > be described. > > I wonder if for absolute generality you want a samsung,pin-stride > property to represent the difference in register address per pin. I > assume that's hard-coded as 4 right now. Hmm, considering so many different possible changes, maybe a more conservative solution would be better, like reducing the amount of information held in DT to bank type, e.g. samsung,bank-type = "exynos4"; or maybe compatible = "samsung,exynos4-pin-bank*; and then define supported bank types in the driver. SoC-specific data would remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc. > > + interrupt-controller; > You need a #interrupt-cells property too. Just as with gpio-controller before, in this RFC this is just a mark to distinguish banks with interrupts from banks without interrupts. Further patches will allow to use it properly (and will add #interrupt- cells property too). > > + gpd0: pin-bank@5 { > > + gpio-controller; > > + samsung,pctl-offset = <0x0A0>; > > I think hex number are usually lower-case in DT, but I may be > extrapolating a generality from a limited set of examples. I have seen both variants, with upper-case being more popular across Samsung's dts files and so I have chosen to use it. (Personally I prefer lower-case, though, if it does matter.) > > + gpy5: pin-bank@19{ > > Missing a space before the { there. Right. > > diff --git a/arch/arm/boot/dts/exynos4210.dtsi > > b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644 > > --- a/arch/arm/boot/dts/exynos4210.dtsi > > +++ b/arch/arm/boot/dts/exynos4210.dtsi > > @@ -59,6 +59,10 @@ > > > > reg = <0x11400000 0x1000>; > > interrupts = <0 47 0>; > > interrupt-controller; > > > > + samsung,geint-con = <0x700>; > > + samsung,geint-mask = <0x900>; > > + samsung,geint-pend = <0xA00>; > > + samsung,svc = <0xB08>; > > I assume those new properties represent register addresses within the > block. If not, I don't understand what they are. Yes, they do. > It's unclear to me why those properties aren't all part of > exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where > the register addresses for the pinctrl registers are the same (hence can > be in a shared exynos4210-pinctrl-banks.dtsi), yet the register > addresses for the geint registers are different (hence must be in > non-shared exynos4210.dtsi)? Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. Other SoCs are going to have their own whatever-pinctrl-banks.dtsi. > Do these properties interact with samsung,eint-offset at all? Oh, > perhaps these properties are defining a top-level interrupt controller > that aggregates all the banks together, whereas samsung,eint-offset is > per-bank? Yes. There is a bunch of CON registers one after another, for each bank, which supports interrupts and similarly for MASK and PEND. All those geint- xxx properties define the location of first register and eint-offset defines location of register for particular bank. Best regards, Tomasz Figa
On 09/21/2012 01:54 PM, Tomasz Figa wrote: > On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: >> On 09/20/2012 02:53 AM, Tomasz Figa wrote: >>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced >>> platform-specific data parsing from DT. >>> >>> This patch adds all necessary nodes and properties to exynos4210 device >>> tree sources. >>> >>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi >>> + samsung,pctl-offset = <0x000>; >>> + samsung,pin-bank = "gpa0"; >>> + samsung,pin-count = <8>; >>> + samsung,func-width = <4>; >>> + samsung,pud-width = <2>; >>> + samsung,drv-width = <2>; >>> + samsung,conpdn-width = <2>; >>> + samsung,pudpdn-width = <2>; >> >> The properties above represent the width of the fields. Must all fields >> always be packed together into say the LSB of the registers? What if >> there are gaps between the fields in a future SoC variant, or the order >> of the fields in the register changes? I think you want to add either a >> samsung,func-bit/samsung,func-position property for each of the fields, >> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC, >> the generic pinctrl binding used a mask for this purpose. >> >> What if a future SoC variant adds more fields to the register? At that >> point, you'd need to edit the driver anyway in order to define a new DT >> property to represent the new field. Perhaps instead of having an >> explicit named property per field in the register, you want a simple >> list of fields: >> >> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; >> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; >> >> That would allow a completely arbitrary number of fields and layouts to >> be described. >> >> I wonder if for absolute generality you want a samsung,pin-stride >> property to represent the difference in register address per pin. I >> assume that's hard-coded as 4 right now. > > Hmm, considering so many different possible changes, maybe a more > conservative solution would be better, like reducing the amount of > information held in DT to bank type, e.g. > > samsung,bank-type = "exynos4"; > > or maybe > > compatible = "samsung,exynos4-pin-bank*; > > and then define supported bank types in the driver. SoC-specific data would > remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc. Yes, removing much of the data from DT and putting it into a tiny table in the driver makes sense to me in this case. >>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi >>> b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644 >>> --- a/arch/arm/boot/dts/exynos4210.dtsi >>> +++ b/arch/arm/boot/dts/exynos4210.dtsi >>> @@ -59,6 +59,10 @@ >>> >>> reg = <0x11400000 0x1000>; >>> interrupts = <0 47 0>; >>> interrupt-controller; >>> >>> + samsung,geint-con = <0x700>; >>> + samsung,geint-mask = <0x900>; >>> + samsung,geint-pend = <0xA00>; >>> + samsung,svc = <0xB08>; >> >> I assume those new properties represent register addresses within the >> block. If not, I don't understand what they are. > > Yes, they do. > >> It's unclear to me why those properties aren't all part of >> exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where >> the register addresses for the pinctrl registers are the same (hence can >> be in a shared exynos4210-pinctrl-banks.dtsi), yet the register >> addresses for the geint registers are different (hence must be in >> non-shared exynos4210.dtsi)? > > Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. > Other SoCs are going to have their own whatever-pinctrl-banks.dtsi. OK, so my point here is: why not put all the pinctrl-related properties into a single file, rather than spreading them across different files. Having separate files makes sense if they can be re-used in different places, but not if they're single-use.
On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: > On 09/21/2012 01:54 PM, Tomasz Figa wrote: > > On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: > >> On 09/20/2012 02:53 AM, Tomasz Figa wrote: > >>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced > >>> platform-specific data parsing from DT. > >>> > >>> This patch adds all necessary nodes and properties to exynos4210 > >>> device > >>> tree sources. > >>> > >>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > >>> > >>> + samsung,pctl-offset = <0x000>; > >>> + samsung,pin-bank = "gpa0"; > >>> + samsung,pin-count = <8>; > >>> + samsung,func-width = <4>; > >>> + samsung,pud-width = <2>; > >>> + samsung,drv-width = <2>; > >>> + samsung,conpdn-width = <2>; > >>> + samsung,pudpdn-width = <2>; > >> > >> The properties above represent the width of the fields. Must all > >> fields > >> always be packed together into say the LSB of the registers? What if > >> there are gaps between the fields in a future SoC variant, or the > >> order > >> of the fields in the register changes? I think you want to add either > >> a > >> samsung,func-bit/samsung,func-position property for each of the > >> fields, > >> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. > >> IIRC, > >> the generic pinctrl binding used a mask for this purpose. > >> > >> What if a future SoC variant adds more fields to the register? At that > >> point, you'd need to edit the driver anyway in order to define a new > >> DT > >> property to represent the new field. Perhaps instead of having an > >> explicit named property per field in the register, you want a simple > >> list of fields: > >> > >> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; > >> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; > >> > >> That would allow a completely arbitrary number of fields and layouts > >> to > >> be described. > >> > >> I wonder if for absolute generality you want a samsung,pin-stride > >> property to represent the difference in register address per pin. I > >> assume that's hard-coded as 4 right now. > > > > Hmm, considering so many different possible changes, maybe a more > > conservative solution would be better, like reducing the amount of > > information held in DT to bank type, e.g. > > > > samsung,bank-type = "exynos4"; > > > > or maybe > > > > compatible = "samsung,exynos4-pin-bank*; > > > > and then define supported bank types in the driver. SoC-specific data > > would remain in DT, i.e. pctl-offset, pin-bank, pin-count, > > eint-offset, etc. > Yes, removing much of the data from DT and putting it into a tiny table > in the driver makes sense to me in this case. A hybrid solution came to my mind, define bank types in device tree once and then reference them in banks. Something like: pinctrl-bank-types { bank_off: bank-off { samsung,func-width = <4>; samsung,pud-width = <2>; samsung,drv-width = <2>; samsung,conpdn-width = <2>; samsung,pudpdn-width = <2>; }; bank_alive: bank-alive { samsung,func-width = <4>; samsung,pud-width = <2>; samsung,drv-width = <2>; }; }; /* ... */ pinctrl@12345678 { /* ... */ gpa0: gpa0 { /* ... */ samsung,bank-type = <&bank_off>; /* ... */ }; /* ... */ }; This would add the possibility to define new banks types quickly, but would not add too much overhead. What do you think? > >>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi > >>> b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644 > >>> --- a/arch/arm/boot/dts/exynos4210.dtsi > >>> +++ b/arch/arm/boot/dts/exynos4210.dtsi > >>> @@ -59,6 +59,10 @@ > >>> > >>> reg = <0x11400000 0x1000>; > >>> interrupts = <0 47 0>; > >>> interrupt-controller; > >>> > >>> + samsung,geint-con = <0x700>; > >>> + samsung,geint-mask = <0x900>; > >>> + samsung,geint-pend = <0xA00>; > >>> + samsung,svc = <0xB08>; > >> > >> I assume those new properties represent register addresses within the > >> block. If not, I don't understand what they are. > > > > Yes, they do. > > > >> It's unclear to me why those properties aren't all part of > >> exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where > >> the register addresses for the pinctrl registers are the same (hence > >> can > >> be in a shared exynos4210-pinctrl-banks.dtsi), yet the register > >> addresses for the geint registers are different (hence must be in > >> non-shared exynos4210.dtsi)? > > > > Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to > > Exynos4210. Other SoCs are going to have their own > > whatever-pinctrl-banks.dtsi. > OK, so my point here is: why not put all the pinctrl-related properties > into a single file, rather than spreading them across different files. > Having separate files makes sense if they can be re-used in different > places, but not if they're single-use. All the definitions in device tree for pinctrl take lots of lines and so I though it would make it more readable if pin groups would have its own source file and so would pin banks. Now that I think of it, they aren't going to be modified too much, so it might be better indeed to put them together in a single file. Best regards, Tomasz Figa
On 09/24/2012 03:31 PM, Tomasz Figa wrote: > On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: >> On 09/21/2012 01:54 PM, Tomasz Figa wrote: >>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: >>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: >>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced >>>>> platform-specific data parsing from DT. >>>>> >>>>> This patch adds all necessary nodes and properties to exynos4210 >>>>> device >>>>> tree sources. >>>>> >>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi >>>>> >>>>> + samsung,pctl-offset = <0x000>; >>>>> + samsung,pin-bank = "gpa0"; >>>>> + samsung,pin-count = <8>; >>>>> + samsung,func-width = <4>; >>>>> + samsung,pud-width = <2>; >>>>> + samsung,drv-width = <2>; >>>>> + samsung,conpdn-width = <2>; >>>>> + samsung,pudpdn-width = <2>; >>>> >>>> The properties above represent the width of the fields. Must all >>>> fields >>>> always be packed together into say the LSB of the registers? What if >>>> there are gaps between the fields in a future SoC variant, or the >>>> order >>>> of the fields in the register changes? I think you want to add either >>>> a >>>> samsung,func-bit/samsung,func-position property for each of the >>>> fields, >>>> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. >>>> IIRC, >>>> the generic pinctrl binding used a mask for this purpose. >>>> >>>> What if a future SoC variant adds more fields to the register? At that >>>> point, you'd need to edit the driver anyway in order to define a new >>>> DT >>>> property to represent the new field. Perhaps instead of having an >>>> explicit named property per field in the register, you want a simple >>>> list of fields: >>>> >>>> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; >>>> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; >>>> >>>> That would allow a completely arbitrary number of fields and layouts >>>> to >>>> be described. >>>> >>>> I wonder if for absolute generality you want a samsung,pin-stride >>>> property to represent the difference in register address per pin. I >>>> assume that's hard-coded as 4 right now. >>> >>> Hmm, considering so many different possible changes, maybe a more >>> conservative solution would be better, like reducing the amount of >>> information held in DT to bank type, e.g. >>> >>> samsung,bank-type = "exynos4"; >>> >>> or maybe >>> >>> compatible = "samsung,exynos4-pin-bank*; >>> >>> and then define supported bank types in the driver. SoC-specific data >>> would remain in DT, i.e. pctl-offset, pin-bank, pin-count, >>> eint-offset, etc. >> Yes, removing much of the data from DT and putting it into a tiny table >> in the driver makes sense to me in this case. > > A hybrid solution came to my mind, define bank types in device tree once > and then reference them in banks. Something like: > > pinctrl-bank-types { > bank_off: bank-off { > samsung,func-width = <4>; > samsung,pud-width = <2>; > samsung,drv-width = <2>; > samsung,conpdn-width = <2>; > samsung,pudpdn-width = <2>; > }; > > bank_alive: bank-alive { > samsung,func-width = <4>; > samsung,pud-width = <2>; > samsung,drv-width = <2>; > }; > }; > > /* ... */ > > pinctrl@12345678 { > /* ... */ > gpa0: gpa0 { > /* ... */ > samsung,bank-type = <&bank_off>; > /* ... */ > }; > /* ... */ > }; > > This would add the possibility to define new banks types quickly, but would > not add too much overhead. > > What do you think? That does solve the issue of parsing those same values over and over, but at the cost of making the binding a lot more conceptually complex. However, it doesn't solve any of the extensibility issues I raised such as whether pos+size or mask would be a better representation, what about if the fields end up being in different (separate) registers on newer SoCs, etc. I forget, do you actually have multiple different SoCs right now (or in the near future where the HW design is known now for certain even if the chip isn't available) that have different values for all these *-width properties and hence can be represented just using this binding and without altering the driver at all? If so, I suppose the original binding is at least useful (although I would certainly still request to use *-mask instead of *-width properties).
Hi Stephen, On Monday 24 of September 2012 17:14:38 Stephen Warren wrote: > On 09/24/2012 03:31 PM, Tomasz Figa wrote: > > On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: > >> On 09/21/2012 01:54 PM, Tomasz Figa wrote: > >>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: > >>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: > >>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced > >>>>> platform-specific data parsing from DT. > >>>>> > >>>>> This patch adds all necessary nodes and properties to exynos4210 > >>>>> device > >>>>> tree sources. > >>>>> > >>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > >>>>> > >>>>> + samsung,pctl-offset = <0x000>; > >>>>> + samsung,pin-bank = "gpa0"; > >>>>> + samsung,pin-count = <8>; > >>>>> + samsung,func-width = <4>; > >>>>> + samsung,pud-width = <2>; > >>>>> + samsung,drv-width = <2>; > >>>>> + samsung,conpdn-width = <2>; > >>>>> + samsung,pudpdn-width = <2>; > >>>> > >>>> The properties above represent the width of the fields. Must all > >>>> fields > >>>> always be packed together into say the LSB of the registers? What if > >>>> there are gaps between the fields in a future SoC variant, or the > >>>> order > >>>> of the fields in the register changes? I think you want to add > >>>> either > >>>> a > >>>> samsung,func-bit/samsung,func-position property for each of the > >>>> fields, > >>>> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. > >>>> IIRC, > >>>> the generic pinctrl binding used a mask for this purpose. > >>>> > >>>> What if a future SoC variant adds more fields to the register? At > >>>> that > >>>> point, you'd need to edit the driver anyway in order to define a new > >>>> DT > >>>> property to represent the new field. Perhaps instead of having an > >>>> explicit named property per field in the register, you want a simple > >>>> list of fields: > >>>> > >>>> samsung,pin-property-names = "func", "pud", "drv", "conpdn", > >>>> "pudpdn"; > >>>> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>; > >>>> > >>>> That would allow a completely arbitrary number of fields and layouts > >>>> to > >>>> be described. > >>>> > >>>> I wonder if for absolute generality you want a samsung,pin-stride > >>>> property to represent the difference in register address per pin. I > >>>> assume that's hard-coded as 4 right now. > >>> > >>> Hmm, considering so many different possible changes, maybe a more > >>> conservative solution would be better, like reducing the amount of > >>> information held in DT to bank type, e.g. > >>> > >>> samsung,bank-type = "exynos4"; > >>> > >>> or maybe > >>> > >>> compatible = "samsung,exynos4-pin-bank*; > >>> > >>> and then define supported bank types in the driver. SoC-specific data > >>> would remain in DT, i.e. pctl-offset, pin-bank, pin-count, > >>> eint-offset, etc. > >> > >> Yes, removing much of the data from DT and putting it into a tiny > >> table > >> in the driver makes sense to me in this case. > > > > A hybrid solution came to my mind, define bank types in device tree > > once > > > > and then reference them in banks. Something like: > > pinctrl-bank-types { > > > > bank_off: bank-off { > > > > samsung,func-width = <4>; > > samsung,pud-width = <2>; > > samsung,drv-width = <2>; > > samsung,conpdn-width = <2>; > > samsung,pudpdn-width = <2>; > > > > }; > > > > bank_alive: bank-alive { > > > > samsung,func-width = <4>; > > samsung,pud-width = <2>; > > samsung,drv-width = <2>; > > > > }; > > > > }; > > > > /* ... */ > > > > pinctrl@12345678 { > > > > /* ... */ > > gpa0: gpa0 { > > > > /* ... */ > > samsung,bank-type = <&bank_off>; > > /* ... */ > > > > }; > > /* ... */ > > > > }; > > > > This would add the possibility to define new banks types quickly, but > > would not add too much overhead. > > > > What do you think? > > That does solve the issue of parsing those same values over and over, > but at the cost of making the binding a lot more conceptually complex. > > However, it doesn't solve any of the extensibility issues I raised such > as whether pos+size or mask would be a better representation, what about > if the fields end up being in different (separate) registers on newer > SoCs, etc. Hmm, could you elaborate on the idea of using mask instead of field widths? I don't see how this could be better and there is an additional drawback of having to calculate width and pos from every mask. Anyway, back to your concern, the values that are written to the bit fields specified by those bindings are arbitrary SoC-specific values anyway, so if, for example, we get a SoC with following register layout: bits: 7 | 6 - 4 | 3 | 2 - 0 meaning: 0 | func 1 | 0 | func 0 or bits: 7 - 5 | 4 | 3 - 1 | 0 meaning: func 1 | 0 | func 0 | 0 we can easily define the width as 4 and use appropriate 4-bit function values with zeroes on reserved positions. > I forget, do you actually have multiple different SoCs right now (or in > the near future where the HW design is known now for certain even if the > chip isn't available) that have different values for all these *-width > properties and hence can be represented just using this binding and > without altering the driver at all? If so, I suppose the original > binding is at least useful (although I would certainly still request to > use *-mask instead of *-width properties). The binding I proposed covers all Samsung SoCs currently available, starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two function registers), to the whole Exynos series, including latest Exynos5. Best regards, -- Tomasz Figa Samsung Poland R&D Center
On 09/25/2012 03:37 AM, Tomasz Figa wrote: > Hi Stephen, > > On Monday 24 of September 2012 17:14:38 Stephen Warren wrote: >> On 09/24/2012 03:31 PM, Tomasz Figa wrote: >>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: >>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote: >>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: >>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: >>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced >>>>>>> platform-specific data parsing from DT. >>>>>>> >>>>>>> This patch adds all necessary nodes and properties to exynos4210 >>>>>>> device >>>>>>> tree sources. >>>>>>> >>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi >>>>>>> >>>>>>> + samsung,pctl-offset = <0x000>; >>>>>>> + samsung,pin-bank = "gpa0"; >>>>>>> + samsung,pin-count = <8>; >>>>>>> + samsung,func-width = <4>; >>>>>>> + samsung,pud-width = <2>; >>>>>>> + samsung,drv-width = <2>; >>>>>>> + samsung,conpdn-width = <2>; >>>>>>> + samsung,pudpdn-width = <2>; ... > Hmm, could you elaborate on the idea of using mask instead of field widths? For background: With e.g.: samsung,func-width = <4>; samsung,pud-width = <2>; samsung,drv-width = <2>; How do you know if the layout is: bits: 7-4 | 3-2 | 1-0 meaning: func | pud | drv or: bits: 7-6 | 5-4 | 3-0 | meaning: drv | pud | func | or: bits: 15-12 | 13-8 | 7-6 | 5-3 | 2-1 | 0 meaning: func | unused | pud | unused | drv | unused I suppose what you're saying is that for all currently extant Samsung SoCs, there's some rule that defines this; perhaps the fields are always in order MSB to LSB func, pud, drv, and there are never any unused bits between the fields? If so, I suppose that's reasonable, even if it does restrict the binding's ability to support any unanticipated future SoC register layout changes. > I don't see how this could be better and there is an additional drawback of > having to calculate width and pos from every mask. With the DT properties just defining "width", the driver still has to calculate the pos from every width by adding up the widths of all fields lower in the register, right? Or, does each field always start at a hard-coded bit position? Anyway, you could completely avoid this question by using masks instead: samsung,func-mask = <0xf0>; samsung,pud-mask = <0xc>; samsung,drv-mask = <0x3>; The mask defines exactly which bits are included in the register field, so it implicitly defines both the position and width of the field. Finding the shift/size is very easy. I believe Tony Lindgren's generic pinctrl already does this along these lines. Very roughly: func_pos = ffs(func_mask); func_width = ffs(~(func_mask >> func_pos)); > Anyway, back to your concern, the values that are written to the bit fields > specified by those bindings are arbitrary SoC-specific values anyway, so > if, for example, we get a SoC with following register layout: > > bits: 7 | 6 - 4 | 3 | 2 - 0 > meaning: 0 | func 1 | 0 | func 0 > > or > > bits: 7 - 5 | 4 | 3 - 1 | 0 > meaning: func 1 | 0 | func 0 | 0 > > we can easily define the width as 4 and use appropriate 4-bit function > values with zeroes on reserved positions. The problem with that is that if the datasheet documents "func" values of 0, 1, 2, 3, whereas your driver expects values that are shifted left one bit in order to fit into the field, the DT would need to contain 0, 2, 4, 6. So, the DT values then don't match the documentation, which would end up being confusing. >> I forget, do you actually have multiple different SoCs right now (or in >> the near future where the HW design is known now for certain even if the >> chip isn't available) that have different values for all these *-width >> properties and hence can be represented just using this binding and >> without altering the driver at all? If so, I suppose the original >> binding is at least useful (although I would certainly still request to >> use *-mask instead of *-width properties). > > The binding I proposed covers all Samsung SoCs currently available, > starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two > function registers), to the whole Exynos series, including latest Exynos5. OK, the HW is nice and consistent then. In that case, the binding is probably reasonable. Hopefully the HW designers are aware they shouldn't randomly break the uniformity!
On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote: > On 09/25/2012 03:37 AM, Tomasz Figa wrote: > > Hi Stephen, > > > > On Monday 24 of September 2012 17:14:38 Stephen Warren wrote: > >> On 09/24/2012 03:31 PM, Tomasz Figa wrote: > >>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: > >>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote: > >>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: > >>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: > >>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced > >>>>>>> platform-specific data parsing from DT. > >>>>>>> > >>>>>>> This patch adds all necessary nodes and properties to exynos4210 > >>>>>>> device > >>>>>>> tree sources. > >>>>>>> > >>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > >>>>>>> > >>>>>>> + samsung,pctl-offset = <0x000>; > >>>>>>> + samsung,pin-bank = "gpa0"; > >>>>>>> + samsung,pin-count = <8>; > >>>>>>> + samsung,func-width = <4>; > >>>>>>> + samsung,pud-width = <2>; > >>>>>>> + samsung,drv-width = <2>; > >>>>>>> + samsung,conpdn-width = <2>; > >>>>>>> + samsung,pudpdn-width = <2>; > > ... > > > Hmm, could you elaborate on the idea of using mask instead of field > > widths? > For background: With e.g.: > > samsung,func-width = <4>; > samsung,pud-width = <2>; > samsung,drv-width = <2>; > > How do you know if the layout is: > > bits: 7-4 | 3-2 | 1-0 > meaning: func | pud | drv > > or: > > bits: 7-6 | 5-4 | 3-0 | > meaning: drv | pud | func | > > or: > > bits: 15-12 | 13-8 | 7-6 | 5-3 | 2-1 | 0 > meaning: func | unused | pud | unused | drv | unused > > I suppose what you're saying is that for all currently extant Samsung > SoCs, there's some rule that defines this; perhaps the fields are always > in order MSB to LSB func, pud, drv, and there are never any unused bits > between the fields? If so, I suppose that's reasonable, even if it does > restrict the binding's ability to support any unanticipated future SoC > register layout changes. I think we have a little misunderstanding here. All the Samsung SoCs currently available have separate registers for particular configuration types. Each register is used to configure all pins in a bank. The width field specifies how many bits are used per pin, not per configuration type. > > I don't see how this could be better and there is an additional > > drawback of having to calculate width and pos from every mask. > > With the DT properties just defining "width", the driver still has to > calculate the pos from every width by adding up the widths of all fields > lower in the register, right? Or, does each field always start at a > hard-coded bit position? > > Anyway, you could completely avoid this question by using masks instead: > > samsung,func-mask = <0xf0>; > samsung,pud-mask = <0xc>; > samsung,drv-mask = <0x3>; > > The mask defines exactly which bits are included in the register field, > so it implicitly defines both the position and width of the field. > > Finding the shift/size is very easy. I believe Tony Lindgren's generic > pinctrl already does this along these lines. Very roughly: > > func_pos = ffs(func_mask); > func_width = ffs(~(func_mask >> func_pos)); Right, this looks fine. > > Anyway, back to your concern, the values that are written to the bit > > fields specified by those bindings are arbitrary SoC-specific values > > anyway, so if, for example, we get a SoC with following register > > layout: > > > > bits: 7 | 6 - 4 | 3 | 2 - 0 > > meaning: 0 | func 1 | 0 | func 0 > > > > or > > > > bits: 7 - 5 | 4 | 3 - 1 | 0 > > meaning: func 1 | 0 | func 0 | 0 > > > > we can easily define the width as 4 and use appropriate 4-bit function > > values with zeroes on reserved positions. > > The problem with that is that if the datasheet documents "func" values > of 0, 1, 2, 3, whereas your driver expects values that are shifted left > one bit in order to fit into the field, the DT would need to contain 0, > 2, 4, 6. So, the DT values then don't match the documentation, which > would end up being confusing. > > >> I forget, do you actually have multiple different SoCs right now (or > >> in > >> the near future where the HW design is known now for certain even if > >> the > >> chip isn't available) that have different values for all these *-width > >> properties and hence can be represented just using this binding and > >> without altering the driver at all? If so, I suppose the original > >> binding is at least useful (although I would certainly still request > >> to > >> use *-mask instead of *-width properties). > > > > The binding I proposed covers all Samsung SoCs currently available, > > starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with > > two > > function registers), to the whole Exynos series, including latest > > Exynos5. > OK, the HW is nice and consistent then. In that case, the binding is > probably reasonable. Hopefully the HW designers are aware they shouldn't > randomly break the uniformity! Let's hope so. Best regards, Tomasz Figa
On 09/25/2012 11:41 AM, Tomasz Figa wrote: > On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote: >> On 09/25/2012 03:37 AM, Tomasz Figa wrote: >>> Hi Stephen, >>> >>> On Monday 24 of September 2012 17:14:38 Stephen Warren wrote: >>>> On 09/24/2012 03:31 PM, Tomasz Figa wrote: >>>>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: >>>>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote: >>>>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: >>>>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: >>>>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced >>>>>>>>> platform-specific data parsing from DT. >>>>>>>>> >>>>>>>>> This patch adds all necessary nodes and properties to exynos4210 >>>>>>>>> device >>>>>>>>> tree sources. >>>>>>>>> >>>>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi >>>>>>>>> >>>>>>>>> + samsung,pctl-offset = <0x000>; >>>>>>>>> + samsung,pin-bank = "gpa0"; >>>>>>>>> + samsung,pin-count = <8>; >>>>>>>>> + samsung,func-width = <4>; >>>>>>>>> + samsung,pud-width = <2>; >>>>>>>>> + samsung,drv-width = <2>; >>>>>>>>> + samsung,conpdn-width = <2>; >>>>>>>>> + samsung,pudpdn-width = <2>; >> >> ... >> >>> Hmm, could you elaborate on the idea of using mask instead of field >>> widths? >> For background: With e.g.: >> >> samsung,func-width = <4>; >> samsung,pud-width = <2>; >> samsung,drv-width = <2>; >> >> How do you know if the layout is: >> >> bits: 7-4 | 3-2 | 1-0 >> meaning: func | pud | drv >> >> or: >> >> bits: 7-6 | 5-4 | 3-0 | >> meaning: drv | pud | func | >> >> or: >> >> bits: 15-12 | 13-8 | 7-6 | 5-3 | 2-1 | 0 >> meaning: func | unused | pud | unused | drv | unused >> >> I suppose what you're saying is that for all currently extant Samsung >> SoCs, there's some rule that defines this; perhaps the fields are always >> in order MSB to LSB func, pud, drv, and there are never any unused bits >> between the fields? If so, I suppose that's reasonable, even if it does >> restrict the binding's ability to support any unanticipated future SoC >> register layout changes. > > I think we have a little misunderstanding here. > > All the Samsung SoCs currently available have separate registers for > particular configuration types. Each register is used to configure all pins > in a bank. The width field specifies how many bits are used per pin, not > per configuration type. Oh I see. In that case, I guess just having "width" properties is fine, and I can see how it's much more likely this scheme would be extensible to any future SoC that sticks with the same overall kind of register structure. It'd be a good idea to describe this explicitly in the binding documentation. BTW, how does the driver know what register addresses to use; I can see the base for each pin controller bank is in samsung,pctl-offset, but what describes the offset for each of the func, pud, drv, ... registers from there? Are the offsets the same for all current Samsung SoCs?
On Tuesday 25 of September 2012 12:22:03 Stephen Warren wrote: > On 09/25/2012 11:41 AM, Tomasz Figa wrote: > > On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote: > >> On 09/25/2012 03:37 AM, Tomasz Figa wrote: > >>> Hi Stephen, > >>> > >>> On Monday 24 of September 2012 17:14:38 Stephen Warren wrote: > >>>> On 09/24/2012 03:31 PM, Tomasz Figa wrote: > >>>>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote: > >>>>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote: > >>>>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote: > >>>>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote: > >>>>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" > >>>>>>>>> introduced > >>>>>>>>> platform-specific data parsing from DT. > >>>>>>>>> > >>>>>>>>> This patch adds all necessary nodes and properties to > >>>>>>>>> exynos4210 > >>>>>>>>> device > >>>>>>>>> tree sources. > >>>>>>>>> > >>>>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi > >>>>>>>>> > >>>>>>>>> + samsung,pctl-offset = <0x000>; > >>>>>>>>> + samsung,pin-bank = "gpa0"; > >>>>>>>>> + samsung,pin-count = <8>; > >>>>>>>>> + samsung,func-width = <4>; > >>>>>>>>> + samsung,pud-width = <2>; > >>>>>>>>> + samsung,drv-width = <2>; > >>>>>>>>> + samsung,conpdn-width = <2>; > >>>>>>>>> + samsung,pudpdn-width = <2>; > >> > >> ... > >> > >>> Hmm, could you elaborate on the idea of using mask instead of field > >>> widths? > >> > >> For background: With e.g.: > >> > >> samsung,func-width = <4>; > >> samsung,pud-width = <2>; > >> samsung,drv-width = <2>; > >> > >> How do you know if the layout is: > >> > >> bits: 7-4 | 3-2 | 1-0 > >> meaning: func | pud | drv > >> > >> or: > >> > >> bits: 7-6 | 5-4 | 3-0 | > >> meaning: drv | pud | func | > >> > >> or: > >> > >> bits: 15-12 | 13-8 | 7-6 | 5-3 | 2-1 | 0 > >> meaning: func | unused | pud | unused | drv | unused > >> > >> I suppose what you're saying is that for all currently extant Samsung > >> SoCs, there's some rule that defines this; perhaps the fields are > >> always > >> in order MSB to LSB func, pud, drv, and there are never any unused > >> bits > >> between the fields? If so, I suppose that's reasonable, even if it > >> does > >> restrict the binding's ability to support any unanticipated future SoC > >> register layout changes. > > > > I think we have a little misunderstanding here. > > > > All the Samsung SoCs currently available have separate registers for > > particular configuration types. Each register is used to configure all > > pins in a bank. The width field specifies how many bits are used per > > pin, not per configuration type. > > Oh I see. In that case, I guess just having "width" properties is fine, > and I can see how it's much more likely this scheme would be extensible > to any future SoC that sticks with the same overall kind of register > structure. > > It'd be a good idea to describe this explicitly in the binding > documentation. OK. > BTW, how does the driver know what register addresses to use; I can see > the base for each pin controller bank is in samsung,pctl-offset, but > what describes the offset for each of the func, pud, drv, ... registers > from there? Are the offsets the same for all current Samsung SoCs? The offsets are defined as constants in the driver. They are the same in all cases, but the "4bit2" bank type of S3C64xx, which can have up to 16 pins with 4-bit function specifiers, so two registers are required for function configuration. In this case all the remaining registers are offset by 0x04. I couldn't think about any good solution for this special case, but still, I haven't been thinking a lot about it, as the driver is targetted at current Exynos SoCs primarily. Best regards, Tomasz Figa
On 09/25/2012 12:35 PM, Tomasz Figa wrote: > On Tuesday 25 of September 2012 12:22:03 Stephen Warren wrote: ... >> BTW, how does the driver know what register addresses to use; I can see >> the base for each pin controller bank is in samsung,pctl-offset, but >> what describes the offset for each of the func, pud, drv, ... registers >> from there? Are the offsets the same for all current Samsung SoCs? > > The offsets are defined as constants in the driver. > > They are the same in all cases, but the "4bit2" bank type of S3C64xx, which > can have up to 16 pins with 4-bit function specifiers, so two registers are > required for function configuration. In this case all the remaining > registers are offset by 0x04. > > I couldn't think about any good solution for this special case, but still, > I haven't been thinking a lot about it, as the driver is targetted at > current Exynos SoCs primarily. I suppose if you always assume that the registers will appear in a specific order, and never have gaps between them, then you can simply always calculate the addresses as e.g.: reg_func = reg_base reg_pud = reg_func + round_up(num_pins / (32 / func_width)) reg_drv = reg_pud + round_up(num_pins / (32 / func_width)) ... Then, there wouldn't ever be any special cases - that calculation would always work. An alternative would be to put each register's address in DT rather than just the base of the register block. It'd certainly be more future-flexible, even if not strictly necessary.
diff --git a/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file mode 100644 index 0000000..cac7f71 --- /dev/null +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi @@ -0,0 +1,605 @@ +/* + * Samsung's Exynos4210 SoC pinctrl banks device tree source + * + * Copyright (c) 2012 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * + * Samsung's Exynos4210 SoC pin banks are listed as device tree nodes + * in this file. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +/ { + pinctrl@11400000 { + gpa0: pin-bank@0 { + gpio-controller; + samsung,pctl-offset = <0x000>; + samsung,pin-bank = "gpa0"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x00>; + }; + + gpa1: pin-bank@1 { + gpio-controller; + samsung,pctl-offset = <0x020>; + samsung,pin-bank = "gpa1"; + samsung,pin-count = <6>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x04>; + }; + + gpb: pin-bank@2 { + gpio-controller; + samsung,pctl-offset = <0x040>; + samsung,pin-bank = "gpb"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x08>; + }; + + gpc0: pin-bank@3 { + gpio-controller; + samsung,pctl-offset = <0x060>; + samsung,pin-bank = "gpc0"; + samsung,pin-count = <5>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x0C>; + }; + + gpc1: pin-bank@4 { + gpio-controller; + samsung,pctl-offset = <0x080>; + samsung,pin-bank = "gpc1"; + samsung,pin-count = <5>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x10>; + }; + + gpd0: pin-bank@5 { + gpio-controller; + samsung,pctl-offset = <0x0A0>; + samsung,pin-bank = "gpd0"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x14>; + }; + + gpd1: pin-bank@6 { + gpio-controller; + samsung,pctl-offset = <0x0C0>; + samsung,pin-bank = "gpd1"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x18>; + }; + + gpe0: pin-bank@7 { + gpio-controller; + samsung,pctl-offset = <0x0E0>; + samsung,pin-bank = "gpe0"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x1C>; + }; + + gpe1: pin-bank@8 { + gpio-controller; + samsung,pctl-offset = <0x100>; + samsung,pin-bank = "gpe1"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x20>; + }; + + gpe2: pin-bank@9 { + gpio-controller; + samsung,pctl-offset = <0x120>; + samsung,pin-bank = "gpe2"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x24>; + }; + + gpe3: pin-bank@10 { + gpio-controller; + samsung,pctl-offset = <0x140>; + samsung,pin-bank = "gpe3"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x28>; + }; + + gpe4: pin-bank@11 { + gpio-controller; + samsung,pctl-offset = <0x160>; + samsung,pin-bank = "gpe4"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x2C>; + }; + + gpf0: pin-bank@12 { + gpio-controller; + samsung,pctl-offset = <0x180>; + samsung,pin-bank = "gpf0"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x30>; + }; + + gpf1: pin-bank@13 { + gpio-controller; + samsung,pctl-offset = <0x1A0>; + samsung,pin-bank = "gpf1"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x34>; + }; + + gpf2: pin-bank@14 { + gpio-controller; + samsung,pctl-offset = <0x1C0>; + samsung,pin-bank = "gpf2"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x38>; + }; + + gpf3: pin-bank@15 { + gpio-controller; + samsung,pctl-offset = <0x1E0>; + samsung,pin-bank = "gpf3"; + samsung,pin-count = <6>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x3C>; + }; + }; + + pinctrl@11000000 { + gpj0: pin-bank@0 { + gpio-controller; + samsung,pctl-offset = <0x000>; + samsung,pin-bank = "gpj0"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x00>; + }; + + gpj1: pin-bank@1 { + gpio-controller; + samsung,pctl-offset = <0x020>; + samsung,pin-bank = "gpj1"; + samsung,pin-count = <5>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x04>; + }; + + gpk0: pin-bank@2 { + gpio-controller; + samsung,pctl-offset = <0x040>; + samsung,pin-bank = "gpk0"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x08>; + }; + + gpk1: pin-bank@3 { + gpio-controller; + samsung,pctl-offset = <0x060>; + samsung,pin-bank = "gpk1"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x0C>; + }; + + gpk2: pin-bank@4 { + gpio-controller; + samsung,pctl-offset = <0x080>; + samsung,pin-bank = "gpk2"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x10>; + }; + + gpk3: pin-bank@5 { + gpio-controller; + samsung,pctl-offset = <0x0A0>; + samsung,pin-bank = "gpk3"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x14>; + }; + + gpl0: pin-bank@6 { + gpio-controller; + samsung,pctl-offset = <0x0C0>; + samsung,pin-bank = "gpl0"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x18>; + }; + + gpl1: pin-bank@7 { + gpio-controller; + samsung,pctl-offset = <0x0E0>; + samsung,pin-bank = "gpl1"; + samsung,pin-count = <2>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x1C>; + }; + + gpl2: pin-bank@8 { + gpio-controller; + samsung,pctl-offset = <0x100>; + samsung,pin-bank = "gpl2"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x20>; + }; + + gpm0: pin-bank@9 { + gpio-controller; + samsung,pctl-offset = <0x260>; + samsung,pin-bank = "gpm0"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x24>; + }; + + gpm1: pin-bank@10 { + gpio-controller; + samsung,pctl-offset = <0x280>; + samsung,pin-bank = "gpm1"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x28>; + }; + + gpm2: pin-bank@11 { + gpio-controller; + samsung,pctl-offset = <0x2A0>; + samsung,pin-bank = "gpm2"; + samsung,pin-count = <5>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x2C>; + }; + + gpm3: pin-bank@12 { + gpio-controller; + samsung,pctl-offset = <0x2C0>; + samsung,pin-bank = "gpm3"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x30>; + }; + + gpm4: pin-bank@13 { + gpio-controller; + samsung,pctl-offset = <0x2E0>; + samsung,pin-bank = "gpm4"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + + interrupt-controller; + samsung,eint-offset = <0x34>; + }; + + gpy0: pin-bank@14 { + gpio-controller; + samsung,pctl-offset = <0x120>; + samsung,pin-bank = "gpy0"; + samsung,pin-count = <6>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy1: pin-bank@15 { + gpio-controller; + samsung,pctl-offset = <0x140>; + samsung,pin-bank = "gpy1"; + samsung,pin-count = <4>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy2: pin-bank@16 { + gpio-controller; + samsung,pctl-offset = <0x160>; + samsung,pin-bank = "gpy2"; + samsung,pin-count = <6>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy3: pin-bank@17 { + gpio-controller; + samsung,pctl-offset = <0x180>; + samsung,pin-bank = "gpy3"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy4: pin-bank@18 { + gpio-controller; + samsung,pctl-offset = <0x1A0>; + samsung,pin-bank = "gpy4"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy5: pin-bank@19{ + gpio-controller; + samsung,pctl-offset = <0x1C0>; + samsung,pin-bank = "gpy5"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpy6: pin-bank@20 { + gpio-controller; + samsung,pctl-offset = <0x1E0>; + samsung,pin-bank = "gpy6"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + samsung,conpdn-width = <2>; + samsung,pudpdn-width = <2>; + }; + + gpx0: pin-bank@21 { + gpio-controller; + samsung,pctl-offset = <0xC00>; + samsung,pin-bank = "gpx0"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + }; + + gpx1: pin-bank@22 { + gpio-controller; + samsung,pctl-offset = <0xC20>; + samsung,pin-bank = "gpx1"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + }; + + gpx2: pin-bank@23 { + gpio-controller; + samsung,pctl-offset = <0xC40>; + samsung,pin-bank = "gpx2"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + }; + + gpx3: pin-bank@24 { + gpio-controller; + samsung,pctl-offset = <0xC60>; + samsung,pin-bank = "gpx3"; + samsung,pin-count = <8>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + }; + }; + + pinctrl@03860000 { + gpz: pin-bank@0 { + gpio-controller; + samsung,pctl-offset = <0x000>; + samsung,pin-bank = "gpz"; + samsung,pin-count = <7>; + samsung,func-width = <4>; + samsung,pud-width = <2>; + samsung,drv-width = <2>; + }; + }; +}; diff --git a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi index b12cf27..94846d5 100644 --- a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi @@ -14,6 +14,8 @@ * published by the Free Software Foundation. */ +/include/ "exynos4210-pinctrl-banks.dtsi" + / { pinctrl@11400000 { uart0_data: uart0-data { diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644 --- a/arch/arm/boot/dts/exynos4210.dtsi +++ b/arch/arm/boot/dts/exynos4210.dtsi @@ -59,6 +59,10 @@ reg = <0x11400000 0x1000>; interrupts = <0 47 0>; interrupt-controller; + samsung,geint-con = <0x700>; + samsung,geint-mask = <0x900>; + samsung,geint-pend = <0xA00>; + samsung,svc = <0xB08>; #interrupt-cells = <2>; }; @@ -67,6 +71,10 @@ reg = <0x11000000 0x1000>; interrupts = <0 46 0>; interrupt-controller; + samsung,geint-con = <0x700>; + samsung,geint-mask = <0x900>; + samsung,geint-pend = <0xA00>; + samsung,svc = <0xB08>; #interrupt-cells = <2>; wakup_eint: wakeup-interrupt-controller { @@ -79,6 +87,10 @@ <0 24 0>, <0 25 0>, <0 26 0>, <0 27 0>, <0 28 0>, <0 29 0>, <0 30 0>, <0 31 0>, <0 32 0>; + samsung,weint-count = <32>; + samsung,weint-con = <0xE00>; + samsung,weint-mask = <0xF00>; + samsung,weint-pend = <0xF40>; }; };