[PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Dinh Nguyen]

Document the optional "altr,sysmgr-syscon" binding that is used to
access the System Manager register that controls the SDMMC clock
phase.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v4: add else statement
v3: document that the "altr,sysmgr-syscon" binding is only applicable to
    "altr,socfpga-dw-mshc"
v2: document "altr,sysmgr-syscon" in the MMC section
---
 .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
index ae6d6fca79e2..b73324273464 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
@@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Synopsys Designware Mobile Storage Host Controller Binding
 
-allOf:
-  - $ref: "synopsys-dw-mshc-common.yaml#"
-
 maintainers:
   - Ulf Hansson <ulf.hansson@linaro.org>
 
@@ -38,6 +35,34 @@ properties:
       - const: biu
       - const: ciu
 
+  altr,sysmgr-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to the sysmgr node
+          - description: register offset that controls the SDMMC clock phase
+    description:
+      Contains the phandle to System Manager block that contains
+      the SDMMC clock-phase control register. The first value is the pointer
+      to the sysmgr and the 2nd value is the register offset for the SDMMC
+      clock phase register.
+
+allOf:
+  - $ref: "synopsys-dw-mshc-common.yaml#"
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const:
+              - altr,socfpga-dw-mshc
+    then:
+      required:
+        - altr,sysmgr-syscon
+    else:
+      properties:
+        altr,sysmgr-syscon: false
+
 required:
   - compatible
   - reg
-- 
2.25.1

[PATCHv4 2/3] arm64: dts: socfpga: Add clk-phase-sd-hs property to the sdmmc node

[Dinh Nguyen]

