| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5774FA15.6060302@arm.com>
Date: Thu, 30 Jun 2016 11:53:09 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Neil Armstrong <narmstrong@...libre.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: Sudeep Holla <sudeep.holla@....com>,
linux-amlogic@...ts.infradead.org, khilman@...libre.com,
heiko@...ech.de, wxt@...k-chips.com, frank.wang@...k-chips.com
Subject: Re: [RFC PATCH v2 6/9] firmware: Add legacy SCPI protocol driver
On 21/06/16 11:02, Neil Armstrong wrote:
> Add legacy SCPI driver based on the latest SCPI driver but modified to behave
> like an earlier technology preview SCPI implementation that at least the
> Amlogic GXBB ARMv8 based platform uses in it's SCP firmware implementation.
>
> The main differences between the mainline, public and recommended SCPI
> implementation are :
> - virtual channels is not implemented
> - command word is passed by the MHU instead of the virtual channel ID
> - uses "sender id" in the command word for each commands groups
> - payload size shift in command word is different
> - command word is not in SRAM, so command queuing is not possible
> - command indexes are different
> - command data structures differs
> - commands are redirected to low or high priority channels by their indexes,
> so round-robin redirection is not possible
I doubt if that's the case. At-least the original arm scp f/w didn't
check that. Can you please trying sending any commands on any channel ?
>
> A clear disclaimer is added to make it clear this implementation should not
> be used for new products and is only here to support already released SoCs.
>
> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
> ---
> drivers/firmware/Kconfig | 20 ++
> drivers/firmware/Makefile | 1 +
> drivers/firmware/legacy_scpi.c | 644 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 665 insertions(+)
> create mode 100644 drivers/firmware/legacy_scpi.c
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 95b01f4..b9c2a33 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -31,6 +31,26 @@ config ARM_SCPI_PROTOCOL
> This protocol library provides interface for all the client drivers
> making use of the features offered by the SCP.
>
> +config LEGACY_SCPI_PROTOCOL
> + bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
Do we really need to add another config ? I thought we could just manage
with compatibles.
> + default y if ARCH_MESON
> + select ARM_SCPI_FW
> + help
> + System Control and Power Interface (SCPI) Message Protocol is
> + defined for the purpose of communication between the Application
> + Cores(AP) and the System Control Processor(SCP). The MHU peripheral
> + provides a mechanism for inter-processor communication between SCP
> + and AP.
[...]
> diff --git a/drivers/firmware/legacy_scpi.c b/drivers/firmware/legacy_scpi.c
> new file mode 100644
> index 0000000..4bd3ff7
> --- /dev/null
> +++ b/drivers/firmware/legacy_scpi.c
> @@ -0,0 +1,644 @@
[...]
> +
> +#define CMD_ID_SHIFT 0
> +#define CMD_ID_MASK 0x7f
> +#define CMD_SENDER_ID_SHIFT 8
> +#define CMD_SENDER_ID_MASK 0xff
Again this is something I introduced in the earlier driver. But from SCP
f/w perspective, it just sends that as is to the sender. I think we
retain token concept as is from the latest driver. Could you check
dropping them and check if f/w makes any assumption about these. It
should not IMO.
> +#define CMD_DATA_SIZE_SHIFT 20
> +#define CMD_DATA_SIZE_MASK 0x1ff
> +#define PACK_SCPI_CMD(cmd_id, sender, tx_sz) \
> + ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) | \
> + (((sender) & CMD_SENDER_ID_MASK) << CMD_SENDER_ID_SHIFT) | \
> + (((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
> +
> +#define CMD_SIZE(cmd) (((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
> +#define CMD_UNIQ_MASK (CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
> +#define CMD_XTRACT_UNIQ(cmd) ((cmd) & CMD_UNIQ_MASK)
> +
> +#define MAX_DVFS_DOMAINS 3
> +#define MAX_DVFS_OPPS 16
> +#define DVFS_LATENCY(hdr) (le32_to_cpu(hdr) >> 16)
> +#define DVFS_OPP_COUNT(hdr) ((le32_to_cpu(hdr) >> 8) & 0xff)
> +
> +#define MAX_RX_TIMEOUT (msecs_to_jiffies(30))
> +
> +enum legacy_scpi_error_codes {
This along with many other defines are exactly same, not need to
duplicate them.
> + SCPI_SUCCESS = 0, /* Success */
> + SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
> + SCPI_ERR_ALIGN = 2, /* Invalid alignment */
> + SCPI_ERR_SIZE = 3, /* Invalid size */
> + SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
> + SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
> + SCPI_ERR_RANGE = 6, /* Value out of range */
> + SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
> + SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
> + SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
> + SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
> + SCPI_ERR_DEVICE = 11, /* Device error */
> + SCPI_ERR_BUSY = 12, /* Device busy */
> + SCPI_ERR_MAX
> +};
> +
> +enum legacy_scpi_client_id {
Could be removed as mentioned above ?
> + SCPI_CL_NONE,
> + SCPI_CL_CLOCKS,
> + SCPI_CL_DVFS,
> + SCPI_CL_POWER,
> + SCPI_CL_THERMAL,
> + SCPI_CL_REMOTE,
> + SCPI_CL_LED_TIMER,
> + SCPI_MAX,
> +};
> +
> +enum legacy_scpi_std_cmd {
> + SCPI_CMD_INVALID = 0x00,
> + SCPI_CMD_SCPI_READY = 0x01,
> + SCPI_CMD_SCPI_CAPABILITIES = 0x02,
> + SCPI_CMD_EVENT = 0x03,
> + SCPI_CMD_SET_CSS_PWR_STATE = 0x04,
> + SCPI_CMD_GET_CSS_PWR_STATE = 0x05,
> + SCPI_CMD_CFG_PWR_STATE_STAT = 0x06,
> + SCPI_CMD_GET_PWR_STATE_STAT = 0x07,
> + SCPI_CMD_SYS_PWR_STATE = 0x08,
> + SCPI_CMD_L2_READY = 0x09,
> + SCPI_CMD_SET_AP_TIMER = 0x0a,
> + SCPI_CMD_CANCEL_AP_TIME = 0x0b,
> + SCPI_CMD_DVFS_CAPABILITIES = 0x0c,
> + SCPI_CMD_GET_DVFS_INFO = 0x0d,
> + SCPI_CMD_SET_DVFS = 0x0e,
> + SCPI_CMD_GET_DVFS = 0x0f,
> + SCPI_CMD_GET_DVFS_STAT = 0x10,
> + SCPI_CMD_SET_RTC = 0x11,
> + SCPI_CMD_GET_RTC = 0x12,
> + SCPI_CMD_CLOCK_CAPABILITIES = 0x13,
> + SCPI_CMD_SET_CLOCK_INDEX = 0x14,
> + SCPI_CMD_SET_CLOCK_VALUE = 0x15,
> + SCPI_CMD_GET_CLOCK_VALUE = 0x16,
> + SCPI_CMD_PSU_CAPABILITIES = 0x17,
> + SCPI_CMD_SET_PSU = 0x18,
> + SCPI_CMD_GET_PSU = 0x19,
> + SCPI_CMD_SENSOR_CAPABILITIES = 0x1a,
> + SCPI_CMD_SENSOR_INFO = 0x1b,
> + SCPI_CMD_SENSOR_VALUE = 0x1c,
> + SCPI_CMD_SENSOR_CFG_PERIODIC = 0x1d,
> + SCPI_CMD_SENSOR_CFG_BOUNDS = 0x1e,
> + SCPI_CMD_SENSOR_ASYNC_VALUE = 0x1f,
> + SCPI_CMD_COUNT
> +};
> +
> +struct legacy_scpi_xfer {
> + u32 cmd;
> + u32 status;
> + const void *tx_buf;
> + void *rx_buf;
> + unsigned int tx_len;
> + unsigned int rx_len;
> + struct completion done;
> +};
> +
> +struct legacy_scpi_chan {
> + struct mbox_client cl;
> + struct mbox_chan *chan;
> + void __iomem *tx_payload;
> + void __iomem *rx_payload;
> + spinlock_t rx_lock; /* locking for the rx pending list */
> + struct mutex xfers_lock;
> + struct legacy_scpi_xfer t;
> +};
> +
> +struct legacy_scpi_drvinfo {
> + int num_chans;
> + struct legacy_scpi_chan *channels;
> + struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
> +};
> +
Even these data structures could remain, and mended wherever needed or
an alternate can be added at worse. Complete copy paste seems
unnecessary to me.
> + legacy_scpi_info->channels = legacy_scpi_chan;
> + legacy_scpi_info->num_chans = count;
> + platform_set_drvdata(pdev, legacy_scpi_info);
> +
> + ret = devm_scpi_ops_register(dev, &legacy_scpi_ops);
Though the concept of registering scpi ops is really nice, I think we
may not require that for support this legacy scpi protocol.
In general, I see lot of code duplication, can you try not add another
file or config and introduce the legacy support into arm_scpi.c itself ?
--
Regards,
Sudeep
Powered by blists - more mailing lists