[v3,00/22] firmware: ARM System Control and Management Interface(SCMI) support

Message ID 1506604306-20739-1-git-send-email-sudeep.holla@arm.com
Headers show
Series
  • firmware: ARM System Control and Management Interface(SCMI) support
Related show

Message

Sudeep Holla Sept. 28, 2017, 1:11 p.m.
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

Comments

Sudeep Holla Sept. 29, 2017, 1:40 p.m. | #1
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
Arnd Bergmann Oct. 4, 2017, 10:50 a.m. | #2
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
Arnd Bergmann Oct. 4, 2017, 10:59 a.m. | #3
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
Arnd Bergmann Oct. 4, 2017, 11:06 a.m. | #4
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
Sudeep Holla Oct. 4, 2017, 11:07 a.m. | #5
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
Arnd Bergmann Oct. 4, 2017, 11:13 a.m. | #6
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
Arnd Bergmann Oct. 4, 2017, 11:19 a.m. | #7
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
Arnd Bergmann Oct. 4, 2017, 11:24 a.m. | #8
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
Arnd Bergmann Oct. 4, 2017, 11:30 a.m. | #9
> +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
Sudeep Holla Oct. 4, 2017, 11:32 a.m. | #10
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
Arnd Bergmann Oct. 4, 2017, 12:35 p.m. | #11
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
Sudeep Holla Oct. 4, 2017, 1:53 p.m. | #12
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
Arnd Bergmann Oct. 4, 2017, 2:17 p.m. | #13
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
Sudeep Holla Oct. 4, 2017, 2:47 p.m. | #14
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
Sudeep Holla Oct. 4, 2017, 5:37 p.m. | #15
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
Arnd Bergmann Oct. 5, 2017, 11:20 a.m. | #16
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
Sudeep Holla Oct. 5, 2017, 11:26 a.m. | #17
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
Arnd Bergmann Oct. 5, 2017, 11:56 a.m. | #18
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
Sudeep Holla Oct. 5, 2017, 12:56 p.m. | #19
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
Rob Herring Oct. 5, 2017, 11:20 p.m. | #20
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
Jassi Brar Oct. 6, 2017, 11:01 a.m. | #21
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
Jassi Brar Oct. 6, 2017, 11:34 a.m. | #22
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
Sudeep Holla Oct. 6, 2017, 1:27 p.m. | #23
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
Jassi Brar Oct. 6, 2017, 1:34 p.m. | #24
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
Sudeep Holla Oct. 6, 2017, 1:41 p.m. | #25
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
Jassi Brar Oct. 7, 2017, 2:26 a.m. | #26
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
Sudeep Holla Oct. 9, 2017, 2:37 p.m. | #27
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
Jassi Brar Oct. 9, 2017, 2:46 p.m. | #28
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
Rob Herring Oct. 9, 2017, 10:57 p.m. | #29
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
Jassi Brar Oct. 10, 2017, 1:52 a.m. | #30
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
Bjorn Andersson Oct. 12, 2017, 9:20 p.m. | #31
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
Stephen Boyd Nov. 2, 2017, 7:23 a.m. | #32
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
Sudeep Holla Nov. 2, 2017, 10:04 a.m. | #33
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
Stephen Boyd Nov. 3, 2017, 3:12 p.m. | #34
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