Message-ID: <1432567116.8736.34.camel@linux.intel.com>
Date:	Mon, 25 May 2015 18:18:36 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Andy Shevchenko <andy.shevchenko@...il.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Jarkko Nikula <jarkko.nikula@...ux.intel.com>
Subject: Re: [PATCH v1 3/3] mfd: Add support for Intel Sunrisepoint LPSS
 devices

On Wed, 2015-04-29 at 16:06 +0100, Lee Jones wrote:
> On Wed, 29 Apr 2015, Andy Shevchenko wrote:
> > On Wed, Apr 29, 2015 at 11:23 AM, Lee Jones <lee.jones@...aro.org> wrote:
> > > On Tue, 31 Mar 2015, Andy Shevchenko wrote:
> > 
> > Thanks for review. My comments / answers below.
> > 
> > >> The new coming Intel platforms such as Skylake will contain Sunrisepoint PCH.
> > >
> > > Can you use the same naming conventions as the Skylake's predecessors?
> > >
> > > Currently lpc_ich and lpc_sch exist.
> > 
> > As far as I understand it is not an LPC bus, thus seems above is
> > irrelevant. Moreover, PCH in Skylake is something new if looking into
> > hardware.
> 
> Perhaps the aforementioned files just need 'intel_' appended then.

Might be. Not concern of this series anyway.

> > >> The main difference to the previous platforms is that the LPSS devices are
> > >> compound devices where usually main (SPI, HSUART, or I2C) and DMA IPs are
> > >> present.
> > >>
> > >> This patch brings the driver for such devices found on Sunrisepoint PCH.
> > >>
> > >> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > >> ---
> > >>  drivers/mfd/Kconfig           |  24 ++
> > >>  drivers/mfd/Makefile          |   3 +
> > >>  drivers/mfd/intel-lpss-acpi.c |  84 +++++++
> > >>  drivers/mfd/intel-lpss-pci.c  | 106 +++++++++
> > >>  drivers/mfd/intel-lpss.c      | 523 ++++++++++++++++++++++++++++++++++++++++++
> > >>  drivers/mfd/intel-lpss.h      |  62 +++++
> > >
> > > Can you move this to include/?
> > 
> > What the point? I don't see the users for that now or in the nearest future.
> 
> The point is to keep drivers/mfd/ clean rather than to share the header
> with external entities.

I don't get this. Why should we go to global space with the header which
is local to couple of modules located under drivers/mfd?

[]

> > >
> > >> +};
> > >> +
> > >> +static const struct resource intel_lpss_dev_resources[] = {
> > >> +     DEFINE_RES_MEM_NAMED(LPSS_DEV_OFFSET, LPSS_DEV_SIZE, "lpss_dev"),
> > >> +     DEFINE_RES_MEM_NAMED(LPSS_PRIV_OFFSET, LPSS_PRIV_SIZE, "lpss_priv"),
> > >> +     DEFINE_RES_IRQ(0),
> > >> +};
> > >
> > > Why has this been called 'dev' and 'priv'?  Neither of which are
> > > particularlly helpful address names and I can't find them in the
> > > datasheet.  Why not 'base' and 'extended', or something?
> > 
> > dev stands for main device controller
> > priv -- for private space. The abbrevation is inherited from LPSS
> > drivers for BayTrail and Co.
> > 
> > In documentation priv space called 'additional registers'.
> 
> In my mind 'additional registers' != 'private registers'.

This documentation somehow hides the purpose of the set of register
called as 'additional'. Their function is to provide a convergence layer
for both devices in MFD cell. Previously (Baytrail, Braswell) we used
name LPSS_PRIV for everything related to that 'additional' register
space. We would like to keep that way.

> > >> +static DEFINE_IDA(intel_lpss_devid_ida);
> > >
> > > What's wrong with using PLATFORM_DEVID_AUTO?
> > 
> > We have to provide right clocks.
> > Is it possible to use AUTO and be sure that right clocks will go to
> > corresponding consumers?
> 
> I guess Mike will have to help us with that, as I've never seen it
> done this way before.
> 
> I guess you need to create a clock driver and register it properly as
> a platform device.

Mika had answered you. I hope he has a valid point.

> > >> +static bool intel_lpss_has_idma(const struct intel_lpss *lpss)
> > >> +{
> > >> +     return (lpss->caps & LPSS_PRIV_CAPS_NO_IDMA) == 0;
> > >> +}
> > >> +
> > >> +static void intel_lpss_reset(const struct intel_lpss *lpss)
> > >
> > > Not very descriptive function name.  'de|assert'?
> > 
> > It actually handles the reset signal. I don't see how de/assert will be better.
> 
> What do you mean by 'handle'?  By the comment below it appers to
> "bring the device out of reset" i.e. deassert the reset line, no?
> 
> intel_lpss_deassert_reset()?

