| Message ID | 20220919181309.286611-2-dinguyen@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] arm64: dts: socfpga: Add clk-phase-sd-hs property to the sdmmc node | expand |
On Mon, 19 Sept 2022 at 20:13, 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> > --- > drivers/mmc/host/dw_mmc-pltfm.c | 68 ++++++++++++++++++++++++++++++++- > 1 file changed, 67 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c > index 9901208be797..9e3237c18a9d 100644 > --- a/drivers/mmc/host/dw_mmc-pltfm.c > +++ b/drivers/mmc/host/dw_mmc-pltfm.c > @@ -17,10 +17,15 @@ > #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 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 +67,70 @@ 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); This needs to be documented through updated DT bindings. > + if (rc) { > + sys_mgr_base_addr = > + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); DT bindings? > + 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); DT bindings? > + > + for (i = 0; i < ARRAY_SIZE(clk_phase); i++) { > + switch (clk_phase[i]) { > + case 0: > + clk_phase[i] = 0; > + break; > + case 45: > + clk_phase[i] = 1; > + break; > + case 90: > + clk_phase[i] = 2; > + break; > + case 135: > + clk_phase[i] = 3; > + break; > + case 180: > + clk_phase[i] = 4; > + break; > + case 225: > + clk_phase[i] = 5; > + break; > + case 270: > + clk_phase[i] = 6; > + break; > + case 315: > + clk_phase[i] = 7; > + break; > + default: > + clk_phase[i] = 0; > + break; > + } > + } In my opinion, it looks like converting to use mmc_of_parse_clk_phase() should be able to avoid some of the open coding above. If you need some inspiration of how to implement this, you may have a look at drivers/mmc/host/sdhci-of-aspeed.c > + 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
Hi Ulf, Thanks for the review! On 9/20/22 07:17, Ulf Hansson wrote: > On Mon, 19 Sept 2022 at 20:13, 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> >> --- >> drivers/mmc/host/dw_mmc-pltfm.c | 68 ++++++++++++++++++++++++++++++++- >> 1 file changed, 67 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c >> index 9901208be797..9e3237c18a9d 100644 >> --- a/drivers/mmc/host/dw_mmc-pltfm.c >> +++ b/drivers/mmc/host/dw_mmc-pltfm.c >> @@ -17,10 +17,15 @@ >> #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 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 +67,70 @@ 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); > > This needs to be documented through updated DT bindings. Ok, but it looks like clk-phase-sd-hs is already documented in Documentation/devicetree/bindings/mmc/mmc-controller.yaml Should I create a specific documentation just for "altr,socfpga-dw-mshc" and document "clk-phase-sd-hs"? > >> + if (rc) { >> + sys_mgr_base_addr = >> + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > > DT bindings? "altr,sysmgr-syscon" has already been documented in Documentation/devicetree/bindings/net/socfpga-dwmac.txt >> + 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); > > DT bindings? Same... > >> + >> + for (i = 0; i < ARRAY_SIZE(clk_phase); i++) { >> + switch (clk_phase[i]) { >> + case 0: >> + clk_phase[i] = 0; >> + break; >> + case 45: >> + clk_phase[i] = 1; >> + break; >> + case 90: >> + clk_phase[i] = 2; >> + break; >> + case 135: >> + clk_phase[i] = 3; >> + break; >> + case 180: >> + clk_phase[i] = 4; >> + break; >> + case 225: >> + clk_phase[i] = 5; >> + break; >> + case 270: >> + clk_phase[i] = 6; >> + break; >> + case 315: >> + clk_phase[i] = 7; >> + break; >> + default: >> + clk_phase[i] = 0; >> + break; >> + } >> + } > > In my opinion, it looks like converting to use > mmc_of_parse_clk_phase() should be able to avoid some of the open > coding above. > > If you need some inspiration of how to implement this, you may have a > look at drivers/mmc/host/sdhci-of-aspeed.c > Got it. >> + 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 Dinh
On 20/09/2022 15:24, Dinh Nguyen wrote: > > Hi Ulf, > > Thanks for the review! > > On 9/20/22 07:17, Ulf Hansson wrote: >> On Mon, 19 Sept 2022 at 20:13, 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> >>> --- >>> drivers/mmc/host/dw_mmc-pltfm.c | 68 ++++++++++++++++++++++++++++++++- >>> 1 file changed, 67 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c >>> index 9901208be797..9e3237c18a9d 100644 >>> --- a/drivers/mmc/host/dw_mmc-pltfm.c >>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c >>> @@ -17,10 +17,15 @@ >>> #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 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 +67,70 @@ 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); >> >> This needs to be documented through updated DT bindings. > > Ok, but it looks like clk-phase-sd-hs is already documented in > Documentation/devicetree/bindings/mmc/mmc-controller.yaml Not in next-20220919. > > Should I create a specific documentation just for > "altr,socfpga-dw-mshc" and document "clk-phase-sd-hs"? All properties must be documented. > >> >>> + if (rc) { >>> + sys_mgr_base_addr = >>> + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); >> >> DT bindings? > > "altr,sysmgr-syscon" has already been documented in > Documentation/devicetree/bindings/net/socfpga-dwmac.txt This is not documentation of nodes you are changing here and in patch 1. You linked altr,socfpga-stmmac and here you have altr,socfpga-dw-mshc... Best regards, Krzysztof
On Tue, 20 Sept 2022 at 17:19, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 20/09/2022 15:24, Dinh Nguyen wrote: > > > > Hi Ulf, > > > > Thanks for the review! > > > > On 9/20/22 07:17, Ulf Hansson wrote: > >> On Mon, 19 Sept 2022 at 20:13, 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> > >>> --- > >>> drivers/mmc/host/dw_mmc-pltfm.c | 68 ++++++++++++++++++++++++++++++++- > >>> 1 file changed, 67 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c > >>> index 9901208be797..9e3237c18a9d 100644 > >>> --- a/drivers/mmc/host/dw_mmc-pltfm.c > >>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c > >>> @@ -17,10 +17,15 @@ > >>> #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 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 +67,70 @@ 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); > >> > >> This needs to be documented through updated DT bindings. > > > > Ok, but it looks like clk-phase-sd-hs is already documented in > > Documentation/devicetree/bindings/mmc/mmc-controller.yaml > > Not in next-20220919. Dinh is right! It seems like both me and Krzysztof missed the already documented binding. Probably because the property is named like below and that I did "git grep clk-phase-sd" :-) "^clk-phase-(legacy|sd-hs|mmc-(hs|hs[24]00|ddr52)|uhs-(sdr(12|25|50|104)|ddr50))$": > > > > > Should I create a specific documentation just for > > "altr,socfpga-dw-mshc" and document "clk-phase-sd-hs"? > > All properties must be documented. Yes, but as stated above, we should be okay in this case. > > > > >> > >>> + if (rc) { > >>> + sys_mgr_base_addr = > >>> + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > >> > >> DT bindings? > > > > "altr,sysmgr-syscon" has already been documented in > > Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > This is not documentation of nodes you are changing here and in patch 1. > > You linked altr,socfpga-stmmac and here you have altr,socfpga-dw-mshc... Right. I guess an option is to convert Documentation/devicetree/bindings/net/socfpga-dwmac.txt into the yaml based format and then reference that binding from synopsys-dw-mshc-common.yaml? > > Best regards, > Krzysztof Kind regards Uffe
On 21/09/2022 12:31, Ulf Hansson wrote: >> Not in next-20220919. > > Dinh is right! > > It seems like both me and Krzysztof missed the already documented > binding. Probably because the property is named like below and that I > did "git grep clk-phase-sd" :-) > > "^clk-phase-(legacy|sd-hs|mmc-(hs|hs[24]00|ddr52)|uhs-(sdr(12|25|50|104)|ddr50))$": Too much trust in git grep. Thanks for finding it. > >> >>> >>> Should I create a specific documentation just for >>> "altr,socfpga-dw-mshc" and document "clk-phase-sd-hs"? >> >> All properties must be documented. > > Yes, but as stated above, we should be okay in this case. > >> >>> >>>> >>>>> + if (rc) { >>>>> + sys_mgr_base_addr = >>>>> + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); >>>> >>>> DT bindings? >>> >>> "altr,sysmgr-syscon" has already been documented in >>> Documentation/devicetree/bindings/net/socfpga-dwmac.txt >> >> This is not documentation of nodes you are changing here and in patch 1. >> >> You linked altr,socfpga-stmmac and here you have altr,socfpga-dw-mshc... > > Right. > > I guess an option is to convert > Documentation/devicetree/bindings/net/socfpga-dwmac.txt into the yaml > based format and then reference that binding from > synopsys-dw-mshc-common.yaml? I did not look much inside, but isn't them entirely different devices (net vs mmc)? If they are different, then such vendor-custom property needs to appear in each bindings. The same as we have for other syscon-like properties. Best regards, Krzysztof
On Wed, 21 Sept 2022 at 13:13, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/09/2022 12:31, Ulf Hansson wrote: > >> Not in next-20220919. > > > > Dinh is right! > > > > It seems like both me and Krzysztof missed the already documented > > binding. Probably because the property is named like below and that I > > did "git grep clk-phase-sd" :-) > > > > "^clk-phase-(legacy|sd-hs|mmc-(hs|hs[24]00|ddr52)|uhs-(sdr(12|25|50|104)|ddr50))$": > > Too much trust in git grep. Thanks for finding it. > > > > >> > >>> > >>> Should I create a specific documentation just for > >>> "altr,socfpga-dw-mshc" and document "clk-phase-sd-hs"? > >> > >> All properties must be documented. > > > > Yes, but as stated above, we should be okay in this case. > > > >> > >>> > >>>> > >>>>> + if (rc) { > >>>>> + sys_mgr_base_addr = > >>>>> + altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > >>>> > >>>> DT bindings? > >>> > >>> "altr,sysmgr-syscon" has already been documented in > >>> Documentation/devicetree/bindings/net/socfpga-dwmac.txt > >> > >> This is not documentation of nodes you are changing here and in patch 1. > >> > >> You linked altr,socfpga-stmmac and here you have altr,socfpga-dw-mshc... > > > > Right. > > > > I guess an option is to convert > > Documentation/devicetree/bindings/net/socfpga-dwmac.txt into the yaml > > based format and then reference that binding from > > synopsys-dw-mshc-common.yaml? > > I did not look much inside, but isn't them entirely different devices > (net vs mmc)? If they are different, then such vendor-custom property > needs to appear in each bindings. The same as we have for other > syscon-like properties. I was thinking that it was a specific binding for the syscon device that could be shared among its consumers. But it isn't. So, you are definitely right, it seems better to have a vendor-custom property defined in the mmc bindings for this. Thanks for your feedback! > > Best regards, > Krzysztof > Kind regards Uffe
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c index 9901208be797..9e3237c18a9d 100644 --- a/drivers/mmc/host/dw_mmc-pltfm.c +++ b/drivers/mmc/host/dw_mmc-pltfm.c @@ -17,10 +17,15 @@ #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 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 +67,70 @@ 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, ®_offset); + + for (i = 0; i < ARRAY_SIZE(clk_phase); i++) { + switch (clk_phase[i]) { + case 0: + clk_phase[i] = 0; + break; + case 45: + clk_phase[i] = 1; + break; + case 90: + clk_phase[i] = 2; + break; + case 135: + clk_phase[i] = 3; + break; + case 180: + clk_phase[i] = 4; + break; + case 225: + clk_phase[i] = 5; + break; + case 270: + clk_phase[i] = 6; + break; + case 315: + clk_phase[i] = 7; + break; + default: + clk_phase[i] = 0; + break; + } + } + 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", }, {}, };
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> --- drivers/mmc/host/dw_mmc-pltfm.c | 68 ++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-)