Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Zyngier [off-list ref] writes:

quoted

+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;

As I said before, this assumes the outer DT node uses a ranges
property.  Normally reg properties work the same whether they specify an
offset within an outer "ranges" or have a full address directly.  It
would be easy enough to make this work with either, so I don't see any
reason not to.

-- 
M?ns Rullg?rd

On 20/01/16 16:10, M?ns Rullg?rd wrote:

Marc Zyngier [off-list ref] writes:

quoted

quoted

+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;

As I said before, this assumes the outer DT node uses a ranges
property.  Normally reg properties work the same whether they specify an
offset within an outer "ranges" or have a full address directly.  It
would be easy enough to make this work with either, so I don't see any
reason not to.

Yup, that is a good point. I guess Marc can address this in the next
round, since we need a DT binding anyway.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

On 20/01/2016 17:10, M?ns Rullg?rd wrote:

Marc Zyngier wrote:

quoted

quoted

+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;

As I said before, this assumes the outer DT node uses a ranges
property.  Normally reg properties work the same whether they specify an
offset within an outer "ranges" or have a full address directly.  It
would be easy enough to make this work with either, so I don't see any
reason not to.

IIRC, I was told very early in the review process that the ranges prop
was mandatory. Lemme look for it... It was Arnd:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/444131/focus=444207

You are missing a ranges property that describes what address
space these addresses are in.

'ranges;' would be wrong here, as the interrupt controller is
not a bus. If you have no ranges property, the bus is interpreted
as having its own address space with no relation to the parent bus
(e.g. an I2C bus uses addresses that are not memory mapped).

Just list the addresses that are actually decoded by child
devices here.

Did I misunderstand?

Regards.

On Wed, Jan 20, 2016 at 05:24:14PM +0100, Marc Gonzalez wrote:

On 20/01/2016 17:10, M?ns Rullg?rd wrote:

quoted

Marc Zyngier wrote:

quoted

quoted

+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;

As I said before, this assumes the outer DT node uses a ranges
property.  Normally reg properties work the same whether they specify an
offset within an outer "ranges" or have a full address directly.  It
would be easy enough to make this work with either, so I don't see any
reason not to.

IIRC, I was told very early in the review process that the ranges prop
was mandatory. Lemme look for it... It was Arnd:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/444131/focus=444207

I believe Arnd's point was that you need _a_ ranges property, rather
than specifically requiring an idmap/empty ranges property.

As Marc pointed out, you can use of_iomap on the child nodes to map the
portions described by the reg properties, which will handle any
ranges-based translation automatically.

When you put together the binding document, please point out that the
ranges property is necessary.

Thanks,
Mark.

On 20/01/2016 18:02, Mark Rutland wrote:

When you put together the binding document, please point out that the
ranges property is necessary.

I don't quite understand.

Can I write the node without a ranges property, and the driver would
still function correctly?

Regards.