Re: [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver

[Paul Cercueil]

Hi Thierry,

On Mon, Mar 4, 2019 at 1:22 PM, Thierry Reding 
<thierry.reding@xxxxxxxxx> wrote:
> On Sat, Mar 02, 2019 at 08:33:50PM -0300, Paul Cercueil wrote:
> [...]
> >  diff --git a/drivers/clocksource/ingenic-timer.c 
> > b/drivers/clocksource/ingenic-timer.c
> [...]
> >  +struct ingenic_tcu {
> >  +	const struct ingenic_soc_info *soc_info;
> >  +	struct regmap *map;
> >  +	struct clk *clk, *timer_clk, *cs_clk;
> >  +
> >  +	struct irq_domain *domain;
> >  +	unsigned int nb_parent_irqs;
> >  +	u32 parent_irqs[3];
> >  +
> >  +	struct clk_hw_onecell_data *clocks;
> >  +
> >  +	unsigned int timer_channel, cs_channel;
> >  +	struct clock_event_device cevt;
> >  +	struct clocksource cs;
> >  +	char name[4];
> >  +
> >  +	unsigned long pwm_channels_mask;
> >  +};
> >  +
> >  +static struct ingenic_tcu *ingenic_tcu;
>
> Is there really a need for this? Seems like the only two reasons for 
> why
> you keep this around are as pointed out below.
>
> >  +void __iomem *ingenic_tcu_base;
> >  +EXPORT_SYMBOL_GPL(ingenic_tcu_base);
>
> Same here.
>
> >  +static u64 notrace ingenic_tcu_timer_read(void)
> >  +{
> >  +	unsigned int channel = ingenic_tcu->cs_channel;
> >  +	u16 count;
> >  +
> >  +	count = readw(ingenic_tcu_base + TCU_REG_TCNTc(channel));
>
> Can't yo do this via the regmap?

I could, but for the sched_clock to be precise the function must return
as fast as possible. That's the rationale behind the use of readw() 
here.

That's also the reason why ingenic_tcu_base is global and exported, so
that the OS Timer driver can use it as well.

> >  +
> >  +	return count;
> >  +}
> >  +
> >  +static u64 notrace ingenic_tcu_timer_cs_read(struct clocksource 
> > *cs)
> >  +{
> >  +	return ingenic_tcu_timer_read();
> >  +}
>
> And here you've got access to the struct clocksource *, from which you
> should be able to get at struct ingenic_tcu * using container_of.
>
> [...]
> >  +static int __init ingenic_tcu_init(struct device_node *np)
> >  +{
> >  +	const struct of_device_id *id = 
> > of_match_node(ingenic_tcu_of_match, np);
> >  +	const struct ingenic_soc_info *soc_info = id->data;
> >  +	struct ingenic_tcu *tcu;
> >  +	struct resource res;
> >  +	void __iomem *base;
> >  +	long rate;
> >  +	int ret;
> >  +
> >  +	of_node_clear_flag(np, OF_POPULATED);
> >  +
> >  +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> >  +	if (!tcu)
> >  +		return -ENOMEM;
> >  +
> >  +	/* Enable all TCU channels for PWM use by default except channel 0
> >  +	 * and channel 1.
> >  +	 */
> >  +	tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
> >  +	of_property_read_u32(np, "ingenic,pwm-channels-mask",
> >  +			     (u32 *)&tcu->pwm_channels_mask);
> >  +
> >  +	/* Verify that we have at least two free channels */
> >  +	if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels - 
> > 2) {
> >  +		pr_crit("ingenic-tcu: Invalid PWM channel mask: 0x%02lx\n",
> >  +			tcu->pwm_channels_mask);
> >  +		return -EINVAL;
> >  +	}
> >  +
> >  +	tcu->soc_info = soc_info;
> >  +	ingenic_tcu = tcu;
> >  +
> >  +	tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
> >  +						 soc_info->num_channels);
> >  +	tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> >  +					     soc_info->num_channels,
> >  +					     tcu->timer_channel + 1);
> >  +
> >  +	base = of_io_request_and_map(np, 0, NULL);
> >  +	if (IS_ERR(base)) {
> >  +		ret = PTR_ERR(base);
> >  +		goto err_free_ingenic_tcu;
> >  +	}
> >  +
> >  +	of_address_to_resource(np, 0, &res);
> >  +
> >  +	ingenic_tcu_base = base;
> >  +
> >  +	tcu->map = regmap_init_mmio(NULL, base, 
> > &ingenic_tcu_regmap_config);
> >  +	if (IS_ERR(tcu->map)) {
> >  +		ret = PTR_ERR(tcu->map);
> >  +		goto err_iounmap;
> >  +	}
> >  +
> >  +	tcu->clk = of_clk_get_by_name(np, "tcu");
> >  +	if (IS_ERR(tcu->clk)) {
> >  +		ret = PTR_ERR(tcu->clk);
> >  +		pr_crit("ingenic-tcu: Unable to find TCU clock: %i\n", ret);
> >  +		goto err_free_regmap;
> >  +	}
> >  +
> >  +	ret = clk_prepare_enable(tcu->clk);
> >  +	if (ret) {
> >  +		pr_crit("ingenic-tcu: Unable to enable TCU clock: %i\n", ret);
> >  +		goto err_clk_put;
> >  +	}
> >  +
> >  +	ret = ingenic_tcu_intc_init(tcu, np);
> >  +	if (ret)
> >  +		goto err_clk_disable;
> >  +
> >  +	ret = ingenic_tcu_clk_init(tcu, np);
> >  +	if (ret)
> >  +		goto err_tcu_intc_cleanup;
> >  +
> >  +	ret = ingenic_tcu_clocksource_init(tcu);
> >  +	if (ret)
> >  +		goto err_tcu_clk_cleanup;
> >  +
> >  +	ret = ingenic_tcu_timer_init(tcu);
> >  +	if (ret)
> >  +		goto err_tcu_clocksource_cleanup;
> >  +
> >  +	// Register the sched_clock at the very end as there's no way to 
> > undo it
> >  +	rate = clk_get_rate(tcu->cs_clk);
> >  +	sched_clock_register(ingenic_tcu_timer_read, 16, rate);
>
> Oh wow... so you managed to nicely encapsulate everything and now this
> seems to be the only reason why you need to rely on global variables.
>
> That's unfortunate. I suppose we could go and add a void *data 
> parameter
> to sched_clock_register() and pass that to the read() function. That 
> way
> you could make this completely independent of global variables, but
> there are 73 callers of sched_clock_register() and they are spread all
> over the place, so sounds a bit daunting to me.

