[U-Boot] [RFC PATCH] gpio: zynq: Setup bank_name to dev->name
Michal Simek
michal.simek at xilinx.com
Thu Aug 2 11:33:54 UTC 2018
On 1.8.2018 20:23, Stefan Herbrechtsmeier wrote:
> 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.
Ok. I have sent regular patch for that.
Thanks,
Michal
More information about the U-Boot
mailing list