[PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Kamal Dasu]

This patch set adds support for Broadcom TZOS to read and write to RPMB
partition using synchronized access to the controller hardware.
To achieve this Linux OS and the secure TZOS make use of:
- shared hardware semaphore register
- a set of SDIO shared work registers and
- IPI interrupt registers
The sdio shared work registers indicates next in queue to access the controller
and current agent in the queue. The currently running OS that needs access to
the controller puts itself in its slot of work register and if its next in line
it can try to grab the hardware semaphore and complete its mmc requests.
Next agent queue state is changed under the hardware semaphore lock before it
release it by looking at work slot register. send and receive IPI interrupts
between linux and secure world are used to indicatecompletion of transaction to
the waiting OS. TZOS has its own RPMB driver which accesses partition when it
wants to read/write RPMB frames. Current implementation assumes Linux and TZOS
as the two work agents. 

Change required adding two core mmc_host_ops request_start() and request_done()
to let the host controller driver know when a mmc request starts and ends so
that the access can be synchronized. This has been tested with both the sdhci
and cqhci access. Currently these ops are implemented by the sdhci-brcmstb
controller dirver to acquire and release the hardware semaphore before and
after access. This change to the mmc/core driver does not have any impact to
existing controller drivers.

Posting this path to get comments on the initial implementation.

Todo :
- Provide  hardware smeaphore using the harware spinlock driver framework
- Use IPI send receive interrupt controller driver

Kamal Dasu (3):
  mmc: add request_start() and request_done() mmc ops
  dt-bindings: mmc: brcm,sdhci-brcmstb: Add sdio sharing support
  mmc: sdhci-brcmstb: Add rpmb sharing support in host driver

 .../bindings/mmc/brcm,sdhci-brcmstb.yaml      |  16 +-
 drivers/mmc/core/core.c                       |  14 +-
 drivers/mmc/host/sdhci-brcmstb.c              | 275 +++++++++++++++++-
 include/linux/mmc/host.h                      |   4 +
 4 files changed, 303 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH RFC 1/3] mmc: add request_start() and request_done() mmc ops

[Kamal Dasu]

From: Kamal Dasu <kdasu@broadcom.com>

These ops enable host vendor driver to take action on request
start and request done both in case of sdhci and cqe requests.

Signed-off-by: Kamal Dasu <kdasu@broadcom.com>
---
 drivers/mmc/core/core.c  | 14 +++++++++++++-
 include/linux/mmc/host.h |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5241528f8b90..a835ebc6282c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -206,8 +206,11 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 	 * Request starter must handle retries - see
 	 * mmc_wait_for_req_done().
 	 */
-	if (mrq->done)
+	if (mrq->done) {
 		mrq->done(mrq);
+		if (host->ops->request_done)
+			host->ops->request_done(host, mrq);
+	}
 }
 
 EXPORT_SYMBOL(mmc_request_done);
@@ -340,6 +343,9 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 
 	init_completion(&mrq->cmd_completion);
 
+	if (host->ops->request_start)
+		host->ops->request_start(host, mrq);
+
 	mmc_retune_hold(host);
 
 	if (mmc_card_removed(host->card))
