| 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: <2c8f7f6c-66bb-f931-c026-6ca2e000433a@collabora.com>
Date: Fri, 26 Nov 2021 11:07:22 +0100
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: "Flora.Fu" <flora.fu@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Mark Brown <broonie@...nel.org>,
Sumit Semwal <sumit.semwal@...aro.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-media@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
Yong Wu <yong.wu@...iatek.com>,
Pi-Cheng Chen <pi-cheng.chen@...iatek.com>
Subject: Re: [RFC 08/13] soc: mediatek: apu: Add apusys rv driver
Il 26/11/21 10:25, Flora.Fu ha scritto:
> On Tue, 2021-10-26 at 17:21 +0200, AngeloGioacchino Del Regno wrote:
>> Il 23/10/21 13:14, Flora Fu ha scritto:
>>> Add driver for control APU tinysys
>>>
>>> APU integrated subsystem having MD32RV33 (MD32) that runs tinysys
>>> The tinsys is running on a micro processor in APU.
>>> Its firmware is load and boot from Kernel side. Kernel and tinysys
>>> use
>>> IPI to tx/rx messages.
>>>
>>> Signed-off-by: Flora Fu <flora.fu@...iatek.com>
>>> ---
>>> drivers/soc/mediatek/apusys/Makefile | 6 +
>>> drivers/soc/mediatek/apusys/apu-config.h | 100 +++
>>> drivers/soc/mediatek/apusys/apu-core.c | 2 +
>>> drivers/soc/mediatek/apusys/apu-core.h | 2 +
>>> drivers/soc/mediatek/apusys/apu-ipi.c | 486 ++++++++++++
>>
>> I'm not sure of that, but your apu-ipi.c may be more suited to be in
>> drivers/remoteproc instead.
>>
>>> drivers/soc/mediatek/apusys/apu-mbox.c | 83 ++
>>
>> apu-mbox.c should go to drivers/mailbox/ and you should register it
>> with
>> the mailbox API as a mailbox controller instead of what you're
>> currently
>> doing...
>>
>> From what I see, you have functions in there that can be indeed
>> mapped
>> to struct mbox_chan_ops .send_data and .peek_data... also your
>> function
>> apu_mbox_wait_inbox seems to be waiting on an interrupt, and such irq
>> is
>> apparently your "mbox0_irq" (as you named it in the dt).
>> In that case, you can also manage that in your drivers/mailbox/
>> driver.
>> +
>>> drivers/soc/mediatek/apusys/apu-mbox.h | 27 +
>>> drivers/soc/mediatek/apusys/apu-rproc.c | 806
>>> ++++++++++++++++++++
>>
>> The apu-rproc.c driver seems to be a good candidate to be moved away
>> from
>>
>> drivers/soc/mediatek/apusys/ - as this is indeed a remoteproc driver.
>>
>> Having it as drivers/remoteproc/mtk_apu.c seems to be a good option.
>>
>
> For mbox/ipi/apu-rproc, we will check the kernel framework and make
> more fix on it.
>
>>
>>> drivers/soc/mediatek/apusys/apu-sw-logger.c | 429 +++++++++++
>>
>> This one definitely belongs here in drivers/soc/mediatek, and it's a
>> consumer
>> of the mailbox driver.
>>
>>> drivers/soc/mediatek/apusys/apu.h | 256 +++++++
>>> drivers/soc/mediatek/apusys/mt81xx-plat.c | 320 ++++++++
>>
>> If we end up keeping to be in need to have a separate mt81xx-plat.c
>> file,
>> then I believe this should have another name, so that it becomes one
>> that
>> aggregates all of the very-platform-specific functions in one,
>> instead of
>> having one file for each platform.
>>
>> Though, it may also be possible that this file will disappear
>> entirely:
>> since most of the things here will be moved around, it may become
>> mostly
>> empty... but it's probably too soon to judge.
>
> The mt81xx-platform.c is prepared for the similiar apu desing platforms
> but some of them will have differnt operations in the functions. I will
> just keep it and we can reivew it again in the feature versions.
>
>>
>>> 11 files changed, 2517 insertions(+)
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-config.h
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-ipi.c
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-mbox.c
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-mbox.h
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-rproc.c
>>> create mode 100644 drivers/soc/mediatek/apusys/apu-sw-logger.c
>>> create mode 100644 drivers/soc/mediatek/apusys/apu.h
>>> create mode 100644 drivers/soc/mediatek/apusys/mt81xx-plat.c
>>>
>>
>> snip...
>>
>>> diff --git a/drivers/soc/mediatek/apusys/apu-ipi.c
>>> b/drivers/soc/mediatek/apusys/apu-ipi.c
>>> new file mode 100644
>>> index 000000000000..547e034b3620
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/apusys/apu-ipi.c
>>
>> snip...
>>
>>> +int apu_ipi_init(struct platform_device *pdev, struct mtk_apu
>>> *apu)
>>> +{
>>> + struct device *dev = apu->dev;
>>> + int i, ret;
>>> +
>>> + tx_serial_no = 0;
>>> + rx_serial_no = 0;
>>> +
>>> + mutex_init(&apu->send_lock);
>>> + spin_lock_init(&apu->usage_cnt_lock);
>>> + for (i = 0; i < APU_IPI_MAX; i++) {
>>> + mutex_init(&apu->ipi_desc[i].lock);
>>> + lockdep_set_class_and_name(&apu->ipi_desc[i].lock,
>>> + &ipi_lock_key[i],
>>> + apu->platdata-
>>>> ipi_attrs[i].name);
>>> + }
>>> +
>>> + init_waitqueue_head(&apu->run.wq);
>>> + init_waitqueue_head(&apu->ack_wq);
>>> +
>>> + /* APU initialization IPI register */
>>> + ret = apu_ipi_register(apu, APU_IPI_INIT, apu_init_ipi_handler,
>>> apu);
>>> + if (ret) {
>>> + dev_err(dev, "failed to register ipi for init,
>>> ret=%d\n",
>>> + ret);
>>> + return ret;
>>> + }
>>> +
>>> + /* add rpmsg subdev */
>>> + apu_add_rpmsg_subdev(apu);
>>> +
>>> + /* register mailbox IRQ */
>>> + apu->mbox0_irq_number = platform_get_irq_byname(pdev,
>>> "mbox0_irq");
>>> + dev_info(&pdev->dev, "%s: mbox0_irq = %d\n", __func__,
>>> + apu->mbox0_irq_number);
>>> +
>>> + ret = devm_request_threaded_irq(&pdev->dev, apu-
>>>> mbox0_irq_number,
>>> + NULL, apu_ipi_handler,
>>> IRQF_ONESHOT,
>>> + "apu_ipi", apu);
>>
>> This is the mailbox interrupt... but it's handled in this driver
>> instead of
>> being handler in the mailbox driver... it's a bit confusing.
>>
>> Is this interrupt supposed to fire as a mailbox doorbell or..?
>> In that case, you should request it in the mailbox driver and
>> register an
>> interrupt controller (still, in the mailbox driver) so that you can
>> export
>> a sw interrupt to this one.
>>
>> Or, maybe you can use notifiers to catch the mailbox message in this
>> driver?
>
> Ack.
>
>>> + if (ret < 0)
>>> + goto remove_rpmsg_subdev;
>>> +
>>> + apu_mbox_hw_init(apu);
>>> +
>>> + return 0;
>>> +
>>> +remove_rpmsg_subdev:
>>> + apu_remove_rpmsg_subdev(apu);
>>> + apu_ipi_unregister(apu, APU_IPI_INIT);
>>> +
>>> + return ret;
>>> +}
>>
>> snip...
>>
>>> diff --git a/drivers/soc/mediatek/apusys/apu-rproc.c
>>> b/drivers/soc/mediatek/apusys/apu-rproc.c
>>> new file mode 100644
>>> index 000000000000..e2fe63dd6cc1
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/apusys/apu-rproc.c
>>> @@ -0,0 +1,806 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2021 MediaTek Inc.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/iommu.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/module.h>
>>> +#include <linux/remoteproc.h>
>>> +#include <linux/time64.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/pm_domain.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/sched/clock.h>
>>> +#include <linux/time64.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include "apu.h"
>>> +#include "apu-config.h"
>>> +#include "apu-core.h"
>>> +
>>> +/* cmd */
>>> +enum {
>>> + DPIDLE_CMD_LOCK_IPI = 0x5a00,
>>> + DPIDLE_CMD_UNLOCK_IPI = 0x5a01,
>>> + DPIDLE_CMD_PDN_UNLOCK = 0x5a02,
>>> +};
>>> +
>>> +/* ack */
>>> +enum {
>>> + DPIDLE_ACK_OK = 0,
>>> + DPIDLE_ACK_LOCK_BUSY,
>>> + DPIDLE_ACK_POWER_DOWN_FAIL,
>>> +};
>>> +
>>> +static struct work_struct *apu_pwr_work;
>>> +static struct workqueue_struct *apu_pwr_wq;
>>> +static struct dentry *dbg_root;
>>> +
>>> +static void *apu_da_to_va(struct rproc *rproc, u64 da, size_t len,
>>> + bool *is_iomem)
>>> +{
>>> + void *ptr = NULL;
>>> + struct mtk_apu *apu = (struct mtk_apu *)rproc->priv;
>>> +
>>> + if (da >= DRAM_OFFSET && da < DRAM_OFFSET + CODE_BUF_SIZE) {
>>> + ptr = apu->code_buf + (da - DRAM_OFFSET);
>>> + } else {
>>> + dev_err(apu->dev, "%s: invalid da: da = 0x%llx, len =
>>> %zu\n",
>>> + __func__, da, len);
>>> + }
>>> + return ptr;
>>> +}
>>> +
>>> +static int apu_run(struct rproc *rproc)
>>> +{
>>> + struct mtk_apu *apu = (struct mtk_apu *)rproc->priv;
>>> + struct mtk_apu_hw_ops *hw_ops = &apu->platdata->ops;
>>> + struct device *dev = apu->dev;
>>> + struct apu_run *run = &apu->run;
>>> + struct timespec64 begin, end, delta;
>>> + int ret;
>>> +
>>> + pm_runtime_get_sync(apu->dev);
>>> + hw_ops->start(apu);
>>
>> Don't forget to always check return values.....
>
> Ack.
>>
>>> +
>>> + /* check if boot success */
>>> + ktime_get_ts64(&begin);
>>> + ret = wait_event_interruptible_timeout(run->wq,
>>> + run->signaled,
>>> + msecs_to_jiffies(10000))
>>> ;
>>
>> #define APU_INIT_TIMEOUT_MS 10000
>>
>> ...but then, does it really need 10 *seconds* for that?! That's a lot
>> of time...
>>
>>> + ktime_get_ts64(&end);
>>> + if (ret == 0) {
>>> + dev_info(dev, "APU initialization timeout!!\n");
>>> + ret = -ETIME;
>>> + goto stop;
>>> + }
>>> + if (ret == -ERESTARTSYS) {
>>> + dev_info(dev, "wait APU interrupted by a signal!!\n");
>>> + goto stop;
>>> + }
>>> +
>>> + apu->boot_done = true;
>>> + delta = timespec64_sub(end, begin);
>>> + dev_info(dev, "APU uP boot success. boot time: %llu s, %llu
>>> ns\n",
>>> + (u64)delta.tv_sec, (u64)delta.tv_nsec);
>>> +
>>> + return 0;
>>> +
>>> +stop:
>>> + hw_ops->stop(apu);
>>> +
>>> + return ret;
>>> +}
>>> +
>>
>>
>> snip...
>>
>>
>>> +
>>> +static int apu_config_setup(struct mtk_apu *apu)
>>> +{
>>> + struct device *dev = apu->dev;
>>> + unsigned long flags;
>>> + int ret;
>>> +
>>> + apu->conf_buf = dma_alloc_coherent(apu->dev, CONFIG_SIZE,
>>> + &apu->conf_da, GFP_KERNEL);
>>> +
>>> + if (!apu->conf_buf || apu->conf_da == 0) {
>>> + dev_info(dev, "%s: dma_alloc_coherent fail\n",
>>> __func__);
>>> + return -ENOMEM;
>>> + }
>>> + memset(apu->conf_buf, 0, CONFIG_SIZE);
>>> +
>>> + apu_config_user_ptr_init(apu);
>>> + spin_lock_irqsave(&apu->reg_lock, flags);
>>> + iowrite32((u32)apu->conf_da, apu->apu_mbox + HOST_CONFIG_ADDR);
>>> + spin_unlock_irqrestore(&apu->reg_lock, flags);
>>> +
>>> + apu->conf_buf->time_offset = sched_clock();
>>> + ret = apu_ipi_config_init(apu);
>>> + if (ret) {
>>> + dev_info(dev, "apu ipi config init failed\n");
>>> + goto out;
>>> + }
>>> +
>>> + ret = sw_logger_config_init(apu);
>>
>> From what I understand, the sw logger is not critical for
>> functionality... so
>> it should probably be a "pluggable" instead.
>> Also, since that sw logger seems to be "simply" reading from a
>> mailbox, it
>> should be pretty straightforward to make it so, in which case, you
>> wouldn't
>> be initializing it here, but as a platform driver instead (if
>> debugging enabled?)
>
> It is for debug usage and I think it can be a pluggable module. But it
> still needs configurations in the boot stage to make micorprocessor
> enable the logger. I will update this part.
>
>>
>>> + if (ret) {
>>> + dev_err(dev, "sw logger config init failed\n");
>>> + goto err_sw_logger;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +err_sw_logger:
>>> + apu_ipi_config_remove(apu);
>>> +out:
>>> + return ret;
>>> +}
>>> +
>>
>> snip...
>>
>>> diff --git a/drivers/soc/mediatek/apusys/apu.h
>>> b/drivers/soc/mediatek/apusys/apu.h
>>> new file mode 100644
>>> index 000000000000..5bbc46416a19
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/apusys/apu.h
>>> @@ -0,0 +1,256 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2021 MediaTek Inc.
>>> + */
>>> +
>>> +#ifndef APU_H
>>> +#define APU_H
>>> +#include <linux/platform_device.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/rpmsg/mtk_rpmsg.h>
>>> +
>>> +#include "apu-config.h"
>>> +
>>> +/* setup the SMC command ops */
>>> +#define MTK_SIP_APU_START_MCU 0x00
>>> +#define MTK_SIP_APU_STOP_MCU 0x01
>>> +
>>> +/* md32_sysctrl register definition */
>>> +#define MD32_SYS_CTRL 0x0
>>> +#define MD32_MON_PC 0x838
>>> +#define MD32_MON_LR 0x83c
>>> +#define MD32_MON_SP 0x840
>>> +#define MD32_STATUS 0x844
>>> +
>>> +/*wdt register */
>>> +#define WDT_INT 0x0
>>> +#define WDT_CTRL0 0x4
>>> +#define WDT_EN BIT(31)
>>> +
>>> +/* apu_mbox spare regiter */
>>
>> /* apu_mbox spare register: mbox 0..6, spare 0..3 */
>> #define REG_MBOX_SPARE(mbox, reg) ((0x40 + (0x100 * mbox)) + (reg
>> * 0x4))
>> #define REG_MBOX0_SPARE(n) REG_MBOX_SPARE(0, n)
>> #define REG_MBOX6_SPARE(n) REG_MBOX_SPARE(6, n)
>>
>> #define HOST_CONFIG_ADDR REG_MBOX_SPARE(0, 2)
>>
>> Would that be better? Granted, mbox1-5 are also accessible and
>> perhaps used
>> in the future.
>
> Ack.
>
>>> +#define MBOX0_SPARE0 0x40
>>> +#define MBOX0_SPARE1 0x44
>>> +#define MBOX0_SPARE2 0x48
>>> +#define MBOX0_SPARE3 0x4C
>>> +#define MBOX6_SPARE0 0x640
>>> +#define MBOX6_SPARE1 0x644
>>> +#define MBOX6_SPARE2 0x648
>>> +#define MBOX6_SPARE3 0x64C
>>> +
>>> +#define HOST_CONFIG_ADDR MBOX0_SPARE2
>>> +
>>> +#define LOG_W_PTR (MBOX0_SPARE0)
>>> +#define LOG_R_PTR (MBOX0_SPARE1)
>>> +#define LOG_OV_FLG (MBOX0_SPARE3)
>>> +
>>> +/* rv setup */
>>> +#define F_PRELOAD_FIRMWARE BIT(0)
>>> +#define F_AUTO_BOOT BIT(1)
>>> +
>>
>> Use SZ_* macros where possible
>
> Ack.
>>
>>> +#define TCM_SIZE (128UL * 1024UL)
>>> +#define CODE_BUF_SIZE (1024UL * 1024UL)
>>> +#define DRAM_DUMP_SIZE (CODE_BUF_SIZE - TCM_SIZE)
>>> +#define REG_SIZE (4UL * 151UL)
>>> +#define TBUF_SIZE (4UL * 32UL)
>>> +#define CACHE_DUMP_SIZE (37UL * 1024UL)
>>> +#define DRAM_OFFSET (0x00000UL)
>>> +#define DRAM_DUMP_OFFSET (TCM_SIZE)
>>> +#define TCM_OFFSET (0x1d700000UL)
>>> +#define CODE_BUF_DA (DRAM_OFFSET)
>>> +
>>> +/* ipi */
>>> +#define APU_FW_VER_LEN 32
>>> +#define APU_SHARE_BUFFER_SIZE 256
>>> +
>>> +#define IPI_LOCKED 1
>>> +#define IPI_UNLOCKED 0
>>> +
>>> +#define IPI_HOST_INITIATE 0
>>> +#define IPI_APU_INITIATE 1
>>> +#define IPI_WITH_ACK 1
>>> +#define IPI_WITHOUT_ACK 0
>>> +
>>> +enum {
>>> + APU_IPI_INIT = 0,
>>> + APU_IPI_NS_SERVICE,
>>> + APU_IPI_DEEP_IDLE,
>>> + APU_IPI_CTRL_RPMSG,
>>> + APU_IPI_MIDDLEWARE,
>>> + APU_IPI_REVISER_RPMSG,
>>> + APU_IPI_PWR_TX,
>>> + APU_IPI_PWR_RX,
>>> + APU_IPI_MDLA_TX,
>>> + APU_IPI_MDLA_RX,
>>> + APU_IPI_TIMESYNC,
>>> + APU_IPI_EDMA_TX,
>>> + APU_IPI_MNOC_TX,
>>> + APU_IPI_MAX,
>>> +};
>>> +
>>> +struct mtk_apu;
>>> +
>>> +struct mtk_apu_hw_ops {
>>> + int (*init)(struct mtk_apu *apu);
>>> + int (*exit)(struct mtk_apu *apu);
>>> + int (*start)(struct mtk_apu *apu);
>>> + int (*stop)(struct mtk_apu *apu);
>>> + int (*resume)(struct mtk_apu *apu);
>>> + int (*apu_memmap_init)(struct mtk_apu *apu);
>>> + void (*apu_memmap_remove)(struct mtk_apu *apu);
>>> + void (*cg_gating)(struct mtk_apu *apu);
>>> + void (*cg_ungating)(struct mtk_apu *apu);
>>> + void (*rv_cachedump)(struct mtk_apu *apu);
>>> +
>>> + /* power related ops */
>>> + int (*power_init)(struct mtk_apu *apu);
>>> + int (*power_on)(struct mtk_apu *apu);
>>> + int (*power_off)(struct mtk_apu *apu);
>>> +};
>>> +
>>> +struct apu_ipi {
>>> + char *name;
>>> + unsigned int direction:1;
>>> + unsigned int ack:1;
>>> +};
>>> +
>>> +struct mtk_apu_platdata {
>>> + u32 flags;
>>> + struct mtk_apu_hw_ops ops;
>>> + const struct apu_ipi *ipi_attrs;
>>> +};
>>> +
>>> +struct dpidle_msg {
>>> + u32 cmd;
>>> + u32 ack;
>>> +};
>>> +
>>> +struct apu_run {
>>> + s8 fw_ver[APU_FW_VER_LEN];
>>> + u32 signaled;
>>> + wait_queue_head_t wq;
>>> +};
>>> +
>>> +struct apu_ipi_desc {
>>> + struct mutex lock; /*ipi hanlder mutex */
>>
>> typo
>>
> Ack.
>>> + ipi_handler_t handler;
>>> + void *priv;
>>> + /*
>>> + * positive: host-initiated ipi outstanding count
>>> + * negative: apu-initiated ipi outstanding count
>>> + */
>>> + int usage_cnt;
>>> +};
>>> +
>>> +struct mtk_share_obj {
>>> + u8 share_buf[APU_SHARE_BUFFER_SIZE];
>>> +};
>>> +
>>> +struct sw_logger_seq_data {
>>> + u32 w_ptr;
>>> + u32 r_ptr;
>>> + u32 overflow_flg;
>>> + int i;
>>> + int is_finished;
>>> + char *data;
>>> + bool startl_first;
>>> +};
>>> +
>>> +struct mtk_apu {
>>> + struct rproc *rproc;
>>> + struct device *dev;
>>> + void __iomem *apu_mbox;
>>> + void __iomem *md32_sysctrl;
>>> + void __iomem *apu_wdt;
>>> + int mbox0_irq_number;
>>> + int wdt_irq_number;
>>> + spinlock_t reg_lock; /* register r/w lock */
>>> +
>>> + /* Buffer to place execution area */
>>> + void *code_buf;
>>> + dma_addr_t code_da;
>>> +
>>> + /* Buffer to place config area */
>>> + struct config_v1 *conf_buf;
>>> + dma_addr_t conf_da;
>>> +
>>> + /* to synchronize boot status of remote processor */
>>> + struct apu_run run;
>>> +
>>> + /* to prevent multiple ipi_send run concurrently */
>>> + struct mutex send_lock;
>>> + spinlock_t usage_cnt_lock; /* ipi occipued lock */
>>> + struct apu_ipi_desc ipi_desc[APU_IPI_MAX];
>>> + bool ipi_id_ack[APU_IPI_MAX]; /* per-ipi ack */
>>> + bool ipi_inbound_locked;
>>> + wait_queue_head_t ack_wq; /* for waiting for ipi ack */
>>> +
>>> + /* ipi */
>>> + struct rproc_subdev *rpmsg_subdev;
>>> + dma_addr_t recv_buf_da;
>>> + struct mtk_share_obj *recv_buf;
>>> + dma_addr_t send_buf_da;
>>> + struct mtk_share_obj *send_buf;
>>> +
>>> + /* time sync */
>>> + struct work_struct timesync_work;
>>> + struct workqueue_struct *timesync_wq;
>>> + u64 timesync_stamp;
>>> +
>>> + /*deep idle */
>>> + struct dpidle_msg recv_msg;
>>> + struct work_struct deepidle_work;
>>> + struct workqueue_struct *apu_deepidle_workq;
>>> + struct work_struct pwron_dbg_wk;
>>> +
>>> + struct mtk_apu_platdata *platdata;
>>> +
>>> + /* link power deive */
>>> + struct device *power_dev;
>>> + bool boot_done;
>>> + struct work_struct pwr_work;
>>> +
>>> + /* logger and debug */
>>> + struct dentry *dbg_root;
>>> + dma_addr_t handle;
>>> + char *sw_log_buf;
>>> + spinlock_t sw_logger_spinlock; /* logger status update lock */
>>> + struct sw_logger_seq_data pseqdata_lock;
>>> + struct sw_logger_seq_data *pseqdata;
>>> +};
>>> +
>>> +struct apu_coredump {
>>> + char tcmdump[TCM_SIZE];
>>> + char ramdump[DRAM_DUMP_SIZE];
>>> + char regdump[REG_SIZE];
>>> + char tbufdump[TBUF_SIZE];
>>> + u32 cachedump[CACHE_DUMP_SIZE / sizeof(u32)];
>>> +} __packed;
>>> +
>>> +int apu_ipi_config_init(struct mtk_apu *apu);
>>> +void apu_ipi_config_remove(struct mtk_apu *apu);
>>> +void apu_ipi_remove(struct mtk_apu *apu);
>>> +int apu_ipi_init(struct platform_device *pdev, struct mtk_apu
>>> *apu);
>>> +int apu_ipi_register(struct mtk_apu *apu, u32 id,
>>> + ipi_handler_t handler, void *priv);
>>> +void apu_ipi_unregister(struct mtk_apu *apu, u32 id);
>>> +int apu_ipi_send(struct mtk_apu *apu, u32 id, void *data, u32 len,
>>> + u32 wait_ms);
>>> +int apu_ipi_lock(struct mtk_apu *apu);
>>> +void apu_ipi_unlock(struct mtk_apu *apu);
>>> +
>>> +void apu_deepidle_power_on_aputop(struct mtk_apu *apu);
>>> +
>>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>>> +int sw_logger_config_init(struct mtk_apu *apu);
>>> +void sw_logger_config_remove(struct mtk_apu *apu);
>>> +int apu_sw_logger_init(struct mtk_apu *apu);
>>> +void apu_sw_logger_remove(struct mtk_apu *apu);
>>> +#else
>>> +static inline int sw_logger_config_init(struct mtk_apu *apu) {
>>> return 0; }
>>> +static inline void sw_logger_config_remove(struct mtk_apu *apu) {
>>> }
>>> +static inline int apu_sw_logger_init(struct mtk_apu *apu) { return
>>> 0; }
>>> +static inline void apu_sw_logger_remove(struct mtk_apu *apu) { }
>>> +#endif
>>> +
>>> +extern const struct mtk_apu_platdata mt8192_platdata;
>>> +#endif /* APU_H */
>>> diff --git a/drivers/soc/mediatek/apusys/mt81xx-plat.c
>>> b/drivers/soc/mediatek/apusys/mt81xx-plat.c
>>> new file mode 100644
>>> index 000000000000..54f75c8d07c3
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/apusys/mt81xx-plat.c
>>> @@ -0,0 +1,320 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2021 MediaTek Inc.
>>> + */
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/sched/clock.h>
>>> +#include <linux/soc/mediatek/mtk_sip_svc.h>
>>> +
>>> +#include "apu.h"
>>> +
>>> +static const struct apu_ipi mt81xx_ipi_attrs[APU_IPI_MAX] = {
>>
Hello Flora,
>> In here, this is used only to pass it to the remoteproc "ipi" driver:
>> it would make
>> sense if this was in the ipi driver, associated to a compatible like
^^^^^ Sorry, I was meaning the -> rproc driver <-
>> mediatek,mt8192-ipi.
>>
> Hi, Algelo,
> I am not quite sure about the comments. Could you explain more details?
>
> The list is used in kernel as ipi channe attributes by
> platforms(different platfrom will has differnt settings) to create ipi
> channesl by mtk-rpmsg. The ipi-attrs is add into platform data.
>
> const struct mtk_apu_platdata mt8192_platdata = {
> .flags = F_AUTO_BOOT,
> .ipi_attrs = mt81xx_ipi_attrs,
> .ops = {
> .init = NULL,
> snip.
> .power_init = mt81xx_apu_power_init,
> .power_on = mt81xx_apu_power_on,
> .power_off = mt81xx_apu_power_off,
> },
> };
>
Reiterating: this file is very short and also it looks like the code
in here will be common (or commonizable) between multiple SoCs,
so please move everything that is in here to apu-rproc.c.
Regards,
- Angelo
>>> + [APU_IPI_INIT] = {
>>> + .name = "init",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITHOUT_ACK,
>>> + },
>>> + [APU_IPI_NS_SERVICE] = {
>>> + .name = "name-service",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITHOUT_ACK,
>>> + },
>>> + [APU_IPI_DEEP_IDLE] = {
>>> + .name = "deep_idle",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_CTRL_RPMSG] = {
>>> + .name = "apu-ctrl-rpmsg",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_MIDDLEWARE] = {
>>> + .name = "apu-mdw-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_REVISER_RPMSG] = {
>>> + .name = "apu-reviser-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_PWR_TX] = {
>>> + .name = "apupwr-tx-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_PWR_RX] = {
>>> + .name = "apupwr-rx-rpmsg",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_MDLA_TX] = {
>>> + .name = "mdla-tx-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_MDLA_RX] = {
>>> + .name = "mdla-rx-rpmsg",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_TIMESYNC] = {
>>> + .name = "apu-timesync",
>>> + .direction = IPI_APU_INITIATE,
>>> + .ack = IPI_WITH_ACK,
>>> + },
>>> + [APU_IPI_EDMA_TX] = {
>>> + .name = "apu-edma-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITHOUT_ACK,
>>> + },
>>> + [APU_IPI_MNOC_TX] = {
>>> + .name = "apu-mnoc-rpmsg",
>>> + .direction = IPI_HOST_INITIATE,
>>> + .ack = IPI_WITHOUT_ACK,
>>> + },
>>> +};
>>> +
>>> +static void apu_reset_mcu(struct mtk_apu *apu)
>>> +{
>>> + u32 reg;
>>> +
>>> + /* assert mcu reset */
>>> + reg = ioread32(apu->md32_sysctrl);
>>> + iowrite32(reg & ~0x1, apu->md32_sysctrl);
>>> + mdelay(10);
>>> + iowrite32(reg | 0x1, apu->md32_sysctrl);
>>> +}
>>> +
>>> +static int apu_start_mcu(struct mtk_apu *apu)
>>> +{
>>> + struct arm_smccc_res ares;
>>> +
>>> + /* initialize IOMMU and ACP config (iommu_tr_en=1, acp_en=0) */
>>> + iowrite32(0xEA9, apu->md32_sysctrl);
>>> +
>>> + arm_smccc_smc(MTK_SIP_APUSYS_CONTROL, MTK_SIP_APU_START_MCU,
>>> + 0, 0, 0, 0, 0, 0, &ares);
>>> + if (ares.a0)
>>> + dev_err(apu->dev, "start mcu fail: %lu\n", ares.a0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int apu_stop_mcu(struct mtk_apu *apu)
>>> +{
>>> + struct arm_smccc_res ares;
>>> +
>>> + arm_smccc_smc(MTK_SIP_APUSYS_CONTROL, MTK_SIP_APU_STOP_MCU,
>>> + 0, 0, 0, 0, 0, 0, &ares);
>>> + if (ares.a0)
>>> + dev_err(apu->dev, "stop mcufail: %lu\n", ares.a0);
>>> +
>>> + return 0;
>>> +}
>>
>> Is it expected for other SoCs to have different (or more) secure
>> world calls?
>> If it is, then it may be worth it to move this to a different driver
>> in
>> drivers/firmware, so that you will be able to map different/more
>> values to
>> different compatibles.
>>
>> Otherwise, keep it here.
>
> This is for apu usage and only for apu. I will keep it here. The SMC
> receivers are also found in ATF upstream.
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/9709/11.
>
> Thanks for your comments.
>
>> - Angelo
Powered by blists - more mailing lists