Re: [PATCH 2/3] pinctrl: meson-a1: add pinctrl driver for Meson A1 Soc

[Qianggui Song <qianggui.song@xxxxxxxxxxx>]

On 2019/9/20 0:26, Jerome Brunet wrote:
> On Wed 18 Sep 2019 at 14:36, Qianggui Song <qianggui.song@xxxxxxxxxxx> wrote:
> 
>> On 2019/9/17 22:07, Jerome Brunet wrote:
>>>
>>> On Tue 17 Sep 2019 at 13:51, Qianggui Song <qianggui.song@xxxxxxxxxxx> wrote:
>>>>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>>>>>> index 8bba9d0..885b89d 100644
>>>>>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>>>>>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>>>>>> @@ -688,8 +688,12 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
>>>>>>  
>>>>>>  	pc->reg_ds = meson_map_resource(pc, gpio_np, "ds");
>>>>>>  	if (IS_ERR(pc->reg_ds)) {
>>>>>> -		dev_dbg(pc->dev, "ds registers not found - skipping\n");
>>>>>> -		pc->reg_ds = NULL;
>>>>>> +		if (pc->data->reg_layout == A1_LAYOUT) {
>>>>>> +			pc->reg_ds = pc->reg_pullen;
>>>>>
>>>>> IMO, this kind of ID based init fixup is not going to scale and will
>>>>> lead to something difficult to maintain in the end.
>>>>>
>>>>> The way the different register sets interract with each other is already
>>>>> pretty complex to follow.
>>>>>
>>>>> You could rework this in 2 different ways:
>>>>> #1 - Have the generic function parse all the register sets and have all
>>>>> drivers provide a specific (as in gxbb, gxl, axg, etc ...)  function to :
>>>>>  - Verify the expected sets have been provided
>>>>>  - Make assignement fixup as above if necessary
>>>>>
>>>>> #2 - Rework the driver to have only one single register region
>>>>>  I think one of your colleague previously mentionned this was not
>>>>>  possible. It is still unclear to me why ...
>>>>>
>>>> Appreciate your advice.  I have an idea based on #1, how about providing
>>>> only two dt parse function, one is for chips before A1(the old one),
>>>> another is for A1 and later chips that share the same layout. Assign
>>>> these two functions to their own driver.
>>>
>>> That's roughly the same thing as your initial proposition with function
>>> pointer instead of IDs ... IMO, this would still be a quick fix to
>>> address your immediate topic instead of dealing with the driver as
>>> whole, which is my concern here.
>>>
>> For #1. It would be like
>> generic_parse_dt()
>> {
>> 	1. parse all register regions (mux gpio pull pull_en ds)
>> 	
>> 	2. call  specific function through function pointer in
>>  	   meson_pinctrl_data.(each platform should have AO and EE two
>>            specific functions for they are not the same)
>> 	{
>> 		do work you mentioned above
>> 	}
>> }
>> right ?
>> If that so, maybe there are a lot of duplicated codes
> 
> Only if you make it so. Providing a callback and duplicating code are
> not the same thing
> 
>> for most Socs share the same reg layout.
> 
> That's not really accurate:
> 
> So far they all have the "mux" and "gpio" region but
> 
> gxbb, gxl, axg, meson8 EE:
>  has: pull, pull-en
>  remap: non
>  unsupported: ds
> 
> gxbb, gxl, axg, meson8 AO:
>  has: pull
>  remap: pull-en -> pull
>  unsupported: ds
> 
> g12 and sm1 EE:
>  has: pull, pull-en, ds
>  remap: none
> 
> g12 and sm1 AO:
>  has: ds
>  remap: pull->gpio, pull_en->gpio
> 
> And now a1 chip remaps "ds" to "pull_en" ...
> 
> As said previouly all this is getting pretty difficult to follow and
> maintain. Adding a proper callback for each meson pinctrl would make the
> above explicit in the code ... which helps maintain thing, at least for
> a while ...
> 
> Judging by the offsets between those regions, I still think one single
> region would make things a whole lot simpler. If it is not possible to
> map it with one single region, could you tell us why ? What non-pinctrl
> related device do we have there ?
>Here I mean duplicated is that m8/m8b/gxl/gxbb/axg use the same layout,
while g12a/b/sm1 are the same, so don't need to implement every Socs
parser functions just as I said below AXG type for m8/m8b/gxl/gxbb/axg,
g12a type for g12a/b/sm1, the last one is for a1. three types functions
(with ao and ee) can cover all platform. But I still consider that
providing an extra meson_a1_pasert_dt like function for a1 or later is
more simpler.

The reason why we can not use one single region for previous Socs, it'
that there is non-pinctrl related device for some Socs region.
Take an example for g12a:

#define	PREG_PAD_GPIO0_EN_N	(0xff634400 + (0x010 << 2))
#define	PREG_PAD_GPIO0_O	(0xff634400 + (0x011 << 2))
#define	PREG_PAD_GPIO0_I	(0xff634400 + (0x012 << 2))

	...continue region...

#define	PAD_PULL_UP_EN_REG5	(0xff634400 + (0x04d << 2))

	... ETH/NAND/VPU/TIMER...  a lot of no-pinctrl registers

#define	PERIPHS_PIN_MUX_0	(0xff634400 + (0x0b0 << 2))

	...continue region...

#define	PERIPHS_PIN_MUX_F	(0xff634400 + (0x0bf << 2))

	...no use region...

#define	EFUSE_CFG_LOCK		(0xff634400 + (0x0c0 << 2))

	...other EFUSE relative registers...
	...no use region...

#define	PAD_DS_REG0A		(0xff634400 + (0x0d0 << 2))

	...continue region for all ds...

#define	PAD_DS_REG5A		(0xff634400 + (0x0d6 << 2))

So from the above we can see there are lots of other registers between
gpio and ds register.When map gpio to ds region, they will bury a lot of
other registers.

>> So I guess five specific functions are
>> enough: AXG and before(ao,ee), G12A(ao,ee) and A1(will place them in
>> pinctrl_meson.c). Since m8 to AXG are the same register layout for both
>> ee and ao, G12A with new feature ds and new ao register layout.
>>
>> Or I misunderstood the #1 ?
>>>>>> +		} else {
>>>>>> +			dev_dbg(pc->dev, "ds registers not found - skipping\n");
>>>>>> +			pc->reg_ds = NULL;
>>>>>> +		}
>>>>>>  	}
>>>>>>  
>>>>>>  	return 0;
>>>>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
>>>>>> index c696f32..3d0c58d 100644
>>>>>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>>>>>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>>>>>> @@ -80,6 +80,14 @@ enum meson_pinconf_drv {
>>>>>>  };
>>>>>>  
>>>>>>  /**
>>>>>> + * enum meson_reg_layout - identify two types of reg layout
>>>>>> + */
>>>>>> +enum meson_reg_layout {
>>>>>> +	LEGACY_LAYOUT,
>>>>>> +	A1_LAYOUT,
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>>   * struct meson bank
>>>>>>   *
>>>>>>   * @name:	bank name
>>>>>> @@ -114,6 +122,7 @@ struct meson_pinctrl_data {
>>>>>>  	unsigned int num_banks;
>>>>>>  	const struct pinmux_ops *pmx_ops;
>>>>>>  	void *pmx_data;
>>>>>> +	unsigned int reg_layout;
>>>>>>  };
>>>>>>  
>>>>>>  struct meson_pinctrl {
>>>>>
>>>>> .
>>>>>
>>>
>>> .
>>>
> 
> .
>