Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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? -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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,
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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! -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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 ther
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at h
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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 = <0x1140 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 dif
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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 = <0x1140 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. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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@1140 {
> > + 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 = <0x1140 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 wh
Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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@1140 {
> + 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 = <0x1140 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers
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.
Signed-off-by: Tomasz Figa
Signed-off-by: Kyungmin Park
---
arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi | 605
arch/arm/boot/dts/exynos4210-pinctrl.dtsi | 2 +
arch/arm/boot/dts/exynos4210.dtsi | 12 +
3 files changed, 619 insertions(+)
create mode 100644 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
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 000..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@1140 {
+ 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;
+
