On Tue, Mar 24, 2020 at 02:22:28PM +0000, Lee Jones wrote:
> On Tue, 24 Mar 2020, Mika Westerberg wrote:
>
> > On Tue, Mar 24, 2020 at 11:52:19AM +0000, Lee Jones wrote:
> > > On Tue, 03 Mar 2020, Mika Westerberg wrote:
> > >
> > > > This driver only creates a bunch of platform devices sharing resources
> > > > belonging to the PMC device. This is pretty much what MFD subsystem is
> > > > for so move the driver there, renaming it to intel_pmc_bxt.c which
> > > > should be more clear what it is.
> > > >
> > > > MFD subsystem provides nice helper APIs for subdevice creation so
> > > > convert the driver to use those. Unfortunately the ACPI device includes
> > > > separate resources for most of the subdevices so we cannot simply call
> > > > mfd_add_devices() to create all of them but instead we need to call it
> > > > separately for each device.
> > > >
> > > > The new MFD driver continues to expose two sysfs attributes that allow
> > > > userspace to send IPC commands to the PMC/SCU to avoid breaking any
> > > > existing applications that may use these. Generally this is bad idea so
> > > > document this in the ABI documentation.
> > > >
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > > ---
> > > > .../ABI/obsolete/sysfs-driver-intel_pmc_bxt | 22 +
> > > > arch/x86/include/asm/intel_pmc_ipc.h | 47 --
> > > > arch/x86/include/asm/intel_telemetry.h | 1 +
> > > > drivers/mfd/Kconfig | 16 +-
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/intel_pmc_bxt.c | 504 ++++++++++++++
> > > > drivers/platform/x86/Kconfig | 16 +-
> > > > drivers/platform/x86/Makefile | 1 -
> > > > drivers/platform/x86/intel_pmc_ipc.c | 645 ------------------
> > > > .../platform/x86/intel_telemetry_debugfs.c | 12 +-
> > > > drivers/platform/x86/intel_telemetry_pltdrv.c | 2 +
> > > > drivers/usb/typec/tcpm/Kconfig | 2 +-
> > > > include/linux/mfd/intel_pmc_bxt.h | 43 ++
> > > > 13 files changed, 602 insertions(+), 710 deletions(-)
> > > > create mode 100644 Documentation/ABI/obsolete/sysfs-driver-intel_pmc_bxt
> > > > delete mode 100644 arch/x86/include/asm/intel_pmc_ipc.h
> > > > create mode 100644 drivers/mfd/intel_pmc_bxt.c
> > > > delete mode 100644 drivers/platform/x86/intel_pmc_ipc.c
> > > > create mode 100644 include/linux/mfd/intel_pmc_bxt.h
> > >
> > > [...]
> > >
> > > > +/*
> > > > + * We use the below templates to construct MFD cells. The struct
> > > > + * intel_pmc_dev instance holds the real MFD cells where we first copy
> > > > + * these and then fill the dynamic parts based on the extracted resources.
> > > > + */
> > > > +
> > > > +static const struct mfd_cell punit = {
> > > > + .name = "intel_punit_ipc",
> > > > +};
> > > > +
> > > > +static int update_no_reboot_bit(void *priv, bool set)
> > > > +{
> > > > + struct intel_pmc_dev *pmc = priv;
> > > > + u32 bits = PMC_CFG_NO_REBOOT_EN;
> > > > + u32 value = set ? bits : 0;
> > > > +
> > > > + return intel_pmc_gcr_update(pmc, PMC_GCR_PMC_CFG_REG, bits, value);
> > > > +}
> > > > +
> > > > +static const struct itco_wdt_platform_data tco_pdata = {
> > > > + .name = "Apollo Lake SoC",
> > > > + .version = 5,
> > > > + .update_no_reboot_bit = update_no_reboot_bit,
> > > > +};
> > > > +
> > > > +static const struct mfd_cell tco = {
> > > > + .name = "iTCO_wdt",
> > > > + .ignore_resource_conflicts = true,
> > > > +};
> > > > +
> > > > +static const struct resource telem_res[] = {
> > > > + DEFINE_RES_MEM(TELEM_PUNIT_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> > > > + DEFINE_RES_MEM(TELEM_PMC_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> > > > +};
> > > > +
> > > > +static const struct mfd_cell telem = {
> > > > + .name = "intel_telemetry",
> > > > + .resources = telem_res,
> > > > + .num_resources = ARRAY_SIZE(telem_res),
> > > > +};
> > > > +
> > > > +static int intel_pmc_get_tco_resources(struct platform_device *pdev,
> > > > + struct intel_pmc_dev *pmc)
> > > > +{
> > > > + struct itco_wdt_platform_data *pdata;
> > > > + struct resource *res, *tco_res;
> > > > +
> > > > + if (acpi_has_watchdog())
> > > > + return 0;
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_IO,
> > > > + PLAT_RESOURCE_ACPI_IO_INDEX);
> > > > + if (!res) {
> > > > + dev_err(&pdev->dev, "Failed to get IO resource\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + tco_res = devm_kcalloc(&pdev->dev, 2, sizeof(*tco_res), GFP_KERNEL);
> > > > + if (!tco_res)
> > > > + return -ENOMEM;
> > > > +
> > > > + tco_res[0].flags = IORESOURCE_IO;
> > > > + tco_res[0].start = res->start + TCO_BASE_OFFSET;
> > > > + tco_res[0].end = tco_res[0].start + TCO_REGS_SIZE - 1;
> > > > + tco_res[1].flags = IORESOURCE_IO;
> > > > + tco_res[1].start = res->start + SMI_EN_OFFSET;
> > > > + tco_res[1].end = tco_res[1].start + SMI_EN_SIZE - 1;
> > > > +
> > > > + pmc->cells[PMC_TCO].resources = tco_res;
> > > > + pmc->cells[PMC_TCO].num_resources = 2;
> > > > +
> > > > + pdata = devm_kmemdup(&pdev->dev, &tco_pdata, sizeof(*pdata), GFP_KERNEL);
> > > > + if (!pdata)
> > > > + return -ENOMEM;
> > >
> > > Why do you need to take a copy?
> > >
> > > This can be referenced directly in 'mfd_cell tco', no?
> >
> > No because I'm filling the priv pointer dynamically. I've tried to
> > explain the same thing in the previous iterations already.
>
> You have, and I didn't agree with you then either. ;)
>
> You can add this directly to 'mfd_cell tco' and make the dynamic
> changes after the fact. You do not need to be duplicating memory all
> over the place.
Well fine I don't want to argue about this. You are the maintainer.
> > > > + pdata->no_reboot_priv = pmc;
> > >
> > > You're putting device data inside platform data?
> > >
> > > This doesn't sit right with me at all.
> > >
> > > You already saved it using platform_set_drvdata(), why do you need it
> > > twice? Why can't you export update_no_reboot_bit() and make it take
> > > 'struct intel_pmc_dev' or better yet 'pdev' as an argument?
> >
> > This is a property of the iTCO_wdt driver, not part of this patch
> > series. I'm just using the interface it provides.
> >
> > iTCO_wdt interface can of course be made better but I don't think it
> > should be part of this series.
>
> As far as I'm concerned, this is a new driver.
>
> If there is some ugliness, it should be ironed out before being
> merged. People have a tendency to lower the priority of fix-ups once
> their patches have been merged. I suggest you fix the interface
> *first*, rather than as an afterthought.
>
> Since the interface is only between this and the iTCO_wdt driver, this
> should be trivial.
OK
> > > > + pmc->cells[PMC_TCO].platform_data = pdata;
> > > > + pmc->cells[PMC_TCO].pdata_size = sizeof(*pdata);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int intel_pmc_get_resources(struct platform_device *pdev,
> > > > + struct intel_pmc_dev *pmc,
> > > > + struct intel_scu_ipc_data *scu_data)
> > > > +{
> > > > + struct resource *res, *punit_res;
> > > > + struct resource gcr_res;
> > > > + size_t npunit_res = 0;
> > > > + int ret;
> > > > +
> > > > + scu_data->irq = platform_get_irq_optional(pdev, 0);
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > > + PLAT_RESOURCE_IPC_INDEX);
> > > > + if (!res) {
> > > > + dev_err(&pdev->dev, "Failed to get IPC resource\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + /* IPC registers */
> > > > + scu_data->mem.flags = res->flags;
> > > > + scu_data->mem.start = res->start;
> > > > + scu_data->mem.end = res->start + PLAT_RESOURCE_IPC_SIZE - 1;
> > > > +
> > > > + /* GCR registers */
> > > > + gcr_res.flags = res->flags;
> > > > + gcr_res.start = res->start + PLAT_RESOURCE_GCR_OFFSET;
> > > > + gcr_res.end = gcr_res.start + PLAT_RESOURCE_GCR_SIZE - 1;
> > > > +
> > > > + pmc->gcr_mem_base = devm_ioremap_resource(&pdev->dev, &gcr_res);
> > > > + if (IS_ERR(pmc->gcr_mem_base))
> > > > + return PTR_ERR(pmc->gcr_mem_base);
> > > > +
> > > > + pmc->cells[PMC_TCO] = tco;
> > > > + pmc->cells[PMC_PUNIT] = punit;
> > > > + pmc->cells[PMC_TELEM] = telem;
> > >
> > > Why are you still saving these to device data?
> > >
> > > What's stopping you operating on the structures directly?
> >
> > OK, I've explained this in the previous iterations but here goes. The
> > problem is that the resources need to be filled dynamically as they are
> > whatever there is in the ACPI table.
>
> Yep. No problem there.
>
> > Now, Consider that we have two PMC devices. It is possible that the
> > driver is bind to both in paraller which means that both are racing to
> > fill and use these structures leading to a corruption.
>
> I'm not saying it can't, but please explain to me how this can
> happen. There are many instances where multiple identical H/W blocks
> occupy a single platform. Please explain why this isn't a problem for
> any other device driver.
Any other device driver either uses per instance data or they don't need
to fill in the resources dynamically after extracting them from the
firmware description.
> Besides, if this is a genuine concern, that's the sort of problem
> locking was designed to solve.
Well that would end up even uglier solution than simply taking a copy.
> > Another issue is that even if we have single device, the driver fills in
> > the structures and then we unbind it. These structures now are left with
> > that data which does not feel right.
>
> What difference does it make if the driver is left with static or
> dynamic data? If the driver is not to be rebound, then it doesn't
> matter. If it is bound again, the data will just be overwritten in
> .probe(). I'm not sure I understand the problem.
>
> > Therefore I've put all we know in advance as const version of these
> > structures and then we use those as template to build custom ones based
> > on resources extracted from ACPI to individual instances.
>
> I can see that, I just don't agree with it. :)
Fair enough. I'll do these changes in v9.
