Hi Rob,
> -----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.
>
I tried to get the technical manual that could be shared publicly, but couldn't find one which has the timer IP details.
In the previous reply above, I have tried to explain the details of the Timer IP. Could you please let me know if I was able to answer your question?
I think the concern point here is the common register that needs to be accessed by the timer during initialization. Same address cannot be defined in multiple timer nodes and also we cannot have this in the driver too as offset because this offset is changing from platform to platform. Hence we have gone with the parent child approach. I need your help here on whats the best possible way to do this?
I need your support in taking this patch forward?
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