The sdmmc controller's CIU(Card Interface Unit) clock's phase can be
adjusted through the register in the system manager. Add the binding
"altr,sysmgr-syscon" to the SDMMC node for the driver to access the
system manager. Add the "clk-phase-sd-hs" property in the SDMMC node to
designate the smpsel and drvsel properties for the CIU clock.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v4: no change
v3: removed unnecessary property in "altr,sysmgr-syscon"
---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi      | 1 +
 arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts | 1 +
 arch/arm64/boot/dts/intel/socfpga_agilex.dtsi          | 1 +
 arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts     | 1 +
 arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts        | 1 +
 5 files changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 14c220d87807..ff2906672deb 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -309,6 +309,7 @@ mmc: mmc@ff808000 {
 				 <&clkmgr STRATIX10_SDMMC_CLK>;
 			clock-names = "biu", "ciu";
 			iommus = <&smmu 5>;
+			altr,sysmgr-syscon = <&sysmgr 0x28>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
index 48424e459f12..19e7284b4cd5 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts
@@ -105,6 +105,7 @@ &mmc {
 	cap-mmc-highspeed;
 	broken-cd;
 	bus-width = <4>;
+	clk-phase-sd-hs = <0>, <135>;
 };
 
 &osc1 {
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
index 7bbec8aafa62..3f694d6fc338 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex.dtsi
@@ -313,6 +313,7 @@ mmc: mmc@ff808000 {
 				 <&clkmgr AGILEX_SDMMC_CLK>;
 			clock-names = "biu", "ciu";
 			iommus = <&smmu 5>;
+			altr,sysmgr-syscon = <&sysmgr 0x28>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
index 26cd3c121757..07c3f8876613 100644
--- a/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_agilex_socdk.dts
@@ -83,6 +83,7 @@ &mmc {
 	cap-sd-highspeed;
 	broken-cd;
 	bus-width = <4>;
+	clk-phase-sd-hs = <0>, <135>;
 };
 
 &osc1 {
diff --git a/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts b/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
index 62c66e52b656..08c088571270 100644
--- a/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
+++ b/arch/arm64/boot/dts/intel/socfpga_n5x_socdk.dts
@@ -74,6 +74,7 @@ &mmc {
 	cap-sd-highspeed;
 	broken-cd;
 	bus-width = <4>;
+	clk-phase-sd-hs = <0>, <135>;
 };
 
 &osc1 {
-- 
2.25.1

[PATCHv4 3/3] mmc: dw_mmc-pltfm: socfpga: add method to configure clk-phase

[Dinh Nguyen]

The clock-phase settings for the SDMMC controller in the SoCFPGA
Strarix10/Agilex/N5X platforms reside in a register in the System
Manager. Add a method to access that register through the syscon
interface.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v4: no change
v3: add space before &socfpga_drv_data
v2: simplify clk-phase calculations
---
 drivers/mmc/host/dw_mmc-pltfm.c | 41 ++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 9901208be797..0f07fa6d0150 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -17,10 +17,16 @@
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
 #include <linux/of.h>
+#include <linux/mfd/altera-sysmgr.h>
+#include <linux/regmap.h>
 
 #include "dw_mmc.h"
 #include "dw_mmc-pltfm.h"
 
+#define SOCFPGA_DW_MMC_CLK_PHASE_STEP	45
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
+	((((smplsel) & 0x7) << 4) | (((drvsel) & 0x7) << 0))
+
 int dw_mci_pltfm_register(struct platform_device *pdev,
 			  const struct dw_mci_drv_data *drv_data)
 {
@@ -62,9 +68,42 @@ const struct dev_pm_ops dw_mci_pltfm_pmops = {
 };
 EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
 
+static int dw_mci_socfpga_priv_init(struct dw_mci *host)
+{
+	struct device_node *np = host->dev->of_node;
+	struct regmap *sys_mgr_base_addr;
+	u32 clk_phase[2] = {0}, reg_offset;
+	int i, rc, hs_timing;
+
+	rc = of_property_read_variable_u32_array(np, "clk-phase-sd-hs", &clk_phase[0], 2, 0);
+	if (rc) {
+		sys_mgr_base_addr =
+			altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
+		if (IS_ERR(sys_mgr_base_addr)) {
+			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
+			return 1;
+		}
+	} else
+		return 1;
+
+	of_property_read_u32_index(np, "altr,sysmgr-syscon", 1, &reg_offset);
+
+	for (i = 0; i < ARRAY_SIZE(clk_phase); i++)
+		clk_phase[i] /= SOCFPGA_DW_MMC_CLK_PHASE_STEP;
+
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
+	regmap_write(sys_mgr_base_addr, reg_offset, hs_timing);
+
+	return 0;
+}
+
+static const struct dw_mci_drv_data socfpga_drv_data = {
+	.init		= dw_mci_socfpga_priv_init,
+};
+
 static const struct of_device_id dw_mci_pltfm_match[] = {
 	{ .compatible = "snps,dw-mshc", },
-	{ .compatible = "altr,socfpga-dw-mshc", },
+	{ .compatible = "altr,socfpga-dw-mshc", .data = &socfpga_drv_data, },
 	{ .compatible = "img,pistachio-dw-mshc", },
 	{},
 };
-- 
2.25.1

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 28/09/2022 18:54, Dinh Nguyen wrote:
> Document the optional "altr,sysmgr-syscon" binding that is used to
> access the System Manager register that controls the SDMMC clock
> phase.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---

Thank you for your patch. There is something to discuss/improve.

> +
> +allOf:
> +  - $ref: "synopsys-dw-mshc-common.yaml#"
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const:
> +              - altr,socfpga-dw-mshc

It still should not be an array, even if there is no warning.

> +    then:
> +      required:
> +        - altr,sysmgr-syscon
> +    else:
> +      properties:
> +        altr,sysmgr-syscon: false

Best regards,
Krzysztof

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Dinh Nguyen]

Hi,

On 9/28/22 12:15, Krzysztof Kozlowski wrote:
> On 28/09/2022 18:54, Dinh Nguyen wrote:
>> Document the optional "altr,sysmgr-syscon" binding that is used to
>> access the System Manager register that controls the SDMMC clock
>> phase.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +
>> +allOf:
>> +  - $ref: "synopsys-dw-mshc-common.yaml#"
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const:
>> +              - altr,socfpga-dw-mshc
> 
> It still should not be an array, even if there is no warning.
> 

I apologize, but I'm confused with the message. Do you mean it should 
not be a "const"?

Dinh

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Rob Herring]

On Wed, 28 Sep 2022 11:54:18 -0500, Dinh Nguyen wrote:
> Document the optional "altr,sysmgr-syscon" binding that is used to
> access the System Manager register that controls the SDMMC clock
> phase.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
> v4: add else statement
> v3: document that the "altr,sysmgr-syscon" binding is only applicable to
>     "altr,socfpga-dw-mshc"
> v2: document "altr,sysmgr-syscon" in the MMC section
> ---
>  .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml: allOf:1:if:properties:compatible:contains:const: ['altr,socfpga-dw-mshc'] is not of type 'integer', 'string'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml: ignoring, error in schema: allOf: 1: if: properties: compatible: contains: const
Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.example.dtb:0:0: /example-0/mmc@12200000: failed to match any schema with compatible: ['snps,dw-mshc']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 28/09/2022 20:37, Dinh Nguyen wrote:
> Hi,
> 
> On 9/28/22 12:15, Krzysztof Kozlowski wrote:
>> On 28/09/2022 18:54, Dinh Nguyen wrote:
>>> Document the optional "altr,sysmgr-syscon" binding that is used to
>>> access the System Manager register that controls the SDMMC clock
>>> phase.
>>>
>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +
>>> +allOf:
>>> +  - $ref: "synopsys-dw-mshc-common.yaml#"
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const:
>>> +              - altr,socfpga-dw-mshc
>>
>> It still should not be an array, even if there is no warning.
>>
> 
> I apologize, but I'm confused with the message. Do you mean it should 
> not be a "const"?

No, it should not be a const. Open any other schema and check how const
is done there.

Best regards,
Krzysztof

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Ulf Hansson]

On Wed, 28 Sept 2022 at 18:54, Dinh Nguyen <dinguyen@kernel.org> wrote:
>
> Document the optional "altr,sysmgr-syscon" binding that is used to
> access the System Manager register that controls the SDMMC clock
> phase.
>
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
> v4: add else statement
> v3: document that the "altr,sysmgr-syscon" binding is only applicable to
>     "altr,socfpga-dw-mshc"
> v2: document "altr,sysmgr-syscon" in the MMC section
> ---
>  .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> index ae6d6fca79e2..b73324273464 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>
>  title: Synopsys Designware Mobile Storage Host Controller Binding
>
> -allOf:
> -  - $ref: "synopsys-dw-mshc-common.yaml#"
> -
>  maintainers:
>    - Ulf Hansson <ulf.hansson@linaro.org>
>
> @@ -38,6 +35,34 @@ properties:
>        - const: biu
>        - const: ciu
>
> +  altr,sysmgr-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to the sysmgr node
> +          - description: register offset that controls the SDMMC clock phase
> +    description:
> +      Contains the phandle to System Manager block that contains
> +      the SDMMC clock-phase control register. The first value is the pointer
> +      to the sysmgr and the 2nd value is the register offset for the SDMMC
> +      clock phase register.
> +
> +allOf:
> +  - $ref: "synopsys-dw-mshc-common.yaml#"
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const:
> +              - altr,socfpga-dw-mshc
> +    then:
> +      required:
> +        - altr,sysmgr-syscon
> +    else:
> +      properties:
> +        altr,sysmgr-syscon: false

So this change will not be backwards compatible with existing DTBs. I
noticed that patch2 updates the DTS files for the arm64 platforms, but
there seems to be some arm32 platforms too. Isn't this going to be a
problem?

> +
>  required:
>    - compatible
>    - reg
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 29/09/2022 11:24, Ulf Hansson wrote:
> On Wed, 28 Sept 2022 at 18:54, Dinh Nguyen <dinguyen@kernel.org> wrote:
>>
>> Document the optional "altr,sysmgr-syscon" binding that is used to
>> access the System Manager register that controls the SDMMC clock
>> phase.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>> v4: add else statement
>> v3: document that the "altr,sysmgr-syscon" binding is only applicable to
>>     "altr,socfpga-dw-mshc"
>> v2: document "altr,sysmgr-syscon" in the MMC section
>> ---
>>  .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> index ae6d6fca79e2..b73324273464 100644
>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>>  title: Synopsys Designware Mobile Storage Host Controller Binding
>>
>> -allOf:
>> -  - $ref: "synopsys-dw-mshc-common.yaml#"
>> -
>>  maintainers:
>>    - Ulf Hansson <ulf.hansson@linaro.org>
>>
>> @@ -38,6 +35,34 @@ properties:
>>        - const: biu
>>        - const: ciu
>>
>> +  altr,sysmgr-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle to the sysmgr node
>> +          - description: register offset that controls the SDMMC clock phase
>> +    description:
>> +      Contains the phandle to System Manager block that contains
>> +      the SDMMC clock-phase control register. The first value is the pointer
>> +      to the sysmgr and the 2nd value is the register offset for the SDMMC
>> +      clock phase register.
>> +
>> +allOf:
>> +  - $ref: "synopsys-dw-mshc-common.yaml#"
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const:
>> +              - altr,socfpga-dw-mshc
>> +    then:
>> +      required:
>> +        - altr,sysmgr-syscon
>> +    else:
>> +      properties:
>> +        altr,sysmgr-syscon: false
> 
> So this change will not be backwards compatible with existing DTBs. I
> noticed that patch2 updates the DTS files for the arm64 platforms, but
> there seems to be some arm32 platforms too. Isn't this going to be a
> problem?
> 

The backwards compatibility is actually expressed by the driver. If the
driver keeps ABI, we can change bindings so that all DTS are being
updated to pass the checks.

On the other hand, the commit should express why it changes the bindings
in incompatible way - this is lacking here.

Best regards,
Krzysztof

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Ulf Hansson]

On Thu, 29 Sept 2022 at 11:39, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/09/2022 11:24, Ulf Hansson wrote:
> > On Wed, 28 Sept 2022 at 18:54, Dinh Nguyen <dinguyen@kernel.org> wrote:
> >>
> >> Document the optional "altr,sysmgr-syscon" binding that is used to
> >> access the System Manager register that controls the SDMMC clock
> >> phase.
> >>
> >> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> >> ---
> >> v4: add else statement
> >> v3: document that the "altr,sysmgr-syscon" binding is only applicable to
> >>     "altr,socfpga-dw-mshc"
> >> v2: document "altr,sysmgr-syscon" in the MMC section
> >> ---
> >>  .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> >> index ae6d6fca79e2..b73324273464 100644
> >> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> >> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
> >> @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>
> >>  title: Synopsys Designware Mobile Storage Host Controller Binding
> >>
> >> -allOf:
> >> -  - $ref: "synopsys-dw-mshc-common.yaml#"
> >> -
> >>  maintainers:
> >>    - Ulf Hansson <ulf.hansson@linaro.org>
> >>
> >> @@ -38,6 +35,34 @@ properties:
> >>        - const: biu
> >>        - const: ciu
> >>
> >> +  altr,sysmgr-syscon:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >> +    items:
> >> +      - items:
> >> +          - description: phandle to the sysmgr node
> >> +          - description: register offset that controls the SDMMC clock phase
> >> +    description:
> >> +      Contains the phandle to System Manager block that contains
> >> +      the SDMMC clock-phase control register. The first value is the pointer
> >> +      to the sysmgr and the 2nd value is the register offset for the SDMMC
> >> +      clock phase register.
> >> +
> >> +allOf:
> >> +  - $ref: "synopsys-dw-mshc-common.yaml#"
> >> +
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const:
> >> +              - altr,socfpga-dw-mshc
> >> +    then:
> >> +      required:
> >> +        - altr,sysmgr-syscon
> >> +    else:
> >> +      properties:
> >> +        altr,sysmgr-syscon: false
> >
> > So this change will not be backwards compatible with existing DTBs. I
> > noticed that patch2 updates the DTS files for the arm64 platforms, but
> > there seems to be some arm32 platforms too. Isn't this going to be a
> > problem?
> >
>
> The backwards compatibility is actually expressed by the driver. If the
> driver keeps ABI, we can change bindings so that all DTS are being
> updated to pass the checks.

Right.

So, I should probably have responded to patch3 instead, as backwards
compatibility doesn't seem to be supported, unless I am mistaken. But
let's move the discussion over to that thread instead.

>
> On the other hand, the commit should express why it changes the bindings
> in incompatible way - this is lacking here.
>
> Best regards,
> Krzysztof
>

Kind regards
Uffe

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 29/09/2022 13:04, Ulf Hansson wrote:
>>> So this change will not be backwards compatible with existing DTBs. I
>>> noticed that patch2 updates the DTS files for the arm64 platforms, but
>>> there seems to be some arm32 platforms too. Isn't this going to be a
>>> problem?
>>>
>>
>> The backwards compatibility is actually expressed by the driver. If the
>> driver keeps ABI, we can change bindings so that all DTS are being
>> updated to pass the checks.
> 
> Right.
> 
> So, I should probably have responded to patch3 instead, as backwards
> compatibility doesn't seem to be supported, unless I am mistaken. 

Yes, it looks like

Best regards,
Krzysztof

Re: [PATCHv4 3/3] mmc: dw_mmc-pltfm: socfpga: add method to configure clk-phase

[Ulf Hansson]

On Wed, 28 Sept 2022 at 18:54, Dinh Nguyen <dinguyen@kernel.org> wrote:
>
> The clock-phase settings for the SDMMC controller in the SoCFPGA
> Strarix10/Agilex/N5X platforms reside in a register in the System
> Manager. Add a method to access that register through the syscon
> interface.
>
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
> v4: no change
> v3: add space before &socfpga_drv_data
> v2: simplify clk-phase calculations
> ---
>  drivers/mmc/host/dw_mmc-pltfm.c | 41 ++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index 9901208be797..0f07fa6d0150 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -17,10 +17,16 @@
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/of.h>
> +#include <linux/mfd/altera-sysmgr.h>
> +#include <linux/regmap.h>
>
>  #include "dw_mmc.h"
>  #include "dw_mmc-pltfm.h"
>
> +#define SOCFPGA_DW_MMC_CLK_PHASE_STEP  45
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
> +       ((((smplsel) & 0x7) << 4) | (((drvsel) & 0x7) << 0))
> +
>  int dw_mci_pltfm_register(struct platform_device *pdev,
>                           const struct dw_mci_drv_data *drv_data)
>  {
> @@ -62,9 +68,42 @@ const struct dev_pm_ops dw_mci_pltfm_pmops = {
>  };
>  EXPORT_SYMBOL_GPL(dw_mci_pltfm_pmops);
>
> +static int dw_mci_socfpga_priv_init(struct dw_mci *host)
> +{
> +       struct device_node *np = host->dev->of_node;
> +       struct regmap *sys_mgr_base_addr;
> +       u32 clk_phase[2] = {0}, reg_offset;
> +       int i, rc, hs_timing;
> +
> +       rc = of_property_read_variable_u32_array(np, "clk-phase-sd-hs", &clk_phase[0], 2, 0);
> +       if (rc) {

This looks wrong, as rc may contain a negative error code,when there
is no clk-phase-sd-hs property found.

Instead, we probably want to check "if (rc < 0)" here instead, then
bail out, but without breaking backwards compatibility.

> +               sys_mgr_base_addr =
> +                       altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
> +               if (IS_ERR(sys_mgr_base_addr)) {
> +                       pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
> +                       return 1;
> +               }
> +       } else
> +               return 1;
> +
> +       of_property_read_u32_index(np, "altr,sysmgr-syscon", 1, &reg_offset);
> +
> +       for (i = 0; i < ARRAY_SIZE(clk_phase); i++)
> +               clk_phase[i] /= SOCFPGA_DW_MMC_CLK_PHASE_STEP;
> +
> +       hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
> +       regmap_write(sys_mgr_base_addr, reg_offset, hs_timing);
> +
> +       return 0;
> +}
> +
> +static const struct dw_mci_drv_data socfpga_drv_data = {
> +       .init           = dw_mci_socfpga_priv_init,
> +};
> +
>  static const struct of_device_id dw_mci_pltfm_match[] = {
>         { .compatible = "snps,dw-mshc", },
> -       { .compatible = "altr,socfpga-dw-mshc", },
> +       { .compatible = "altr,socfpga-dw-mshc", .data = &socfpga_drv_data, },
>         { .compatible = "img,pistachio-dw-mshc", },
>         {},
>  };
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Dinh Nguyen]

On 9/29/22 04:24, Ulf Hansson wrote:
> On Wed, 28 Sept 2022 at 18:54, Dinh Nguyen <dinguyen@kernel.org> wrote:
>>
>> Document the optional "altr,sysmgr-syscon" binding that is used to
>> access the System Manager register that controls the SDMMC clock
>> phase.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>> v4: add else statement
>> v3: document that the "altr,sysmgr-syscon" binding is only applicable to
>>      "altr,socfpga-dw-mshc"
>> v2: document "altr,sysmgr-syscon" in the MMC section
>> ---
>>   .../bindings/mmc/synopsys-dw-mshc.yaml        | 31 +++++++++++++++++--
>>   1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> index ae6d6fca79e2..b73324273464 100644
>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.yaml
>> @@ -6,9 +6,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>>   title: Synopsys Designware Mobile Storage Host Controller Binding
>>
>> -allOf:
>> -  - $ref: "synopsys-dw-mshc-common.yaml#"
>> -
>>   maintainers:
>>     - Ulf Hansson <ulf.hansson@linaro.org>
>>
>> @@ -38,6 +35,34 @@ properties:
>>         - const: biu
>>         - const: ciu
>>
>> +  altr,sysmgr-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle to the sysmgr node
>> +          - description: register offset that controls the SDMMC clock phase
>> +    description:
>> +      Contains the phandle to System Manager block that contains
>> +      the SDMMC clock-phase control register. The first value is the pointer
>> +      to the sysmgr and the 2nd value is the register offset for the SDMMC
>> +      clock phase register.
>> +
>> +allOf:
>> +  - $ref: "synopsys-dw-mshc-common.yaml#"
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const:
>> +              - altr,socfpga-dw-mshc
>> +    then:
>> +      required:
>> +        - altr,sysmgr-syscon
>> +    else:
>> +      properties:
>> +        altr,sysmgr-syscon: false
> 
> So this change will not be backwards compatible with existing DTBs. I
> noticed that patch2 updates the DTS files for the arm64 platforms, but
> there seems to be some arm32 platforms too. Isn't this going to be a
> problem?
> 

The arm32 platforms makes the clk-phase adjustment through the clock 
driver. There was a discussion when I originally submitted the support 
for the arm32 platforms, and we landed on going through the clock driver 
instead of using the MMC driver. The updates to the arm32 platforms can 
be done after this patch series.

Dinh

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 29/09/2022 16:20, Dinh Nguyen wrote:
>>
>> So this change will not be backwards compatible with existing DTBs. I
>> noticed that patch2 updates the DTS files for the arm64 platforms, but
>> there seems to be some arm32 platforms too. Isn't this going to be a
>> problem?
>>
> 
> The arm32 platforms makes the clk-phase adjustment through the clock 
> driver. There was a discussion when I originally submitted the support 
> for the arm32 platforms, and we landed on going through the clock driver 
> instead of using the MMC driver. The updates to the arm32 platforms can 
> be done after this patch series.

How the update "can be done after"? Didn't you break all boards in- and
out-of-tree?

Best regards,
Krzysztof

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Dinh Nguyen]

On 9/29/22 09:38, Krzysztof Kozlowski wrote:
> On 29/09/2022 16:20, Dinh Nguyen wrote:
>>>
>>> So this change will not be backwards compatible with existing DTBs. I
>>> noticed that patch2 updates the DTS files for the arm64 platforms, but
>>> there seems to be some arm32 platforms too. Isn't this going to be a
>>> problem?
>>>
>>
>> The arm32 platforms makes the clk-phase adjustment through the clock
>> driver. There was a discussion when I originally submitted the support
>> for the arm32 platforms, and we landed on going through the clock driver
>> instead of using the MMC driver. The updates to the arm32 platforms can
>> be done after this patch series.
> 
> How the update "can be done after"? Didn't you break all boards in- and
> out-of-tree?
> 

I don't think so! At least, I don't see how, for the arm32 boards, here 
are the dts entry for setting the clock-phase:

sdmmc_clk: sdmmc_clk {
	#clock-cells = <0>;
	compatible = "altr,socfpga-gate-clk";
	clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>,<&per_nand_mmc_clk>;
	clk-gate = <0xa0 8>;
	clk-phase = <0 135>;   <-----
};

sdmmc_clk_divided: sdmmc_clk_divided {
	#clock-cells = <0>;
	compatible = "altr,socfpga-gate-clk";
	clocks = <&sdmmc_clk>;
	clk-gate = <0xa0 8>;
	fixed-divider = <4>;
	};

...
mmc: dwmmc0@ff704000 {
	compatible = "altr,socfpga-dw-mshc";
	reg = <0xff704000 0x1000>;
	interrupts = <0 139 4>;
	fifo-depth = <0x400>;
	#address-cells = <1>;
	#size-cells = <0>;
	clocks = <&l4_mp_clk>, <&sdmmc_clk_divided>;
	clock-names = "biu", "ciu";
	resets = <&rst SDMMC_RESET>;
	status = "disabled";
	};


So the setting for the clk-phase is done in the clock driver, 
(drivers/clk/socfpga/clk-gate.c). This has been done many years now, 
before the clk-phase-hs-sd concept was added to the sdmmc driver.

When I originally submitted the patches for the ARM64 clock driver 
support, I forgot to add the clk-phase support for the SD controller. 
Now that I realized we needed it, the concept to set the clk-phase is in 
the SD driver, thus I'm just adding the support for arm64.

The arm32 support does not change in any way, so I don't see how it will 
break it.

I can update the arm32 support with the same function in patch3 after 
this series. Because updating the arm32 will require me to remove the 
support in the clock driver, thus, I want to break it out.

Dinh

Re: [PATCHv4 1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon"

[Krzysztof Kozlowski]

On 29/09/2022 17:18, Dinh Nguyen wrote:
> 
> 
> On 9/29/22 09:38, Krzysztof Kozlowski wrote:
>> On 29/09/2022 16:20, Dinh Nguyen wrote:
>>>>
>>>> So this change will not be backwards compatible with existing DTBs. I
>>>> noticed that patch2 updates the DTS files for the arm64 platforms, but
>>>> there seems to be some arm32 platforms too. Isn't this going to be a
>>>> problem?
>>>>
>>>
>>> The arm32 platforms makes the clk-phase adjustment through the clock
>>> driver. There was a discussion when I originally submitted the support
>>> for the arm32 platforms, and we landed on going through the clock driver
>>> instead of using the MMC driver. The updates to the arm32 platforms can
>>> be done after this patch series.
>>
>> How the update "can be done after"? Didn't you break all boards in- and
>> out-of-tree?
>>
> 
> I don't think so! At least, I don't see how, for the arm32 boards, here 
> are the dts entry for setting the clock-phase:
> 
> sdmmc_clk: sdmmc_clk {
> 	#clock-cells = <0>;
> 	compatible = "altr,socfpga-gate-clk";
> 	clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>,<&per_nand_mmc_clk>;
> 	clk-gate = <0xa0 8>;
> 	clk-phase = <0 135>;   <-----

It's different node...

> };
> 
> sdmmc_clk_divided: sdmmc_clk_divided {
> 	#clock-cells = <0>;
> 	compatible = "altr,socfpga-gate-clk";
> 	clocks = <&sdmmc_clk>;
> 	clk-gate = <0xa0 8>;
> 	fixed-divider = <4>;
> 	};
> 
> ...
> mmc: dwmmc0@ff704000 {
> 	compatible = "altr,socfpga-dw-mshc";
> 	reg = <0xff704000 0x1000>;
> 	interrupts = <0 139 4>;
> 	fifo-depth = <0x400>;
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	clocks = <&l4_mp_clk>, <&sdmmc_clk_divided>;
> 	clock-names = "biu", "ciu";
> 	resets = <&rst SDMMC_RESET>;
> 	status = "disabled";

And this one does not have clk-phase-sd-hs

> 	};
> 
> 
> So the setting for the clk-phase is done in the clock driver, 
> (drivers/clk/socfpga/clk-gate.c). This has been done many years now, 
> before the clk-phase-hs-sd concept was added to the sdmmc driver.

Yes and the driver now requires clk-phase-sd-hs or altr,sysmgr-syscon
which is not present in DTS.

> 
> When I originally submitted the patches for the ARM64 clock driver 
> support, I forgot to add the clk-phase support for the SD controller. 
> Now that I realized we needed it, the concept to set the clk-phase is in 
> the SD driver, thus I'm just adding the support for arm64.
> 
> The arm32 support does not change in any way, so I don't see how it will 
> break it.

Isn't your driver returning ERRNO for all existing DTS (so without patch
#2) and for all out of tree DTS?

> 
> I can update the arm32 support with the same function in patch3 after 
> this series. Because updating the arm32 will require me to remove the 
> support in the clock driver, thus, I want to break it out.


Best regards,
Krzysztof