Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1230092 > unrolled thread

Re: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings

Started by"majun (F)" <majun258@huawei.com>
First post2015-09-22 13:40 +0200
Last post2015-09-23 09:30 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings "majun (F)" <majun258@huawei.com> - 2015-09-22 13:40 +0200
    Re: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings Marc Zyngier <marc.zyngier@arm.com> - 2015-09-22 16:50 +0200
      Re: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings "majun (F)" <majun258@huawei.com> - 2015-09-23 09:30 +0200

#1230092 — Re: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings

From"majun (F)" <majun258@huawei.com>
Date2015-09-22 13:40 +0200
SubjectRe: [PATCH v4 2/2] dt-binding:Documents of the mbigen bindings
Message-ID<qbu5s-45y-23@gated-at.bofh.it>
Hi Marc:

在 2015/9/22 2:09, Marc Zyngier 写道:
> On Wed, 19 Aug 2015 03:55:20 +0100
> MaJun <majun258@huawei.com> wrote:
> 
[...]
>> +These nodes must have the following properties:
>> +- msi-parent: This property has two cells.
>> +	The 1st cell specifies the ITS this device connected.
>> +	The 2nd cell specifies the device id.
>> +- nr-interrupts:Specifies the total number of interrupt this device has.
>> +- mbigen_node: Specifies the information of mbigen nodes this device
>> +  connected.Some devices with many interrupts maybe connects with several
>> +  mbigen nodes.
>> +
>> +Examples:
>> +
>> +		mbigen_dsa: interrupt-controller@c0080000 {
>> +			compatible = "hisilicon,mbigen-v2";
>> +			interrupt-controller;
>> +			#interrupt-cells = <5>;
>> +			#mbigen-node-cells = <3>;
>> +			reg = <0xc0080000 0x10000>;
>> +
>> +			mbigen_device_01 {
>> +				msi-parent = <&its 0x40b1c>;
>> +				nr-interrupts = <9>;
>> +				mbigen_node = <1 2 0>,
>> +							<3 2 4>,
>> +							<4 5 0>;
>> +			}
>> +
>> +			mbigen_device_02 {
>> +				msi-parent = <&its 0x40b1d>;
>> +				nr-interrupts = <3>;
>> +				mbigen_node = <6 3 0>;
>> +				interrupt-controller;
>> +			}
>> +		};
>> +
>> +Device connect to mbigen required properties:
>> +----------------------------------------------------
>> +-interrupt-parent: Specifies the mbigen node which device connected.
>> +-interrupts:specifies the interrupt source.The first cell is hwirq num, the
>> +  second number is trigger type.
>> +
>> +Examples:
>> +	smmu_dsa {
>> +		compatible = "arm,smmu-v3";
>> +		reg = <0x0 0xc0040000 0x0 0x20000>;
>> +		interrupt-parent  = <&mbigen_dsa>;
>> +		interrupts = <0x40b20 6 78 1>,
>> +				<0x40b20 6 79 1>,
>> +				<0x40b20 6 80 1>;
>> +	};
>> +
> 
> I find the current split very confusing. In your example, the interrupt
> controller is the mbigen block, which forces you to encode the DevID as
> part of the interrupt specifier. This doesn't feel like an ideal
> design, because you end up duplicating the DevID information at both
> the "client" device and the mbi_device.
> 
> I'd be more inclined to have the mbi_device itself be the interrupt
> controller for the client device. This would eliminate information
> duplication, and reflect the hardware (or what I understand of the
> hardware) a bit more.

