| Message ID | 20220928165420.1212284-1-dinguyen@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCHv4,1/3] dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon" | expand |
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
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
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.
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
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
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
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
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
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, ®_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
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
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
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
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
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
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(-)