Re: [PATCH v5 05/10] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@xxxxxx> [190219 08:51]:
> Hi Tony,
> 
> On 18/02/19 8:02 PM, Tony Lindgren wrote:
> > * Lokesh Vutla <lokeshvutla@xxxxxx> [190216 03:30]:
> >> On 2/15/2019 9:46 PM, Tony Lindgren wrote:
> >>> The dts node for the interrupt controller should describe a
> >>> proper Linux device, that is with reg entries and so on.
> >>
> >> You are asking to just keep the compatible property :)
> > 
> > Right, and then I realized this node is missing the standard
> > reg entry too. And you're saying the registers are not even
> > accissible from Linux.
> > 
> > So based on that IMO you should not even have a device tree
> > node for it at all. You should just have the interrupt
> 
> Practically lets look at what all I am adding in the DT node. Below is one such
> example:
> 
> main_intr: interrupt-controller0 {
> 	compatible = "ti,sci-intr";
> 	interrupt-controller;
> 	interrupt-parent = <&gic500>;
> 	#interrupt-cells = <4>;
> 	ti,sci = <&dmsc>;
> 	ti,sci-dst-id = <56>;
> 	ti,sci-rm-range-girq = <0x1>;
> };
> 
> The following 4 properties are required at least for probing, to represent the
> hierarchy and for  interrupt definition:
> 	compatible = "ti,sci-intr";
> 	interrupt-controller;
> 	interrupt-parent = <&gic500>;
> 	#interrupt-cells = <4>;
> 
> The remaining 3 properties represents the TISCI interface. Let's go step by step:
> * ti,sci = <&dmsc> :This is the phandle to the firmware protocol driver using
> which the messages are sent
> * ti,sci-dst-id = <56> : This is the TISCI device ID for the parent controller
> for which your irqs needs to be connected. As I said this cannot be queried from
> sysfw and this is the input to the messages that are send to sysfw.

Let's not add anything that does not describe hardware to the device
tree. This is ID is an invented number used by the firmware.

> *  ti,sci-rm-range-girq = <0x1>: This define the ids using which the parent-irq
> ranges that are allocated to this interrupt router instance can be queried from
> sysfw.
> If the above 2 properties are to be added as driver phandle then for every
> instance of interrupt router in the SoC, a new compatible needs to be created. I
> don't think this is a desirable solution.

To me it seems that the interrupt router _must_ have proper IO
configuration registers available to the Linux running SoC.

Are you sure the interrupt route does not have proper IO
configuration registers available for the Linux running SoC?

If the there are not, I'd be surprised how the SoC is designed :)

So assuming it does, you should just use the standard device tree
reg property to differentiate between the various interrupt router
instances. And then you can have the driver talk to the firmware
in a way where the driver instances are separate even if no IO
access to these shared registers is done by the Linux running SoC.

But see also the mux comment below.

> With this can you tell me how can we not have a device-tree and still support
> irq allocation?

Using standard dts reg property to differentiate the interrupt
router instances. And if the interrupt router is a mux, you should
treat it as a mux rather than a chained interrupt controller.

We do have drivers/mux nowadays, not sure if it helps in this case
as at least timer interrupts need to be configured very early.

> Also, this is not the first time a driver based on a firmware is being added.
> K2g clock, power and reset drivers are based on this where device ids are being
> passed from consumers. Similarly arm scpi based drivers are also available.

Having drivers communicate with firmware is quite standard.

However, stuffing firmware specific data to the device tree
does not describe the hardware and must not be done.

Regards,

Tony