| Message ID | 1624564058-24095-1-git-send-email-sibis@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add Modem support on SC7280 SoCs | expand |
Hi Sibi, On Fri, Jun 25, 2021 at 01:17:34AM +0530, Sibi Sankar wrote: > Add out of reset sequence support for modem sub-system on SC7280 SoCs. > It requires access to an additional set of qaccept registers, external > power/clk control registers and halt vq6 register to put the modem back > into reset. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 245 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 241 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 5d21084004cb..4e32811e0025 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -77,6 +77,14 @@ > > #define HALT_ACK_TIMEOUT_US 100000 > > +/* QACCEPT Register Offsets */ > +#define QACCEPT_ACCEPT_REG 0x0 > +#define QACCEPT_ACTIVE_REG 0x4 > +#define QACCEPT_DENY_REG 0x8 > +#define QACCEPT_REQ_REG 0xC > + > +#define QACCEPT_TIMEOUT_US 50 > + > /* QDSP6SS_RESET */ > #define Q6SS_STOP_CORE BIT(0) > #define Q6SS_CORE_ARES BIT(1) > @@ -143,6 +151,9 @@ struct rproc_hexagon_res { > bool has_alt_reset; > bool has_mba_logs; > bool has_spare_reg; > + bool has_qaccept_regs; > + bool has_ext_cntl_regs; > + bool has_vq6; > }; > > struct q6v5 { > @@ -158,8 +169,18 @@ struct q6v5 { > u32 halt_q6; > u32 halt_modem; > u32 halt_nc; > + u32 halt_vq6; > u32 conn_box; > > + u32 qaccept_mdm; > + u32 qaccept_cx; > + u32 qaccept_axi; > + > + u32 axim1_clk_off; > + u32 crypto_clk_off; > + u32 force_clk_on; > + u32 rscc_disable; > + > struct reset_control *mss_restart; > struct reset_control *pdc_reset; > > @@ -201,6 +222,9 @@ struct q6v5 { > bool has_alt_reset; > bool has_mba_logs; > bool has_spare_reg; > + bool has_qaccept_regs; > + bool has_ext_cntl_regs; > + bool has_vq6; > int mpss_perm; > int mba_perm; > const char *hexagon_mdt_image; > @@ -213,6 +237,7 @@ enum { > MSS_MSM8996, > MSS_MSM8998, > MSS_SC7180, > + MSS_SC7280, > MSS_SDM845, > }; > > @@ -473,6 +498,12 @@ static int q6v5_reset_assert(struct q6v5 *qproc) > regmap_update_bits(qproc->conn_map, qproc->conn_box, > AXI_GATING_VALID_OVERRIDE, 0); > ret = reset_control_deassert(qproc->mss_restart); > + } else if (qproc->has_ext_cntl_regs) { > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); > + reset_control_assert(qproc->pdc_reset); > + reset_control_assert(qproc->mss_restart); > + reset_control_deassert(qproc->pdc_reset); > + ret = reset_control_deassert(qproc->mss_restart); > } else { > ret = reset_control_assert(qproc->mss_restart); > } > @@ -490,7 +521,7 @@ static int q6v5_reset_deassert(struct q6v5 *qproc) > ret = reset_control_reset(qproc->mss_restart); > writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); > reset_control_deassert(qproc->pdc_reset); > - } else if (qproc->has_spare_reg) { > + } else if (qproc->has_spare_reg || qproc->has_ext_cntl_regs) { > ret = reset_control_reset(qproc->mss_restart); > } else { > ret = reset_control_deassert(qproc->mss_restart); > @@ -604,7 +635,7 @@ static int q6v5proc_reset(struct q6v5 *qproc) > } > > goto pbl_wait; > - } else if (qproc->version == MSS_SC7180) { > + } else if (qproc->version == MSS_SC7180 || qproc->version == MSS_SC7280) { > val = readl(qproc->reg_base + QDSP6SS_SLEEP); > val |= Q6SS_CBCR_CLKEN; > writel(val, qproc->reg_base + QDSP6SS_SLEEP); > @@ -787,6 +818,82 @@ static int q6v5proc_reset(struct q6v5 *qproc) > return ret; > } > > +static int q6v5proc_enable_qchannel(struct q6v5 *qproc, struct regmap *map, u32 offset) > +{ > + unsigned int val; > + int ret; > + > + if (!qproc->has_qaccept_regs) > + return 0; > + > + if (qproc->has_ext_cntl_regs) { > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); > + regmap_write(qproc->conn_map, qproc->force_clk_on, 1); > + > + ret = regmap_read_poll_timeout(qproc->halt_map, qproc->axim1_clk_off, val, > + !val, 1, Q6SS_CBCR_TIMEOUT_US); > + if (ret) { > + dev_err(qproc->dev, "failed to enable axim1 clock\n"); > + return -ETIMEDOUT; > + } > + } > + > + regmap_write(map, offset + QACCEPT_REQ_REG, 1); > + > + /* Wait for accept */ > + ret = regmap_read_poll_timeout(map, offset + QACCEPT_ACCEPT_REG, val, val, 5, > + QACCEPT_TIMEOUT_US); > + if (ret) { > + dev_err(qproc->dev, "qchannel enable failed\n"); > + return -ETIMEDOUT; > + } > + > + return 0; > +} > + > +static void q6v5proc_disable_qchannel(struct q6v5 *qproc, struct regmap *map, u32 offset) > +{ > + int ret; > + unsigned int val, retry; > + unsigned int nretry = 10; > + bool takedown_complete = false; > + > + if (!qproc->has_qaccept_regs) > + return; > + > + while (!takedown_complete && nretry) { > + nretry--; > + > + regmap_read_poll_timeout(map, offset + QACCEPT_ACTIVE_REG, val, !val, 5, > + QACCEPT_TIMEOUT_US); > + > + regmap_write(map, offset + QACCEPT_REQ_REG, 0); > + > + retry = 10; > + while (retry) { > + usleep_range(5, 10); > + retry--; > + ret = regmap_read(map, offset + QACCEPT_DENY_REG, &val); > + if (!ret && val) { > + regmap_write(map, offset + QACCEPT_REQ_REG, 1); > + break; > + } > + > + ret = regmap_read(map, offset + QACCEPT_ACCEPT_REG, &val); > + if (!ret && !val) { > + takedown_complete = true; > + break; > + } A bit of commentary in this branch would do no harm. From the code flow I can guess that disabling the channel failed when QACCEPT_DENY_REG != 0, and hence the channel is re-enabled (?) for the next try, and apparently things are fine when QACCEPT_ACCEPT_REG is 0 after disabling the channel. Would be good to be a bit more explicit about what all that actually means. > + } > + > + if (!retry) > + break; > + } > + > + if (!takedown_complete) > + dev_err(qproc->dev, "qchannel takedown failed\n"); > +}
On 2021-06-25 01:17, Sibi Sankar wrote: > The SID configuration requirement for Modem on SC7280 is similar to the > ones found on SC7180/SDM845 SoCs. So, add the SC7280 modem compatible > to > the list to defer the programming of the modem SIDs to the kernel. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index 7771d40176de..90d471a387bf 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -179,6 +179,7 @@ static const struct of_device_id > qcom_smmu_client_of_match[] __maybe_unused = { > { .compatible = "qcom,sc7180-mdss" }, > { .compatible = "qcom,sc7180-mss-pil" }, > { .compatible = "qcom,sc7280-mdss" }, > + { .compatible = "qcom,sc7280-mss-pil" }, > { .compatible = "qcom,sc8180x-mdss" }, > { .compatible = "qcom,sdm845-mdss" }, > { .compatible = "qcom,sdm845-mss-pil" }, Reviewed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Hey Matthias, Thanks for taking time to review the patch series. On 2021-06-25 06:05, Matthias Kaehlcke wrote: > Hi Sibi, > > On Fri, Jun 25, 2021 at 01:17:34AM +0530, Sibi Sankar wrote: >> Add out of reset sequence support for modem sub-system on SC7280 SoCs. >> It requires access to an additional set of qaccept registers, external >> power/clk control registers and halt vq6 register to put the modem >> back >> into reset. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_mss.c | 245 >> ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 241 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c >> b/drivers/remoteproc/qcom_q6v5_mss.c >> index 5d21084004cb..4e32811e0025 100644 >> --- a/drivers/remoteproc/qcom_q6v5_mss.c >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c >> @@ -77,6 +77,14 @@ >> >> #define HALT_ACK_TIMEOUT_US 100000 >> >> +/* QACCEPT Register Offsets */ >> +#define QACCEPT_ACCEPT_REG 0x0 >> +#define QACCEPT_ACTIVE_REG 0x4 >> +#define QACCEPT_DENY_REG 0x8 >> +#define QACCEPT_REQ_REG 0xC >> + >> +#define QACCEPT_TIMEOUT_US 50 >> + >> /* QDSP6SS_RESET */ >> #define Q6SS_STOP_CORE BIT(0) >> #define Q6SS_CORE_ARES BIT(1) >> @@ -143,6 +151,9 @@ struct rproc_hexagon_res { >> bool has_alt_reset; >> bool has_mba_logs; >> bool has_spare_reg; >> + bool has_qaccept_regs; >> + bool has_ext_cntl_regs; >> + bool has_vq6; >> }; >> >> struct q6v5 { >> @@ -158,8 +169,18 @@ struct q6v5 { >> u32 halt_q6; >> u32 halt_modem; >> u32 halt_nc; >> + u32 halt_vq6; >> u32 conn_box; >> >> + u32 qaccept_mdm; >> + u32 qaccept_cx; >> + u32 qaccept_axi; >> + >> + u32 axim1_clk_off; >> + u32 crypto_clk_off; >> + u32 force_clk_on; >> + u32 rscc_disable; >> + >> struct reset_control *mss_restart; >> struct reset_control *pdc_reset; >> >> @@ -201,6 +222,9 @@ struct q6v5 { >> bool has_alt_reset; >> bool has_mba_logs; >> bool has_spare_reg; >> + bool has_qaccept_regs; >> + bool has_ext_cntl_regs; >> + bool has_vq6; >> int mpss_perm; >> int mba_perm; >> const char *hexagon_mdt_image; >> @@ -213,6 +237,7 @@ enum { >> MSS_MSM8996, >> MSS_MSM8998, >> MSS_SC7180, >> + MSS_SC7280, >> MSS_SDM845, >> }; >> >> @@ -473,6 +498,12 @@ static int q6v5_reset_assert(struct q6v5 *qproc) >> regmap_update_bits(qproc->conn_map, qproc->conn_box, >> AXI_GATING_VALID_OVERRIDE, 0); >> ret = reset_control_deassert(qproc->mss_restart); >> + } else if (qproc->has_ext_cntl_regs) { >> + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); >> + reset_control_assert(qproc->pdc_reset); >> + reset_control_assert(qproc->mss_restart); >> + reset_control_deassert(qproc->pdc_reset); >> + ret = reset_control_deassert(qproc->mss_restart); >> } else { >> ret = reset_control_assert(qproc->mss_restart); >> } >> @@ -490,7 +521,7 @@ static int q6v5_reset_deassert(struct q6v5 *qproc) >> ret = reset_control_reset(qproc->mss_restart); >> writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); >> reset_control_deassert(qproc->pdc_reset); >> - } else if (qproc->has_spare_reg) { >> + } else if (qproc->has_spare_reg || qproc->has_ext_cntl_regs) { >> ret = reset_control_reset(qproc->mss_restart); >> } else { >> ret = reset_control_deassert(qproc->mss_restart); >> @@ -604,7 +635,7 @@ static int q6v5proc_reset(struct q6v5 *qproc) >> } >> >> goto pbl_wait; >> - } else if (qproc->version == MSS_SC7180) { >> + } else if (qproc->version == MSS_SC7180 || qproc->version == >> MSS_SC7280) { >> val = readl(qproc->reg_base + QDSP6SS_SLEEP); >> val |= Q6SS_CBCR_CLKEN; >> writel(val, qproc->reg_base + QDSP6SS_SLEEP); >> @@ -787,6 +818,82 @@ static int q6v5proc_reset(struct q6v5 *qproc) >> return ret; >> } >> >> +static int q6v5proc_enable_qchannel(struct q6v5 *qproc, struct regmap >> *map, u32 offset) >> +{ >> + unsigned int val; >> + int ret; >> + >> + if (!qproc->has_qaccept_regs) >> + return 0; >> + >> + if (qproc->has_ext_cntl_regs) { >> + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); >> + regmap_write(qproc->conn_map, qproc->force_clk_on, 1); >> + >> + ret = regmap_read_poll_timeout(qproc->halt_map, >> qproc->axim1_clk_off, val, >> + !val, 1, Q6SS_CBCR_TIMEOUT_US); >> + if (ret) { >> + dev_err(qproc->dev, "failed to enable axim1 clock\n"); >> + return -ETIMEDOUT; >> + } >> + } >> + >> + regmap_write(map, offset + QACCEPT_REQ_REG, 1); >> + >> + /* Wait for accept */ >> + ret = regmap_read_poll_timeout(map, offset + QACCEPT_ACCEPT_REG, >> val, val, 5, >> + QACCEPT_TIMEOUT_US); >> + if (ret) { >> + dev_err(qproc->dev, "qchannel enable failed\n"); >> + return -ETIMEDOUT; >> + } >> + >> + return 0; >> +} >> + >> +static void q6v5proc_disable_qchannel(struct q6v5 *qproc, struct >> regmap *map, u32 offset) >> +{ >> + int ret; >> + unsigned int val, retry; >> + unsigned int nretry = 10; >> + bool takedown_complete = false; >> + >> + if (!qproc->has_qaccept_regs) >> + return; >> + >> + while (!takedown_complete && nretry) { >> + nretry--; >> + >> + regmap_read_poll_timeout(map, offset + QACCEPT_ACTIVE_REG, val, >> !val, 5, >> + QACCEPT_TIMEOUT_US); >> + >> + regmap_write(map, offset + QACCEPT_REQ_REG, 0); Sure I'll add more comments to this func. After lowering the request ^^ we wait for deny to go high or accept to go low. If it's the former then we do a request high and repeat the entire process again. If it's the latter then its considered that the takedown is success. Let me know if you feel any other parts of the patch requires more comments as well. >> + >> + retry = 10; >> + while (retry) { >> + usleep_range(5, 10); >> + retry--; >> + ret = regmap_read(map, offset + QACCEPT_DENY_REG, &val); >> + if (!ret && val) { >> + regmap_write(map, offset + QACCEPT_REQ_REG, 1); >> + break; >> + } >> + >> + ret = regmap_read(map, offset + QACCEPT_ACCEPT_REG, &val); >> + if (!ret && !val) { >> + takedown_complete = true; >> + break; >> + } > > A bit of commentary in this branch would do no harm. From the code flow > I can guess that disabling the channel failed when QACCEPT_DENY_REG != > 0, > and hence the channel is re-enabled (?) for the next try, and > apparently > things are fine when QACCEPT_ACCEPT_REG is 0 after disabling the > channel. > Would be good to be a bit more explicit about what all that actually > means. > >> + } >> + >> + if (!retry) >> + break; >> + } >> + >> + if (!takedown_complete) >> + dev_err(qproc->dev, "qchannel takedown failed\n"); >> +}
Hi Sibi, On Fri, Jun 25, 2021 at 07:51:38PM +0530, Sibi Sankar wrote: > Hey Matthias, > Thanks for taking time to review the patch > series. > > On 2021-06-25 06:05, Matthias Kaehlcke wrote: > > Hi Sibi, > > > > On Fri, Jun 25, 2021 at 01:17:34AM +0530, Sibi Sankar wrote: > > > Add out of reset sequence support for modem sub-system on SC7280 SoCs. > > > It requires access to an additional set of qaccept registers, external > > > power/clk control registers and halt vq6 register to put the modem > > > back > > > into reset. > > > > > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > > > --- > > > drivers/remoteproc/qcom_q6v5_mss.c | 245 > > > ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 241 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c > > > b/drivers/remoteproc/qcom_q6v5_mss.c > > > index 5d21084004cb..4e32811e0025 100644 > > > --- a/drivers/remoteproc/qcom_q6v5_mss.c > > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > > > @@ -77,6 +77,14 @@ > > > > > > #define HALT_ACK_TIMEOUT_US 100000 > > > > > > +/* QACCEPT Register Offsets */ > > > +#define QACCEPT_ACCEPT_REG 0x0 > > > +#define QACCEPT_ACTIVE_REG 0x4 > > > +#define QACCEPT_DENY_REG 0x8 > > > +#define QACCEPT_REQ_REG 0xC > > > + > > > +#define QACCEPT_TIMEOUT_US 50 > > > + > > > /* QDSP6SS_RESET */ > > > #define Q6SS_STOP_CORE BIT(0) > > > #define Q6SS_CORE_ARES BIT(1) > > > @@ -143,6 +151,9 @@ struct rproc_hexagon_res { > > > bool has_alt_reset; > > > bool has_mba_logs; > > > bool has_spare_reg; > > > + bool has_qaccept_regs; > > > + bool has_ext_cntl_regs; > > > + bool has_vq6; > > > }; > > > > > > struct q6v5 { > > > @@ -158,8 +169,18 @@ struct q6v5 { > > > u32 halt_q6; > > > u32 halt_modem; > > > u32 halt_nc; > > > + u32 halt_vq6; > > > u32 conn_box; > > > > > > + u32 qaccept_mdm; > > > + u32 qaccept_cx; > > > + u32 qaccept_axi; > > > + > > > + u32 axim1_clk_off; > > > + u32 crypto_clk_off; > > > + u32 force_clk_on; > > > + u32 rscc_disable; > > > + > > > struct reset_control *mss_restart; > > > struct reset_control *pdc_reset; > > > > > > @@ -201,6 +222,9 @@ struct q6v5 { > > > bool has_alt_reset; > > > bool has_mba_logs; > > > bool has_spare_reg; > > > + bool has_qaccept_regs; > > > + bool has_ext_cntl_regs; > > > + bool has_vq6; > > > int mpss_perm; > > > int mba_perm; > > > const char *hexagon_mdt_image; > > > @@ -213,6 +237,7 @@ enum { > > > MSS_MSM8996, > > > MSS_MSM8998, > > > MSS_SC7180, > > > + MSS_SC7280, > > > MSS_SDM845, > > > }; > > > > > > @@ -473,6 +498,12 @@ static int q6v5_reset_assert(struct q6v5 *qproc) > > > regmap_update_bits(qproc->conn_map, qproc->conn_box, > > > AXI_GATING_VALID_OVERRIDE, 0); > > > ret = reset_control_deassert(qproc->mss_restart); > > > + } else if (qproc->has_ext_cntl_regs) { > > > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); > > > + reset_control_assert(qproc->pdc_reset); > > > + reset_control_assert(qproc->mss_restart); > > > + reset_control_deassert(qproc->pdc_reset); > > > + ret = reset_control_deassert(qproc->mss_restart); > > > } else { > > > ret = reset_control_assert(qproc->mss_restart); > > > } > > > @@ -490,7 +521,7 @@ static int q6v5_reset_deassert(struct q6v5 *qproc) > > > ret = reset_control_reset(qproc->mss_restart); > > > writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET); > > > reset_control_deassert(qproc->pdc_reset); > > > - } else if (qproc->has_spare_reg) { > > > + } else if (qproc->has_spare_reg || qproc->has_ext_cntl_regs) { > > > ret = reset_control_reset(qproc->mss_restart); > > > } else { > > > ret = reset_control_deassert(qproc->mss_restart); > > > @@ -604,7 +635,7 @@ static int q6v5proc_reset(struct q6v5 *qproc) > > > } > > > > > > goto pbl_wait; > > > - } else if (qproc->version == MSS_SC7180) { > > > + } else if (qproc->version == MSS_SC7180 || qproc->version == > > > MSS_SC7280) { > > > val = readl(qproc->reg_base + QDSP6SS_SLEEP); > > > val |= Q6SS_CBCR_CLKEN; > > > writel(val, qproc->reg_base + QDSP6SS_SLEEP); > > > @@ -787,6 +818,82 @@ static int q6v5proc_reset(struct q6v5 *qproc) > > > return ret; > > > } > > > > > > +static int q6v5proc_enable_qchannel(struct q6v5 *qproc, struct > > > regmap *map, u32 offset) > > > +{ > > > + unsigned int val; > > > + int ret; > > > + > > > + if (!qproc->has_qaccept_regs) > > > + return 0; > > > + > > > + if (qproc->has_ext_cntl_regs) { > > > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0); > > > + regmap_write(qproc->conn_map, qproc->force_clk_on, 1); > > > + > > > + ret = regmap_read_poll_timeout(qproc->halt_map, > > > qproc->axim1_clk_off, val, > > > + !val, 1, Q6SS_CBCR_TIMEOUT_US); > > > + if (ret) { > > > + dev_err(qproc->dev, "failed to enable axim1 clock\n"); > > > + return -ETIMEDOUT; > > > + } > > > + } > > > + > > > + regmap_write(map, offset + QACCEPT_REQ_REG, 1); > > > + > > > + /* Wait for accept */ > > > + ret = regmap_read_poll_timeout(map, offset + QACCEPT_ACCEPT_REG, > > > val, val, 5, > > > + QACCEPT_TIMEOUT_US); > > > + if (ret) { > > > + dev_err(qproc->dev, "qchannel enable failed\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void q6v5proc_disable_qchannel(struct q6v5 *qproc, struct > > > regmap *map, u32 offset) > > > +{ > > > + int ret; > > > + unsigned int val, retry; > > > + unsigned int nretry = 10; > > > + bool takedown_complete = false; > > > + > > > + if (!qproc->has_qaccept_regs) > > > + return; > > > + > > > + while (!takedown_complete && nretry) { > > > + nretry--; > > > + > > > + regmap_read_poll_timeout(map, offset + QACCEPT_ACTIVE_REG, val, > > > !val, 5, > > > + QACCEPT_TIMEOUT_US); > > > + > > > + regmap_write(map, offset + QACCEPT_REQ_REG, 0); > > Sure I'll add more comments to this func. > After lowering the request ^^ we wait > for deny to go high or accept to go low. > If it's the former then we do a request > high and repeat the entire process again. > If it's the latter then its considered > that the takedown is success. The above essentially is a transcript of the code into prose. For a reader who isn't familiar with the hardware and might not have access to the corresponding documentation the exact roles of the ACCEPT registers might not be evident. I was looking for something slightly higher level, a one liner here and there might be enough. E.g. something like 'request to disable the channel denied, re-enable it' in the loop below, if that is semantically correct. Is there a typical reason why such a request would be denied, maybe because the channel was busy? Also why is re-enabling actually required if the request to disable was denied? > Let me know if you feel any other parts of the patch requires more comments as well. For now it's mainly the code involving the ACCEPT registers and _disable_channel() in particular. > > > > + > > > + retry = 10; > > > + while (retry) { > > > + usleep_range(5, 10); > > > + retry--; > > > + ret = regmap_read(map, offset + QACCEPT_DENY_REG, &val); > > > + if (!ret && val) { > > > + regmap_write(map, offset + QACCEPT_REQ_REG, 1); > > > + break; > > > + } > > > + > > > + ret = regmap_read(map, offset + QACCEPT_ACCEPT_REG, &val); > > > + if (!ret && !val) { > > > + takedown_complete = true; > > > + break; > > > + } > > > > A bit of commentary in this branch would do no harm. From the code flow > > I can guess that disabling the channel failed when QACCEPT_DENY_REG != > > 0, > > and hence the channel is re-enabled (?) for the next try, and apparently > > things are fine when QACCEPT_ACCEPT_REG is 0 after disabling the > > channel. > > Would be good to be a bit more explicit about what all that actually > > means.
On Fri, Jun 25, 2021 at 01:17:31AM +0530, Sibi Sankar wrote: > Add support for booting the Modem DSP found on QTI SC7280 SoCs. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On Fri, Jun 25, 2021 at 01:17:35AM +0530, Sibi Sankar wrote: > Subject: arm64: dts: qcom: sc7280: Update reserved memory map That's very vague. Also personally I'm not a fan of patches that touch SoC and board files with a commit message that only mentions the SoC, as is frequently done for IDP boards. Why not split this in (at least) two, one for adding the missing memory regions to the SoC, and one for the IDP. > Add missing regions and remove unused regions from the reserved memory > map, as described in version 1. What is this 'version 1'?
On Fri, Jun 25, 2021 at 01:17:37AM +0530, Sibi Sankar wrote: > This patch adds Q6V5 MSS PAS remoteproc node for SC7280 SoCs. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 40 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 3fb6a6ef39f8..56ea172f641f 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -584,6 +584,46 @@ > #power-domain-cells = <1>; > }; > > + remoteproc_mpss: remoteproc@4080000 { > + compatible = "qcom,sc7280-mpss-pas"; > + reg = <0 0x04080000 0 0x10000>; > + > + interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, looks like this patch/series depends on "Enable miscellaneous hardware blocks to boot WPSS" (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=475089) which is not mentioned. > + <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>; > + interrupt-names = "wdog", "fatal", "ready", "handover", > + "stop-ack", "shutdown-ack"; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>; > + clock-names = "xo"; > + > + power-domains = <&rpmhpd SC7280_CX>, > + <&rpmhpd SC7280_MSS>; > + power-domain-names = "cx", "mss"; > + > + memory-region = <&mpss_mem>; > + > + qcom,qmp = <&aoss_qmp>; > + > + qcom,smem-states = <&modem_smp2p_out 0>; > + qcom,smem-state-names = "stop"; > + > + status = "disabled"; > + > + glink-edge { > + interrupts-extended = <&ipcc IPCC_CLIENT_MPSS > + IPCC_MPROC_SIGNAL_GLINK_QMP > + IRQ_TYPE_EDGE_RISING>; > + mboxes = <&ipcc IPCC_CLIENT_MPSS > + IPCC_MPROC_SIGNAL_GLINK_QMP>; > + label = "modem"; > + qcom,remote-pid = <1>; > + }; > + }; > + > stm@6002000 { > compatible = "arm,coresight-stm", "arm,primecell"; > reg = <0 0x06002000 0 0x1000>, Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On Fri, Jun 25, 2021 at 01:17:38AM +0530, Sibi Sankar wrote: > Update MSS node to support MSA based modem boot on SC7280 SoCs. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7280-idp.dts | 7 +++++++ > arch/arm64/boot/dts/qcom/sc7280.dtsi | 19 ++++++++++++++++--- > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts > index 191e8a92d153..d66e3ca42ad5 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts > @@ -343,3 +343,10 @@ > bias-pull-up; > }; > }; > + > +&remoteproc_mpss { > + status = "okay"; > + compatible = "qcom,sc7280-mss-pil"; > + iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>; > + memory-region = <&mba_mem &mpss_mem>; > +}; > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 56ea172f641f..6d3687744440 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -586,7 +586,8 @@ > > remoteproc_mpss: remoteproc@4080000 { > compatible = "qcom,sc7280-mpss-pas"; > - reg = <0 0x04080000 0 0x10000>; > + reg = <0 0x04080000 0 0x10000>, <0 0x04180000 0 0x48>; > + reg-names = "qdsp6", "rmb"; Binding needs update? Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: reg: maxItems: 1 > > interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, > <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, > @@ -597,8 +598,11 @@ > interrupt-names = "wdog", "fatal", "ready", "handover", > "stop-ack", "shutdown-ack"; > > - clocks = <&rpmhcc RPMH_CXO_CLK>; > - clock-names = "xo"; > + clocks = <&gcc GCC_MSS_CFG_AHB_CLK>, > + <&gcc GCC_MSS_OFFLINE_AXI_CLK>, > + <&gcc GCC_MSS_SNOC_AXI_CLK>, > + <&rpmhcc RPMH_CXO_CLK>; > + clock-names = "iface", "offline", "snoc_axi", "xo"; Binding needs update? Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: clocks: items: - description: XO clock clock-names: items: - const: xo
On 2021-06-28 23:41, Matthias Kaehlcke wrote: > On Fri, Jun 25, 2021 at 01:17:35AM +0530, Sibi Sankar wrote: > >> Subject: arm64: dts: qcom: sc7280: Update reserved memory map > > That's very vague. Also personally I'm not a fan of patches that touch > SoC and board files with a commit message that only mentions the SoC, > as > is frequently done for IDP boards. Why not split this in (at least) > two, > one for adding the missing memory regions to the SoC, and one for the > IDP. > sure will split this up. >> Add missing regions and remove unused regions from the reserved memory >> map, as described in version 1. > > What is this 'version 1'? lol, it's the memory map version number and it's not entirely internal to qc so we have been mentioning them in commit messages from older SoCs. I'll just drop it when I re-spin the series since it doesn't add much value.
On 2021-06-29 00:09, Matthias Kaehlcke wrote: > On Fri, Jun 25, 2021 at 01:17:37AM +0530, Sibi Sankar wrote: >> This patch adds Q6V5 MSS PAS remoteproc node for SC7280 SoCs. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sc7280.dtsi | 40 >> ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 3fb6a6ef39f8..56ea172f641f 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -584,6 +584,46 @@ >> #power-domain-cells = <1>; >> }; >> >> + remoteproc_mpss: remoteproc@4080000 { >> + compatible = "qcom,sc7280-mpss-pas"; >> + reg = <0 0x04080000 0 0x10000>; >> + >> + interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, >> + <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, > > looks like this patch/series depends on "Enable miscellaneous hardware > blocks to boot WPSS" > (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=475089) > which is not mentioned. ^^ is already in lnext so didn't mention it. > >> + <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>, >> + <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>, >> + <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>, >> + <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>; >> + interrupt-names = "wdog", "fatal", "ready", "handover", >> + "stop-ack", "shutdown-ack"; >> + >> + clocks = <&rpmhcc RPMH_CXO_CLK>; >> + clock-names = "xo"; >> + >> + power-domains = <&rpmhpd SC7280_CX>, >> + <&rpmhpd SC7280_MSS>; >> + power-domain-names = "cx", "mss"; >> + >> + memory-region = <&mpss_mem>; >> + >> + qcom,qmp = <&aoss_qmp>; >> + >> + qcom,smem-states = <&modem_smp2p_out 0>; >> + qcom,smem-state-names = "stop"; >> + >> + status = "disabled"; >> + >> + glink-edge { >> + interrupts-extended = <&ipcc IPCC_CLIENT_MPSS >> + IPCC_MPROC_SIGNAL_GLINK_QMP >> + IRQ_TYPE_EDGE_RISING>; >> + mboxes = <&ipcc IPCC_CLIENT_MPSS >> + IPCC_MPROC_SIGNAL_GLINK_QMP>; >> + label = "modem"; >> + qcom,remote-pid = <1>; >> + }; >> + }; >> + >> stm@6002000 { >> compatible = "arm,coresight-stm", "arm,primecell"; >> reg = <0 0x06002000 0 0x1000>, > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On 2021-06-29 00:35, Matthias Kaehlcke wrote: > On Fri, Jun 25, 2021 at 01:17:38AM +0530, Sibi Sankar wrote: >> Update MSS node to support MSA based modem boot on SC7280 SoCs. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sc7280-idp.dts | 7 +++++++ >> arch/arm64/boot/dts/qcom/sc7280.dtsi | 19 ++++++++++++++++--- >> 2 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts >> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts >> index 191e8a92d153..d66e3ca42ad5 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts >> @@ -343,3 +343,10 @@ >> bias-pull-up; >> }; >> }; >> + >> +&remoteproc_mpss { >> + status = "okay"; >> + compatible = "qcom,sc7280-mss-pil"; >> + iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>; >> + memory-region = <&mba_mem &mpss_mem>; >> +}; >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 56ea172f641f..6d3687744440 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -586,7 +586,8 @@ >> >> remoteproc_mpss: remoteproc@4080000 { >> compatible = "qcom,sc7280-mpss-pas"; >> - reg = <0 0x04080000 0 0x10000>; >> + reg = <0 0x04080000 0 0x10000>, <0 0x04180000 0 0x48>; >> + reg-names = "qdsp6", "rmb"; > > Binding needs update? > > Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: > > reg: > maxItems: 1 > >> >> interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, >> <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, >> @@ -597,8 +598,11 @@ >> interrupt-names = "wdog", "fatal", "ready", "handover", >> "stop-ack", "shutdown-ack"; >> >> - clocks = <&rpmhcc RPMH_CXO_CLK>; >> - clock-names = "xo"; >> + clocks = <&gcc GCC_MSS_CFG_AHB_CLK>, >> + <&gcc GCC_MSS_OFFLINE_AXI_CLK>, >> + <&gcc GCC_MSS_SNOC_AXI_CLK>, >> + <&rpmhcc RPMH_CXO_CLK>; >> + clock-names = "iface", "offline", "snoc_axi", "xo"; > > Binding needs update? > > Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: > > clocks: > items: > - description: XO clock > clock-names: > items: > - const: xo qcom,sc7280-mpss-pas compatible requires just the xo clock and one reg space whereas the qcom,sc7280-mss-pil compatible requires the additional clks and reg spaces. We just overload properties where re-use is possible across boards. Hence it would be wrong to list those clks/reg spaces as requirements for the pas compatible.
Hi! > This patch series adds support for booting the Modem Q6 DSP found on > Qualcomm's SC7280 SoCs. Am I right this is phone related? Can I get you to cc phone-devel mailing list? Best regards, Pavel
On Thu 24 Jun 14:47 CDT 2021, Sibi Sankar wrote: > Add miscellaneous nodes to boot the modem and support post-mortem debug > on SC7280 SoCs. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 5ed7a511bfc9..3fb6a6ef39f8 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -547,6 +547,11 @@ > #hwlock-cells = <1>; > }; > > + tcsr_regs: syscon@1fc0000 { Is there a different "tcsr"? Does the "_regs" suffix add any value? > + compatible = "syscon"; Rob has pointed out a few times that a lone "syscon" isn't going to be accepted going forward. Could you also add a qualifying "qcom,sc7280-tcsr" or something like that? > + reg = <0 0x01fc0000 0 0x30000>; > + }; > + > lpasscc: lpasscc@3000000 { > compatible = "qcom,sc7280-lpasscc"; > reg = <0 0x03000000 0 0x40>, > @@ -1219,6 +1224,21 @@ > }; > }; > > + imem@146aa000 { > + compatible = "syscon", "simple-mfd"; As above "qcom,sc7280-imem"? I presume we need some new binding documents for these two though, perhaps you can add the specific compatibles and we agree that one of us will write these two bindings soon? Regards, Bjorn > + reg = <0 0x146aa000 0 0x2000>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + > + ranges = <0 0x0 0 0x146aa000 0 0x2000>; > + > + pil-reloc@94c { > + compatible = "qcom,pil-reloc-info"; > + reg = <0 0x94c 0 0xc8>; > + }; > + }; > + > apps_smmu: iommu@15000000 { > compatible = "qcom,sc7280-smmu-500", "arm,mmu-500"; > reg = <0 0x15000000 0 0x100000>; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Thu 24 Jun 14:47 CDT 2021, Sibi Sankar wrote: > This patch adds Q6V5 MSS PAS remoteproc node for SC7280 SoCs. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 40 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 3fb6a6ef39f8..56ea172f641f 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -584,6 +584,46 @@ > #power-domain-cells = <1>; > }; > > + remoteproc_mpss: remoteproc@4080000 { > + compatible = "qcom,sc7280-mpss-pas"; > + reg = <0 0x04080000 0 0x10000>; > + > + interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>, > + <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>; > + interrupt-names = "wdog", "fatal", "ready", "handover", > + "stop-ack", "shutdown-ack"; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>; > + clock-names = "xo"; > + > + power-domains = <&rpmhpd SC7280_CX>, > + <&rpmhpd SC7280_MSS>; > + power-domain-names = "cx", "mss"; > + > + memory-region = <&mpss_mem>; > + > + qcom,qmp = <&aoss_qmp>; > + > + qcom,smem-states = <&modem_smp2p_out 0>; > + qcom,smem-state-names = "stop"; > + > + status = "disabled"; > + > + glink-edge { > + interrupts-extended = <&ipcc IPCC_CLIENT_MPSS > + IPCC_MPROC_SIGNAL_GLINK_QMP > + IRQ_TYPE_EDGE_RISING>; > + mboxes = <&ipcc IPCC_CLIENT_MPSS > + IPCC_MPROC_SIGNAL_GLINK_QMP>; > + label = "modem"; > + qcom,remote-pid = <1>; > + }; > + }; > + > stm@6002000 { > compatible = "arm,coresight-stm", "arm,primecell"; > reg = <0 0x06002000 0 0x1000>, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Wed 30 Jun 15:08 CDT 2021, Sibi Sankar wrote: > On 2021-06-29 00:35, Matthias Kaehlcke wrote: > > On Fri, Jun 25, 2021 at 01:17:38AM +0530, Sibi Sankar wrote: > > > Update MSS node to support MSA based modem boot on SC7280 SoCs. > > > > > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > > > --- > > > arch/arm64/boot/dts/qcom/sc7280-idp.dts | 7 +++++++ > > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 19 ++++++++++++++++--- > > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts > > > b/arch/arm64/boot/dts/qcom/sc7280-idp.dts > > > index 191e8a92d153..d66e3ca42ad5 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts > > > @@ -343,3 +343,10 @@ > > > bias-pull-up; > > > }; > > > }; > > > + > > > +&remoteproc_mpss { > > > + status = "okay"; > > > + compatible = "qcom,sc7280-mss-pil"; > > > + iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>; > > > + memory-region = <&mba_mem &mpss_mem>; > > > +}; > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > index 56ea172f641f..6d3687744440 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > @@ -586,7 +586,8 @@ > > > > > > remoteproc_mpss: remoteproc@4080000 { > > > compatible = "qcom,sc7280-mpss-pas"; > > > - reg = <0 0x04080000 0 0x10000>; > > > + reg = <0 0x04080000 0 0x10000>, <0 0x04180000 0 0x48>; > > > + reg-names = "qdsp6", "rmb"; > > > > Binding needs update? > > > > Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: > > > > reg: > > maxItems: 1 > > > > > > > > interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>, > > > <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>, > > > @@ -597,8 +598,11 @@ > > > interrupt-names = "wdog", "fatal", "ready", "handover", > > > "stop-ack", "shutdown-ack"; > > > > > > - clocks = <&rpmhcc RPMH_CXO_CLK>; > > > - clock-names = "xo"; > > > + clocks = <&gcc GCC_MSS_CFG_AHB_CLK>, > > > + <&gcc GCC_MSS_OFFLINE_AXI_CLK>, > > > + <&gcc GCC_MSS_SNOC_AXI_CLK>, > > > + <&rpmhcc RPMH_CXO_CLK>; > > > + clock-names = "iface", "offline", "snoc_axi", "xo"; > > > > Binding needs update? > > > > Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml: > > > > clocks: > > items: > > - description: XO clock > > clock-names: > > items: > > - const: xo > > qcom,sc7280-mpss-pas compatible requires > just the xo clock and one reg space whereas > the qcom,sc7280-mss-pil compatible requires > the additional clks and reg spaces. We just > overload properties where re-use is possible > across boards. Hence it would be wrong to > list those clks/reg spaces as requirements > for the pas compatible. > Our decision to describe the platform node as a superset of the resources needed by the pas and pil variants was never reflected in the DT bindings; resulting in the issue that the superset doesn't validate against the pas binding and both bindings are full of platform-specific conditionals. To resolve the two issues I think we should split the current binding(s) in a set of platform-centric bindings, that captures the idea of describing the superset. To reduce the duplication - that already exists between the two bindings - I think we should break those out in a common part. I'm however fine with not delaying this series further, if we agree that the end result matches what we would put in a combined qcom,sc7280-mpss binding. Regards, Bjorn
On Wed 30 Jun 15:02 CDT 2021, Sibi Sankar wrote: > On 2021-06-28 23:41, Matthias Kaehlcke wrote: > > On Fri, Jun 25, 2021 at 01:17:35AM +0530, Sibi Sankar wrote: > > > > > Subject: arm64: dts: qcom: sc7280: Update reserved memory map > > > > That's very vague. Also personally I'm not a fan of patches that touch > > SoC and board files with a commit message that only mentions the SoC, as > > is frequently done for IDP boards. Why not split this in (at least) two, > > one for adding the missing memory regions to the SoC, and one for the > > IDP. > > > > sure will split this up. > > > > Add missing regions and remove unused regions from the reserved memory > > > map, as described in version 1. > > > > What is this 'version 1'? > > lol, it's the memory map version number > and it's not entirely internal to qc so > we have been mentioning them in commit > messages from older SoCs. I'll just drop > it when I re-spin the series since it > doesn't add much value. > Every now and then we run into issues with the reserved-memory layout, where knowing were the numbers comes from is useful information to have in order to characterize the issue and come up with a fix. So including information about where those numbers came from is useful, even if it's referencing a version of a document that's not public. Regards, Bjorn