2020-10-13 11:19:55

by Shawn Guo

[permalink] [raw]
Subject: [PATCH] mailbox: qcom-apcs-ipc: use PLATFORM_DEVID_AUTO to register device

On MSM8916, only one qcom-apcs-msm8916-clk device is needed, as there is
only one APCS clock. However, on MSM8939 three APCS clocks need to be
registered for cluster0 (little cores), cluster1 (big cores) and CCI
(Cache Coherent Interconnect). That said, we will need to register 3
qcom-apcs-msm8916-clk devices. Let's use PLATFORM_DEVID_AUTO rather
than PLATFORM_DEVID_NONE for platform_device_register_data() call.
Otherwise, the second APCS clock registration will fail due to duplicate
device name.

[ 0.519657] sysfs: cannot create duplicate filename '/bus/platform/devices/qcom-apcs-msm8916-clk'
...
[ 0.661158] qcom_apcs_ipc b111000.mailbox: failed to register APCS clk

Signed-off-by: Shawn Guo <[email protected]>
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 077e5c6a9ef7..3d100a004760 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -128,7 +128,7 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
if (apcs_data->clk_name) {
apcs->clk = platform_device_register_data(&pdev->dev,
apcs_data->clk_name,
- PLATFORM_DEVID_NONE,
+ PLATFORM_DEVID_AUTO,
NULL, 0);
if (IS_ERR(apcs->clk))
dev_err(&pdev->dev, "failed to register APCS clk\n");
--
2.17.1


2020-12-12 01:35:16

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH] mailbox: qcom-apcs-ipc: use PLATFORM_DEVID_AUTO to register device

On Mon 12 Oct 21:12 CDT 2020, Shawn Guo wrote:

> On MSM8916, only one qcom-apcs-msm8916-clk device is needed, as there is
> only one APCS clock. However, on MSM8939 three APCS clocks need to be
> registered for cluster0 (little cores), cluster1 (big cores) and CCI
> (Cache Coherent Interconnect). That said, we will need to register 3
> qcom-apcs-msm8916-clk devices. Let's use PLATFORM_DEVID_AUTO rather
> than PLATFORM_DEVID_NONE for platform_device_register_data() call.
> Otherwise, the second APCS clock registration will fail due to duplicate
> device name.
>
> [ 0.519657] sysfs: cannot create duplicate filename '/bus/platform/devices/qcom-apcs-msm8916-clk'
> ...
> [ 0.661158] qcom_apcs_ipc b111000.mailbox: failed to register APCS clk
>

Spotted this as I was looking for another patch, hence the late reply.
But it seems this was never reviewed or merged.

What does the bigger picture look like? Do we have 3 APCS global, or
it's a single memory region that provides the 3 clocks? How does the
qcom-apcs-msm8916-clk with it's hard coded offset of 0x50 deal with
this? (Is this a single qcom-apcs-msm8939-clk that registers 3 clocks
instead? Making this patch unnecessary?)

Regards,
Bjorn

> Signed-off-by: Shawn Guo <[email protected]>
> ---
> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index 077e5c6a9ef7..3d100a004760 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -128,7 +128,7 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
> if (apcs_data->clk_name) {
> apcs->clk = platform_device_register_data(&pdev->dev,
> apcs_data->clk_name,
> - PLATFORM_DEVID_NONE,
> + PLATFORM_DEVID_AUTO,
> NULL, 0);
> if (IS_ERR(apcs->clk))
> dev_err(&pdev->dev, "failed to register APCS clk\n");
> --
> 2.17.1
>

2020-12-12 16:11:23

by Shawn Guo

[permalink] [raw]
Subject: Re: [PATCH] mailbox: qcom-apcs-ipc: use PLATFORM_DEVID_AUTO to register device

On Fri, Dec 11, 2020 at 12:16 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Mon 12 Oct 21:12 CDT 2020, Shawn Guo wrote:
>
> > On MSM8916, only one qcom-apcs-msm8916-clk device is needed, as there is
> > only one APCS clock. However, on MSM8939 three APCS clocks need to be
> > registered for cluster0 (little cores), cluster1 (big cores) and CCI
> > (Cache Coherent Interconnect). That said, we will need to register 3
> > qcom-apcs-msm8916-clk devices. Let's use PLATFORM_DEVID_AUTO rather
> > than PLATFORM_DEVID_NONE for platform_device_register_data() call.
> > Otherwise, the second APCS clock registration will fail due to duplicate
> > device name.
> >
> > [ 0.519657] sysfs: cannot create duplicate filename '/bus/platform/devices/qcom-apcs-msm8916-clk'
> > ...
> > [ 0.661158] qcom_apcs_ipc b111000.mailbox: failed to register APCS clk
> >
>
> Spotted this as I was looking for another patch, hence the late reply.
> But it seems this was never reviewed or merged.
>
> What does the bigger picture look like? Do we have 3 APCS global, or
> it's a single memory region that provides the 3 clocks? How does the
> qcom-apcs-msm8916-clk with it's hard coded offset of 0x50 deal with
> this? (Is this a single qcom-apcs-msm8939-clk that registers 3 clocks
> instead? Making this patch unnecessary?)

We have 3 APCS global instances. I haven't got time to send out other
related patches, but the DT for these 3 APCS global looks like below.

apcs2: mailbox@b1d1000 {
compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
reg = <0xb1d1000 0x1000>;
#mbox-cells = <1>;
clocks = <&a53pll_cci>, <&gcc GPLL0_VOTE>;
clock-names = "pll", "aux";
#clock-cells = <0>;
clock-output-names = "a53mux_cci";
};

apcs1: mailbox@b011000 {
compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
reg = <0xb011000 0x1000>;
#mbox-cells = <1>;
clocks = <&a53pll_c1>, <&gcc GPLL0_VOTE>;
clock-names = "pll", "aux";
#clock-cells = <0>;
clock-output-names = "a53mux_c1";
};

apcs0: mailbox@b111000 {
compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
reg = <0xb111000 0x1000>;
#mbox-cells = <1>;
clocks = <&a53pll_c0>, <&gcc GPLL0_VOTE>;
clock-names = "pll", "aux";
#clock-cells = <0>;
clock-output-names = "a53mux_c0";
};

Shawn