Do you mean make the dts likes below?


		mbigen_dsa: interrupt-controller@c0080000 {
			compatible = "hisilicon,mbigen-v2";
			interrupt-controller;
			#interrupt-cells = <5>;
			#mbigen-node-cells = <3>;
			reg = <0xc0080000 0x10000>;
			
			mbigen_client_device1 {
				msi-parent = <&its 0x40b1c>;
				nr-interrupts = <9>;
				interrupt-controller;
			}

			mbigen_client_device2{
				msi-parent = <&its 0x40b1d>;
				nr-interrupts = <3>;
				interrupt-controller;
			}
		};


	client_device1 {
		compatible = "arm,smmu-v3";
		reg = <0x0 0xc0040000 0x0 0x20000>;
		interrupt-parent  = <&mbigen_client_device1>;
		interrupts = <pin_offset 78 1>,
				<pin_offset 79 1>,
				<pin_offset 80 1>;
	};


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1230291

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-09-22 16:50 +0200
Message-ID<qbx3j-8hS-15@gated-at.bofh.it>
In reply to#1230092
On Tue, 22 Sep 2015 19:35:38 +0800
"majun (F)" <majun258@huawei.com> wrote:

> Hi Marc:
> 
> 在 2015/9/22 2:09, Marc Zyngier 写道:
> > On Wed, 19 Aug 2015 03:55:20 +0100
> > MaJun <majun258@huawei.com> wrote:
> > 
> [...]
> >> +These nodes must have the following properties:
> >> +- msi-parent: This property has two cells.
> >> +	The 1st cell specifies the ITS this device connected.
> >> +	The 2nd cell specifies the device id.
> >> +- nr-interrupts:Specifies the total number of interrupt this device has.
> >> +- mbigen_node: Specifies the information of mbigen nodes this device
> >> +  connected.Some devices with many interrupts maybe connects with several
> >> +  mbigen nodes.
> >> +
> >> +Examples:
> >> +
> >> +		mbigen_dsa: interrupt-controller@c0080000 {
> >> +			compatible = "hisilicon,mbigen-v2";
> >> +			interrupt-controller;
> >> +			#interrupt-cells = <5>;
> >> +			#mbigen-node-cells = <3>;
> >> +			reg = <0xc0080000 0x10000>;
> >> +
> >> +			mbigen_device_01 {
> >> +				msi-parent = <&its 0x40b1c>;
> >> +				nr-interrupts = <9>;
> >> +				mbigen_node = <1 2 0>,
> >> +							<3 2 4>,
> >> +							<4 5 0>;
> >> +			}
> >> +
> >> +			mbigen_device_02 {
> >> +				msi-parent = <&its 0x40b1d>;
> >> +				nr-interrupts = <3>;
> >> +				mbigen_node = <6 3 0>;
> >> +				interrupt-controller;
> >> +			}
> >> +		};
> >> +
> >> +Device connect to mbigen required properties:
> >> +----------------------------------------------------
> >> +-interrupt-parent: Specifies the mbigen node which device connected.
> >> +-interrupts:specifies the interrupt source.The first cell is hwirq num, the
> >> +  second number is trigger type.
> >> +
> >> +Examples:
> >> +	smmu_dsa {
> >> +		compatible = "arm,smmu-v3";
> >> +		reg = <0x0 0xc0040000 0x0 0x20000>;
> >> +		interrupt-parent  = <&mbigen_dsa>;
> >> +		interrupts = <0x40b20 6 78 1>,
> >> +				<0x40b20 6 79 1>,
> >> +				<0x40b20 6 80 1>;
> >> +	};
> >> +
> > 
> > I find the current split very confusing. In your example, the interrupt
> > controller is the mbigen block, which forces you to encode the DevID as
> > part of the interrupt specifier. This doesn't feel like an ideal
> > design, because you end up duplicating the DevID information at both
> > the "client" device and the mbi_device.
> > 
> > I'd be more inclined to have the mbi_device itself be the interrupt
> > controller for the client device. This would eliminate information
> > duplication, and reflect the hardware (or what I understand of the
> > hardware) a bit more.
> 
> Do you mean make the dts likes below?
> 
> 
> 		mbigen_dsa: interrupt-controller@c0080000 {
> 			compatible = "hisilicon,mbigen-v2";
> 			interrupt-controller;
> 			#interrupt-cells = <5>;

These two statements shouldn't be here...

> 			#mbigen-node-cells = <3>;
> 			reg = <0xc0080000 0x10000>;
> 			
> 			mbigen_client_device1 {
> 				msi-parent = <&its 0x40b1c>;
> 				nr-interrupts = <9>;
> 				interrupt-controller;

This is where #interrupt-cells should be.

> 			}
> 
> 			mbigen_client_device2{
> 				msi-parent = <&its 0x40b1d>;
> 				nr-interrupts = <3>;
> 				interrupt-controller;
> 			}
> 		};
> 
> 
> 	client_device1 {
> 		compatible = "arm,smmu-v3";
> 		reg = <0x0 0xc0040000 0x0 0x20000>;
> 		interrupt-parent  = <&mbigen_client_device1>;
> 		interrupts = <pin_offset 78 1>,
> 				<pin_offset 79 1>,
> 				<pin_offset 80 1>;
> 	};
> 
> 

