| Message ID | 1521019283-32212-1-git-send-email-sibis@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add support for remoteproc modem-pil on SDM845 SoCs | expand |
Hi Sibi, On 3/14/2018 2:51 PM, Sibi S wrote: > Add reset controller driver for Qualcomm SDM845 SoC to > control reset signals provided by AOSS for Modem, Venus > ADSP, GPU, Camera, Wireless, Display subsystem > > Signed-off-by: Sibi S <sibis@codeaurora.org> > --- > drivers/reset/Kconfig | 10 +++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-qcom-aoss.c | 156 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 167 insertions(+) > create mode 100644 drivers/reset/reset-qcom-aoss.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 7fc7769..d06bd1d 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -81,6 +81,16 @@ config RESET_PISTACHIO > help > This enables the reset driver for ImgTec Pistachio SoCs. > > +config RESET_QCOM_AOSS > + bool "Qcom AOSS Reset Driver" > + depends on ARCH_QCOM || COMPILE_TEST > + select MFD_SYSCON I think we don't need this after we moved away from syscon? > + help > + This enables the AOSS (always on subsystem) reset driver > + for Qualcomm SDM845 SoCs. Say Y if you want to control > + reset signals provided by AOSS for Modem, Venus, ADSP, > + GPU, Camera, Wireless, Display subsystem. Otherwise, say N. > + > config RESET_SIMPLE > bool "Simple Reset Controller Driver" if COMPILE_TEST > default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 132c24f..c30044a 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o > obj-$(CONFIG_RESET_MESON) += reset-meson.o > obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o > obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o > +obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o > obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o > obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o > obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o > diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c > new file mode 100644 > index 0000000..e70fb37 > --- /dev/null > +++ b/drivers/reset/reset-qcom-aoss.c > @@ -0,0 +1,156 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > +#include <linux/regmap.h> > +#include <linux/of_device.h> > +#include <dt-bindings/reset/qcom,sdm845-aoss.h> > + > +struct qcom_aoss_reset_map { > + unsigned int reg; > + u8 bit; > +}; > + > +struct qcom_aoss_desc { > + const struct regmap_config *config; > + const struct qcom_aoss_reset_map *resets; > + int delay; > + size_t num_resets; > +}; > + > +struct qcom_aoss_reset_data { > + struct reset_controller_dev rcdev; > + struct regmap *regmap; > + const struct qcom_aoss_desc *desc; > +}; > + > +static const struct regmap_config sdm845_aoss_regmap_config = { > + .name = "aoss-reset", > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = 0x20000, > + .fast_io = true, > +}; > + > +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = { > + [AOSS_CC_MSS_RESTART] = { 0x0, 0 }, > + [AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 }, > + [AOSS_CC_VENUS_RESTART] = { 0x2000, 0 }, > + [AOSS_CC_GPU_RESTART] = { 0x3000, 0 }, > + [AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 }, > + [AOSS_CC_WCSS_RESTART] = { 0x10000, 0 }, > + [AOSS_CC_LPASS_RESTART] = { 0x20000, 0 }, > +}; > + > +static const struct qcom_aoss_desc sdm845_aoss_desc = { > + .config = &sdm845_aoss_regmap_config, > + .resets = sdm845_aoss_resets, > + /* Wait 6 32kHz sleep cycles for reset */ > + .delay = 200, > + .num_resets = ARRAY_SIZE(sdm845_aoss_resets), > +}; > + > +static struct qcom_aoss_reset_data *to_qcom_aoss_reset_data( > + struct reset_controller_dev *rcdev) > +{ > + return container_of(rcdev, struct qcom_aoss_reset_data, rcdev); > +} Either a macro for such definition or 'inline'? > + > +static int qcom_aoss_control_assert(struct reset_controller_dev *rcdev, > + unsigned long idx) > +{ > + struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev); > + const struct qcom_aoss_reset_map *map = &data->desc->resets[idx]; > + > + return regmap_update_bits(data->regmap, map->reg, > + BIT(map->bit), BIT(map->bit)); > +} > + > +static int qcom_aoss_control_deassert(struct reset_controller_dev *rcdev, > + unsigned long idx) > +{ > + struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev); > + const struct qcom_aoss_reset_map *map = &data->desc->resets[idx]; > + > + return regmap_update_bits(data->regmap, map->reg, BIT(map->bit), 0); > +} > + > +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev, > + unsigned long idx) > +{ > + struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev); > + int ret; > + > + ret = qcom_aoss_control_assert(rcdev, idx); > + if (ret) > + return ret; > + > + udelay(data->desc->delay); > + > + return qcom_aoss_control_deassert(rcdev, idx); > +} > + > +static const struct reset_control_ops qcom_aoss_reset_ops = { > + .reset = qcom_aoss_control_reset, > + .assert = qcom_aoss_control_assert, > + .deassert = qcom_aoss_control_deassert, > +}; > + > +static int qcom_aoss_reset_probe(struct platform_device *pdev) > +{ > + struct qcom_aoss_reset_data *data; > + struct device *dev = &pdev->dev; > + const struct qcom_aoss_desc *desc; > + void __iomem *base; > + struct resource *res; > + > + desc = of_device_get_match_data(dev); > + if (!desc) > + return -EINVAL; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->desc = desc; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + data->regmap = devm_regmap_init_mmio(dev, base, desc->config); > + if (IS_ERR(data->regmap)) { > + dev_err(dev, "Unable to get aoss-reset regmap"); > + return PTR_ERR(data->regmap); > + } > + > + data->rcdev.owner = THIS_MODULE; > + data->rcdev.ops = &qcom_aoss_reset_ops; > + data->rcdev.nr_resets = desc->num_resets; > + data->rcdev.of_node = dev->of_node; > + > + return devm_reset_controller_register(dev, &data->rcdev); > +} > + > +static const struct of_device_id qcom_aoss_reset_of_match[] = { > + { .compatible = "qcom,sdm845-aoss-reset", .data = &sdm845_aoss_desc }, > + {} > +}; > + > +static struct platform_driver qcom_aoss_reset_driver = { > + .probe = qcom_aoss_reset_probe, > + .driver = { > + .name = "qcom_aoss_reset", > + .of_match_table = qcom_aoss_reset_of_match, > + }, > +}; > + > +builtin_platform_driver(qcom_aoss_reset_driver); > + > +MODULE_DESCRIPTION("Qualcomm AOSS Reset Driver"); > +MODULE_LICENSE("GPL v2"); Rest looks good to me. After taking above changes - Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> regards Vivek -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 7fc7769..d06bd1d 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -81,6 +81,16 @@ config RESET_PISTACHIO > help > This enables the reset driver for ImgTec Pistachio SoCs. > > +config RESET_QCOM_AOSS > + bool "Qcom AOSS Reset Driver" > + depends on ARCH_QCOM || COMPILE_TEST > + select MFD_SYSCON Drop syscon > + help > + This enables the AOSS (always on subsystem) reset driver > + for Qualcomm SDM845 SoCs. Say Y if you want to control > + reset signals provided by AOSS for Modem, Venus, ADSP, > + GPU, Camera, Wireless, Display subsystem. Otherwise, say N. > + [..] > diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c [..] > +static const struct regmap_config sdm845_aoss_regmap_config = { > + .name = "aoss-reset", > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = 0x20000, > + .fast_io = true, > +}; Is there a particular reason why you're setting up a regmap and not just operate on the ioremap region directly with readl/writel? It would save you a few lines of code and some runtime memory. > + > +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = { > + [AOSS_CC_MSS_RESTART] = { 0x0, 0 }, > + [AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 }, > + [AOSS_CC_VENUS_RESTART] = { 0x2000, 0 }, > + [AOSS_CC_GPU_RESTART] = { 0x3000, 0 }, > + [AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 }, > + [AOSS_CC_WCSS_RESTART] = { 0x10000, 0 }, > + [AOSS_CC_LPASS_RESTART] = { 0x20000, 0 }, Do you have a case where bit != 0? If not please drop the bit until it's necessary. > +}; > + > +static const struct qcom_aoss_desc sdm845_aoss_desc = { > + .config = &sdm845_aoss_regmap_config, > + .resets = sdm845_aoss_resets, > + /* Wait 6 32kHz sleep cycles for reset */ > + .delay = 200, Please move this constant into qcom_aoss_control_reset(), until there's a need for it to be configurable. > + .num_resets = ARRAY_SIZE(sdm845_aoss_resets), > +}; > + [..] > +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev, > + unsigned long idx) > +{ > + struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev); > + int ret; > + > + ret = qcom_aoss_control_assert(rcdev, idx); > + if (ret) > + return ret; > + > + udelay(data->desc->delay); Per Documentation/timers/timers-howto.txt please use usleep_range() when delays are between 10us and 20ms. > + > + return qcom_aoss_control_deassert(rcdev, idx); > +} Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: > Add the corresponding APCS offset for SDM845 SoC > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > Signed-off-by: Sibi S <sibis@codeaurora.org> > --- > drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > index 57bde0d..62d704d 100644 > --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c > +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > @@ -125,6 +125,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev) > static const struct of_device_id qcom_apcs_ipc_of_match[] = { > { .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 }, > { .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 }, > + { .compatible = "qcom,sdm845-apcs-hmss-global", .data = (void *)12 }, > {} > }; > MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c [..] > +struct q6v5_reset_ops { > + int (*reset_start)(struct q6v5 *qproc); > + int (*reset_stop)(struct q6v5 *qproc); > +}; Please add these two ops directly in q6v5 instead and please keep the naming "reset_assert" and "reset_deassert". > + > enum { > MSS_MSM8916, > MSS_MSM8974, > @@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > return 0; > } > > +static void alt_reset_restart(struct q6v5 *qproc, u32 restart) > +{ > + writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET); Just move this write into q6v5_sdm_reset_start() > +} > + > +static int q6v5_msm_reset_stop(struct q6v5 *qproc) > +{ > + return reset_control_assert(qproc->mss_restart); > +} > + > +static int q6v5_msm_reset_start(struct q6v5 *qproc) > +{ > + return reset_control_deassert(qproc->mss_restart); > +} > + > +static int q6v5_sdm_reset_stop(struct q6v5 *qproc) > +{ > + return reset_control_reset(qproc->mss_restart); > +} > + > +static int q6v5_sdm_reset_start(struct q6v5 *qproc) > +{ > + int ret; > + > + alt_reset_restart(qproc, 1); > + /* Ensure alt reset is written before restart reg */ That's not what udelay does ;) If you want to make sure that the register is written and then give it 100us delay until you reset the reset you have to readl() the same register back after the writel() I think the delay deserves a comment on what we're waiting for. > + udelay(100); Use usleep_range() for delays longer than 10us. > + > + ret = reset_control_reset(qproc->mss_restart); > + > + udelay(100); Same as the delay above, what are we waiting for? > + alt_reset_restart(qproc, 0); > + > + return ret; > +} > + [..] > @@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev) > qproc->dev = &pdev->dev; > qproc->rproc = rproc; > platform_set_drvdata(pdev, qproc); > + qproc->version = desc->version; > + > + if (qproc->version == MSS_SDM845) > + qproc->ops = &q6v5_sdm_ops; > + else > + qproc->ops = &q6v5_msm_ops; Can we carry a boolean for "has_alt_reset" or something that picks the new reset ops, rather than picking by version? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for the review On 03/19/2018 04:16 AM, Bjorn Andersson wrote: > On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c > [..] >> +struct q6v5_reset_ops { >> + int (*reset_start)(struct q6v5 *qproc); >> + int (*reset_stop)(struct q6v5 *qproc); >> +}; > > Please add these two ops directly in q6v5 instead and please keep the > naming "reset_assert" and "reset_deassert". > sure will do >> + >> enum { >> MSS_MSM8916, >> MSS_MSM8974, >> @@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> return 0; >> } >> >> +static void alt_reset_restart(struct q6v5 *qproc, u32 restart) >> +{ >> + writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET); > > Just move this write into q6v5_sdm_reset_start() > sure will do >> +} >> + >> +static int q6v5_msm_reset_stop(struct q6v5 *qproc) >> +{ >> + return reset_control_assert(qproc->mss_restart); >> +} >> + >> +static int q6v5_msm_reset_start(struct q6v5 *qproc) >> +{ >> + return reset_control_deassert(qproc->mss_restart); >> +} >> + >> +static int q6v5_sdm_reset_stop(struct q6v5 *qproc) >> +{ >> + return reset_control_reset(qproc->mss_restart); >> +} >> + >> +static int q6v5_sdm_reset_start(struct q6v5 *qproc) >> +{ >> + int ret; >> + >> + alt_reset_restart(qproc, 1); >> + /* Ensure alt reset is written before restart reg */ > > That's not what udelay does ;) > > If you want to make sure that the register is written and then give it > 100us delay until you reset the reset you have to readl() the same > register back after the writel() > > I think the delay deserves a comment on what we're waiting for. > the delay can be removed for the reasons stated below >> + udelay(100); > > Use usleep_range() for delays longer than 10us. > >> + >> + ret = reset_control_reset(qproc->mss_restart); >> + >> + udelay(100); > > Same as the delay above, what are we waiting for? > The point is to wait for 6 32khz sleep cycles both after assert and after de-assert of the aoss mss reset line. So moving the udelay to the aoss reset controller should have been right thing to do. alt_reset shouldn't need delays in between will remove them. >> + alt_reset_restart(qproc, 0); >> + >> + return ret; >> +} >> + > [..] >> @@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev) >> qproc->dev = &pdev->dev; >> qproc->rproc = rproc; >> platform_set_drvdata(pdev, qproc); >> + qproc->version = desc->version; >> + >> + if (qproc->version == MSS_SDM845) >> + qproc->ops = &q6v5_sdm_ops; >> + else >> + qproc->ops = &q6v5_msm_ops; > > Can we carry a boolean for "has_alt_reset" or something that picks the > new reset ops, rather than picking by version? > Though alt_reset is specific to SDM845 SoCs it also includes another reset controller (pdc_sync) in the modem bringup sequence, it isn't included it in the patch series since it doesn't seem to affect the modem start/stop functionality. Knowing that it may be added wouldn't version be a better choice here? > Regards, > Bjorn >
Hi Bjorn, Thanks for the review. On 03/19/2018 04:15 AM, Bjorn Andersson wrote: > On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 7fc7769..d06bd1d 100644 >> --- a/drivers/reset/Kconfig >> +++ b/drivers/reset/Kconfig >> @@ -81,6 +81,16 @@ config RESET_PISTACHIO >> help >> This enables the reset driver for ImgTec Pistachio SoCs. >> >> +config RESET_QCOM_AOSS >> + bool "Qcom AOSS Reset Driver" >> + depends on ARCH_QCOM || COMPILE_TEST >> + select MFD_SYSCON > > Drop syscon > will drop it >> + help >> + This enables the AOSS (always on subsystem) reset driver >> + for Qualcomm SDM845 SoCs. Say Y if you want to control >> + reset signals provided by AOSS for Modem, Venus, ADSP, >> + GPU, Camera, Wireless, Display subsystem. Otherwise, say N. >> + > [..] >> diff --git a/drivers/reset/reset-qcom-aoss.c b/drivers/reset/reset-qcom-aoss.c > [..] >> +static const struct regmap_config sdm845_aoss_regmap_config = { >> + .name = "aoss-reset", >> + .reg_bits = 32, >> + .reg_stride = 4, >> + .val_bits = 32, >> + .max_register = 0x20000, >> + .fast_io = true, >> +}; > > Is there a particular reason why you're setting up a regmap and not just > operate on the ioremap region directly with readl/writel? It would save > you a few lines of code and some runtime memory. > The idea here is to reuse the driver in modified configuration as pdc_sync restart controller which is to be used for adsp_pil. PDC sync reset register is a single register with multiple reset lines hence would warrant setting up a regmap. I can remove these changes and add them when I add pdc_sync reset controller though. >> + >> +static const struct qcom_aoss_reset_map sdm845_aoss_resets[] = { >> + [AOSS_CC_MSS_RESTART] = { 0x0, 0 }, >> + [AOSS_CC_CAMSS_RESTART] = { 0x1000, 0 }, >> + [AOSS_CC_VENUS_RESTART] = { 0x2000, 0 }, >> + [AOSS_CC_GPU_RESTART] = { 0x3000, 0 }, >> + [AOSS_CC_DISPSS_RESTART] = { 0x4000, 0 }, >> + [AOSS_CC_WCSS_RESTART] = { 0x10000, 0 }, >> + [AOSS_CC_LPASS_RESTART] = { 0x20000, 0 }, > > Do you have a case where bit != 0? If not please drop the bit until it's > necessary. > had the bit variable for the above state reason. >> +}; >> + >> +static const struct qcom_aoss_desc sdm845_aoss_desc = { >> + .config = &sdm845_aoss_regmap_config, >> + .resets = sdm845_aoss_resets, >> + /* Wait 6 32kHz sleep cycles for reset */ >> + .delay = 200, > > Please move this constant into qcom_aoss_control_reset(), until there's > a need for it to be configurable. > >> + .num_resets = ARRAY_SIZE(sdm845_aoss_resets), >> +}; >> + > [..] >> +static int qcom_aoss_control_reset(struct reset_controller_dev *rcdev, >> + unsigned long idx) >> +{ >> + struct qcom_aoss_reset_data *data = to_qcom_aoss_reset_data(rcdev); >> + int ret; >> + >> + ret = qcom_aoss_control_assert(rcdev, idx); >> + if (ret) >> + return ret; >> + >> + udelay(data->desc->delay); > > Per Documentation/timers/timers-howto.txt please use usleep_range() when > delays are between 10us and 20ms. > will replace them >> + >> + return qcom_aoss_control_deassert(rcdev, idx); >> +} > > Regards, > Bjorn >
On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote: > Add the corresponding APCS offset for SDM845 SoC > > Signed-off-by: Sibi S <sibis@codeaurora.org> Full name please. > --- > drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > index 57bde0d..62d704d 100644 > --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c > +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c > @@ -125,6 +125,7 @@ static int qcom_apcs_ipc_remove(struct platform_device *pdev) > static const struct of_device_id qcom_apcs_ipc_of_match[] = { > { .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 }, > { .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 }, > + { .compatible = "qcom,sdm845-apcs-hmss-global", .data = (void *)12 }, The block seems to be called "apss shared", so make the compatible "qcom,sdm845-apss-shared". Please resubmit this patch separate from the others in the series, as they can be merged independently of the rest of the series. PS. For updates like this you can generally submit the dt and code together. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html