Re: [PATCH] usb: chipidea: tegra: fix flexible_array.cocci warnings
13.02.2021 23:09, Julia Lawall пишет:
> From: kernel test robot
>
> Zero-length and one-element arrays are deprecated, see
> Documentation/process/deprecated.rst
> Flexible-array members should be used instead.
>
> Generated by: scripts/coccinelle/misc/flexible_array.cocci
>
> Fixes: fc53d5279094 ("usb: chipidea: tegra: Support host mode")
> CC: Peter Geis
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
> Signed-off-by: Julia Lawall
>
> ---
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> usb-next
> head: 7a1e838d0cdce7d09a0bd81d45c7b5a660e71ac7
> commit: fc53d5279094e38e6363506339772a7021da2df8 [64/198] usb: chipidea:
> tegra: Support host mode
> :: branch date: 19 hours ago
> :: commit date: 4 weeks ago
>
> host.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -32,7 +32,7 @@ struct ehci_ci_priv {
> struct ci_hdrc_dma_aligned_buffer {
> void *kmalloc_ptr;
> void *old_xfer_buffer;
> - u8 data[0];
> + u8 data[];
> };
>
> static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>
Reviewed-by: Dmitry Osipenko
Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver
02.02.2021 16:22, Jon Hunter пишет: > So this is very similar to tegra_rt5677, is it not possible to support > both codecs with the same machine driver? These codecs require individual configurations and those "../codecs/rt5631.h" and "../codecs/rt5677.h" aren't compatible at a quick glance. The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new driver should be a better base anyways. Overall it shouldn't worth time and effort trying to squeeze these drivers into a single one, IMO.
Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver
31.01.2021 21:41, Ion Agorria пишет:
> + np_codec = of_parse_phandle(pdev->dev.of_node, "nvidia,audio-codec", 0);
> + if (!np_codec) {
> + dev_err(&pdev->dev,
> + "Property 'nvidia,audio-codec' missing or invalid\n");
> + return -EINVAL;
> + }
> +
> + np_i2s = of_parse_phandle(pdev->dev.of_node, "nvidia,i2s-controller",
> 0);
> + if (!np_i2s) {
> + dev_err(&pdev->dev,
> + "Property 'nvidia,i2s-controller' missing or
> invalid\n");
> + return -EINVAL;
> + }
We missed that the np_codec and np_i2s should be put when driver is released.
https://elixir.bootlin.com/linux/v5.11-rc6/source/drivers/of/base.c#L1429
We could fix it with a devm helper in v2.
diff --git a/sound/soc/tegra/tegra_rt5631.c b/sound/soc/tegra/tegra_rt5631.c
index 9034f48bcb26..84f23915bd95 100644
--- a/sound/soc/tegra/tegra_rt5631.c
+++ b/sound/soc/tegra/tegra_rt5631.c
@@ -172,6 +172,30 @@ static struct snd_soc_card snd_soc_tegra_rt5631 = {
.fully_routed = true,
};
+static void tegra_rt5631_node_release(void *of_node)
+{
+ of_node_put(of_node);
+}
+
+static struct device_node *
+tegra_rt5631_parse_phandle(struct device *dev, const char *name)
+{
+ struct device_node *np;
+ int err;
+
+ np = of_parse_phandle(dev->of_node, name, 0);
+ if (!np) {
+ dev_err(dev, "Property '%s' missing or invalid\n", name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ err = devm_add_action_or_reset(dev, tegra_rt5631_node_release, np);
+ if (err)
+ return ERR_PTR(err);
+
+ return np;
+}
+
static int tegra_rt5631_probe(struct platform_device *pdev)
{
struct snd_soc_card *card = &snd_soc_tegra_rt5631;
@@ -209,19 +233,13 @@ static int tegra_rt5631_probe(struct platform_device
*pdev)
if (ret)
return ret;
- np_codec = of_parse_phandle(pdev->dev.of_node, "nvidia,audio-codec", 0);
- if (!np_codec) {
- dev_err(&pdev->dev,
- "Property 'nvidia,audio-codec' missing or invalid\n");
- return -EINVAL;
- }
+ np_codec = tegra_rt5631_parse_phandle(&pdev->dev, "nvidia,audio-codec");
+ if (IS_ERR(np_codec))
+ return PTR_ERR(np_codec);
- np_i2s = of_parse_phandle(pdev->dev.of_node, "nvidia,i2s-controller",
0);
- if (!np_i2s) {
- dev_err(&pdev->dev,
- "Property 'nvidia,i2s-controller' missing or
invalid\n");
- return -EINVAL;
- }
+ np_i2s = tegra_rt5631_parse_phandle(&pdev->dev,
"nvidia,i2s-controller");
+ if (!np_i2s)
+ return PTR_ERR(np_i2s);
tegra_rt5631_dai.cpus->of_node = np_i2s;
tegra_rt5631_dai.codecs->of_node = np_codec;
Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver
02.02.2021 19:24, Jon Hunter пишет: > > On 02/02/2021 15:25, Dmitry Osipenko wrote: >> 02.02.2021 16:22, Jon Hunter пишет: >>> So this is very similar to tegra_rt5677, is it not possible to support >>> both codecs with the same machine driver? >> >> These codecs require individual configurations and those >> "../codecs/rt5631.h" and "../codecs/rt5677.h" aren't compatible at a >> quick glance. > > Right but not all of that is needed. What is actually needed from the > header files? I recall that some downstream drivers were doing some special programming of codecs. This is not relevant to the current upstream drivers, but potentially it may become needed and then that single driver could become unmanageable. >> The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new >> driver should be a better base anyways. > > Sounds like a good time to update it :-) > >> Overall it shouldn't worth time and effort trying to squeeze these >> drivers into a single one, IMO. > > Not sure I agree when these drivers appear to be 90% the same. Of course we could try, but I suggest that it should be done separately from this series. Certainly it will take a lot of extra effort and this series isn't about improving older drivers, it's about enabling h/w support for the RT5631 codec. It shouldn't be a problem to switch to the common machine driver later on if this driver will turn out to be feasible.
Re: [PATCH V2 3/3] opp: Add dev_pm_opp_of_add_table_noclk()
02.02.2021 08:02, Viresh Kumar пишет: > On 01-02-21, 23:00, Dmitry Osipenko wrote: >> 28.01.2021 10:00, Viresh Kumar пишет: >>> A few drivers have device's clk but they don't want the OPP core to >>> handle that. Add a new helper for them, dev_pm_opp_of_add_table_noclk(). >>> >>> Signed-off-by: Viresh Kumar >>> --- >>> V2: >>> - Split this into a separate patch. >>> >>> drivers/opp/of.c | 18 ++ >>> include/linux/pm_opp.h | 6 ++ >>> 2 files changed, 24 insertions(+) >> >> For the all current/latest OPP patches: >> >> Tested-by: Dmitry Osipenko > > Hmm, I have now added your Tested-by to all the patches that we worked > together on.. > > Thanks. > That's exactly what I meant, thank you.
Re: [PATCH v1 0/2] ASoC: tegra: Add RT5631 machine driver
02.02.2021 16:23, Jon Hunter пишет: > > On 31/01/2021 18:40, Ion Agorria wrote: >> From: Ion Agorria >> >> Adds support for Tegra SoC devices with RT5631 sound codec. >> Playback to speakers, headphones and internal mic recording works. >> >> This driver is used for ASUS Transformer TF201, TF700T and others >> Tegra based devices containing RT5631. >> >> Svyatoslav Ryhel (2): >> ASoC: dt-bindings: tegra: Add binding for RT5631 >> ASoC: tegra: Add RT5631 machine driver >> >> .../sound/nvidia,tegra-audio-rt5631.yaml | 134 + >> sound/soc/tegra/Kconfig | 8 + >> sound/soc/tegra/Makefile | 2 + >> sound/soc/tegra/tegra_rt5631.c| 261 ++ >> 4 files changed, 405 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml >> create mode 100644 sound/soc/tegra/tegra_rt5631.c > > > I don't see any user of this driver included. Any reason why? They should come a bit later. The TF201/300T device-trees should be in a good state already, we just need to finalize them for upstream and send out. IIUC, the audio codec and dock station drivers are the most noticeable missing parts in upsteam kernel. Ion and Svyatoslav should be able to clarify the state of the devices in a more details.
Re: [PATCH] opp: Don't ignore clk_get() errors other than -ENODEV
29.01.2021 20:46, Viresh Kumar пишет:
> On 29-01-21, 18:23, Dmitry Osipenko wrote:
>> 29.01.2021 13:51, Viresh Kumar пишет:
>>> Not all devices that need to use OPP core need to have clocks, a missing
>>> clock is fine in which case -ENODEV shall be returned by clk_get().
>>>
>>> Anything else is an error and must be handled properly.
>>>
>>> Reported-by: Dmitry Osipenko
>>> Signed-off-by: Viresh Kumar
>>> ---
>>> Stephen, is the understanding correct that -ENODEV is the only error
>>> returned for missing clocks ?
>>>
>>> Dmitry: I hope this is on the lines of what you were looking for ?
>>
>> Viresh, thank you! This is not what I was looking for because clk core
>> doesn't return -ENODEV for a missing clock, but -ENOENT. The ENODEV
>> certainly should break drivers.
>
> My bad.
>
>> I was looking for this:
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0305861fee1b..3dd9cdbc0e75 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1264,7 +1264,7 @@ static struct opp_table
>> *_update_opp_table_clk(struct device *dev,
>> if (IS_ERR(opp_table->clk)) {
>> int ret = PTR_ERR(opp_table->clk);
>>
>> -if (ret == -EPROBE_DEFER) {
>> +if (ret != -ENOENT) {
>> dev_pm_opp_put_opp_table(opp_table);
>> return ERR_PTR(ret);
>> }
>
> You should be looking for this instead, isn't it ?
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 049d45e70807..4bfcbe5b57af 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1268,7 +1268,7 @@ static struct opp_table *_update_opp_table_clk(struct
> device *dev,
> if (!ret)
> return opp_table;
>
> - if (ret == -ENODEV) {
> + if (ret == -ENOENT) {
> dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> return opp_table;
> }
>
>
This will work too.
Then also could be better to replace the "if (ret != -EPROBE_DEFER)"
with dev_err_probe(..).
Re: [PATCH v5] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default
22.01.2021 22:35, Saravana Kannan пишет:
> There are multiple instances of GPIO device tree nodes of the form:
>
> foo {
> compatible = "acme,foo";
> ...
>
> gpio0: gpio0@ {
> compatible = "acme,bar";
> ...
> gpio-controller;
> };
>
> gpio1: gpio1@ {
> compatible = "acme,bar";
> ...
> gpio-controller;
> };
>
> ...
> }
>
> bazz {
> my-gpios = <&gpio0 ...>;
> }
>
> Case 1: The driver for "foo" populates struct device for these gpio*
> nodes and then probes them using a driver that binds with "acme,bar".
> This driver for "acme,bar" then registers the gpio* nodes with gpiolib.
> This lines up with how DT nodes with the "compatible" property are
> typically converted to struct devices and then registered with driver
> core to probe them. This also allows the gpio* devices to hook into all
> the driver core capabilities like runtime PM, probe deferral,
> suspend/resume ordering, device links, etc.
>
> Case 2: The driver for "foo" doesn't populate struct devices for these
> gpio* nodes before registering them with gpiolib. Instead it just loops
> through its child nodes and directly registers the gpio* nodes with
> gpiolib.
>
> Drivers that follow case 2 cause problems with fw_devlink=on. This is
> because fw_devlink will prevent bazz from probing until there's a struct
> device that has gpio0 as its fwnode (because bazz lists gpio0 as a GPIO
> supplier). Once the struct device is available, fw_devlink will create a
> device link with gpio0 device as the supplier and bazz device as the
> consumer. After this point, since the gpio0 device will never bind to a
> driver, the device link will prevent bazz device from ever probing.
>
> Finding and refactoring all the instances of drivers that follow case 2
> will cause a lot of code churn and it is not something that can be done
> in one shot. In some instances it might not even be possible to refactor
> them cleanly. Examples of such instances are [1] [2].
>
> This patch works around this problem and avoids all the code churn by
> simply setting the fwnode of the gpio_device and creating a stub driver
> to bind to the gpio_device. This allows all the consumers to continue
> probing when the driver follows case 2.
>
> [1] - https://lore.kernel.org/lkml/20201014191235.7f71fcb4@xhacker.debian/
> [2] -
> https://lore.kernel.org/lkml/e28e1f38d87c12a3c714a6573beba...@kernel.org/
> Cc: Marc Zyngier
> Cc: Jisheng Zhang
> Cc: Kever Yang
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Signed-off-by: Saravana Kannan
> ---
> v1 -> v2:
> - Fixed up compilation errors that were introduced accidentally
> - Fixed a missing put_device()
>
> v2 -> v3:
> - Changed chip_warn() to pr_warn()
> - Changed some variable names
>
> v3 -> v4:
> - Dropped the warning since it's not always valid
> - This simplifies the code a lot
> - Added comments and fixed up commit text
>
> v4 -> v5:
> - Fixed the code to not mess up non-DT cases.
> - Moved code into gpiolib-of.c
>
> drivers/gpio/gpiolib-of.c | 11 +++
> drivers/gpio/gpiolib-of.h | 5 +
> drivers/gpio/gpiolib.c| 38 +++---
> 3 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index b4a71119a4b0..baf0153b7bca 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -1039,3 +1039,14 @@ void of_gpiochip_remove(struct gpio_chip *chip)
> {
> of_node_put(chip->of_node);
> }
> +
> +void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
> +{
> + /* If the gpiochip has an assigned OF node this takes precedence */
> + if (gc->of_node)
> + gdev->dev.of_node = gc->of_node;
> + else
> + gc->of_node = gdev->dev.of_node;
> + if (gdev->dev.of_node)
> + gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);
> +}
> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
> index ed26664f1537..8af2bc899aab 100644
> --- a/drivers/gpio/gpiolib-of.h
> +++ b/drivers/gpio/gpiolib-of.h
> @@ -15,6 +15,7 @@ int of_gpiochip_add(struct gpio_chip *gc);
> void of_gpiochip_remove(struct gpio_chip *gc);
> int of_gpio_get_count(struct device *dev, const char *con_id);
> bool of_gpio_need_valid_mask(const struct gpio_chip *gc);
> +void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
> #else
> static inline struct gpio_desc *of_find_gpio(struct device *dev,
>const char *con_id,
> @@ -33,6 +34,10 @@ static inline bool of_gpio_need_valid_mask(const struct
> gpio_chip *gc)
> {
> return false;
> }
> +static inline void of_gpio_dev_init(struct gpio_chip *gc,
> + struct gpio_device *gdev)
> +{
> +}
> #endif /* CONFIG_OF_GPIO */
>
> extern struct notifier_block
Re: [PATCH v5] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default
01.02.2021 00:28, Saravana Kannan пишет: >> This patch causes these new errors on NVIDIA Tegra30 Nexus 7 using recent >> linux-next: >> >> gpio-1022 (cpu-pwr-req-hog): hogged as input >> max77620-pinctrl max77620-pinctrl: pin gpio4 already requested by >> max77620-pinctrl; cannot claim for gpiochip1 >> max77620-pinctrl max77620-pinctrl: pin-4 (gpiochip1) status -22 >> max77620-pinctrl max77620-pinctrl: could not request pin 4 (gpio4) from >> group gpio4 on device max77620-pinctrl >> gpio_stub_drv gpiochip1: Error applying setting, reverse things back >> gpio_stub_drv: probe of gpiochip1 failed with error -22 >> >> Please fix, thanks in advance. > I have a partial guess on why this is happening. So can you try this patch? > > Thanks, > Saravana > > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -4213,6 +4213,8 @@ static int gpio_stub_drv_probe(struct device *dev) > * gpio_device of the GPIO chip with the firmware node and then simply > * bind it to this stub driver. > */ > + if (dev->fwnode && dev->fwnode->dev != dev) > + return -EBUSY; > return 0; > } This change doesn't help, exactly the same errors are still there.
Re: [PATCH V2] opp: Don't ignore clk_get() errors other than -ENOENT
01.02.2021 07:22, Viresh Kumar пишет:
> Not all devices that need to use OPP core need to have clocks, a missing
> clock is fine in which case -ENOENT shall be returned by clk_get().
>
> Anything else is an error and must be handled properly.
>
> Reported-by: Dmitry Osipenko
> Signed-off-by: Viresh Kumar
> ---
> V2:
> - s/ENODEV/ENOENT
> - Use dev_err_probe()
>
> Stephen, is the understanding correct that -ENOENT is the only error
> returned for missing clocks ?
>
> drivers/opp/core.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index a518173fd64a..0beb3ee79523 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1252,6 +1252,8 @@ static struct opp_table *_update_opp_table_clk(struct
> device *dev,
> struct opp_table *opp_table,
> bool getclk)
> {
> + int ret;
> +
> /*
>* Return early if we don't need to get clk or we have already tried it
>* earlier.
> @@ -1261,18 +1263,19 @@ static struct opp_table *_update_opp_table_clk(struct
> device *dev,
>
> /* Find clk for the device */
> opp_table->clk = clk_get(dev, NULL);
> - if (IS_ERR(opp_table->clk)) {
> - int ret = PTR_ERR(opp_table->clk);
>
> - if (ret == -EPROBE_DEFER) {
> - dev_pm_opp_put_opp_table(opp_table);
> - return ERR_PTR(ret);
> - }
> + ret = PTR_ERR_OR_ZERO(opp_table->clk);
> + if (!ret)
> + return opp_table;
>
> + if (ret == -ENOENT) {
> dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> + return opp_table;
> }
>
> - return opp_table;
> + dev_pm_opp_put_opp_table(opp_table);
> +
> + return ERR_PTR(dev_err_probe(dev, ret, "Couldn't find clock\n"));
> }
>
> /*
>
Thanks, looks okay.
Although, I think the previous variant of coding style was a bit more
appropriate, i.e. like this:
dev_err_probe(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
return ERR_PTR(ret);
Re: [PATCH V2 11/13] devfreq: tegra30: Migrate to dev_pm_opp_set_opp()
27.01.2021 12:10, Viresh Kumar пишет:
> dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
> be used instead. Migrate to the new API.
>
> We don't want the OPP core to manage the clk for this driver, migrate to
> dev_pm_opp_of_add_table_noclk() to make sure dev_pm_opp_set_opp()
> doesn't have any side effects.
>
> Signed-off-by: Viresh Kumar
> ---
> Dmitry,
>
> This is based over the patches sent here:
>
> https://lore.kernel.org/lkml/6c2160ff30a8f421563793020264cf9f533f293c.1611738228.git.viresh.ku...@linaro.org/
>
> This should fix the problem you mentioned earlier. Will push this for
> linux-next unless you have any issues with it.
>
> drivers/devfreq/tegra30-devfreq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c
> b/drivers/devfreq/tegra30-devfreq.c
> index 117cad7968ab..31f7dec5990b 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -647,7 +647,7 @@ static int tegra_devfreq_target(struct device *dev,
> unsigned long *freq,
> return PTR_ERR(opp);
> }
>
> - ret = dev_pm_opp_set_bw(dev, opp);
> + ret = dev_pm_opp_set_opp(dev, opp);
> dev_pm_opp_put(opp);
>
> return ret;
> @@ -849,7 +849,7 @@ static int tegra_devfreq_probe(struct platform_device
> *pdev)
> return err;
> }
>
> - err = dev_pm_opp_of_add_table(&pdev->dev);
> + err = dev_pm_opp_of_add_table_noclk(&pdev->dev);
> if (err) {
> dev_err(&pdev->dev, "Failed to add OPP table: %d\n", err);
> goto put_hw;
>
Tested-by: Dmitry Osipenko
Re: [PATCH V2 3/3] opp: Add dev_pm_opp_of_add_table_noclk()
28.01.2021 10:00, Viresh Kumar пишет: > A few drivers have device's clk but they don't want the OPP core to > handle that. Add a new helper for them, dev_pm_opp_of_add_table_noclk(). > > Signed-off-by: Viresh Kumar > --- > V2: > - Split this into a separate patch. > > drivers/opp/of.c | 18 ++ > include/linux/pm_opp.h | 6 ++ > 2 files changed, 24 insertions(+) For the all current/latest OPP patches: Tested-by: Dmitry Osipenko
Re: [PATCH v5] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default
01.02.2021 23:15, Saravana Kannan пишет:
> On Mon, Feb 1, 2021 at 8:49 AM Dmitry Osipenko wrote:
>>
>> 01.02.2021 00:28, Saravana Kannan пишет:
>>>> This patch causes these new errors on NVIDIA Tegra30 Nexus 7 using recent
>>>> linux-next:
>>>>
>>>> gpio-1022 (cpu-pwr-req-hog): hogged as input
>>>> max77620-pinctrl max77620-pinctrl: pin gpio4 already requested by
>>>> max77620-pinctrl; cannot claim for gpiochip1
>>>> max77620-pinctrl max77620-pinctrl: pin-4 (gpiochip1) status -22
>>>> max77620-pinctrl max77620-pinctrl: could not request pin 4 (gpio4) from
>>>> group gpio4 on device max77620-pinctrl
>>>> gpio_stub_drv gpiochip1: Error applying setting, reverse things back
>>>> gpio_stub_drv: probe of gpiochip1 failed with error -22
>>>>
>>>> Please fix, thanks in advance.
>>> I have a partial guess on why this is happening. So can you try this patch?
>>>
>>> Thanks,
>>> Saravana
>>>
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -4213,6 +4213,8 @@ static int gpio_stub_drv_probe(struct device *dev)
>>> * gpio_device of the GPIO chip with the firmware node and then
>>> simply
>>> * bind it to this stub driver.
>>> */
>>> + if (dev->fwnode && dev->fwnode->dev != dev)
>>> + return -EBUSY;
>>> return 0;
>>> }
>>
>> This change doesn't help, exactly the same errors are still there.
>
> Sorry, I see what's happening. Try this instead. If it works, I'll
> send out a proper patch.
>
> Thanks,
> Saravana
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 8e0564c50840..f3d0ffe8a930 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -56,8 +56,10 @@
> static DEFINE_IDA(gpio_ida);
> static dev_t gpio_devt;
> #define GPIO_DEV_MAX 256 /* 256 GPIO chip devices supported */
> +static int gpio_bus_match(struct device *dev, struct device_driver *drv);
> static struct bus_type gpio_bus_type = {
> .name = "gpio",
> + .match = gpio_bus_match,
> };
>
> /*
> @@ -4199,6 +4201,14 @@ void gpiod_put_array(struct gpio_descs *descs)
> }
> EXPORT_SYMBOL_GPL(gpiod_put_array);
>
> +
> +static int gpio_bus_match(struct device *dev, struct device_driver *drv)
> +{
> + if (dev->fwnode && dev->fwnode->dev != dev)
> + return 0;
> + return 1;
> +}
> +
> static int gpio_stub_drv_probe(struct device *dev)
> {
> /*
>
This works, thank you!
Tested-by: Dmitry Osipenko
Re: [PATCH] rtc: tps65910: include linux/property.h
25.02.2021 16:42, Arnd Bergmann пишет:
> From: Arnd Bergmann
>
> The added device_property_present() call causes a build
> failure in some configurations because of the missing header:
>
> drivers/rtc/rtc-tps65910.c:422:7: error: implicit declaration of function
> 'device_property_present' [-Werror,-Wimplicit-function-declaration]
>
> Fixes: 454ba154a62c ("rtc: tps65910: Support wakeup-source property")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/rtc/rtc-tps65910.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/rtc/rtc-tps65910.c b/drivers/rtc/rtc-tps65910.c
> index 288abb1abdb8..bc89c62ccb9b 100644
> --- a/drivers/rtc/rtc-tps65910.c
> +++ b/drivers/rtc/rtc-tps65910.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
>
Reviewed-by: Dmitry Osipenko
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
25.02.2021 09:27, Nicolin Chen пишет: ... >> The partially revert should be okay, but it's not clear to me what makes >> difference for T124 since I don't see that problem on T30, which also >> has active display at a boot time. > > Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach > or from of_dma_configure_id/arch_setup_dma_ops? > I applied yours debug-patch, please see dmesg.txt attached to the email. Seems probe-defer of the tegra-dc driver prevents the implicit tegra_smmu_attach_dev, so it happens to work by accident. [0.00] Booting Linux on physical CPU 0x0 [0.00] Linux version 5.11.0-next-20210226-3-g89069e0f4a28 (dima@dimapc) (armv7a-hardfloat-linux-gnueabi-gcc (Gentoo 9.3.0-r2 p4) 9.3.0, GNU ld (Gentoo 2.26.1 p1.0) 2.26.1) #7012 SMP PREEMPT Sat Feb 27 11:49:27 MSK 2021 [0.00] CPU: ARMv7 Processor [412fc099] revision 9 (ARMv7), cr=50c5387d [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [0.00] OF: fdt: Machine model: ASUS Google Nexus 7 (Project Bach / ME370TG) E1565 [0.00] Memory policy: Data cache writealloc [0.00] Reserved memory: created CMA memory pool at 0xa000, size 256 MiB [0.00] OF: reserved mem: initialized node linux,cma@8000, compatible id shared-dma-pool [0.00] Zone ranges: [0.00] Normal [mem 0x-0x9f7f] [0.00] HighMem [mem 0x9f80-0xbfdf] [0.00] Movable zone start for each node [0.00] Early memory node ranges [0.00] node 0: [mem 0x8000-0xbfdf] [0.00] Initmem setup node 0 [mem 0x8000-0xbfdf] [0.00] On node 0 totalpages: 261632 [0.00] Normal zone: 1134 pages used for memmap [0.00] Normal zone: 0 pages reserved [0.00] Normal zone: 129024 pages, LIFO batch:31 [0.00] HighMem zone: 132608 pages, LIFO batch:31 [0.00] percpu: Embedded 21 pages/cpu s53324 r8192 d24500 u86016 [0.00] pcpu-alloc: s53324 r8192 d24500 u86016 alloc=21*4096 [0.00] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 260498 [0.00] Kernel command line: tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 init=/usr/lib/systemd/systemd root=/dev/nfs nfsroot=192.168.3.100:/nfs/root,v3,tcp rw ip=192.168.3.101::192.168.3.100:255.255.255.0::usb0 no_console_suspend gpt gpt_sector=61079551 console=tty1 no_console_suspend [0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear) [0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear) [0.00] mem auto-init: stack:off, heap alloc:off, heap free:off [0.00] Memory: 754680K/1046528K available (10240K kernel code, 1579K rwdata, 5404K rodata, 1024K init, 418K bss, 29704K reserved, 262144K cma-reserved, 268224K highmem) [0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 [0.00] ftrace: allocating 46557 entries in 91 pages [0.00] [ cut here ] [0.00] WARNING: CPU: 0 PID: 0 at arch/arm/kernel/insn.c:15 __arm_gen_branch+0x7f/0x84 [0.00] Modules linked in: [0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.11.0-next-20210226-3-g89069e0f4a28 #7012 [0.00] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [0.00] [] (unwind_backtrace) from [] (show_stack+0x11/0x14) [0.00] [] (show_stack) from [] (dump_stack+0x8d/0x9a) [0.00] [] (dump_stack) from [] (__warn+0xb7/0xc8) [0.00] [] (__warn) from [] (warn_slowpath_fmt+0x45/0x78) [0.00] [] (warn_slowpath_fmt) from [] (__arm_gen_branch+0x7f/0x84) [0.00] [] (__arm_gen_branch) from [] (ftrace_make_nop+0xf/0x20) [0.00] [] (ftrace_make_nop) from [] (ftrace_process_locs+0x211/0x320) [0.00] [] (ftrace_process_locs) from [] (ftrace_init+0x6f/0xbe) [0.00] [] (ftrace_init) from [] (start_kernel+0x42d/0x662) [0.00] [] (start_kernel) from [<>] (0x0) [0.00] random: get_random_bytes called from print_oops_end_marker+0x25/0x64 with crng_init=0 [0.00] ---[ end trace ]--- [0.00] [ ftrace bug ] [0.00] ftrace failed to modify [0.00] [] crash_notes_memory_init+0x4/0x34 [0.00] actual: 23:f0:9c:ec [0.00] Initializing ftrace call sites [0.00] ftrace record flags: 0
Re: tegra124-jetson-tk1: sata doesnt work since 5.2
31.05.2020 22:31, LABBE Corentin пишет: > On Thu, Mar 19, 2020 at 08:44:01AM +0100, LABBE Corentin wrote: >> Hello >> >> sata doesnt work on tegra124-jetson-tk1 on next and master and at least >> since 5.2 (but 5.1 works). >> [0.492810] +5V_SATA: supplied by +5V_SYS >> [0.493230] +12V_SATA: supplied by +VDD_MUX >> [2.088675] tegra-ahci 70027000.sata: 70027000.sata supply ahci not >> found, using dummy regulator >> [2.097643] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, >> using dummy regulator >> [3.314776] tegra-ahci 70027000.sata: 70027000.sata supply ahci not >> found, using dummy regulator >> [3.323658] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, >> using dummy regulator >> [5.236964] tegra-ahci 70027000.sata: 70027000.sata supply ahci not >> found, using dummy regulator >> [5.245867] tegra-ahci 70027000.sata: 70027000.sata supply phy not found, >> using dummy regulator >> [5.254706] tegra-ahci 70027000.sata: 70027000.sata supply target not >> found, using dummy regulator >> [5.310270] phy phy-sata.6: phy poweron failed --> -110 >> [5.315604] tegra-ahci 70027000.sata: failed to power on AHCI controller: >> -110 >> [5.323022] tegra-ahci: probe of 70027000.sata failed with error -110 >> [ 35.694269] +5V_SATA: disabling >> [ 35.697438] +12V_SATA: disabling >> >> I have bisected this problem: >> git bisect start >> # bad: [22c58fd70ca48a29505922b1563826593b08cc00] Merge tag 'armsoc-soc' of >> git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc >> git bisect bad 22c58fd70ca48a29505922b1563826593b08cc00 >> # good: [67e38f578aaebf34fc1278bbe45a78ee8c73dd33] ARM: ep93xx: move pinctrl >> interfaces into include/linux/soc >> git bisect good 67e38f578aaebf34fc1278bbe45a78ee8c73dd33 >> # good: [80f232121b69cc69a31ccb2b38c1665d770b0710] Merge >> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next >> git bisect good 80f232121b69cc69a31ccb2b38c1665d770b0710 >> # good: [e57ccca1ba33e1d92cc3bbf8b6304a46948844b0] Merge tag 'sound-5.2-rc1' >> of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound >> git bisect good e57ccca1ba33e1d92cc3bbf8b6304a46948844b0 >> # bad: [983dfa4b6ee556563f7963348e4e2f97fc8a15b8] Merge tag >> 'for-linus-5.2-rc1' of >> ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/rw/uml >> git bisect bad 983dfa4b6ee556563f7963348e4e2f97fc8a15b8 >> # good: [8e4ff713ce313dcabbb60e6ede1ffc193e67631f] Merge tag 'rtc-5.2' of >> git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux >> git bisect good 8e4ff713ce313dcabbb60e6ede1ffc193e67631f >> # bad: [b970afcfcabd63cd3832e95db096439c177c3592] Merge tag 'powerpc-5.2-1' >> of ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/powerpc/linux >> git bisect bad b970afcfcabd63cd3832e95db096439c177c3592 >> # bad: [601e6bcc4ef02bda2831d5ac8133947b5edf597b] Merge >> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net >> git bisect bad 601e6bcc4ef02bda2831d5ac8133947b5edf597b >> # good: [7e9c62bdb41af76974d594da89854a6aba645e58] Merge branches 'clk-sa', >> 'clk-aspeed', 'clk-samsung', 'clk-ingenic' and 'clk-zynq' into clk-next >> git bisect good 7e9c62bdb41af76974d594da89854a6aba645e58 >> # bad: [0caf000817353cfc5db22363ecdac63b83d3a3f9] Merge branch 'clk-ti' into >> clk-next >> git bisect bad 0caf000817353cfc5db22363ecdac63b83d3a3f9 >> # good: [5816b74581b45cf086a84ab14e13354a65e8e22c] Merge branches >> 'clk-hisi', 'clk-lochnagar', 'clk-allwinner', 'clk-rockchip' and 'clk-qoriq' >> into clk-next >> git bisect good 5816b74581b45cf086a84ab14e13354a65e8e22c >> # good: [7b4c162e03d47e037f8ee773c3e300eefb599a83] clk: at91: Mark struct >> clk_range as const >> git bisect good 7b4c162e03d47e037f8ee773c3e300eefb599a83 >> # bad: [e71f4d385878671991e200083c7d30eb4ca8e99a] clk: tegra: divider: Mark >> Memory Controller clock as read-only >> git bisect bad e71f4d385878671991e200083c7d30eb4ca8e99a >> # bad: [924ee3d551c9deb16090230b824988bd37e72aa8] clk: tegra: emc: Don't >> enable EMC clock manually >> git bisect bad 924ee3d551c9deb16090230b824988bd37e72aa8 >> # bad: [40db569d6769ffa3864fd1b89616b1a7323568a8] clk: tegra: Fix PLLM >> programming on Tegra124+ when PMC overrides divider >> git bisect bad 40db569d6769ffa3864fd1b89616b1a7323568a8 >> # bad: [bff1cef5f23afbe49f5ebd766980dc612f5e9d0a] clk: tegra: Don't enable >> already enabled PLLs >> git bisect bad bff1cef5f23afbe49f5ebd766980dc612f5e9d0a >> # first bad commit: [bff1cef5f23afbe49f5ebd766980dc612f5e9d0a] clk: tegra: >> Don't enable already enabled PLLs >> > > Hello > > I have digged a bit more and with the following "patch" I have now access to > sata again > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c > index 0b212cf2e794..b4e2020051d5 100644 > --- a/drivers/clk/tegra/clk-pll.c > +++ b/drivers/clk/tegra/clk-pll.c > @@ -1602,7 +1603,7 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw) > unsigned long input_rate; > > if (clk_pll_
Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties
19.05.2020 10:28, Ulf Hansson пишет: > On Sat, 16 May 2020 at 17:44, Dmitry Osipenko wrote: >> >> Several people asked me about the MMC warnings in the KMSG log and >> I had to tell to ignore them because these warning are irrelevant to >> pre-Tegra210 SoCs. > > Why are the warnings irrelevant? That's what the DT binding doc says [1]. [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt Although, looking at the driver's code and TRM docs, it seems that all those properties are really irrelevant only to the older Terga20 SoC. So the binding doc is a bit misleading. Nevertheless, the binding explicitly says that the properties are optional, which is correct. >> It should be up to a board's device-tree writer to >> properly describe all the necessary properties. Secondly, eventually all >> device-tree bindings will be converted to YAML, which allows to validate >> board DT files, giving a warning about missing properties. Hence let's >> remove the noisy warnings to stop the confusion. > > Yep, makes sense. However, perhaps we should do this conversion then, > rather than first drop the warnings? I don't mind to postpone this patch. But again, IIUC, all these properties are optional, and thus, there is no critical need to verify them in DT right now, it could be done later on.
Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties
19.05.2020 19:24, Thierry Reding пишет:
> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote:
>> 19.05.2020 10:28, Ulf Hansson пишет:
>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko wrote:
>>>>
>>>> Several people asked me about the MMC warnings in the KMSG log and
>>>> I had to tell to ignore them because these warning are irrelevant to
>>>> pre-Tegra210 SoCs.
>>>
>>> Why are the warnings irrelevant?
>>
>> That's what the DT binding doc says [1].
>>
>> [1]
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt
>>
>> Although, looking at the driver's code and TRM docs, it seems that all
>> those properties are really irrelevant only to the older Terga20 SoC. So
>> the binding doc is a bit misleading.
>>
>> Nevertheless, the binding explicitly says that the properties are
>> optional, which is correct.
>
> Optional only means that drivers must not fail if these properties
> aren't found, it doesn't mean that the driver can't warn that they
> are missing.
>
> Quite possibly the only reason why they were made optional is because
> they weren't part of the bindings since the beginning. Anything added
> to a binding after the first public release has to be optional by
> definition, otherwise DT ABI wouldn't be stable.
>
> I think these warnings were added on purpose, though I also see that
> there are only values for these in device tree for Tegra186 and Tegra194
> but not Tegra210 where these should also be necessary.
>
> Adding Sowjanya who wrote this code. Perhaps she can clarify why the
> warnings were added. If these values /should/ be there on a subset of
> Tegra, then I think we should keep them, or add them again, but perhaps
> add a better way of identifying when they are necessary and when it is
> safe to work without them.
>
> That said, looking at those checks I wonder if they are perhaps just
> wrong. Or at the very least they seem redundant. It looks to me like
> they can just be:
>
> if (tegra_host->pinctrl_state_XYZ == NULL) {
> ...
> }
>
> That !IS_ERR(...) doesn't seem to do anything. But in that case, it's
> also obvious why we're warning about them on platforms where these
> properties don't exist in DT.
>
> So I think we either need to add those values where appropriate so that
> the warning doesn't show, or we need to narrow down where they are
> really needed and add a corresponding condition.
>
> But again, perhaps Sowjanya can help clarify if these really are only
> needed on Tegra210 and later or if these also apply to older chips.
Either way will be cleaner to convert the DT binding to YAML rather than
clutter the driver, IMO.
Re: [PATCH v1] sdhci: tegra: Remove warnings about missing device-tree properties
19.05.2020 23:44, Sowjanya Komatineni пишет: > > On 5/19/20 12:07 PM, Sowjanya Komatineni wrote: >> >> On 5/19/20 11:41 AM, Sowjanya Komatineni wrote: >>> >>> On 5/19/20 11:34 AM, Sowjanya Komatineni wrote: >>>> >>>> On 5/19/20 9:33 AM, Dmitry Osipenko wrote: >>>>> 19.05.2020 19:24, Thierry Reding пишет: >>>>>> On Tue, May 19, 2020 at 05:05:27PM +0300, Dmitry Osipenko wrote: >>>>>>> 19.05.2020 10:28, Ulf Hansson пишет: >>>>>>>> On Sat, 16 May 2020 at 17:44, Dmitry Osipenko >>>>>>>> wrote: >>>>>>>>> Several people asked me about the MMC warnings in the KMSG log and >>>>>>>>> I had to tell to ignore them because these warning are >>>>>>>>> irrelevant to >>>>>>>>> pre-Tegra210 SoCs. >>>>>>>> Why are the warnings irrelevant? >>>>>>> That's what the DT binding doc says [1]. >>>>>>> >>>>>>> [1] >>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/nvidia%2Ctegra20-sdhci.txt >>>>>>> >>>>>>> >>>>>>> Although, looking at the driver's code and TRM docs, it seems >>>>>>> that all >>>>>>> those properties are really irrelevant only to the older Terga20 >>>>>>> SoC. So >>>>>>> the binding doc is a bit misleading. >>>>>>> >>>>>>> Nevertheless, the binding explicitly says that the properties are >>>>>>> optional, which is correct. >>>>>> Optional only means that drivers must not fail if these properties >>>>>> aren't found, it doesn't mean that the driver can't warn that they >>>>>> are missing. >>>>>> >>>>>> Quite possibly the only reason why they were made optional is because >>>>>> they weren't part of the bindings since the beginning. Anything added >>>>>> to a binding after the first public release has to be optional by >>>>>> definition, otherwise DT ABI wouldn't be stable. >>>>>> >>>>>> I think these warnings were added on purpose, though I also see that >>>>>> there are only values for these in device tree for Tegra186 and >>>>>> Tegra194 >>>>>> but not Tegra210 where these should also be necessary. >>>> >>>> dt binding doc we have is common for MMC, SD and SDIO of all Tegras. >>>> Its not mandatory to have both 3v3 and 1v8 in device tree as based >>>> on signal mode. >>>> >>>> As these driver strengths are SoC specific, they are part of Tegra >>>> SoC specific device tree where same values will be applicable to all >>>> Tegra SoC specific platforms. >>>> >>>> Based on interface usage on platform, we use one or both of them >>>> like sdcard supports dual voltage and we use both 3V3 and 1V8 but if >>>> same interface is used for WIFI SD we use 1V8 only. >>>> >>>> So made these dt properties as optional. >>>> >>>> Other reason they are optional is, Tegra210 and prior has drive >>>> strength settings part of apb_misc and Tegra186 and later has driver >>>> strengths part of SDMMC controller. So, >>>> >>>> - Pinctrls "sdmmc-3v3-drv" and "sdmmc-1v8-drv" for driver strengths >>>> are applicable for Tegra210 and prior. >>>> - dt properties pad-autocal-pull-up/down-offset-1v8/3v3-timeout are >>>> for T186 onwards for driver strengths >>>> >>>> Looks like dt binding doc need fix to clearly document these based >>>> on SoC or agree with Yaml we can conditionally specify pinctrl or dt >>>> properties based on SoC dependent. >>>> >>>> >>>>>> Adding Sowjanya who wrote this code. Perhaps she can clarify why the >>>>>> warnings were added. If these values /should/ be there on a subset of >>>>>> Tegra, then I think we should keep them, or add them again, but >>>>>> perhaps >>>>>> add a better way of identifying when they are necessary and when >>>>>> it is >>>>>> safe to work without them. >>>>>> >>>>>> That said, looking at those c
Re: [RFC PATCH v5 12/14] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
28.07.2020 19:04, Sowjanya Komatineni пишет:
...
>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>> +{
>> Doesn't MIPI_CAL need to be reset here?
> No need to reset MIPI CAL
Could you please explain why. There is a calibration state-machine that
apparently needs to be returned into initial state, does it return by
itself?
TRM says that MIPI block needs to be reset before of starting
calibration process. The reset is completely missing in the driver, I
assume it needs to be corrected with another patch.
Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration
28.07.2020 18:59, Sowjanya Komatineni пишет: ... >>> + ret = tegra_mipi_finish_calibration(csi_chan->mipi); >>> + if (ret < 0) >>> + dev_err(csi_chan->csi->dev, >>> + "MIPI calibration failed: %d\n", ret); >> Doesn't v4l2_subdev_call(OFF) need to be invoked here on error? > > Not required as on error streaming fails and runtime PM will turn off > power anyway. I see that camera drivers bump theirs RPM on s_stream=1, and thus, s_stream=0 should be invoked in order to balance the RPM. What am I missing? https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/i2c/ov2740.c#L634 > Also we only did csi subdev s_stream on and during sensor subdev > s_stream on fail, actual stream dont happen and on tegra side frame > capture by HW happens only when kthreads run. Secondly, perhaps a failed calibration isn't a very critical error? Hence just printing a warning message should be enough. Could you please make a patch that factors all ON/OFF code paths into a separate functions? It's a bit difficult to follow the combined code, especially partial changes in the patches. Thanks in advance!
Re: [RFC PATCH v5 12/14] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
29.07.2020 20:55, Sowjanya Komatineni пишет:
>
> On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
>> 28.07.2020 19:04, Sowjanya Komatineni пишет:
>> ...
>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>> +{
>>>> Doesn't MIPI_CAL need to be reset here?
>>> No need to reset MIPI CAL
>> Could you please explain why. There is a calibration state-machine that
>> apparently needs to be returned into initial state, does it return by
>> itself?
>>
>> TRM says that MIPI block needs to be reset before of starting
>> calibration process. The reset is completely missing in the driver, I
>> assume it needs to be corrected with another patch.
>
> TRM documented incorrectly. There is no need to reset MIPI_CAL.
>
> MIPI CAL is FSM and it does not hang and done bit is to indicate if
> results are applied to pads or not.
>
> If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
> results are not applied to pads.
But how to stop calibration from triggering on LP-11 once it has been
enabled? The reset should be needed since there is no other way to reset
the calibration state.
> Also while multiple streams can happen in parallel and we can't reset
> MIPI CAL as other CSI channel streams (using other pads) may also be
> going thru calibration process in parallel depending and also DSI pads
> also are calibrated thru same MIPI CAL controller.
Perhaps this should be the case for a shared reset control API usage.
Re: [RFC PATCH v5 12/14] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
30.07.2020 02:54, Sowjanya Komatineni пишет:
>
> On 7/29/20 4:42 PM, Dmitry Osipenko wrote:
>> 29.07.2020 20:55, Sowjanya Komatineni пишет:
>>> On 7/29/20 10:08 AM, Dmitry Osipenko wrote:
>>>> 28.07.2020 19:04, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>> *device)
>>>>>>> +{
>>>>>> Doesn't MIPI_CAL need to be reset here?
>>>>> No need to reset MIPI CAL
>>>> Could you please explain why. There is a calibration state-machine that
>>>> apparently needs to be returned into initial state, does it return by
>>>> itself?
>>>>
>>>> TRM says that MIPI block needs to be reset before of starting
>>>> calibration process. The reset is completely missing in the driver, I
>>>> assume it needs to be corrected with another patch.
>>> TRM documented incorrectly. There is no need to reset MIPI_CAL.
>>>
>>> MIPI CAL is FSM and it does not hang and done bit is to indicate if
>>> results are applied to pads or not.
>>>
>>> If we don't see done bit set meaning, MIPI CAL did not see LP-11 and
>>> results are not applied to pads.
>> But how to stop calibration from triggering on LP-11 once it has been
>> enabled? The reset should be needed since there is no other way to reset
>> the calibration state.
>
> Its a finite state machine that goes thru fixed steps of sequence codes
> internally and holds results in registers.
>
> When it sees LP-11 results are applied to pads.
>
> If it does not see LP-11, next start will again trigger calibrating with
> finite sequence codes.
>
> As per HW designers, we don't have to do any reverts when done bit is
> not set.
Alright, then should be good if HW can't stuck.
Reviewed-by: Dmitry Osipenko
Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration
30.07.2020 03:27, Sowjanya Komatineni пишет:
...
>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>> Hence just printing a warning message should be enough.
>>
>> Using dev_err to report calibration failure. Are you suggesting to use
>> dev_warn instead of dev_err?
I meant that failing s_stream might be unnecessary.
The dev_warn should be more appropriate for a non-critical errors.
>>> Could you please make a patch that factors all ON/OFF code paths into a
>>> separate functions? It's a bit difficult to follow the combined code,
>>> especially partial changes in the patches. Thanks in advance!
>>
>> what do you mean by partial changes in patches?
>>
>> Can you please be more clear?
>
> Also please specify what ON/OFF code paths you are referring to when you
> say to move into separate functions?
I meant to change all the code like this:
set(on) {
if (on) {
...
return;
}
if (!on)
...
return;
}
to somewhat like this:
set(on) {
if (on)
ret = enable();
else
ret = disable();
return ret;
}
Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration
30.07.2020 03:55, Sowjanya Komatineni пишет:
>
> On 7/29/20 5:52 PM, Sowjanya Komatineni wrote:
>>
>> On 7/29/20 5:43 PM, Dmitry Osipenko wrote:
>>> 30.07.2020 03:27, Sowjanya Komatineni пишет:
>>> ...
>>>>>> Secondly, perhaps a failed calibration isn't a very critical error?
>>>>>> Hence just printing a warning message should be enough.
>>>>> Using dev_err to report calibration failure. Are you suggesting to use
>>>>> dev_warn instead of dev_err?
>>> I meant that failing s_stream might be unnecessary.
>>>
>>> The dev_warn should be more appropriate for a non-critical errors.
>>>
>>>>>> Could you please make a patch that factors all ON/OFF code paths
>>>>>> into a
>>>>>> separate functions? It's a bit difficult to follow the combined code,
>>>>>> especially partial changes in the patches. Thanks in advance!
>>>>> what do you mean by partial changes in patches?
>>>>>
>>>>> Can you please be more clear?
>>>> Also please specify what ON/OFF code paths you are referring to when
>>>> you
>>>> say to move into separate functions?
>>> I meant to change all the code like this:
>>>
>>> set(on) {
>>> if (on) {
>>> ...
>>> return;
>>> }
>>>
>>> if (!on)
>>> ...
>>>
>>> return;
>>> }
>>>
>>> to somewhat like this:
>>>
>>> set(on) {
>>> if (on)
>>> ret = enable();
>>> else
>>> ret = disable();
>>>
>>> return ret;
>>> }
>>
>> You mean to change tegra_channel_set_stream() ?
Yes, and tegra_csi_s_stream().
> changing tegra_channel_set_stream() to have like below will have
> redundant calls as most of the code b/w enable and disable is same
> except calling them in reverse order based on on/off and doing MIPI
> calibration only during ON
>
>
> if (on)
> ret = enable()
> else
> ret = disable()
> return ret;
Readability should be more important than number of lines.
Re: [RFC PATCH v5 13/14] media: tegra-video: Add CSI MIPI pads calibration
30.07.2020 04:06, Sowjanya Komatineni пишет: ... > Will have v6 and add additional patch at the end to do enable/disable > separately. > > Separating this out with additional patch before adding sensor support > patch requires all patches to be updated. > > So I am ok either ways. Please let me know if adding additional patch at > the end to split tegra_channel_set_stream() and tegra_csi_s_stream() > separately is ok? Should be okay, thanks!
Re: [PATCH] media: staging: tegra-vde: fix runtime pm imbalance on error
20.05.2020 12:51, Dinghao Liu пишет: > pm_runtime_get_sync() increments the runtime PM usage counter even > it returns an error code. Thus a pairing decrement is needed on > the error handling path to keep the counter balanced. > > Signed-off-by: Dinghao Liu > --- > drivers/staging/media/tegra-vde/vde.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/tegra-vde/vde.c > b/drivers/staging/media/tegra-vde/vde.c > index d3e63512a765..dd134a3a15c7 100644 > --- a/drivers/staging/media/tegra-vde/vde.c > +++ b/drivers/staging/media/tegra-vde/vde.c > @@ -777,7 +777,7 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde > *vde, > > ret = pm_runtime_get_sync(dev); > if (ret < 0) > - goto unlock; > + goto put_runtime_pm; > > /* >* We rely on the VDE registers reset value, otherwise VDE > Hello Dinghao, Thank you for the patch. I sent out a similar patch a week ago [1]. [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20200514210847.9269-2-dig...@gmail.com/ The pm_runtime_put_noidle() should have the same effect as yours variant, although my variant won't change the last_busy RPM time, which I think is a bit more appropriate behavior.
Re: [PATCH] sdhci: tegra: Avoid reading autocal timeout values when not applicable
20.05.2020 23:08, Sowjanya Komatineni пишет: > When auto calibration timeouts, calibration is disabled and fail-safe > drive strength values are programmed based on the signal voltage. > > Different fail-safe drive strength values based on voltage are > applicable only for SoCs supporting 3V3 and 1V8 pad controls. > > So, this patch avoids reading these properties from the device tree > for SoCs not using pad controls and the warning of missing properties > will not show up on these SoC platforms. > > Signed-off-by: Sowjanya Komatineni > --- > drivers/mmc/host/sdhci-tegra.c | 57 > -- > 1 file changed, 33 insertions(+), 24 deletions(-) Hello Sowjanya, Thank you for the patch. Tested-by: Dmitry Osipenko
Re: regulator: deadlock vs memory reclaim
10.08.2020 23:09, Michał Mirosław пишет: > At first I also thought so, but there's more. Below is a lockdep > complaint with your patch applied. I did a similar patch and then two more > (following) and that is still not enough (sysfs/debugfs do allocations, > too). Then it should be good to move the locking for init_coupling() like I suggested and use GFP_NOWAIT for the two other cases. It all could be a single small patch. Could you please check whether GFP_NOWAIT helps?
Re: regulator: deadlock vs memory reclaim
10.08.2020 23:18, Michał Mirosław пишет: > On Mon, Aug 10, 2020 at 11:15:28PM +0300, Dmitry Osipenko wrote: >> 10.08.2020 23:09, Michał Mirosław пишет: >>> At first I also thought so, but there's more. Below is a lockdep >>> complaint with your patch applied. I did a similar patch and then two more >>> (following) and that is still not enough (sysfs/debugfs do allocations, >>> too). >> Then it should be good to move the locking for init_coupling() like I >> suggested and use GFP_NOWAIT for the two other cases. It all could be a >> single small patch. Could you please check whether GFP_NOWAIT helps? > > This would be equivalent to my patches. Problem with sysfs and debugfs > remains as they don't have the option of GFP_NOWAIT. This needs to be > moved outside of the locks. Ah okay, you meant the debugfs core. I see now, thanks.
Re: regulator: deadlock vs memory reclaim
10.08.2020 23:21, Dmitry Osipenko пишет: > 10.08.2020 23:18, Michał Mirosław пишет: >> On Mon, Aug 10, 2020 at 11:15:28PM +0300, Dmitry Osipenko wrote: >>> 10.08.2020 23:09, Michał Mirosław пишет: >>>> At first I also thought so, but there's more. Below is a lockdep >>>> complaint with your patch applied. I did a similar patch and then two more >>>> (following) and that is still not enough (sysfs/debugfs do allocations, >>>> too). >>> Then it should be good to move the locking for init_coupling() like I >>> suggested and use GFP_NOWAIT for the two other cases. It all could be a >>> single small patch. Could you please check whether GFP_NOWAIT helps? >> >> This would be equivalent to my patches. Problem with sysfs and debugfs >> remains as they don't have the option of GFP_NOWAIT. This needs to be >> moved outside of the locks. > > Ah okay, you meant the debugfs core. I see now, thanks. > This indeed needs a capital solution. It's not obvious how to fix it.. we can probably remove taking the list_mutex from lock_dependent(), but this still won't help the case of memory reclaiming because reclaim may cause touching the already locked regulator. IIUC, the case of memory reclaiming under regulator lock was always dangerous and happened to work by chance before, correct?
Re: regulator: deadlock vs memory reclaim
10.08.2020 23:56, Dmitry Osipenko пишет: > 10.08.2020 23:21, Dmitry Osipenko пишет: >> 10.08.2020 23:18, Michał Mirosław пишет: >>> On Mon, Aug 10, 2020 at 11:15:28PM +0300, Dmitry Osipenko wrote: >>>> 10.08.2020 23:09, Michał Mirosław пишет: >>>>> At first I also thought so, but there's more. Below is a lockdep >>>>> complaint with your patch applied. I did a similar patch and then two more >>>>> (following) and that is still not enough (sysfs/debugfs do allocations, >>>>> too). >>>> Then it should be good to move the locking for init_coupling() like I >>>> suggested and use GFP_NOWAIT for the two other cases. It all could be a >>>> single small patch. Could you please check whether GFP_NOWAIT helps? >>> >>> This would be equivalent to my patches. Problem with sysfs and debugfs >>> remains as they don't have the option of GFP_NOWAIT. This needs to be >>> moved outside of the locks. >> >> Ah okay, you meant the debugfs core. I see now, thanks. >> > > This indeed needs a capital solution. > > It's not obvious how to fix it.. we can probably remove taking the > list_mutex from lock_dependent(), but this still won't help the case of > memory reclaiming because reclaim may cause touching the already locked > regulator. IIUC, the case of memory reclaiming under regulator lock was > always dangerous and happened to work by chance before, correct? > And like Mark mentioned before, this situation also potentially may happen from other paths.
Re: regulator: deadlock vs memory reclaim
11.08.2020 03:07, Michał Mirosław пишет: ... > I just noticed that locking in regulator_resolve_coupling() is bogus. > This all holds up because regulator_list_mutex is held during the call. > Feel free to test a patch below. > > I'm working my way to push allocations outside of the locks, but the > coupling-related locking will need to be fixed regardless. > > Best Regards, > Michał Mirosław > > >8< > > [PATCH] regulator: remove superfluous lock in regulator_resolve_coupling() > > The code modifies rdev, but locks c_rdev instead. The bug remains: > stored c_rdev could be freed just after unlock anyway. This doesn't blow > up because regulator_list_mutex taken outside holds it together. > > Signed-off-by: Michał Mirosław > --- > drivers/regulator/core.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 94f9225869da..e519bc9a860d 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -4859,13 +4859,9 @@ static void regulator_resolve_coupling(struct > regulator_dev *rdev) > return; > } > > - regulator_lock(c_rdev); > - > c_desc->coupled_rdevs[i] = c_rdev; > c_desc->n_resolved++; > > - regulator_unlock(c_rdev); > - > regulator_resolve_coupling(c_rdev); > } > } > The change looks like a good cleanup to me, thanks. I think that c_rdev locking was accidentally left from some older version of the patch that introduced the coupling support. There shouldn't be any real bug in this code. IIRC, at some point I changed the code to disallow consumers to get a partially coupled regulator and then protected the resolve_coupling() with list_mutex, but seems missed to remove that c_rdev locking. Hence there shouldn't be a need to lock regulators individually during the resolve because nothing should touch the coupled regulators until all the coupling has been resolved.
Re: [PATCH 7/7] regulator: remove superfluous lock in regulator_resolve_coupling()
11.08.2020 04:07, Michał Mirosław пишет: > The code modifies rdev, but locks c_rdev instead. The bug remains: > stored c_rdev could be freed just after unlock anyway. This doesn't blow > up because regulator_list_mutex taken outside holds it together. > > Signed-off-by: Michał Mirosław > --- > drivers/regulator/core.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index b85ec974944e..f8834559a2fb 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -4942,13 +4942,9 @@ static void regulator_resolve_coupling(struct > regulator_dev *rdev) > return; > } > > - regulator_lock(c_rdev); > - > c_desc->coupled_rdevs[i] = c_rdev; > c_desc->n_resolved++; > > - regulator_unlock(c_rdev); > - > regulator_resolve_coupling(c_rdev); > } > } > As I replied to the other email, there is no real bug here. The regulators are uncoupled before regulator is freed and the uncoupling is also protected by the list_mutex. Hence the resolve_coupling() doesn't need to lock regulators and this change looks like a good cleanup. Perhaps the commit message could be improved a tad, either way: Reviewed-by: Dmitry Osipenko
Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock
11.08.2020 04:07, Michał Mirosław пишет: > Allocating memory with regulator_list_mutex held makes lockdep unhappy > when memory pressure makes the system do fs_reclaim on eg. eMMC using > a regulator. Push the lock inside regulator_init_coupling() after the > allocation. ... Reviewed-by: Dmitry Osipenko
Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock
11.08.2020 18:59, Dmitry Osipenko пишет: > 11.08.2020 04:07, Michał Mirosław пишет: >> Allocating memory with regulator_list_mutex held makes lockdep unhappy >> when memory pressure makes the system do fs_reclaim on eg. eMMC using >> a regulator. Push the lock inside regulator_init_coupling() after the >> allocation. > ... > > Reviewed-by: Dmitry Osipenko > On the other hand, couldn't it be better to just remove taking the list_mutex from the regulator_lock_dependent()? I think the list_mutex is only needed to protect from supply/couple regulator being removed during of the locking process, but maybe this is not something we should worry about?
Re: [PATCH v9 09/10] media: tegra-video: Add CSI MIPI pads calibration
06.08.2020 22:01, Sowjanya Komatineni пишет: ... > + err = tegra_mipi_finish_calibration(csi_chan->mipi); > + > + if (ret < 0 && ret != -ENOIOCTLCMD) > + goto err_disable_csi_stream; > + > + if (err < 0) > + dev_warn(csi_chan->csi->dev, > + "MIPI calibration failed: %d\n", ret); err
Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock
11.08.2020 20:20, Michał Mirosław пишет: > On Tue, Aug 11, 2020 at 07:27:43PM +0300, Dmitry Osipenko wrote: >> 11.08.2020 18:59, Dmitry Osipenko пишет: >>> 11.08.2020 04:07, Michał Mirosław пишет: >>>> Allocating memory with regulator_list_mutex held makes lockdep unhappy >>>> when memory pressure makes the system do fs_reclaim on eg. eMMC using >>>> a regulator. Push the lock inside regulator_init_coupling() after the >>>> allocation. >>> ... >>> >>> Reviewed-by: Dmitry Osipenko >> On the other hand, couldn't it be better to just remove taking the >> list_mutex from the regulator_lock_dependent()? >> >> I think the list_mutex is only needed to protect from supply/couple >> regulator being removed during of the locking process, but maybe this is >> not something we should worry about? > > This is what I would like to see in the end, but it requires more > thought, at least around interaction with regulator_resolve_coupling() > and the regulator removal. I meant that it's very unlikely to have regulator gone while it's in-use. Hence it could be okay to ignore this rare case, and thus, simplify the fix significantly by removing the offending lock. Still this won't solve the root of the problem because potentially reclaim could happen while storage regulator (or its supply) is locked, although it should be a very unlikely condition in practice.
Re: [RFC PATCH v6 09/10] media: tegra-video: Add CSI MIPI pads calibration
31.07.2020 12:02, Sowjanya Komatineni пишет:
...
> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct v4l2_subdev
> *subdev)
> return ret;
> }
>
> + if (csi_chan->mipi) {
> + ret = tegra_mipi_enable(csi_chan->mipi);
> + if (ret < 0) {
> + dev_err(csi->dev,
> + "failed to enable MIPI pads: %d\n", ret);
> + goto rpm_put;
> + }
> +
> + /*
> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to
> + * be calibrated after power on.
> + * So, trigger the calibration start here and results will
> + * be latched and applied to the pads when link is in LP11
> + * state during start of sensor streaming.
> + */
> + ret = tegra_mipi_start_calibration(csi_chan->mipi);
> + if (ret < 0) {
> + dev_err(csi->dev,
> + "failed to start MIPI calibration: %d\n", ret);
> + goto disable_mipi;
> + }
What would happen if CSI stream is enabled and then immediately disabled
without enabling camera sensor?
> + }
> +
...
> static int tegra_channel_enable_stream(struct tegra_vi_channel *chan)
> {
> struct v4l2_subdev *csi_subdev, *src_subdev;
> + struct tegra_csi_channel *csi_chan;
> int ret;
>
> /*
> @@ -206,13 +207,30 @@ static int tegra_channel_enable_stream(struct
> tegra_vi_channel *chan)
> if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
> return 0;
>
> + csi_chan = v4l2_get_subdevdata(csi_subdev);
> + /*
> + * TRM has incorrectly documented to wait for done status from
> + * calibration logic after CSI interface power on.
> + * As per the design, calibration results are latched and applied
> + * to the pads only when the link is in LP11 state which will happen
> + * during the sensor stream-on.
> + * CSI subdev stream-on triggers start of MIPI pads calibration.
> + * Wait for calibration to finish here after sensor subdev stream-on
> + * and in case of sensor stream-on failure, cancel the calibration.
> + */
> src_subdev = tegra_channel_get_remote_source_subdev(chan);
Is it possible to move the start_calibration() here?
> ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> if (ret < 0 && ret != -ENOIOCTLCMD) {
> + tegra_mipi_cancel_calibration(csi_chan->mipi);
> v4l2_subdev_call(csi_subdev, video, s_stream, false);
> return ret;
> }
>
> + ret = tegra_mipi_finish_calibration(csi_chan->mipi);
> + if (ret < 0)
> + dev_warn(csi_chan->csi->dev,
> + "MIPI calibration failed: %d\n", ret);
> +
> return 0;
> }
>
>
Re: [RFC PATCH v6 05/10] media: tegra-video: Separate CSI stream enable and disable implementations
31.07.2020 12:02, Sowjanya Komatineni пишет:
> This patch separates implementation of CSI stream enable and disable
> into separate functions for readability.
>
> Signed-off-by: Sowjanya Komatineni
> ---
> drivers/staging/media/tegra-video/csi.c | 51
> ++---
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/media/tegra-video/csi.c
> b/drivers/staging/media/tegra-video/csi.c
> index fb667df..cfe6187 100644
> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -232,34 +232,53 @@ static int tegra_csi_g_frame_interval(struct
> v4l2_subdev *subdev,
> return 0;
> }
>
> -static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
> +static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
> {
> struct tegra_vi_channel *chan = v4l2_get_subdev_hostdata(subdev);
> struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
> struct tegra_csi *csi = csi_chan->csi;
> - int ret = 0;
> + int ret;
> +
> + ret = pm_runtime_get_sync(csi->dev);
> + if (ret < 0) {
> + dev_err(csi->dev, "failed to get runtime PM: %d\n", ret);
> + pm_runtime_put_noidle(csi->dev);
> + return ret;
> + }
>
> csi_chan->pg_mode = chan->pg_mode;
> - if (enable) {
> - ret = pm_runtime_get_sync(csi->dev);
> - if (ret < 0) {
> - dev_err(csi->dev,
> - "failed to get runtime PM: %d\n", ret);
> - pm_runtime_put_noidle(csi->dev);
> - return ret;
> - }
> + ret = csi->ops->csi_start_streaming(csi_chan);
> + if (ret < 0)
> + goto rpm_put;
>
> - ret = csi->ops->csi_start_streaming(csi_chan);
> - if (ret < 0)
> - goto rpm_put;
> + return 0;
>
> - return 0;
> - }
> +rpm_put:
> + pm_runtime_put(csi->dev);
> + return ret;
> +}
> +
> +static int tegra_csi_disable_stream(struct v4l2_subdev *subdev)
> +{
> + struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
> + struct tegra_csi *csi = csi_chan->csi;
>
> csi->ops->csi_stop_streaming(csi_chan);
>
> -rpm_put:
> pm_runtime_put(csi->dev);
> +
> + return 0;
> +}
> +
> +static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> + int ret;
> +
> + if (enable)
> + ret = tegra_csi_enable_stream(subdev);
> + else
> + ret = tegra_csi_disable_stream(subdev);
> +
> return ret;
> }
>
>
Thanks!
Reviewed-by: Dmitry Osipenko
Re: [RFC PATCH v6 09/10] media: tegra-video: Add CSI MIPI pads calibration
31.07.2020 18:46, Sowjanya Komatineni пишет:
>
> On 7/31/20 4:39 AM, Dmitry Osipenko wrote:
>> 31.07.2020 12:02, Sowjanya Komatineni пишет:
>> ...
>>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct
>>> v4l2_subdev *subdev)
>>> return ret;
>>> }
>>> + if (csi_chan->mipi) {
>>> + ret = tegra_mipi_enable(csi_chan->mipi);
>>> + if (ret < 0) {
>>> + dev_err(csi->dev,
>>> + "failed to enable MIPI pads: %d\n", ret);
>>> + goto rpm_put;
>>> + }
>>> +
>>> + /*
>>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to
>>> + * be calibrated after power on.
>>> + * So, trigger the calibration start here and results will
>>> + * be latched and applied to the pads when link is in LP11
>>> + * state during start of sensor streaming.
>>> + */
>>> + ret = tegra_mipi_start_calibration(csi_chan->mipi);
>>> + if (ret < 0) {
>>> + dev_err(csi->dev,
>>> + "failed to start MIPI calibration: %d\n", ret);
>>> + goto disable_mipi;
>>> + }
>> What would happen if CSI stream is enabled and then immediately disabled
>> without enabling camera sensor?
>
> Nothing will happen as during stream enable csi receiver is kept ready.
>
> But actual capture will not happen during that point.
Could you please show how the full call chain looks like? It's not clear
to me what keeps CSI stream "ready".
Re: [RFC PATCH v6 09/10] media: tegra-video: Add CSI MIPI pads calibration
31.07.2020 19:29, Sowjanya Komatineni пишет:
>
> On 7/31/20 9:14 AM, Dmitry Osipenko wrote:
>> 31.07.2020 18:46, Sowjanya Komatineni пишет:
>>> On 7/31/20 4:39 AM, Dmitry Osipenko wrote:
>>>> 31.07.2020 12:02, Sowjanya Komatineni пишет:
>>>> ...
>>>>> @@ -249,13 +249,47 @@ static int tegra_csi_enable_stream(struct
>>>>> v4l2_subdev *subdev)
>>>>> return ret;
>>>>> }
>>>>> + if (csi_chan->mipi) {
>>>>> + ret = tegra_mipi_enable(csi_chan->mipi);
>>>>> + if (ret < 0) {
>>>>> + dev_err(csi->dev,
>>>>> + "failed to enable MIPI pads: %d\n", ret);
>>>>> + goto rpm_put;
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * CSI MIPI pads PULLUP, PULLDN and TERM impedances need to
>>>>> + * be calibrated after power on.
>>>>> + * So, trigger the calibration start here and results will
>>>>> + * be latched and applied to the pads when link is in LP11
>>>>> + * state during start of sensor streaming.
>>>>> + */
>>>>> + ret = tegra_mipi_start_calibration(csi_chan->mipi);
>>>>> + if (ret < 0) {
>>>>> + dev_err(csi->dev,
>>>>> + "failed to start MIPI calibration: %d\n", ret);
>>>>> + goto disable_mipi;
>>>>> + }
>>>> What would happen if CSI stream is enabled and then immediately
>>>> disabled
>>>> without enabling camera sensor?
>>> Nothing will happen as during stream enable csi receiver is kept ready.
>>>
>>> But actual capture will not happen during that point.
>> Could you please show how the full call chain looks like? It's not clear
>> to me what keeps CSI stream "ready".
>
> VI is the main video input (video device) and on streaming it starts
> stream of CSI subdev prior to stream of Sensor.
>
> HW path, sensor stream (CSI TX) -> CSI stream (RX)
>
> During CSI stream on, CSI PHY receiver is enabled to start receiving the
> data but internally capture assembled to active state will happen only
> when Tegra VI single shot is issues where VI thru pixel parser gets
> captures data into the memory
Alright, I see now.
Will be great if you could change this hunk:
{
ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
if (ret < 0 && ret != -ENOIOCTLCMD) {
tegra_mipi_cancel_calibration(csi_chan->mipi);
v4l2_subdev_call(csi_subdev, video, s_stream, false);
return ret;
}
}
to look like this:
{
err = v4l2_subdev_call(src_subdev, video, s_stream, true);
if (err < 0 && err != -ENOIOCTLCMD)
goto err_disable_csi_stream;
...
return 0;
err_disable_csi_stream:
tegra_mipi_cancel_calibration(csi_chan->mipi);
v4l2_subdev_call(csi_subdev, video, s_stream, false);
return err;
}
It should make code a bit easier to read and follow.
Otherwise this patch looks good to me, thanks.
Re: [PATCH v7 06/10] media: tegra-video: Add support for external sensor capture
01.08.2020 00:32, Sowjanya Komatineni пишет:
...
> +static int tegra_csi_channels_alloc(struct tegra_csi *csi)
> +{
> + struct device_node *node = csi->dev->of_node;
> + struct v4l2_fwnode_endpoint v4l2_ep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + struct fwnode_handle *fwh;
> + struct device_node *channel;
> + struct device_node *ep;
> + unsigned int lanes, portno, num_pads;
> + int ret;
> +
> + for_each_child_of_node(node, channel) {
> + if (!of_node_name_eq(channel, "channel"))
> + continue;
> +
> + ret = of_property_read_u32(channel, "reg", &portno);
> + if (ret < 0)
> + continue;
> +
> + if (portno >= csi->soc->csi_max_channels) {
> + dev_err(csi->dev, "invalid port num %d\n", portno);
The "channel" node should be put on error.
> + return -EINVAL;
> + }
> +
> + ep = of_graph_get_endpoint_by_regs(channel, 0, 0);
> + if (!ep)
> + continue;
> +
> + fwh = of_fwnode_handle(ep);
> + ret = v4l2_fwnode_endpoint_parse(fwh, &v4l2_ep);
> + of_node_put(ep);
> + if (ret) {
> + dev_err(csi->dev,
> + "failed to parse v4l2 endpoint: %d\n", ret);
> + return ret;
> + }
> +
> + lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> + if (!lanes || ((lanes & (lanes - 1)) != 0)) {
> + dev_err(csi->dev, "invalid data-lanes %d\n", lanes);
> + return -EINVAL;
> + }
> +
> + num_pads = of_graph_get_endpoint_count(channel);
> + if (num_pads == TEGRA_CSI_PADS_NUM) {
> + ret = tegra_csi_channel_alloc(csi, channel, portno,
> + lanes, num_pads);
> + if (ret < 0)
> + return ret;
> + }
> }
...
> +static int tegra_vi_channels_alloc(struct tegra_vi *vi)
> +{
> + struct device_node *node = vi->dev->of_node;
> + struct device_node *ep = NULL;
> + struct device_node *ports;
> + struct device_node *port;
> + unsigned int port_num;
> + int ret;
> +
> + ports = of_get_child_by_name(node, "ports");
> + if (!ports)
> + return -ENODEV;
> +
> + for_each_child_of_node(ports, port) {
> + if (!of_node_name_eq(port, "port"))
> + continue;
> +
> + ret = of_property_read_u32(port, "reg", &port_num);
> + if (ret < 0)
> + continue;
> +
> + if (port_num > vi->soc->vi_max_channels) {
> + of_node_put(ports);
s/ports/port/
> + dev_err(vi->dev, "invalid port num %d\n", port_num);
> + return -EINVAL;
> + }
> +
> + ep = of_get_child_by_name(port, "endpoint");
> + if (!ep)
> + continue;
> +
> + of_node_put(ep);
> + ret = tegra_vi_channel_alloc(vi, port_num, port);
> + if (ret < 0) {
> + of_node_put(ports);
s/ports/port/
> + return ret;
> + }
> }
Re: [PATCH v7 09/10] media: tegra-video: Add CSI MIPI pads calibration
01.08.2020 00:32, Sowjanya Komatineni пишет: > CSI MIPI pads need to be enabled and calibrated for capturing from > the external sensor or transmitter. > > MIPI CAL unit calibrates MIPI pads pull-up, pull-down and termination > impedances. Calibration is done by co-work of MIPI BIAS pad and MIPI > CAL control unit. > > Triggering calibration start can happen any time after MIPI pads are > enabled but calibration results will be latched and applied to MIPI > pads by MIPI CAL unit only when the link is in LP11 state and then > calibration status register gets updated. > > This patch enables CSI MIPI pads and calibrates them during streaming. > > Tegra CSI receiver is able to catch the very first clock transition. > So, CSI receiver is always enabled prior to sensor streaming and > trigger of calibration start is done during CSI subdev streaming and > status of calibration is verified after sensor stream on. > > Signed-off-by: Sowjanya Komatineni > --- > drivers/staging/media/tegra-video/TODO | 1 - > drivers/staging/media/tegra-video/csi.c | 61 > +++-- > drivers/staging/media/tegra-video/csi.h | 2 ++ > drivers/staging/media/tegra-video/vi.c | 28 ++++--- > 4 files changed, 84 insertions(+), 8 deletions(-) Reviewed-by: Dmitry Osipenko
Re: [PATCH -next] media: staging: tegra-vde: Mark tegra_vde_runtime_suspend as __maybe_unused
03.08.2020 14:59, YueHaibing пишет:
> If CONFIG_PM is not set, gcc warns:
>
> drivers/staging/media/tegra-vde/vde.c:916:12:
> warning: 'tegra_vde_runtime_suspend' defined but not used [-Wunused-function]
>
> Make it __maybe_unused to fix this.
>
> Signed-off-by: YueHaibing
> ---
> drivers/staging/media/tegra-vde/vde.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/tegra-vde/vde.c
> b/drivers/staging/media/tegra-vde/vde.c
> index a3c24d96d5b9..2d043d518eef 100644
> --- a/drivers/staging/media/tegra-vde/vde.c
> +++ b/drivers/staging/media/tegra-vde/vde.c
> @@ -913,7 +913,7 @@ static irqreturn_t tegra_vde_isr(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static int tegra_vde_runtime_suspend(struct device *dev)
> +static __maybe_unused int tegra_vde_runtime_suspend(struct device *dev)
> {
> struct tegra_vde *vde = dev_get_drvdata(dev);
> int err;
>
Hello Yue,
Shouldn't the tegra_vde_runtime_resume() be marked as well?
Re: [PATCH -next] media: staging: tegra-vde: Mark tegra_vde_runtime_suspend as __maybe_unused
03.08.2020 16:00, Yuehaibing пишет:
> On 2020/8/3 20:51, Dmitry Osipenko wrote:
>> 03.08.2020 14:59, YueHaibing пишет:
>>> If CONFIG_PM is not set, gcc warns:
>>>
>>> drivers/staging/media/tegra-vde/vde.c:916:12:
>>> warning: 'tegra_vde_runtime_suspend' defined but not used
>>> [-Wunused-function]
>>>
>>> Make it __maybe_unused to fix this.
>>>
>>> Signed-off-by: YueHaibing
>>> ---
>>> drivers/staging/media/tegra-vde/vde.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/tegra-vde/vde.c
>>> b/drivers/staging/media/tegra-vde/vde.c
>>> index a3c24d96d5b9..2d043d518eef 100644
>>> --- a/drivers/staging/media/tegra-vde/vde.c
>>> +++ b/drivers/staging/media/tegra-vde/vde.c
>>> @@ -913,7 +913,7 @@ static irqreturn_t tegra_vde_isr(int irq, void *data)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> -static int tegra_vde_runtime_suspend(struct device *dev)
>>> +static __maybe_unused int tegra_vde_runtime_suspend(struct device *dev)
>>> {
>>> struct tegra_vde *vde = dev_get_drvdata(dev);
>>> int err;
>>>
>>
>> Hello Yue,
>>
>> Shouldn't the tegra_vde_runtime_resume() be marked as well?
>
> No, tegra_vde_runtime_resume() always be called by tegra_vde_shutdown().
Well.. it's unused, but compiler doesn't complain about runtime_resume()
because it sees the potential reference to that function in the code
(even that it's a dead code), while runtime_suspend() reference is
completely removed by preprocessor before compiler sees the code.
Should be nicer to have both suspend and resume functions marked, for
consistency, IMO.
Re: [PATCH 16/18] staging/media/tegra-vde: Clean up IOMMU workaround
20.08.2020 18:08, Robin Murphy пишет:
> Now that arch/arm is wired up for default domains and iommu-dma, we no
> longer need to work around the arch-private mapping.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/staging/media/tegra-vde/iommu.c | 12
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/staging/media/tegra-vde/iommu.c
> b/drivers/staging/media/tegra-vde/iommu.c
> index 6af863d92123..4f770189ed34 100644
> --- a/drivers/staging/media/tegra-vde/iommu.c
> +++ b/drivers/staging/media/tegra-vde/iommu.c
> @@ -10,10 +10,6 @@
> #include
> #include
>
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> -#include
> -#endif
> -
> #include "vde.h"
>
> int tegra_vde_iommu_map(struct tegra_vde *vde,
> @@ -70,14 +66,6 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
> if (!vde->group)
> return 0;
>
> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
> - if (dev->archdata.mapping) {
> - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> -
> - arm_iommu_detach_device(dev);
> - arm_iommu_release_mapping(mapping);
> - }
> -#endif
> vde->domain = iommu_domain_alloc(&platform_bus_type);
> if (!vde->domain) {
> err = -ENOMEM;
>
Hello, Robin! Thank you for yours work!
Some drivers, like this Tegra VDE (Video Decoder Engine) driver for
example, do not want to use implicit IOMMU domain. Tegra VDE driver
relies on explicit IOMMU domain in a case of Tegra SMMU because VDE
hardware can't access last page of the AS and because driver wants to
reserve some fixed addresses [1].
[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/iommu.c#L100
Tegra30 SoC supports up to 4 domains, hence it's not possible to afford
wasting unused implicit domains. I think this needs to be addressed
before this patch could be applied.
Would it be possible for IOMMU drivers to gain support for filtering out
devices in iommu_domain_alloc(dev, type)? Then perhaps Tegra SMMU driver
could simply return NULL in a case of type=IOMMU_DOMAIN_DMA and
dev=tegra-vde.
Alternatively, the Tegra SMMU could be changed such that the devices
will be attached to a domain at the time of a first IOMMU mapping
invocation instead of attaching at the time of attach_dev() callback
invocation.
Or maybe even IOMMU core could be changed to attach devices at the time
of the first IOMMU mapping invocation? This could be a universal
solution for all drivers.
Re: [PATCH 16/18] staging/media/tegra-vde: Clean up IOMMU workaround
20.08.2020 22:51, Dmitry Osipenko пишет: > Alternatively, the Tegra SMMU could be changed such that the devices > will be attached to a domain at the time of a first IOMMU mapping > invocation instead of attaching at the time of attach_dev() callback > invocation. > > Or maybe even IOMMU core could be changed to attach devices at the time > of the first IOMMU mapping invocation? This could be a universal > solution for all drivers. Although, please scratch this :) I'll need to revisit how DMA mapping API works.
Re: [PATCH 12/18] iommu/tegra-gart: Add IOMMU_DOMAIN_DMA support
20.08.2020 18:08, Robin Murphy пишет:
> Now that arch/arm is wired up for default domains and iommu-dma,
> implement the corresponding driver-side support for DMA domains.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/tegra-gart.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index fac720273889..e081387080f6 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -9,6 +9,7 @@
>
> #define dev_fmt(fmt) "gart: " fmt
>
> +#include
> #include
> #include
> #include
> @@ -145,16 +146,22 @@ static struct iommu_domain
> *gart_iommu_domain_alloc(unsigned type)
> {
> struct iommu_domain *domain;
Hello, Robin!
Tegra20 GART isn't a real IOMMU, but a small relocation aperture. We
would only want to use it for a temporal mappings (managed by GPU
driver) for the time while GPU hardware is busy and working with a
sparse DMA buffers, the driver will take care of unmapping the sparse
buffers once GPU work is finished [1]. In a case of contiguous DMA
buffers, we want to bypass the IOMMU and use buffer's phys address
because GART aperture is small and all buffers simply can't fit into
GART for a complex GPU operations that involve multiple buffers [2][3].
The upstream GPU driver still doesn't support GART, but eventually it
needs to be changed.
[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L489
[2]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L542
[3]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/uapi/patching.c#L90
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> return NULL;
Will a returned NULL tell to IOMMU core that implicit domain shouldn't
be used? Is it possible to leave this driver as-is?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 17:05, Dmitry Osipenko пишет:
> 05.08.2020 16:46, Thierry Reding пишет:
>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>> is done.
>>>
>>> So, this patch skips disabling MIPI clock after triggering start of
>>> calibration and disables it only after waiting for done status from
>>> the calibration logic.
>>>
>>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>> with their usage.
>>>
>>> As MIPI clock is left enabled and in case of any failures with CSI input
>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>>> and consumer drivers can call this in such cases.
>>>
>>> Reviewed-by: Dmitry Osipenko
>>> Signed-off-by: Sowjanya Komatineni
>>> ---
>>> drivers/gpu/drm/tegra/dsi.c | 4 ++--
>>> drivers/gpu/host1x/mipi.c | 19 ++-
>>> include/linux/host1x.h | 5 +++--
>>> 3 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>> index 3820e8d..a7864e9 100644
>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi
>>> *dsi)
>>> DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>> tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>
>>> - err = tegra_mipi_calibrate(dsi->mipi);
>>> + err = tegra_mipi_start_calibration(dsi->mipi);
>>> if (err < 0)
>>> return err;
>>>
>>> - return tegra_mipi_wait(dsi->mipi);
>>> + return tegra_mipi_finish_calibration(dsi->mipi);
>>> }
>>>
>>> static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long
>>> bclk,
>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>> index e606464..b15ab6e 100644
>>> --- a/drivers/gpu/host1x/mipi.c
>>> +++ b/drivers/gpu/host1x/mipi.c
>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>>> }
>>> EXPORT_SYMBOL(tegra_mipi_disable);
>>>
>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>> +{
>>> + clk_disable(device->mipi->clk);
>>
>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>> registers here? We don't clear the START bit in the former when the
>> calibration has successfully finished, but I suspect that's because
>> the bit is self-clearing. But I wonder if we still need to clear it
>> upon cancellation to make sure the calibration does indeed stop.
>
> Apparently there is no way to explicitly stop calibration other than to
> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>
> Perhaps having a fixed delay before disabling clock could be enough to
> ensure that calibration is stopped before the clock is disabled?
>
Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
it needs to be polled until it's unset?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 19:50, Sowjanya Komatineni пишет:
>
> On 8/5/20 9:47 AM, Dmitry Osipenko wrote:
>> 05.08.2020 19:33, Sowjanya Komatineni пишет:
>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>>>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the
>>>>>>> calibration
>>>>>>> is done.
>>>>>>>
>>>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>>>> calibration and disables it only after waiting for done status from
>>>>>>> the calibration logic.
>>>>>>>
>>>>>>> This patch renames tegra_mipi_calibrate() as
>>>>>>> tegra_mipi_start_calibration()
>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be
>>>>>>> inline
>>>>>>> with their usage.
>>>>>>>
>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>>>> input
>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>>>> MIPI clock
>>>>>>> and consumer drivers can call this in such cases.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Osipenko
>>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>>> ---
>>>>>>> drivers/gpu/drm/tegra/dsi.c | 4 ++--
>>>>>>> drivers/gpu/host1x/mipi.c | 19 ++-
>>>>>>> include/linux/host1x.h | 5 +++--
>>>>>>> 3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> index 3820e8d..a7864e9 100644
>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>>>> tegra_dsi *dsi)
>>>>>>> DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>>>> tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>>>> - err = tegra_mipi_calibrate(dsi->mipi);
>>>>>>> + err = tegra_mipi_start_calibration(dsi->mipi);
>>>>>>> if (err < 0)
>>>>>>> return err;
>>>>>>> - return tegra_mipi_wait(dsi->mipi);
>>>>>>> + return tegra_mipi_finish_calibration(dsi->mipi);
>>>>>>> }
>>>>>>> static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>>>> unsigned long bclk,
>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>>>> index e606464..b15ab6e 100644
>>>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>>>> tegra_mipi_device *dev)
>>>>>>> }
>>>>>>> EXPORT_SYMBOL(tegra_mipi_disable);
>>>>>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device
>>>>>>> *device)
>>>>>>> +{
>>>>>>> + clk_disable(device->mipi->clk);
>>>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>>>> registers here? We don't clear the START bit in the former when the
>>>>>> calibration has successfully finished, but I suspect that's because
>>>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>>>> upon cancellation to make sure the calibration does indeed stop.
>>>>> Apparently there is no way to explicitly stop calibration other
>>>>> than to
>>>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>>>
>>>>> Perhaps having a fixed delay before disabling clock could be enough to
>>>>> ensure that calibration is stopped before the clock is disabled?
>>>>>
>>>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>>>> it needs to be polled until it's unset?
>>> Confirmed with HW design team during this patch update.
>>>
>>> SW does not need to clear START bit and only write 1 takes effect to
>>> that bit.
>>>
>>> Also, no need to have delay or do any other register settings unclear as
>>> its FSM and there's nothing to get stuck.
>>>
>>> Also it goes thru small finite set of codes and by the time sensor
>>> programming happens for enabling streaming FSM will finish its
>>> calibration sequence much early and it will only wait for pads LP-11.
>>>
>>> So, during cancel we only need disable MIPI clock.
>>>
>> But there is no guarantee that cancel_calibration() couldn't be invoked
>> in the middle of the calibration process, hence FSM could freeze in an
>> intermediate state if it's running on the disabled MIPI clock, this
>> doesn't sound good.
> Actual calibration logic uses UART_FST_CAL clock which is always enabled
What enables the UART_FST_CAL clock? I don't see this clock used anywhere.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 19:33, Sowjanya Komatineni пишет:
>
> On 8/5/20 7:19 AM, Dmitry Osipenko wrote:
>> 05.08.2020 17:05, Dmitry Osipenko пишет:
>>> 05.08.2020 16:46, Thierry Reding пишет:
>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>>>>> is done.
>>>>>
>>>>> So, this patch skips disabling MIPI clock after triggering start of
>>>>> calibration and disables it only after waiting for done status from
>>>>> the calibration logic.
>>>>>
>>>>> This patch renames tegra_mipi_calibrate() as
>>>>> tegra_mipi_start_calibration()
>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>>>>> with their usage.
>>>>>
>>>>> As MIPI clock is left enabled and in case of any failures with CSI
>>>>> input
>>>>> streaming tegra_mipi_finish_calibration() will not get invoked.
>>>>> So added new API tegra_mipi_cancel_calibration() which disables
>>>>> MIPI clock
>>>>> and consumer drivers can call this in such cases.
>>>>>
>>>>> Reviewed-by: Dmitry Osipenko
>>>>> Signed-off-by: Sowjanya Komatineni
>>>>> ---
>>>>> drivers/gpu/drm/tegra/dsi.c | 4 ++--
>>>>> drivers/gpu/host1x/mipi.c | 19 ++-
>>>>> include/linux/host1x.h | 5 +++--
>>>>> 3 files changed, 15 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>>>> index 3820e8d..a7864e9 100644
>>>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct
>>>>> tegra_dsi *dsi)
>>>>> DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>>>>> tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>>>> - err = tegra_mipi_calibrate(dsi->mipi);
>>>>> + err = tegra_mipi_start_calibration(dsi->mipi);
>>>>> if (err < 0)
>>>>> return err;
>>>>> - return tegra_mipi_wait(dsi->mipi);
>>>>> + return tegra_mipi_finish_calibration(dsi->mipi);
>>>>> }
>>>>> static void tegra_dsi_set_timeout(struct tegra_dsi *dsi,
>>>>> unsigned long bclk,
>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>>>>> index e606464..b15ab6e 100644
>>>>> --- a/drivers/gpu/host1x/mipi.c
>>>>> +++ b/drivers/gpu/host1x/mipi.c
>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct
>>>>> tegra_mipi_device *dev)
>>>>> }
>>>>> EXPORT_SYMBOL(tegra_mipi_disable);
>>>>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>>>>> +{
>>>>> + clk_disable(device->mipi->clk);
>>>> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
>>>> registers here? We don't clear the START bit in the former when the
>>>> calibration has successfully finished, but I suspect that's because
>>>> the bit is self-clearing. But I wonder if we still need to clear it
>>>> upon cancellation to make sure the calibration does indeed stop.
>>> Apparently there is no way to explicitly stop calibration other than to
>>> reset MIPI calibration block, but Sowjanya says this is unnecessary.
>>>
>>> Perhaps having a fixed delay before disabling clock could be enough to
>>> ensure that calibration is stopped before the clock is disabled?
>>>
>> Actually, there is a MIPI_CAL_ACTIVE bit in the status register. Maybe
>> it needs to be polled until it's unset?
>
> Confirmed with HW design team during this patch update.
>
> SW does not need to clear START bit and only write 1 takes effect to
> that bit.
>
> Also, no need to have delay or do any other register settings unclear as
> its FSM and there's nothing to get stuck.
>
> Also it goes thru small finite set of codes and by the time sensor
> programming happens for enabling streaming FSM will finish its
> calibration sequence much early and it will only wait for pads LP-11.
>
> So, during cancel we only need disable MIPI clock.
>
But there is no guarantee that cancel_calibration() couldn't be invoked
in the middle of the calibration process, hence FSM could freeze in an
intermediate state if it's running on the disabled MIPI clock, this
doesn't sound good.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 20:04, Sowjanya Komatineni пишет: > > On 8/5/20 9:57 AM, Dmitry Osipenko wrote: >> 05.08.2020 19:50, Sowjanya Komatineni пишет: >>> On 8/5/20 9:47 AM, Dmitry Osipenko wrote: >>>> 05.08.2020 19:33, Sowjanya Komatineni пишет: >>>>> On 8/5/20 7:19 AM, Dmitry Osipenko wrote: >>>>>> 05.08.2020 17:05, Dmitry Osipenko пишет: >>>>>>> 05.08.2020 16:46, Thierry Reding пишет: >>>>>>>> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni >>>>>>>> wrote: >>>>>>>>> With the split of MIPI calibration into tegra_mipi_calibrate() and >>>>>>>>> tegra_mipi_wait(), MIPI clock is not kept enabled till the >>>>>>>>> calibration >>>>>>>>> is done. >>>>>>>>> >>>>>>>>> So, this patch skips disabling MIPI clock after triggering >>>>>>>>> start of >>>>>>>>> calibration and disables it only after waiting for done status >>>>>>>>> from >>>>>>>>> the calibration logic. >>>>>>>>> >>>>>>>>> This patch renames tegra_mipi_calibrate() as >>>>>>>>> tegra_mipi_start_calibration() >>>>>>>>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be >>>>>>>>> inline >>>>>>>>> with their usage. >>>>>>>>> >>>>>>>>> As MIPI clock is left enabled and in case of any failures with CSI >>>>>>>>> input >>>>>>>>> streaming tegra_mipi_finish_calibration() will not get invoked. >>>>>>>>> So added new API tegra_mipi_cancel_calibration() which disables >>>>>>>>> MIPI clock >>>>>>>>> and consumer drivers can call this in such cases. >>>>>>>>> >>>>>>>>> Reviewed-by: Dmitry Osipenko >>>>>>>>> Signed-off-by: Sowjanya Komatineni >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/tegra/dsi.c | 4 ++-- >>>>>>>>> drivers/gpu/host1x/mipi.c | 19 ++- >>>>>>>>> include/linux/host1x.h | 5 +++-- >>>>>>>>> 3 files changed, 15 insertions(+), 13 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dsi.c >>>>>>>>> b/drivers/gpu/drm/tegra/dsi.c >>>>>>>>> index 3820e8d..a7864e9 100644 >>>>>>>>> --- a/drivers/gpu/drm/tegra/dsi.c >>>>>>>>> +++ b/drivers/gpu/drm/tegra/dsi.c >>>>>>>>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct >>>>>>>>> tegra_dsi *dsi) >>>>>>>>> DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3); >>>>>>>>> tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3); >>>>>>>>> - err = tegra_mipi_calibrate(dsi->mipi); >>>>>>>>> + err = tegra_mipi_start_calibration(dsi->mipi); >>>>>>>>> if (err < 0) >>>>>>>>> return err; >>>>>>>>> - return tegra_mipi_wait(dsi->mipi); >>>>>>>>> + return tegra_mipi_finish_calibration(dsi->mipi); >>>>>>>>> } >>>>>>>>> static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, >>>>>>>>> unsigned long bclk, >>>>>>>>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c >>>>>>>>> index e606464..b15ab6e 100644 >>>>>>>>> --- a/drivers/gpu/host1x/mipi.c >>>>>>>>> +++ b/drivers/gpu/host1x/mipi.c >>>>>>>>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct >>>>>>>>> tegra_mipi_device *dev) >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL(tegra_mipi_disable); >>>>>>>>> -int tegra_mipi_wait(struct tegra_mipi_device *device) >>>>>>>>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device >>>>>>>>> *device) >
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 20:29, Sowjanya Komatineni пишет: ... > UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM > that goes thru sequence codes and when done waits for pads to be in > LP-11 to apply results. > > MIPI_CLK is controller gate clock which is also need to be kept enabled > as incase if it sees LP-11 it updates registers so its recommended to > have this clock enabled. > > We can cancel_calibration() in CSI only when csi/sensor stream on fails > and in which case there will be no LP-11 so we can unconditionally > disable MIPI_CLK. > There is no guarantee that the fail comes before the LP-11. For example, some odd camera driver may have a complicated enable sequence which may fail after enabling the hardware streaming.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
05.08.2020 16:46, Thierry Reding пишет:
> On Mon, Aug 03, 2020 at 08:42:24AM -0700, Sowjanya Komatineni wrote:
>> With the split of MIPI calibration into tegra_mipi_calibrate() and
>> tegra_mipi_wait(), MIPI clock is not kept enabled till the calibration
>> is done.
>>
>> So, this patch skips disabling MIPI clock after triggering start of
>> calibration and disables it only after waiting for done status from
>> the calibration logic.
>>
>> This patch renames tegra_mipi_calibrate() as tegra_mipi_start_calibration()
>> and tegra_mipi_wait() as tegra_mipi_finish_calibration() to be inline
>> with their usage.
>>
>> As MIPI clock is left enabled and in case of any failures with CSI input
>> streaming tegra_mipi_finish_calibration() will not get invoked.
>> So added new API tegra_mipi_cancel_calibration() which disables MIPI clock
>> and consumer drivers can call this in such cases.
>>
>> Reviewed-by: Dmitry Osipenko
>> Signed-off-by: Sowjanya Komatineni
>> ---
>> drivers/gpu/drm/tegra/dsi.c | 4 ++--
>> drivers/gpu/host1x/mipi.c | 19 ++-
>> include/linux/host1x.h | 5 +++--
>> 3 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>> index 3820e8d..a7864e9 100644
>> --- a/drivers/gpu/drm/tegra/dsi.c
>> +++ b/drivers/gpu/drm/tegra/dsi.c
>> @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi
>> *dsi)
>> DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3);
>> tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3);
>>
>> -err = tegra_mipi_calibrate(dsi->mipi);
>> +err = tegra_mipi_start_calibration(dsi->mipi);
>> if (err < 0)
>> return err;
>>
>> -return tegra_mipi_wait(dsi->mipi);
>> +return tegra_mipi_finish_calibration(dsi->mipi);
>> }
>>
>> static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk,
>> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
>> index e606464..b15ab6e 100644
>> --- a/drivers/gpu/host1x/mipi.c
>> +++ b/drivers/gpu/host1x/mipi.c
>> @@ -293,17 +293,19 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>> }
>> EXPORT_SYMBOL(tegra_mipi_disable);
>>
>> -int tegra_mipi_wait(struct tegra_mipi_device *device)
>> +void tegra_mipi_cancel_calibration(struct tegra_mipi_device *device)
>> +{
>> +clk_disable(device->mipi->clk);
>
> Do we need to do anything with the MIPI_CAL_CTRL and MIPI_CAL_STATUS
> registers here? We don't clear the START bit in the former when the
> calibration has successfully finished, but I suspect that's because
> the bit is self-clearing. But I wonder if we still need to clear it
> upon cancellation to make sure the calibration does indeed stop.
Apparently there is no way to explicitly stop calibration other than to
reset MIPI calibration block, but Sowjanya says this is unnecessary.
Perhaps having a fixed delay before disabling clock could be enough to
ensure that calibration is stopped before the clock is disabled?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 19:41, Sowjanya Komatineni пишет: ... >> What about to add 72us delay to the end of start_calibration() in order >> to ensure that FSM is finished before LP-11? > > Why we should add 72uS in start_calibration() when can use same > finish_calibration() for both pass/fail cases? > > Only timing loose we see is in case of failure we still wait for 250ms > and as this is failing case I hope should be ok. > You said that calibration settings are applied to pads on LP-11, but if LP-11 happens before FSM is finished, then what values will be applied if any?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 18:59, Sowjanya Komatineni пишет: .. > We cant use active status check for specific pads under calibration. > This is common bit for all pads. I'm not sure why this is a problem.
Re: [Patch v2 2/4] dmaengine: tegra: Add Tegra GPC DMA driver
06.08.2020 10:30, Rajesh Gumasta пишет:
> +static struct dma_async_tx_descriptor *tegra_dma_prep_dma_memset(
> + struct dma_chan *dc, dma_addr_t dest, int value, size_t len,
> + unsigned long flags)
> +{
This looks and reads okay, although the following style of code's
formatting is a bit more common for the kernel:
static struct dma_async_tx_descriptor *
tegra_dma_prep_dma_memset(struct dma_chan *dc, dma_addr_t dest,
int value, size_t len, unsigned long flags)
You could make this small improvement in a v3. Same for all similar
cases in the code.
Re: [Patch v2 2/4] dmaengine: tegra: Add Tegra GPC DMA driver
06.08.2020 10:30, Rajesh Gumasta пишет:
> +static int tegra_dma_program_sid(struct tegra_dma_channel *tdc,
> + int chan, int stream_id)
> +{
> + unsigned int reg_val = tdc_read(tdc, TEGRA_GPCDMA_CHAN_MCSEQ);
> +
> + reg_val &= ~(TEGRA_GPCDMA_MCSEQ_STREAM_ID_MASK <<
> + TEGRA_GPCDMA_MCSEQ_STREAM_ID0_SHIFT);
> + reg_val &= ~(TEGRA_GPCDMA_MCSEQ_STREAM_ID_MASK <<
> + TEGRA_GPCDMA_MCSEQ_STREAM_ID1_SHIFT);
> +
> + reg_val |= (stream_id << TEGRA_GPCDMA_MCSEQ_STREAM_ID0_SHIFT);
> + reg_val |= (stream_id << TEGRA_GPCDMA_MCSEQ_STREAM_ID1_SHIFT);
Have you seen my comment to v1 about FIELD_GET/FIELD_PREP?
If you disagree with something, then you should answer with a rationale
and not silently ignore review comments.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 18:59, Sowjanya Komatineni пишет: ... >>> Confirmed from HW designer, calibration FSM to finish takes worst case >>> 72uS so by the time it gets to sensor stream it will be done its >>> sequence and will be waiting for DONE bit. >>> >>> So disabling MIPI CAL clock on sensor stream fails is safe. >> >> 72us is quite a lot of time, what will happen if LP-11 happens before >> FSM finished calibration? >> >> Maybe the finish_calibration() needs to split into two parts: >> >> 1. wait for CAL_STATUS_ACTIVE before enabling sensor >> 2. wait for CAL_STATUS_DONE after enabling sensor > > I don't think we need to split for active and done. Active will be 1 as > long as other pads are in calibration as well. > > We cant use active status check for specific pads under calibration. > This is common bit for all pads. Does hardware have a single FSM block shared by all pads or there is FSM per group of pads? > Unfortunately HW don't have separate status indicating when sequence is > done to indicate its waiting for LP11. > > > To avoid all this, will remove cancel_calibration() totally and use same > finish calibration even in case of stream failure then. > What about to add 72us delay to the end of start_calibration() in order to ensure that FSM is finished before LP-11?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 19:51, Sowjanya Komatineni пишет: What about to add 72us delay to the end of start_calibration() in order to ensure that FSM is finished before LP-11? >>> Why we should add 72uS in start_calibration() when can use same >>> finish_calibration() for both pass/fail cases? >>> >>> Only timing loose we see is in case of failure we still wait for 250ms >>> and as this is failing case I hope should be ok. >>> >> You said that calibration settings are applied to pads on LP-11, but if >> LP-11 happens before FSM is finished, then what values will be applied >> if any? > > No calibration logic will check for LP-11 only after finishing > calibration sequence codes. > > After that if it sees LP-11, it will apply results to pads and DONE bit > will then be set to 1 indication pad results update. Are you sure that HW doesn't use level-triggered logic for LP-11 signal? > Unfortunately like I said we don't have status indication for > calibrating finished before waiting for LP-11. This is not a problem if hardware can cope with LP-11 happened at the time of calibration. If hardware can't cope with that, then somethings needs to be done about it.
Re: [Patch v2 2/4] dmaengine: tegra: Add Tegra GPC DMA driver
06.08.2020 10:30, Rajesh Gumasta пишет:
...
> +/*
> + * Save and restore csr and channel register on pm_suspend
> + * and pm_resume respectively
> + */
> +static int __maybe_unused tegra_dma_pm_suspend(struct device *dev)
> +{
> + struct tegra_dma *tdma = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < tdma->chip_data->nr_channels; i++) {
> + struct tegra_dma_channel *tdc = &tdma->channels[i];
> + struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> +
> + ch_reg->csr = tdc_read(tdc, TEGRA_GPCDMA_CHAN_CSR);
> + ch_reg->src_ptr = tdc_read(tdc, TEGRA_GPCDMA_CHAN_SRC_PTR);
> + ch_reg->dst_ptr = tdc_read(tdc, TEGRA_GPCDMA_CHAN_DST_PTR);
> + ch_reg->high_addr_ptr = tdc_read(tdc,
> +
> TEGRA_GPCDMA_CHAN_HIGH_ADDR_PTR);
> + ch_reg->mc_seq = tdc_read(tdc, TEGRA_GPCDMA_CHAN_MCSEQ);
> + ch_reg->mmio_seq = tdc_read(tdc, TEGRA_GPCDMA_CHAN_MMIOSEQ);
> + ch_reg->wcount = tdc_read(tdc, TEGRA_GPCDMA_CHAN_WCOUNT);
> + }
> + return 0;
> +}
> +
> +static int __maybe_unused tegra_dma_pm_resume(struct device *dev)
> +{
> + struct tegra_dma *tdma = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < tdma->chip_data->nr_channels; i++) {
> + struct tegra_dma_channel *tdc = &tdma->channels[i];
> + struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> +
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_WCOUNT, ch_reg->wcount);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_DST_PTR, ch_reg->dst_ptr);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_SRC_PTR, ch_reg->src_ptr);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_HIGH_ADDR_PTR,
> + ch_reg->high_addr_ptr);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_MMIOSEQ, ch_reg->mmio_seq);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_MCSEQ, ch_reg->mc_seq);
> + tdc_write(tdc, TEGRA_GPCDMA_CHAN_CSR,
> + (ch_reg->csr & ~TEGRA_GPCDMA_CSR_ENB));
> + }
> + return 0;
> +}
> +
> +static const struct __maybe_unused dev_pm_ops tegra_dma_dev_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
> +};
Please explain why this is needed. All DMA should be stopped (not
paused) on system's suspend, shouldn't it?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 03:47, Sowjanya Komatineni пишет: > > On 8/5/20 11:06 AM, Sowjanya Komatineni wrote: >> >> On 8/5/20 10:46 AM, Sowjanya Komatineni wrote: >>> >>> On 8/5/20 10:34 AM, Dmitry Osipenko wrote: >>>> 05.08.2020 20:29, Sowjanya Komatineni пишет: >>>> ... >>>>> UART_FST_MIPI_CAL is the clock used for calibration logic which is FSM >>>>> that goes thru sequence codes and when done waits for pads to be in >>>>> LP-11 to apply results. >>>>> >>>>> MIPI_CLK is controller gate clock which is also need to be kept >>>>> enabled >>>>> as incase if it sees LP-11 it updates registers so its recommended to >>>>> have this clock enabled. >>>>> >>>>> We can cancel_calibration() in CSI only when csi/sensor stream on >>>>> fails >>>>> and in which case there will be no LP-11 so we can unconditionally >>>>> disable MIPI_CLK. >>>>> >>>> There is no guarantee that the fail comes before the LP-11. For >>>> example, >>>> some odd camera driver may have a complicated enable sequence which may >>>> fail after enabling the hardware streaming. >>> >>> MIPI_CLK to keep enable is for calibration logic to update results, >>> but like I said calibration logic uses UART_FST_MIPI_CAL clock. So >>> even in case if fail happens from sensor after having pads in LP-11 >>> then, calibration logic will still be running but result update will >>> not happen with clock disabled. But HW will not stuck as this is >>> confirmed from HW designer. >> >> If LP-11 happens from sensor stream (followed by fail) and by that >> time if calibration FSM is done and if calibration logic sees LP-11 >> then results will be applied to pads. >> >> We did start of calibration before CSI stream so by the time we do >> sensor stream enable, calibration logic might have done with FSM and >> waiting for LP-11 >> >> Also if we see any special case, we always can use >> finish_calibration() instead of cancel_calibration() as well. Why not to do it right now? Then the code could look like this: src_subdev = tegra_channel_get_remote_source_subdev(chan); ret = v4l2_subdev_call(src_subdev, video, s_stream, true); err = tegra_mipi_finish_calibration(csi_chan->mipi); if (ret < 0 && ret != -ENOIOCTLCMD) goto err_disable_csi_stream; if (err < 0) dev_warn(csi_chan->csi->dev, "MIPI calibration failed: %d\n", err); >> finish_calibration() has extra 250ms wait time polling done bit and we >> can ignore its return code during fail pathway. >> > Confirmed from HW designer, calibration FSM to finish takes worst case > 72uS so by the time it gets to sensor stream it will be done its > sequence and will be waiting for DONE bit. > > So disabling MIPI CAL clock on sensor stream fails is safe. 72us is quite a lot of time, what will happen if LP-11 happens before FSM finished calibration? Maybe the finish_calibration() needs to split into two parts: 1. wait for CAL_STATUS_ACTIVE before enabling sensor 2. wait for CAL_STATUS_DONE after enabling sensor
Re: [Patch v2 2/4] dmaengine: tegra: Add Tegra GPC DMA driver
06.08.2020 16:56, Rajesh Gumasta пишет:
...
>>> +static const struct __maybe_unused dev_pm_ops
>> tegra_dma_dev_pm_ops = {
>>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend,
>>> +tegra_dma_pm_resume) };
>>
>> Please explain why this is needed. All DMA should be stopped (not
>> paused) on system's suspend, shouldn't it?
> I have rechecked with HW verification team and they confirmed that after
> suspend, csr and channel registers will get reset hence on resume we need to
> restore back.
> Also GPCDMA does not support power gate as a unit.
But all registers are re-programmed on starting a DMA transfer, hence
why do you need to save-restore them on suspend-resume?
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 20:12, Sowjanya Komatineni пишет: > > On 8/6/20 9:41 AM, Sowjanya Komatineni wrote: >> >> On 8/6/20 9:10 AM, Dmitry Osipenko wrote: >>> 06.08.2020 18:59, Sowjanya Komatineni пишет: >>> ... >>>>>> Confirmed from HW designer, calibration FSM to finish takes worst >>>>>> case >>>>>> 72uS so by the time it gets to sensor stream it will be done its >>>>>> sequence and will be waiting for DONE bit. >>>>>> >>>>>> So disabling MIPI CAL clock on sensor stream fails is safe. >>>>> 72us is quite a lot of time, what will happen if LP-11 happens before >>>>> FSM finished calibration? >>>>> >>>>> Maybe the finish_calibration() needs to split into two parts: >>>>> >>>>> 1. wait for CAL_STATUS_ACTIVE before enabling sensor >>>>> 2. wait for CAL_STATUS_DONE after enabling sensor >>>> I don't think we need to split for active and done. Active will be 1 as >>>> long as other pads are in calibration as well. >>>> >>>> We cant use active status check for specific pads under calibration. >>>> This is common bit for all pads. >>> Does hardware have a single FSM block shared by all pads or there is FSM >>> per group of pads? >> >> MIPI CAL status register has DONE bits for individual pads status and >> single ACTIVE bit. >> >> ACTIVE bit set to 1 indicates auto calibration is active which is the >> case even when other pads (other CSI pads from other ports streaming >> in case of parallel stream) are under calibration. Also DSI is shared >> as well. >> >> We do calibration for individual pads. So, we should not rely on >> ACTIVE bit. >> >> >> MIPI driver checks for condition ACTIVE == 1 && DONE == 1 from the >> beginning. >> >> But I think this also should be fixed as in case of parallel streams >> calibration can happen in parallel waiting for ACTIVE to be cleared >> makes all calibration callers to wait for longer than needed as ACTIVE >> is common for all pads. >> >>> >>>> Unfortunately HW don't have separate status indicating when sequence is >>>> done to indicate its waiting for LP11. >>>> >>>> >>>> To avoid all this, will remove cancel_calibration() totally and use >>>> same >>>> finish calibration even in case of stream failure then. >>>> >>> What about to add 72us delay to the end of start_calibration() in order >>> to ensure that FSM is finished before LP-11? >> >> Why we should add 72uS in start_calibration() when can use same >> finish_calibration() for both pass/fail cases? >> >> Only timing loose we see is in case of failure we still wait for 250ms >> and as this is failing case I hope should be ok. >> > Also as we don't need cancel_calibration(), keeping tegra_mipi_wait() > like earlier makes sense I believe as we are letting it finish going > thru sequence. > > So I think below are fixes, > > 1. Existing MIPI driver, tegra_mipi_wait() to not use status ACTIVE bit > to be 0 and use only DONE bit to be 1 for wait condition as we are > calibrating separately for individual pads and this ACTIVE bit is common > for all pads where it will not be 0 in case of other parallel streams > which may also be under calibration. Yes, looks like it's a mistake of the current MIPI driver that it polls the ACTIVE bit. > 2. No need for separate cancel_calibration. So, probably earlier names > tegra_mipi_calibrate() and tegra_mipi_wait() hols good as we are waiting > for calibration sequence to finish irrespective of fail/pass. The new names reflect better what those functions actually do, IMO. What about to make finish_calibration() to take an additional argument which corresponds to the awaited HW bits? For example if it's CSIA, then it could be: tegra_mipi_finish_calibration(csi_chan->mipi, MIPI_CAL_CSIA); Also, is it okay that DSI and CSI could change MIPI_CAL_CTRL after DSI or CSI already started calibration? Looking at the current start_calibration(), I think the mutex should be kept locked and then finish_calibration() should unlock it.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 19:13, Dmitry Osipenko пишет: > 06.08.2020 18:59, Sowjanya Komatineni пишет: > .. >> We cant use active status check for specific pads under calibration. >> This is common bit for all pads. > > I'm not sure why this is a problem. > IIUC, the start_calibration() should wait for the MIPI_CAL_STATUS_ACTIVE and finish_calibration() should wait for MIPI_AUTO_CAL_DONE_CSIA/B.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 20:52, Sowjanya Komatineni пишет: ... > Right mutex_unlock should happen at end of finish_calibration. > > With keeping mutex locked in start, we dont have to check for active to > be 0 to issue start as mutex will keep it locked and other pads > calibration can only go thru when current one is done. > > So instead of below sequence, its simpler to do this way? > > start_calibration() > > - mutex_lock > > - wait for 72uS after start > > finish_calibration() > > - keep check for ACTIVE = 0 and DONE = 1 I think only the DONE bits which correspond to the mipi_device->pads bitmask should be awaited. > - mutex_unlock() Perhaps the start_calibration() also needs to be changed to not touch the MIPI_CAL_CONFIG bits of the unrelated pads? Otherwise sounds good to me.
Re: [PATCH v8 08/10] gpu: host1x: mipi: Keep MIPI clock enabled till calibration is done
06.08.2020 21:07, Sowjanya Komatineni пишет: > > On 8/6/20 11:01 AM, Dmitry Osipenko wrote: >> 06.08.2020 20:52, Sowjanya Komatineni пишет: >> ... >>> Right mutex_unlock should happen at end of finish_calibration. >>> >>> With keeping mutex locked in start, we dont have to check for active to >>> be 0 to issue start as mutex will keep it locked and other pads >>> calibration can only go thru when current one is done. >>> >>> So instead of below sequence, its simpler to do this way? >>> >>> start_calibration() >>> >>> - mutex_lock >>> >>> - wait for 72uS after start >>> >>> finish_calibration() >>> >>> - keep check for ACTIVE = 0 and DONE = 1 >> I think only the DONE bits which correspond to the mipi_device->pads >> bitmask should be awaited. > > As next START can't be triggered when auto cal is ACTIVE, we should keep > this in finish. > > As we do mutex_unlock only at end of finish, other pads calibrations > dont go thru till the one in process is finished. > > So in this case ACTIVE applies to current selected pads that are under > calibration. Should be better to check only the relevant bits in order to catch bugs, otherwise you may get a DONE status from the irrelevant pads. >>> - mutex_unlock() >> Perhaps the start_calibration() also needs to be changed to not touch >> the MIPI_CAL_CONFIG bits of the unrelated pads? > Driver already takes care of programming corresponding pads config only. It writes 0 to the config of the unrelated pads, which probably isn't nice if some pads use periodic auto-calibration. https://elixir.bootlin.com/linux/v5.8/source/drivers/gpu/host1x/mipi.c#L350 Although looks like auto-calibration isn't supported by the current driver.
Re: [PATCH v9 08/10] gpu: host1x: mipi: Keep MIPI clock enabled and mutex locked till calibration done
06.08.2020 22:01, Sowjanya Komatineni пишет:
...
> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
> {
> const struct tegra_mipi_soc *soc = device->mipi->soc;
> unsigned int i;
> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct tegra_mipi_device
> *device)
> value |= MIPI_CAL_CTRL_START;
> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
>
> - mutex_unlock(&device->mipi->lock);
> - clk_disable(device->mipi->clk);
> + /*
> + * Wait for min 72uS to let calibration logic finish calibration
> + * sequence codes before waiting for pads idle state to apply the
> + * results.
> + */
> + usleep_range(75, 80);
Could you please explain why the ACTIVE bit can't be polled instead of
using the fixed delay? Doesn't ACTIVE bit represents the state of the
busy FSM?
Re: [PATCH v9 08/10] gpu: host1x: mipi: Keep MIPI clock enabled and mutex locked till calibration done
07.08.2020 06:18, Sowjanya Komatineni пишет:
>
> On 8/6/20 8:14 PM, Sowjanya Komatineni wrote:
>>
>> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote:
>>>
>>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote:
>>>> 06.08.2020 22:01, Sowjanya Komatineni пишет:
>>>> ...
>>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
>>>>> {
>>>>> const struct tegra_mipi_soc *soc = device->mipi->soc;
>>>>> unsigned int i;
>>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct
>>>>> tegra_mipi_device *device)
>>>>> value |= MIPI_CAL_CTRL_START;
>>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
>>>>> - mutex_unlock(&device->mipi->lock);
>>>>> - clk_disable(device->mipi->clk);
>>>>> + /*
>>>>> + * Wait for min 72uS to let calibration logic finish calibration
>>>>> + * sequence codes before waiting for pads idle state to apply the
>>>>> + * results.
>>>>> + */
>>>>> + usleep_range(75, 80);
>>>> Could you please explain why the ACTIVE bit can't be polled instead of
>>>> using the fixed delay? Doesn't ACTIVE bit represents the state of the
>>>> busy FSM?
>>>
>>> Based on internal discussion, ACTIVE bit gets cleared when all
>>> enabled pads calibration is done (same time as when DONE set to 1).
>>>
>>> Will request HW designer to look into design and confirm exactly when
>>> ACTIVE bit gets cleared.
>>>
>>> Will get back on this.
>>>
>> Verified with HW designer. above is correct. ACTIVE bit update happens
>> same time as DONE bit.
>>
>> Active = !(DONE)
>>
>> In case of calibration logic waiting for LP-11 where done bit does not
>> get set, ACTIVE will still be 1 and on next start trigger new
>> calibration will start
>>
> Based on internal design check from designer, as long as its in waiting
> for LP-11 stage, next calibration request can be triggered again but
> ACTIVE bit we will see it at 1. So we should check for DONE bits to
> confirm if calibration is done or not.
>
> To start next calibration, it can take effect as long as its in wait for
> LP-11 mode.
I meant the start_calibration() will poll the ACTIVE bit (calibration
busy), while the finish_calibration() will poll the DONE bit
(calibration applied).
Re: [PATCH v9 08/10] gpu: host1x: mipi: Keep MIPI clock enabled and mutex locked till calibration done
07.08.2020 07:06, Sowjanya Komatineni пишет:
>
> On 8/6/20 9:05 PM, Sowjanya Komatineni wrote:
>>
>> On 8/6/20 9:01 PM, Dmitry Osipenko wrote:
>>> 07.08.2020 06:18, Sowjanya Komatineni пишет:
>>>> On 8/6/20 8:14 PM, Sowjanya Komatineni wrote:
>>>>> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote:
>>>>>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote:
>>>>>>> 06.08.2020 22:01, Sowjanya Komatineni пишет:
>>>>>>> ...
>>>>>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
>>>>>>>> {
>>>>>>>> const struct tegra_mipi_soc *soc = device->mipi->soc;
>>>>>>>> unsigned int i;
>>>>>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct
>>>>>>>> tegra_mipi_device *device)
>>>>>>>> value |= MIPI_CAL_CTRL_START;
>>>>>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL);
>>>>>>>> - mutex_unlock(&device->mipi->lock);
>>>>>>>> - clk_disable(device->mipi->clk);
>>>>>>>> + /*
>>>>>>>> + * Wait for min 72uS to let calibration logic finish
>>>>>>>> calibration
>>>>>>>> + * sequence codes before waiting for pads idle state to
>>>>>>>> apply the
>>>>>>>> + * results.
>>>>>>>> + */
>>>>>>>> + usleep_range(75, 80);
>>>>>>> Could you please explain why the ACTIVE bit can't be polled
>>>>>>> instead of
>>>>>>> using the fixed delay? Doesn't ACTIVE bit represents the state of
>>>>>>> the
>>>>>>> busy FSM?
>>>>>> Based on internal discussion, ACTIVE bit gets cleared when all
>>>>>> enabled pads calibration is done (same time as when DONE set to 1).
>>>>>>
>>>>>> Will request HW designer to look into design and confirm exactly when
>>>>>> ACTIVE bit gets cleared.
>>>>>>
>>>>>> Will get back on this.
>>>>>>
>>>>> Verified with HW designer. above is correct. ACTIVE bit update happens
>>>>> same time as DONE bit.
>>>>>
>>>>> Active = !(DONE)
>>>>>
>>>>> In case of calibration logic waiting for LP-11 where done bit does not
>>>>> get set, ACTIVE will still be 1 and on next start trigger new
>>>>> calibration will start
>>>>>
>>>> Based on internal design check from designer, as long as its in waiting
>>>> for LP-11 stage, next calibration request can be triggered again but
>>>> ACTIVE bit we will see it at 1. So we should check for DONE bits to
>>>> confirm if calibration is done or not.
>>>>
>>>> To start next calibration, it can take effect as long as its in wait
>>>> for
>>>> LP-11 mode.
>>> I meant the start_calibration() will poll the ACTIVE bit (calibration
>>> busy), while the finish_calibration() will poll the DONE bit
>>> (calibration applied).
>>
>> ACTIVE bit can be 1 when previous calibration process does not see LP-11.
>>
>> So there is no need to use ACTIVE bit during start of calibration.
>>
>> At HW level, both ACTIVE and DONE bits get set at same time.
>>
>> So waiting for ACTIVE to be 0 during start calibration instead of
>> *75uS will not work as ACTIVE bit will not become 0 after calibration
>> sequence codes and it will get updated along with DONE bits only after
>> applying results to pads which happens after seeing LP-11 on pads.
>>
> *typo fixed
I see now, thank you.
Re: [PATCH] regulator: fix pointer table overallocation
09.08.2020 22:21, Michał Mirosław пишет:
> The code allocates sizeof(regulator_dev) for a pointer. Make it less
> generous. Let kcalloc() calculate the size, while at it.
>
> Cc: sta...@vger.kernel.org
> Fixes: d8ca7d184b33 ("regulator: core: Introduce API for regulators coupling
> customization")
> Signed-off-by: Michał Mirosław
> ---
> drivers/regulator/core.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 75ff7c563c5d..9e1899d3 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5011,20 +5011,20 @@ static void regulator_remove_coupling(struct
> regulator_dev *rdev)
>
> static int regulator_init_coupling(struct regulator_dev *rdev)
> {
> + struct regulator_dev **coupled;
> int err, n_phandles;
> - size_t alloc_size;
>
> if (!IS_ENABLED(CONFIG_OF))
> n_phandles = 0;
> else
> n_phandles = of_get_n_coupled(rdev);
>
> - alloc_size = sizeof(*rdev) * (n_phandles + 1);
> -
> - rdev->coupling_desc.coupled_rdevs = kzalloc(alloc_size, GFP_KERNEL);
> - if (!rdev->coupling_desc.coupled_rdevs)
> + coupled = kcalloc(n_phandles + 1, sizeof(*coupled), GFP_KERNEL);
> + if (!coupled)
> return -ENOMEM;
>
> + rdev->coupling_desc.coupled_rdevs = coupled;
> +
> /*
>* Every regulator should always have coupling descriptor filled with
>* at least pointer to itself.
>
Hello, Michał! Thank you for the patch! Not sure whether it's worthwhile
to backport this change since it's an improvement, I'll leave it to Mark
to decide, otherwise looks good to me.
Reviewed-by: Dmitry Osipenko
Re: [PATCH 1/1] Input: atmel_mxt_ts - allow specification of firmware file name
31.07.2020 10:57, Jiada Wang пишет:
> From: Nick Dyer
>
> On platforms which have multiple device instances using this driver, the
> firmware may be different on each device. This patch makes the user give
> the name of the firmware file when flashing.
>
> This also prevents accidental triggering of the firmware load process.
>
> Signed-off-by: Nick Dyer
> Acked-by: Benson Leung
> Acked-by: Yufeng Shen
> (cherry picked from ndyer/linux/for-upstream commit
> 76ebb7cee971cb42dfb0a3a9224403b8b09abcf1)
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis
> Signed-off-by: Jiada Wang
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 42
> 1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f..024dee7a3571 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -30,8 +30,6 @@
> #include
> #include
>
> -/* Firmware files */
> -#define MXT_FW_NAME "maxtouch.fw"
> #define MXT_CFG_NAME "maxtouch.cfg"
> #define MXT_CFG_MAGIC"OBP_RAW V1"
>
> @@ -308,6 +306,7 @@ struct mxt_data {
> struct t7_config t7_cfg;
> struct mxt_dbg dbg;
> struct gpio_desc *reset_gpio;
> + char *fw_name;
Hello, Jiada!
Have you decided to abandon the patch which allowed to specify firmware
name in a device tree?
> /* Cached parameters from object table */
> u16 T5_address;
> @@ -2766,7 +2765,7 @@ static int mxt_check_firmware_format(struct device *dev,
> return -EINVAL;
> }
>
> -static int mxt_load_fw(struct device *dev, const char *fn)
> +static int mxt_load_fw(struct device *dev)
> {
> struct mxt_data *data = dev_get_drvdata(dev);
> const struct firmware *fw = NULL;
> @@ -2776,9 +2775,9 @@ static int mxt_load_fw(struct device *dev, const char
> *fn)
> unsigned int frame = 0;
> int ret;
>
> - ret = request_firmware(&fw, fn, dev);
> + ret = request_firmware(&fw, data->fw_name, dev);
> if (ret) {
> - dev_err(dev, "Unable to open firmware %s\n", fn);
> + dev_err(dev, "Unable to open firmware %s\n", data->fw_name);
> return ret;
> }
>
> @@ -2887,6 +2886,33 @@ static int mxt_load_fw(struct device *dev, const char
> *fn)
> return ret;
> }
>
> +static int mxt_update_file_name(struct device *dev, char **file_name,
> + const char *buf, size_t count)
> +{
> + char *file_name_tmp;
> +
> + /* Simple sanity check */
> + if (count > 64) {
> + dev_warn(dev, "File name too long\n");
> + return -EINVAL;
> + }
> +
> + file_name_tmp = krealloc(*file_name, count + 1, GFP_KERNEL);
> + if (!file_name_tmp)
> + return -ENOMEM;
The allocated string is never release, this is not good.
Wouldn't it be nicer to make data->fw_name a fixed-size string?
> + *file_name = file_name_tmp;
> + memcpy(*file_name, buf, count);
> +
> + /* Echo into the sysfs entry may append newline at the end of buf */
> + if (buf[count - 1] == '\n')
> + (*file_name)[count - 1] = '\0';
> + else
> + (*file_name)[count] = '\0';
What about to use strscpy?
> + return 0;
> +}
> +
> static ssize_t mxt_update_fw_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -2894,7 +2920,11 @@ static ssize_t mxt_update_fw_store(struct device *dev,
> struct mxt_data *data = dev_get_drvdata(dev);
> int error;
>
> - error = mxt_load_fw(dev, MXT_FW_NAME);
> + error = mxt_update_file_name(dev, &data->fw_name, buf, count);
> + if (error)
> + return error;
This change breaks old behavior, I'm not sure that it's acceptable. The
default name should remain to be "maxtouch.fw", IMO.
> + error = mxt_load_fw(dev);
> if (error) {
> dev_err(dev, "The firmware update failed(%d)\n", error);
> count = error;
>
Re: [PATCH] regulator: simplify locking
10.08.2020 00:16, Michał Mirosław пишет:
> Simplify regulator locking by removing locking around locking. rdev->ref
> is now accessed only when the lock is taken. The code still smells fishy,
> but now its obvious why.
>
> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
> Signed-off-by: Michał Mirosław
> ---
> drivers/regulator/core.c | 37 ++--
> include/linux/regulator/driver.h | 1 -
> 2 files changed, 6 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9e1899d3..b0662927487c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -45,7 +45,6 @@
> pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
>
> static DEFINE_WW_CLASS(regulator_ww_class);
> -static DEFINE_MUTEX(regulator_nesting_mutex);
> static DEFINE_MUTEX(regulator_list_mutex);
> static LIST_HEAD(regulator_map_list);
> static LIST_HEAD(regulator_ena_gpio_list);
> @@ -150,32 +149,13 @@ static bool regulator_ops_is_valid(struct regulator_dev
> *rdev, int ops)
> static inline int regulator_lock_nested(struct regulator_dev *rdev,
> struct ww_acquire_ctx *ww_ctx)
> {
> - bool lock = false;
> int ret = 0;
>
> - mutex_lock(®ulator_nesting_mutex);
> + if (ww_ctx || !mutex_trylock_recursive(&rdev->mutex.base))
Have you seen comment to the mutex_trylock_recursive()?
https://elixir.bootlin.com/linux/v5.8/source/include/linux/mutex.h#L205
* This function should not be used, _ever_. It is purely for hysterical GEM
* raisins, and once those are gone this will be removed.
I knew about this function and I don't think it's okay to use it, hence
this is why there is that "nesting_mutex" and "owner" checking.
If you disagree, then perhaps you should make another patch to remove
the stale comment to trylock_recursive().
Re: regulator: deadlock vs memory reclaim
10.08.2020 01:25, Michał Mirosław пишет:
> Hi guys,
>
> Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
> from Nov 2018 tried to fix possible deadlocks when handling coupled
> regulators. Unfortunately it introduced another possible deadlock,
> as discovered by lockdep (see below), instead.
>
> regulator_lock_dependent() starts by taking regulator_list_mutex, The
> same mutex covers eg. regulator initialization, including memory allocations
> that happen there. This will deadlock when you have filesystem on eg. eMMC
> (which uses a regulator to control module voltages) and you register
> a new regulator (hotplug a device?) when under memory pressure.
>
> There is also another problem with regulator_lock_dependent(): all the
> w/w rollback stuff is useless: because of the outer lock, there can only
> be one contendant doing multiple-lock-grabbing procedure. In this setup,
> the procedure cannot detect other processes waiting on
> regulator_lock_dependent() and it cannot signal (wound a transaction of)
> current holders of locks taken by regulator_lock().
>
> Basically, we have a BKL for regulator_enable() and we're using ww_mutex
> as a recursive mutex with no deadlock prevention whatsoever. The locks
> also seem to cover way to much (eg. initialization even before making the
> regulator visible to the system).
>
> To fix the regulator vs memory reclaim path I tried pushing all allocations
> out of protected sections. After doing a few patches, though, I'm not sure
> I'm going in the right direction. Your thoughts?
IIRC, taking the regulator_list_mutex within regulator_lock_dependent()
is needed in order to protect the case of decoupling regulators.
Perhaps moving out allocations or making them GFP_NOWAIT should be the
easiest solution.
Re: [PATCH] regulator: simplify locking
10.08.2020 01:30, Michał Mirosław пишет:
> On Mon, Aug 10, 2020 at 12:40:04AM +0300, Dmitry Osipenko wrote:
>> 10.08.2020 00:16, Michał Mirosław пишет:
>>> Simplify regulator locking by removing locking around locking. rdev->ref
>>> is now accessed only when the lock is taken. The code still smells fishy,
>>> but now its obvious why.
>>>
>>> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
>>> Signed-off-by: Michał Mirosław
>>> ---
>>> drivers/regulator/core.c | 37 ++--
>>> include/linux/regulator/driver.h | 1 -
>>> 2 files changed, 6 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index 9e1899d3..b0662927487c 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -45,7 +45,6 @@
>>> pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
>>>
>>> static DEFINE_WW_CLASS(regulator_ww_class);
>>> -static DEFINE_MUTEX(regulator_nesting_mutex);
>>> static DEFINE_MUTEX(regulator_list_mutex);
>>> static LIST_HEAD(regulator_map_list);
>>> static LIST_HEAD(regulator_ena_gpio_list);
>>> @@ -150,32 +149,13 @@ static bool regulator_ops_is_valid(struct
>>> regulator_dev *rdev, int ops)
>>> static inline int regulator_lock_nested(struct regulator_dev *rdev,
>>> struct ww_acquire_ctx *ww_ctx)
>>> {
>>> - bool lock = false;
>>> int ret = 0;
>>>
>>> - mutex_lock(®ulator_nesting_mutex);
>>> + if (ww_ctx || !mutex_trylock_recursive(&rdev->mutex.base))
>>
>> Have you seen comment to the mutex_trylock_recursive()?
>>
>> https://elixir.bootlin.com/linux/v5.8/source/include/linux/mutex.h#L205
>>
>> * This function should not be used, _ever_. It is purely for hysterical GEM
>> * raisins, and once those are gone this will be removed.
>>
>> I knew about this function and I don't think it's okay to use it, hence
>> this is why there is that "nesting_mutex" and "owner" checking.
>>
>> If you disagree, then perhaps you should make another patch to remove
>> the stale comment to trylock_recursive().
>
> I think that reimplementing the function just to not use it is not the
> right solution. The whole locking protocol is problematic and this patch
> just uncovers one side of it.
It's not clear to me what is uncovered, the ref_cnt was always accessed
under lock. Could you please explain in a more details?
Would be awesome if you could improve the code, but then you should
un-deprecate the trylock_recursive() before making use of it. Maybe
nobody will mind and it all will be good in the end.
Re: [PATCH] regulator: simplify locking
10.08.2020 03:59, Michał Mirosław пишет:
> On Mon, Aug 10, 2020 at 03:21:47AM +0300, Dmitry Osipenko wrote:
>> 10.08.2020 01:30, Michał Mirosław пишет:
>>> On Mon, Aug 10, 2020 at 12:40:04AM +0300, Dmitry Osipenko wrote:
>>>> 10.08.2020 00:16, Michał Mirosław пишет:
>>>>> Simplify regulator locking by removing locking around locking. rdev->ref
>>>>> is now accessed only when the lock is taken. The code still smells fishy,
>>>>> but now its obvious why.
>>>>>
>>>>> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators
>>>>> locking")
>>>>> Signed-off-by: Michał Mirosław
>>>>> ---
>>>>> drivers/regulator/core.c | 37 ++--
>>>>> include/linux/regulator/driver.h | 1 -
>>>>> 2 files changed, 6 insertions(+), 32 deletions(-)
>>>>>
>>>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>>>> index 9e1899d3..b0662927487c 100644
>>>>> --- a/drivers/regulator/core.c
>>>>> +++ b/drivers/regulator/core.c
>>>>> @@ -45,7 +45,6 @@
>>>>> pr_debug("%s: " fmt, rdev_get_name(rdev), ##__VA_ARGS__)
>>>>>
>>>>> static DEFINE_WW_CLASS(regulator_ww_class);
>>>>> -static DEFINE_MUTEX(regulator_nesting_mutex);
>>>>> static DEFINE_MUTEX(regulator_list_mutex);
>>>>> static LIST_HEAD(regulator_map_list);
>>>>> static LIST_HEAD(regulator_ena_gpio_list);
>>>>> @@ -150,32 +149,13 @@ static bool regulator_ops_is_valid(struct
>>>>> regulator_dev *rdev, int ops)
>>>>> static inline int regulator_lock_nested(struct regulator_dev *rdev,
>>>>> struct ww_acquire_ctx *ww_ctx)
>>>>> {
>>>>> - bool lock = false;
>>>>> int ret = 0;
>>>>>
>>>>> - mutex_lock(®ulator_nesting_mutex);
>>>>> + if (ww_ctx || !mutex_trylock_recursive(&rdev->mutex.base))
>>>>
>>>> Have you seen comment to the mutex_trylock_recursive()?
>>>>
>>>> https://elixir.bootlin.com/linux/v5.8/source/include/linux/mutex.h#L205
>>>>
>>>> * This function should not be used, _ever_. It is purely for hysterical
>>>> GEM
>>>> * raisins, and once those are gone this will be removed.
>>>>
>>>> I knew about this function and I don't think it's okay to use it, hence
>>>> this is why there is that "nesting_mutex" and "owner" checking.
>>>>
>>>> If you disagree, then perhaps you should make another patch to remove
>>>> the stale comment to trylock_recursive().
>>>
>>> I think that reimplementing the function just to not use it is not the
>>> right solution. The whole locking protocol is problematic and this patch
>>> just uncovers one side of it.
>>
>> It's not clear to me what is uncovered, the ref_cnt was always accessed
>> under lock. Could you please explain in a more details?
>>
>> Would be awesome if you could improve the code, but then you should
>> un-deprecate the trylock_recursive() before making use of it. Maybe
>> nobody will mind and it all will be good in the end.
>
> I'm not sure why the framework wants recursive locking? If only for the
> coupling case, then ww_mutex seems the right direction to replace it:
> while walking the graph it will detect entering the same node
> a second time. But this works only during the locking transaction (with
> ww_context != NULL). Allowing recursive regulator_lock() outside of it
> seems inviting trouble.
Yes, it's for the coupling case. Coupled regulators may have common
ancestors and then the whole sub-tree needs to be locked while operating
with a coupled regulator.
The nested locking usage is discouraged in general because it is a
source of bugs. I guess it should be possible to get rid of all nested
lockings in the regulator core and use a pure ww_mutex, but somebody
should dedicate time to work on it.
Re: regulator: deadlock vs memory reclaim
10.08.2020 22:25, Michał Mirosław пишет:
> regulator_lock_dependent() starts by taking regulator_list_mutex, The
> same mutex covers eg. regulator initialization, including memory
> allocations
> that happen there. This will deadlock when you have filesystem on eg. eMMC
> (which uses a regulator to control module voltages) and you register
> a new regulator (hotplug a device?) when under memory pressure.
OK, that's very much a corner case, it only applies in the case of
coupled regulators. The most obvious thing here would be to move the
allocations on registration out of the locked region, we really only
need this in the regulator_find_coupler() call I think. If the
regulator isn't coupled we don't need to take the lock at all.
>>> Currently, regulator_lock_dependent() is called by eg. regulator_enable()
>>> and
>>> regulator_get_voltage(), so actually any regulator can deadlock this way.
>> The initialization cases that are the trigger are only done for coupled
>> regulators though AFAICT, otherwise we're not doing allocations with the
>> lock held and should be able to progress.
>
> I caught a few lockdep complaints that suggest otherwise, but I'm still
> looking into that.
The problem looks obvious to me. The regulator_init_coupling() is
protected with the list_mutex, the regulator_lock_dependent() also
protected with the list_mutex. Hence if offending reclaim happens from
init_coupling(), then there is a lockup.
1. mutex_lock(®ulator_list_mutex);
2. regulator_init_coupling()
3. kzalloc()
4. reclaim ...
5. regulator_get_voltage()
6. regulator_lock_dependent()
7. mutex_lock(®ulator_list_mutex);
It should be enough just to keep the regulator_find_coupler() under
lock, or even completely remove the locking around init_coupling(). I
think it should be better to keep the find_coupler() protected.
Michał, does this fix yours problem?
--- >8 ---
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 75ff7c563c5d..513f95c6f837 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5040,7 +5040,10 @@ static int regulator_init_coupling(struct
regulator_dev *rdev)
if (!of_check_coupling_data(rdev))
return -EPERM;
+ mutex_lock(®ulator_list_mutex);
rdev->coupling_desc.coupler = regulator_find_coupler(rdev);
+ mutex_unlock(®ulator_list_mutex);
+
if (IS_ERR(rdev->coupling_desc.coupler)) {
err = PTR_ERR(rdev->coupling_desc.coupler);
rdev_err(rdev, "failed to get coupler: %d\n", err);
@@ -5248,9 +5251,7 @@ regulator_register(const struct regulator_desc
*regulator_desc,
if (ret < 0)
goto wash;
- mutex_lock(®ulator_list_mutex);
ret = regulator_init_coupling(rdev);
- mutex_unlock(®ulator_list_mutex);
if (ret < 0)
goto wash;
--- >8 ---
Re: [PATCH v2 21/48] PM: domains: Add "performance" column to debug summary
11.01.2021 12:13, Ulf Hansson пишет: > On Thu, 17 Dec 2020 at 19:07, Dmitry Osipenko wrote: >> >> Add "performance" column to debug summary which shows performance state >> of all power domains and theirs devices. >> >> Signed-off-by: Dmitry Osipenko > > Reviewed-by: Ulf Hansson > > As I stated for patch 20, I suggest you resend this to the linux-pm > mailing list so it can be picked up by Rafael. Thanks, I'm finalizing v3 with various fixes and improvements over all the patches, including this one, which will get a slightly improved seq_printf formatting.
Re: [PATCH v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl()
20.10.2020 19:37, Sowjanya Komatineni пишет:
>
> On 10/20/20 12:48 AM, Thierry Reding wrote:
>> On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
>>> VI I2C don't have DMA support and uses PIO mode all the time.
>>>
>>> Current driver uses writesl() to fill TX FIFO based on available
>>> empty slots and with this seeing strange silent hang during any I2C
>>> register access after filling TX FIFO with 8 words.
>>>
>>> Using writel() followed by i2c_readl() in a loop to write all words
>>> to TX FIFO instead of using writesl() helps for large transfers in
>>> PIO mode.
>>>
>>> So, this patch updates i2c_writesl() API to use writel() in a loop
>>> instead of writesl().
>>>
>>> Signed-off-by: Sowjanya Komatineni
>>> ---
>>> drivers/i2c/busses/i2c-tegra.c | 9 ++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c
>>> b/drivers/i2c/busses/i2c-tegra.c
>>> index 6f08c0c..274bf3a 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev
>>> *i2c_dev, unsigned int reg)
>>> return readl_relaxed(i2c_dev->base +
>>> tegra_i2c_reg_addr(i2c_dev, reg));
>>> }
>>> -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>>> unsigned int reg, unsigned int len)
>>> {
>>> - writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data,
>>> len);
>>> + while (len--) {
>>> + writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev,
>>> reg));
>>> + i2c_readl(i2c_dev, I2C_INT_STATUS);
>>> + }
>>> }
>>> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>> i2c_dev->msg_buf_remaining = buf_remaining;
>>> i2c_dev->msg_buf = buf + words_to_transfer *
>>> BYTES_PER_FIFO_WORD;
>>> - i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
>>> + i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO,
>>> words_to_transfer);
>> I've thought a bit more about this and I wonder if we're simply reading
>> out the wrong value for tx_fifo_avail and therefore end up overflowing
>> the TX FIFO. Have you checked what the value is for tx_fifo_avail when
>> this silent hang occurs? Given that this is specific to the VI I2C I'm
>> wondering if this is perhaps a hardware bug where we read the wrong TX
>> FIFO available count.
>>
>> Thierry
>
> Yes FIFO status shows all 8 slots available.
Please explain how you checked that 8 slots are available, provide
example code.
Re: [PATCH v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl()
11.01.2021 14:50, Dmitry Osipenko пишет:
> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>
>> On 10/20/20 12:48 AM, Thierry Reding wrote:
>>> On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
>>>> VI I2C don't have DMA support and uses PIO mode all the time.
>>>>
>>>> Current driver uses writesl() to fill TX FIFO based on available
>>>> empty slots and with this seeing strange silent hang during any I2C
>>>> register access after filling TX FIFO with 8 words.
>>>>
>>>> Using writel() followed by i2c_readl() in a loop to write all words
>>>> to TX FIFO instead of using writesl() helps for large transfers in
>>>> PIO mode.
>>>>
>>>> So, this patch updates i2c_writesl() API to use writel() in a loop
>>>> instead of writesl().
>>>>
>>>> Signed-off-by: Sowjanya Komatineni
>>>> ---
>>>> drivers/i2c/busses/i2c-tegra.c | 9 ++---
>>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c
>>>> b/drivers/i2c/busses/i2c-tegra.c
>>>> index 6f08c0c..274bf3a 100644
>>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>>> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev
>>>> *i2c_dev, unsigned int reg)
>>>> return readl_relaxed(i2c_dev->base +
>>>> tegra_i2c_reg_addr(i2c_dev, reg));
>>>> }
>>>> -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>>>> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>>>> unsigned int reg, unsigned int len)
>>>> {
>>>> - writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data,
>>>> len);
>>>> + while (len--) {
>>>> + writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev,
>>>> reg));
>>>> + i2c_readl(i2c_dev, I2C_INT_STATUS);
>>>> + }
>>>> }
>>>> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>>>> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct
>>>> tegra_i2c_dev *i2c_dev)
>>>> i2c_dev->msg_buf_remaining = buf_remaining;
>>>> i2c_dev->msg_buf = buf + words_to_transfer *
>>>> BYTES_PER_FIFO_WORD;
>>>> - i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
>>>> + i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO,
>>>> words_to_transfer);
>>> I've thought a bit more about this and I wonder if we're simply reading
>>> out the wrong value for tx_fifo_avail and therefore end up overflowing
>>> the TX FIFO. Have you checked what the value is for tx_fifo_avail when
>>> this silent hang occurs? Given that this is specific to the VI I2C I'm
>>> wondering if this is perhaps a hardware bug where we read the wrong TX
>>> FIFO available count.
>>>
>>> Thierry
>>
>> Yes FIFO status shows all 8 slots available.
>
> Please explain how you checked that 8 slots are available, provide
> example code.
>
Have you checked the FIFO overflow interrupt?
Re: [PATCH] i2c: tegra: Wait for config load atomically while in ISR
11.01.2021 16:55, Mikko Perttunen пишет:
> Upon a communication error, the interrupt handler can call
> tegra_i2c_disable_packet_mode. This causes a sleeping poll to happen
> unless the current transaction was marked atomic. Since
> tegra_i2c_disable_packet_mode is only called from the interrupt path,
> make it use atomic waiting always.
>
> Fixes: ede2299f7101 ("i2c: tegra: Support atomic transfers")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Mikko Perttunen
> ---
> This fixes constant spew for me while HDMI is connected to a
> powered off television.
>
> drivers/i2c/busses/i2c-tegra.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6f08c0c3238d..4a7ff1840565 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -528,12 +528,12 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev
> *i2c_dev)
>
> static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
> u32 reg, u32 mask, u32 delay_us,
> -u32 timeout_us)
> +u32 timeout_us, bool force_atomic)
> {
> void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> u32 val;
>
> - if (!i2c_dev->atomic_mode)
> + if (!i2c_dev->atomic_mode && !force_atomic)
> return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> delay_us, timeout_us);
>
> @@ -560,7 +560,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
> *i2c_dev)
> val |= mask;
> i2c_writel(i2c_dev, val, offset);
>
> - err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100);
> + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 100,
> false);
> if (err) {
> dev_err(i2c_dev->dev, "failed to flush FIFO\n");
> return err;
> @@ -569,7 +569,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
> *i2c_dev)
> return 0;
> }
>
> -static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
> +static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev,
> bool force_atomic)
> {
> int err;
>
> @@ -579,7 +579,7 @@ static int tegra_i2c_wait_for_config_load(struct
> tegra_i2c_dev *i2c_dev)
> i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>
> err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0x,
> - 1000, I2C_CONFIG_LOAD_TIMEOUT);
> + 1000, I2C_CONFIG_LOAD_TIMEOUT,
> force_atomic);
> if (err) {
> dev_err(i2c_dev->dev, "failed to load config\n");
> return err;
> @@ -684,7 +684,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
> i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
>
> - err = tegra_i2c_wait_for_config_load(i2c_dev);
> + err = tegra_i2c_wait_for_config_load(i2c_dev, false);
> if (err)
> return err;
>
> @@ -707,7 +707,7 @@ static int tegra_i2c_disable_packet_mode(struct
> tegra_i2c_dev *i2c_dev)
> if (cnfg & I2C_CNFG_PACKET_MODE_EN)
> i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG);
>
> - return tegra_i2c_wait_for_config_load(i2c_dev);
> + return tegra_i2c_wait_for_config_load(i2c_dev, true);
> }
>
> static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> @@ -1090,7 +1090,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter
> *adap)
> I2C_BC_TERMINATE;
> i2c_writel(i2c_dev, val, I2C_BUS_CLEAR_CNFG);
>
> - err = tegra_i2c_wait_for_config_load(i2c_dev);
> + err = tegra_i2c_wait_for_config_load(i2c_dev, false);
> if (err)
> return err;
>
>
What about a simpler variant:
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 6f08c0c3238d..0727383f4940 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -533,7 +533,7 @@ static int tegra_i2c_poll_register(struct
tegra_i2c_dev *i2c_dev,
void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
u32 val;
- if (!i2c_dev->atomic_mode)
+ if (!i2c_dev->atomic_mode && !in_irq())
return readl_relaxed_poll_timeout(addr, val, !(val & mask),
delay_us, timeout_us);
Re: [PATCH v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl()
11.01.2021 20:38, Sowjanya Komatineni пишет:
>
> On 1/11/21 4:09 AM, Dmitry Osipenko wrote:
>> 11.01.2021 14:50, Dmitry Osipenko пишет:
>>> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>>> On 10/20/20 12:48 AM, Thierry Reding wrote:
>>>>> On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
>>>>>> VI I2C don't have DMA support and uses PIO mode all the time.
>>>>>>
>>>>>> Current driver uses writesl() to fill TX FIFO based on available
>>>>>> empty slots and with this seeing strange silent hang during any I2C
>>>>>> register access after filling TX FIFO with 8 words.
>>>>>>
>>>>>> Using writel() followed by i2c_readl() in a loop to write all words
>>>>>> to TX FIFO instead of using writesl() helps for large transfers in
>>>>>> PIO mode.
>>>>>>
>>>>>> So, this patch updates i2c_writesl() API to use writel() in a loop
>>>>>> instead of writesl().
>>>>>>
>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>> ---
>>>>>> drivers/i2c/busses/i2c-tegra.c | 9 ++---
>>>>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c
>>>>>> b/drivers/i2c/busses/i2c-tegra.c
>>>>>> index 6f08c0c..274bf3a 100644
>>>>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>>>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>>>>> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev
>>>>>> *i2c_dev, unsigned int reg)
>>>>>> return readl_relaxed(i2c_dev->base +
>>>>>> tegra_i2c_reg_addr(i2c_dev, reg));
>>>>>> }
>>>>>> -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void
>>>>>> *data,
>>>>>> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>>>>>> unsigned int reg, unsigned int len)
>>>>>> {
>>>>>> - writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data,
>>>>>> len);
>>>>>> + while (len--) {
>>>>>> + writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev,
>>>>>> reg));
>>>>>> + i2c_readl(i2c_dev, I2C_INT_STATUS);
>>>>>> + }
>>>>>> }
>>>>>> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void
>>>>>> *data,
>>>>>> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct
>>>>>> tegra_i2c_dev *i2c_dev)
>>>>>> i2c_dev->msg_buf_remaining = buf_remaining;
>>>>>> i2c_dev->msg_buf = buf + words_to_transfer *
>>>>>> BYTES_PER_FIFO_WORD;
>>>>>> - i2c_writesl(i2c_dev, buf, I2C_TX_FIFO,
>>>>>> words_to_transfer);
>>>>>> + i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO,
>>>>>> words_to_transfer);
>>>>> I've thought a bit more about this and I wonder if we're simply
>>>>> reading
>>>>> out the wrong value for tx_fifo_avail and therefore end up overflowing
>>>>> the TX FIFO. Have you checked what the value is for tx_fifo_avail when
>>>>> this silent hang occurs? Given that this is specific to the VI I2C I'm
>>>>> wondering if this is perhaps a hardware bug where we read the wrong TX
>>>>> FIFO available count.
>>>>>
>>>>> Thierry
>>>> Yes FIFO status shows all 8 slots available.
>>> Please explain how you checked that 8 slots are available, provide
>>> example code.
>>>
>> Have you checked the FIFO overflow interrupt?
>
> This is seen with VI I2C (which is under host1x) as we use PIO mode
> always even for large transfers.
> HW wise VI I2C is similar to other I2C and FIFO depth is also 8 words.
>
> tegra_i2c_fill_tx_fifo() reads I2C_FIFO_STATUS register field
> TX_FIFO_EMPTY_CNT which tells empty slots available in TX_FIFO that can
> be filled.
> Added debug message to print empty count and, during beginning of
> transfer when it executes tegra_i2c_fill_tx_fifo(), empty slots are 8
>
>
> Using writesl() for filling TX_FIFO causing silent hang immediate on any
> i2c register access after filling FIFO with 8 words and some times with
> 6 words as well.
>
> So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows
> when this silent hang happens.
>
> Tried to read thru back-door (JTAG path) but could not connect to JTAG
> either. Looks like Tegra chip is in some weird state.
>
> But using writel() followed by i2c_readl helps. Not sure if any thing
> related to register access delay or some other issue.
>
>
Does downstream kernel have this problem?
If there is really no good alternative right now, then perhaps should be
a bit cleaner to add i2c_writesl_vi(), which should contain explanatory
comment telling why it's needed and then it should be used only for the
VI I2C controller.
Re: [PATCH v2] i2c: tegra: Wait for config load atomically while in ISR
11.01.2021 19:08, Mikko Perttunen пишет:
> Upon a communication error, the interrupt handler can call
> tegra_i2c_disable_packet_mode. This causes a sleeping poll to happen
> unless the current transaction was marked atomic. Fix this by
> making the poll happen atomically if we are in an IRQ.
>
> This matches the behavior prior to the patch mentioned
> in the Fixes tag.
>
> Fixes: ede2299f7101 ("i2c: tegra: Support atomic transfers")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Mikko Perttunen
> ---
> v2:
> * Use in_irq() instead of passing a flag from the ISR.
> Thanks to Dmitry for the suggestion.
> * Update commit message.
> ---
> drivers/i2c/busses/i2c-tegra.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6f08c0c3238d..0727383f4940 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -533,7 +533,7 @@ static int tegra_i2c_poll_register(struct tegra_i2c_dev
> *i2c_dev,
> void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
> u32 val;
>
> - if (!i2c_dev->atomic_mode)
> + if (!i2c_dev->atomic_mode && !in_irq())
> return readl_relaxed_poll_timeout(addr, val, !(val & mask),
> delay_us, timeout_us);
>
>
Reviewed-by: Dmitry Osipenko
Re: [PATCH v2] i2c: tegra: Create i2c_writesl_vi() to use with VI I2C for filling TX FIFO
12.01.2021 07:06, Sowjanya Komatineni пишет:
> VI I2C don't have DMA support and uses PIO mode all the time.
>
> Current driver uses writesl() to fill TX FIFO based on available
> empty slots and with this seeing strange silent hang during any I2C
> register access after filling TX FIFO with 8 words.
>
> Using writel() followed by i2c_readl() in a loop to write all words
> to TX FIFO instead of using writesl() helps for large transfers in
> PIO mode.
>
> So, this patch creates i2c_writesl_vi() API to use with VI I2C for
> filling TX FIFO.
>
> Signed-off-by: Sowjanya Komatineni
> ---
> drivers/i2c/busses/i2c-tegra.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6f08c0c..e2b7503 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -339,6 +339,21 @@ static void i2c_writesl(struct tegra_i2c_dev *i2c_dev,
> void *data,
> writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> }
>
> +static void i2c_writesl_vi(struct tegra_i2c_dev *i2c_dev, u32 *data,
> +unsigned int reg, unsigned int len)
> +{
> + /*
> + * Using writesl() to fill VI I2C TX FIFO for transfers more than
> + * 6 words is causing a silent hang on any VI I2C register access
> + * after TX FIFO writes.
> + * So using writel() followed by i2c_readl().
> + */
> + while (len--) {
> + writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev,
> reg));
> + i2c_readl(i2c_dev, I2C_INT_STATUS);
> + }
> +}
> +
> static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
> unsigned int reg, unsigned int len)
> {
> @@ -811,7 +826,10 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev
> *i2c_dev)
> i2c_dev->msg_buf_remaining = buf_remaining;
> i2c_dev->msg_buf = buf + words_to_transfer *
> BYTES_PER_FIFO_WORD;
>
> - i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
> + if (i2c_dev->is_vi)
> + i2c_writesl_vi(i2c_dev, (u32 *)buf, I2C_TX_FIFO,
> words_to_transfer);
> + else
> + i2c_writesl(i2c_dev, buf, I2C_TX_FIFO,
> words_to_transfer);
>
> buf += words_to_transfer * BYTES_PER_FIFO_WORD;
> }
>
Looks almost good, could we please use a relaxed writel and avoid the casting
in the code?
Like this:
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 6f08c0c3238d..4f843b423d83 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -326,6 +326,8 @@ static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32
val, unsigned int reg)
/* read back register to make sure that register writes completed */
if (reg != I2C_TX_FIFO)
readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+ else
+ readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev,
I2C_INT_STATUS));
}
static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned int reg)
@@ -339,6 +341,21 @@ static void i2c_writesl(struct tegra_i2c_dev *i2c_dev,
void *data,
writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
}
+static void i2c_writesl_vi(struct tegra_i2c_dev *i2c_dev, void *data,
+ unsigned int reg, unsigned int len)
+{
+ u32 *data32 = data;
+
+ /*
+* Using writesl() to fill VI I2C TX FIFO for transfers more than
+* 6 words is causing a silent hang on any VI I2C register access
+* after TX FIFO writes. Each write to FIFO should follow by a read
+* of any I2C register in order to work around the problem.
+*/
+ while (len--)
+ i2c_writel(i2c_dev, *data32++, reg);
+}
+
static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
unsigned int reg, unsigned int len)
{
@@ -811,7 +828,10 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev
*i2c_dev)
i2c_dev->msg_buf_remaining = buf_remaining;
i2c_dev->msg_buf = buf + words_to_transfer *
BYTES_PER_FIFO_WORD;
- i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
+ if (i2c_dev->is_vi)
+ i2c_writesl_vi(i2c_dev, buf, I2C_TX_FIFO,
words_to_transfer);
+ else
+ i2c_writesl(i2c_dev, buf, I2C_TX_FIFO,
words_to_transfer);
buf += words_to_transfer * BYTES_PER_FIFO_WORD;
}
Re: [PATCH v2] i2c: tegra: Wait for config load atomically while in ISR
11.01.2021 22:31, Dmitry Osipenko пишет:
> 11.01.2021 19:08, Mikko Perttunen пишет:
>> Upon a communication error, the interrupt handler can call
>> tegra_i2c_disable_packet_mode. This causes a sleeping poll to happen
>> unless the current transaction was marked atomic. Fix this by
>> making the poll happen atomically if we are in an IRQ.
>>
>> This matches the behavior prior to the patch mentioned
>> in the Fixes tag.
>>
>> Fixes: ede2299f7101 ("i2c: tegra: Support atomic transfers")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Mikko Perttunen
>> ---
>> v2:
>> * Use in_irq() instead of passing a flag from the ISR.
>> Thanks to Dmitry for the suggestion.
>> * Update commit message.
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 6f08c0c3238d..0727383f4940 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -533,7 +533,7 @@ static int tegra_i2c_poll_register(struct tegra_i2c_dev
>> *i2c_dev,
>> void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
>> u32 val;
>>
>> -if (!i2c_dev->atomic_mode)
>> +if (!i2c_dev->atomic_mode && !in_irq())
>> return readl_relaxed_poll_timeout(addr, val, !(val & mask),
>>delay_us, timeout_us);
>>
>>
>
> Reviewed-by: Dmitry Osipenko
>
Perhaps a follow up change could be to use a threaded interrupt context,
I'll type a patch for that.
Re: [PATCH v3 0/9] Support Runtime PM and host mode by Tegra ChipIdea USB driver
29.12.2020 17:26, Dmitry Osipenko пишет: > 29.12.2020 08:16, Peter Chen пишет: >> On 20-12-18 15:02:37, Dmitry Osipenko wrote: >>> This series implements Runtime PM support for the Tegra ChipIdea USB driver. >>> It also squashes the older ehci-tegra driver into the ChipIdea driver, hence >>> the RPM is supported by both UDC and host controllers, secondly this opens >>> opportunity for implementing OTG support in the future. >>> >>> Patchset was tested on various Tegra20, Tegra30 and Tegra124 devices. >>> Thanks to Peter Geis, Matt Merhar, Nicolas Chauvet and Ion Agorria for >>> helping with the extensive and productive testing! >>> >>> Changelog: >>> >>> v3: - Replaced "goto" with if-statements as was suggested by Thierry Reding. >>> >>> - Improved wording of the deprecated Kconfig entry as was suggested >>> by Alan Stern. >>> >>> - Added ACKs from Thierry Reding and Alan Stern. >>> >>> - Added a new minor patch "Specify TX FIFO threshold in UDC SoC info" >>> just for completeness, since we can now switch OTG to host mode in >>> the ChipIdea driver. Although, OTG support remains a work-in-progress >>> for now. >>> >>> v2: - Improved comments in the code as it was suggested by Peter Chen and >>> Sergei Shtylyov for v1. >>> >>> - Replaced mdelay() with fsleep() and made ci->hdc to reset to NULL in >>> a error code path, like it was suggested by Peter Chen. >>> >>> - Redirected deprecated USB_EHCI_TEGRA Kconfig entry to >>> USB_CHIPIDEA_TEGRA >>> as was suggested by Alan Stern. >>> >>> - Improved commit message and added ACK from Thierry Reding to the patch >>> that removes MODULE_ALIAS. >>> >>> - Fixed UDC PHY waking up on ASUS TF201 tablet device by utilizing >>> additional VBUS sensor. This was reported and tested by Ion Agorria. >>> >>> - Added t-b from Ion Agorria. >>> >>> Dmitry Osipenko (8): >>> usb: phy: tegra: Add delay after power up >>> usb: phy: tegra: Support waking up from a low power mode >>> usb: chipidea: tegra: Remove MODULE_ALIAS >>> usb: chipidea: tegra: Rename UDC to USB >>> usb: chipidea: tegra: Support runtime PM >>> usb: chipidea: tegra: Specify TX FIFO threshold in UDC SoC info >>> usb: host: ehci-tegra: Remove the driver >>> ARM: tegra_defconfig: Enable USB_CHIPIDEA_HOST and remove >>> USB_EHCI_TEGRA >>> >>> Peter Geis (1): >>> usb: chipidea: tegra: Support host mode >> >> Chipidea related (patch 3-7) are applied, thanks. > > Hello Peter, > > Thank you for applying the patches. > > Who will apply the remaining patches? > > The Chipidea patch #6 depends on the PHY changes, otherwise USB will > suspend and never resume. > Peter, could you please apply the PHY and defconfig patches along with the CI patches to -next? I.e. the whole series. Preferentially in original ordering of patches should be preserved. Thanks in advance.
Re: [PATCH v8 0/3] Introduce Embedded Controller driver for Acer A500
28.12.2020 19:05, Dmitry Osipenko пишет: > This series adds support for the Embedded Controller which is found on > Acer Iconia Tab A500 (Android tablet device). > > The Embedded Controller is ENE KB930 and it's running firmware customized > for the A500. The firmware interface may be reused by some other sibling > Acer tablets, although none of those tablets are supported in upstream yet. > > Changelog: > > v8: - This series partially missed v5.11 kernel release, hence resending > for v5.12. Hello Lee, Could you please take a look at the MFD patch? If it's good to you, then please apply this whole series via MFD tree. Thanks in advance.
[bug] RTC alarm vs system suspend race condition
Hello RTC maintainers, A day ago we were testing RTC alarm on NVIDIA Tegra devices and noticed that there is a problem in the RTC core where it schedules __rtc_set_alarm work when alarm is set, but this work isn't flushed before RTC drivers are suspended. In general RTC devices can't be accessed once driver's suspend is invoked, creating the problem. Please see this example: # rtcwake -s15 -mmem On Ouya board: PM: suspend entry (deep) Filesystems sync: 0.001 seconds Freezing user space processes ... (elapsed 0.002 seconds) done. OOM killer disabled. Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. smsc95xx 1-1:1.0 enxb85af7003b21: entering SUSPEND2 mode [ cut here ] WARNING: CPU: 1 PID: 1337 at drivers/i2c/i2c-core.h:54 __i2c_transfer+0x6d0/0x6ec i2c i2c-1: Transfer while suspended Modules linked in: brcmfmac brcmutil CPU: 1 PID: 1337 Comm: kworker/1:3 Not tainted 5.11.0-rc2-next-20210108-15881-g0baf1450b32d #196 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events rtc_timer_do_work [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0xc4/0xd8) [] (dump_stack) from [] (__warn+0xec/0x104) [] (__warn) from [] (warn_slowpath_fmt+0x98/0xc8) [] (warn_slowpath_fmt) from [] (__i2c_transfer+0x6d0/0x6ec) [] (__i2c_transfer) from [] (i2c_transfer+0x9c/0x108) [] (i2c_transfer) from [] (regmap_i2c_read+0x60/0x9c) [] (regmap_i2c_read) from [] (_regmap_raw_read+0x104/0x314) [] (_regmap_raw_read) from [] (_regmap_bus_read+0x44/0x70) Disabling non-boot CPUs ... [] (_regmap_bus_read) from [] (_regmap_read+0x60/0x180) [] (_regmap_read) from [] (_regmap_update_bits+0xbc/0xf8) [] (_regmap_update_bits) from [] (regmap_update_bits_base+0x4c/0x70) [] (regmap_update_bits_base) from [] (tps65910_rtc_read_time+0x50/0x134) [] (tps65910_rtc_read_time) from [] (__rtc_read_time+0x48/0x94) [] (__rtc_read_time) from [] (__rtc_set_alarm+0x80/0x1dc) [] (__rtc_set_alarm) from [] (rtc_timer_do_work+0x254/0x448) [] (rtc_timer_do_work) from [] (process_one_work+0x1dc/0x5a0) [] (process_one_work) from [] (worker_thread+0x4c/0x520) [] (worker_thread) from [] (kthread+0x18c/0x190) [] (kthread) from [] (ret_from_fork+0x14/0x24) Exception stack(0xc5709fb0 to 0xc5709ff8) 9fa0: 9fc0: 9fe0: 0013 ---[ end trace 2df194007d41e38b ]--- tps65910-rtc tps65910-rtc: RTC CTRL reg update failed with err:-108 tps65910-rtc tps65910-rtc: RTC CTRL reg update failed with err:-108 tps65910-rtc tps65910-rtc: RTC CTRL reg update failed with err:-108 tps65910-rtc tps65910-rtc: RTC CTRL reg update failed with err:-108 rtc rtc0: __rtc_set_alarm: err=-108 tps65910-rtc tps65910-rtc: RTC CTRL reg update failed with err:-108 IRQ 26: no longer affine to CPU1 IRQ 27: no longer affine to CPU2 IRQ 28: no longer affine to CPU3 Entering suspend state LP1 Enabling non-boot CPUs ... CPU1 is up On PAZ00 board: PM: suspend entry (deep) Filesystems sync: 0.697 seconds Freezing user space processes ... (elapsed 0.002 seconds) done. OOM killer disabled. Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. printk: Suspending console(s) (use no_console_suspend to debug) Disabling non-boot CPUs ... IRQ 26: no longer affine to CPU1 Entering suspend state LP1 [ cut here ] WARNING: CPU: 0 PID: 83 at drivers/i2c/i2c-core.h:54 __i2c_transfer+0x400/0x458 i2c i2c-2: Transfer while suspended Modules linked in: rt2800usb rt2x00usb rt2800lib rt2x00lib xfs libcrc32c fuse crc32_generic f2fs tegra_drm gpu_sched panel_simple tegra20_emc ci_hdrc_tegra host1x_drv iova CPU: 0 PID: 83 Comm: kworker/0:2 Not tainted 5.11.0-rc2-next-20210106-tegra+ #181 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events_power_efficient sync_hw_clock [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0xc0/0xd4) [] (dump_stack) from [] (__warn+0xc0/0x11c) [] (__warn) from [] (warn_slowpath_fmt+0x98/0xc0) [] (warn_slowpath_fmt) from [] (__i2c_transfer+0x400/0x458) [] (__i2c_transfer) from [] (i2c_transfer+0x9c/0x108) [] (i2c_transfer) from [] (regmap_i2c_read+0x60/0x90) [] (regmap_i2c_read) from [] (_regmap_raw_read+0xe0/0x160) [] (_regmap_raw_read) from [] (_regmap_bus_read+0x44/0x70) [] (_regmap_bus_read) from [] (_regmap_read+0x60/0xb8) [] (_regmap_read) from [] (_regmap_update_bits+0xb0/0xec) [] (_regmap_update_bits) from [] (regmap_update_bits_base+0x50/0x74) [] (regmap_update_bits_base) from [] (tps6586x_clr_bits+0x24/0x2c) [] (tps6586x_clr_bits) from [] (tps6586x_rtc_set_time+0x68/0x104) [] (tps6586x_rtc_set_time) from [] (rtc_set_time+0xcc/0x178) [] (rtc_set_time) from [] (sync_hw_clock+0x1d0/0x250) [] (sync_hw_clock) from [] (process_one_work+0x1e8/0x44c) [] (process_one_work) from [] (worker_thread+0x64/0x5a8)
Re: [PATCH] drivers: core: Detach device from power domain on shutdown
02.12.2020 00:30, Furquan Shaikh пишет: > When the system is powered off or rebooted, devices are not detached > from their PM domain. This results in ACPI PM not being invoked and > hence PowerResouce _OFF method not being invoked for any of the > devices. Because the ACPI power resources are not turned off in case > of poweroff and reboot, it violates the power sequencing requirements > which impacts the reliability of the devices over the lifetime of the > platform. This is currently observed on all Chromebooks using ACPI. > > In order to solve the above problem, this change detaches a device > from its PM domain whenever it is shutdown. This action is basically > analogous to ->remove() from driver model perspective. Detaching the > device from its PM domain ensures that the ACPI PM gets a chance to > turn off the power resources for the device thus complying with its > power sequencing requirements. > > Signed-off-by: Furquan Shaikh > --- > drivers/base/core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index d661ada1518f..5823f1d719e1 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -4057,6 +4058,8 @@ void device_shutdown(void) > dev->driver->shutdown(dev); > } > > + dev_pm_domain_detach(dev, true); > + > device_unlock(dev); > if (parent) > device_unlock(parent); > This patch broke system shutdown on NVIDIA Tegra using today's linux-next because power domain can't be turned off until device drivers handed control over device resets to the power domain of Power Management controller on Tegra. This patch introduced the wrong behaviour, apparently it should be made specific to ACPI only. Please fix, thanks in advance.
[PATCH v2 3/5] clk: tegra: Ensure that PLLU configuration is applied properly
The PLLU (USB) consists of the PLL configuration itself and configuration of the PLLU outputs. The PLLU programming is inconsistent on T30 vs T114, where T114 immediately bails out if PLLU is enabled and T30 re-enables a potentially already enabled PLL (left after bootloader) and then fully reprograms it, which could be unsafe to do. The correct way should be to skip enabling of the PLL if it's already enabled and then apply configuration to the outputs. This patch doesn't fix any known problems, it's a minor improvement. Signed-off-by: Dmitry Osipenko --- drivers/clk/tegra/clk-pll.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c index c5cc0a2dac6f..d709ecb7d8d7 100644 --- a/drivers/clk/tegra/clk-pll.c +++ b/drivers/clk/tegra/clk-pll.c @@ -1131,7 +1131,8 @@ static int clk_pllu_enable(struct clk_hw *hw) if (pll->lock) spin_lock_irqsave(pll->lock, flags); - _clk_pll_enable(hw); + if (!clk_pll_is_enabled(hw)) + _clk_pll_enable(hw); ret = clk_pll_wait_for_lock(pll); if (ret < 0) @@ -1748,15 +1749,13 @@ static int clk_pllu_tegra114_enable(struct clk_hw *hw) return -EINVAL; } - if (clk_pll_is_enabled(hw)) - return 0; - input_rate = clk_hw_get_rate(__clk_get_hw(osc)); if (pll->lock) spin_lock_irqsave(pll->lock, flags); - _clk_pll_enable(hw); + if (!clk_pll_is_enabled(hw)) + _clk_pll_enable(hw); ret = clk_pll_wait_for_lock(pll); if (ret < 0) -- 2.29.2
[PATCH v2 4/5] clk: tegra: Halve SCLK rate on Tegra20
Higher SCLK rates on Tegra20 require high core voltage. The higher
clock rate may have a positive performance effect only for AHB DMA
transfers and AVP CPU, but both aren't used by upstream kernel at all.
Halve SCLK rate on Tegra20 in order to remove the high core voltage
requirement.
Signed-off-by: Dmitry Osipenko
---
drivers/clk/tegra/clk-tegra20.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 3efc651b42e3..3664593a5ba4 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1021,9 +1021,9 @@ static struct tegra_clk_init_table init_table[]
__initdata = {
{ TEGRA20_CLK_PLL_P_OUT3, TEGRA20_CLK_CLK_MAX, 7200, 1 },
{ TEGRA20_CLK_PLL_P_OUT4, TEGRA20_CLK_CLK_MAX, 2400, 1 },
{ TEGRA20_CLK_PLL_C, TEGRA20_CLK_CLK_MAX, 6, 0 },
- { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 24000, 0 },
- { TEGRA20_CLK_SCLK, TEGRA20_CLK_PLL_C_OUT1, 24000, 0 },
- { TEGRA20_CLK_HCLK, TEGRA20_CLK_CLK_MAX, 24000, 0 },
+ { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 12000, 0 },
+ { TEGRA20_CLK_SCLK, TEGRA20_CLK_PLL_C_OUT1, 12000, 0 },
+ { TEGRA20_CLK_HCLK, TEGRA20_CLK_CLK_MAX, 12000, 0 },
{ TEGRA20_CLK_PCLK, TEGRA20_CLK_CLK_MAX, 6000, 0 },
{ TEGRA20_CLK_CSITE, TEGRA20_CLK_CLK_MAX, 0, 1 },
{ TEGRA20_CLK_CCLK, TEGRA20_CLK_CLK_MAX, 0, 1 },
--
2.29.2
[PATCH v2 1/5] clk: tegra30: Use 300MHz for video decoder by default
The 600MHz is a too high clock rate for some SoC versions for the video
decoder hardware and this may cause stability issues. Use 300MHz for the
video decoder by default, which is supported by all hardware versions.
Fixes: ed1a2459e20c ("clk: tegra: Add Tegra20/30 EMC clock implementation")
Signed-off-by: Dmitry Osipenko
---
drivers/clk/tegra/clk-tegra30.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 37244a7e68c2..98923c4674bf 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1248,7 +1248,7 @@ static struct tegra_clk_init_table init_table[]
__initdata = {
{ TEGRA30_CLK_GR3D, TEGRA30_CLK_PLL_C, 3, 0 },
{ TEGRA30_CLK_GR3D2, TEGRA30_CLK_PLL_C, 3, 0 },
{ TEGRA30_CLK_PLL_U, TEGRA30_CLK_CLK_MAX, 48000, 0 },
- { TEGRA30_CLK_VDE, TEGRA30_CLK_PLL_C, 6, 0 },
+ { TEGRA30_CLK_VDE, TEGRA30_CLK_PLL_C, 3, 0 },
{ TEGRA30_CLK_SPDIF_IN_SYNC, TEGRA30_CLK_CLK_MAX, 2400, 0 },
{ TEGRA30_CLK_I2S0_SYNC, TEGRA30_CLK_CLK_MAX, 2400, 0 },
{ TEGRA30_CLK_I2S1_SYNC, TEGRA30_CLK_CLK_MAX, 2400, 0 },
--
2.29.2
[PATCH v2 2/5] clk: tegra: Fix refcounting of gate clocks
The refcounting of the gate clocks has a bug causing the enable_refcnt
to underflow when unused clocks are disabled. This happens because clk
provider erroneously bumps the refcount if clock is enabled at a boot
time, which it shouldn't be doing, and it does this only for the gate
clocks, while peripheral clocks are using the same gate ops and the
peripheral clocks are missing the initial bump. Hence the refcount of
the peripheral clocks is 0 when unused clocks are disabled and then the
counter is decremented further by the gate ops, causing the integer
underflow.
Fix this problem by removing the erroneous bump and by implementing the
disable_unused() callback, which disables the unused gates properly.
The visible effect of the bug is such that the unused clocks are never
gated if a loaded kernel module grabs the unused clocks and starts to use
them. In practice this shouldn't cause any real problems for the drivers
and boards supported by the kernel today.
Signed-off-by: Dmitry Osipenko
---
drivers/clk/tegra/clk-periph-gate.c | 72 +++--
drivers/clk/tegra/clk-periph.c | 11 +
2 files changed, 58 insertions(+), 25 deletions(-)
diff --git a/drivers/clk/tegra/clk-periph-gate.c
b/drivers/clk/tegra/clk-periph-gate.c
index 4b31beefc9fc..3c4259fec82e 100644
--- a/drivers/clk/tegra/clk-periph-gate.c
+++ b/drivers/clk/tegra/clk-periph-gate.c
@@ -48,18 +48,9 @@ static int clk_periph_is_enabled(struct clk_hw *hw)
return state;
}
-static int clk_periph_enable(struct clk_hw *hw)
+static void clk_periph_enable_locked(struct clk_hw *hw)
{
struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw);
- unsigned long flags = 0;
-
- spin_lock_irqsave(&periph_ref_lock, flags);
-
- gate->enable_refcnt[gate->clk_num]++;
- if (gate->enable_refcnt[gate->clk_num] > 1) {
- spin_unlock_irqrestore(&periph_ref_lock, flags);
- return 0;
- }
write_enb_set(periph_clk_to_bit(gate), gate);
udelay(2);
@@ -78,6 +69,32 @@ static int clk_periph_enable(struct clk_hw *hw)
udelay(1);
writel_relaxed(0, gate->clk_base + LVL2_CLK_GATE_OVRE);
}
+}
+
+static void clk_periph_disable_locked(struct clk_hw *hw)
+{
+ struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw);
+
+ /*
+* If peripheral is in the APB bus then read the APB bus to
+* flush the write operation in apb bus. This will avoid the
+* peripheral access after disabling clock
+*/
+ if (gate->flags & TEGRA_PERIPH_ON_APB)
+ tegra_read_chipid();
+
+ write_enb_clr(periph_clk_to_bit(gate), gate);
+}
+
+static int clk_periph_enable(struct clk_hw *hw)
+{
+ struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw);
+ unsigned long flags = 0;
+
+ spin_lock_irqsave(&periph_ref_lock, flags);
+
+ if (!gate->enable_refcnt[gate->clk_num]++)
+ clk_periph_enable_locked(hw);
spin_unlock_irqrestore(&periph_ref_lock, flags);
@@ -91,21 +108,28 @@ static void clk_periph_disable(struct clk_hw *hw)
spin_lock_irqsave(&periph_ref_lock, flags);
- gate->enable_refcnt[gate->clk_num]--;
- if (gate->enable_refcnt[gate->clk_num] > 0) {
- spin_unlock_irqrestore(&periph_ref_lock, flags);
- return;
- }
+ WARN_ON(!gate->enable_refcnt[gate->clk_num]);
+
+ if (gate->enable_refcnt[gate->clk_num]-- == 1)
+ clk_periph_disable_locked(hw);
+
+ spin_unlock_irqrestore(&periph_ref_lock, flags);
+}
+
+static void clk_periph_disable_unused(struct clk_hw *hw)
+{
+ struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw);
+ unsigned long flags = 0;
+
+ spin_lock_irqsave(&periph_ref_lock, flags);
/*
-* If peripheral is in the APB bus then read the APB bus to
-* flush the write operation in apb bus. This will avoid the
-* peripheral access after disabling clock
+* Some clocks are duplicated and some of them are marked as critical,
+* like fuse and fuse_burn for example, thus the enable_refcnt will
+* be non-zero here id the "unused" duplicate is disabled by CCF.
*/
- if (gate->flags & TEGRA_PERIPH_ON_APB)
- tegra_read_chipid();
-
- write_enb_clr(periph_clk_to_bit(gate), gate);
+ if (!gate->enable_refcnt[gate->clk_num])
+ clk_periph_disable_locked(hw);
spin_unlock_irqrestore(&periph_ref_lock, flags);
}
@@ -114,6 +138,7 @@ const struct clk_ops tegra_clk_periph_gate_ops = {
.is_enabled = clk_periph_is_enabled,
.enable = clk_periph_enable,
.disable = clk_periph_disable,
+ .disable_unused = clk_periph_disable_unused,
};
struct clk *tegra_clk_regis
[PATCH v2 0/5] Couple improvements for Tegra clk driver
This series fixes couple minor problems of the Tegra clk driver. Please review and apply. Changelog: v2: - Added these new patches: clk: tegra: Halve SCLK rate on Tegra20 MAINTAINERS: Hand Tegra clk driver to Jon and Thierry v1: - Collected clk patches into a single series. Dmitry Osipenko (5): clk: tegra30: Use 300MHz for video decoder by default clk: tegra: Fix refcounting of gate clocks clk: tegra: Ensure that PLLU configuration is applied properly clk: tegra: Halve SCLK rate on Tegra20 MAINTAINERS: Hand Tegra clk driver to Jon and Thierry CREDITS | 6 +++ MAINTAINERS | 4 +- drivers/clk/tegra/clk-periph-gate.c | 72 +++-- drivers/clk/tegra/clk-periph.c | 11 + drivers/clk/tegra/clk-pll.c | 9 ++-- drivers/clk/tegra/clk-tegra20.c | 6 +-- drivers/clk/tegra/clk-tegra30.c | 2 +- 7 files changed, 74 insertions(+), 36 deletions(-) -- 2.29.2
[PATCH v2 5/5] MAINTAINERS: Hand Tegra clk driver to Jon and Thierry
Peter and Prashant aren't actively maintaining Tegra clock driver anymore. Jonathan and Thierry will pick up maintaining of the driver from now on. Signed-off-by: Dmitry Osipenko --- CREDITS | 6 ++ MAINTAINERS | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CREDITS b/CREDITS index 090ed4b004a5..aa4ac5810afc 100644 --- a/CREDITS +++ b/CREDITS @@ -1246,6 +1246,10 @@ S: 12 Shraga Raphaeli S: Petah-Tikva, 4906418 S: Israel +N: Prashant Gaikwad +E: pgaik...@nvidia.com +D: Maintained NVIDIA Tegra clock driver + N: Kumar Gala E: ga...@kernel.crashing.org D: Embedded PowerPC 6xx/7xx/74xx/82xx/83xx/85xx support @@ -3374,7 +3378,9 @@ E: D: Macintosh IDE Driver N: Peter De Schrijver +E: pdeschrij...@nvidia.com E: stu...@cc4.kuleuven.ac.be +D: Maintained NVIDIA Tegra clock driver D: Mitsumi CD-ROM driver patches March version S: Molenbaan 29 S: B2240 Zandhoven diff --git a/MAINTAINERS b/MAINTAINERS index ad9abb42f852..b2132b2e00d9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17525,8 +17525,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git N: [^a-z]tegra TEGRA CLOCK DRIVER -M: Peter De Schrijver -M: Prashant Gaikwad +M: Jonathan Hunter +M: Thierry Reding S: Supported F: drivers/clk/tegra/ -- 2.29.2
Re: [PATCH RESEND v8 0/4] input: elants: Support Asus TF300T and Nexus 7 touchscreens
09.11.2020 20:28, Michał Mirosław пишет: > This series cleans up the driver a bit and implements changes needed to > support EKTF3624-based touchscreen used in Asus TF300T, Google Nexus 7 > and similar Tegra3-based tablets. > > --- > v2: extended with Dmitry's patches (replaced v1 patches 3 and 4) > v3: rebased for v5.7-rc1 > v4: rebased onto v5.7-rc2+ (current Linus' master) > update "remove unused axes" and "refactor > elants_i2c_execute_command()" patches after review > add David's patch converting DT binding to YAML > v5: rebased onto dtor/input/for-linus > v6: rebased onto newer dtor/input/for-linus > remove yet unused constants from patch 1 > added a new drive-by cleanup (last patch) > v7: rebased onto current dtor/input/for-next > v8: rebased onto current dtor/input/for-linus > --- > > Dmitry Osipenko (1): > input: elants: support 0x66 reply opcode for reporting touches > > Michał Mirosław (3): > input: elants: document some registers and values > input: elants: support old touch report format > input: elants: read touchscreen size for EKTF3624 > > drivers/input/touchscreen/elants_i2c.c | 149 + > 1 file changed, 127 insertions(+), 22 deletions(-) > This patchset missed another kernel release cycle and touchscreen hardware remains unusable on Nexus 7 [1] and other Asus devices. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tegra30-asus-nexus7-grouper-common.dtsi?h=v5.10-rc7#n845 Dmitry Torokhov, could you please take a look at the v8 and let us know whether it's good already or something needs to be improved?
Re: [PATCH 1/2] of: property: Get rid of code duplication in port getting
10.12.2020 23:29, Sam Protsenko пишет:
> Both of_graph_is_present() and of_graph_get_next_endpoint() functions
> share common piece of code for obtaining the graph port. Extract it into
> separate static function to get rid of code duplication and avoid
> possible coding errors in future.
>
> Fixes: 4ec0a44ba8d7 ("of_graph: add of_graph_is_present()")
The "fixes" tag should be used only for bug-fixes and there is no bug
fixed in this patch.
> Signed-off-by: Sam Protsenko
> ---
> drivers/of/property.c | 34 --
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 408a7b5f06a9..da111fcf37ac 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -30,13 +30,13 @@
> #include "of_private.h"
>
> /**
> - * of_graph_is_present() - check graph's presence
> + * of_graph_get_port - find the "port" node in a given node
> * @node: pointer to device_node containing graph port
> *
> - * Return: True if @node has a port or ports (with a port) sub-node,
> - * false otherwise.
> + * Return: A 'port' node pointer with refcount incremented if found or NULL
> + * otherwise. The caller has to use of_node_put() on it when done.
> */
> -bool of_graph_is_present(const struct device_node *node)
> +static struct device_node *of_graph_get_port(const struct device_node *node)
> {
> struct device_node *ports, *port;
>
> @@ -46,8 +46,22 @@ bool of_graph_is_present(const struct device_node *node)
>
> port = of_get_child_by_name(node, "port");
> of_node_put(ports);
> - of_node_put(port);
>
> + return port;
> +}
> +
> +/**
> + * of_graph_is_present() - check graph's presence
> + * @node: pointer to device_node containing graph port
> + *
> + * Return: True if @node has a port or ports (with a port) sub-node,
> + * false otherwise.
> + */
> +bool of_graph_is_present(const struct device_node *node)
> +{
> + struct device_node *port = of_graph_get_port(node);
> +
> + of_node_put(port);
> return !!port;
> }
> EXPORT_SYMBOL(of_graph_is_present);
> @@ -631,15 +645,7 @@ struct device_node *of_graph_get_next_endpoint(const
> struct device_node *parent,
>* parent port node.
>*/
> if (!prev) {
> - struct device_node *node;
> -
> - node = of_get_child_by_name(parent, "ports");
> - if (node)
> - parent = node;
> -
> - port = of_get_child_by_name(parent, "port");
> - of_node_put(node);
> -
> + port = of_graph_get_port(parent);
> if (!port) {
> pr_err("graph: no port node found in %pOF\n", parent);
> return NULL;
>
This repeats the problem which was made once before:
https://lore.kernel.org/patchwork/patch/1266028/#1461493
Re: [PATCH RESEND v8 2/4] input: elants: support old touch report format
11.12.2020 19:09, Michał Mirosław пишет:
> On Thu, Dec 10, 2020 at 11:29:40PM -0800, Dmitry Torokhov wrote:
>> Hi Michał,
>> On Fri, Dec 11, 2020 at 07:53:56AM +0100, Michał Mirosław wrote:
>>> @@ -998,17 +1011,18 @@ static irqreturn_t elants_i2c_irq(int irq, void
>>> *_dev)
>>> }
>>>
>>> report_len = ts->buf[FW_HDR_LENGTH] / report_count;
>>> - if (report_len != PACKET_SIZE) {
>>> + if (report_len != PACKET_SIZE &&
>>> + report_len != PACKET_SIZE_OLD) {
>>> dev_err(&client->dev,
>>> - "mismatching report length: %*ph\n",
>>> + "unsupported report length: %*ph\n",
>>> HEADER_SIZE, ts->buf);
>> Do I understand this correctly that the old packets are only observed on
>> EKTF3624? If so can we expand the check so that we only accept packets
>> with "old" size when we know we are dealing with this device?
>
> We only have EKTF3624 and can't be sure there are no other chips needing this.
In practice this older packet format should be seen only on 3624, but
nevertheless we could make it more explicit by adding the extra chip_id
checks.
It won't be difficult to change it in the future if will be needed.
I think the main point that Dmitry Torokhov conveys here is that we
should minimize the possible impact on the current EKT3500 code since we
don't have definitive answers regarding the firmware differences among
the hardware variants.
Re: [PATCH v3 0/3] Support wakeup methods of Atmel maXTouch controllers
Hello, 12.12.2020 05:43, Dmitry Torokhov пишет: > Hi Dmitry, > > On Mon, Dec 07, 2020 at 12:22:14AM +0300, Dmitry Osipenko wrote: >> Some Atmel maXTouch controllers, like mXT1386 and mXT3432S1 for example, >> have a WAKE line that needs to be asserted in order to wake controller >> from a deep sleep, otherwise it will be unusable. This series implements >> support for the wakeup methods in accordance to the mXT1386 datasheet [1], >> see page 29 (chapter "5.8 WAKE Line"). >> >> The mXT1386 is a widely used controller found on many older Android tablet >> devices. Touchscreen on Acer A500 tablet now works properly after this >> series. > > I am trying to understand how your controller is configured on that > system. Could you please enable all debug messages in the driver and > post the logs? I am a bit confused why the controller needs to be woken > up twice in mxt_start() given that according to the spec it is supposed > to stay up for 2 seconds after successful I2C transfer... >From the page 30 in the datasheet: "Note that when the mXT1386 is sent into deep sleep mode, it goes to sleep immediately. In this case the two-second timeout does not apply until the WAKE pin is asserted." The debug log seems confirm that quote: ... [ 1.196404] Family: 160 Variant: 0 Firmware V1.0.AA Objects: 18 [ 1.196572] T37 Start:118 Size:130 Instances:1 Report IDs:0-0 [ 1.196586] T44 Start:248 Size:1 Instances:1 Report IDs:0-0 [ 1.196597] T5 Start:249 Size:9 Instances:1 Report IDs:0-0 [ 1.196608] T6 Start:258 Size:6 Instances:1 Report IDs:1-1 [ 1.196617] T38 Start:264 Size:64 Instances:1 Report IDs:0-0 [ 1.196628] T7 Start:328 Size:3 Instances:1 Report IDs:0-0 [ 1.196638] T8 Start:331 Size:10 Instances:1 Report IDs:0-0 [ 1.196648] T9 Start:341 Size:34 Instances:1 Report IDs:2-17 [ 1.196658] T15 Start:375 Size:11 Instances:2 Report IDs:18-19 [ 1.196668] T18 Start:397 Size:2 Instances:1 Report IDs:0-0 [ 1.196678] T22 Start:399 Size:17 Instances:1 Report IDs:20-20 [ 1.196688] T24 Start:416 Size:19 Instances:1 Report IDs:21-24 [ 1.196698] T25 Start:435 Size:14 Instances:1 Report IDs:25-25 [ 1.196708] T27 Start:449 Size:7 Instances:1 Report IDs:26-26 [ 1.196718] T28 Start:456 Size:6 Instances:1 Report IDs:27-27 [ 1.196728] T40 Start:462 Size:5 Instances:1 Report IDs:0-0 [ 1.196738] T41 Start:467 Size:6 Instances:1 Report IDs:0-0 [ 1.196748] T43 Start:473 Size:6 Instances:1 Report IDs:0-0 [ 1.196852] Direct firmware load for maxtouch.cfg failed with error -2 [ 1.197305] T6 Config Checksum: 0x8D7459 [ 1.197318] T6 Status 0x90 RESET CAL [ 1.197543] Initialized power cfg: ACTV 10, IDLE 50 [ 1.198387] Touchscreen size X1279Y799 ... [ 1.211686] T6 Status 0x00 OK ... [15.576573] Set T7 ACTV:10 IDLE:50 [15.592142] T6 Status 0x10 CAL [15.597920] T6 Status 0x00 OK [15.604846] Set T7 ACTV:0 IDLE:0 [15.831477] waking up controller [15.862912] Set T7 ACTV:10 IDLE:50 [15.872783] Set T7 ACTV:0 IDLE:0 [15.880333] T6 Status 0x10 CAL [15.946853] Set T7 ACTV:10 IDLE:50 [15.956582] Set T7 ACTV:0 IDLE:0 ... [27.897337] waking up controller [27.924172] Set T7 ACTV:10 IDLE:50 [27.947286] T6 Status 0x00 OK [27.959754] Set T7 ACTV:0 IDLE:0 [27.970258] Set T7 ACTV:10 IDLE:50 [27.987546] T6 Status 0x10 CAL [27.993327] T6 Status 0x00 OK
