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

[Sanil, Shruthi]

Hi Rob,

I tried to the technical manual that could be shared publicly, but couldn't find one which had the timer IP details.
In the email below I have tried to answer your question. Could you please let me know if I was able to answer your question?
Can we try to discuss and close it in the best possible way? Need your help in taking this patch forward?

Regards,
Shruthi

> -----Original Message-----
> From: Sanil, Shruthi
> Sent: Friday, March 18, 2022 11:07 AM
> To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>
> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; Thomas Gleixner
> <tglx@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Mark Gross <mgross@xxxxxxxxxxxxxxx>;
> Thokala, Srikanth <srikanth.thokala@xxxxxxxxx>; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>; Sangannavar,
> Mallikarjunappa <mallikarjunappa.sangannavar@xxxxxxxxx>
> Subject: RE: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem
> Bay SoC Timer
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Sent: Tuesday, March 8, 2022 3:44 PM
> > To: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Sanil, Shruthi <shruthi.sanil@xxxxxxxxx>; Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>;
> > linux- kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Mark Gross
> > <mgross@xxxxxxxxxxxxxxx>; Thokala, Srikanth
> > <srikanth.thokala@xxxxxxxxx>; Raja Subramanian, Lakshmi Bai
> > <lakshmi.bai.raja.subramanian@xxxxxxxxx>;
> > Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@xxxxxxxxx>
> > Subject: Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel
> > Keem Bay SoC Timer
> >
> > On Mon, Mar 07, 2022 at 04:33:23PM -0600, Rob Herring wrote:
> > > 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?
> >
> > 5 devices, one of which is a timer block out of 8 timers.
> > You may count them as 12 altogether.
> >
> > > > 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.
> >
> > AFAIU the hardware architecture the amount of timers is dependent on
> > the IP synthesis configuration. On this platform it's 8, but it may be
> > 1 or 2, for example.
> 
> Yes, the number of timers can vary between platforms.
> For eg., Intel Keem Bay SoC has 8 timers where as in Intel Thunder Bay SoC
> has 6 timers.
> 
> >
> > > 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.
> >
> > I guess they need to share TRM, if possible, to show what this block is.
> >
> 
> I would like to explain briefly about the Timer IP in the Keem Bay Soc.
> The Timers block contains 8 general purpose timers, a free running counter.
> Each general purpose timer can generate an individual interrupt to the
> interrupt controller.
> The timer block consists of secure and non-secure timers. Hence there are
> secure and non-secure registers in separate address banks.
> The secure register bank consists of the common control register where the
> timers and counters need to be enabled.
> From the driver we try to check if these bits are enabled to continue with the
> initialization of the driver.
> Hence we need to pass the base address of both the address banks to the
> driver from the DTB.
> The control register is common for both timer and counter. Hence we went
> for parent child module in DTB. 'gpt-creg' represents this control register.
> 
> > --
> > With Best Regards,
> > Andy Shevchenko
> >