Changes in v5:
- Rebased on next-20240612
- Tested again on MT8195 and MT8395
Changes in v4:
- Replaced Stanley Chu with myself in maintainers as his email is gone
- Fixed commit [5/8]
Changes in v3:
- Disallowed mt8192 compatible in isolation
- Added maxItems to clocks
Changes in v2:
- Rebased over next-20240409 (because of merge issue for patch 1)
- Added ufs: prefix to patch 1
- Added forgotten ufs-rx-symbol clock to the binding
This series performs some fixes and cleanups for the MediaTek UFSHCI
controller driver.
In particular, while adding the MT8195 compatible to the mediatek,ufs
binding, I noticed that it was allowing just one clock, completely
ignoring the optional ones, including the crypt-xxx clocks, all of
the optional regulators, and other properties.
Between all the other properties, two are completely useless, as they
are there just to activate features that, on SoCs that don't support
these, won't anyway be activated because of missing clocks or missing
regulators, or missing other properties;
as for the other vendor-specific properties, like ufs-disable-ah8,
ufs-broken-vcc, ufs-pmc-via-fastauto, since the current merge window
is closing, I didn't do extensive research so I've left them in place
but didn't add them to the devicetree binding yet.
The plan is to check those later and eventually give them a removal
treatment, or add them to the bindings in a part two series.
For now, at least, this is already a big improvement.
P.S.: The only SoC having UFSHCI upstream is MT8183, which only has
just one clock, and *nothing else* uses properties, clocks, etc that
were renamed in this cleanup.
Cheers!
AngeloGioacchino Del Regno (8):
scsi: ufs: ufs-mediatek: Remove useless mediatek,ufs-support-va09
property
scsi: ufs: ufs-mediatek: Fix property name for crypt boost voltage
scsi: ufs: ufs-mediatek: Remove useless mediatek,ufs-boost-crypt
property
scsi: ufs: ufs-mediatek: Avoid underscores in crypt clock names
dt-bindings: ufs: mediatek,ufs: Document MT8192 compatible with MT8183
dt-bindings: ufs: mediatek,ufs: Document MT8195 compatible
dt-bindings: ufs: mediatek,ufs: Document additional clocks
dt-bindings: ufs: mediatek,ufs: Document optional dvfsrc/va09
regulators
.../devicetree/bindings/ufs/mediatek,ufs.yaml | 29 +++++-
drivers/ufs/host/ufs-mediatek.c | 91 +++++++++++--------
2 files changed, 79 insertions(+), 41 deletions(-)
--
2.45.2
There is no need to have a property that activates the inline crypto
boost feature, as this needs many things: a regulator, three clocks,
and the mediatek,boost-crypt-microvolt property to be set.
If any one of these is missing, the feature won't be activated,
hence, it is useless to have yet one more property to enable that.
While at it, also address another two issues:
1. Give back the return value to the caller and make sure to fail
probing if we get an -EPROBE_DEFER or -ENOMEM; and
2. Free the ufs_mtk_crypt_cfg structure allocated in the crypto
boost function if said functionality could not be enabled because
it's not supported, as that'd be only wasted memory.
Last but not least, move the devm_kzalloc() call for ufs_mtk_crypt_cfg
to after getting the dvfsrc-vcore regulator and the boost microvolt
property, as if those fail there's no reason to even allocate that.
Fixes: ac8c2459091c ("scsi: ufs-mediatek: Decouple features from platform bindings")
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/ufs/host/ufs-mediatek.c | 55 ++++++++++++++++++---------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 23271eb1a244..8d0e7ea52541 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -576,51 +576,55 @@ static int ufs_mtk_init_host_clk(struct ufs_hba *hba, const char *name,
return ret;
}
-static void ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
+static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
{
struct ufs_mtk_host *host = ufshcd_get_variant(hba);
struct ufs_mtk_crypt_cfg *cfg;
struct device *dev = hba->dev;
struct regulator *reg;
u32 volt;
-
- host->crypt = devm_kzalloc(dev, sizeof(*(host->crypt)),
- GFP_KERNEL);
- if (!host->crypt)
- goto disable_caps;
+ int ret;
reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
if (IS_ERR(reg)) {
- dev_info(dev, "failed to get dvfsrc-vcore: %ld",
- PTR_ERR(reg));
- goto disable_caps;
+ ret = PTR_ERR(reg);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ return 0;
}
- if (of_property_read_u32(dev->of_node, "mediatek,boost-crypt-microvolt",
- &volt)) {
+ ret = of_property_read_u32(dev->of_node, "mediatek,boost-crypt-microvolt", &volt);
+ if (ret) {
dev_info(dev, "failed to get mediatek,boost-crypt-microvolt");
- goto disable_caps;
+ return 0;
}
+ host->crypt = devm_kzalloc(dev, sizeof(*host->crypt), GFP_KERNEL);
+ if (!host->crypt)
+ return -ENOMEM;
+
cfg = host->crypt;
- if (ufs_mtk_init_host_clk(hba, "crypt_mux",
- &cfg->clk_crypt_mux))
- goto disable_caps;
+ ret = ufs_mtk_init_host_clk(hba, "crypt_mux", &cfg->clk_crypt_mux);
+ if (ret)
+ goto out;
- if (ufs_mtk_init_host_clk(hba, "crypt_lp",
- &cfg->clk_crypt_lp))
- goto disable_caps;
+ ret = ufs_mtk_init_host_clk(hba, "crypt_lp", &cfg->clk_crypt_lp);
+ if (ret)
+ goto out;
- if (ufs_mtk_init_host_clk(hba, "crypt_perf",
- &cfg->clk_crypt_perf))
- goto disable_caps;
+ ret = ufs_mtk_init_host_clk(hba, "crypt_perf", &cfg->clk_crypt_perf);
+ if (ret)
+ goto out;
cfg->reg_vcore = reg;
cfg->vcore_volt = volt;
host->caps |= UFS_MTK_CAP_BOOST_CRYPT_ENGINE;
-disable_caps:
- return;
+out:
+ if (ret)
+ devm_kfree(dev, host->crypt);
+ return 0;
}
static int ufs_mtk_init_va09_pwr_ctrl(struct ufs_hba *hba)
@@ -649,8 +653,9 @@ static int ufs_mtk_init_host_caps(struct ufs_hba *hba)
struct device_node *np = hba->dev->of_node;
int ret;
- if (of_property_read_bool(np, "mediatek,ufs-boost-crypt"))
- ufs_mtk_init_boost_crypt(hba);
+ ret = ufs_mtk_init_boost_crypt(hba);
+ if (ret)
+ return ret;
ret = ufs_mtk_init_va09_pwr_ctrl(hba);
if (ret)
--
2.45.2
Change all of crypt_{mux,lp,perf} clock names to crypt-{mux,lp-perf}:
retaining compatibility with the old names is ignored as there is no
user of this driver declaring any of those clocks, and the binding
also doesn't allow these ones at all.
Fixes: 590b0d2372fe ("scsi: ufs-mediatek: Support performance mode for inline encryption engine")
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/ufs/host/ufs-mediatek.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 8d0e7ea52541..10a550e7e628 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -605,15 +605,15 @@ static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
return -ENOMEM;
cfg = host->crypt;
- ret = ufs_mtk_init_host_clk(hba, "crypt_mux", &cfg->clk_crypt_mux);
+ ret = ufs_mtk_init_host_clk(hba, "crypt-mux", &cfg->clk_crypt_mux);
if (ret)
goto out;
- ret = ufs_mtk_init_host_clk(hba, "crypt_lp", &cfg->clk_crypt_lp);
+ ret = ufs_mtk_init_host_clk(hba, "crypt-lp", &cfg->clk_crypt_lp);
if (ret)
goto out;
- ret = ufs_mtk_init_host_clk(hba, "crypt_perf", &cfg->clk_crypt_perf);
+ ret = ufs_mtk_init_host_clk(hba, "crypt-perf", &cfg->clk_crypt_perf);
if (ret)
goto out;
--
2.45.2
The MT8192 UFS controller is compatible with the MT8183 one:
document this by allowing to assign both compatible strings
"mediatek,mt8192-ufshci", "mediatek,mt8183-ufshci" to the UFSHCI node.
Moreover, since no MT8192 devicetree ever declared any UFSHCI node,
disallow specifying only the MT8192 compatible.
In preparation for adding MT8195 to the mix, the MT8192 compatible
was added as enum instead of const.
Also, while at it, replace Stanley Chu with me in the maintainers
field, as he is unreachable and his email isn't active anymore.
Acked-by: Conor Dooley <[email protected]>
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
.../devicetree/bindings/ufs/mediatek,ufs.yaml | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
index 32fd535a514a..f14887ea6fdc 100644
--- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
+++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
@@ -7,16 +7,19 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
title: Mediatek Universal Flash Storage (UFS) Controller
maintainers:
- - Stanley Chu <[email protected]>
+ - AngeloGioacchino Del Regno <[email protected]>
allOf:
- $ref: ufs-common.yaml
properties:
compatible:
- enum:
- - mediatek,mt8183-ufshci
- - mediatek,mt8192-ufshci
+ oneOf:
+ - const: mediatek,mt8183-ufshci
+ - items:
+ - enum:
+ - mediatek,mt8192-ufshci
+ - const: mediatek,mt8183-ufshci
clocks:
maxItems: 1
--
2.45.2
Add additional clocks, used on all MediaTek SoCs' UFSHCI controllers:
some of these clocks are optional and used only for scaling purposes
to save power, or to improve performance in the case of the crypt
clocks.
Acked-by: Conor Dooley <[email protected]>
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
.../devicetree/bindings/ufs/mediatek,ufs.yaml | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
index 5728e750761f..1df8779ee902 100644
--- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
+++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
@@ -23,11 +23,24 @@ properties:
- const: mediatek,mt8183-ufshci
clocks:
- maxItems: 1
+ minItems: 1
+ maxItems: 12
clock-names:
+ minItems: 1
items:
- const: ufs
+ - const: ufs-aes
+ - const: ufs-tick
+ - const: unipro-sys
+ - const: unipro-tick
+ - const: ufs-sap
+ - const: ufs-tx-symbol
+ - const: ufs-rx-symbol
+ - const: ufs-mem
+ - const: crypt-mux
+ - const: crypt-lp
+ - const: crypt-perf
phys:
maxItems: 1
--
2.45.2
Add the new mediatek,mt8195-ufshci string.
This SoC's UFSHCI controller is compatible with MT8183.
Acked-by: Conor Dooley <[email protected]>
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
index f14887ea6fdc..5728e750761f 100644
--- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
+++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
@@ -19,6 +19,7 @@ properties:
- items:
- enum:
- mediatek,mt8192-ufshci
+ - mediatek,mt8195-ufshci
- const: mediatek,mt8183-ufshci
clocks:
--
2.45.2
Document the optional dvfsrc-vcore and va09 regulators used for,
respectively, crypt boost and internal MPHY power management in
when powering on/off the (external) MediaTek UFS PHY.
Acked-by: Conor Dooley <[email protected]>
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
index 1df8779ee902..b74a2464196d 100644
--- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
+++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
@@ -48,6 +48,8 @@ properties:
reg:
maxItems: 1
+ dvfsrc-vcore-supply: true
+ va09-supply: true
vcc-supply: true
required:
--
2.45.2
On Wed, 2024-06-12 at 09:43 +0200, AngeloGioacchino Del Regno wrote:
> The MT8192 UFS controller is compatible with the MT8183 one:
> document this by allowing to assign both compatible strings
> "mediatek,mt8192-ufshci", "mediatek,mt8183-ufshci" to the UFSHCI
> node.
>
> Moreover, since no MT8192 devicetree ever declared any UFSHCI node,
> disallow specifying only the MT8192 compatible.
>
> In preparation for adding MT8195 to the mix, the MT8192 compatible
> was added as enum instead of const.
>
> Also, while at it, replace Stanley Chu with me in the maintainers
> field, as he is unreachable and his email isn't active anymore.
>
> Acked-by: Conor Dooley <[email protected]>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> ---
> .../devicetree/bindings/ufs/mediatek,ufs.yaml | 11 +++++++
> ----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> index 32fd535a514a..f14887ea6fdc 100644
> --- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> +++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> @@ -7,16 +7,19 @@ $schema:
> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!kyjoVMsolIn8UqpISxVGSoDhD_bRBX0JEC0OOPb5e1JsXd-TePG6OpTbK6Muyst5dg0grOuoNe3_H25C61ajBHgvNk_dYObW$
>
> title: Mediatek Universal Flash Storage (UFS) Controller
>
> maintainers:
> - - Stanley Chu <[email protected]>
> + - AngeloGioacchino Del Regno <
> [email protected]>
>
Hi AngeloGioacchino,
Stanley Chu has left MediaTek.
I am the new MediaTek UFS maintainer.
Please see
https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
The role of MediaTek UFS maintainer is not suitable to be handed over
to someone outside of MediaTek.
Thanks.
Peter
>
> allOf:
> - $ref: ufs-common.yaml
>
> properties:
> compatible:
> - enum:
> - - mediatek,mt8183-ufshci
> - - mediatek,mt8192-ufshci
> + oneOf:
> + - const: mediatek,mt8183-ufshci
> + - items:
> + - enum:
> + - mediatek,mt8192-ufshci
> + - const: mediatek,mt8183-ufshci
>
> clocks:
> maxItems: 1
On Wed, 2024-06-12 at 09:43 +0200, AngeloGioacchino Del Regno wrote:
> There is no need to have a property that activates the inline crypto
> boost feature, as this needs many things: a regulator, three clocks,
> and the mediatek,boost-crypt-microvolt property to be set.
>
> If any one of these is missing, the feature won't be activated,
> hence, it is useless to have yet one more property to enable that.
>
> While at it, also address another two issues:
> 1. Give back the return value to the caller and make sure to fail
> probing if we get an -EPROBE_DEFER or -ENOMEM; and
> 2. Free the ufs_mtk_crypt_cfg structure allocated in the crypto
> boost function if said functionality could not be enabled because
> it's not supported, as that'd be only wasted memory.
>
> Last but not least, move the devm_kzalloc() call for
> ufs_mtk_crypt_cfg
> to after getting the dvfsrc-vcore regulator and the boost microvolt
> property, as if those fail there's no reason to even allocate that.
>
> Fixes: ac8c2459091c ("scsi: ufs-mediatek: Decouple features from
> platform bindings")
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> ---
> drivers/ufs/host/ufs-mediatek.c | 55 ++++++++++++++++++-------------
> --
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index 23271eb1a244..8d0e7ea52541 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -576,51 +576,55 @@ static int ufs_mtk_init_host_clk(struct ufs_hba
> *hba, const char *name,
> return ret;
> }
>
> -static void ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
> +static int ufs_mtk_init_boost_crypt(struct ufs_hba *hba)
> {
> struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> struct ufs_mtk_crypt_cfg *cfg;
> struct device *dev = hba->dev;
> struct regulator *reg;
> u32 volt;
> -
> - host->crypt = devm_kzalloc(dev, sizeof(*(host->crypt)),
> - GFP_KERNEL);
> - if (!host->crypt)
> - goto disable_caps;
> + int ret;
>
> reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
> if (IS_ERR(reg)) {
> - dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> - PTR_ERR(reg));
> - goto disable_caps;
> + ret = PTR_ERR(reg);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> +
> + return 0;
> }
>
> - if (of_property_read_u32(dev->of_node, "mediatek,boost-crypt-
> microvolt",
> - &volt)) {
> + ret = of_property_read_u32(dev->of_node, "mediatek,boost-crypt-
> microvolt", &volt);
> + if (ret) {
> dev_info(dev, "failed to get mediatek,boost-crypt-
> microvolt");
> - goto disable_caps;
> + return 0;
> }
>
> + host->crypt = devm_kzalloc(dev, sizeof(*host->crypt),
> GFP_KERNEL);
> + if (!host->crypt)
> + return -ENOMEM;
> +
> cfg = host->crypt;
> - if (ufs_mtk_init_host_clk(hba, "crypt_mux",
> - &cfg->clk_crypt_mux))
> - goto disable_caps;
> + ret = ufs_mtk_init_host_clk(hba, "crypt_mux", &cfg-
> >clk_crypt_mux);
> + if (ret)
> + goto out;
>
> - if (ufs_mtk_init_host_clk(hba, "crypt_lp",
> - &cfg->clk_crypt_lp))
> - goto disable_caps;
> + ret = ufs_mtk_init_host_clk(hba, "crypt_lp", &cfg-
> >clk_crypt_lp);
> + if (ret)
> + goto out;
>
> - if (ufs_mtk_init_host_clk(hba, "crypt_perf",
> - &cfg->clk_crypt_perf))
> - goto disable_caps;
> + ret = ufs_mtk_init_host_clk(hba, "crypt_perf", &cfg-
> >clk_crypt_perf);
> + if (ret)
> + goto out;
>
> cfg->reg_vcore = reg;
> cfg->vcore_volt = volt;
> host->caps |= UFS_MTK_CAP_BOOST_CRYPT_ENGINE;
>
> -disable_caps:
> - return;
> +out:
> + if (ret)
> + devm_kfree(dev, host->crypt);
> + return 0;
> }
>
> static int ufs_mtk_init_va09_pwr_ctrl(struct ufs_hba *hba)
> @@ -649,8 +653,9 @@ static int ufs_mtk_init_host_caps(struct ufs_hba
> *hba)
> struct device_node *np = hba->dev->of_node;
> int ret;
>
> - if (of_property_read_bool(np, "mediatek,ufs-boost-crypt"))
> - ufs_mtk_init_boost_crypt(hba);
>
Hi AngeloGioacchino,
As previously explained, removing these DTS settings will make what was
originally a simple task
more complicated. In addition, it will require MediaTek to put in extra
effort to migrate the kernel.
We do not believe that such changes have any benefits.
Thanks.
Peter
> + ret = ufs_mtk_init_boost_crypt(hba);
> + if (ret)
> + return ret;
>
> ret = ufs_mtk_init_va09_pwr_ctrl(hba);
> if (ret)