@@ -437,6 +443,9 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
 {
 	int err;
 
+	if (host->ops->request_start)
+		host->ops->request_start(host, mrq);
+
 	/*
 	 * CQE cannot process re-tuning commands. Caller must hold retuning
 	 * while CQE is in use.  Re-tuning can happen here only when CQE has no
@@ -512,6 +521,9 @@ void mmc_cqe_request_done(struct mmc_host *host, struct mmc_request *mrq)
 	}
 
 	mrq->done(mrq);
+
+	if (host->ops->request_done)
+		host->ops->request_done(host, mrq);
 }
 EXPORT_SYMBOL(mmc_cqe_request_done);
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 68f09a955a90..739b14117c26 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -171,6 +171,10 @@ struct mmc_host_ops {
 			    int err);
 	void	(*pre_req)(struct mmc_host *host, struct mmc_request *req);
 	void	(*request)(struct mmc_host *host, struct mmc_request *req);
+	void	(*request_start)(struct mmc_host *host,
+				 struct mmc_request *req);
+	void	(*request_done)(struct mmc_host *host,
+				 struct mmc_request *req);
 	/* Submit one request to host in atomic context. */
 	int	(*request_atomic)(struct mmc_host *host,
 				  struct mmc_request *req);
-- 
2.17.1

[PATCH RFC 2/3] dt-bindings: mmc: brcm,sdhci-brcmstb: Add sdio sharing support

[Kamal Dasu]

From: Kamal Dasu <kdasu@broadcom.com>

Introduce emmc flash sharing support using hardware semaphore
and SDIO share register to synchronize between TZOS and linux.

Signed-off-by: Kamal Dasu <kdasu@broadcom.com>
---
 .../bindings/mmc/brcm,sdhci-brcmstb.yaml         | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
index eee6be7a7867..b0ee39aeccb1 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
@@ -27,15 +27,23 @@ properties:
           - const: brcm,sdhci-brcmstb
 
   reg:
-    maxItems: 2
+    minItems: 2
 
   reg-names:
     items:
       - const: host
       - const: cfg
+      - const: share        # Optional reg 
+      - const: mmc_sem      # Optional reg
+      - const: flshr_ipis0  # Optional reg
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+
+  interrupt-names:
+    items:
+      - const: SDIO1_0
+      - const: recv_ipi0   # Optional interrupt
 
   clocks:
     minItems: 1
@@ -76,6 +84,7 @@ required:
   - compatible
   - reg
   - interrupts
+  - interrupt-names
   - clocks
   - clock-names
 
@@ -111,7 +120,8 @@ examples:
       supports-cqe;
       non-removable;
       bus-width = <0x8>;
-      interrupts = <0x0 0x27 0x4>;
+      interrupts-extended = <0x1 0x0 0x1f 0x4 0x18 0x11>;
+      interrupt-names = "SDIO1_0", "recv_ipi0";
       clocks = <&scmi_clk 245>;
       clock-names = "sw_sdio";
     };
-- 
2.17.1

Re: [PATCH RFC 2/3] dt-bindings: mmc: brcm,sdhci-brcmstb: Add sdio sharing support

[Krzysztof Kozlowski]

On 06/02/2025 23:09, Kamal Dasu wrote:
> From: Kamal Dasu <kdasu@broadcom.com>
> 
> Introduce emmc flash sharing support using hardware semaphore
> and SDIO share register to synchronize between TZOS and linux.
> 
> Signed-off-by: Kamal Dasu <kdasu@broadcom.com>

It's RFC, so I assume you do not expect review and then you can skip
this and obviously it is also NAK for the patch (so maintainers won't
take it).

However if you actually expect review, then:


> ---
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml         | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> index eee6be7a7867..b0ee39aeccb1 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> @@ -27,15 +27,23 @@ properties:
>            - const: brcm,sdhci-brcmstb
>  
>    reg:
> -    maxItems: 2
> +    minItems: 2

1. Missing constraint
2. Please follow DTS coding style
3. and finally this needs to be tested, so:

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Best regards,
Krzysztof

[PATCH RFC 3/3] mmc: sdhci-brcmstb: Add rpmb sharing support in host driver

[Kamal Dasu]

From: Kamal Dasu <kdasu@broadcom.com>

Adding sdio rpmb parition sharing support to brcmstb host driver.
The sdhci-brcmstb controller driver uses SDIO_SHARE registers and
HW semaphore register to synchronize access between linux and the
trusted zone firmware.

Signed-off-by: Kamal Dasu <kdasu@broadcom.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 275 ++++++++++++++++++++++++++++++-
 1 file changed, 273 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index 0ef4d578ade8..6ba90108cd5c 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -46,9 +46,32 @@
 /* Select all SD UHS type I SDR speed above 50MB/s */
 #define MMC_CAP_UHS_I_SDR_MASK	(MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104)
 
+#define SDHCI_BRCMSTB_AGENT_LINUX		'L'
+#define SDHCI_BRCMSTB_AGENT_TZOS		'A'
+#define FLSHARE_IPIS0_INT_SEND_MASK		BIT(17)
+#define HWSEM_AQUIRE		1
+#define HWSEM_RELEASE		0
+
+enum brcmstb_sdhci_share_reg {
+	BRCMSTB_SD_SHARE_REG_NEXT = 0x0,	/* Next Agent Register */
+	BRCMSTB_SD_SHARE_REG_PMC = 0x4,		/* Work Agent1 Register */
+	BRCMSTB_SD_SHARE_REG_TZOS = 0x8,	/* Work Agent2 Register */
+	BRCMSTB_SD_SHARE_REG_LINUX = 0xc,	/* Work Agent3 Register */
+};
+
+struct brcmstb_sdio_share_info {
+	void __iomem *share_reg;
+	void __iomem *sem_reg;
+	void __iomem *ipis0_reg;
+	int irq_recv;
+	wait_queue_head_t wq;
+};
+
+
 struct sdhci_brcmstb_priv {
 	void __iomem *cfg_regs;
 	unsigned int flags;
+	struct brcmstb_sdio_share_info *si;
 	struct clk *base_clk;
 	u32 base_freq_hz;
 };
