From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Thu, 2 Aug 2018 13:33:54 +0200 Subject: [U-Boot] [RFC PATCH] gpio: zynq: Setup bank_name to dev->name In-Reply-To: References: <3a5a2fbe9d0aad4fdbbbf197c39dc0f973e5045e.1531404282.git.michal.simek@xilinx.com> <589cfccf-dc79-c84f-f147-36a2ac7b4aee@herbrechtsmeier.net> <9ea0f292-f1e4-bc31-b4d4-fa2b68c737e1@herbrechtsmeier.net> <7c732bfa-ee2c-af2c-1cdc-d155a3b5a186@xilinx.com> <6c5495e1-ad56-8c7a-9d99-2855f0a52a46@herbrechtsmeier.net> <44ee8611-d91d-2266-ad92-ba1b2bf5e4dd@xilinx.com> <8fe8a826-0a73-cb6c-7368-e97cd8e3c1ec@herbrechtsmeier.net> <7195399b-1273-2e46-a2e6-a3431e188eca@xilinx.com> <7b73b429-2f1d-1ac4-bf7a-fd743fd7f899@herbrechtsmeier.net> <0effb92c-2014-d2be-3a23-9c04ab5bdb43@xilinx.com> Message-ID: <993126ab-6c46-ca7c-7b07-830cb811a448@xilinx.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> >>>>>>>>>>>>>>         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