| 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
| ||
|
Message-ID: <362d66661fcafc09c8d8d15be9e81823caa4ad1b.camel@mediatek.com>
Date: Thu, 23 Jan 2025 01:42:03 +0000
From: Andy-ld Lu (卢东) <Andy-ld.Lu@...iatek.com>
To: "ansuelsmth@...il.com" <ansuelsmth@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Wenbin Mei (梅文彬) <Wenbin.Mei@...iatek.com>,
Chaotian Jing (井朝天)
<Chaotian.Jing@...iatek.com>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
"robh@...nel.org" <robh@...nel.org>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
<matthias.bgg@...il.com>, "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, upstream <upstream@...oha.com>
Subject: Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host
On Wed, 2025-01-22 at 10:32 +0100, Christian Marangi wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On Tue, Jan 21, 2025 at 06:25:48AM +0000, Andy-ld Lu (卢东) wrote:
> > On Mon, 2025-01-20 at 18:29 +0100, Christian Marangi wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >
> > >
> > > On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> > > > On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > > > > Add support for AN7581 MMC Host. The MMC Host controller is
> > > > > based
> > > > > on
> > > > > mt7622 with the difference of not having regulator supply and
> > > > > state_uhs
> > > > > pins and hclk clock.
> > > > >
> > > > > Some minor fixes are applied to check if the state_uhs pins
> > > > > are
> > > > > defined
> > > > > and make hclk optional for the new airoha compatible.
> > > > >
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > > > > ---
> > > > > drivers/mmc/host/mtk-sd.c | 55
> > > > > ++++++++++++++++++++++++++++++++-
> > > > > ----
> > > > > --
> > > > > 1 file changed, 46 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/mtk-sd.c
> > > > > b/drivers/mmc/host/mtk-
> > > > > sd.c
> > > > > index efb0d2d5716b..9d6868883c91 100644
> > > > > --- a/drivers/mmc/host/mtk-sd.c
> > > > > +++ b/drivers/mmc/host/mtk-sd.c
> > > > > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > > > > mt8196_compat = {
> > > > > .support_new_rx = true,
> > > > > };
> > > > >
> > > > > +static const struct mtk_mmc_compatible an7581_compat = {
> > > > > + .clk_div_bits = 12,
> > > > > + .recheck_sdio_irq = true,
> > > > > + .hs400_tune = false,
> > > > > + .pad_tune_reg = MSDC_PAD_TUNE0,
> > > > > + .async_fifo = true,
> > > > > + .data_tune = true,
> > > > > + .busy_check = true,
> > > > > + .stop_clk_fix = true,
> > > > > + .stop_dly_sel = 3,
> > > > > + .enhance_rx = true,
> > > > > + .support_64g = false,
> > > > > +};
> > > > > +
> > > > > static const struct of_device_id msdc_of_ids[] = {
> > > > > { .compatible = "mediatek,mt2701-mmc", .data =
> > > > > &mt2701_compat},
> > > > > { .compatible = "mediatek,mt2712-mmc", .data =
> > > > > &mt2712_compat},
> > > > > @@ -680,7 +694,7 @@ static const struct of_device_id
> > > > > msdc_of_ids[] =
> > > > > {
> > > > > { .compatible = "mediatek,mt8183-mmc", .data =
> > > > > &mt8183_compat},
> > > > > { .compatible = "mediatek,mt8196-mmc", .data =
> > > > > &mt8196_compat},
> > > > > { .compatible = "mediatek,mt8516-mmc", .data =
> > > > > &mt8516_compat},
> > > > > -
> > > > > + { .compatible = "airoha,an7581-mmc", .data =
> > > > > &an7581_compat},
> > > > > {}
> > > > > };
> > > > > MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > > > > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > > > > mmc_host *mmc, struct mmc_ios *ios)
> > > > > struct msdc_host *host = mmc_priv(mmc);
> > > > > int ret;
> > > > >
> > > > > + /* Skip setting supply if not supported */
> > > > > + if (!mmc->supply.vqmmc)
> > > > > + return 0;
> > > > > +
> > > >
> > > > Hi Christian,
> > > >
> > > > I think here is no need. If you have not 'vqmmc' in the
> > > > dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the
> > > > corresponding
> > > > flow would not be executed.
> > > >
> > > > And another question, host->pins_default is just selected here,
> > > > that
> > > > would be lost.
> > > >
> > > > > if (!IS_ERR(mmc->supply.vqmmc)) {
> > > > > if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330
> > > > > &&
> > > > > ios->signal_voltage !=
> > > > > MMC_SIGNAL_VOLTAGE_180) {
> > > > > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > > > > mmc_host *mmc, int enb)
> > > > > dev_dbg(host->dev, "SDIO eint
> > > > > irq:
> > > > > %d!\n", host->eint_irq);
> > > > > }
> > > > >
> > > > > - pinctrl_select_state(host->pinctrl, host-
> > > > > > pins_uhs);
> > > > >
> > > > > + /* Skip setting uhs pins if not supported
> > > > > */
> > > > > + if (host->pins_uhs)
> > > > > + pinctrl_select_state(host-
> > > > > >pinctrl,
> > > > > host->pins_uhs);
> > > > > } else {
> > > > > dev_pm_clear_wake_irq(host->dev);
> > > > > }
> > > > > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct
> > > > > mmc_host
> > > > > *mmc, struct mmc_ios *ios)
> > > > >
> > > > > msdc_set_buswidth(host, ios->bus_width);
> > > > >
> > > > > + /* Skip regulator if not supported */
> > > > > + if (!mmc->supply.vmmc)
> > > > > + goto skip_regulator;
> > > > > +
> > > >
> > > > No need too.
> > > >
> > > > > /* Suspend/Resume will do power off/on */
> > > > > switch (ios->power_mode) {
> > > > > case MMC_POWER_UP:
> > > > > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct
> > > > > mmc_host
> > > > > *mmc, struct mmc_ios *ios)
> > > > > break;
> > > > > }
> > > > >
> > > > > +skip_regulator:
> > > > > if (host->mclk != ios->clock || host->timing != ios-
> > > > > >timing)
> > > > > msdc_set_mclk(host, ios->timing, ios->clock);
> > > > > }
> > > > > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > > > > platform_device *pdev,
> > > > > if (IS_ERR(host->src_clk))
> > > > > return PTR_ERR(host->src_clk);
> > > > >
> > > > > - host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > - if (IS_ERR(host->h_clk))
> > > > > - return PTR_ERR(host->h_clk);
> > > > > + /* AN7581 SoC doesn't have hclk */
> > > > > + if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > mmc")) {
> > > > > + host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > + if (IS_ERR(host->h_clk))
> > > > > + return PTR_ERR(host->h_clk);
> > > > > + }
> > > >
> > > > devm_clk_get_optional could be used to instead here, no need to
> > > > use
> > > > compatible to distinguish.
> > > >
> > >
> > > I can make the hclk optional but I think this would affect also
> > > every
> > > other compatible by hiding broken clock configuration.
> > >
> > > > > host->bus_clk = devm_clk_get_optional(&pdev->dev,
> > > > > "bus_clk");
> > > > > if (IS_ERR(host->bus_clk))
> > > > > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > > > > platform_device *pdev)
> > > > > return PTR_ERR(host->pins_default);
> > > > > }
> > > > >
> > > > > - host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > > > "state_uhs");
> > > > > - if (IS_ERR(host->pins_uhs)) {
> > > > > - dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > uhs!\n");
> > > > > - return PTR_ERR(host->pins_uhs);
> > > > > + /* AN7581 doesn't have state_uhs pins */
> > > > > + if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > mmc")) {
> > > > > + host->pins_uhs = pinctrl_lookup_state(host-
> > > > > >pinctrl,
> > > > > "state_uhs");
> > > > > + if (IS_ERR(host->pins_uhs)) {
> > > > > + dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > uhs!\n");
> > > > > + return PTR_ERR(host->pins_uhs);
> > > > > + }
> > > > > }
> > > >
> > > > Could you consider to set a dummy 'state_uhs' same as
> > > > 'state_default'
> > > > in the dts, that you could not use compatible to distinguish
> > > > here.
> > > >
> > >
> > > This is problematic, correct me if I'm wrong, you are suggesting
> > > to
> > > assign the emmc pins to both default and uhs? This is problematic
> > > as
> > > the
> > > pinctrl driver would complain that such pins are already assigned
> > > to
> > > something. Also I don't think it's possible to assign these pins
> > > to a
> > > dummy pin.
> > >
> >
> > Maybe I have not expressed clearly...What I mean is that you could
> > set
> > as below, and the content of &mmc_pins_uhs is just copied from
> > &mmc_pins_default.
> >
> > mmc@...0e000 {
> > ...
> > pinctrl-names = "default", "state_uhs";
> > pinctrl-0 = <&mmc_pins_default>;
> > pinctrl-1 = <&mmc_pins_uhs>;
> > }
>
> Ok my bad. I did declared the second pin to pinctrl-0 instead of
> adding
> pinctrl-1. With that it does work correctly.
>
> > > > >
> > > > > /* Support for SDIO eint irq ? */
> > > > > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > > > > platform_device *pdev)
> > > > > dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> > > > > goto release_clk;
> > > > > }
> > > > > +
> > > > > + /* AN7581 without regulator require tune to OCR values */
> > > > > + if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc")
> > > > > &&
> > > > > + !mmc->ocr_avail)
> > > > > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > > > +
> > > >
> > > > Maybe you could use regulator-fixed in the dts and configure
> > > > min/max
> > > > voltage to get ocr_avail, no need to set hard code here.
> > > >
> > >
> > > Also suggested by Wenbin Mei (梅文彬) and I just tried this.
> > >
> > > This can't be done, fixed-regulator needs to have the same min
> > > and
> > > max
> > > voltage or they fail to probe sooo fixed-regulator saddly can't
> > > be
> > > used
> > > :(
> > >
> > > I will send a new version of this with the other point corrected
> > > but
> > > I
> > > think a compatible and these additional if is a must :(
> >
> > If use the fixed regulator such as below, you will get the same
> > ocr_avail as 'MMC_VDD_32_33 | MMC_VDD_33_34' through
> > mmc_regulator_get_ocrmask().
> >
> > vmmc_3v3: regulator-vmmc-3v3 {
> > compatible = "regulator-fixed";
> > regulator-name = "vmmc";
> > regulator-min-microvolt = <3300000>;
> > regulator-max-microvolt = <3300000>;
> > regulator-always-on;
> > }
>
> Ok the code was a bit confusing but yes I can confirm that a 3.3
> fixed
> regulator define those 2 flags so also this is OK.
>
> There is still the discussion about clock. You are totally against a
> new
> compatible for the hclk?
>
As the comment in the v2 patches, the better way is to add a bool
variable like 'needs_not_hclk' in the compat data, which is just true
for you, and use !host->dev_comp->needs_not_hclk as the condition to
get 'hclk'.
But I would like to confirm if the 'fixed-clock' could be supported in
your project, which is also used in mt7622.dtsi, you may align 'hclk'
to a dummy fixed-clock...
> > >
> > > > > msdc_init_hw(host);
> > > > >
> > > > > if (mmc->caps2 & MMC_CAP2_CQE) {
> > >
> > > --
> > > Ansuel
>
> --
> Ansuel
Powered by blists - more mailing lists