@@ -288,7 +311,7 @@ static const struct brcmstb_match_priv match_priv_7216 = {
 	.ops = &sdhci_brcmstb_ops_7216,
 };
 
-static struct brcmstb_match_priv match_priv_74165b0 = {
+static const struct brcmstb_match_priv match_priv_74165b0 = {
 	.flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
 	.hs400es = sdhci_brcmstb_hs400es,
 	.ops = &sdhci_brcmstb_ops_74165b0,
@@ -303,6 +326,176 @@ static const struct of_device_id __maybe_unused sdhci_brcm_of_match[] = {
 	{},
 };
 
+static void sdhci_brcmstb_dbg_dump_sdio_share_regs(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+
+	dev_dbg(mmc_dev(host->mmc),
+		"sem:0x%x wn:0x%x wa:0x%x wl:0x%x\n",
+		readl(si->sem_reg),
+		readl(si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT),
+		readl(si->share_reg + BRCMSTB_SD_SHARE_REG_TZOS),
+		readl(si->share_reg + BRCMSTB_SD_SHARE_REG_LINUX));
+}
+
+static bool sdhci_brcmstb_host_is_next(struct brcmstb_sdio_share_info *si)
+{
+	u32 wn;
+
+	/* if work queue is empty or we are next */
+	wn = readl(si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT);
+	return (wn == 0 ? true : (wn == SDHCI_BRCMSTB_AGENT_LINUX));
+}
+
+#define SDHCI_BRCMSTB_HOST_TIMEOUT_MS  5
+
+static int sdhci_brcmstb_wait_for_host(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+	int ret = 0;
+
+	/* put self in wait queue if host not avialable  */
+	ret = wait_event_timeout(si->wq, sdhci_brcmstb_host_is_next(si),
+			 msecs_to_jiffies(SDHCI_BRCMSTB_HOST_TIMEOUT_MS) + 1);
+
+	return ret;
+}
+
+static u32 semreg_wrrdl(struct sdhci_host *host, u32 value, void __iomem *addr)
+{
+	writel(value, addr);
+	return readl(addr);
+}
+
+static void sdhci_brcmstb_hwsem(struct sdhci_host *host, int sem_op)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+	u32 wr_val;
+	u32 wa, sem_val;
+	bool is_next;
+	int ret = 0;
+
+	while (1) {
+		if (sem_op == HWSEM_AQUIRE)  {
+			/*
+			 * get hw sem :
+			 *
+			 * 1. write linux agent id to work register WL
+			 * 2. check if we are next in line or workqueue empty
+			 * 3. if linux is not next in queue go into waitq
+			 *
+			 * handle case where the next work agent was not set
+			 * but TZOS grabbed the semaphore before we could
+			 * in that case too we put ourselves in wait queue,
+			 * tzos shall remove linux from wait queue using IPI
+			 */
+			wr_val = SDHCI_BRCMSTB_AGENT_LINUX;
+			writel(wr_val, si->share_reg + BRCMSTB_SD_SHARE_REG_LINUX);
+			is_next = sdhci_brcmstb_host_is_next(si);
+			if (!is_next)
+				ret = sdhci_brcmstb_wait_for_host(host);
+		} else {
+			/*
+			 * release hw semphore
+			 * 1. set the next agent before releasing hw sem
+			 * 2. clear work agent
+			 * 3. Release hw semaphore
+			 */
+			wa = readl(si->share_reg + BRCMSTB_SD_SHARE_REG_TZOS);
+			writel(wa, si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT);
+			writel(0, si->share_reg + BRCMSTB_SD_SHARE_REG_LINUX);
+			wr_val = 0;
+			sem_val = readl(si->sem_reg);
+			if (!sem_val || sem_val != SDHCI_BRCMSTB_AGENT_LINUX)
+				goto out;
+
+		}
+
+		/* try to grab/release hw semaphore */
+		sem_val = semreg_wrrdl(host, wr_val, si->sem_reg);
+
+		if (!wr_val || (sem_val == wr_val))
+			break;
+	}
+
+	return;
+out:
+	sdhci_brcmstb_dbg_dump_sdio_share_regs(host);
+	dev_dbg(mmc_dev(host->mmc), "%s ret:%d\n",
+		(sem_op == HWSEM_AQUIRE) ? "get" : "release", ret);
+}
+
+static bool sdhci_brcmstb_cqe_is_idle(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct mmc_host *mmc = host->mmc;
+	struct cqhci_host *cq_host;
+
+	if (!(priv->flags & BRCMSTB_PRIV_FLAGS_HAS_CQE))
+		return true;
+
+	cq_host = mmc->cqe_private;
+	/* still processing mrqs in cqe mode  */
+	return (cq_host->qcnt == 0 && !cq_host->recovery_halt) ? true : false;
+}
+
+static void sdhci_brcmstb_aquire_hwsem(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+	u32 wn = SDHCI_BRCMSTB_AGENT_LINUX;
+
+	sdhci_brcmstb_hwsem(host, HWSEM_AQUIRE);
+	/* indicate linux as next agent since it got the hw semaphore */
+	writel(wn, si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT);
+	dev_dbg(mmc_dev(host->mmc), "hwsem get\n");
+	sdhci_brcmstb_dbg_dump_sdio_share_regs(host);
+}
+
+static void sdhci_brcmstb_release_hwsem(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+	u32 wn;
+
+	if (!sdhci_brcmstb_cqe_is_idle(host))
+		return;
+
+	wn = readl(si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT);
+	/* release hw semaphore send IPI when done */
+	sdhci_brcmstb_hwsem(host, HWSEM_RELEASE);
+
+	if (wn == SDHCI_BRCMSTB_AGENT_TZOS)
+		writel(FLSHARE_IPIS0_INT_SEND_MASK, si->ipis0_reg);
+
+	dev_dbg(mmc_dev(host->mmc), "hwsem release\n");
+	sdhci_brcmstb_dbg_dump_sdio_share_regs(host);
+}
+
+static void sdhci_brcmstb_request_start(struct mmc_host *mmchost,
+					struct mmc_request *mrq)
+{
+	sdhci_brcmstb_aquire_hwsem(mmc_priv(mmchost));
+}
+
+static void sdhci_brcmstb_request_done(struct mmc_host *mmchost,
+				       struct mmc_request *mrq)
+{
+	struct sdhci_host *host = mmc_priv(mmchost);
+
+	if (!mmchost->ongoing_mrq)
+		sdhci_brcmstb_release_hwsem(host);
+}
+
 static u32 sdhci_brcmstb_cqhci_irq(struct sdhci_host *host, u32 intmask)
 {
 	int cmd_error = 0;
@@ -316,6 +509,81 @@ static u32 sdhci_brcmstb_cqhci_irq(struct sdhci_host *host, u32 intmask)
 	return 0;
 }
 
+static irqreturn_t sdhci_brcmstb_recv_ipi0_irq(int irq, void *dev_id)
+{
+	struct sdhci_host *host = dev_id;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si = priv->si;
+	u32 wn;
+
+	wn = readl(si->share_reg + BRCMSTB_SD_SHARE_REG_NEXT);
+	dev_dbg(mmc_dev(host->mmc), "ipi irq %d wn 0x%x\n", irq, wn);
+
+	if (wn == SDHCI_BRCMSTB_AGENT_LINUX)
+		wake_up(&si->wq);
+
+	return IRQ_HANDLED;
+}
+
+static int sdhci_brcmstb_sdio_share_init(struct platform_device *pdev)
+{
+	struct sdhci_host *host = dev_get_drvdata(&pdev->dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	struct brcmstb_sdio_share_info *si;
+	void __iomem *sdio_sh_regs;
+	int ret;
+
+	/* sdio_share block */
+	sdio_sh_regs = devm_platform_ioremap_resource_byname(pdev, "share");
+	if (IS_ERR(sdio_sh_regs))
+		return 0;
+
+	si = kcalloc(1, sizeof(struct brcmstb_sdio_share_info),
+		     GFP_KERNEL);
+	if (!si)
+		return -ENOMEM;
+
+	si->share_reg = sdio_sh_regs;
+	si->sem_reg = devm_platform_ioremap_resource_byname(pdev,
+							    "mmc_sem");
+	if (IS_ERR(si->sem_reg)) {
+		ret = PTR_ERR(si->sem_reg);
+		dev_err(&pdev->dev, "mmc_sem register\n");
+		goto err;
+	}
+
+	si->irq_recv = platform_get_irq_byname_optional(pdev, "recv_ipi0");
+	if (si->irq_recv < 0) {
+		ret = si->irq_recv;
+		dev_err(&pdev->dev, "recv_ipi0 IRQ not found\n");
+		goto err;
+	}
+
+	ret = devm_request_irq(&pdev->dev, si->irq_recv,
+			       sdhci_brcmstb_recv_ipi0_irq,
+			       0, "mmc_recv_ipi0", host);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "mmc_recv_ipi0 IRQ request_irq failed\n");
+		goto err;
+	}
+
+	si->ipis0_reg = devm_platform_ioremap_resource_byname(pdev, "flshr_ipis0");
+	if (IS_ERR(si->ipis0_reg))
+		goto err;
+
+	priv->si = si;
+	init_waitqueue_head(&si->wq);
+	host->mmc_host_ops.request_start = sdhci_brcmstb_request_start;
+	host->mmc_host_ops.request_done = sdhci_brcmstb_request_done;
+
+	return 0;
+err:
+	kfree(si);
+	return ret;
+}
+
 static int sdhci_brcmstb_add_host(struct sdhci_host *host,
 				  struct sdhci_brcmstb_priv *priv)
 {
@@ -482,8 +750,11 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 		goto err;
 
 	pltfm_host->clk = clk;
-	return res;
+	res  = sdhci_brcmstb_sdio_share_init(pdev);
+	if (res)
+		goto err;
 
+	return res;
 err:
 	sdhci_pltfm_free(pdev);
 	clk_disable_unprepare(base_clk);
-- 
2.17.1

Re: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Ulf Hansson]

