Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC Timer

[Rob Herring]

On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@xxxxxxxxx wrote:
> > > From: Shruthi Sanil <shruthi.sanil@xxxxxxxxx>
> > >
> > > Add Device Tree bindings for the Timer IP, which can be used as
> > > clocksource and clockevent device in the Intel Keem Bay SoC.
>
> ...
>
> > > +    soc {
> > > +        #address-cells = <0x2>;
> > > +        #size-cells = <0x2>;
> > > +
> > > +        gpt@20331000 {
> > > +            compatible = "intel,keembay-gpt-creg", "simple-mfd";
> >
> > It looks like you are splitting things based on Linux implementation
> > details. Does this h/w block have different combinations of timers and
> > counters? If not, then you don't need the child nodes at all. There's
> > plenty of h/w blocks that get used as both a clocksource and clockevent.
> >
> > Maybe I already raised this, but assume I don't remember and this patch
> > needs to address any questions I already asked.
>
> I dunno if I mentioned that hardware seems to have 5 or so devices behind
> the block, so ideally it should be one device node that represents the global
> register spaces and several children nodes.

Is it 5 devices or 9 devices?

> However, I am not familiar with the established practices in DT world, but
> above seems to me the right thing to do since it describes the hardware as
> is (without any linuxisms).

The Linuxism in these cases defining 1 node per driver because that's
what is convenient for automatic probing. That appears to be exactly
the case here. The red flag is nodes with a compatible and nothing
else. The next question is whether the sub-devices are blocks that
will be assembled in varying combinations and quantities. If not, then
not much point subdividing the h/w blocks.

There's also many cases of having multiple 'identical' timers and
wanting to encode which timer gets assigned to clocksource vs.
clockevent. But those 'identical' timers aren't if you care about
which timer gets assigned where. I *think* that's not the case here
unless you are trying to pick the timer for the clockevent by not
defining the other timers.

Without having a complete picture of what's in 'gpt-creg', I can't
give better advice.

Rob