Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Stephen Boyd <sboyd@kernel.org>]

Quoting ppvk@codeaurora.org (2019-09-06 05:51:54)
> +Georgi Djakov
> 
> On 2019-09-06 18:17, Pradeep P V K wrote:
> > diff --git a/drivers/mmc/host/sdhci-msm.c 
> > b/drivers/mmc/host/sdhci-msm.c
> > index b75c82d..71515ca 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> >  #define msm_host_writel(msm_host, val, host, offset) \
> >       msm_host->var_ops->msm_writel_relaxed(val, host, offset)
> > 
> > +#define SDHC_DDR "sdhc-ddr"
> > +#define CPU_SDHC "cpu-sdhc"

Do you really need these defines? They're not used more than once or
twice and just seem to make the code harder to read.

> > +
> >  struct sdhci_msm_offset {
> >       u32 core_hc_mode;
> >       u32 core_mci_data_cnt;
> > @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
> >       const struct sdhci_msm_offset *offset;
> >  };
> > 
> > +struct msm_bus_vectors {
> > +     uint64_t ab;
> > +     uint64_t ib;
> > +};
> > +
> > +struct msm_bus_path {
> > +     unsigned int num_paths;
> > +     struct msm_bus_vectors *vec;
> > +};
> > +
> > +struct sdhci_msm_bus_vote_data {
> > +     const char *name;
> > +     unsigned int num_usecase;
> > +     struct msm_bus_path *usecase;
> > +
> > +     unsigned int *bw_vecs;
> > +     unsigned int bw_vecs_size;
> > +
> > +     struct icc_path *sdhc_ddr;
> > +     struct icc_path *cpu_sdhc;
> > +
> > +     uint32_t curr_vote;
> > +

Please use u32 instead of uint32_t. Same comment for u64.

> > +};
> > +
> >  struct sdhci_msm_host {
> >       struct platform_device *pdev;
> >       void __iomem *core_mem; /* MSM SDCC mapped address */
> > @@ -1678,6 +1714,341 @@ static void
> > sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
> >       pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
> >  }
> > 
> > +static int sdhci_msm_dt_get_array(struct device *dev, const char 
> > *prop_name,
> > +                              u32 **bw_vecs, int *len, u32 size)
> > +{
> > +     int ret = 0;
> > +     struct device_node *np = dev->of_node;
> > +     size_t sz;
> > +     u32 *arr = NULL;
> > +
> > +     if (!of_get_property(np, prop_name, len)) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +     sz = *len = *len / sizeof(*arr);
> > +     if (sz <= 0 || (size > 0 && (sz > size))) {
> > +             dev_err(dev, "%s invalid size\n", prop_name);
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > +     if (!arr) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > +     if (ret < 0) {
> > +             dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > +             goto out;
> > +     }
> > +     *bw_vecs = arr;
> > +out:
> > +     if (ret)
> > +             *len = 0;
> > +     return ret;
> > +}
> > +
> > +/* Returns required bandwidth in Bytes per Sec */
> > +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
> > +                                     struct mmc_ios *ios)
> > +{
> > +     unsigned long bw;
> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +     bw = msm_host->clk_rate;
> > +
> > +     if (ios->bus_width == MMC_BUS_WIDTH_4)
> > +             bw /= 2;
> > +     else if (ios->bus_width == MMC_BUS_WIDTH_1)
> > +             bw /= 8;
> > +
> > +     return bw;
> > +}
> > +
> > +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
> > +                                        unsigned int bw)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
> > +
> > +     unsigned int *table = bvd->bw_vecs;

Should probably be a const bw_vecs pointer so that it can't be modified
after the fact.

> > +     unsigned int size = bvd->bw_vecs_size;
> > +     int i;
> > +
> > +     for (i = 0; i < size; i++) {
> > +             if (bw <= table[i])
> > +                     break;

return i;

> > +     }
> > +

return i - 1;

> > +     if (i && (i == size))
> > +             i--;
> > +
> > +     return i;

And then this is useless.....

