[PATCH v2 0/3] media: camss: Link CAMSS power domain on MSM8996

[PATCH v2 0/3] media: camss: Link CAMSS power domain on MSM8996

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

CAMSS on MSM8996 has been broken since commit
46cc03175498 ("media: camss: Split power domain management").
This would happen when trying to start streaming:

[  199.097810] ------------[ cut here ]------------
[  199.097893] camss_top_ahb_clk status stuck at 'off'
[  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
...
[  199.100064]  clk_branch_wait+0x140/0x160
[  199.100112]  clk_branch2_enable+0x30/0x40
[  199.100159]  clk_core_enable+0x6c/0xb0
[  199.100211]  clk_enable+0x2c/0x50
[  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
[  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
...
[  199.101594] ---[ end trace 0000000000000000 ]---
[  199.101736] qcom-camss a34000.camss: clock enable failed: -16
[  199.101813] qcom-camss a34000.camss: Failed to power up pipeline: -16

Turns out camss_top_ahb_clk needs the CAMSS power domain to be on. Before
the change, VFE power domains were enabled before CSIPHY enabled clocks,
and since the CAMSS power domain was their parent, it got enabled as well.
With the VFE power domains now enabled after CSIPHY is powered on, the
CAMSS power domain remains off and things go south when CSIPHY tries to
enable camss_top_ahb_clk.

Link the CAMSS power domain in camss_configure_pd to make sure it gets
enabled before CSIPHY tries to enable clocks.

Changes since v1:
  - Wrap commit messages at 75 lines.
  - Remove duplicate "media" from and shorten the subject of the first
    patch.
  - Expand the commit message of the second patch.
  - Fix "Fixes" tag in the third patch as well as other commit references.

Yassine Oudjana (3):
  dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain
  arm64: dts: qcom: msm8996: Add CAMSS power domain and
    power-domain-names to CAMSS
  media: camss: Link CAMSS power domain

 .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
 arch/arm64/boot/dts/qcom/msm8996.dtsi               |  4 +++-
 drivers/media/platform/qcom/camss/camss.c           |  9 ++++++++-
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add the CAMSS power domain which is needed for the proper operation of
CAMSS, and add power-domain-names to ease fetching it as well as the other
power domains.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
index 8a10aa1cafc5..27c9a11f0df9 100644
--- a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
@@ -85,6 +85,13 @@ properties:
     items:
       - description: VFE0 GDSC - Video Front End, Global Distributed Switch Controller.
       - description: VFE1 GDSC - Video Front End, Global Distributed Switch Controller.
+      - description: CAMSS GDSC - Camera Subsystem, Global Distributed Switch Controller.
+
+  power-domain-names:
+    items:
+      - const: vfe0
+      - const: vfe1
+      - const: camss
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -209,6 +216,7 @@ required:
   - interrupts
   - iommus
   - power-domains
+  - power-domain-names
   - reg
   - reg-names
   - vdda-supply
@@ -326,7 +334,10 @@ examples:
          <&vfe_smmu 3>;
 
       power-domains = <&mmcc VFE0_GDSC>,
-        <&mmcc VFE1_GDSC>;
+        <&mmcc VFE1_GDSC>,
+        <&mmcc CAMSS_GDSC>;
+
+      power-domain-names = "vfe0", "vfe1", "camss";
 
       reg = <0x00a34000 0x1000>,
         <0x00a00030 0x4>,
-- 
2.40.1

[PATCH v2 2/3] arm64: dts: qcom: msm8996: Add CAMSS power domain and power-domain-names to CAMSS

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

Add the CAMSS power domain which powers CAMSS and is needed to enable its
clocks, as well as power-domain-names for all CAMSS power domains to make
fetching them easier.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 2b35cb3f5292..b2e0cd739d90 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2118,7 +2118,9 @@ camss: camss@a00000 {
 				"vfe0",
 				"vfe1";
 			power-domains = <&mmcc VFE0_GDSC>,
-					<&mmcc VFE1_GDSC>;
+					<&mmcc VFE1_GDSC>,
+					<&mmcc CAMSS_GDSC>;
+			power-domain-names = "vfe0", "vfe1", "camss";
 			clocks = <&mmcc CAMSS_TOP_AHB_CLK>,
 				<&mmcc CAMSS_ISPIF_AHB_CLK>,
 				<&mmcc CAMSS_CSI0PHYTIMER_CLK>,
-- 
2.40.1

[PATCH v2 3/3] media: camss: Link CAMSS power domain

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

The CAMSS power domain was previously enabled implicitly when the VFE
power domains were enabled.
Commit 46cc03175498 ("media: camss: Split power domain management")
delayed enabling VFE power domains which in turn delayed enabling the
CAMSS power domain. This made CSIPHY fail to enable camss_top_ahb_clk
which requires the CAMSS power domain to be on:

[  199.097810] ------------[ cut here ]------------
[  199.097893] camss_top_ahb_clk status stuck at 'off'
[  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
...
[  199.100064]  clk_branch_wait+0x140/0x160
[  199.100112]  clk_branch2_enable+0x30/0x40
[  199.100159]  clk_core_enable+0x6c/0xb0
[  199.100211]  clk_enable+0x2c/0x50
[  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
[  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
...
[  199.101594] ---[ end trace 0000000000000000 ]---

Link the CAMSS power domain in camss_configure_pd to make sure it gets
enabled before CSIPHY tries to enable clocks.

Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")
Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1ef26aea3eae..9aea8220d923 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
 static int camss_configure_pd(struct camss *camss)
 {
 	struct device *dev = camss->dev;
+	int camss_pd_index;
 	int i;
 	int ret;
 
@@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
 		}
 	}
 
-	if (i > camss->vfe_num) {
+	/* Link CAMSS power domain if available */
+	camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
+	if (camss_pd_index >= 0)
+		device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS |
+				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+
+	if (i > camss->vfe_num && i != camss_pd_index) {
 		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
 							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
 							   DL_FLAG_RPM_ACTIVE);
-- 
2.40.1

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

Yo Yassine,

On Fri, May 26, 2023 at 09:07:10PM +0300, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add the CAMSS power domain which is needed for the proper operation of
> CAMSS, and add power-domain-names to ease fetching it as well as the other
> power domains.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../bindings/media/qcom,msm8996-camss.yaml          | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> index 8a10aa1cafc5..27c9a11f0df9 100644
> --- a/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,msm8996-camss.yaml
> @@ -85,6 +85,13 @@ properties:
>      items:
>        - description: VFE0 GDSC - Video Front End, Global Distributed Switch Controller.
>        - description: VFE1 GDSC - Video Front End, Global Distributed Switch Controller.
> +      - description: CAMSS GDSC - Camera Subsystem, Global Distributed Switch Controller.
> +
> +  power-domain-names:
> +    items:
> +      - const: vfe0
> +      - const: vfe1
> +      - const: camss
>  
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
> @@ -209,6 +216,7 @@ required:
>    - interrupts
>    - iommus
>    - power-domains
> +  - power-domain-names

Why is this now required?

Thanks,
Conor.

>    - reg
>    - reg-names
>    - vdda-supply
> @@ -326,7 +334,10 @@ examples:
>           <&vfe_smmu 3>;
>  
>        power-domains = <&mmcc VFE0_GDSC>,
> -        <&mmcc VFE1_GDSC>;
> +        <&mmcc VFE1_GDSC>,
> +        <&mmcc CAMSS_GDSC>;
> +
> +      power-domain-names = "vfe0", "vfe1", "camss";
>  
>        reg = <0x00a34000 0x1000>,
>          <0x00a00030 0x4>,
> -- 
> 2.40.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 20:46, Conor Dooley wrote:
>> +  - power-domain-names
> Why is this now required?
> 
> Thanks,
> Conor.
> 

Its an accurate description of the power/clock tree to have the top 
power domain be switched on prior to the clocks that depend on it.

I think Yassine, you could probably include the majority of your 
cover-letter text in this commit to explain this change a bit better.

---
bod

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Fri, May 26, 2023 at 09:05:47PM +0100, Bryan O'Donoghue wrote:
> On 26/05/2023 20:46, Conor Dooley wrote:
> > > +  - power-domain-names
> > Why is this now required?
> > 
> > Thanks,
> > Conor.
> > 
> 
> Its an accurate description of the power/clock tree to have the top power
> domain be switched on prior to the clocks that depend on it.

But what does that have to do with the *names* now being required?

> I think Yassine, you could probably include the majority of your
> cover-letter text in this commit to explain this change a bit better.

I think it would be good to have that regardless.

Cheers,
Conor.

> bod
  ^^^ I've been trying not to think about rugby since the weekend :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 21:19, Conor Dooley wrote:
> On Fri, May 26, 2023 at 09:05:47PM +0100, Bryan O'Donoghue wrote:
>> On 26/05/2023 20:46, Conor Dooley wrote:
>>>> +  - power-domain-names
>>> Why is this now required?
>>>
>>> Thanks,
>>> Conor.
>>>
>>
>> Its an accurate description of the power/clock tree to have the top power
>> domain be switched on prior to the clocks that depend on it.
> 
> But what does that have to do with the *names* now being required?

oh the names

no toss that

> 
>> I think Yassine, you could probably include the majority of your
>> cover-letter text in this commit to explain this change a bit better.
> 
> I think it would be good to have that regardless.
> 
> Cheers,
> Conor.
> 
>> bod
>    ^^^ I've been trying not to think about rugby since the weekend :(

Pockets O'Gara should have his paddy papers rescinded

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Konrad Dybcio]

On 26.05.2023 22:21, Bryan O'Donoghue wrote:
> On 26/05/2023 21:19, Conor Dooley wrote:
>> On Fri, May 26, 2023 at 09:05:47PM +0100, Bryan O'Donoghue wrote:
>>> On 26/05/2023 20:46, Conor Dooley wrote:
>>>>> +  - power-domain-names
>>>> Why is this now required?
>>>>
>>>> Thanks,
>>>> Conor.
>>>>
>>>
>>> Its an accurate description of the power/clock tree to have the top power
>>> domain be switched on prior to the clocks that depend on it.
>>
>> But what does that have to do with the *names* now being required?
> 
> oh the names
> 
> no toss that
this should be

if:properties:compatible:blahblahmsm8996:then:required:power-domain-names

Konrad
> 
>>
>>> I think Yassine, you could probably include the majority of your
>>> cover-letter text in this commit to explain this change a bit better.
>>
>> I think it would be good to have that regardless.
>>
>> Cheers,
>> Conor.
>>
>>> bod
>>    ^^^ I've been trying not to think about rugby since the weekend :(
> 
> Pockets O'Gara should have his paddy papers rescinded

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 21:36, Konrad Dybcio wrote:
>> oh the names
>>
>> no toss that
> this should be
> 
> if:properties:compatible:blahblahmsm8996:then:required:power-domain-names
> 
> Konrad

Hmm, we don't depend on the names though.

---
bod

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Konrad Dybcio]

On 26.05.2023 22:40, Bryan O'Donoghue wrote:
> On 26/05/2023 21:36, Konrad Dybcio wrote:
>>> oh the names
>>>
>>> no toss that
>> this should be
>>
>> if:properties:compatible:blahblahmsm8996:then:required:power-domain-names
>>
>> Konrad
> 
> Hmm, we don't depend on the names though.
Check patch 3!

Konrad
> 
> ---
> bod

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 21:43, Konrad Dybcio wrote:
> 
> 
> On 26.05.2023 22:40, Bryan O'Donoghue wrote:
>> On 26/05/2023 21:36, Konrad Dybcio wrote:
>>>> oh the names
>>>>
>>>> no toss that
>>> this should be
>>>
>>> if:properties:compatible:blahblahmsm8996:then:required:power-domain-names
>>>
>>> Konrad
>>
>> Hmm, we don't depend on the names though.
> Check patch 3!

Hmm but we already count the number of power domains in 
camss_configure_pd().

There's no logic in counting it twice using two different methods.

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 19:07, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> The CAMSS power domain was previously enabled implicitly when the VFE
> power domains were enabled.
> Commit 46cc03175498 ("media: camss: Split power domain management")
> delayed enabling VFE power domains which in turn delayed enabling the
> CAMSS power domain. This made CSIPHY fail to enable camss_top_ahb_clk
> which requires the CAMSS power domain to be on:
> 
> [  199.097810] ------------[ cut here ]------------
> [  199.097893] camss_top_ahb_clk status stuck at 'off'
> [  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
> ...
> [  199.100064]  clk_branch_wait+0x140/0x160
> [  199.100112]  clk_branch2_enable+0x30/0x40
> [  199.100159]  clk_core_enable+0x6c/0xb0
> [  199.100211]  clk_enable+0x2c/0x50
> [  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
> [  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
> ...
> [  199.101594] ---[ end trace 0000000000000000 ]---
> 
> Link the CAMSS power domain in camss_configure_pd to make sure it gets
> enabled before CSIPHY tries to enable clocks.
> 
> Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1ef26aea3eae..9aea8220d923 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
>   static int camss_configure_pd(struct camss *camss)
>   {
>   	struct device *dev = camss->dev;
> +	int camss_pd_index;
>   	int i;
>   	int ret;
>   
> @@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
>   		}
>   	}
>   
> -	if (i > camss->vfe_num) {
> +	/* Link CAMSS power domain if available */
> +	camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
> +	if (camss_pd_index >= 0)
> +		device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS |
> +				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> +
> +	if (i > camss->vfe_num && i != camss_pd_index) {
>   		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
>   							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>   							   DL_FLAG_RPM_ACTIVE);

Konrad pointed this out.

Are you 100% sure you want to do this. We already have a way to count 
the # of power-domains in camss_configure_pd().

Your series is now adding a dependency on power-domain-names.

Is there a good reason to add that dependency ? If not, then lets just 
take the code from camss_configure_pd() and make it so that it can be 
used/reused here.

---
bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 19:07, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> The CAMSS power domain was previously enabled implicitly when the VFE
> power domains were enabled.
> Commit 46cc03175498 ("media: camss: Split power domain management")
> delayed enabling VFE power domains which in turn delayed enabling the
> CAMSS power domain. This made CSIPHY fail to enable camss_top_ahb_clk
> which requires the CAMSS power domain to be on:
> 
> [  199.097810] ------------[ cut here ]------------
> [  199.097893] camss_top_ahb_clk status stuck at 'off'
> [  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
> ...
> [  199.100064]  clk_branch_wait+0x140/0x160
> [  199.100112]  clk_branch2_enable+0x30/0x40
> [  199.100159]  clk_core_enable+0x6c/0xb0
> [  199.100211]  clk_enable+0x2c/0x50
> [  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
> [  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
> ...
> [  199.101594] ---[ end trace 0000000000000000 ]---
> 
> Link the CAMSS power domain in camss_configure_pd to make sure it gets
> enabled before CSIPHY tries to enable clocks.
> 
> Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1ef26aea3eae..9aea8220d923 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
>   static int camss_configure_pd(struct camss *camss)
>   {
>   	struct device *dev = camss->dev;
> +	int camss_pd_index;
>   	int i;
>   	int ret;
>   
> @@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
>   		}
>   	}
>   
> -	if (i > camss->vfe_num) {
> +	/* Link CAMSS power domain if available */
> +	camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
> +	if (camss_pd_index >= 0)

Surely if you get this far you already know that

camss->genpd_num = of_count_phandle_with_args(dev->of_node,
                                               "power-domains",
                                               "#power-domain-cells");
if (camss->genpd_num < 0) {
         dev_err(dev, "Power domains are not defined for camss\n");
         return camss->genpd_num;
}

So you don't need to add this additional dependency ?

---
bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Konrad Dybcio]

On 26.05.2023 22:49, Bryan O'Donoghue wrote:
> On 26/05/2023 19:07, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>
>> The CAMSS power domain was previously enabled implicitly when the VFE
>> power domains were enabled.
>> Commit 46cc03175498 ("media: camss: Split power domain management")
>> delayed enabling VFE power domains which in turn delayed enabling the
>> CAMSS power domain. This made CSIPHY fail to enable camss_top_ahb_clk
>> which requires the CAMSS power domain to be on:
>>
>> [  199.097810] ------------[ cut here ]------------
>> [  199.097893] camss_top_ahb_clk status stuck at 'off'
>> [  199.097913] WARNING: CPU: 3 PID: 728 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
>> ...
>> [  199.100064]  clk_branch_wait+0x140/0x160
>> [  199.100112]  clk_branch2_enable+0x30/0x40
>> [  199.100159]  clk_core_enable+0x6c/0xb0
>> [  199.100211]  clk_enable+0x2c/0x50
>> [  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
>> [  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
>> ...
>> [  199.101594] ---[ end trace 0000000000000000 ]---
>>
>> Link the CAMSS power domain in camss_configure_pd to make sure it gets
>> enabled before CSIPHY tries to enable clocks.
>>
>> Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 1ef26aea3eae..9aea8220d923 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1453,6 +1453,7 @@ static const struct media_device_ops camss_media_ops = {
>>   static int camss_configure_pd(struct camss *camss)
>>   {
>>       struct device *dev = camss->dev;
>> +    int camss_pd_index;
>>       int i;
>>       int ret;
>>   @@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss *camss)
>>           }
>>       }
>>   -    if (i > camss->vfe_num) {
>> +    /* Link CAMSS power domain if available */
>> +    camss_pd_index = device_property_match_string(camss->dev, "power-domain-names", "camss");
>> +    if (camss_pd_index >= 0)
>> +        device_link_add(camss->dev, camss->genpd[camss_pd_index], DL_FLAG_STATELESS |
>> +                DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
>> +
>> +    if (i > camss->vfe_num && i != camss_pd_index) {
>>           camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
>>                                  DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>>                                  DL_FLAG_RPM_ACTIVE);
> 
> Konrad pointed this out.
> 
> Are you 100% sure you want to do this. We already have a way to count the # of power-domains in camss_configure_pd().
> 
> Your series is now adding a dependency on power-domain-names.
> 
> Is there a good reason to add that dependency ? If not, then lets just take the code from camss_configure_pd() and make it so that it can be used/reused here.
This code contains a whole bunch of hacky counting logic that should have
been substituted with _byname, but now we're stuck with indices to keep
compatibility with old DTs :/

If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
CAMSS-related GDSCs, we could make it their parent in the clock driver
and call it a day.

Konrad

> 
> ---
> bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 21:57, Konrad Dybcio wrote:
> This code contains a whole bunch of hacky counting logic that should have
> been substituted with _byname, but now we're stuck with indices to keep
> compatibility with old DTs :/
> 
> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
> CAMSS-related GDSCs, we could make it their parent in the clock driver
> and call it a day.

I mean, it wouldn't make much sense from a hw design POV if that weren't 
the case..

Hmm looks like its already there.

static struct gdsc vfe0_gdsc = {
         .gdscr = 0x3664,
         .cxcs = (unsigned int []){ 0x36a8 },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe0",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

static struct gdsc vfe1_gdsc = {
         .gdscr = 0x3674,
         .cxcs = (unsigned int []){ 0x36ac },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe1",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

I feel this is probably a problem in the description of dependencies for 
the CSIPHY in the dts for the 8996..

I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer" },


should probably look something like

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer", "vfe0"},

But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC 
via the VFEx

Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?

git diff
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi 
b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 30257c07e1279..60e5d3f5336d4 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2120,7 +2120,8 @@ camss: camss@a00000 {
                                 "vfe0",
                                 "vfe1";
                         power-domains = <&mmcc VFE0_GDSC>,
-                                       <&mmcc VFE1_GDSC>;
+                                       <&mmcc VFE1_GDSC>,
+                                       <&mmcc CAMSS_GDSC>;

Either of those approaches should mitigate this patch.

---
bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Konrad Dybcio]

On 26.05.2023 23:17, Bryan O'Donoghue wrote:
> On 26/05/2023 21:57, Konrad Dybcio wrote:
>> This code contains a whole bunch of hacky counting logic that should have
>> been substituted with _byname, but now we're stuck with indices to keep
>> compatibility with old DTs :/
>>
>> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
>> CAMSS-related GDSCs, we could make it their parent in the clock driver
>> and call it a day.
> 
> I mean, it wouldn't make much sense from a hw design POV if that weren't the case..
> 
> Hmm looks like its already there.
> 
> static struct gdsc vfe0_gdsc = {
>         .gdscr = 0x3664,
>         .cxcs = (unsigned int []){ 0x36a8 },
>         .cxc_count = 1,
>         .pd = {
>                 .name = "vfe0",
>         },
>         .parent = &camss_gdsc.pd,
>         .pwrsts = PWRSTS_OFF_ON,
> };
> 
> static struct gdsc vfe1_gdsc = {
>         .gdscr = 0x3674,
>         .cxcs = (unsigned int []){ 0x36ac },
>         .cxc_count = 1,
>         .pd = {
>                 .name = "vfe1",
>         },
>         .parent = &camss_gdsc.pd,
>         .pwrsts = PWRSTS_OFF_ON,
> };
> 
> I feel this is probably a problem in the description of dependencies for the CSIPHY in the dts for the 8996..
> 
> I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..
> 
> static const struct resources csiphy_res_8x96[] = {
>         /* CSIPHY0 */
>         {
>                 .regulators = {},
>                 .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer" },
> 
> 
> should probably look something like
> 
> static const struct resources csiphy_res_8x96[] = {
>         /* CSIPHY0 */
>         {
>                 .regulators = {},
>                 .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer", "vfe0"},
> 
> But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC via the VFEx
I have little idea how CAMSS is tied together, but the VFEn clocks
are assigned in vfe_res_8x96.clock and ispif_res_8x96.clock_for_reset.

FWIW the ancient msm-3.18 doesn't reference the VFE clocks in CSIPHY.

Anyway, looks like the issue here is that we're not toggling the
GDSC early enough in cases where something that's not VFE needs it.

> 
> Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?
Since both VFE GDSCs are children of CAMSS_GDSC and (as mentioned in the
commit message) the power sequencing used to be different, it just seems
to me like we've been piggybacking on lucky ordering since the introduction
of 8996 support.

For comparison, 8916 doesn't define it because it doesn't have it and newer
SoCs use TITAN.

SDM630 doesn't define it, but nobody touched it since like 2021 (except
Dmitry's fixups when he got his hands on the inforce baord) so it's
probably broken as well..


Konrad
> 
> ---
> bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 26/05/2023 22:28, Konrad Dybcio wrote:
> 
> 
> On 26.05.2023 23:17, Bryan O'Donoghue wrote:
>> On 26/05/2023 21:57, Konrad Dybcio wrote:
>>> This code contains a whole bunch of hacky counting logic that should have
>>> been substituted with _byname, but now we're stuck with indices to keep
>>> compatibility with old DTs :/
>>>
>>> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
>>> CAMSS-related GDSCs, we could make it their parent in the clock driver
>>> and call it a day.
>>
>> I mean, it wouldn't make much sense from a hw design POV if that weren't the case..
>>
>> Hmm looks like its already there.
>>
>> static struct gdsc vfe0_gdsc = {
>>          .gdscr = 0x3664,
>>          .cxcs = (unsigned int []){ 0x36a8 },
>>          .cxc_count = 1,
>>          .pd = {
>>                  .name = "vfe0",
>>          },
>>          .parent = &camss_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>> };
>>
>> static struct gdsc vfe1_gdsc = {
>>          .gdscr = 0x3674,
>>          .cxcs = (unsigned int []){ 0x36ac },
>>          .cxc_count = 1,
>>          .pd = {
>>                  .name = "vfe1",
>>          },
>>          .parent = &camss_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>> };
>>
>> I feel this is probably a problem in the description of dependencies for the CSIPHY in the dts for the 8996..
>>
>> I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..
>>
>> static const struct resources csiphy_res_8x96[] = {
>>          /* CSIPHY0 */
>>          {
>>                  .regulators = {},
>>                  .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer" },
>>
>>
>> should probably look something like
>>
>> static const struct resources csiphy_res_8x96[] = {
>>          /* CSIPHY0 */
>>          {
>>                  .regulators = {},
>>                  .clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer", "vfe0"},
>>
>> But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC via the VFEx
> I have little idea how CAMSS is tied together, but the VFEn clocks
> are assigned in vfe_res_8x96.clock and ispif_res_8x96.clock_for_reset.
> 
> FWIW the ancient msm-3.18 doesn't reference the VFE clocks in CSIPHY.
> 
> Anyway, looks like the issue here is that we're not toggling the
> GDSC early enough in cases where something that's not VFE needs it.
> 
>>
>> Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?
> Since both VFE GDSCs are children of CAMSS_GDSC and (as mentioned in the
> commit message) the power sequencing used to be different, it just seems
> to me like we've been piggybacking on lucky ordering since the introduction
> of 8996 support.
> 
> For comparison, 8916 doesn't define it because it doesn't have it and newer
> SoCs use TITAN.
> 
> SDM630 doesn't define it, but nobody touched it since like 2021 (except
> Dmitry's fixups when he got his hands on the inforce baord) so it's
> probably broken as well..
> 
> 
> Konrad
>>
>> ---
>> bod

Hmm, so what I suggested is what Yassine has i.e. adds <&mmcc 
CAMSS_GDSC>; to the top-level camss node.

Without testing, this _looks_ right to me. I just think, like Conor 
flagged we don't need to add a dependency on the power-domain names.

I don't quite know whats the reference for downstream you are looking at 
but, just generally it is possible to waggle the CSIPHY, VFE, IFE - my 
guess would be that CAMX @ the time of 8996 always had the CAMSS_GDSC or 
the VFEx - and hence the VFE -> CAMSS_GDSC on by the time the CSIPHY cod 
ran.

Anyway my comments here still stand.

- I don't think we need to nor should be counting power-domain names
- I do think we should be adding CAMSS_GDSC to the 8996 top-level CAMSS node

Yassine ? Can you take a stab at that ?

---
bod

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Yassine Oudjana]

On Fri, May 26 2023 at 09:49:30 PM +01:00:00, Bryan O'Donoghue 
<bryan.odonoghue@linaro.org> wrote:
> On 26/05/2023 19:07, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>> The CAMSS power domain was previously enabled implicitly when the VFE
>> power domains were enabled.
>> Commit 46cc03175498 ("media: camss: Split power domain management")
>> delayed enabling VFE power domains which in turn delayed enabling the
>> CAMSS power domain. This made CSIPHY fail to enable camss_top_ahb_clk
>> which requires the CAMSS power domain to be on:
>> 
>> [  199.097810] ------------[ cut here ]------------
>> [  199.097893] camss_top_ahb_clk status stuck at 'off'
>> [  199.097913] WARNING: CPU: 3 PID: 728 at 
>> drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x140/0x160
>> ...
>> [  199.100064]  clk_branch_wait+0x140/0x160
>> [  199.100112]  clk_branch2_enable+0x30/0x40
>> [  199.100159]  clk_core_enable+0x6c/0xb0
>> [  199.100211]  clk_enable+0x2c/0x50
>> [  199.100257]  camss_enable_clocks+0x94/0xe0 [qcom_camss]
>> [  199.100342]  csiphy_set_power+0x154/0x2a0 [qcom_camss]
>> ...
>> [  199.101594] ---[ end trace 0000000000000000 ]---
>> 
>> Link the CAMSS power domain in camss_configure_pd to make sure it 
>> gets
>> enabled before CSIPHY tries to enable clocks.
>> 
>> Fixes: 02afa816dbbf ("media: camss: Add basic runtime PM support")
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/media/platform/qcom/camss/camss.c 
>> b/drivers/media/platform/qcom/camss/camss.c
>> index 1ef26aea3eae..9aea8220d923 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1453,6 +1453,7 @@ static const struct media_device_ops 
>> camss_media_ops = {
>>   static int camss_configure_pd(struct camss *camss)
>>   {
>>   	struct device *dev = camss->dev;
>> +	int camss_pd_index;
>>   	int i;
>>   	int ret;
>>   \x7f@@ -1496,7 +1497,13 @@ static int camss_configure_pd(struct camss 
>> *camss)
>>   		}
>>   	}
>>   \x7f-	if (i > camss->vfe_num) {
>> +	/* Link CAMSS power domain if available */
>> +	camss_pd_index = device_property_match_string(camss->dev, 
>> "power-domain-names", "camss");
>> +	if (camss_pd_index >= 0)
>> +		device_link_add(camss->dev, camss->genpd[camss_pd_index], 
>> DL_FLAG_STATELESS |
>> +				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
>> +
>> +	if (i > camss->vfe_num && i != camss_pd_index) {
>>   		camss->genpd_link[i - 1] = device_link_add(camss->dev, 
>> camss->genpd[i - 1],
>>   							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>>   							   DL_FLAG_RPM_ACTIVE);
> 
> Konrad pointed this out.
> 
> Are you 100% sure you want to do this. We already have a way to count 
> the # of power-domains in camss_configure_pd().
> 
> Your series is now adding a dependency on power-domain-names.
> 
> Is there a good reason to add that dependency ? If not, then lets 
> just take the code from camss_configure_pd() and make it so that it 
> can be used/reused here.

Is there a good reason not to? I found that using the existing 
index-based method would unnecessarily complicate things since an extra 
layer of checks would be needed to differentiate between MSM8996 and 
TITAN SoCs, since those have the TITAN GDSC at the same index where the 
CAMSS GDSC is now added for MSM8996. The same checks will also have to 
be repeated in error paths and during cleanup.

I guessed the only reason we were still using this method for the 
existing PDs was to remain compatible with old DT as Konrad mentioned, 
and since this CAMSS PD is only added now, I thought it'd be a good 
opportunity to introduce power-domain-names and simplify things a bit.

> ---
> bod

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Yassine Oudjana]

On Fri, May 26 2023 at 10:36:32 PM +02:00:00, Konrad Dybcio 
<konrad.dybcio@linaro.org> wrote:
> 
> 
> On 26.05.2023 22:21, Bryan O'Donoghue wrote:
>>  On 26/05/2023 21:19, Conor Dooley wrote:
>>>  On Fri, May 26, 2023 at 09:05:47PM +0100, Bryan O'Donoghue wrote:
>>>>  On 26/05/2023 20:46, Conor Dooley wrote:
>>>>>>  +  - power-domain-names
>>>>>  Why is this now required?
>>>>> 
>>>>>  Thanks,
>>>>>  Conor.
>>>>> 
>>>> 
>>>>  Its an accurate description of the power/clock tree to have the 
>>>> top power
>>>>  domain be switched on prior to the clocks that depend on it.
>>> 
>>>  But what does that have to do with the *names* now being required?
>> 
>>  oh the names
>> 
>>  no toss that
> this should be
> 
> if:properties:compatible:blahblahmsm8996:then:required:power-domain-names

The only compatible in this binding is qcom,msm8996-camss, so what 
would this achieve?

> 
> Konrad
>> 
>>> 
>>>>  I think Yassine, you could probably include the majority of your
>>>>  cover-letter text in this commit to explain this change a bit 
>>>> better.
>>> 
>>>  I think it would be good to have that regardless.
>>> 
>>>  Cheers,
>>>  Conor.
>>> 
>>>>  bod
>>>     ^^^ I've been trying not to think about rugby since the weekend 
>>> :(
>> 
>>  Pockets O'Gara should have his paddy papers rescinded

Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain

[Bryan O'Donoghue]

On 27/05/2023 07:02, Yassine Oudjana wrote:
>> Konrad pointed this out.
>>
>> Are you 100% sure you want to do this. We already have a way to count 
>> the # of power-domains in camss_configure_pd().
>>
>> Your series is now adding a dependency on power-domain-names.
>>
>> Is there a good reason to add that dependency ? If not, then lets just 
>> take the code from camss_configure_pd() and make it so that it can be 
>> used/reused here.
> 
> Is there a good reason not to?I found that using the existing 
> index-based method would unnecessarily complicate things since an extra 
> layer of checks would be needed to differentiate between MSM8996 and 
> TITAN SoCs, since those have the TITAN GDSC at the same index where the 
> CAMSS GDSC is now added for MSM8996. The same checks will also have to 
> be repeated in error paths and during cleanup.
> 
> I guessed the only reason we were still using this method for the 
> existing PDs was to remain compatible with old DT as Konrad mentioned, 
> and since this CAMSS PD is only added now, I thought it'd be a good 
> opportunity to introduce power-domain-names and simplify things a bit.

I think actually I agree with you but, I don't think you've gone far 
enough with this patch.

Now that I look at this code a bit more, it looks like we need to place 
the TITAN/CAMSS GDSC last in the list of power-domains or the magic 
indices won't work. So my suggestion to you to place the CAMSS_GDSC in 
the power-domain list wouldn't work, unless it was the last entry,..

Having magic indices doesn't make much sense to me. Aside from anything 
else we don't document or require that indexing behavior in our 
Documentation.

In fact, I'm wondering what is the use case of a vfe_lite on its own - 
without the TITAN_TOP GDSC switched on ? I'm looking at the block 
diagram of the clocks for the sm8250 the IFE_LITE is buried well inside 
of a series of other components..

The reverse OTOH holds. Full fat VFE can be collapsed individually, 
which is why they have their own GDSCs...

OK, we should get away from magic indices ASAP.

This is a good find, thank you for bringing it up.

Could you take a named pointer for the CAMSS/TITAN instead of an index ?

camss->genpd_camss_top * =
camss->genpd_vfe[] =

These have a very obvious meaning. We can read a top-level struct camss 
{} and immediately understand what is meant, whereas index = 0 doesn't 
mean anything and isn't obvious from the code anyway.

1. You're right we should introduce some kind of naming to
    break the bonds of magic indices.

    So lets do as you suggest and name the power-domains.

    However we should refactor the code to drop magic indices.

2. If and only if named power-domains are absent, fall back on
    legacy indexing. In this case we will assume legacy indexing
    assigns to our new named pointers.

3. New CAMSS dts will need to have named power-domains as a result.

---
bod

Re: [PATCH v2 1/3] dt-bindings: media: camss: qcom,msm8996-camss: Add CAMSS power domain

[Konrad Dybcio]

On 27.05.2023 08:05, Yassine Oudjana wrote:
> 
> On Fri, May 26 2023 at 10:36:32 PM +02:00:00, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>> On 26.05.2023 22:21, Bryan O'Donoghue wrote:
>>>  On 26/05/2023 21:19, Conor Dooley wrote:
>>>>  On Fri, May 26, 2023 at 09:05:47PM +0100, Bryan O'Donoghue wrote:
>>>>>  On 26/05/2023 20:46, Conor Dooley wrote:
>>>>>>>  +  - power-domain-names
>>>>>>  Why is this now required?
>>>>>>
>>>>>>  Thanks,
>>>>>>  Conor.
>>>>>>
>>>>>
>>>>>  Its an accurate description of the power/clock tree to have the top power
>>>>>  domain be switched on prior to the clocks that depend on it.
>>>>
>>>>  But what does that have to do with the *names* now being required?
>>>
>>>  oh the names
>>>
>>>  no toss that
>> this should be
>>
>> if:properties:compatible:blahblahmsm8996:then:required:power-domain-names
> 
> The only compatible in this binding is qcom,msm8996-camss, so what would this achieve?
You're right - I didn't notice and assumed it was a camss-common one.

Konrad
> 
>>
>> Konrad
>>>
>>>>
>>>>>  I think Yassine, you could probably include the majority of your
>>>>>  cover-letter text in this commit to explain this change a bit better.
>>>>
>>>>  I think it would be good to have that regardless.
>>>>
>>>>  Cheers,
>>>>  Conor.
>>>>
>>>>>  bod
>>>>     ^^^ I've been trying not to think about rugby since the weekend :(
>>>
>>>  Pockets O'Gara should have his paddy papers rescinded
> 
>