Ok.

> > >> +int intel_lpss_probe(struct device *dev,
> > >> +                  const struct intel_lpss_platform_info *info)
> > >> +{
> > >> +     const struct mfd_cell *devs;
> > >> +     struct intel_lpss *lpss;
> > >> +     struct resource priv;
> > >> +     int ret;
> > >> +
> > >> +     if (!info || !info->mem || info->irq <= 0)
> > >> +             return -EINVAL;
> > >> +
> > >> +     lpss = devm_kzalloc(dev, sizeof(*lpss), GFP_KERNEL);
> > >> +     if (!lpss)
> > >> +             return -ENOMEM;
> > >> +
> > >> +     memset(&priv, 0, sizeof(priv));
> > >> +     priv.start = info->mem->start + LPSS_PRIV_OFFSET;
> > >> +     priv.end = priv.start + LPSS_PRIV_SIZE - 1;
> > >> +     priv.flags = IORESOURCE_MEM;
> > >> +
> > >> +     lpss->priv = devm_ioremap_resource(dev, &priv);
> > >> +     if (!lpss->priv)
> > >> +             return -ENOMEM;
> > >> +
> > >> +     intel_lpss_reset(lpss);
> > >> +
> > >> +     lpss->info = info;
> > >> +     lpss->dev = dev;
> > >> +     lpss->caps = readl(lpss->priv + LPSS_PRIV_CAPS);
> > >> +     lpss->ndevs = intel_lpss_has_idma(lpss) ? 2 : 1;
> > >
> > > This is fragile.
> > >
> > > I'd prefer you dynamically allocate resources here, instead of
> > > statically up the top then 'jumping' over the ones you don't need.
> > 
> > (1)

> > >> +     ret = intel_lpss_register_clock(lpss);
> > >> +     if (ret < 0)
> > >> +             goto err_clk_register;
> > >> +
> > >> +     intel_lpss_ltr_add(lpss);
> > >
> > > Add what, to what?  What is 'ltr'?
> > 
> > LTR stands for Latency Tolerance Reporting, the PCI property.
> > 
> > https://www.pcisig.com/specifications/pciexpress/specifications/ECN_LatencyTolnReporting_14Aug08.pdf
> 
> So what's the _add part? What are you adding and to what?

So, we rename in new version like this
_add -> _expose
_remove -> _hide

in correspondence with PM QoS calls.

> > >> +     ret = intel_lpss_debugfs_add(lpss);
> > >> +     if (ret)
> > >> +             dev_warn(lpss->dev, "Failed to create debugfs entries\n");
> > >> +
> > >> +     devs = intel_lpss_has_idma(lpss) ? lpss->devs : lpss->devs + 1;
> > >
> > > *cringe*
> > 
> > Here the comment also for (1).
> > 
> > In case of DMA IP present on compound device we have to register it
> > first, otherwise main device driver may miss that (SPI checks DMA on
> > ->probe() stage). Unfortunately there is no possibility to tell MFD
> > framework the order of the device initialization except the direct
> > order in the array. That's why this solution was implemented. If you
> > have any better idea, please, share.
> 
> Relying on the order a given set of devices will probe() is fragile.
> 
> That's why -EPROBE_DEFER exists.

Let's consider this case in more generic way (arbitrary host controller
and arbitrary DMA that could or, what is important, *couldn't* be used.
The DMA channel is optional to many drivers to go well. For example SPI
controller when gets no DMA channel will easily switch to PIO mode and
user will be happy by having working, though slow, device.

Thus EDEFER_PROBE doesn't solve the problem but makes situation even
worse!

> > >> +     ida_simple_rnemove(&intel_lpss_devid_ida, lpss->devid);
> Does this even compile?

Of course it does. I have no idea how it was happened in the mail, but
in sources we have ida_simple_remove().

> > >> +}
> > >> +EXPORT_SYMBOL_GPL(intel_lpss_remove);
> > >> +
> > >> +static int resume_lpss_device(struct device *dev, void *data)
> > >> +{
> > >> +     pm_runtime_resume(dev);
> > >> +     return 0;
> > >> +}
> > >> +
> > >> +int intel_lpss_prepare(struct device *dev)
> > >> +{
> > >> +     /*
> > >> +      * Resume both child devices before entering system sleep. This
> > >> +      * ensures that they are in proper state before they get suspended.
> > >> +      */
> > >> +     device_for_each_child(dev, NULL, resume_lpss_device);
> > >> +     return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(intel_lpss_prepare);
> > >> +
> > >> +int intel_lpss_suspend(struct device *dev)
> > >> +{
> > >> +     return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(intel_lpss_suspend);
> > >> +
> > >> +int intel_lpss_resume(struct device *dev)
> > >> +{
> > >> +     struct intel_lpss *lpss = dev_get_drvdata(dev);
> > >> +     intel_lpss_reset(lpss);
> > >> +     return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(intel_lpss_resume);
> > >> +
> > >> +static int __init intel_lpss_init(void)
> > >> +{
> > >> +     intel_lpss_debugfs = debugfs_create_dir("intel_lpss", NULL);
> > >> +     return 0;
> > >> +}
> > >> +module_init(intel_lpss_init);
> > >> +
> > >> +static void __exit intel_lpss_exit(void)
> > >> +{
> > >> +     debugfs_remove(intel_lpss_debugfs);
> > >> +}
> > >> +module_exit(intel_lpss_exit);
> > >> +
> > >> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@...ux.intel.com>");
> > >> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@...ux.intel.com>");
> > >> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@...ux.intel.com>");
> > >> +MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@...ux.intel.com>");
> > >> +MODULE_DESCRIPTION("Intel LPSS core driver");
> > >> +MODULE_LICENSE("GPL v2");
> > >> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> > >> new file mode 100644
> > >> index 0000000..f28cb28
> > >> --- /dev/null
> > >> +++ b/drivers/mfd/intel-lpss.h
> > >> @@ -0,0 +1,62 @@
> > >> +/*
> > >> + * Intel LPSS core support.
> > >> + *
> > >> + * Copyright (C) 2015, Intel Corporation
> > >> + *
> > >> + * Authors: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > >> + *          Mika Westerberg <mika.westerberg@...ux.intel.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.
> > >> + */
> > >> +
> > >> +#ifndef __MFD_INTEL_LPSS_H
> > >> +#define __MFD_INTEL_LPSS_H
> > >> +
> > >> +struct device;
> > >> +struct resource;
> > >> +
> > >> +struct intel_lpss_platform_info {
> > >> +     struct resource *mem;
> > >> +     int irq;
> > >> +     unsigned long clk_rate;
> > >> +     const char *clk_con_id;
> > >> +};
> > >> +
> > >> +int intel_lpss_probe(struct device *dev,
> > >> +                  const struct intel_lpss_platform_info *info);
> > >> +void intel_lpss_remove(struct device *dev);
> > >> +
> > >> +#ifdef CONFIG_PM
> > >> +int intel_lpss_prepare(struct device *dev);
> > >> +int intel_lpss_suspend(struct device *dev);
> > >> +int intel_lpss_resume(struct device *dev);
> > >
> > > Why are you exporting these?
> > 
> > LPSS devices are located in its own power domain. That's why we do all this.
> 
> Not sure I get that.
> 
> Looking closer it appears that the reason you do it is because you
> are registering the power ops in the bus specific (-acpi.c, -pci.c)
> probes.  What does that have to do with power domains?

Oops, my mistake since I was thinking it was done in similar way to
acpi_lpss.c driver, but you are right. It's about sharing for -acpi.c
and -pci.c.

(2)

> 
> > >> +#ifdef CONFIG_PM_SLEEP
> > >> +#define INTEL_LPSS_SLEEP_PM_OPS                      \
> > >> +     .prepare = intel_lpss_prepare,          \
> > >> +     .suspend = intel_lpss_suspend,          \
> > >> +     .resume = intel_lpss_resume,            \
> > >> +     .freeze = intel_lpss_suspend,           \
> > >> +     .thaw = intel_lpss_resume,              \
> > >> +     .poweroff = intel_lpss_suspend,         \
> > >> +     .restore = intel_lpss_resume,
> > >> +#endif
> > >
> > > SET_SYSTEM_SLEEP_PM_OPS?

Doesn't cover .prepare.

> > >
> > >> +#define INTEL_LPSS_RUNTIME_PM_OPS            \
> > >> +     .runtime_suspend = intel_lpss_suspend,  \
> > >> +     .runtime_resume = intel_lpss_resume,
> > >
> > > SET_RUNTIME_PM_OPS

Since above doesn't cover prepare we follow the same style.

> > >
> > >> +#else /* !CONFIG_PM */
> > >> +#define INTEL_LPSS_SLEEP_PM_OPS
> > >> +#define INTEL_LPSS_RUNTIME_PM_OPS
> > >> +#endif /* CONFIG_PM */
> > >> +
> > >> +#define INTEL_LPSS_PM_OPS(name)                      \
> > >> +const struct dev_pm_ops name = {             \
> > >> +     INTEL_LPSS_SLEEP_PM_OPS                 \
> > >> +     INTEL_LPSS_RUNTIME_PM_OPS               \
> > >> +}
> > >
> > > Why is this in the header?

Same as in (2)

We are about to send v2 of the series which is reworked a lot. We
addressed most of your comments regarding to function names, LTR stuff
and minor amendments.

I think we may continue discussion on new series then.

-- 
Andy Shevchenko <andriy.shevchenko@...el.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists