| Message ID | 20221019170657.68014-1-dinguyen@kernel.org |
|---|---|
| Headers | show |
| Series | arm: socfpga: use clk-phase-sd-hs | expand |
On Wed, Oct 19, 2022 at 06:31:53PM -0500, Rob Herring wrote: > On Wed, 19 Oct 2022 12:06:52 -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> > > --- > > v5: document reg shift > > 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 | 32 +++++++++++++++++-- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > Running 'make dtbs_check' with the schema in this patch gives the > following warnings. Consider if they are expected or the schema is > incorrect. These may not be new warnings. > > Note that it is not yet a requirement to have 0 warnings for dtbs_check. > This will change in the future. > > Full log is available here: https://patchwork.ozlabs.org/patch/ > > > dwmmc0@ff704000: $nodename:0: 'dwmmc0@ff704000' does not match '^mmc(@.*)?$' Not necessary for this series, but it would be nice if existing warnings were fixed before adding new things. Especially since most of the warnings on this common bindings are all socfpga. It may become required at some point, not just nice. The node name is the cause of most/all the unevaluated property warnings. > arch/arm/boot/dts/socfpga_arria5_socdk.dtb > arch/arm/boot/dts/socfpga_cyclone5_chameleon96.dtb > arch/arm/boot/dts/socfpga_cyclone5_de0_nano_soc.dtb > arch/arm/boot/dts/socfpga_cyclone5_mcvevk.dtb > arch/arm/boot/dts/socfpga_cyclone5_socdk.dtb > arch/arm/boot/dts/socfpga_cyclone5_sockit.dtb > arch/arm/boot/dts/socfpga_cyclone5_socrates.dtb > arch/arm/boot/dts/socfpga_cyclone5_sodia.dtb > arch/arm/boot/dts/socfpga_cyclone5_vining_fpga.dtb > arch/arm/boot/dts/socfpga_vt.dtb > > dwmmc0@ff704000: 'altr,sysmgr-syscon' is a required property > arch/arm/boot/dts/socfpga_arria5_socdk.dtb > arch/arm/boot/dts/socfpga_cyclone5_chameleon96.dtb > arch/arm/boot/dts/socfpga_cyclone5_de0_nano_soc.dtb > arch/arm/boot/dts/socfpga_cyclone5_mcvevk.dtb > arch/arm/boot/dts/socfpga_cyclone5_socdk.dtb > arch/arm/boot/dts/socfpga_cyclone5_sockit.dtb > arch/arm/boot/dts/socfpga_cyclone5_socrates.dtb > arch/arm/boot/dts/socfpga_cyclone5_sodia.dtb I thought it was optional? New required properties are a possible ABI break. Rob
On 19/10/2022 13:06, Dinh Nguyen wrote: > The clock-phase settings for the SDMMC controller in the SoCFPGA > 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> > --- > v5: change error handling from of_property_read_variable_u32_array() > support arm32 by reading the reg_shift > v4: no change > v3: add space before &socfpga_drv_data > v2: simplify clk-phase calculations > --- > drivers/mmc/host/dw_mmc-pltfm.c | 43 ++++++++++++++++++++++++++++++++- > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c > index 9901208be797..74421d13f466 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, reg_shift) \ > + ((((smplsel) & 0x7) << reg_shift) | (((drvsel) & 0x7) << 0)) > + > int dw_mci_pltfm_register(struct platform_device *pdev, > const struct dw_mci_drv_data *drv_data) > { > @@ -62,9 +68,44 @@ 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, reg_shift; > + int i, rc, hs_timing; > + > + rc = of_property_read_variable_u32_array(np, "clk-phase-sd-hs", &clk_phase[0], 2, 0); > + if (rc < 0) { > + dev_err(host->dev, "clk-phase-sd-hs not found!\n"); > + return rc; > + } > + > + sys_mgr_base_addr = altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > + if (IS_ERR(sys_mgr_base_addr)) { > + dev_err(host->dev, "failed to find altr,sys-mgr regmap!\n"); > + return -ENODEV; Isn't this now an ABI break? I have an impression we talked about this... Best regards, Krzysztof
On 10/21/22 08:32, Krzysztof Kozlowski wrote: > On 19/10/2022 13:06, Dinh Nguyen wrote: >> The clock-phase settings for the SDMMC controller in the SoCFPGA >> 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> >> --- >> v5: change error handling from of_property_read_variable_u32_array() >> support arm32 by reading the reg_shift >> v4: no change >> v3: add space before &socfpga_drv_data >> v2: simplify clk-phase calculations >> --- >> drivers/mmc/host/dw_mmc-pltfm.c | 43 ++++++++++++++++++++++++++++++++- >> 1 file changed, 42 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c >> index 9901208be797..74421d13f466 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, reg_shift) \ >> + ((((smplsel) & 0x7) << reg_shift) | (((drvsel) & 0x7) << 0)) >> + >> int dw_mci_pltfm_register(struct platform_device *pdev, >> const struct dw_mci_drv_data *drv_data) >> { >> @@ -62,9 +68,44 @@ 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, reg_shift; >> + int i, rc, hs_timing; >> + >> + rc = of_property_read_variable_u32_array(np, "clk-phase-sd-hs", &clk_phase[0], 2, 0); >> + if (rc < 0) { >> + dev_err(host->dev, "clk-phase-sd-hs not found!\n"); >> + return rc; >> + } >> + >> + sys_mgr_base_addr = altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); >> + if (IS_ERR(sys_mgr_base_addr)) { >> + dev_err(host->dev, "failed to find altr,sys-mgr regmap!\n"); >> + return -ENODEV; > > Isn't this now an ABI break? I have an impression we talked about this... > My fault, I'll make this optional. Dinh
Hi, Just wanted to address the comments regarding the dt-bindings document of "altr,sysmgr-syscon". I ran the 'make dt_binding_check' after: pip3 install dtschema --upgrade and I checked to make sure I have yamllint installed and I still don't see any warnings. I'm also confused about whether "altr,socfpga-dw-mshc" should be a const. I see the same usage in: Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml allOf: - $ref: "mmc-controller.yaml#" - if: properties: compatible: contains: const: arasan,sdhci-5.1 Please advise on how to address this comment! Thanks, Dinh Dinh Nguyen (6): dt-bindings: mmc: synopsys-dw-mshc: document "altr,sysmgr-syscon" arm64: dts: socfpga: Add clk-phase-sd-hs property to the sdmmc node arm: dts: socfpga: Add clk-phase-sd-hs property to the sdmmc node mmc: dw_mmc-pltfm: socfpga: add method to configure clk-phase clk: socfpga: remove the setting of clk-phase for sdmmc_clk arm: dts: socfpga: remove "clk-phase" in sdmmc_clk .../bindings/mmc/synopsys-dw-mshc.yaml | 32 ++++++++- arch/arm/boot/dts/socfpga.dtsi | 2 +- arch/arm/boot/dts/socfpga_arria10.dtsi | 2 +- .../boot/dts/socfpga_arria10_mercury_aa1.dtsi | 1 + .../boot/dts/socfpga_arria10_socdk_sdmmc.dts | 1 + arch/arm/boot/dts/socfpga_arria5.dtsi | 1 + arch/arm/boot/dts/socfpga_cyclone5.dtsi | 1 + arch/arm/boot/dts/socfpga_cyclone5_mcv.dtsi | 1 + .../boot/dts/altera/socfpga_stratix10.dtsi | 1 + .../dts/altera/socfpga_stratix10_socdk.dts | 1 + arch/arm64/boot/dts/intel/socfpga_agilex.dtsi | 1 + .../boot/dts/intel/socfpga_agilex_socdk.dts | 1 + .../boot/dts/intel/socfpga_n5x_socdk.dts | 1 + drivers/clk/socfpga/clk-gate-a10.c | 68 ------------------- drivers/clk/socfpga/clk-gate.c | 60 ---------------- drivers/clk/socfpga/clk.h | 1 - drivers/mmc/host/dw_mmc-pltfm.c | 43 +++++++++++- 17 files changed, 83 insertions(+), 135 deletions(-)