| Message ID | 20201013021241.5656-1-shawn.guo@linaro.org |
|---|---|
| State | Accepted |
| Commit | 96e39e95c01283ff5695dafe659df88ada802159 |
| Headers | show |
| Series | mailbox: qcom-apcs-ipc: use PLATFORM_DEVID_AUTO to register device | expand |
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 <shawn.guo@linaro.org> > --- > 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 >
On Fri, Dec 11, 2020 at 12:16 PM Bjorn Andersson <bjorn.andersson@linaro.org> 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
On Fri 11 Dec 08:12 CST 2020, Shawn Guo wrote: > On Fri, Dec 11, 2020 at 12:16 PM Bjorn Andersson > <bjorn.andersson@linaro.org> 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>; The thing at 0xb1d1000 does not have the IPC bits, so this shouldn't have the #mbox-cells. Other than that, this seems to be correct. Please do add the msm8939 compatible as you send this out. Regards, Bjorn > 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
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 > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > 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 >
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");
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 <shawn.guo@linaro.org> --- drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)