| Message ID | 1506604306-20739-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| Headers | show |
| Series | firmware: ARM System Control and Management Interface(SCMI) support | expand |
On 28/09/17 22:18, Ulf Hansson wrote: > On 28 September 2017 at 15:11, Sudeep Holla <sudeep.holla@arm.com> wrote: >> This patch hooks up the support for device power domain provided by >> SCMI using the Linux generic power domain infrastructure. >> >> Cc: Kevin Hilman <khilman@baylibre.com> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/firmware/Kconfig | 13 +++ >> drivers/firmware/arm_scmi/Makefile | 1 + >> drivers/firmware/arm_scmi/scmi_pm_domain.c | 134 +++++++++++++++++++++++++++++ >> 3 files changed, 148 insertions(+) >> create mode 100644 drivers/firmware/arm_scmi/scmi_pm_domain.c >> [...] >> + for (i = 0; i < num_domains; i++, scmi_pd++) { >> + domains[i] = &scmi_pd->genpd; >> + >> + scmi_pd->domain = i; >> + scmi_pd->handle = handle; >> + scmi_pd->name = handle->power_ops->name_get(handle, i); >> + scmi_pd->genpd.name = scmi_pd->name; >> + scmi_pd->genpd.power_off = scmi_pd_power_off; >> + scmi_pd->genpd.power_on = scmi_pd_power_on; >> + >> + /* >> + * Treat all power domains as off at boot. >> + * >> + * The SCP firmware itself may have switched on some domains, >> + * but for reference counting purpose, keep it this way. >> + */ > > I think it would be better to give the correct information about the > state of the PM domain. Otherwise genpd may end up thinking a PM > domain is off, while in fact it's on - thus wasting power. > Agreed, I missed to notice that. I will fixup and send the update. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > + > +The SCMI is intended to allow agents such as OSPM to manage various functions > +that are provided by the hardware platform it is running on, including power > +and performance functions. > + > +This binding is intended to define the interface the firmware implementing > +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control > +and Management Interface Platform Design Document")[0] provide for OSPM in > +the device tree. > + > +Required properties: > + > +The scmi node with the following properties shall be under the /firmware/ node. > + > +- compatible : shall be "arm,scmi" > +- mboxes: List of phandle and mailbox channel specifiers. It should contain > + exactly one or two mailboxes, one for transmitting messages("tx") > + and another optional for receiving the notifications("rx") if > + supported. > +- mbox-names: shall be "tx" or "rx" The example below does not have the mbox-names property. If you require exactly two mailboxes, why do you need the names anyway? However, your example does have a #addresss-cells/#size-cells property that are not documented here. Please add them here as either optional or required, and describe what the permitted values are and how the address is interpreted. > +- shmem : List of phandle pointing to the shared memory(SHM) area as per > + generic mailbox client binding. > + > +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details > +about the generic mailbox controller and client driver bindings. > + > +The mailbox is the only permitted method of calling the SCMI firmware. > +Mailbox doorbell is used as a mechanism to alert the presence of a > +messages and/or notification. This looks odd: why not make the message itself part of the mailbox protocol here, and leave the shmem as a implementation detail of the mailbox driver? > +Each protocol supported shall have a sub-node with corresponding compatible > +as described in the following sections. If the platform supports dedicated > +communication channel for a particular protocol, the 3 properties namely: > +mboxes, mbox-names and shmem shall be present in the sub-node corresponding > +to that protocol. > + > +Clock/Performance bindings for the clocks/OPPs based on SCMI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the common clock binding[1]. > + > +Required properties: > +- #clock-cells : Should be 1. Contains the Clock ID value used by SCMI commands. > + How does the OS identify the fact that a subnode uses the clock binding? Do you need to look for the #clock-cells property, or is this based on the unit address? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > +/** > + * struct scmi_msg_hdr - Message(Tx/Rx) header > + * > + * @id: The identifier of the command being sent > + * @protocol_id: The identifier of the protocol used to send @id command > + * @seq: The token to identify the message. when a message/command returns, > + * the platform returns the whole message header unmodified including > + * the token. > + */ > +struct scmi_msg_hdr { > + u8 id; > + u8 protocol_id; > + u16 seq; > + u32 status; > + bool poll_completion; > +}; Is this structure part of the protocol, or just part of the linux implementation? If this is in the protocol, you should not have a 'bool' member in there, which does not have a well-defined binary representation across architectures. > +/* > + * The SCP firmware providing SCM interface to OSPM and other agents must > + * execute only in little-endian mode as per SCMI specification, so any buffers > + * shared through SCMI should have their contents converted to little-endian > + */ That is a very odd thing to put into a specification, are you sure it requires a specific runtime endian-mode? I would bet that it only requires the protocol to use little-endian data, so better describe it like that. > +struct scmi_shared_mem { > + __le32 reserved; > + __le32 channel_status; > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1) > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0) > + __le32 reserved1[2]; > + __le32 flags; > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0) > + __le32 length; > + __le32 msg_header; > + u8 msg_payload[0]; > +}; > + > +static int scmi_linux_errmap[] = { > + /* better than switch case as long as return value is continuous */ > + 0, /* SCMI_SUCCESS */ > + -EOPNOTSUPP, /* SCMI_ERR_SUPPORT */ > + -EINVAL, /* SCMI_ERR_PARAM */ > + -EACCES, /* SCMI_ERR_ACCESS */ > + -ENOENT, /* SCMI_ERR_ENTRY */ > + -ERANGE, /* SCMI_ERR_RANGE */ > + -EBUSY, /* SCMI_ERR_BUSY */ > + -ECOMM, /* SCMI_ERR_COMMS */ > + -EIO, /* SCMI_ERR_GENERIC */ > + -EREMOTEIO, /* SCMI_ERR_HARDWARE */ > + -EPROTO, /* SCMI_ERR_PROTOCOL */ > +}; maybe make this 'const'. > +static struct platform_driver scmi_driver = { > + .driver = { > + .name = "arm-scmi", > + .of_match_table = of_match_ptr(scmi_of_match), > + }, > + .probe = scmi_probe, > + .remove = scmi_remove, > +}; The 'of_match_ptr' annotation probably causes an 'unused variable' warning, better just drop that. > +#if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL) > +int scmi_handle_put(const struct scmi_handle *handle); > +const struct scmi_handle *scmi_handle_get(struct device *dev); > +const struct scmi_handle *devm_scmi_handle_get(struct device *dev); IS_REACHABLE() can easily lead to confusion when the driver is a loadable module but never gets used by a built-in driver. Maybe use IS_ENABLED() here, and add a Kconfig symbol that other drivers can depend on if you want them to optionally use it, like: config MAYBE_ARM_SCMI_PROTOCOL default y if ARM_SCMI_PROTOCOL=n default ARM_SCMI_PROTOCOL Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > +static const struct scmi_protocol_match scmi_protocols[] = { > + { > + .protocol_id = SCMI_PROTOCOL_PERF, > + .fn = scmi_perf_protocol_init, > + .name = "scmi-cpufreq", > + }, { > + .protocol_id = SCMI_PROTOCOL_CLOCK, > + .fn = scmi_clock_protocol_init, > + .name = "scmi-clocks", > + }, { > + .protocol_id = SCMI_PROTOCOL_POWER, > + .fn = scmi_power_protocol_init, > + .name = "scmi-power-domain", > + }, { > + .protocol_id = SCMI_PROTOCOL_SENSOR, > + .fn = scmi_sensors_protocol_init, > + .name = "scmi-hwmon", > + }, > + {} > +}; This looks backwards from what we do elsewhere in the kernel, and could be considered a layering violation: it requires a generic part of of the driver to know about all the more specific ones, and it means that we have to modify the global table here each time we add another protocol. Can you rewrite this to allow dynamic registration of the protocols? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Thanks for taking a look at this. On 04/10/17 11:50, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> + >> +The SCMI is intended to allow agents such as OSPM to manage various functions >> +that are provided by the hardware platform it is running on, including power >> +and performance functions. >> + >> +This binding is intended to define the interface the firmware implementing >> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >> +and Management Interface Platform Design Document")[0] provide for OSPM in >> +the device tree. >> + >> +Required properties: >> + >> +The scmi node with the following properties shall be under the /firmware/ node. >> + >> +- compatible : shall be "arm,scmi" >> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >> + exactly one or two mailboxes, one for transmitting messages("tx") >> + and another optional for receiving the notifications("rx") if >> + supported. >> +- mbox-names: shall be "tx" or "rx" > > The example below does not have the mbox-names property. If you require > exactly two mailboxes, why do you need the names anyway? > Good question. I can drop it, but would like to keep in case we need to extend it in future. We can always use then to identify. > However, your example does have a #addresss-cells/#size-cells > property that are not documented here. Please add them here as either > optional or required, and describe what the permitted values are and > how the address is interpreted. > Ah right, I didn't notice that. I will add it. It was added to provide the protocol number in "reg" property. >> +- shmem : List of phandle pointing to the shared memory(SHM) area as per >> + generic mailbox client binding. >> + >> +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details >> +about the generic mailbox controller and client driver bindings. >> + >> +The mailbox is the only permitted method of calling the SCMI firmware. >> +Mailbox doorbell is used as a mechanism to alert the presence of a >> +messages and/or notification. > > This looks odd: why not make the message itself part of the mailbox > protocol here, and leave the shmem as a implementation detail of the > mailbox driver? > I am not sure if I follow you here. But generally shmem can be memory carved out of anything in the system and it's dependent on the protocol and the remote firmware rather than the mailbox hardware itself. >> +Each protocol supported shall have a sub-node with corresponding compatible >> +as described in the following sections. If the platform supports dedicated >> +communication channel for a particular protocol, the 3 properties namely: >> +mboxes, mbox-names and shmem shall be present in the sub-node corresponding >> +to that protocol. >> + >> +Clock/Performance bindings for the clocks/OPPs based on SCMI Message Protocol >> +------------------------------------------------------------ >> + >> +This binding uses the common clock binding[1]. >> + >> +Required properties: >> +- #clock-cells : Should be 1. Contains the Clock ID value used by SCMI commands. >> + > > How does the OS identify the fact that a subnode uses the clock binding? > Do you need to look for the #clock-cells property, or is this based on the > unit address? > Yes it depends on #clock-cells property. That's the main reason for adding #clock-cells -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > It would be useful to have options to perform some SCMI transfers > atomically by polling for the completion flag instead of interrupt > driven. The SCMI specification has option to disable the interrupt and > poll for the completion flag in the shared memory. > > This patch adds support for polling based SCMI transfers using that > option. This might be used for uninterrupted/atomic DVFS operations > from the scheduler context. multi-millisecond timeouts from inside the scheduler sound like a really bad idea. Could this maybe get changed to an asynchronous operation? > + if (xfer->hdr.poll_completion) { > + timeout = info->desc->max_rx_timeout_ms * 100; > + while (!scmi_xfer_poll_done(info, xfer) && timeout--) > + udelay(10); The timeout calculation is bad as well, since both the scmi_xfer_poll_done() call and udelay() can take much longer than the 10 microsecond delay that you use for the calculation. If you want to do a timeout check like this, it should generally be done using ktime_get()/ktime_add()/ktime_before(). Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > +static int scmi_mbox_free_channel(struct scmi_info *info) > +{ > + if (!IS_ERR_OR_NULL(info->tx_chan)) { > + mbox_free_channel(info->tx_chan); > + info->tx_chan = NULL; > + } > + > + return 0; > +} Any use of IS_ERR_OR_NULL() tends to be an indication of a bad API design. Please make a decision about what you want to store in info->tx_chan and stick with it. Usually, we don't store error pointers in permanent data structures. If you can deal with tx_chan being absent here, then you can just store NULL into it when you don't have one, otherwise you should make sure you never call this and fail the probe function earlier. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Some of the mailbox controller expects controller specific data in order > to implement simple doorbell mechanism as expected by SCMI specification. > > This patch creates a shim layer to abstract the mailbox interface so > that it can support any mailbox controller. It also provides default > implementation which maps to standard mailbox client APIs, so that > controllers implementing doorbell mechanism need not require any > additional layer. > > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Another level? Now we have three levels of stacked mailboxes, with the highest level being the combined mailbox/memory, then the shim, and below it the hardware mailbox. Can you try to come up with a way to do this with fewer abstractions? Maybe you could assume that the mailbox itself can take variable-length data packets, and then use the shim here for those that require something else? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> +static int scmi_cpufreq_probe(struct platform_device *pdev) > +{ > + int ret; > + > + handle = devm_scmi_handle_get(&pdev->dev); > + > + if (IS_ERR_OR_NULL(handle) || !handle->perf_ops) > + return -EPROBE_DEFER; As mentioned before, never create an interface that needs to use IS_ERR_OR_NULL(), make it return either NULL on error, or always have an error code. > + > +static struct platform_driver scmi_cpufreq_platdrv = { > + .driver = { > + .name = "scmi-cpufreq", > + }, > + .probe = scmi_cpufreq_probe, > + .remove = scmi_cpufreq_remove, > +}; You appear to have split this driver into the 'cpufreq' side and the 'protocol' handler in a different file. I already commented that the way the main scmi driver knows about all the protocols looks bad, here we can see another aspect of the same problem. Rather than manually register a platform_device for the purpose of connecting it to the cpufreq driver, there should be a way to dynamically register the protocol from the cpufreq driver and then have both in the same file. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/17 12:24, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Some of the mailbox controller expects controller specific data in order >> to implement simple doorbell mechanism as expected by SCMI specification. >> >> This patch creates a shim layer to abstract the mailbox interface so >> that it can support any mailbox controller. It also provides default >> implementation which maps to standard mailbox client APIs, so that >> controllers implementing doorbell mechanism need not require any >> additional layer. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > Another level? Now we have three levels of stacked mailboxes, with > the highest level being the combined mailbox/memory, then the shim, > and below it the hardware mailbox. > > Can you try to come up with a way to do this with fewer abstractions? > I completely agree with you. I was against this but Jassi recommended this. I just wanted this SCMI to work with mailbox controllers that support simple doorbell mechanism as specified in the specification but Jassi disagrees with that. > Maybe you could assume that the mailbox itself can take variable-length > data packets, and then use the shim here for those that require > something else? > As per SCMI specification, we pass all the data in shared memory and it just expects to use a simple doorbell feature from hardware mailbox controllers. It's done that way intentionally to avoid dependency on h/w and we for sure will have variety of it and that defeats the purpose of this standard specification. Also, I have added shim only for specific controllers that need them. E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. mbox_if provides default implementation that just calls direct mailbox APIs. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Arnd, > > Thanks for taking a look at this. > > On 04/10/17 11:50, Arnd Bergmann wrote: >> On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> + >>> +The SCMI is intended to allow agents such as OSPM to manage various functions >>> +that are provided by the hardware platform it is running on, including power >>> +and performance functions. >>> + >>> +This binding is intended to define the interface the firmware implementing >>> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >>> +and Management Interface Platform Design Document")[0] provide for OSPM in >>> +the device tree. >>> + >>> +Required properties: >>> + >>> +The scmi node with the following properties shall be under the /firmware/ node. >>> + >>> +- compatible : shall be "arm,scmi" >>> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >>> + exactly one or two mailboxes, one for transmitting messages("tx") >>> + and another optional for receiving the notifications("rx") if >>> + supported. >>> +- mbox-names: shall be "tx" or "rx" >> >> The example below does not have the mbox-names property. If you require >> exactly two mailboxes, why do you need the names anyway? >> > > Good question. I can drop it, but would like to keep in case we need to > extend it in future. We can always use then to identify. I don't think it's necessary, as long you always need to have the first two, but it doesn't hurt either. Just make the description match the example. >> However, your example does have a #addresss-cells/#size-cells >> property that are not documented here. Please add them here as either >> optional or required, and describe what the permitted values are and >> how the address is interpreted. >> > > Ah right, I didn't notice that. I will add it. It was added to provide > the protocol number in "reg" property. ... >> How does the OS identify the fact that a subnode uses the clock binding? >> Do you need to look for the #clock-cells property, or is this based on the >> unit address? >> > > Yes it depends on #clock-cells property. That's the main reason for > adding #clock-cells I'm still unclear on this. Do you mean we look for a subnode with reg=<0x14> and then assume it's a clock node and require the #clock-cells to be there, or do we look through the sub-nodes to find one with the #clock-cells property and then look up the 'reg' property to find out which protocol number to use? >>> +- shmem : List of phandle pointing to the shared memory(SHM) area as per >>> + generic mailbox client binding. >>> + >>> +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details >>> +about the generic mailbox controller and client driver bindings. >>> + >>> +The mailbox is the only permitted method of calling the SCMI firmware. >>> +Mailbox doorbell is used as a mechanism to alert the presence of a >>> +messages and/or notification. >> >> This looks odd: why not make the message itself part of the mailbox >> protocol here, and leave the shmem as a implementation detail of the >> mailbox driver? >> > > I am not sure if I follow you here. But generally shmem can be memory > carved out of anything in the system and it's dependent on the protocol > and the remote firmware rather than the mailbox hardware itself. I think the problem is the way we use the mailbox API in Linux, which is completely abstract at the moment: it could be a pure doorbell, a single-register for a data, some structured memory, or a variable-length message. The assumption today is that the mailbox user and the mailbox driver agree on the interpretation of that void pointer. This breaks down here, as you require the message to be a variable-length message in a fixed physical location, but assume that the mailbox serves only as a doorbell. The solution might be to extend the mailbox API slightly, to have explicit support for variable-length messages, and implement support for that in either mailbox drivers or as an abstraction on top of doorbell-type mailboxes. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/17 13:35, Arnd Bergmann wrote: > On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Hi Arnd, >> >> Thanks for taking a look at this. >> >> On 04/10/17 11:50, Arnd Bergmann wrote: >>> On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> + >>>> +The SCMI is intended to allow agents such as OSPM to manage various functions >>>> +that are provided by the hardware platform it is running on, including power >>>> +and performance functions. >>>> + >>>> +This binding is intended to define the interface the firmware implementing >>>> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >>>> +and Management Interface Platform Design Document")[0] provide for OSPM in >>>> +the device tree. >>>> + >>>> +Required properties: >>>> + >>>> +The scmi node with the following properties shall be under the /firmware/ node. >>>> + >>>> +- compatible : shall be "arm,scmi" >>>> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >>>> + exactly one or two mailboxes, one for transmitting messages("tx") >>>> + and another optional for receiving the notifications("rx") if >>>> + supported. >>>> +- mbox-names: shall be "tx" or "rx" >>> >>> The example below does not have the mbox-names property. If you require >>> exactly two mailboxes, why do you need the names anyway? >>> >> >> Good question. I can drop it, but would like to keep in case we need to >> extend it in future. We can always use then to identify. > > I don't think it's necessary, as long you always need to have the first two, > but it doesn't hurt either. > > Just make the description match the example. > Sure. >>> However, your example does have a #addresss-cells/#size-cells >>> property that are not documented here. Please add them here as either >>> optional or required, and describe what the permitted values are and >>> how the address is interpreted. >>> >> >> Ah right, I didn't notice that. I will add it. It was added to provide >> the protocol number in "reg" property. > ... >>> How does the OS identify the fact that a subnode uses the clock binding? >>> Do you need to look for the #clock-cells property, or is this based on the >>> unit address? >>> >> >> Yes it depends on #clock-cells property. That's the main reason for >> adding #clock-cells > > I'm still unclear on this. Do you mean we look for a subnode with > reg=<0x14> and then assume it's a clock node and require the > #clock-cells to be there, Yes that's how it's used. Presence of subnode with reg=0x14 indicates clock protocol and #clock-cells to indicate that it's clock provider expecting 1 parameter from consumer which is the clock identifier. or do we look through the sub-nodes to > find one with the #clock-cells property and then look up the 'reg' > property to find out which protocol number to use? > Not this way. Do you see any issues ? >>>> +- shmem : List of phandle pointing to the shared memory(SHM) area as per >>>> + generic mailbox client binding. >>>> + >>>> +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details >>>> +about the generic mailbox controller and client driver bindings. >>>> + >>>> +The mailbox is the only permitted method of calling the SCMI firmware. >>>> +Mailbox doorbell is used as a mechanism to alert the presence of a >>>> +messages and/or notification. >>> >>> This looks odd: why not make the message itself part of the mailbox >>> protocol here, and leave the shmem as a implementation detail of the >>> mailbox driver? >>> >> >> I am not sure if I follow you here. But generally shmem can be memory >> carved out of anything in the system and it's dependent on the protocol >> and the remote firmware rather than the mailbox hardware itself. > > I think the problem is the way we use the mailbox API in Linux, which > is completely abstract at the moment: it could be a pure doorbell, a > single-register for a data, some structured memory, or a > variable-length message. The assumption today is that the mailbox > user and the mailbox driver agree on the interpretation of that > void pointer. > Unfortunately true. > This breaks down here, as you require the message to be a > variable-length message in a fixed physical location, but assume that > the mailbox serves only as a doorbell. > Yes. > The solution might be to extend the mailbox API slightly, to > have explicit support for variable-length messages, and implement > support for that in either mailbox drivers or as an abstraction > on top of doorbell-type mailboxes. > I got the concept. But are you also suggesting that in bindings it shmem should be associated with mailbox controller rather than client ? -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 04/10/17 13:35, Arnd Bergmann wrote: >> On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>> Yes it depends on #clock-cells property. That's the main reason for >>> adding #clock-cells >> >> I'm still unclear on this. Do you mean we look for a subnode with >> reg=<0x14> and then assume it's a clock node and require the >> #clock-cells to be there, > > Yes that's how it's used. Presence of subnode with reg=0x14 indicates > clock protocol and #clock-cells to indicate that it's clock provider > expecting 1 parameter from consumer which is the clock identifier. > > or do we look through the sub-nodes to >> find one with the #clock-cells property and then look up the 'reg' >> property to find out which protocol number to use? >> > > Not this way. Do you see any issues ? We normally don't assume that a particular unit address implies a specific function. Conventionally that would be done by matching the "compatible" property instead. What you do clearly works, but it's surprising to the reader. >> I think the problem is the way we use the mailbox API in Linux, which >> is completely abstract at the moment: it could be a pure doorbell, a >> single-register for a data, some structured memory, or a >> variable-length message. The assumption today is that the mailbox >> user and the mailbox driver agree on the interpretation of that >> void pointer. >> > > Unfortunately true. > >> This breaks down here, as you require the message to be a >> variable-length message in a fixed physical location, but assume that >> the mailbox serves only as a doorbell. >> > > Yes. > >> The solution might be to extend the mailbox API slightly, to >> have explicit support for variable-length messages, and implement >> support for that in either mailbox drivers or as an abstraction >> on top of doorbell-type mailboxes. >> > I got the concept. But are you also suggesting that in bindings it shmem > should be associated with mailbox controller rather than client ? There are probably several ways of doing this better, we should see what the best is we can come up with. I think generally speaking we need a way for a mailbox user to know what it should use as the mailbox data here, so it is able to talk to different incompatible mailbox providers. One idea I had is to use a nested mailbox driver, that turns a doorbell or single-register styled mailbox into a variable-length mailbox by adding a memory region, like mailbox@1233000 { compatible = "vendor-hardware-specifc-id"; interrupts = <34>; reg = <0x1233000 0x100>; #mbox-cells = <1>; }; mailbox { compatible = "shmem-mailbox"; mboxes = <&/mailbox@1233000 25>; #mbox-cells = <1>; shmem = <&cpu_scp_lpri &cpu_scp_hpri>; }; This would create one mailbox that only takes a register argument, and another one that can take longer messages based on the first. In your driver, you then refer to the second one and pass the variable-length data into that directly. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/17 15:17, Arnd Bergmann wrote: > On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 04/10/17 13:35, Arnd Bergmann wrote: >>> On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>>> >>>> Yes it depends on #clock-cells property. That's the main reason for >>>> adding #clock-cells >>> >>> I'm still unclear on this. Do you mean we look for a subnode with >>> reg=<0x14> and then assume it's a clock node and require the >>> #clock-cells to be there, >> >> Yes that's how it's used. Presence of subnode with reg=0x14 indicates >> clock protocol and #clock-cells to indicate that it's clock provider >> expecting 1 parameter from consumer which is the clock identifier. >> >> or do we look through the sub-nodes to >>> find one with the #clock-cells property and then look up the 'reg' >>> property to find out which protocol number to use? >>> >> >> Not this way. Do you see any issues ? > > We normally don't assume that a particular unit address implies > a specific function. Conventionally that would be done by matching > the "compatible" property instead. > > What you do clearly works, but it's surprising to the reader. > > >>> I think the problem is the way we use the mailbox API in Linux, which >>> is completely abstract at the moment: it could be a pure doorbell, a >>> single-register for a data, some structured memory, or a >>> variable-length message. The assumption today is that the mailbox >>> user and the mailbox driver agree on the interpretation of that >>> void pointer. >>> >> >> Unfortunately true. >> >>> This breaks down here, as you require the message to be a >>> variable-length message in a fixed physical location, but assume that >>> the mailbox serves only as a doorbell. >>> >> >> Yes. >> >>> The solution might be to extend the mailbox API slightly, to >>> have explicit support for variable-length messages, and implement >>> support for that in either mailbox drivers or as an abstraction >>> on top of doorbell-type mailboxes. >>> >> I got the concept. But are you also suggesting that in bindings it shmem >> should be associated with mailbox controller rather than client ? > > There are probably several ways of doing this better, we should see > what the best is we can come up with. > > I think generally speaking we need a way for a mailbox user to > know what it should use as the mailbox data here, so it is > able to talk to different incompatible mailbox providers. > > One idea I had is to use a nested mailbox driver, that turns > a doorbell or single-register styled mailbox into a variable-length > mailbox by adding a memory region, like > > mailbox@1233000 { > compatible = "vendor-hardware-specifc-id"; > interrupts = <34>; > reg = <0x1233000 0x100>; > #mbox-cells = <1>; > }; > > mailbox { > compatible = "shmem-mailbox"; > mboxes = <&/mailbox@1233000 25>; > #mbox-cells = <1>; > shmem = <&cpu_scp_lpri &cpu_scp_hpri>; > }; > > This would create one mailbox that only takes a register argument, > and another one that can take longer messages based on the first. > In your driver, you then refer to the second one and pass the > variable-length data into that directly. 1. IIUC it was intentional not to include shmem as part of mailbox controller binding and was pushed to client drivers as it's generally not part of mailbox IP block. I am not sure if there are any other specific reasons for that, but I may be missing some facts. 2. I am not sure if we need nested driver/bindings (at-least to begin with). On a platform I don't think both/all modes will be used. I had proposal for adding doorbell for ARM MHU based on extended bindings, but it was rejected[1]. But I really preferred that over the shim layer I had to add in v3. -- Regards, Sudeep [1] https://patchwork.kernel.org/patch/9745683/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/17 11:59, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> +/** >> + * struct scmi_msg_hdr - Message(Tx/Rx) header >> + * >> + * @id: The identifier of the command being sent >> + * @protocol_id: The identifier of the protocol used to send @id command >> + * @seq: The token to identify the message. when a message/command returns, >> + * the platform returns the whole message header unmodified including >> + * the token. >> + */ >> +struct scmi_msg_hdr { >> + u8 id; >> + u8 protocol_id; >> + u16 seq; >> + u32 status; >> + bool poll_completion; >> +}; > > Is this structure part of the protocol, or just part of the linux > implementation? > If this is in the protocol, you should not have a 'bool' member in there, which > does not have a well-defined binary representation across architectures. > No, it's not part of specification just linux, added to support polling based DVFS messages. >> +/* >> + * The SCP firmware providing SCM interface to OSPM and other agents must >> + * execute only in little-endian mode as per SCMI specification, so any buffers >> + * shared through SCMI should have their contents converted to little-endian >> + */ > > That is a very odd thing to put into a specification, are you sure it requires > a specific runtime endian-mode? I would bet that it only requires the protocol > to use little-endian data, so better describe it like that. > Right, my bad. Not sure where I copied that text from, but as you mention specification just expects data to be in LE format. [..] > >> +#if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL) >> +int scmi_handle_put(const struct scmi_handle *handle); >> +const struct scmi_handle *scmi_handle_get(struct device *dev); >> +const struct scmi_handle *devm_scmi_handle_get(struct device *dev); > > IS_REACHABLE() can easily lead to confusion when the driver is > a loadable module but never gets used by a built-in driver. Maybe use > IS_ENABLED() here, and add a Kconfig symbol that other drivers > can depend on if you want them to optionally use it, like: > > config MAYBE_ARM_SCMI_PROTOCOL > default y if ARM_SCMI_PROTOCOL=n > default ARM_SCMI_PROTOCOL OK, will check, we may not need that yet. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 5:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> + >>> +static struct platform_driver scmi_cpufreq_platdrv = { >>> + .driver = { >>> + .name = "scmi-cpufreq", >>> + }, >>> + .probe = scmi_cpufreq_probe, >>> + .remove = scmi_cpufreq_remove, >>> +}; >> >> You appear to have split this driver into the 'cpufreq' side and >> the 'protocol' handler in a different file. I already commented that >> the way the main scmi driver knows about all the protocols looks >> bad, here we can see another aspect of the same problem. >> >> Rather than manually register a platform_device for the purpose >> of connecting it to the cpufreq driver, there should be a way >> to dynamically register the protocol from the cpufreq driver >> and then have both in the same file. >> > > I agree that should be possible. I took this approach for 2 reasons: > > 1. to avoid all sorts of probe ordering issues > 2. we may have system with multiple instances of SCMI. E.g. a system > may have multiple remote processors, each controlling dvfs/power mgmt > of a subset of CPUs/devices controller in OS. > > I have to admit that I haven't thought too much in details yet. That's > the main idea behind scmi_handle and restricting access to that only to > sub-nodes in it. I am open to suggestions. How about introducing a separate bus_type for protocols? The platform_device you use here isn't really the best abstraction, and with a new bus_type, you can handle multiple instances of scmi as well as decoupling them from the protocol drivers. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/17 12:20, Arnd Bergmann wrote: > On Wed, Oct 4, 2017 at 5:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> + >>>> +static struct platform_driver scmi_cpufreq_platdrv = { >>>> + .driver = { >>>> + .name = "scmi-cpufreq", >>>> + }, >>>> + .probe = scmi_cpufreq_probe, >>>> + .remove = scmi_cpufreq_remove, >>>> +}; >>> >>> You appear to have split this driver into the 'cpufreq' side and >>> the 'protocol' handler in a different file. I already commented that >>> the way the main scmi driver knows about all the protocols looks >>> bad, here we can see another aspect of the same problem. >>> >>> Rather than manually register a platform_device for the purpose >>> of connecting it to the cpufreq driver, there should be a way >>> to dynamically register the protocol from the cpufreq driver >>> and then have both in the same file. >>> >> >> I agree that should be possible. I took this approach for 2 reasons: >> >> 1. to avoid all sorts of probe ordering issues >> 2. we may have system with multiple instances of SCMI. E.g. a system >> may have multiple remote processors, each controlling dvfs/power mgmt >> of a subset of CPUs/devices controller in OS. >> >> I have to admit that I haven't thought too much in details yet. That's >> the main idea behind scmi_handle and restricting access to that only to >> sub-nodes in it. I am open to suggestions. > > How about introducing a separate bus_type for protocols? > The platform_device you use here isn't really the best abstraction, > and with a new bus_type, you can handle multiple instances of > scmi as well as decoupling them from the protocol drivers. > Yes based on some discussion on this thread yesterday, I started exploring and seem to have come to same conclusions. I will try to hack and see how that evolves. Thanks for the suggestion. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 4:47 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 04/10/17 15:17, Arnd Bergmann wrote: >> On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> On 04/10/17 13:35, Arnd Bergmann wrote: >>>> On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> There are probably several ways of doing this better, we should see >> what the best is we can come up with. >> >> I think generally speaking we need a way for a mailbox user to >> know what it should use as the mailbox data here, so it is >> able to talk to different incompatible mailbox providers. >> >> One idea I had is to use a nested mailbox driver, that turns >> a doorbell or single-register styled mailbox into a variable-length >> mailbox by adding a memory region, like >> >> mailbox@1233000 { >> compatible = "vendor-hardware-specifc-id"; >> interrupts = <34>; >> reg = <0x1233000 0x100>; >> #mbox-cells = <1>; >> }; >> >> mailbox { >> compatible = "shmem-mailbox"; >> mboxes = <&/mailbox@1233000 25>; >> #mbox-cells = <1>; >> shmem = <&cpu_scp_lpri &cpu_scp_hpri>; >> }; >> >> This would create one mailbox that only takes a register argument, >> and another one that can take longer messages based on the first. >> In your driver, you then refer to the second one and pass the >> variable-length data into that directly. > > 1. IIUC it was intentional not to include shmem as part of mailbox > controller binding and was pushed to client drivers as it's generally > not part of mailbox IP block. I am not sure if there are any other > specific reasons for that, but I may be missing some facts. Ok, I see. > 2. I am not sure if we need nested driver/bindings (at-least to begin > with). On a platform I don't think both/all modes will be used. > I had proposal for adding doorbell for ARM MHU based on extended > bindings, but it was rejected[1]. But I really preferred that over > the shim layer I had to add in v3. Maybe we can come up with a more generic way to do doorbells on top of mailboxes instead? This sounds like a problem that would come back with other drivers, so the MHU-specific shim will not be a permanent solution either. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/17 12:56, Arnd Bergmann wrote: > On Wed, Oct 4, 2017 at 4:47 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 04/10/17 15:17, Arnd Bergmann wrote: >>> On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> On 04/10/17 13:35, Arnd Bergmann wrote: >>>>> On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>> There are probably several ways of doing this better, we should see >>> what the best is we can come up with. >>> >>> I think generally speaking we need a way for a mailbox user to >>> know what it should use as the mailbox data here, so it is >>> able to talk to different incompatible mailbox providers. >>> >>> One idea I had is to use a nested mailbox driver, that turns >>> a doorbell or single-register styled mailbox into a variable-length >>> mailbox by adding a memory region, like >>> >>> mailbox@1233000 { >>> compatible = "vendor-hardware-specifc-id"; >>> interrupts = <34>; >>> reg = <0x1233000 0x100>; >>> #mbox-cells = <1>; >>> }; >>> >>> mailbox { >>> compatible = "shmem-mailbox"; >>> mboxes = <&/mailbox@1233000 25>; >>> #mbox-cells = <1>; >>> shmem = <&cpu_scp_lpri &cpu_scp_hpri>; >>> }; >>> >>> This would create one mailbox that only takes a register argument, >>> and another one that can take longer messages based on the first. >>> In your driver, you then refer to the second one and pass the >>> variable-length data into that directly. >> >> 1. IIUC it was intentional not to include shmem as part of mailbox >> controller binding and was pushed to client drivers as it's generally >> not part of mailbox IP block. I am not sure if there are any other >> specific reasons for that, but I may be missing some facts. > > Ok, I see. > >> 2. I am not sure if we need nested driver/bindings (at-least to begin >> with). On a platform I don't think both/all modes will be used. >> I had proposal for adding doorbell for ARM MHU based on extended >> bindings, but it was rejected[1]. But I really preferred that over >> the shim layer I had to add in v3. > > Maybe we can come up with a more generic way to do doorbells > on top of mailboxes instead? This sounds like a problem that > would come back with other drivers, so the MHU-specific shim > will not be a permanent solution either. > I completely agree. I have seen few drivers that just implement doorbells in their controller. I will check them in details again. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: > This patch adds ARM MHU specific mailbox client bindings to support > SCMI. Since SCMI specification just requires doorbell mechanism from > mailbox controllers, we add mailbox data to specify the doorbell bit(s). > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > .../devicetree/bindings/arm/arm,mhu-scmi.txt | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt > > diff --git a/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt b/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt > new file mode 100644 > index 000000000000..8c106f1cdeb8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt > @@ -0,0 +1,19 @@ > +ARM MHU mailbox client bindings for SCMI Message Protocol > +---------------------------------------------------------- > + > +This binding is intended to define the ARM MHU specific extensions to > +the generic SCMI bindings[2]. > + > +Required properties: > + > +The scmi node with the following properties shall be under the /firmware/ node. > + > +- compatible : shall be "arm,scmi" and "arm,mhu-scmi" Most specific first. > +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit > + data as expected by the mailbox controller Shouldn't that be cells as part of mboxes property? > + > +See [1] for details on all other required/optional properties of the generic > +mailbox controller and [2] for generic SCMI bindings. > + > +[1] Documentation/devicetree/bindings/mailbox/mailbox.txt > +[2] Documentation/devicetree/bindings/arm/arm,scmi.txt > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: > On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >> This patch adds ARM MHU specific mailbox client bindings to support >> SCMI. Since SCMI specification just requires doorbell mechanism from >> mailbox controllers, we add mailbox data to specify the doorbell bit(s). >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> .../devicetree/bindings/arm/arm,mhu-scmi.txt | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt b/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt >> new file mode 100644 >> index 000000000000..8c106f1cdeb8 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt >> @@ -0,0 +1,19 @@ >> +ARM MHU mailbox client bindings for SCMI Message Protocol >> +---------------------------------------------------------- >> + >> +This binding is intended to define the ARM MHU specific extensions to >> +the generic SCMI bindings[2]. >> + >> +Required properties: >> + >> +The scmi node with the following properties shall be under the /firmware/ node. >> + >> +- compatible : shall be "arm,scmi" and "arm,mhu-scmi" > > Most specific first. > >> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >> + data as expected by the mailbox controller > > Shouldn't that be cells as part of mboxes property? > A MHU client can send any number of commands (such u32 values) over a channel. This client (SCMI) sends just one command over a channel, but other clients may/do send two or more. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Also, I have added shim only for specific controllers that need them. > E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. > mbox_if provides default implementation that just calls direct mailbox > APIs. > Yeah you could hack away the MHU driver to make your life easy at the cost of duplicated code and extra DT bindings, but for a moment think what if your development platform wasn't MHU but, say, Rockchip mailbox controller? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/17 12:34, Jassi Brar wrote: > On Wed, Oct 4, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> Also, I have added shim only for specific controllers that need them. >> E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. >> mbox_if provides default implementation that just calls direct mailbox >> APIs. >> > Yeah you could hack away the MHU driver to make your life easy at the > cost of duplicated code and extra DT bindings, but for a moment think > what if your development platform wasn't MHU but, say, Rockchip > mailbox controller? > As mentioned before I understand your concern. But the point is this needs to be replicated with each protocol on that controller. So as Arnd pointed out we can reduce that by generalizing common things like doorbell. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 6:57 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 06/10/17 12:34, Jassi Brar wrote: >> On Wed, Oct 4, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >>> Also, I have added shim only for specific controllers that need them. >>> E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. >>> mbox_if provides default implementation that just calls direct mailbox >>> APIs. >>> >> Yeah you could hack away the MHU driver to make your life easy at the >> cost of duplicated code and extra DT bindings, but for a moment think >> what if your development platform wasn't MHU but, say, Rockchip >> mailbox controller? >> > > As mentioned before I understand your concern. But the point is this > needs to be replicated with each protocol on that controller. > Only generic protocols need to have a platform specific transport layer. There's no escaping that. > So as Arnd > pointed out we can reduce that by generalizing common things like doorbell. > Rockchip, and most other controllers, has no "doorbell". And yet each is perfectly capable of supporting SCMI. Looking forward to your "generalised doorbell". -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/10/17 14:34, Jassi Brar wrote: > On Fri, Oct 6, 2017 at 6:57 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 06/10/17 12:34, Jassi Brar wrote: >>> On Wed, Oct 4, 2017 at 5:02 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>>> Also, I have added shim only for specific controllers that need them. >>>> E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. >>>> mbox_if provides default implementation that just calls direct mailbox >>>> APIs. >>>> >>> Yeah you could hack away the MHU driver to make your life easy at the >>> cost of duplicated code and extra DT bindings, but for a moment think >>> what if your development platform wasn't MHU but, say, Rockchip >>> mailbox controller? >>> >> >> As mentioned before I understand your concern. But the point is this >> needs to be replicated with each protocol on that controller. >> > Only generic protocols need to have a platform specific transport > layer. There's no escaping that. > >> So as Arnd >> pointed out we can reduce that by generalizing common things like doorbell. >> > Rockchip, and most other controllers, has no "doorbell". And yet each > is perfectly capable of supporting SCMI. Not sure of that, may be Linux drivers can be made to support but the firmware needs to conform the SCMI specification. And when it does, all we need is just notion of doorbell. > Looking forward to your "generalised doorbell". > It won't be completely generic alone. The controllers need to have some logic. It's just that they don't use any data from client, they just signal the remote. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 6, 2017 at 9:24 PM, Rob Herring <robh@kernel.org> wrote: > On Fri, Oct 6, 2017 at 6:01 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: >>> On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >>> >>>> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >>>> + data as expected by the mailbox controller >>> >>> Shouldn't that be cells as part of mboxes property? >>> >> A MHU client can send any number of commands (such u32 values) over a channel. >> This client (SCMI) sends just one command over a channel, but other >> clients may/do send two or more. > > Okay, then I guess I don't understand why this is in DT. > Yeah the client has to provide code (u32 value) for the commands it sends, and that value is going to be platform specific. For example, on Juno the ITS_AN_SCMI_COMMAND may be defined as BIT(7) while on my platform it may be 0x4567 For MHU based platforms, it becomes easy if the u32 is passed from DT. And that should be ok since that is like a h/w parameter - a value chosen/expected by the remote firmware. thnx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/17 14:52, Rob Herring wrote: > On Fri, Oct 6, 2017 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Fri, Oct 6, 2017 at 9:24 PM, Rob Herring <robh@kernel.org> wrote: >>> On Fri, Oct 6, 2017 at 6:01 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>>> On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: >>>>> On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >> >>>>> >>>>>> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >>>>>> + data as expected by the mailbox controller >>>>> >>>>> Shouldn't that be cells as part of mboxes property? >>>>> >>>> A MHU client can send any number of commands (such u32 values) over a channel. >>>> This client (SCMI) sends just one command over a channel, but other >>>> clients may/do send two or more. > > The above definition doesn't support 2 or more as it is 1-1 with channels. > In case of MHU, we just need one u32 per channel in SCMI context. >>> Okay, then I guess I don't understand why this is in DT. >>> >> Yeah the client has to provide code (u32 value) for the commands it >> sends, and that value is going to be platform specific. For example, >> on Juno the ITS_AN_SCMI_COMMAND may be defined as BIT(7) while on my >> platform it may be 0x4567 >> >> For MHU based platforms, it becomes easy if the u32 is passed from DT. >> And that should be ok since that is like a h/w parameter - a value >> chosen/expected by the remote firmware. > > Could it ever be more than 1 cell? > No, as mentioned above. > I guess being in DT is fine, but I'm still not sure about the naming. > The current name suggests it is part of the mbox binding. Do we want > that or should it be SCMI specific? Then "data" is vague. Perhaps > "scmi-commands"? > How about "scmi-mhu-commands" ? As scmi-commands sounds too generic and part of SCMI specification. But they are rather MHU specific to make use of each 32 bit in physical channel for a virtual channel. IOW, it's just different way of representing the doorbell bits I proposed previously as a 32-bit command expected by the driver. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 9, 2017 at 7:22 PM, Rob Herring <robh@kernel.org> wrote: > On Fri, Oct 6, 2017 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Fri, Oct 6, 2017 at 9:24 PM, Rob Herring <robh@kernel.org> wrote: >>> On Fri, Oct 6, 2017 at 6:01 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>>> On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: >>>>> On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >> >>>>> >>>>>> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >>>>>> + data as expected by the mailbox controller >>>>> >>>>> Shouldn't that be cells as part of mboxes property? >>>>> >>>> A MHU client can send any number of commands (such u32 values) over a channel. >>>> This client (SCMI) sends just one command over a channel, but other >>>> clients may/do send two or more. > > The above definition doesn't support 2 or more as it is 1-1 with channels. > I thought you suggested to make controller driver accept the command as another cell in client's mboxes property. Which we can't do. >>> Okay, then I guess I don't understand why this is in DT. >>> >> Yeah the client has to provide code (u32 value) for the commands it >> sends, and that value is going to be platform specific. For example, >> on Juno the ITS_AN_SCMI_COMMAND may be defined as BIT(7) while on my >> platform it may be 0x4567 >> >> For MHU based platforms, it becomes easy if the u32 is passed from DT. >> And that should be ok since that is like a h/w parameter - a value >> chosen/expected by the remote firmware. > > Could it ever be more than 1 cell? > SCMI sends sub-commands via SHMEM, so it is always going to be 1cell for _scmi_. However many firmwares are unlikely to use just one command over a channel - say, the protocol is trivial or the linux and remote have no SHMEM. > I guess being in DT is fine, but I'm still not sure about the naming. > The current name suggests it is part of the mbox binding. Do we want > that or should it be SCMI specific? Then "data" is vague. Perhaps > "scmi-commands"? > Sure. I have no problem with whatever we wanna call it. thnx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 9, 2017 at 9:46 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Mon, Oct 9, 2017 at 7:22 PM, Rob Herring <robh@kernel.org> wrote: >> On Fri, Oct 6, 2017 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>> On Fri, Oct 6, 2017 at 9:24 PM, Rob Herring <robh@kernel.org> wrote: >>>> On Fri, Oct 6, 2017 at 6:01 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>>>> On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: >>>>>> On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >>> >>>>>> >>>>>>> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >>>>>>> + data as expected by the mailbox controller >>>>>> >>>>>> Shouldn't that be cells as part of mboxes property? >>>>>> >>>>> A MHU client can send any number of commands (such u32 values) over a channel. >>>>> This client (SCMI) sends just one command over a channel, but other >>>>> clients may/do send two or more. >> >> The above definition doesn't support 2 or more as it is 1-1 with channels. >> > I thought you suggested to make controller driver accept the command > as another cell in client's mboxes property. > Which we can't do. Yes, agreed. But I'm wondering since a client may need more than one, how would that be expressed? >>>> Okay, then I guess I don't understand why this is in DT. >>>> >>> Yeah the client has to provide code (u32 value) for the commands it >>> sends, and that value is going to be platform specific. For example, >>> on Juno the ITS_AN_SCMI_COMMAND may be defined as BIT(7) while on my >>> platform it may be 0x4567 >>> >>> For MHU based platforms, it becomes easy if the u32 is passed from DT. >>> And that should be ok since that is like a h/w parameter - a value >>> chosen/expected by the remote firmware. >> >> Could it ever be more than 1 cell? >> > SCMI sends sub-commands via SHMEM, so it is always going to be 1cell for _scmi_. > However many firmwares are unlikely to use just one command over a > channel - say, the protocol is trivial or the linux and remote have no > SHMEM. I'd hope the normal case is not enumerating commands and sub-commands in DT and this is special case of a "generic" protocol with platform specific aspects. It could be solved with a specific compatible for each platform/implementation. We'll probably regret not doing that, but I'm going to pretend that this time SoC vendors won't mess it up. >> I guess being in DT is fine, but I'm still not sure about the naming. >> The current name suggests it is part of the mbox binding. Do we want >> that or should it be SCMI specific? Then "data" is vague. Perhaps >> "scmi-commands"? >> > Sure. I have no problem with whatever we wanna call it. Okay. That should have an "arm" prefix too BTW. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 10, 2017 at 4:27 AM, Rob Herring <robh@kernel.org> wrote: > On Mon, Oct 9, 2017 at 9:46 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Mon, Oct 9, 2017 at 7:22 PM, Rob Herring <robh@kernel.org> wrote: >>> On Fri, Oct 6, 2017 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>>> On Fri, Oct 6, 2017 at 9:24 PM, Rob Herring <robh@kernel.org> wrote: >>>>> On Fri, Oct 6, 2017 at 6:01 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>>>>> On Fri, Oct 6, 2017 at 4:50 AM, Rob Herring <robh@kernel.org> wrote: >>>>>>> On Thu, Sep 28, 2017 at 02:11:27PM +0100, Sudeep Holla wrote: >>>> >>>>>>> >>>>>>>> +- mbox-data : For each phandle listed in mboxes property, an unsigned 32-bit >>>>>>>> + data as expected by the mailbox controller >>>>>>> >>>>>>> Shouldn't that be cells as part of mboxes property? >>>>>>> >>>>>> A MHU client can send any number of commands (such u32 values) over a channel. >>>>>> This client (SCMI) sends just one command over a channel, but other >>>>>> clients may/do send two or more. >>> >>> The above definition doesn't support 2 or more as it is 1-1 with channels. >>> >> I thought you suggested to make controller driver accept the command >> as another cell in client's mboxes property. >> Which we can't do. > > Yes, agreed. But I'm wondering since a client may need more than one, > how would that be expressed? > Most (except SCMI) protocols are proprietary and can not be used generically, so the command codes get hardcoded in the client driver. SCMI+MHU is going to be rare case when we have a chance to get the code via DT. >>>>> Okay, then I guess I don't understand why this is in DT. >>>>> >>>> Yeah the client has to provide code (u32 value) for the commands it >>>> sends, and that value is going to be platform specific. For example, >>>> on Juno the ITS_AN_SCMI_COMMAND may be defined as BIT(7) while on my >>>> platform it may be 0x4567 >>>> >>>> For MHU based platforms, it becomes easy if the u32 is passed from DT. >>>> And that should be ok since that is like a h/w parameter - a value >>>> chosen/expected by the remote firmware. >>> >>> Could it ever be more than 1 cell? >>> >> SCMI sends sub-commands via SHMEM, so it is always going to be 1cell for _scmi_. >> However many firmwares are unlikely to use just one command over a >> channel - say, the protocol is trivial or the linux and remote have no >> SHMEM. > > I'd hope the normal case is not enumerating commands and sub-commands > in DT and this is special case of a "generic" protocol with platform > specific aspects. > Yes. It is only for SCMI protocol running over the variations of MHU controller. Cheers -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 4:32 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 04/10/17 12:24, Arnd Bergmann wrote: >> On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> Some of the mailbox controller expects controller specific data in order >>> to implement simple doorbell mechanism as expected by SCMI specification. >>> >>> This patch creates a shim layer to abstract the mailbox interface so >>> that it can support any mailbox controller. It also provides default >>> implementation which maps to standard mailbox client APIs, so that >>> controllers implementing doorbell mechanism need not require any >>> additional layer. >>> >>> Cc: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> >> Another level? Now we have three levels of stacked mailboxes, with >> the highest level being the combined mailbox/memory, then the shim, >> and below it the hardware mailbox. >> >> Can you try to come up with a way to do this with fewer abstractions? >> > > I completely agree with you. I was against this but Jassi recommended > this. I just wanted this SCMI to work with mailbox controllers that > support simple doorbell mechanism as specified in the specification but > Jassi disagrees with that. > >> Maybe you could assume that the mailbox itself can take variable-length >> data packets, and then use the shim here for those that require >> something else? >> > > As per SCMI specification, we pass all the data in shared memory and it > just expects to use a simple doorbell feature from hardware mailbox > controllers. It's done that way intentionally to avoid dependency on h/w > and we for sure will have variety of it and that defeats the purpose > of this standard specification. > > Also, I have added shim only for specific controllers that need them. > E.g. ARM MHU as Jassi disagreed to add doorbell mechanism to that. > mbox_if provides default implementation that just calls direct mailbox > APIs. > drivers/mailbox is a framework for interfacing/abstracting hardware mailboxes. If you're starting to layer mailboxes ontop of each-other chances are very high that you're confusing it with the computer science term "mailbox". Abstracting a doorbell-like piece of hardware behind the mbox framework makes a lot of sense, but the interface between your clients and the code that fills out shared memory and then invokes said doorbell is a higher level of "mailbox" and is probably better implemented using a direct function call. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/28, Sudeep Holla wrote: > On some ARM based systems, a separate Cortex-M based System Control > Processor(SCP) provides the overall power, clock, reset and system > control. System Control and Management Interface(SCMI) Message Protocol > is defined for the communication between the Application Cores(AP) > and the SCP. > > This patch adds support for the clocks provided by SCP using SCMI > protocol. > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: linux-clk@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- Acked-by: Stephen Boyd <sboyd@codeaurora.org> > MAINTAINERS | 2 +- > drivers/clk/Kconfig | 10 +++ > drivers/clk/Makefile | 1 + > drivers/clk/clk-scmi.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 222 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/clk-scmi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 23ec3471f542..32c184391aee 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12941,7 +12941,7 @@ M: Sudeep Holla <sudeep.holla@arm.com> > L: linux-arm-kernel@lists.infradead.org > S: Maintained > F: Documentation/devicetree/bindings/arm/arm,sc[mp]i.txt > -F: drivers/clk/clk-scpi.c > +F: drivers/clk/clk-sc[mp]i.c Is there a lot of copy/paste going on from clk-scpi.c? Maybe it could be consolidated? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/11/17 07:23, Stephen Boyd wrote: > On 09/28, Sudeep Holla wrote: >> On some ARM based systems, a separate Cortex-M based System Control >> Processor(SCP) provides the overall power, clock, reset and system >> control. System Control and Management Interface(SCMI) Message Protocol >> is defined for the communication between the Application Cores(AP) >> and the SCP. >> >> This patch adds support for the clocks provided by SCP using SCMI >> protocol. >> >> Cc: Michael Turquette <mturquette@baylibre.com> >> Cc: Stephen Boyd <sboyd@codeaurora.org> >> Cc: linux-clk@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- > > Acked-by: Stephen Boyd <sboyd@codeaurora.org> > Thanks >> MAINTAINERS | 2 +- >> drivers/clk/Kconfig | 10 +++ >> drivers/clk/Makefile | 1 + >> drivers/clk/clk-scmi.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 222 insertions(+), 1 deletion(-) >> create mode 100644 drivers/clk/clk-scmi.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 23ec3471f542..32c184391aee 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -12941,7 +12941,7 @@ M: Sudeep Holla <sudeep.holla@arm.com> >> L: linux-arm-kernel@lists.infradead.org >> S: Maintained >> F: Documentation/devicetree/bindings/arm/arm,sc[mp]i.txt >> -F: drivers/clk/clk-scpi.c >> +F: drivers/clk/clk-sc[mp]i.c > > Is there a lot of copy/paste going on from clk-scpi.c? Maybe it > could be consolidated? > Not much apart from the usual driver skeleton. Also SCPI specification will not be enhanced any further while SCMI will be. So they will deviated in the feature set going further. Even I was thinking of merging then together initially but based on some WIP changes to the specification, I thought it may not be good idea. But if we think it can be merged in future , I will do that for sure(for easy maintenance) -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/02, Sudeep Holla wrote: > > > On 02/11/17 07:23, Stephen Boyd wrote: > > Is there a lot of copy/paste going on from clk-scpi.c? Maybe it > > could be consolidated? > > > > Not much apart from the usual driver skeleton. Also SCPI specification > will not be enhanced any further while SCMI will be. So they will > deviated in the feature set going further. Even I was thinking of > merging then together initially but based on some WIP changes to the > specification, I thought it may not be good idea. But if we think it can > be merged in future , I will do that for sure(for easy maintenance) > Ok, no worries from me. Thanks for taking another look. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, Let me begin admitting that we are introducing yet another protocol to achieve same things as many existing protocols like ARM SCPI, TI SCI, QCOM RPM, Nvidia Tegra BPMP, and so on. All I can say is that this new ARM System Control and Management Interface(SCMI) is more flexible and easily extensible than any of the existing ones. Many vendors were involved in the making of this formal specification and is now officially published[1]. There is a strong trend in the industry to provide micro-controllers in systems to abstract various power, or other system management tasks. These controllers usually have similar interfaces, both in terms of the functions that are provided by them, and in terms of how requests are communicated to them. This specification is to standardise and avoid (any further) fragmentation in the design of such interface by various vendors. This patch set is intended to get feedback on the design and structure of the code. This is not complete and not fully tested due to non-availability of firmware with full feature set at this time. It currently doesn't support notification, asynchronous/delayed response, perf/power statistics region and sensor register region to name a few. I have borrowed some of the ideas of message allocation/management from TI SCI. Changes: v2[4]->v3: - Addressed various comments recieved so far(clock, hwmon and cpufreq drivers along with scmi drivers) - Hwmon driver now uses core layer to create and manage sysfs attributes - Added a shim layer to abstract the mailbox interface to support any custom adaptation required by the controller driver - Simple ARM MHU shim layer using newly added abstraction v1[3]->v2[4]: - Additional support for polling based DVFS and per protocol channels - Dependent drivers(clock, hwmon, cpufreq and power domains) - Various other review comments and issued found during testing addressed - Explicit binding for method dropped as even SMC based method are adviertised as mailbox RFC[2]->v1[3]: - Add generic mailbox binding for shared memory(Rob H) - Dropped compatibles per protocol(Suggested by Matt S) - Dropped lot of unnecessary pointer casting(Arnd B) - Dropped packing of structures(Arnd B) - Few other changes/additions based initial testing with firmware providing SCMI interface to OSPM -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html [2] https://marc.info/?l=linux-kernel&m=149685193627620&w=2 [3] https://marc.info/?l=linux-arm-kernel&m=149849482623492&w=2 [4] https://marc.info/?l=devicetree&m=150185763105926&w=2 Sudeep Holla (22): dt-bindings: mailbox: add support for mailbox client shared memory dt-bindings: arm: add support for ARM System Control and Management Interface(SCMI) protocol dt-bindings: arm: scmi: add ARM MHU specific mailbox client bindings firmware: arm_scmi: add basic driver infrastructure for SCMI firmware: arm_scmi: add common infrastructure and support for base protocol firmware: arm_scmi: add initial support for performance protocol firmware: arm_scmi: add initial support for clock protocol firmware: arm_scmi: add initial support for power protocol firmware: arm_scmi: add initial support for sensor protocol firmware: arm_scmi: probe and initialise all the supported protocols firmware: arm_scmi: add support for polling based SCMI transfers firmware: arm_scmi: add option for polling based performance domain operations firmware: arm_scmi: refactor in preparation to support per-protocol channels firmware: arm_scmi: add per-protocol channels support using idr objects firmware: arm_scmi: abstract mailbox interface firmware: arm_scmi: add arm_mhu specific mailbox interface firmware: arm_scmi: add device power domain support using genpd clk: add support for clocks provided by SCMI hwmon: (core) Add hwmon_max to hwmon_sensor_types enumeration hwmon: add support for sensors exported via ARM SCMI cpufreq: add support for CPU DVFS based on SCMI message protocol cpufreq: scmi: add support for fast frequency switching .../devicetree/bindings/arm/arm,mhu-scmi.txt | 19 + Documentation/devicetree/bindings/arm/arm,scmi.txt | 171 ++++ .../devicetree/bindings/mailbox/mailbox.txt | 28 + MAINTAINERS | 11 +- drivers/clk/Kconfig | 10 + drivers/clk/Makefile | 1 + drivers/clk/clk-scmi.c | 210 +++++ drivers/cpufreq/Kconfig.arm | 11 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/scmi-cpufreq.c | 287 +++++++ drivers/firmware/Kconfig | 34 + drivers/firmware/Makefile | 1 + drivers/firmware/arm_scmi/Makefile | 4 + drivers/firmware/arm_scmi/arm_mhu_if.c | 106 +++ drivers/firmware/arm_scmi/base.c | 293 +++++++ drivers/firmware/arm_scmi/clock.c | 339 ++++++++ drivers/firmware/arm_scmi/common.h | 127 +++ drivers/firmware/arm_scmi/driver.c | 956 +++++++++++++++++++++ drivers/firmware/arm_scmi/mbox_if.c | 80 ++ drivers/firmware/arm_scmi/mbox_if.h | 71 ++ drivers/firmware/arm_scmi/perf.c | 514 +++++++++++ drivers/firmware/arm_scmi/power.c | 242 ++++++ drivers/firmware/arm_scmi/scmi_pm_domain.c | 134 +++ drivers/firmware/arm_scmi/sensors.c | 287 +++++++ drivers/hwmon/Kconfig | 12 + drivers/hwmon/Makefile | 1 + drivers/hwmon/scmi-hwmon.c | 235 +++++ include/linux/hwmon.h | 1 + include/linux/scmi_protocol.h | 216 +++++ 29 files changed, 4397 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/arm,mhu-scmi.txt create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt create mode 100644 drivers/clk/clk-scmi.c create mode 100644 drivers/cpufreq/scmi-cpufreq.c create mode 100644 drivers/firmware/arm_scmi/Makefile create mode 100644 drivers/firmware/arm_scmi/arm_mhu_if.c create mode 100644 drivers/firmware/arm_scmi/base.c create mode 100644 drivers/firmware/arm_scmi/clock.c create mode 100644 drivers/firmware/arm_scmi/common.h create mode 100644 drivers/firmware/arm_scmi/driver.c create mode 100644 drivers/firmware/arm_scmi/mbox_if.c create mode 100644 drivers/firmware/arm_scmi/mbox_if.h create mode 100644 drivers/firmware/arm_scmi/perf.c create mode 100644 drivers/firmware/arm_scmi/power.c create mode 100644 drivers/firmware/arm_scmi/scmi_pm_domain.c create mode 100644 drivers/firmware/arm_scmi/sensors.c create mode 100644 drivers/hwmon/scmi-hwmon.c create mode 100644 include/linux/scmi_protocol.h -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html