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

Message ID 20171205154701.27730-3-georgi.djakov@linaro.org
State Accepted
Commit c815d769b598196bdbd104a7e049d07ae6fba0d2
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
Georgi Djakov Feb. 1, 2018, 8:01 a.m. | #2
On 02/01/2018 08:57 AM, Jassi Brar wrote:
> On Thu, Feb 1, 2018 at 12:10 AM, Georgi Djakov <georgi.djakov@linaro.org> wrote:

>>

>> Hi Jassi,

>>

>> On 01/27/2018 05:44 AM, Jassi Brar wrote:

>>> On Thu, Jan 4, 2018 at 10:26 PM, Georgi Djakov <georgi.djakov@linaro.org> wrote:

>>>> Hi Jassi,

>>>>

>>>> On 12/29/2017 08:14 AM, Jassi Brar wrote:

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

>>>>

>>>> The APCS is one separate register block related to the CPU cluster. I

>>>> haven't seen any strict guidelines for such cases in the DT docs, and

>>>> during the discussion got the impression that this is the preferred

>>>> binding. Rob has also reviewed the binding, so we should be fine to move

>>>> forward with this one.

>>>>

>>> Well, I can't overrule Rob. But I am really not happy with random

>>> device spawning from mailbox drivers. I know there are such instances

>>> already in the kernel but that doesn't make it legit... unless there

>>> is some hard dependency. Is there?

>>

>> The dependency is that on this SoC, these functionalities are combined

>> into this "CPU subsystem" block.

>>

> I see the register space is shared between mailbox and the clock. So I

> guess, yes, simply creating a device here and passing the common

> regmap is tidier. Which patches are already picked up?


Patches 3, 4 and 6 are already picked into the clk tree. Still pending
are patches 1, 2 and 5.

Thanks,
Georgi
--
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;
 }