[RFC PATCH v2 6/9] firmware: Add legacy SCPI protocol driver

[Sudeep Holla]

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 at baylibre.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