Yes, that's the main reason behind the use of a global variables.
Is there a way we could introduce another callback, e.g. .read_value(),
that would receive a void *param? Then the current .read() callback
can be deprecated and the 73 callers can be migrated later.

> >  +
> >  +	return 0;
> >  +
> >  +err_tcu_clocksource_cleanup:
> >  +	ingenic_tcu_clocksource_cleanup(tcu);
> >  +err_tcu_clk_cleanup:
> >  +	ingenic_tcu_clk_cleanup(tcu, np);
> >  +err_tcu_intc_cleanup:
> >  +	ingenic_tcu_intc_cleanup(tcu);
> >  +err_clk_disable:
> >  +	clk_disable_unprepare(tcu->clk);
> >  +err_clk_put:
> >  +	clk_put(tcu->clk);
> >  +err_free_regmap:
> >  +	regmap_exit(tcu->map);
> >  +err_iounmap:
> >  +	iounmap(base);
> >  +	release_mem_region(res.start, resource_size(&res));
> >  +err_free_ingenic_tcu:
> >  +	kfree(tcu);
> >  +	return ret;
> >  +}
> >  +
> >  +TIMER_OF_DECLARE(jz4740_tcu_intc,  "ingenic,jz4740-tcu",  
> > ingenic_tcu_init);
> >  +TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu", 
> > ingenic_tcu_init);
> >  +TIMER_OF_DECLARE(jz4770_tcu_intc,  "ingenic,jz4770-tcu",  
> > ingenic_tcu_init);
> >  +
> >  +
> >  +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> >  +{
> >  +	platform_set_drvdata(pdev, ingenic_tcu);
>
> Then there's also this. Oh well... nevermind then.

The content of ingenic_tcu_platform_init() could be moved inside
ingenic_tcu_init(). But can we get a hold of the struct device before 
the
probe function is called? That would allow to set the drvdata and regmap
without relying on global state.

> >  +bool ingenic_tcu_pwm_can_use_chn(unsigned int channel)
> >  +{
> >  +	return !!(ingenic_tcu->pwm_channels_mask & BIT(channel));
> >  +}
> >  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>
> I wonder if we could make this slightly nicer, though. Judging by the
> name, this one seems like it would only ever be used by the PWM. If 
> the
> PWM is a child of the TCU, could it not pass in a struct device * that
> points to its parent into this? Or perhaps pass its own struct device 
> *
> and have this function look up the struct ingenic_tcu from that? 
> Should
> be something trivial as:
>
> 	bool ingenic_tcu_pwm_can_use_channel(struct device *dev, unsigned 
> int channel)
> 	{
> 		struct ingenic_tcu *tcu = dev_get_drvdata(dev->parent);
>
> 		return !!(tcu->pwm_channels_mask & BIT(channel));
> 	}
>
> Thierry

Yes, that definitely looks better. Thanks for the hint.

-Paul