On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote: > On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@xxxxxxx> wrote: > > Hi Jassi, > > > > On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote: > >> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@xxxxxxxxxx> wrote: > >>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote: > >>> > >>>> Being more specific to your platform, I think you need some server > >>>> code (mailbox's client) that every driver (like clock, pmu, pinmux > >>>> etc) registers with to send messages to remote and receive commands > >>>> from remote ... perhaps by registering some filter to sort out > >>>> messages for each driver. > >>> > >>> Right, and here's where you hit on the problem. This server you mention > >>> is not a piece of hardware, it would be a software construct. As such, it > >>> doesn't fit into the DT binding as it exists. It's probably best to > >>> illustrate in DT syntax. > >>> > >>> If I were to represent the hardware relationship in the DT binding now > >>> it would look like this: > >>> > >>> --- > >>> cpm: mailbox@deadbeef { > >>> compatible = "brcm,bcm-cpm-mailbox"; > >>> reg = <...>; > >>> #mbox-cells <1>; > >>> interrupts = <...>; > >>> }; > >>> > >>> /* clock complex */ > >>> ccu { > >>> compatible = "brcm,bcm-foo-ccu"; > >>> mbox = <&cpm CPM_SYSTEM_CHANNEL>; > >>> mbox-names = "system"; > >>> /* leaving out other mailboxes for brevity */ > >>> #clock-cells <1>; > >>> clock-output-names = "bar", > >>> "baz"; > >>> }; > >>> > >>> pmu { > >>> compatible = "brcm,bcm-foo-pmu" > >>> mbox = <&cpm CPM_SYSTEM_CHANNEL>; > >>> mbox-names = "system"; > >>> }; > >>> > >>> pinmux { > >>> compatible = "brcm,bcm-foo-pinctrl"; > >>> mbox = <&cpm CPM_SYSTEM_CHANNEL>; > >>> mbox-names = "system"; > >>> }; > >>> --- > >> Yeah, I too don't think its a good idea. > >> > >> > >>> What we would need to do is completely ignore this information in each > >>> of the of the client drivers associated with the clock, pmu, and pinmux > >>> devices. This IPC server would need to be instantiated and get the > >>> mailbox information from some source. mbox_request_channel() only works > >>> when the client has an of_node with the mbox-names property present. > >>> Let's say we follow this model and represent it in DT: > >>> > >>> -- > >>> cpm: mailbox@deadbeef { > >>> compatible = "brcm,bcm-cpm-mailbox"; > >>> reg = <...>; > >>> #mbox-cells <1>; > >>> interrupts = <...>; > >>> }; > >>> > >>> cpm_ipc { > >>> compatible = "brcm,bcm-cpm-ipc"; > >>> mbox = <&cpm CPM_SYSTEM_CHANNEL>; > >>> mbox-names = "system"; > >>> /* leaving out other mailboxes for brevity */ > >>> }; > >>> --- > >>> > >>> This would allow an ipc driver to exclusively own this system channel, > >>> but now we've invented a binding that doesn't reflect the hardware at > >>> all. It's describing software so I don't believe the DT maintainers will > >>> allow this type of construct. > >>> > >> Must the server node specify MMIO and an IRQ, to be acceptable? Like ... > >> > >> cpm_ipc : cpm@deadbeef { > >> compatible = "brcm,bcm-cpm-ipc"; > >> /* reg = <0xdeadbeef 0x100>; */ > >> /* interrupts = <0 123 4>; */ > >> mbox = <&cpm CPM_SYSTEM_CHANNEL>; > >> mbox-names = "system"; > >> }; > >> > >> cpm_ipc already specifies a hardware resource (mbox) that its driver > >> needs, I think that should be enough reason. If it were some purely > >> soft property for the driver like > >> mode = "poll"; //or "irq" > >> then the node wouldn't be justified because that is the job of a > >> build-time config or run-time module option. > >> > > > > Like Matt, I am also in similar situation where there's a lot of common > > code necessary to construct/parse IPCs for each of the drivers using the > > mailbox. > > > > As per your suggestion if we have single DT node to specify both the > > controller and the client, we might still have to pollute this node with > > software specific compatibles. > > > I am afraid you misunderstood me. I don't suggest single node for > mailbox controller and client, and IIUC, neither did Matt. Please note > the controller is cpm and client is cpm_ipc. Correct, I had separate controller and consumer nodes as written above...to match the binding. > BTW, here we at least have a hardware resource to specify in the DT > node, there are examples in kernel where the DT nodes are purely > virtual. For ex, grep for "linux,spdif-dit". So I think we should be > ok. > There's a bit of a difference between my concern over a virtual node and this example you've cited. In the dummy spdif transmitter, it's defining a virtual device that plugs in for a codec, a hardware concept well defined in the audio bindings. I agree that there are many examples of this type of virtual node, including dummy phys, but in all cases they are stubbing out a real hardware concept. I find it to be distinctly different to create a node that doesn't represent the hardware's use of mailboxes. I'd be happy if a DT maintainer could say that this is acceptable though. ;) -Matt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html