[PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Robert Marko]

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(+)

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,
-- 
2.41.0

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Konrad Dybcio]

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

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Kathiravan Thirumoorthy]

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

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Robert Marko]

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

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Kathiravan Thirumoorthy]

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

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Robert Marko]

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

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Kathiravan Thirumoorthy]

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,

Re: [PATCH] clk: qcom: gcc-ipq6018: add QUP6 I2C clock

[Robert Marko]

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,