[v11,2/6] mailbox: qcom: Create APCS child device for clock controller

Message ID 20171205154701.27730-3-georgi.djakov@linaro.org
State New
Headers show
Series
  • Add support for Qualcomm A53 CPU clock
Related show

Commit Message

Georgi Djakov Dec. 5, 2017, 3:46 p.m.
There is a clock controller functionality provided by the APCS hardware
block of msm8916 devices. The device-tree would represent an APCS node
with both mailbox and clock provider properties.
Create a platform child device for the clock controller functionality so
the driver can probe and use APCS as parent.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jassi Brar Dec. 29, 2017, 6:14 a.m. | #1
Hi Bjorn,

On Sun, Dec 24, 2017 at 10:36 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Fri 22 Dec 20:57 PST 2017, Jassi Brar wrote:

>

>> On Tue, Dec 5, 2017 at 9:16 PM, Georgi Djakov <georgi.djakov@linaro.org> wrote:

>> > There is a clock controller functionality provided by the APCS hardware

>> > block of msm8916 devices. The device-tree would represent an APCS node

>> > with both mailbox and clock provider properties.

>> >

>> The spec might depict a 'clock' box and 'mailbox' box inside the

>> bigger APCS box. However, from the code I see in this patchset, they

>> are orthogonal and can & should be represented as independent DT

>> nodes.

>

> The APCS consists of a number of different hardware blocks, one of them

> being the "APCS global" block, which is what this node and drivers

> relate to. On 8916 this contains both the IPC register and clock

> control. But it's still just one block according to the hardware

> specification.

>

> As such DT should describe the one hardware block by one node IMHO.

>

In my even humbler opinion, DT should describe a h/w functional unit
which _could_ be seen as a standalone component.
For example, if this APCS had a mac controller, would we also populate
a netdev from mailbox driver? And what if next revision moves/drops
this clock controller out of APCS, keeping mailbox controller exactly
same?

Maybe some DT maintainer could enlighten either of us.

Cheers!
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index ab344bc6fa63..57bde0dfd12f 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -29,6 +29,7 @@  struct qcom_apcs_ipc {
 
 	struct regmap *regmap;
 	unsigned long offset;
+	struct platform_device *clk;
 };
 
 static const struct regmap_config apcs_regmap_config = {
@@ -96,6 +97,14 @@  static int qcom_apcs_ipc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
+		apcs->clk = platform_device_register_data(&pdev->dev,
+							  "qcom-apcs-msm8916-clk",
+							  -1, NULL, 0);
+		if (IS_ERR(apcs->clk))
+			dev_err(&pdev->dev, "failed to register APCS clk\n");
+	}
+
 	platform_set_drvdata(pdev, apcs);
 
 	return 0;
@@ -104,8 +113,10 @@  static int qcom_apcs_ipc_probe(struct platform_device *pdev)
 static int qcom_apcs_ipc_remove(struct platform_device *pdev)
 {
 	struct qcom_apcs_ipc *apcs = platform_get_drvdata(pdev);
+	struct platform_device *clk = apcs->clk;
 
 	mbox_controller_unregister(&apcs->mbox);
+	platform_device_unregister(clk);
 
 	return 0;
 }