Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

2012-09-25 Thread Stephen Warren 
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

2012-09-25 Thread Stephen Warren 
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

2012-09-25 Thread Stephen Warren 
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

2012-09-24 Thread Stephen Warren 
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

2012-09-21 Thread Stephen Warren 
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

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

2012-09-21 Thread Tomasz Figa 
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 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