[PATCH 1/5] clk: ti: add custom API for memory access

[Tero Kristo]

On 28/04/2021 20:31, Dario Binacchi wrote:
> Hi Tero,
> 
>> Il 27/04/2021 09:01 Tero Kristo <kristo at kernel.org> ha scritto:
>>
>>   
>> Hi Dario,
>>
>> One question below.
>>
>> On 25/04/2021 17:17, Dario Binacchi wrote:
>>> As pointed by [1] and [2], commit
>>> d64b9cdcd4 ("fdt: translate address if #size-cells = <0>") is wrong:
>>> - It makes every 'reg' DT property translatable. It changes the address
>>>     translation so that for an I2C 'reg' address you'll get back as reg
>>>     the I2C controller address + reg value.
>>> - The quirk must be fixed with platform code.
>>>
>>> The clk_ti_get_reg_addr() is the platform code able to make the correct
>>> address translation for the AM33xx clocks registers. Its implementation
>>> was inspired by the Linux Kernel code.
>>>
>>> [1] https://patchwork.ozlabs.org/project/uboot/patch/1614324949-61314-1-git-send-email-bmeng.cn@gmail.com/
>>> [2] https://lore.kernel.org/linux-clk/20210402192054.7934-1-dariobin@libero.it/T/
>>>
>>> Signed-off-by: Dario Binacchi <dariobin at libero.it>
>>> ---
>>>
>>>    drivers/clk/ti/clk.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>>    drivers/clk/ti/clk.h | 13 +++++++
>>>    2 files changed, 105 insertions(+)
>>>
>>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
>>> index c999df213a..68abe053cb 100644
>>> --- a/drivers/clk/ti/clk.c
>>> +++ b/drivers/clk/ti/clk.c
>>> @@ -6,10 +6,23 @@
>>>     */
>>>    
>>>    #include <common.h>
>>> +#include <dm.h>
>>>    #include <fdtdec.h>
>>> +#include <regmap.h>
>>>    #include <asm/io.h>
>>> +#include <dm/device_compat.h>
>>>    #include "clk.h"
>>>    
>>> +#define CLK_MAX_MEMMAPS           10
>>> +
>>> +struct clk_iomap {
>>> +	struct regmap *regmap;
>>> +	ofnode node;
>>> +};
>>> +
>>> +static unsigned int clk_memmaps_num;
>>> +static struct clk_iomap clk_memmaps[CLK_MAX_MEMMAPS];
>>> +
>>>    static void clk_ti_rmw(u32 val, u32 mask, fdt_addr_t reg)
>>>    {
>>>    	u32 v;
>>> @@ -33,3 +46,82 @@ void clk_ti_latch(fdt_addr_t reg, s8 shift)
>>>    	clk_ti_rmw(0, latch, reg);
>>>    	readl(reg);		/* OCP barrier */
>>>    }
>>> +
>>> +void clk_ti_writel(u32 val, struct clk_ti_reg *reg)
>>> +{
>>> +	struct clk_iomap *io = &clk_memmaps[reg->index];
>>> +
>>> +	regmap_write(io->regmap, reg->offset, val);
>>> +}
>>> +
>>> +u32 clk_ti_readl(struct clk_ti_reg *reg)
>>> +{
>>> +	struct clk_iomap *io = &clk_memmaps[reg->index];
>>> +	u32 val;
>>> +
>>> +	regmap_read(io->regmap, reg->offset, &val);
>>> +	return val;
>>> +}
>>> +
>>> +#if CONFIG_IS_ENABLED(AM33XX)
>>
>> Why do you have this ifdef here? These drivers are not planned to be
>> used by anything but am33xx, or they don't work on any other device?
>>
> 
> The patch was developed and tested for the AM33XX SOC (beaglebone black).
> The drivers in the clk/ti folder were added by my patches but can also be
> used by boards based on different device trees. In those cases, if required,
> platform versions of clk_ti_get_regmap_node() could be implemented.

Ok, but this could be handled simply via the defconfigs? None of these 
drivers are enabled explicitly for any platform afaics. If someone wants 
to try this out, it might work out of box for, say am43xx also. Having 
to deal with this ifdef seems counter intuitive.

-Tero

> 
> Thanks and regards,
> Dario
> 
>> -Tero
>>
>>> +static ofnode clk_ti_get_regmap_node(struct udevice *dev)
>>> +{
>>> +	ofnode node = dev_ofnode(dev), parent;
>>> +
>>> +	if (!ofnode_valid(node))
>>> +		return ofnode_null();
>>> +
>>> +	parent = ofnode_get_parent(node);
>>> +	if (strcmp(ofnode_get_name(parent), "clocks"))
>>> +		return ofnode_null();
>>> +
>>> +	return ofnode_get_parent(parent);
>>> +}
>>> +#else
>>> +static ofnode clk_ti_get_regmap_node(struct udevice *dev)
>>> +{
>>> +	return ofnode_null();
>>> +}
>>> +#endif
>>> +
>>> +int clk_ti_get_reg_addr(struct udevice *dev, int index, struct clk_ti_reg *reg)
>>> +{
>>> +	ofnode node;
>>> +	int i, ret;
>>> +	u32 val;
>>> +
>>> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "reg", index, &val);
>>> +	if (ret) {
>>> +		dev_err(dev, "%s must have reg[%d]\n", ofnode_get_name(node),
>>> +			index);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* parent = ofnode_get_parent(parent); */
>>> +	node = clk_ti_get_regmap_node(dev);
>>> +	if (!ofnode_valid(node)) {
>>> +		dev_err(dev, "failed to get regmap node\n");
>>> +		return -EFAULT;
>>> +	}
>>> +
>>> +	for (i = 0; i < clk_memmaps_num; i++) {
>>> +		if (ofnode_equal(clk_memmaps[i].node, node))
>>> +			break;
>>> +	}
>>> +
>>> +	if (i == clk_memmaps_num) {
>>> +		if (i == CLK_MAX_MEMMAPS)
>>> +			return -ENOMEM;
>>> +
>>> +		ret = regmap_init_mem(node, &clk_memmaps[i].regmap);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		clk_memmaps[i].node = node;
>>> +		clk_memmaps_num++;
>>> +	}
>>> +
>>> +	reg->index = i;
>>> +	reg->offset = val;
>>> +	return 0;
>>> +}
>>> diff --git a/drivers/clk/ti/clk.h b/drivers/clk/ti/clk.h
>>> index 601c3823f7..ea36d065ac 100644
>>> --- a/drivers/clk/ti/clk.h
>>> +++ b/drivers/clk/ti/clk.h
>>> @@ -9,5 +9,18 @@
>>>    #define _CLK_TI_H
>>>    
>>>    void clk_ti_latch(fdt_addr_t reg, s8 shift);
>>> +/**
>>> + * struct clk_ti_reg - TI register declaration
>>> + * @offset: offset from the master IP module base address
>>> + * @index: index of the master IP module
>>> + */
>>> +struct clk_ti_reg {
>>> +	u16 offset;
>>> +	u8 index;
>>> +};
>>> +
>>> +void clk_ti_writel(u32 val, struct clk_ti_reg *reg);
>>> +u32 clk_ti_readl(struct clk_ti_reg *reg);
>>> +int clk_ti_get_reg_addr(struct udevice *dev, int index, struct clk_ti_reg *reg);
>>>    
>>>    #endif /* #ifndef _CLK_TI_H */
>>>