linux-kernel - Re: [PATCH v3 1/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Nov 2020 15:12:17 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Andrew Jeffery <andrew@...id.au>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Joel Stanley <joel@....id.au>,
        Adrian Hunter <adrian.hunter@...el.com>,
        DTML <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-aspeed <linux-aspeed@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ryan_chen@...eedtech.com
Subject: Re: [PATCH v3 1/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

On Mon, 23 Nov 2020 at 07:30, Andrew Jeffery <andrew@...id.au> wrote:
>
> The Aspeed SD/eMMC controllers feature up to two SDHCIs alongside a
> a set of "global" configuration registers. The global configuration
> registers house controller-specific settings that aren't exposed by the
> SDHCI, one example being a register for phase tuning.
>
> The phase tuning feature is new in the AST2600 design. It's exposed as a
> single register in the global register set and controls both the input
> and output phase adjustment for each slot. As the settings are
> slot-specific, the values to program are extracted from properties in
> the SDHCI devicetree nodes.
>
> Signed-off-by: Andrew Jeffery <andrew@...id.au>

[...]

>
> +static void
> +aspeed_sdhci_of_parse_phase(struct device_node *np, const char *prop,
> +                           struct aspeed_sdhci_phase_param *phase)
> +{
> +       int degrees[2] = {0};
> +       int rc;
> +
> +       rc = of_property_read_variable_u32_array(np, prop, degrees, 2, 0);
> +       phase->set = rc == 2;
> +       if (phase->set) {
> +               phase->in_deg = degrees[0];
> +               phase->out_deg = degrees[1];
> +       }
> +}
> +
> +static int aspeed_sdhci_of_parse(struct platform_device *pdev,
> +                                struct aspeed_sdhci *sdhci)
> +{
> +       struct device_node *np;
> +       struct device *dev;
> +
> +       if (!sdhci->phase_desc)
> +               return 0;
> +
> +       dev = &pdev->dev;
> +       np = dev->of_node;
> +
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-legacy",
> +                                   &sdhci->phase_param[MMC_TIMING_LEGACY]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-sd-hs",
> +                                   &sdhci->phase_param[MMC_TIMING_SD_HS]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr12",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR12]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr25",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR25]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr50",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR50]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr104",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR104]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-ddr50",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_DDR50]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-ddr52",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_DDR52]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs200",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS200]);
> +
> +       return 0;
> +}

If it's not too much to ask, would you mind adding a helper function
to the mmc core, as to let us avoid open coding? Then we should be
able to move the sdhci-of-arasan driver to use this as well.

Perhaps the definition of the helper could look something like this:
int mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase
*phases) (or something along those lines)

I think the struct mmc_clk_phase could be something that is stored in
the host specific struct, rather than in the common struct mmc_host
(to avoid sprinkle it with unnecessary data).

Moreover, we should probably use the device_property_* APIs instead of
the DT specific of_property_*.

[...]

Kind regards
Uffe