On Thu, 4 Nov 2021 at 08:23, Peng.Zhou <peng.zhou@xxxxxxxxxxxx> wrote: > > On Mon, 2021-08-09 at 17:29 +0200, Ulf Hansson wrote: > > On Fri, 6 Aug 2021 at 05:16, Peng.Zhou <peng.zhou@xxxxxxxxxxxx> > > wrote: > > > > > > Hi Ulf, > > > > > > Thank you for your advise, I have a question about genpd provider, > > > is it > > > a mandatory DT node for eMMC? Why I confuse it due to I found we > > > only do > > > enable/disable clock during used eMMC in our platforms without any > > > power > > > releated actions....it seems that there is no any "pwner-domain" > > > used > > > for eMMC in our platforms. > > > > A power-domain is optional for an eMMC controller to use. > > > > It then sounds like you should add it, in cases when you have mmc > > crypto support. > > > > Hope this helps! > > > > Kind regards > > Uffe > > Hi Ulf, > > Sorry for long time no response. I have checked eMMC power domain with > our SoC designers and power experts in the past period of time, both > past and present, our power domain hardware design doesn't include > any eMMC part and the future will be same...so I am not sure is > there is any idea in your view? Do you think put atf smc call in genpd > hook is mandatory for us? Yes. To me, it seems best suited to be managed from a genpd provider. Moreover, the corresponding crypto clock can then also be nicely abstracted and managed outside of the driver. Or perhaps you see some other concerns with this approach? Kind regards Uffe > > Thanks! > > > > > > > > On Mon, 2021-05-10 at 19:57 +0800, Ulf Hansson wrote: > > > > On Wed, 14 Apr 2021 at 11:12, Peng Zhou <peng.zhou@xxxxxxxxxxxx> > > > > wrote: > > > > > > > > > > From: Peng Zhou <Peng.Zhou@xxxxxxxxxxxx> > > > > > > > > > > 1. add crypto clock control and ungate it before CQHCI init > > > > > > > > > > 2. set MMC_CAP2_CRYPTO property of eMMC > > > > > > > > > > Signed-off-by: Peng Zhou <Peng.Zhou@xxxxxxxxxxxx> > > > > > > > > As discussed [1], I think the ARM SMC call should be managed > > > > together > > > > with the crypto clock, from a PM domain (a genpd provider). > > > > > > > > These are the steps I think should help to get this supported: > > > > > > > > 1) > > > > Add a new generic mmc DT property for crypto support. Make it > > > > being > > > > parsed from mmc_of_parse() and let it set MMC_CAP2_CRYPTO. > > > > > > > > 2) > > > > Start by adding a genpd provider in DT and implement support for > > > > it in > > > > drivers/soc/*. See > > > > Documentation/devicetree/bindings/power/power-domain.yaml, for > > > > generic > > > > DT bindings. > > > > > > > > If you already have a genpd provider for the device, you may > > > > either > > > > extend it to cope with the below or implement a new and model the > > > > it > > > > as a child (subdomain) of the existing one. > > > > > > > > 3) > > > > To fetch the crypto clock during probe, the genpd provider should > > > > implement the ->attach|detach_dev() callbacks. The ->attach_dev() > > > > callback is invoked from genpd_add_device(), which is called > > > > while > > > > probing the device. Vice verse for the ->detach_dev() callback. > > > > > > > > 4) > > > > Ungating/gating the clock should be implemented from the genpd > > > > provider's ->start|stop() callbacks. These callbacks are called > > > > from > > > > genpd_runtime_suspend|resume(), thus when the device gets runtime > > > > suspended/resumed. As the mmc host driver already supports > > > > runtime PM, > > > > only a minor update in the driver's ->probe() is needed, which is > > > > to > > > > call dev_pm_domain_start(). This turns on the crypto clock and > > > > allows > > > > its registers to be accessed. > > > > > > > > 5) > > > > Add also the ARM SMC calls to the genpd provider's ->start|stop() > > > > callbacks. This means those gets called from SoC specific code, > > > > which > > > > helps to keep the driver portable. > > > > > > > > 6) > > > > Add the crypto clock DT binding for the mmc host. > > > > > > > > 7) > > > > Wire up the mmc host device node in DT to be attached to the > > > > genpd > > > > provider (see power-domains and power-domain-names properties). > > > > Make > > > > sure to also specify the crypto clock and the new mmc crypto > > > > property > > > > for it. > > > > > > > > Please, just ask if there is anything unclear. I will do my best > > > > to help! > > > > > > > > Kind regards > > > > Uffe > > > > > > > > [1] > > > > > https://patchwork.kernel.org/project/linux-mmc/patch/20210309015750.6283-1-peng.zhou@xxxxxxxxxxxx/ > > > > > > > > > --- > > > > > drivers/mmc/host/mtk-sd.c | 15 ++++++++++++++- > > > > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk- > > > > > sd.c > > > > > index 898ed1b023df..1c90360d6cf2 100644 > > > > > --- a/drivers/mmc/host/mtk-sd.c > > > > > +++ b/drivers/mmc/host/mtk-sd.c > > > > > @@ -442,6 +442,7 @@ struct msdc_host { > > > > > struct clk *src_clk_cg; /* msdc source clock control > > > > > gate */ > > > > > struct clk *sys_clk_cg; /* msdc subsys clock control > > > > > gate */ > > > > > struct clk_bulk_data bulk_clks[MSDC_NR_CLOCKS]; > > > > > + struct clk *crypto_clk; /* msdc crypto clock */ > > > > > u32 mclk; /* mmc subsystem clock > > > > > frequency */ > > > > > u32 src_clk_freq; /* source clock frequency */ > > > > > unsigned char timing; > > > > > @@ -802,6 +803,7 @@ static void msdc_set_busy_timeout(struct > > > > > msdc_host *host, u64 ns, u64 clks) > > > > > > > > > > static void msdc_gate_clock(struct msdc_host *host) > > > > > { > > > > > + clk_disable_unprepare(host->crypto_clk); > > > > > clk_bulk_disable_unprepare(MSDC_NR_CLOCKS, host- > > > > > >bulk_clks); > > > > > clk_disable_unprepare(host->src_clk_cg); > > > > > clk_disable_unprepare(host->src_clk); > > > > > @@ -822,6 +824,7 @@ static void msdc_ungate_clock(struct > > > > > msdc_host *host) > > > > > dev_err(host->dev, "Cannot enable pclk/axi/ahb > > > > > clock gates\n"); > > > > > return; > > > > > } > > > > > + clk_prepare_enable(host->crypto_clk); > > > > > > > > > > while (!(readl(host->base + MSDC_CFG) & > > > > > MSDC_CFG_CKSTB)) > > > > > cpu_relax(); > > > > > @@ -2512,6 +2515,15 @@ static int msdc_drv_probe(struct > > > > > platform_device *pdev) > > > > > goto host_free; > > > > > } > > > > > > > > > > + /* only eMMC has crypto property */ > > > > > + if ((mmc->caps2 & MMC_CAP2_NO_SD) && (mmc->caps2 & > > > > > MMC_CAP2_NO_SDIO)) { > > > > > + host->crypto_clk = devm_clk_get(&pdev->dev, > > > > > "crypto"); > > > > > + if (IS_ERR(host->crypto_clk)) > > > > > + host->crypto_clk = NULL; > > > > > + else > > > > > + mmc->caps2 |= MMC_CAP2_CRYPTO; > > > > > + } > > > > > + > > > > > host->irq = platform_get_irq(pdev, 0); > > > > > if (host->irq < 0) { > > > > > ret = -EINVAL; > > > > > @@ -2582,6 +2594,8 @@ static int msdc_drv_probe(struct > > > > > platform_device *pdev) > > > > > host->dma_mask = DMA_BIT_MASK(32); > > > > > mmc_dev(mmc)->dma_mask = &host->dma_mask; > > > > > > > > > > + /* here ungate due to cqhci init will access registers > > > > > */ > > > > > + msdc_ungate_clock(host); > > > > > if (mmc->caps2 & MMC_CAP2_CQE) { > > > > > host->cq_host = devm_kzalloc(mmc->parent, > > > > > sizeof(*host- > > > > > >cq_host), > > > > > @@ -2618,7 +2632,6 @@ static int msdc_drv_probe(struct > > > > > platform_device *pdev) > > > > > spin_lock_init(&host->lock); > > > > > > > > > > platform_set_drvdata(pdev, mmc); > > > > > - msdc_ungate_clock(host); > > > > > msdc_init_hw(host); > > > > > > > > > > ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq, > > > > > -- > > > > > 2.18.0
