| Message ID | 20171205154701.27730-3-georgi.djakov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. Patch-1 is ok, though. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. The fact that we in Linux split this into two different drivers is an unrelated implementation detail. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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? The clock controller may change in some next SoC architecture and that's why the SoC version is also part of the the compatible string. Thanks, Georgi -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? >> 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? > > The clock controller may change in some next SoC architecture and that's > why the SoC version is also part of the the compatible string. > So the mailbox driver will be updated to spawn yet another type of clock? And again for next revision and so on... I know that is unlikely but the point is why not have separate clock drivers for independent h/w clocks? I'll let Rob take the final call. Cheers! -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Initially APCS was a syscon and it was discussed it an year ago in this mail thread [1] and we are trying to move away from the syscon and come up with some bindings. During v8 and v9 of this patchset i have switched from multiple child nodes to a single node as this seem to be the preferred form. >>> 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? >> >> The clock controller may change in some next SoC architecture and that's >> why the SoC version is also part of the the compatible string. >> > So the mailbox driver will be updated to spawn yet another type of clock? > And again for next revision and so on... I know that is unlikely but > the point is why not have separate clock drivers for independent h/w > clocks? Each revision re-shuffles the APCS block, so at least the offsets would be different. I don't see anything bad with spawning a child device, but what are the other options? Should we go back on the bindings and suggest something new all over again? Or do you have any other idea? Could you please explain your point further? Thanks, Georgi [1] https://lkml.org/lkml/2017/3/20/991 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Cheers! -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }