Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov <nborisov@suse.com>]

On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
> Hi,
> 
> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
>> Based on the datasheet this implements support for the hwspinlock IP
>> block.
> 
> How was this tested?

I tested it on my pine64 lts e.g. loading the driver and reading the
reset/clock/sysstatus registers to ensure everything is unmasked and has
expected values.

> 
> There's also a lot of checkpatch issues, make sure you fix those.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  drivers/hwspinlock/Kconfig            |   9 ++
>>  drivers/hwspinlock/Makefile           |   1 +
>>  drivers/hwspinlock/sunxi_hwspinlock.c | 181 ++++++++++++++++++++++++++
>>  3 files changed, 191 insertions(+)
>>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
>>
>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>> index 37740e992cfa..ebc1ea48ef16 100644
>> --- a/drivers/hwspinlock/Kconfig
>> +++ b/drivers/hwspinlock/Kconfig
>> @@ -68,3 +68,12 @@ config HSEM_U8500
>>  	  SoC.
>>
>>  	  If unsure, say N.
>> +
>> +config HWSPINLOCK_SUNXI
>> +	tristate "Allwinner Hardware Spinlock device"
>> +	depends on ARCH_SUNXI
>> +	depends on HWSPINLOCK
>> +	help
>> +	  Say y here to support the SUNXI Hardware Spinlock device.
>> +
>> +	  If unsure, say N.
> 
> sunxi doesn't really mean anything though, the A10 is also part of the
> sunxi family and doesn't have that IP. Similarly, nothing prevents a
> future SoC from changing that design. The first SoC that used it was
> the A33 iirc, so let's just use sun8i.

Fair enough, I will use the same for the symbols as well. TBH the
nomenclature is quite confusing in allwinner land...

Actually since this driver will initially support A64 shouldn't the
symbols really be prefixed with sun50i, since looking at
https://linux-sunxi.org/Allwinner_SoC_Family sun8i pertains to 32 bit A7
cores?

> 
<snip>
>> +
>> +	/*
>> +	 * make sure the module is enabled and clocked before reading
>> +	 * the module SYSSTATUS register
>> +	 */
> 
> You don't define that register anywhere?
> 
>> +	clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Cannot enable clock\n");
>> +		return ret;
>> +	}
> 
> Can't we do that with runtime_pm?

Probably can but I'm new to device driver development so I don't
understand all the implications of using the pm framework. I saw that
other drivers did this but atm it's terra incognita to me.

> 
>> +	/* Disable soft reset */
>> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> +        if (IS_ERR(reset)) {
>> +                ret = PTR_ERR(reset);
>> +                goto out_declock;
>> +        }
>> +        reset_control_deassert(reset);
> 
> We might have the same issue than the mailbox driver where the
> firmware will need to access the block at any time, so we can't really
> toggle the reset line as we want.

What should we do then ?

> 
>> +	num_locks = sunxi_get_num_locks(io_base);
>> +	if (!num_locks) {
>> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
>> +		ret = -EINVAL;
>> +		goto out_reset;
>> +	}
>> +
>> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
>> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);
>> +	if (!hw) {
>> +		ret = -ENOMEM;
>> +		goto out_reset;
>> +	}
> 
> That looks rather convoluted (especially since the variable length
> array is at the second level), and can be made more obvious by:
> 
> - Removing the hwspinlock_device from sunxi_hwspinlock and allocating
>   both separately.
> 
> - And then allocate the hwspinlock_device separately with struct_size

Actually this can be allocated via struct_size e.g. struct_size(hw,
bank.lock, num_locks).

> 
>> +	hw->clk = clk;
>> +	hw->reset = reset;
> 
> Why not using the structure directly instead of having temporary
> variables?

Because I use the clk/reset before allocating the devmem.

> 
>> +	io_base += LOCK_BASE_OFFSET;
>> +	for (i = 0; i < num_locks; i++)
>> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> 
> Using a define for the registers offset would be nice here. 

you mean something like:

# define LOCK(X) (x * sizeof(u32))

I think this brings more needless indirection than helps but if you
insist I won't mind.

> 
>> +	platform_set_drvdata(pdev, hw);
>> +
>> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
>> +				   0, num_locks);
>> +
>> +	if (!ret)
>> +		return ret;
> 
> That's a slightly weird construct too, since in pretty much all the
> driver you return early on error and here you return early on
> success. Just return ret if there's an error just like you're doing
> everywhere else, and return 0 after that.
> 
>> +out_reset:
>> +	reset_control_assert(reset);
>> +out_declock:
>> +	clk_disable_unprepare(clk);
>> +	return ret;
>> +}
>> +

<snip>

>> +/* board init code might need to reserve hwspinlocks for predefined purposes */
>> +postcore_initcall(sunxi_hwspinlock_init);
> 
> We don't have any board init code. Can't we just use a regular
> module_platform_driver call here?

Perhaps we can, I will test on my pine64.
> 
> Thanks!
> Maxime
> 

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