Re: [PATCH 4/4] drm/msm: add OCMEM driver

[Stephen Boyd <sboyd@codeaurora.org>]

On 09/28, Rob Clark wrote:
> On Mon, Sep 28, 2015 at 6:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 09/28, Rob Clark wrote:
> >> diff --git a/drivers/gpu/drm/msm/ocmem/ocmem.c b/drivers/gpu/drm/msm/ocmem/ocmem.c
> >> new file mode 100644
> >> index 0000000..d3cdd64
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/ocmem/ocmem.c
> >> @@ -0,0 +1,396 @@
> >> +/*
> >> + * Copyright (C) 2015 Red Hat
> >> + * Author: Rob Clark <robdclark@gmail.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms of the GNU General Public License version 2 as published by
> >> + * the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License along with
> >> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/cpuset.h>
> >
> > What is this include for?
> 
> needed for qcom_scm.h, although I guess I could just add the missing
> #includes in qcom_scm.h instead..

Ok, we should fix that in scm header files. It probably needs a
forward declare of struct cpumask and it should be struct cpumask
* instead of cpumask_t *.

> 
> >> +#include <linux/qcom_scm.h>
> >> +
> >> +#include "msm_drv.h"
> >> +#include "ocmem.h"
> >> +#include "ocmem.xml.h"
> >> +
[..]
> >> +
> >> +static void update_ocmem(struct ocmem *ocmem)
> >> +{
> >> +     uint32_t region_mode_ctrl = 0x0;
> >> +     unsigned pos = 0;
> >> +     unsigned i = 0;
> >> +
> >> +     if (!qcom_scm_ocmem_lock_available()) {
> >> +             for (i = 0; i < ocmem->config->num_regions; i++) {
> >> +                     struct ocmem_region *region = &ocmem->regions[i];
> >> +                     pos = i << 2;
> >> +                     if (region->mode == THIN_MODE)
> >> +                             region_mode_ctrl |= BIT(pos);
> >> +             }
> >> +             dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n", region_mode_ctrl);
> >> +             ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, region_mode_ctrl);
> >> +             /* Barrier to commit the region mode */
> >> +             mb();
> >
> > msm_writel() already has a barrier, so now we have a double
> > barrier?
> 
> hmm, msm_writel() doesn't have any more barrier than writel().. so I
> kept the mb() from downstream..

Yes writel() already has a barrier. Downstream is using
writel_relaxed() instead of writel() in ocmem_write() and then
adding the barrier explicitly after ocmem_write() in the right
places.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project