+ Jens

On Thu, 6 Feb 2025 at 23:09, Kamal Dasu <kamal.dasu@broadcom.com> wrote:
>
> This patch set adds support for Broadcom TZOS to read and write to RPMB
> partition using synchronized access to the controller hardware.
> To achieve this Linux OS and the secure TZOS make use of:
> - shared hardware semaphore register
> - a set of SDIO shared work registers and
> - IPI interrupt registers
> The sdio shared work registers indicates next in queue to access the controller
> and current agent in the queue. The currently running OS that needs access to
> the controller puts itself in its slot of work register and if its next in line
> it can try to grab the hardware semaphore and complete its mmc requests.
> Next agent queue state is changed under the hardware semaphore lock before it
> release it by looking at work slot register. send and receive IPI interrupts
> between linux and secure world are used to indicatecompletion of transaction to
> the waiting OS. TZOS has its own RPMB driver which accesses partition when it
> wants to read/write RPMB frames. Current implementation assumes Linux and TZOS
> as the two work agents.

We recently added an in-kernel interface/subsystem for RPMB
(drivers/misc/rpmb-core.c). The optee driver (drivers/tee/*) uses it
ro read/write frames and route them for the secure OS.

When the mmc subsystem probes the eMMC card, it registers it as an
RPMB device via the new RPMB subsystem. In this way, it allows
consumers (as the optee driver) to read/write to/from it.

>
> Change required adding two core mmc_host_ops request_start() and request_done()
> to let the host controller driver know when a mmc request starts and ends so
> that the access can be synchronized. This has been tested with both the sdhci
> and cqhci access. Currently these ops are implemented by the sdhci-brcmstb
> controller dirver to acquire and release the hardware semaphore before and
> after access. This change to the mmc/core driver does not have any impact to
> existing controller drivers.

It seems to me that this isn't needed at all, assuming we have an
in-kernel tee driver that can route the RPMB frames, but maybe I don't
fully understand the use case.

I have looped in Jens Wiklander, allowing him to comment on this too.

Kind regards
Uffe

>
> Posting this path to get comments on the initial implementation.
>
> Todo :
> - Provide  hardware smeaphore using the harware spinlock driver framework
> - Use IPI send receive interrupt controller driver
>
> Kamal Dasu (3):
>   mmc: add request_start() and request_done() mmc ops
>   dt-bindings: mmc: brcm,sdhci-brcmstb: Add sdio sharing support
>   mmc: sdhci-brcmstb: Add rpmb sharing support in host driver
>
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      |  16 +-
>  drivers/mmc/core/core.c                       |  14 +-
>  drivers/mmc/host/sdhci-brcmstb.c              | 275 +++++++++++++++++-
>  include/linux/mmc/host.h                      |   4 +
>  4 files changed, 303 insertions(+), 6 deletions(-)
>
> --
> 2.17.1
>

Kind regards
Uffe

Re: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Florian Fainelli]

On 2/10/25 05:21, Ulf Hansson wrote:
> + Jens
> 
> On Thu, 6 Feb 2025 at 23:09, Kamal Dasu <kamal.dasu@broadcom.com> wrote:
>>
>> This patch set adds support for Broadcom TZOS to read and write to RPMB
>> partition using synchronized access to the controller hardware.
>> To achieve this Linux OS and the secure TZOS make use of:
>> - shared hardware semaphore register
>> - a set of SDIO shared work registers and
>> - IPI interrupt registers
>> The sdio shared work registers indicates next in queue to access the controller
>> and current agent in the queue. The currently running OS that needs access to
>> the controller puts itself in its slot of work register and if its next in line
>> it can try to grab the hardware semaphore and complete its mmc requests.
>> Next agent queue state is changed under the hardware semaphore lock before it
>> release it by looking at work slot register. send and receive IPI interrupts
>> between linux and secure world are used to indicatecompletion of transaction to
>> the waiting OS. TZOS has its own RPMB driver which accesses partition when it
>> wants to read/write RPMB frames. Current implementation assumes Linux and TZOS
>> as the two work agents.
> 
> We recently added an in-kernel interface/subsystem for RPMB
> (drivers/misc/rpmb-core.c). The optee driver (drivers/tee/*) uses it
> ro read/write frames and route them for the secure OS.
> 
> When the mmc subsystem probes the eMMC card, it registers it as an
> RPMB device via the new RPMB subsystem. In this way, it allows
> consumers (as the optee driver) to read/write to/from it.

Yes we are quite familiar with this subsystem and the many iterations 
that were proposed before it eventually landed upstream. At the time the 
hardware was designed, we were not sure of the direction that the 
generic RPMB subsystem would take so we decided to add the semaphore, 
scratch registers and interrupt generation capability so we would not be 
dependent upon such a subsystem. We also had other factors playing into 
designing it the way it is, such as allowing for N participants, 
including another processor/firmware.

> 
>>
>> Change required adding two core mmc_host_ops request_start() and request_done()
>> to let the host controller driver know when a mmc request starts and ends so
>> that the access can be synchronized. This has been tested with both the sdhci
>> and cqhci access. Currently these ops are implemented by the sdhci-brcmstb
>> controller dirver to acquire and release the hardware semaphore before and
>> after access. This change to the mmc/core driver does not have any impact to
>> existing controller drivers.
> 
> It seems to me that this isn't needed at all, assuming we have an
> in-kernel tee driver that can route the RPMB frames, but maybe I don't
> fully understand the use case.

The proposed scheme here scales to an arbitrary number of agents in the 
system. Our immediate use case is for both Linux and a Trusted OS (not 
OP-TEE based BTW) to share the eMMC controller, but we also accounted 
for a third agent which is a power management micro controller firmware 
to be able to participate in the scheme and occasionally make its own 
eMMC operations.
-- 
Florian

RE: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Avri Altman]

> >> This patch set adds support for Broadcom TZOS to read and write to
> >> RPMB partition using synchronized access to the controller hardware.
Practically it establishes a communication channel between the trust zone and the host controller regardless of the rpmb protocol.
Or did I get it wrong?

Thanks,
Avri

> >> To achieve this Linux OS and the secure TZOS make use of:
> >> - shared hardware semaphore register
> >> - a set of SDIO shared work registers and
> >> - IPI interrupt registers
> >> The sdio shared work registers indicates next in queue to access the
> >> controller and current agent in the queue. The currently running OS
> >> that needs access to the controller puts itself in its slot of work
> >> register and if its next in line it can try to grab the hardware semaphore and
> complete its mmc requests.
> >> Next agent queue state is changed under the hardware semaphore lock
> >> before it release it by looking at work slot register. send and
> >> receive IPI interrupts between linux and secure world are used to
> >> indicatecompletion of transaction to the waiting OS. TZOS has its own
> >> RPMB driver which accesses partition when it wants to read/write RPMB
> >> frames. Current implementation assumes Linux and TZOS as the two work
> agents.
> >
> > We recently added an in-kernel interface/subsystem for RPMB
> > (drivers/misc/rpmb-core.c). The optee driver (drivers/tee/*) uses it
> > ro read/write frames and route them for the secure OS.
> >
> > When the mmc subsystem probes the eMMC card, it registers it as an
> > RPMB device via the new RPMB subsystem. In this way, it allows
> > consumers (as the optee driver) to read/write to/from it.
> 
> Yes we are quite familiar with this subsystem and the many iterations that
> were proposed before it eventually landed upstream. At the time the
> hardware was designed, we were not sure of the direction that the generic
> RPMB subsystem would take so we decided to add the semaphore, scratch
> registers and interrupt generation capability so we would not be dependent
> upon such a subsystem. We also had other factors playing into designing it the
> way it is, such as allowing for N participants, including another
> processor/firmware.
> 
> >
> >>
> >> Change required adding two core mmc_host_ops request_start() and
> >> request_done() to let the host controller driver know when a mmc
> >> request starts and ends so that the access can be synchronized. This
> >> has been tested with both the sdhci and cqhci access. Currently these
> >> ops are implemented by the sdhci-brcmstb controller dirver to acquire
> >> and release the hardware semaphore before and after access. This
> >> change to the mmc/core driver does not have any impact to existing
> controller drivers.
> >
> > It seems to me that this isn't needed at all, assuming we have an
> > in-kernel tee driver that can route the RPMB frames, but maybe I don't
> > fully understand the use case.
> 
> The proposed scheme here scales to an arbitrary number of agents in the
> system. Our immediate use case is for both Linux and a Trusted OS (not OP-
> TEE based BTW) to share the eMMC controller, but we also accounted for a
> third agent which is a power management micro controller firmware to be able
> to participate in the scheme and occasionally make its own eMMC operations.
> --
> Florian

Re: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Florian Fainelli]

On 2/11/25 00:13, Avri Altman wrote:
>>>> This patch set adds support for Broadcom TZOS to read and write to
>>>> RPMB partition using synchronized access to the controller hardware.
> Practically it establishes a communication channel between the trust zone and the host controller regardless of the rpmb protocol.
> Or did I get it wrong?

Rather than communication channel, I would describe it as an arbitration 
scheme between N participants, with guaranteed forward progress and 
fairness between all participants.

The interest here is for one of those participants to own the eMMC 
controller for a certain amount of time and indicate when it is done 
with it. This is not specific to eMMC as this could scale to virtually 
any piece of HW that is driven by transactions from a CPU, but the main 
application is for allowing the Trusted OS to own the eMMC controller 
for a short period of time in order to do its RPMB access, and then give 
it back in the same state it found it to the next participant.
-- 
Florian

RE: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Avri Altman]

> On 2/11/25 00:13, Avri Altman wrote:
> >>>> This patch set adds support for Broadcom TZOS to read and write to
> >>>> RPMB partition using synchronized access to the controller hardware.
> > Practically it establishes a communication channel between the trust zone and
> the host controller regardless of the rpmb protocol.
> > Or did I get it wrong?
> 
> Rather than communication channel, I would describe it as an arbitration scheme
> between N participants, with guaranteed forward progress and fairness between
> all participants.
> 
> The interest here is for one of those participants to own the eMMC controller for
> a certain amount of time and indicate when it is done with it. This is not specific
> to eMMC as this could scale to virtually any piece of HW that is driven by
> transactions from a CPU, but the main application is for allowing the Trusted OS to
> own the eMMC controller for a short period of time in order to do its RPMB
> access, and then give it back in the same state it found it to the next participant.
Then maybe edit the subject accordingly and enclose this excellent explanation in the cover letter.
 
Thanks,
Avri

> --
> Florian

Re: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Ulf Hansson]

On Tue, 11 Feb 2025 at 18:01, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 2/11/25 00:13, Avri Altman wrote:
> >>>> This patch set adds support for Broadcom TZOS to read and write to
> >>>> RPMB partition using synchronized access to the controller hardware.
> > Practically it establishes a communication channel between the trust zone and the host controller regardless of the rpmb protocol.
> > Or did I get it wrong?
>
> Rather than communication channel, I would describe it as an arbitration
> scheme between N participants, with guaranteed forward progress and
> fairness between all participants.

A scheduler in the MMC controller HW?

Nevertheless, it means bypassing the I/O scheduler in Linux and the
mmc block layer, kind of breaking "fairness" from Linux point of view.

>
> The interest here is for one of those participants to own the eMMC
> controller for a certain amount of time and indicate when it is done
> with it. This is not specific to eMMC as this could scale to virtually
> any piece of HW that is driven by transactions from a CPU, but the main
> application is for allowing the Trusted OS to own the eMMC controller
> for a short period of time in order to do its RPMB access, and then give
> it back in the same state it found it to the next participant.

Honestly, I think this is a really terrible idea, sorry.

Data and communication with an eMMC needs to be synchronized and
managed by a single owner. Having multiple clients with their own
channel to the eMMC sounds prone to problems. Is each client going to
have its own mmc protocol stack, dealing with eMMC initialization,
data-synchronization and power-management, etc?

As I said, we now have the RPMB subsystem for in-kernel access. Please
use it instead.

Kind regards
Uffe

Re: [PATCH RFC 0/3] mmc: sdhci-brcmstb: Add rpmb sharing support

[Florian Fainelli]

On 3/12/25 06:17, Ulf Hansson wrote:
> On Tue, 11 Feb 2025 at 18:01, Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> On 2/11/25 00:13, Avri Altman wrote:
>>>>>> This patch set adds support for Broadcom TZOS to read and write to
>>>>>> RPMB partition using synchronized access to the controller hardware.
>>> Practically it establishes a communication channel between the trust zone and the host controller regardless of the rpmb protocol.
>>> Or did I get it wrong?
>>
>> Rather than communication channel, I would describe it as an arbitration
>> scheme between N participants, with guaranteed forward progress and
>> fairness between all participants.
> 
> A scheduler in the MMC controller HW?

There is no scheduler in the eMMC controller HW, all of the scheduling 
and coordination is left to the OS and TZOS, and other participants.

> 
> Nevertheless, it means bypassing the I/O scheduler in Linux and the
> mmc block layer, kind of breaking "fairness" from Linux point of view.

Yes that is a given, our approach favors the TZOS that has the ability 
to preempt for short periods of time the eMMC controller issue a RPMB 
access and then return control of the eMMC controller back to Linux. The 
very reason we came up with that scheme is that we are not comfortable 
with having a Linux task responsible for issuing RPMB accesses to the 
eMMC device on behalf of the TEE. That task is subject to the same Linux 
scheduling rules as any other task (yes we can play with priorities and 
classes) and our TEE team was not comfortable with that, they prefer 
hard guarantees that their RPMB accesses can complete within a certain 
time, which this scheme provides.

> 
>>
>> The interest here is for one of those participants to own the eMMC
>> controller for a certain amount of time and indicate when it is done
>> with it. This is not specific to eMMC as this could scale to virtually
>> any piece of HW that is driven by transactions from a CPU, but the main
>> application is for allowing the Trusted OS to own the eMMC controller
>> for a short period of time in order to do its RPMB access, and then give
>> it back in the same state it found it to the next participant.
> 
> Honestly, I think this is a really terrible idea, sorry.
 > > Data and communication with an eMMC needs to be synchronized and
> managed by a single owner. Having multiple clients with their own
> channel to the eMMC sounds prone to problems. Is each client going to
> have its own mmc protocol stack, dealing with eMMC initialization,
> data-synchronization and power-management, etc?

The synchronization is done around the start/end of transactions and yes 
each participant does have a minimal amount of eMMC driver knowledge, 
but is confined to doing RPMB accesses only. The contract is to put the 
eMMC controller back into the state where you found it for the next 
participant to make use of it.

When we operate with a single participant such as Linux, which is a 
degraded mode there is no loss of performance nor any observable difference.

> 
> As I said, we now have the RPMB subsystem for in-kernel access. Please
> use it instead.

That scheme works when all of the participants run on the same CPU, that 
is not our case, as we have another participant that is a separate CPU 
which you cannot factor into Linux's RPMB subsystem.

We considered doing a mediated/proxied eMMC access through a firmware 
interface running on a CPU that would exclusively own the hardware, but 
we really did not like losing access to mmc-utils and other things.
-- 
Florian