> > +}
> > +
> > +/*
> > + * This function must be called with host lock acquired.
> > + * Caller of this function should also ensure that msm bus client
> > + * handle is not null.

If it was NULL it would be pretty sad.

> > + */
> > +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host 
> > *msm_host,
> > +                                          int vote,
> > +                                          unsigned long *flags)
> > +{
> > +     struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
> > +     struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
> > +     struct msm_bus_path *usecase = bvd->usecase;
> > +     struct msm_bus_vectors *vec = usecase[vote].vec;
> > +     int ddr_rc = 0, cpu_rc = 0;

Why initialize to 0?

> > +
> > +     if (vote != bvd->curr_vote) {
> > +             spin_unlock_irqrestore(&host->lock, *flags);
> > +             pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu 
> > ib:%llu\n",
> > +                             mmc_hostname(host->mmc), vote, vec[0].ab,
> > +                             vec[0].ib, vec[1].ab, vec[1].ib);
> > +             ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
> > +             cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
> > +             spin_lock_irqsave(&host->lock, *flags);

This is some tricky spin-lockery.

> > +             if (ddr_rc || cpu_rc) {
> > +                     pr_err("%s: icc_set() failed\n",
> > +                             mmc_hostname(host->mmc));
> > +                     goto out;
> > +             }
> > +             bvd->curr_vote = vote;
> > +     }
> > +out:
> > +     return cpu_rc;
> > +}
> > +
> > +/*
> > + * Internal work. Work to set 0 bandwidth for msm bus.
> > + */
> > +static void sdhci_msm_bus_work(struct work_struct *work)
> > +{
> > +     struct sdhci_msm_host *msm_host;
> > +     struct sdhci_host *host;
> > +     unsigned long flags;
> > +
> > +     msm_host = container_of(work, struct sdhci_msm_host,
> > +                             bus_vote_work.work);
> > +     host =  platform_get_drvdata(msm_host->pdev);
> > +
> > +     /* Check handle and return */

This comment is useless, please remove it.

> > +     if (!msm_host->bus_vote_data->sdhc_ddr ||
> > +                     !msm_host->bus_vote_data->cpu_sdhc)
> > +             return;
> > +     spin_lock_irqsave(&host->lock, flags);
> > +     /* don't vote for 0 bandwidth if any request is in progress */
> > +     if (!host->mmc->ongoing_mrq)
> > +             sdhci_msm_bus_set_vote(msm_host, 0, &flags);
> > +     else
> > +             pr_warn("Transfer in progress.Skipping bus voting to 0\n");

Missing space after the full stop. Also, useless warning so just remove
it?

> > +     spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +/*
> > + * This function cancels any scheduled delayed work and sets the bus
> > + * vote based on bw (bandwidth) argument.
> > + */
> > +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host 
> > *host,
> > +                                             unsigned int bw)
> > +{
> > +     int vote;
> > +     unsigned long flags;
> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +     cancel_delayed_work_sync(&msm_host->bus_vote_work);
> > +     spin_lock_irqsave(&host->lock, flags);

Why does the lock need to be held? Is the interconnect framework not
consistent with itself?

> > +     vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
> > +     sdhci_msm_bus_set_vote(msm_host, vote, &flags);
> > +     spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +
> > +#define MSM_MMC_BUS_VOTING_DELAY     200 /* msecs */
> > +#define VOTE_ZERO  0
> > +
> > +/* This function queues a work which will set the bandwidth requiement 
> > to 0 */

This comment style is wrong. Also s/requiement/requirement/

> > +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
> > +{
> > +     unsigned long flags;
> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +     spin_lock_irqsave(&host->lock, flags);
> > +     if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
> > +             queue_delayed_work(system_wq,
> > +                                &msm_host->bus_vote_work,
> > +                                msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
> > +     spin_unlock_irqrestore(&host->lock, flags);

Ok it seems that the sdhci code isn't consistent with itself?

> > +}
> > +
> > +static struct sdhci_msm_bus_vote_data
> > *sdhci_msm_get_bus_vote_data(struct device
> > +                                    *dev, struct sdhci_msm_host *host)
> > +
> > +{
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct device_node *of_node = dev->of_node;
> > +     struct sdhci_msm_bus_vote_data *bvd = NULL;
> > +     struct msm_bus_path *usecase = NULL;
> > +     int ret = 0, i = 0, j, k, num_paths, len;
> > +     const uint32_t *vec_arr = NULL;
> > +     bool mem_err = false;
> > +
> > +     if (!pdev) {
> > +             dev_err(dev, "Null platform device!\n");
> > +             return NULL;
> > +     }
> > +
> > +     bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),

sizeof(*bvd) is better and shorter!

> > +                             GFP_KERNEL);
> > +     if (!bvd) {
> > +             ret = -ENOMEM;
> > +             dev_err(dev, "No sufficient memory!\n");

Don't print errors for allocation failures

> > +             return bvd;
> > +     }
> > +     ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
> > +                             &bvd->bw_vecs, &bvd->bw_vecs_size, 0);
> > +     if (ret) {
> > +             dev_info(dev, "No dt property of bus bw. voting defined!\n");
> > +             dev_info(dev, "Skipping Bus BW voting now!!\n");

Is this debug junk? Why does the user care?

> > +             host->skip_bus_bw_voting = true;
> > +             if (ret != -EINVAL && ret != -ENOMEM)
> > +                     goto free;
> > +             goto err;
> > +     }
> > +
> > +     ret = of_property_read_string(of_node, "qcom,msm-bus,name", 
> > &bvd->name);
> > +     if (ret) {
> > +             dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> > +             goto err;
> > +     }
> > +
> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> > +             &bvd->num_usecase);
> > +     if (ret) {
> > +             dev_err(dev, "Error: num-usecases not found\n");
> > +             goto err;
> > +     }
> > +
> > +     usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
> > +                                bvd->num_usecase), GFP_KERNEL);
> > +     if (!usecase)
> > +             goto err;
> > +
> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> > +                                &num_paths);
> > +     if (ret) {
> > +             dev_err(dev, "Error: num_paths not found\n");
> > +             goto out;
> > +     }
> > +
> > +     vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", 