But otherwise, this looks better.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231192

From"majun (F)" <majun258@huawei.com>
Date2015-09-23 09:30 +0200
Message-ID<qbMF3-5Lu-11@gated-at.bofh.it>
In reply to#1230291

在 2015/9/22 22:41, Marc Zyngier 写道:
> On Tue, 22 Sep 2015 19:35:38 +0800
> "majun (F)" <majun258@huawei.com> wrote:
> 
[...]
>>>> +Examples:
>>>> +	smmu_dsa {
>>>> +		compatible = "arm,smmu-v3";
>>>> +		reg = <0x0 0xc0040000 0x0 0x20000>;
>>>> +		interrupt-parent  = <&mbigen_dsa>;
>>>> +		interrupts = <0x40b20 6 78 1>,
>>>> +				<0x40b20 6 79 1>,
>>>> +				<0x40b20 6 80 1>;
>>>> +	};
>>>> +
>>>
>>> I find the current split very confusing. In your example, the interrupt
>>> controller is the mbigen block, which forces you to encode the DevID as
>>> part of the interrupt specifier. This doesn't feel like an ideal
>>> design, because you end up duplicating the DevID information at both
>>> the "client" device and the mbi_device.
>>>
>>> I'd be more inclined to have the mbi_device itself be the interrupt
>>> controller for the client device. This would eliminate information
>>> duplication, and reflect the hardware (or what I understand of the
>>> hardware) a bit more.
>>
>> Do you mean make the dts likes below?
>>
>>
>> 		mbigen_dsa: interrupt-controller@c0080000 {
>> 			compatible = "hisilicon,mbigen-v2";
>> 			interrupt-controller;
>> 			#interrupt-cells = <5>;
> 
> These two statements shouldn't be here...

According to you suggest that irq chip should be probed with IRQCHIP_DECLARE(),
I think the compatible string also should be moved into mbigen_clinet_device node.

And, changing mbigen driver likes below:

IRQCHIP_DECLARE(hisi_mbigen, "hisilicon,mbigen-v2", mbigen_of_init);

Thanks!
Ma Jun

> 
>> 			#mbigen-node-cells = <3>;
>> 			reg = <0xc0080000 0x10000>;
>> 			
>> 			mbigen_client_device1 {
>> 				msi-parent = <&its 0x40b1c>;
>> 				nr-interrupts = <9>;
>> 				interrupt-controller;
> 
> This is where #interrupt-cells should be.
> 
>> 			}
>>
>> 			mbigen_client_device2{
>> 				msi-parent = <&its 0x40b1d>;
>> 				nr-interrupts = <3>;
>> 				interrupt-controller;
>> 			}
>> 		};
>>
>>
>> 	client_device1 {
>> 		compatible = "arm,smmu-v3";
>> 		reg = <0x0 0xc0040000 0x0 0x20000>;
>> 		interrupt-parent  = <&mbigen_client_device1>;
>> 		interrupts = <pin_offset 78 1>,
>> 				<pin_offset 79 1>,
>> 				<pin_offset 80 1>;
>> 	};
>>
>>
> 
> But otherwise, this looks better.
> 
> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web