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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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