Why are all the properties qcom specific?

> > &len);
> > +     if (vec_arr == NULL) {

A more consistent style is if (!vec_arr)

> > +             dev_err(dev, "Error: Vector array not found\n");
> > +             goto out;
> > +     }
> > +
> > +     for (i = 0; i < bvd->num_usecase; i++) {
> > +             usecase[i].num_paths = num_paths;
> > +             usecase[i].vec = devm_kzalloc(dev, num_paths *

Use devm_kcalloc().

> > +                                           sizeof(struct msm_bus_vectors),
> > +                                           GFP_KERNEL);
> > +             if (!usecase[i].vec) {
> > +                     mem_err = true;
> > +                     dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> > +                     goto out;
> > +             }
> > +             for (j = 0; j < num_paths; j++) {
> > +                     int idx = ((i * num_paths) + j) * 2;
> > +
> > +                     usecase[i].vec[j].ab = (uint64_t)
> > +                             be32_to_cpu(vec_arr[idx]);
> > +                     usecase[i].vec[j].ib = (uint64_t)
> > +                             be32_to_cpu(vec_arr[idx + 1]);
> > +             }
> > +     }
> > +
> > +     bvd->usecase = usecase;
> > +     return bvd;
> > +out:
> > +     if (mem_err) {

Should be possible to not have this flag.

> > +             for (k = i - 1; k >= 0; k--)
> > +                     devm_kfree(dev, usecase[k].vec);
> > +     }
> > +     devm_kfree(dev, usecase);
> > +free:
> > +     devm_kfree(dev, bvd->bw_vecs);
> > +err:
> > +     devm_kfree(dev, bvd);

You don't need to devm_kfree() anything, just let probe fail it.

> > +     bvd = NULL;
> > +     return bvd;
> > +}
> > +
> > +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> > +                             struct platform_device *pdev)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bsd;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     bsd = sdhci_msm_get_bus_vote_data(dev, host);
> > +     if (!bsd) {
> > +             dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
> > +             return PTR_ERR(bsd);
> > +     }
> > +     host->bus_vote_data = bsd;
> > +
> > +     bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
> > +     if (IS_ERR(bsd->sdhc_ddr)) {
> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",

We don't need "Error: " prefix. That's what the kernel log level is for.

> > +                     PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
> > +             return PTR_ERR(bsd->sdhc_ddr);
> > +     }
> > +
> > +     bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
> > +     if (IS_ERR(bsd->cpu_sdhc)) {
> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> > +                     PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
> > +             return PTR_ERR(bsd->cpu_sdhc);
> > +     }
> > +
> > +     INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);

Why is it in a workqueue context? Is there any reason it can' be done in
whatever calling context it is?

> > +
> > +     return 0;
> > +}
> > +
> > +static void sdhci_msm_bus_unregister(struct device *dev,
> > +                             struct sdhci_msm_host *host)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
> > +     int i;
> > +
> > +     icc_put(bsd->sdhc_ddr);
> > +     icc_put(bsd->cpu_sdhc);

Is there a devm_icc_get() API? If not, please add it and use it.

> > +
> > +     for (i = 0; i < bsd->num_usecase; i++)
> > +             devm_kfree(dev, bsd->usecase[i].vec);
> > +     devm_kfree(dev, bsd->usecase);
> > +     devm_kfree(dev, bsd->bw_vecs);
> > +     devm_kfree(dev, bsd);

Again, not sure we need any devm_kfree() stuff.

> > +}
> > +
> > +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> > +{
> > +     struct mmc_ios *ios = &host->mmc->ios;
> > +     unsigned int bw;
> > +
> > +     bw = sdhci_get_bw_required(host, ios);
> > +     if (enable)
> > +             sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
> > +     else
> > +             sdhci_msm_bus_queue_work(host);
> > +}
> > +
> >  static const struct sdhci_msm_variant_ops mci_var_ops = {
> >       .msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
> >       .msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
> > @@ -1839,6 +2210,13 @@ static int sdhci_msm_probe(struct 
> > platform_device *pdev)
> >               dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
> >       }
> > 
> > +     ret = sdhci_msm_bus_register(msm_host, pdev);
> > +     if (ret && !msm_host->skip_bus_bw_voting)

Can this be a check for a NULL icc handle instead of the bool?