| Message ID | 20231015162114.976202-1-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk: qcom: gcc-ipq6018: add QUP6 I2C clock | expand |
On 10/15/23 18:20, Robert Marko wrote: > QUP6 I2C clock is listed in the dt bindings but it was never included in > the GCC driver. > So lets add support for it, its intentionally marked to never be disabled > as its somehow affecting DVFS and if disabled it sometimes crashes the > board. > > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- Bjorn, would you be able to get an idea of what could be sitting on that bus? Or maybe the IPQ folks could know? Konrad
On 10/19/2023 1:59 AM, Konrad Dybcio wrote: > > > On 10/15/23 18:20, Robert Marko wrote: >> QUP6 I2C clock is listed in the dt bindings but it was never included in >> the GCC driver. >> So lets add support for it, its intentionally marked to never be >> disabled >> as its somehow affecting DVFS and if disabled it sometimes crashes the >> board. >> >> Signed-off-by: Robert Marko <robimarko@gmail.com> >> --- > Bjorn, would you be able to get an idea of what could be sitting > on that bus? > > Or maybe the IPQ folks could know? > Konrad / Robert, Similar to IPQ9574, RPM needs this clock to communicate with PMIC over I2C interface. Discussion happened here[1] is pretty much applicable to IPQ6018 as well. Based on previous experience, we may need to document the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> [1] https://lore.kernel.org/linux-arm-msm/2852fc37-284f-6534-f163-45b37b153db1@quicinc.com/ > Konrad
On Thu, 19 Oct 2023 at 08:46, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > On 10/19/2023 1:59 AM, Konrad Dybcio wrote: > > > > > > On 10/15/23 18:20, Robert Marko wrote: > >> QUP6 I2C clock is listed in the dt bindings but it was never included in > >> the GCC driver. > >> So lets add support for it, its intentionally marked to never be > >> disabled > >> as its somehow affecting DVFS and if disabled it sometimes crashes the > >> board. > >> > >> Signed-off-by: Robert Marko <robimarko@gmail.com> > >> --- > > Bjorn, would you be able to get an idea of what could be sitting > > on that bus? > > > > Or maybe the IPQ folks could know? > > > > Konrad / Robert, > > Similar to IPQ9574, RPM needs this clock to communicate with PMIC over > I2C interface. Discussion happened here[1] is pretty much applicable to > IPQ6018 as well. Based on previous experience, we may need to document > the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, > > Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> Thanks for the explanation, it makes sense as I used to control the MP5496 directly via I2C. Is it possible to disable RPM so it doesn't conflict and just use the full MP5496 driver instead? I find the RPM quite limiting as there is no way to actually retrieve the current-voltage. Regards, Robert > > [1] > https://lore.kernel.org/linux-arm-msm/2852fc37-284f-6534-f163-45b37b153db1@quicinc.com/ > > > > Konrad
On 10/19/2023 7:53 PM, Robert Marko wrote: > On Thu, 19 Oct 2023 at 08:46, Kathiravan Thirumoorthy > <quic_kathirav@quicinc.com> wrote: >> >> On 10/19/2023 1:59 AM, Konrad Dybcio wrote: >>> >>> On 10/15/23 18:20, Robert Marko wrote: >>>> QUP6 I2C clock is listed in the dt bindings but it was never included in >>>> the GCC driver. >>>> So lets add support for it, its intentionally marked to never be >>>> disabled >>>> as its somehow affecting DVFS and if disabled it sometimes crashes the >>>> board. >>>> >>>> Signed-off-by: Robert Marko <robimarko@gmail.com> >>>> --- >>> Bjorn, would you be able to get an idea of what could be sitting >>> on that bus? >>> >>> Or maybe the IPQ folks could know? >>> >> Konrad / Robert, >> >> Similar to IPQ9574, RPM needs this clock to communicate with PMIC over >> I2C interface. Discussion happened here[1] is pretty much applicable to >> IPQ6018 as well. Based on previous experience, we may need to document >> the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, >> >> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> > Thanks for the explanation, it makes sense as I used to control the > MP5496 directly > via I2C. > > Is it possible to disable RPM so it doesn't conflict and just use the > full MP5496 driver instead? > I find the RPM quite limiting as there is no way to actually retrieve > the current-voltage. Robert, checked with the team and currently we don't have any option to disable the RPM as such. > > Regards, > Robert >> [1] >> https://lore.kernel.org/linux-arm-msm/2852fc37-284f-6534-f163-45b37b153db1@quicinc.com/ >> >> >>> Konrad
On Fri, 20 Oct 2023 at 08:40, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > On 10/19/2023 7:53 PM, Robert Marko wrote: > > On Thu, 19 Oct 2023 at 08:46, Kathiravan Thirumoorthy > > <quic_kathirav@quicinc.com> wrote: > >> > >> On 10/19/2023 1:59 AM, Konrad Dybcio wrote: > >>> > >>> On 10/15/23 18:20, Robert Marko wrote: > >>>> QUP6 I2C clock is listed in the dt bindings but it was never included in > >>>> the GCC driver. > >>>> So lets add support for it, its intentionally marked to never be > >>>> disabled > >>>> as its somehow affecting DVFS and if disabled it sometimes crashes the > >>>> board. > >>>> > >>>> Signed-off-by: Robert Marko <robimarko@gmail.com> > >>>> --- > >>> Bjorn, would you be able to get an idea of what could be sitting > >>> on that bus? > >>> > >>> Or maybe the IPQ folks could know? > >>> > >> Konrad / Robert, > >> > >> Similar to IPQ9574, RPM needs this clock to communicate with PMIC over > >> I2C interface. Discussion happened here[1] is pretty much applicable to > >> IPQ6018 as well. Based on previous experience, we may need to document > >> the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, > >> > >> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> > > Thanks for the explanation, it makes sense as I used to control the > > MP5496 directly > > via I2C. > > > > Is it possible to disable RPM so it doesn't conflict and just use the > > full MP5496 driver instead? > > I find the RPM quite limiting as there is no way to actually retrieve > > the current-voltage. > > > Robert, checked with the team and currently we don't have any option to > disable the RPM as such. Thanks for checking. BTW, is there any information you could share on the USB GDSC-s? I tried converting them to GDSC-s like IPQ8074 has, but it seems that they have different features. It would be great to use proper GSDC support instead of manually setting bits during GCC probe. Regards, Robert > > > > > > Regards, > > Robert > >> [1] > >> https://lore.kernel.org/linux-arm-msm/2852fc37-284f-6534-f163-45b37b153db1@quicinc.com/ > >> > >> > >>> Konrad
On 10/21/2023 5:27 PM, Robert Marko wrote: > On Fri, 20 Oct 2023 at 08:40, Kathiravan Thirumoorthy > <quic_kathirav@quicinc.com> wrote: >> >> >> On 10/19/2023 7:53 PM, Robert Marko wrote: >>> On Thu, 19 Oct 2023 at 08:46, Kathiravan Thirumoorthy >>> <quic_kathirav@quicinc.com> wrote: >>>> >>>> On 10/19/2023 1:59 AM, Konrad Dybcio wrote: >>>>> >>>>> On 10/15/23 18:20, Robert Marko wrote: >>>>>> QUP6 I2C clock is listed in the dt bindings but it was never included in >>>>>> the GCC driver. >>>>>> So lets add support for it, its intentionally marked to never be >>>>>> disabled >>>>>> as its somehow affecting DVFS and if disabled it sometimes crashes the >>>>>> board. >>>>>> >>>>>> Signed-off-by: Robert Marko <robimarko@gmail.com> >>>>>> --- >>>>> Bjorn, would you be able to get an idea of what could be sitting >>>>> on that bus? >>>>> >>>>> Or maybe the IPQ folks could know? >>>>> >>>> Konrad / Robert, >>>> >>>> Similar to IPQ9574, RPM needs this clock to communicate with PMIC over >>>> I2C interface. Discussion happened here[1] is pretty much applicable to >>>> IPQ6018 as well. Based on previous experience, we may need to document >>>> the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, >>>> >>>> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> >>> Thanks for the explanation, it makes sense as I used to control the >>> MP5496 directly >>> via I2C. >>> >>> Is it possible to disable RPM so it doesn't conflict and just use the >>> full MP5496 driver instead? >>> I find the RPM quite limiting as there is no way to actually retrieve >>> the current-voltage. >> >> >> Robert, checked with the team and currently we don't have any option to >> disable the RPM as such. > > Thanks for checking. > > BTW, is there any information you could share on the USB GDSC-s? > I tried converting them to GDSC-s like IPQ8074 has, but it seems that > they have different features. > It would be great to use proper GSDC support instead of manually > setting bits during GCC probe. Could you please explain the issue which you are facing? Based on quick look at the HW documentation, seems there is no difference between IPQ8074 and IPQ6018 GDSC-s. Thanks,
On Wed, 25 Oct 2023 at 10:42, Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> wrote: > > > > On 10/21/2023 5:27 PM, Robert Marko wrote: > > On Fri, 20 Oct 2023 at 08:40, Kathiravan Thirumoorthy > > <quic_kathirav@quicinc.com> wrote: > >> > >> > >> On 10/19/2023 7:53 PM, Robert Marko wrote: > >>> On Thu, 19 Oct 2023 at 08:46, Kathiravan Thirumoorthy > >>> <quic_kathirav@quicinc.com> wrote: > >>>> > >>>> On 10/19/2023 1:59 AM, Konrad Dybcio wrote: > >>>>> > >>>>> On 10/15/23 18:20, Robert Marko wrote: > >>>>>> QUP6 I2C clock is listed in the dt bindings but it was never included in > >>>>>> the GCC driver. > >>>>>> So lets add support for it, its intentionally marked to never be > >>>>>> disabled > >>>>>> as its somehow affecting DVFS and if disabled it sometimes crashes the > >>>>>> board. > >>>>>> > >>>>>> Signed-off-by: Robert Marko <robimarko@gmail.com> > >>>>>> --- > >>>>> Bjorn, would you be able to get an idea of what could be sitting > >>>>> on that bus? > >>>>> > >>>>> Or maybe the IPQ folks could know? > >>>>> > >>>> Konrad / Robert, > >>>> > >>>> Similar to IPQ9574, RPM needs this clock to communicate with PMIC over > >>>> I2C interface. Discussion happened here[1] is pretty much applicable to > >>>> IPQ6018 as well. Based on previous experience, we may need to document > >>>> the reason for CLK_IGNORE_UNUSED in driver as well. Nevertheless, > >>>> > >>>> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> > >>> Thanks for the explanation, it makes sense as I used to control the > >>> MP5496 directly > >>> via I2C. > >>> > >>> Is it possible to disable RPM so it doesn't conflict and just use the > >>> full MP5496 driver instead? > >>> I find the RPM quite limiting as there is no way to actually retrieve > >>> the current-voltage. > >> > >> > >> Robert, checked with the team and currently we don't have any option to > >> disable the RPM as such. > > > > Thanks for checking. > > > > BTW, is there any information you could share on the USB GDSC-s? > > I tried converting them to GDSC-s like IPQ8074 has, but it seems that > > they have different features. > > It would be great to use proper GSDC support instead of manually > > setting bits during GCC probe. > > > Could you please explain the issue which you are facing? Based on quick > look at the HW documentation, seems there is no difference between > IPQ8074 and IPQ6018 GDSC-s. Thanks for checking, it seems I hit a buggy kernel version last time I tried to convert the driver to use proper GDSC-s and now it seems to work fine, I will send patches to add GDSC-s. Regards, Robert > > Thanks,
diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c index 31eef38c5864c..126d87df80c5b 100644 --- a/drivers/clk/qcom/gcc-ipq6018.c +++ b/drivers/clk/qcom/gcc-ipq6018.c @@ -2125,6 +2125,22 @@ static struct clk_branch gcc_blsp1_qup5_spi_apps_clk = { }, }; +static struct clk_branch gcc_blsp1_qup6_i2c_apps_clk = { + .halt_reg = 0x07010, + .clkr = { + .enable_reg = 0x07010, + .enable_mask = BIT(0), + .hw.init = &(struct clk_init_data){ + .name = "gcc_blsp1_qup6_i2c_apps_clk", + .parent_hws = (const struct clk_hw *[]){ + &blsp1_qup6_i2c_apps_clk_src.clkr.hw }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, + .ops = &clk_branch2_ops, + }, + }, +}; + static struct clk_branch gcc_blsp1_qup6_spi_apps_clk = { .halt_reg = 0x0700c, .clkr = { @@ -4281,6 +4297,7 @@ static struct clk_regmap *gcc_ipq6018_clks[] = { [GCC_BLSP1_QUP4_SPI_APPS_CLK] = &gcc_blsp1_qup4_spi_apps_clk.clkr, [GCC_BLSP1_QUP5_I2C_APPS_CLK] = &gcc_blsp1_qup5_i2c_apps_clk.clkr, [GCC_BLSP1_QUP5_SPI_APPS_CLK] = &gcc_blsp1_qup5_spi_apps_clk.clkr, + [GCC_BLSP1_QUP6_I2C_APPS_CLK] = &gcc_blsp1_qup6_i2c_apps_clk.clkr, [GCC_BLSP1_QUP6_SPI_APPS_CLK] = &gcc_blsp1_qup6_spi_apps_clk.clkr, [GCC_BLSP1_UART1_APPS_CLK] = &gcc_blsp1_uart1_apps_clk.clkr, [GCC_BLSP1_UART2_APPS_CLK] = &gcc_blsp1_uart2_apps_clk.clkr,
QUP6 I2C clock is listed in the dt bindings but it was never included in the GCC driver. So lets add support for it, its intentionally marked to never be disabled as its somehow affecting DVFS and if disabled it sometimes crashes the board. Signed-off-by: Robert Marko <robimarko@gmail.com> --- drivers/clk/qcom/gcc-ipq6018.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)