| 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: <578DF042.4080801@baylibre.com>
Date: Tue, 19 Jul 2016 11:17:54 +0200
From: Neil Armstrong <narmstrong@...libre.com>
To: Sudeep Holla <sudeep.holla@....com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: 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 06/30/2016 12:53 PM, Sudeep Holla wrote:
>
>
> 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 ?
I did, and it fails.
I'm waiting for advanced documentation for the fw, but it really seems the SCP
fw filter the command by the 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.
I can check. I'm afraid it may check for these.
>> +#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 ?
>
I can try but it will create a spaguetti monster since the core functions will be duplicated.
I really think the official scpi driver should follow it's path and stay clean and reliable,
and create a side-monster to support the ugly legacy based firmware for Amlogic and Rockchip.
The point is to find a way to share the scpi resource drivers in common !
Neil
Powered by blists - more mailing lists