[U-Boot] [RFC PATCH] gpio: zynq: Setup bank_name to dev->name
Stefan Herbrechtsmeier
stefan at herbrechtsmeier.net
Wed Aug 1 18:23:21 UTC 2018
Am 30.07.2018 um 10:00 schrieb Michal Simek:
> On 27.7.2018 11:14, Stefan Herbrechtsmeier wrote:
>> Am 27.07.2018 um 08:42 schrieb Michal Simek:
>>> On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
>>>> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>>>>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>>>>> Am 25.07.2018 um 08:07 schrieb Michal Simek:
>>>>>>> On 24.7.2018 21:39, Stefan Herbrechtsmeier wrote:
>>>>>>>> Am 24.07.2018 um 10:37 schrieb Michal Simek:
>>>>>>>>> On 23.7.2018 20:29, Stefan Herbrechtsmeier wrote:
>>>>>>>>>> Am 23.07.2018 um 11:08 schrieb Michal Simek:
>>>>>>>>>>> On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>>> Am 12.07.2018 um 16:04 schrieb Michal Simek:
>>>>>>>>>>>>> There should be proper bank name setup to distiguish between
>>>>>>>>>>>>> different
>>>>>>>>>>>>> gpio drivers. Use dev->name for it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> drivers/gpio/zynq_gpio.c | 2 ++
>>>>>>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/gpio/zynq_gpio.c
>>>>>>>>>>>>> b/drivers/gpio/zynq_gpio.c
>>>>>>>>>>>>> index 26f69b1a713f..f793ee5754a8 100644
>>>>>>>>>>>>> --- a/drivers/gpio/zynq_gpio.c
>>>>>>>>>>>>> +++ b/drivers/gpio/zynq_gpio.c
>>>>>>>>>>>>> @@ -337,6 +337,8 @@ static int zynq_gpio_probe(struct udevice
>>>>>>>>>>>>> *dev)
>>>>>>>>>>>>> struct zynq_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>>>> struct gpio_dev_priv *uc_priv =
>>>>>>>>>>>>> dev_get_uclass_priv(dev);
>>>>>>>>>>>>> + uc_priv->bank_name = dev->name;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> if (priv->p_data)
>>>>>>>>>>>>> uc_priv->gpio_count = priv->p_data->ngpio;
>>>>>>>>>>>>>
>>>>>>>>>>>> Does this not lead to ugly names because the gpio number is
>>>>>>>>>>>> append to
>>>>>>>>>>>> the bank_name? Have you check the "gpio status -a" output?
>>>>>>>>>>> Yes I was checking it. Names are composed together but also just
>>>>>>>>>>> numbers
>>>>>>>>>>> works as before.
>>>>>>>>>>>
>>>>>>>>>>> gpio at ff0a00000: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00001: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00002: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00003: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00004: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00005: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00006: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00007: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00008: input: 0 [ ]
>>>>>>>>>>> gpio at ff0a00009: input: 0 [ ]
>>>>>>>>>> Do you think that this are meaningful names? It isn't possible to
>>>>>>>>>> separate the device and pin number as well as it mix hex and
>>>>>>>>>> decimal
>>>>>>>>>> numbers.
>>>>>>>>>>
>>>>>>>>>>> If you know better way how to setup a bank name please let me
>>>>>>>>>>> know
>>>>>>>>>>> but I
>>>>>>>>>>> need to distinguish ps gpio from pl one and for pl we need to
>>>>>>>>>>> know
>>>>>>>>>>> the
>>>>>>>>>>> address.
>>>>>>>>>> I know the use case.
>>>>>>>>>>
>>>>>>>>>> A lot of drivers use the bank_name from the device tree, some
>>>>>>>>>> drivers
>>>>>>>>>> append an underscore to the bank name and others add the
>>>>>>>>>> req_seq of
>>>>>>>>>> the
>>>>>>>>>> device to an alphabetic character.
>>>>>>>>>>
>>>>>>>>>>>> Other drivers use the gpio-bank-name from the device tree.
>>>>>>>>>>> I can't see this property inside Linux kernel. If this has been
>>>>>>>>>>> reviewed
>>>>>>>>>>> by dt guys please let me know.
>>>>>>>>>> This property is only used by u-boot. I think it isn't needed
>>>>>>>>>> by the
>>>>>>>>>> Linux kernel.
>>>>>>>>> I am happy to use consistent solution but what's that?
>>>>>>>> Consistent solution between what?
>>>>>>> all drivers. Name should be composed consistently among all drivers.
>>>>>>> It means drivers shouldn't add additional "_" in driver code for
>>>>>>> example.
>>>>>>>
>>>>>>>>> Mixing name with hex and int is not nice but adding "_" or
>>>>>>>>> something
>>>>>>>>> else is just a pain in driver code. If this is done in core I am
>>>>>>>>> fine
>>>>>>>>> with that but adding this code to all drivers don't look like
>>>>>>>>> generic
>>>>>>>>> solution at all.
>>>>>>>> Normally the bank name is an alphabetic character or string.
>>>>>>>> Maybe we
>>>>>>>> could add the device name to the gpio_lookup_name function and
>>>>>>>> add an
>>>>>>>> additional optional device name parameter to the gpio command.
>>>>>>>>
>>>>>>>>> Using additional u-boot property is not good too.
>>>>>>>>>
>>>>>>>>> I have mentioned in "gpio: xilinx: Convert driver to DM"
>>>>>>>>> (sha1:10441ec9224d0d269dc512819a32c0785a6338d3)
>>>>>>>>> that uc-priv->name is completely unused. Maybe this should be
>>>>>>>>> dev->name
>>>>>>>>> and bank_name should be really used for banks.
>>>>>>>> Isn't the uc-priv->name used for the label of the request?
>>>>>>> Last time when I looked at it and I didn't see this name listed
>>>>>>> anywhere
>>>>>>> in output.
>>>>>>>
>>>>>>>
>>>>>>>>> Then in gpio status -a can be
>>>>>>>>>
>>>>>>>>> Device gpio at a0001000:
>>>>>>>>> Bank:
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> but not sure how gpio commands will work to address exact pin from
>>>>>>>>> prompt. Because this is normally working
>>>>>>>>> gpio toggle gpio at a00010001
>>>>>>>> With an optional device name this would be:
>>>>>>>> gpio toggle gpio at a0001000 1
>>>>>>>>
>>>>>>>> Alternative the gpio command could support the requested labels:
>>>>>>>> gpio toggle second-gpio
>>>>>>> I am open to see this solution. Feel free to invest your time support
>>>>>>> this but I have no time for that.
>>>>>> What does this mean?
>>>>>>
>>>>>> I understand that you don't have the time to develop a new common
>>>>>> solution.
>>>>>>
>>>>>> But the discussion disappoints me. You misuse the bank name in a poor
>>>>>> way and decline alternative solutions with additional requirements
>>>>>> even
>>>>>> if they are already used in u-boot.
>>>>>>
>>>>>> The name "gpio at a000100011" for pin 11 of the device gpio at a0001000
>>>>>> isn't
>>>>>> consistent between the u-boot drivers nor is the name used in Linux. A
>>>>>> bank-name from the device tree is used by several u-boot drivers
>>>>>> even if
>>>>>> it isn't consistent between the drivers.
>>>>> I am sorry that you feel like that. It is not about declining
>>>>> alternative solution. I want to know what's the right solution is.
>>>> Thanks a lot for taking the time to write a detail explanation.
>>>>
>>>>> Using bank-name/gpio-bank-name via DT is something what is IMHO not
>>>>> correct.
>>>>> The first thing is if this is used just by u-boot it should have at
>>>>> least u-boot prefix. It means u-boot,bank-name, u-boot,gpio-bank-name.
>>>>> (Even I am not sure if u-boot prefix is properly allocated and can be
>>>>> allocated via sort of vendor-prefix).
>>>> I agree with you.
>>>>
>>>>> The second thing is that I don't think it is good to have two different
>>>>> dts files. One in the kernel and second in u-boot. Even I know we have
>>>>> problem with that but we are trying to sync it as much as possible.
>>>> Does the kernel accept properly allocated but not used entries?
>>> kernel like a code doesn't care and ignores what it is not used. But DT
>>> maintainers as far as I know don't like these options.
>>>
>>>>> Regarding others options:
>>>>>
>>>>> at91_gpio: plat->bank_name - which is not filled for OF case. (2 chars
>>>>> via platdata)
>>>>> da8xx - plat->port_name - which is not filled for OF case and also no
>>>>> user
>>>>>
>>>>> altera_pio, hsdk, msm, pm8916, sandbox: gpio-bank-name
>>>>> intel_broadwell, intel_ich6: bank-name:
>>>>>
>>>>> pcf8575 - gpio-bank-name or fdt_get_name
>>>>>
>>>>> atmel_pio4, s5p, vybrid: fdt_get_name
>>>>> bcm6345, rcar: dev->name
>>>>>
>>>>>
>>>>> hi6220, imx, mxc, omap: "GPIO%d_" plat->bank_index or banknum
>>>>> mpc8xxx: "MPC@%lx_" data->addr
>>>>> pca953x: "%s@%x_", label, info->addr or "gpio@%x_", info->addr
>>>>> axp_gpio : AXP0- hardcoded - "-" prefix
>>>>> bcm2835 - GPIO - without anything
>>>>> sunxi: PA + bank
>>>>> tegra, tegra186: 2char names via list
>>>>> mvebu: 'A' + dev->req_seq
>>>>> pic32, rk: 'A' + bank + some ligc around dev->name
>>>>>
>>>>> stm32f7: st,bank-name
>>>>>
>>>>> ###################################################################
>>>>> Sum:
>>>>> 2 are not OF
>>>>> 1 is using one prefix (st)
>>>>> 7,5 are using gpio-bank-name or bank-name
>>>>> 5.5 are using dev->name (+2 xilinx which are not listed now)
>>>>> 14 are using own prefixes - made or hardcoded
>>>>> 6 of them are ending with _
>>>>> 1 ends with -
>>>>> 7 don't end with - or _
>>>>>
>>>>> Cases with gpio-bank-name, bank-name I have described above.
>>>>>
>>>>> In case of "_" or "-" suffix Bank name will be listed with this suffix
>>>>> which also doesn't look good. GPIO names below with appended number
>>>>> looks good.
>>>>>
>>>>> ZynqMP> gpio status -a
>>>>> Bank GPIO_: <================ HERE
>>>>> GPIO_0: input: 0 [ ]
>>>>> GPIO_1: input: 0 [ ]
>>>>> GPIO_2: input: 0 [ ]
>>>>> GPIO_3: input: 0 [ ]
>>>>> GPIO_4: input: 0 [ ]
>>>>> GPIO_5: input: 0 [ ]
>>>>> GPIO_6: input: 0 [ ]
>>>>> GPIO_7: input: 0 [ ]
>>>>> GPIO_8: input: 0 [ ]
>>>> This doesn't look good but therefore the pin name looks okay.
>>> yes.
>>>
>>>>> And I hope it is clear that I can't make this bank_name empty or we
>>>>> will
>>>>> end up with this when PL gpios are included which is total mess.
>>>> I have the same problem.
>>> ok.
>>>
>>>>> It means what I have used and send patch for is used in 5,5 other cases
>>>>> and they could have the same issue which we are talking about.
>>>> The problem I see is that you introduce a suboptimal API which make it
>>>> hard to changed later or do you accept incompatible changes?
>>> I didn't introduce any API. API was there and I haven't done any change
>>> there. I just setup a name which the whole interface expects.
>>> gpio numbers are still working without any change. It means gpio toggle
>>> 170 just works without any breakage.
>> But we couldn't change the bank name after the release.
> It will be the best to have one stable - yes.
>
>>> This change is new and will be new in 2018.09 release and we haven't
>>> reached rc1 yet. It means we still have enough time to keep this, revert
>>> this, find a better solution or use temporary solution (with that _ for
>>> example).
>>> The same stuff is used by zynq and axi gpio drivers.
>> Okay
>>
>>>>> If you think that we should append _ in the driver then I would expect
>>>>> that we should also remove _ it from name when Bank XXX_ is listed.
>>>> This would be okay for me.
>>>>
>>>> But maybe there is a better solution. Would it be okay to add an device
>>>> tree alias for the gpios?
>>> I have convinced Linus to accept dt alias wiring for zynq gpio added by
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17
>>>
>> Okay
>>
>>> but in general statement from the is that gpios don't need to use
>>> aliases. In this case alias ID setup base chip ID for sysfs interface
>>> which is deprecated.
>> This means the alias is okay but shouldn't be used from user space?
>>
>>>> In this case we could use the seq number to
>>>> select the device:
>>>>
>>>> gpio set 0 2
>>>> gpio set 1 6
>>>>
>>>> The first number would be the seq and the second the offset. This would
>>>> make the bank name obsolete and could be backward compatible implemented
>>>> in the gpio command.
>>> seq number is setup based on aliases(if enabled - and in this gpio case
>>> not recommended) or based on bind/probe order.
>> Why isn't it recommended?
> Feel free to check with them but my understanding is that if alias is
> not used by core/drivers then it shouldn't be listed because it is not
> clear what it is for. Rob was asking me to remove these gpio aliases
> ,which I had in xilinx tree, when I was pushing zynqmp dts to mainline.
>
>
>>> aliases - we know that this is not good to do
>>> probe order - depends on DT, clock, etc
>> But isn't this widely used in u-boot?
> Some uclasses enables that options. In u-boot seq alias is present for
> gpio uclass.
> Also keep in your mind that it depends on option which can be disabled
> (DM_SEQ_ALIAS)
>
>
>>> I don't think it is a good idea to use seq in this case.
>> I don't understand why it is widely used if it isn't a good idea.
>>
>> At the moment the gpio framework use a bank name to distinguish between
>> different controllers of the same type. We need to distinguish between
>> different gpio controllers and hardware configurations. The right place
>> for hardware configuration is the device tree. This means we have to add
>> additional information into the device tree. This could be a bank-name
>> per controller or an device tree alias. The later approach is already
>> used by different other frameworks. It implements a low level interface
>> for u-boot and makes the bank name obsolete.
> It is about the flow and recent discussion with had in connection to
> CMD_DM. To find which seq is which controller you need to run dm command
> (which is for debug only) or list fdt(where u-boot dt address is not
> setup by default and also it doesn't reflect if driver was binded).
> Maybe it is not issue for others but looking for information which
> controller is which one seems to me additional step. Feel free to enable
> this option. dev->seq is filled by core automatically.
Thanks for the informations.
> Regarding dt property. I have no problem to use property which are
> already the part of binding. It means using label which describe what it
> is for seems to be the best candidate to use (used by pca953x_gpio in
> one case).
> But again we have the same issue with this approach when name ends with
> hex number and normally the name won't contain "_" suffix.
>
> Anyway I have not a problem with this change.
>
> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
> index 6fbaafb3fa3c..aa8d51e2f709 100644
> --- a/drivers/gpio/zynq_gpio.c
> +++ b/drivers/gpio/zynq_gpio.c
> @@ -336,8 +336,18 @@ static int zynq_gpio_probe(struct udevice *dev)
> {
> struct zynq_gpio_platdata *platdata = dev_get_platdata(dev);
> struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> -
> - uc_priv->bank_name = dev->name;
> + const void *label_ptr;
> + void *label_c;
> + int size;
> +
> + label_ptr = dev_read_prop(dev, "label", &size);
> + if (label_ptr) {
> + label_c = calloc(1, size);
> + memcpy(label_c, label_ptr, size);
> + uc_priv->bank_name = label_c;
> + } else {
> + uc_priv->bank_name = dev->name;
> + }
>
> if (platdata->p_data)
> uc_priv->gpio_count = platdata->p_data->ngpio;
This would be okay for me.
Best regards
Stefan
More information about the U-Boot
mailing list