Re: [RFC/PATCH 2/7] arm: omap: devicetree: add new properties for OMAP devices

Tony Lindgren
2014-12-11T17:11:45Z

Hi,

* Felipe Balbi <balbi@ti.com> [141211 06:24]:
> On Thu, Dec 11, 2014 at 01:46:28AM +0100, Sebastian Reichel wrote:
> >
> > Each IP-Core connected to the bus of OMAP processors has
> > three registers, which specify the IP-Core's version, its
> > status and setup of PM features.
> >
> > Required Properties:
> > - ti,prcm-type: must be one of the following:
> > 1 for OMAP2+ register style,
> > 2 for OMAP4+ register style,
> > 3 for AM33xx register style

This is the simple part and tells the type of the wiring of
the device :)

> > - reg: offset to revision, config and status registers
> > relative to module base address

I don't think we should set up the sysconf registers as a child
of the device. These registers are not a child of the device on
the bus. They are sprinkled within the driver reg space. And some
of these sysc registers are way into the driver registers, it's
not always 0-0x10-0x14 layout for the sysconfig register offsets.

> > Optional Properties:
> > - ti,idlemodes: bit field of flags (SIDLE)
> > PRCM_IDLE_FORCE (1 << 0)
> > PRCM_IDLE_NO (1 << 1)
> > PRCM_IDLE_SMART (1 << 2)
> > PRCM_IDLE_SMART_WKUP (1 << 3)
> > - ti,standbymodes: bit field of flags (MIDLE)
> > PRCM_STANDBY_FORCE (1 << 0)
> > PRCM_STANDBY_NO (1 << 1)
> > PRCM_STANDBY_SMART (1 << 2)
> > PRCM_STANDBY_SMART_WKUP (1 << 3)

These look OK to me. They describe the features available in the
sysconfig registers that map in a different way depending of the
sysconfig type 1, 2 or 3.

Too bad we cannot autoprobe this information from the hardware.

> > - ti,sysc-has-autoidle: config register has AUTOIDLE bit
> > - ti,sysc-has-softreset: config register has SOFTRESET bit
> > - ti,sysc-has-enawakeup: config register has ENAWAKEUP bit
> > - ti,sysc-has-emufree: config register has EMUFREE bit
> > - ti,sysc-has-clock-activity: config register has CLOCKACTIVITY bit
> > - ti,sysc-has-dma-disable: config register has DMADISABLE bit
> > - ti,sysc-has-reset-status: config register has RESETDONE bit
> > - ti,syss-has-reset-status: status register has RESETDONE bit
>
> I thought about having boolean flags like this but after talking to Tony
> we agreed that it would just increase the amount of string parsing
> during early initialization. Besides, why have two properties using an
> integer bitfield and another huge number of boolean properties ?

Yeah setting up individual register bits as strings is just too
much java like.. Setting them up like this means we are doing
devices * nr_sysc_feature_bits of string parsing. So I'd like to
avoid that by using bits like Felipe has done now that we have the
dts preprocessing happening.

> > - ti,reset-delay-us: reset delay in us
> >
> > Example:
> >
> > ocp {
> > gpio1: gpio@48310000 {
> > compatible = "ti,omap3-gpio";
> >
> > ... /* IP-Core specific properties */
> >
> > ti,sysconfig {
>
> this is what I'm still waiting for comments from Tony. I'm not convinced
> we need this extra indentation level. It really brings nothing of value.

Me neither. It's also wrong from the register mapping point of
view like I explained above.

Cheers,

Tony