Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Philipp Zabel <p.zabel@pengutronix.de>]

On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
> Add initial PRM (Power and Reset Management) driver for TI OMAP class
> SoCs. Initially this driver only supports reset control, but can be
> extended to support rest of the functionality, like powerdomain
> control, PRCM irq support etc.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  arch/arm/mach-omap2/Kconfig |   1 +
>  drivers/soc/ti/Makefile     |   1 +
>  drivers/soc/ti/omap_prm.c   | 235 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/soc/ti/omap_prm.c
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index fdb6743760a2..ad08d470a2ca 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>  	select TI_SYSC
>  	select OMAP_IRQCHIP
>  	select CLKSRC_TI_32K
> +	select ARCH_HAS_RESET_CONTROLLER
>  	help
>  	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>  
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index b3868d392d4f..788b5cd1e180 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
>  knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>  obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
> +obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
>  obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>  obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>  obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> new file mode 100644
> index 000000000000..fd5c431f8736
> --- /dev/null
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OMAP2+ PRM driver
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *	Tero Kristo <t-kristo@ti.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Why <linux/module.h>? This is a builtin driver.

> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/delay.h>
> +
> +struct omap_rst_map {
> +	s8 rst;
> +	s8 st;
> +};
> +
> +struct omap_prm_data {
> +	u32 base;
> +	const char *name;
> +	u16 rstctrl;
> +	u16 rstst;
> +	const struct omap_rst_map *rstmap;
> +	u8 flags;
> +};

I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
would make sense. That could be linked from omap_reset_data directly.
That only makes sense if there'd be enough cases where it can be reused
for multiple PRMs instances.

> +
> +struct omap_prm {
> +	const struct omap_prm_data *data;
> +	void __iomem *base;
> +};
> +
> +struct omap_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct omap_prm *prm;
> +};
> +
> +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> +
> +#define OMAP_MAX_RESETS		8
> +#define OMAP_RESET_MAX_WAIT	10000
> +
> +#define OMAP_PRM_HAS_RSTCTRL	BIT(0)
> +#define OMAP_PRM_HAS_RSTST	BIT(1)
> +
> +#define OMAP_PRM_HAS_RESETS	(OMAP_PRM_HAS_RSTCTRL | OMAP_PRM_HAS_RSTST)
> +
> +static const struct of_device_id omap_prm_id_table[] = {
> +	{ },
> +};
> +
> +static bool _is_valid_reset(struct omap_reset_data *reset, unsigned long id)
> +{
> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {

If rstmap is never NULL,

	while (map->rst >= 0) {

should be enough.

> +		if (map->rst == id)
> +			return true;
> +		map++;
> +	}
> +
> +	return false;
> +}
> +
> +static int omap_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Don't check this in the status/(de)assert/reset callbacks. Instead,
implement rcdev.of_xlate and return -EINVAL there, so that invalid ids
can never be requested.

> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +	v &= 1 << id;
> +	v >>= id;

omap_get_st_bit below makes it look like the status bit can be in a
different place than the reset control bit, should that be used here as
well?

> +
> +	return v;
> +}
> +
> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Same as above.

> +	/* assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
> +	v |= 1 << id;
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

This read-modify-write should be protected with a lock.

> +
> +	return 0;
> +}
> +
> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
> +				 unsigned long id)
> +{
> +	const struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {

Same as above.

> +		if (map->rst == id)
> +			return map->st;
> +
> +		map++;
> +	}
> +
> +	return id;
> +}
> +
> +/*
> + * Note that status will not change until clocks are on, and clocks cannot be
> + * enabled until reset is deasserted. Consumer drivers must check status
> + * separately after enabling clocks.
> + */
> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +	int st_bit;
> +	bool has_rstst;
> +
> +	if (!_is_valid_reset(reset, id))
> +		return -EINVAL;

Same as above.

> +	/* check the current status to avoid de-asserting the line twice */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);
> +	if (!(v & BIT(id)))
> +		return -EEXIST;

What is the purpose of this? For shared consumers the core already does
refcounting, and I expect exclusive consumers won't deassert twice.
Since the reset signal is deasserted after this call, this should not
return an error.

> +
> +	has_rstst = reset->prm->data->rstst ||
> +		(reset->prm->data->flags & OMAP_PRM_HAS_RSTST);
> +
> +	if (has_rstst) {
> +		st_bit = omap_reset_get_st_bit(reset, id);
> +
> +		/* Clear the reset status by writing 1 to the status bit */
> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +		v |= 1 << st_bit;
> +		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);

What does the value read from the rstst register indicate? Is it the
current state of the reset line? Is it 0 until deassertion is completed,
and then it turns to 1?

> +	}
> +
> +	/* de-assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl);

Reading the register again seems unnecessary.

> +	v &= ~(1 << id);
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

As above, the read-modify-write should be locked.

> +
> +	return 0;
> +}
> +
> +static const struct reset_control_ops omap_reset_ops = {
> +	.assert		= omap_reset_assert,
> +	.deassert	= omap_reset_deassert,
> +	.status		= omap_reset_status,
> +};
> +
> +static int omap_prm_reset_init(struct platform_device *pdev,
> +			       struct omap_prm *prm)
> +{
> +	struct omap_reset_data *reset;
> +
> +	/*
> +	 * Check if we have controllable resets. If either rstctrl is non-zero
> +	 * or OMAP_PRM_HAS_RSTCTRL flag is set, we have reset control register
> +	 * for the domain.
> +	 */
> +	if (!prm->data->rstctrl && !(prm->data->flags & OMAP_PRM_HAS_RSTCTRL))
> +		return 0;
> +
> +	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> +	if (!reset)
> +		return -ENOMEM;
> +
> +	reset->rcdev.owner = THIS_MODULE;
> +	reset->rcdev.ops = &omap_reset_ops;
> +	reset->rcdev.of_node = pdev->dev.of_node;
> +	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> +
> +	reset->prm = prm;
> +
> +	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
> +}
> +
> +static int omap_prm_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	const struct omap_prm_data *data;
> +	struct omap_prm *prm;
> +	const struct of_device_id *match;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;

This can be merged with devm_ioremap_resource below.

> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENOTSUPP;
> +
> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> +	if (!prm)
> +		return -ENOMEM;
> +
> +	data = match->data;
> +
> +	while (data->base != res->start) {
> +		if (!data->base)
> +			return -EINVAL;
> +		data++;
> +	}

Is this not something that you want to have encoded in the compatible
string? They all have a different register layout.

> +
> +	prm->data = data;
> +
> +	prm->base = devm_ioremap_resource(&pdev->dev, res);

	prm->base = devm_platform_ioremap_resource(pdev, 0);

> +	if (!prm->base)
> +		return -ENOMEM;
> +
> +	return omap_prm_reset_init(pdev, prm);
> +}
> +
> +static struct platform_driver omap_prm_driver = {
> +	.probe = omap_prm_probe,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= omap_prm_id_table,
> +	},
> +};
> +builtin_platform_driver(omap_prm_driver);

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel