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

[Ulf Hansson]

On Tue, 20 Sept 2022 at 17:19, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>>   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