| Message ID | 20220916120749.2517727-1-benedikt.niedermayr@siemens.com |
|---|---|
| Headers | show |
| Series | gpmc wait-pin additions | expand |
Hi, On 20/09/2022 15:17, Niedermayr, BENEDIKT wrote: > Hi Krzysztof, > > On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote: >> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote: >>>> I commented exactly below the line which I question. I don't question >>>> other lines. So let me be a bit more specific: >>>> >>>> Why do you need >>>> "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT" >>>> ? Can you write a scenario where this is useful? >>>> >>> Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me... >>> >>> If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT". >> >> Exactly this will happen. As expected. This value cannot appear in DTS, >> therefore I would expect error message. >> >> Now you allow such value in DTS which is not the same as your bindings. >> > And now I completely got it... > With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT... > > Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional > property anymore. > If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed. > Is this an acceptable solution for you? > No this is not acceptable. As current implementations don't define polarity and rely on reset defaults. You can check return value of "of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait_pin_polarity))" " * Return: 0 on success, -EINVAL if the property does not exist, * -ENODATA if property does not have a value, and -EOVERFLOW if the * property data isn't large enough." If property is present you don't need to check for WAITPINPOLARITY_DEFAULT as that is not valid value for this property. If property is not present you force WAITPINPOLARITY_DEFAULT and don't print error message. > >> >>> But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from >>> DT), >>> and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings(). >>> So in gpmc_cs_program_settings() if: >>> p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register >>> p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW -> Issua a write to the GPMC_CONFIG register >>> p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT -> Do not touch the GPMC_CONFIG register >>> >>> We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise >>> we might break platforms which rely on these reset values. >> >> Best regards, >> Krzysztof > Cheers, > benedikt > cheers, -roger
From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> Currently it is not possible to configure the WAIT0PINPOLARITY and WAIT1PINPOLARITY bits of the GPMC_CONFIG register directly via device tree properties. It is also not possible to use the same wait-pin for different cs-regions. While the current implementation may fullfill most usecases, it may not be sufficient for more complex setups (e.g. FPGA/ASIC interfaces), where more complex interfacing options where possible. For example interfacing an ASIC which offers multiple cs-regions but only one waitpin the current driver and dt-bindings are not sufficient. While using the same waitpin for different cs-regions worked for older kernels (4.14) the omap-gpmc.c driver refused to probe (-EBUSY) with newer kernels (>5.10). Changes since v1: * Rebase against recent 6.0.0-rc3 kernel, but the maintainers list stays the same! * Updated eMail recipients list Changes since v2: * Remove the gpmc register configuration out of the gpiochip callbacks. In this case the memory interface configuration is not done via gpio bindings. * Some minor code fixes * Changed git commit descriptions Change since v3: * Use a uint32 dt-property instean a boolean one * If the dt-property is not set, then don't touch the GPMC_CONFIG register * Changed git commit descriptions Changes since v4: * Use checkpatch with "--strict" option * Moved wait-pin sanity checks to gpmc_read_settings_dt() * Always assign WAITPINPOLARITY_DEFAULT on error cases * Track waitpin allocation within gpmc for determine allocation origin Benedikt Niedermayr (3): memory: omap-gpmc: allow shared wait pins memory: omap-gpmc: add support for wait pin polarity dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity .../memory-controllers/ti,gpmc-child.yaml | 7 ++++ drivers/memory/omap-gpmc.c | 39 ++++++++++++++++++- include/linux/platform_data/gpmc-omap.h | 6 +++ 3 files changed, 50 insertions(+), 2 deletions(-) -- 2.34.1