lkml.org 
[lkml]   [2017]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 04/22] firmware: arm_scmi: add basic driver infrastructure for SCMI
    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

    \
     
     \ /
      Last update: 2017-10-04 13:00    [W:4.046 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site