Re: [PATCH -next] staging: nvec: minor coding style fix
On Fri, Feb 12, 2021 at 10:34:23AM +0300, Fatih Yildirim wrote: > Fix for the below coding style warning. > Warning: Move const after static - use 'static const int' > > Signed-off-by: Fatih Yildirim > --- > drivers/staging/nvec/nvec_power.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
On Fri, Mar 12, 2021 at 01:24:54PM +0100, Nicolas Saenz Julienne wrote: > Adds support to control the PWM bus available in official Raspberry Pi > PoE HAT. Only RPi's co-processor has access to it, so commands have to > be sent through RPi's firmware mailbox interface. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > Changes since v7: > - Remove unwarranted RPI_PWM_DEF_DUTY_REG usage > > Changes since v6: > - Use %pe > - Round divisions properly > - Use dev_err_probe() > - Pass check_patch > > Changes since v3: > - Rename compatible string to be more explicit WRT to bus's limitations > > Changes since v2: > - Use devm_rpi_firmware_get() > - Rename driver > - Small cleanups > > Changes since v1: > - Use default pwm bindings and get rid of xlate() function > - Correct spelling errors > - Correct apply() function > - Round values > - Fix divisions in arm32 mode > - Small cleanups > > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi-poe.c | 206 ++++++++++ > 3 files changed, 216 insertions(+) > create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c Acked-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
On Thu, Nov 05, 2020 at 02:44:08AM +0300, Dmitry Osipenko wrote:
> Add OPP and SoC core voltage scaling support to the display controller
> driver. This is required for enabling system-wide DVFS on older Tegra
> SoCs.
>
> Tested-by: Peter Geis
> Tested-by: Nicolas Chauvet
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/tegra/Kconfig | 1 +
> drivers/gpu/drm/tegra/dc.c| 138 +-
> drivers/gpu/drm/tegra/dc.h| 5 ++
> 3 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 1650a448eabd..9eec4c3fbd3b 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -12,6 +12,7 @@ config DRM_TEGRA
> select INTERCONNECT
> select IOMMU_IOVA
> select CEC_CORE if CEC_NOTIFIER
> + select PM_OPP
> help
> Choose this option if you have an NVIDIA Tegra SoC.
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index fd7c8828652d..babcb66a335b 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -11,9 +11,13 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
>
> +#include
> +#include
> #include
>
> #include
> @@ -1699,6 +1703,55 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> return 0;
> }
>
> +static void tegra_dc_update_voltage_state(struct tegra_dc *dc,
> + struct tegra_dc_state *state)
> +{
> + struct dev_pm_opp *opp;
> + unsigned long rate;
> + int err, min_uV;
> +
> + /* OPP usage is optional */
> + if (!dc->opp_table)
> + return;
> +
> + /* calculate actual pixel clock rate which depends on internal divider
> */
> + rate = DIV_ROUND_UP(clk_get_rate(dc->clk) * 2, state->div + 2);
> +
> + /* find suitable OPP for the rate */
> + opp = dev_pm_opp_find_freq_ceil(dc->dev, &rate);
> +
> + if (opp == ERR_PTR(-ERANGE))
> + opp = dev_pm_opp_find_freq_floor(dc->dev, &rate);
> +
> + if (IS_ERR(opp)) {
> + dev_err(dc->dev, "failed to find OPP for %lu Hz: %ld\n",
> + rate, PTR_ERR(opp));
> + return;
> + }
> +
> + min_uV = dev_pm_opp_get_voltage(opp);
> + dev_pm_opp_put(opp);
> +
> + /*
> + * Voltage scaling is optional and trying to set voltage for a dummy
> + * regulator will error out.
> + */
> + if (!device_property_present(dc->dev, "core-supply"))
> + return;
This is a potentially heavy operation, so I think we should avoid that
here. How about you use devm_regulator_get_optional() in ->probe()? That
returns -ENODEV if no regulator was specified, in which case you can set
dc->core_reg = NULL and use that as the condition here.
> +
> + /*
> + * Note that the minimum core voltage depends on the pixel clock
> + * rate (which depends on internal clock divider of CRTC) and not on
> + * the rate of the display controller clock. This is why we're not
> + * using dev_pm_opp_set_rate() API and instead are managing the
> + * voltage by ourselves.
> + */
> + err = regulator_set_voltage(dc->core_reg, min_uV, INT_MAX);
> + if (err)
> + dev_err(dc->dev, "failed to set CORE voltage to %duV: %d\n",
> + min_uV, err);
> +}
Also, I'd prefer if the flow here was more linear, such as:
if (dc->core_reg) {
err = regulator_set_voltage(...);
...
}
> +
> static void tegra_dc_commit_state(struct tegra_dc *dc,
> struct tegra_dc_state *state)
> {
> @@ -1738,6 +1791,8 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
> if (err < 0)
> dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
> dc->clk, state->pclk, err);
> +
> + tegra_dc_update_voltage_state(dc, state);
> }
>
> static void tegra_dc_stop(struct tegra_dc *dc)
> @@ -2521,6 +2576,7 @@ static int tegra_dc_runtime_suspend(struct
> host1x_client *client)
>
> clk_disable_unprepare(dc->clk);
> pm_runtime_put_sync(dev);
> + regulator_disable(dc->core_reg);
>
> return 0;
> }
> @@ -2531,10 +2587,16 @@ static int tegra_dc_runtime_resume(struct
> host1x_client *client)
> struct device *dev = client->dev;
> int err;
>
> + err = regulator_enable(dc->core_reg);
> + if (err < 0) {
> + dev_err(dev, "failed to enable CORE regulator: %d\n", err);
> + return err;
> + }
> +
> err = pm_runtime_get_sync(dev);
> if (err < 0) {
> dev_err(dev, "failed to get runtime PM: %d\n", err);
> - return err;
> + goto disable_regulator;
> }
>
> if (dc->soc->has_powergate) {
> @@ -2564,6 +2626,9 @@ static int tegra_dc_runtime_resume(struct host1x_client
> *cl
Re: [PATCH v1 07/30] soc/tegra: Add sync state API
On Thu, Nov 05, 2020 at 02:44:04AM +0300, Dmitry Osipenko wrote:
> Introduce sync state API that will be used by Tegra device drivers. This
> new API is primarily needed for syncing state of SoC devices that are left
> ON after bootloader or permanently enabled. All these devices belong to a
> shared CORE voltage domain, and thus, we needed to bring all the devices
> into expected state before the voltage scaling could be performed.
>
> All drivers of DVFS-critical devices shall sync theirs the state before
> Tegra's voltage regulator coupler will be allowed to perform a system-wide
> voltage scaling.
>
> Tested-by: Peter Geis
> Tested-by: Nicolas Chauvet
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/soc/tegra/common.c | 152 -
> include/soc/tegra/common.h | 22 ++
> 2 files changed, 170 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index 3dc54f59cafe..f9b2b6f57887 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -3,13 +3,52 @@
> * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
> */
>
> +#define dev_fmt(fmt) "%s: " fmt, __func__
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include
> +#include
> +#include
> #include
> +#include
>
> #include
>
> +#define terga_soc_for_each_device(__dev) \
tegra_soc_for_each_device
> + for ((__dev) = tegra_soc_devices; (__dev) && (__dev)->compatible; \
> + (__dev)++)
> +
> +struct tegra_soc_device {
> + const char *compatible;
> + const bool dvfs_critical;
> + unsigned int sync_count;
> +};
> +
> +static DEFINE_MUTEX(tegra_soc_lock);
> +static struct tegra_soc_device *tegra_soc_devices;
> +
> +/*
> + * DVFS-critical devices are either active at a boot time or permanently
> + * active, like EMC for example. System-wide DVFS should be deferred until
> + * drivers of the critical devices synced theirs state.
> + */
> +
> +static struct tegra_soc_device tegra20_soc_devices[] = {
> + { .compatible = "nvidia,tegra20-dc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra20-emc", .dvfs_critical = true, },
> + { }
> +};
> +
> +static struct tegra_soc_device tegra30_soc_devices[] = {
> + { .compatible = "nvidia,tegra30-dc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra30-emc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra30-pwm", .dvfs_critical = true, },
> + { }
> +};
> +
> static const struct of_device_id tegra_machine_match[] = {
> - { .compatible = "nvidia,tegra20", },
> - { .compatible = "nvidia,tegra30", },
> + { .compatible = "nvidia,tegra20", .data = tegra20_soc_devices, },
> + { .compatible = "nvidia,tegra30", .data = tegra30_soc_devices, },
> { .compatible = "nvidia,tegra114", },
> { .compatible = "nvidia,tegra124", },
> { .compatible = "nvidia,tegra132", },
> @@ -17,7 +56,7 @@ static const struct of_device_id tegra_machine_match[] = {
> { }
> };
>
> -bool soc_is_tegra(void)
> +static const struct of_device_id *tegra_soc_of_match(void)
> {
> const struct of_device_id *match;
> struct device_node *root;
> @@ -29,5 +68,110 @@ bool soc_is_tegra(void)
> match = of_match_node(tegra_machine_match, root);
> of_node_put(root);
>
> - return match != NULL;
> + return match;
> +}
> +
> +bool soc_is_tegra(void)
> +{
> + return tegra_soc_of_match() != NULL;
> +}
> +
> +void tegra_soc_device_sync_state(struct device *dev)
> +{
> + struct tegra_soc_device *soc_dev;
> +
> + mutex_lock(&tegra_soc_lock);
> + terga_soc_for_each_device(soc_dev) {
tegra_soc_for_each_device
> + if (!of_device_is_compatible(dev->of_node, soc_dev->compatible))
> + continue;
> +
> + if (!soc_dev->sync_count) {
> + dev_err(dev, "already synced\n");
> + break;
> + }
> +
> + /*
> + * All DVFS-capable devices should have the CORE regulator
> + * phandle. Older device-trees don't have it, hence state
> + * won't be synced for the older DTBs, allowing them to work
> + * properly.
> + */
> + if (soc_dev->dvfs_critical &&
> + !device_property_present(dev, "core-supply")) {
> + dev_dbg(dev, "doesn't have core supply\n");
> + break;
> + }
> +
> + soc_dev->sync_count--;
> + dev_dbg(dev, "sync_count=%u\n", soc_dev->sync_count);
> + break;
> + }
> + mutex_unlock(&tegra_soc_lock);
> +}
> +EXPORT_SYMBOL_GPL(tegra_soc_device_sync_state);
> +
> +bool tegra_soc_dvfs_state_synced(void)
> +{
> + struct tegra_soc_device *soc_dev;
> + bool synced_state = true;
> +
> + /*
> + * CORE voltage scaling is limited until drivers of the critical
> + * devices synced
Re: [PATCH v1 18/30] pwm: tegra: Support OPP and core voltage scaling
On Thu, Nov 05, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
[...]
> +static void tegra_pwm_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_tegra_pwm_init_opp_table(struct device *dev)
> +{
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + int err;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> + "failed to prepare OPP table\n");
> +
> + /*
> + * OPP table presence is optional and we want the set_rate() of OPP
> + * API to work similarly to clk_set_rate() if table is missing in a
> + * device-tree. The add_table() errors out if OPP is missing in DT.
> + */
> + if (device_property_present(dev, "operating-points-v2")) {
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> + }
> +
> + err = devm_add_action(dev, tegra_pwm_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);
> +
> + return err;
> +}
These two functions seem to be heavily boilerplate across all these
drivers. Have you considered splitting these out into separate helpers?
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On Thu, Nov 12, 2020 at 10:57:27PM +0300, Dmitry Osipenko wrote: > 11.11.2020 14:38, Ulf Hansson пишет: > > On Sun, 8 Nov 2020 at 13:19, Dmitry Osipenko wrote: > >> > >> 05.11.2020 18:22, Dmitry Osipenko пишет: > >>> 05.11.2020 12:45, Ulf Hansson пишет: > >>> ... > I need some more time to review this, but just a quick check found a > few potential issues... > >>> > >>> Thank you for starting the review! I'm pretty sure it will take a couple > >>> revisions until all the questions will be resolved :) > >>> > The "core-supply", that you specify as a regulator for each > controller's device node, is not the way we describe power domains. > Instead, it seems like you should register a power-domain provider > (with the help of genpd) and implement the ->set_performance_state() > callback for it. Each device node should then be hooked up to this > power-domain, rather than to a "core-supply". For DT bindings, please > have a look at Documentation/devicetree/bindings/power/power-domain.yaml > and Documentation/devicetree/bindings/power/power_domain.txt. > > In regards to the "sync state" problem (preventing to change > performance states until all consumers have been attached), this can > then be managed by the genpd provider driver instead. > >>> > >>> I'll need to take a closer look at GENPD, thank you for the suggestion. > >>> > >>> Sounds like a software GENPD driver which manages clocks and voltages > >>> could be a good idea, but it also could be an unnecessary > >>> over-engineering. Let's see.. > >>> > >> > >> Hello Ulf and all, > >> > >> I took a detailed look at the GENPD and tried to implement it. Here is > >> what was found: > >> > >> 1. GENPD framework doesn't aggregate performance requests from the > >> attached devices. This means that if deviceA requests performance state > >> 10 and then deviceB requests state 3, then framework will set domain's > >> state to 3 instead of 10. > >> > >> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L376 > > > > As Viresh also stated, genpd does aggregate the votes. It even > > performs aggregation hierarchy (a genpd is allowed to have parent(s) > > to model a topology). > > Yes, I already found and fixed the bug which confused me previously and > it's working well now. > > >> 2. GENPD framework has a sync() callback in the genpd.domain structure, > >> but this callback isn't allowed to be used by the GENPD implementation. > >> The GENPD framework always overrides that callback for its own needs. > >> Hence GENPD doesn't allow to solve the bootstrapping > >> state-synchronization problem in a nice way. > >> > >> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L2606 > > > > That ->sync() callback isn't the callback you are looking for, it's a > > PM domain specific callback - and has other purposes. > > > > To solve the problem you refer to, your genpd provider driver (a > > platform driver) should assign its ->sync_state() callback. The > > ->sync_state() callback will be invoked, when all consumer devices > > have been attached (and probed) to their corresponding provider. > > > > You may have a look at drivers/cpuidle/cpuidle-psci-domain.c, to see > > an example of how this works. If there is anything unclear, just tell > > me and I will try to help. > > Indeed, thank you for the clarification. This variant works well. > > >> 3. Tegra doesn't have a dedicated hardware power-controller for the core > >> domain, instead there is only an external voltage regulator. Hence we > >> will need to create a phony device-tree node for the virtual power > >> domain, which is probably a wrong thing to do. > > > > No, this is absolutely the correct thing to do. > > > > This isn't a virtual power domain, it's a real power domain. You only > > happen to model the control of it as a regulator, as it fits nicely > > with that for *this* SoC. Don't get me wrong, that's fine as long as > > the supply is specified only in the power-domain provider node. > > > > On another SoC, you might have a different FW interface for the power > > domain provider that doesn't fit well with the regulator. When that > > happens, all you need to do is to implement a new power domain > > provider and potentially re-define the power domain topology. More > > importantly, you don't need to re-invent yet another slew of device > > specific bindings - for each SoC. > > > >> > >> === > >> > >> Perhaps it should be possible to create some hacks to work around > >> bullets 2 and 3 in order to achieve what we need for DVFS on Tegra, but > >> bullet 1 isn't solvable without changing how the GENPD core works. > >> > >> Altogether, the GENPD in its current form is a wrong abstraction for a > >> system-wide DVFS in a case where multiple devices share power domain and > >> this domain is a voltage regulator. The regulator framework is the > >> correct abstraction in this
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On Fri, Nov 13, 2020 at 01:14:45AM +0300, Dmitry Osipenko wrote: > 12.11.2020 23:43, Thierry Reding пишет: > >> The difference in comparison to using voltage regulator directly is > >> minimal, basically the core-supply phandle is replaced is replaced with > >> a power-domain phandle in a device tree. > > These new power-domain handles would have to be added to devices that > > potentially already have a power-domain handle, right? Isn't that going > > to cause issues? I vaguely recall that we already have multiple power > > domains for the XUSB controller and we have to jump through extra hoops > > to make that work. > > I modeled the core PD as a parent of the PMC sub-domains, which > presumably is a correct way to represent the domains topology. > > https://gist.github.com/digetx/dfd92c7f7e0aa6cef20403c4298088d7 > > >> The only thing which makes me feel a bit uncomfortable is that there is > >> no real hardware node for the power domain node in a device-tree. > > Could we anchor the new power domain at the PMC for example? That would > > allow us to avoid the "virtual" node. > > I had a thought about using PMC for the core domain, but not sure > whether it will be an entirely correct hardware description. Although, > it will be nice to have it this way. > > This is what Tegra TRM says about PMC: > > "The Power Management Controller (PMC) block interacts with an external > or Power Manager Unit (PMU). The PMC mostly controls the entry and exit > of the system from different sleep modes. It provides power-gating > controllers for SOC and CPU power-islands and also provides scratch > storage to save some of the context during sleep modes (when CPU and/or > SOC power rails are off). Additionally, PMC interacts with the external > Power Manager Unit (PMU)." > > The core voltage regulator is a part of the PMU. > > Not all core SoC devices are behind PMC, IIUC. There are usually some SoC devices that are always-on. Things like the RTC for example, can never be power-gated, as far as I recall. On newer chips there are usually many more blocks that can't be powergated at all. > > On the other hand, if we were to > > use a regulator, we'd be adding a node for that, right? So isn't this > > effectively going to be the same node if we use a power domain? Both > > software constructs are using the same voltage regulator, so they should > > be able to be described by the same device tree node, shouldn't they? > > I'm not exactly sure what you're meaning by "use a regulator" and "we'd > be adding a node for that", could you please clarify? This v1 approach > uses a core-supply phandle (i.e. regulator is used), it doesn't require > extra nodes. What I meant to say was that the actual supply voltage is generated by some device (typically one of the SD outputs of the PMIC). Whether we model this as a power domain or a regulator doesn't really matter, right? So I'm wondering if the device that generates the voltage should be the power domain provider, just like it is the provider of the regulator if this was modelled as a regulator. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
On Fri, Nov 13, 2020 at 08:13:49PM +0300, Dmitry Osipenko wrote: > 13.11.2020 19:15, Mark Brown пишет: > > On Fri, Nov 13, 2020 at 06:55:27PM +0300, Dmitry Osipenko wrote: > >> 13.11.2020 17:29, Mark Brown пишет: > > > >>> It's not clear if it matters - it's more a policy decision on the part > >>> of the driver about what it thinks safe error handling is. If it's not > > > >> If regulator_get() returns a dummy regulator, then this means that > >> regulator isn't specified in a device-tree. But then the only way for a > >> consumer driver to check whether regulator is dummy, is to check > >> presence of the supply property in a device-tree. > > > > My point here is that the driver shouldn't be checking for a dummy > > regulator, the driver should be checking the features that are provided > > to it by the regulator and handling those. > > I understand yours point, but then we need dummy regulator to provide > all the features, and currently it does the opposite. But that's exactly Mark's point. Any regular regulator could be lacking all of the features just as well. If the regulator supports a fixed voltage, then it's not going to allow you to set a different one, etc. The point is that the regulator consumer should then be written in a way to deal with varying levels of features. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/16] staging: media: tegra-vde: add proper SPDX identifiers on file that did not have it.
On Tue, Apr 02, 2019 at 12:32:03PM +0200, Greg Kroah-Hartman wrote: > There was a single file for the tegra-vde driver that did not have SPDX > identifiers on it, so fix that up. At the same time, remove the "free > form" text that specified the license of the file, as that is impossible > for any tool to properly parse. > > Cc: Dmitry Osipenko > Cc: Mauro Carvalho Chehab > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-me...@vger.kernel.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman > --- > drivers/staging/media/tegra-vde/uapi.h | 11 ++- > 1 file changed, 2 insertions(+), 9 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
> On 8/26/19 3:31 PM, YueHaibing wrote:
> > If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> > to m will set IOMMU_IOVA to m, this fails the building of
> > TEGRA_HOST1X and DRM_TEGRA which is y like this:
> >
> > drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> > cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> > cdma.c:(.text+0x698): undefined reference to `__free_iova'
> >
> > drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> > drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> > drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> >
> > Reported-by: Hulk Robot
> > Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> > Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU support")
> > Signed-off-by: YueHaibing
> > ---
> > drivers/staging/media/tegra-vde/Kconfig | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/tegra-vde/Kconfig
> > b/drivers/staging/media/tegra-vde/Kconfig
> > index ba49ea5..a41d30c 100644
> > --- a/drivers/staging/media/tegra-vde/Kconfig
> > +++ b/drivers/staging/media/tegra-vde/Kconfig
> > @@ -1,9 +1,9 @@
> > # SPDX-License-Identifier: GPL-2.0
> > config TEGRA_VDE
> > tristate "NVIDIA Tegra Video Decoder Engine driver"
> > - depends on ARCH_TEGRA || COMPILE_TEST
> > + depends on ARCH_TEGRA
>
> What happens if you drop this change,
>
> > select DMA_SHARED_BUFFER
> > - select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> > + select IOMMU_IOVA if IOMMU_SUPPORT
>
> but keep this change?
>
> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
> work when compile testing this tegra-vde driver.
>
> Haven't tried it, but making sure that compile testing keep working is
> really important.
Yeah, that variant seems to work for me. I think it's also more correct
because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
then IOVA is not needed either, so the dummies will do just fine.
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging: tegra-vde: Disable building with COMPILE_TEST
On Thu, Aug 29, 2019 at 04:58:22PM +0300, Dmitry Osipenko wrote:
> 29.08.2019 15:40, Thierry Reding пишет:
> > On Thu, Aug 29, 2019 at 01:39:32PM +0200, Hans Verkuil wrote:
> >> On 8/26/19 3:31 PM, YueHaibing wrote:
> >>> If COMPILE_TEST is y and IOMMU_SUPPORT is n, selecting TEGRA_VDE
> >>> to m will set IOMMU_IOVA to m, this fails the building of
> >>> TEGRA_HOST1X and DRM_TEGRA which is y like this:
> >>>
> >>> drivers/gpu/host1x/cdma.o: In function `host1x_cdma_init':
> >>> cdma.c:(.text+0x66c): undefined reference to `alloc_iova'
> >>> cdma.c:(.text+0x698): undefined reference to `__free_iova'
> >>>
> >>> drivers/gpu/drm/tegra/drm.o: In function `tegra_drm_unload':
> >>> drm.c:(.text+0xeb0): undefined reference to `put_iova_domain'
> >>> drm.c:(.text+0xeb4): undefined reference to `iova_cache_put'
> >>>
> >>> Reported-by: Hulk Robot
> >>> Fixes: 6b2265975239 ("media: staging: tegra-vde: Fix build error")
> >>> Fixes: b301f8de1925 ("media: staging: media: tegra-vde: Add IOMMU
> >>> support")
> >>> Signed-off-by: YueHaibing
> >>> ---
> >>> drivers/staging/media/tegra-vde/Kconfig | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/tegra-vde/Kconfig
> >>> b/drivers/staging/media/tegra-vde/Kconfig
> >>> index ba49ea5..a41d30c 100644
> >>> --- a/drivers/staging/media/tegra-vde/Kconfig
> >>> +++ b/drivers/staging/media/tegra-vde/Kconfig
> >>> @@ -1,9 +1,9 @@
> >>> # SPDX-License-Identifier: GPL-2.0
> >>> config TEGRA_VDE
> >>> tristate "NVIDIA Tegra Video Decoder Engine driver"
> >>> - depends on ARCH_TEGRA || COMPILE_TEST
> >>> + depends on ARCH_TEGRA
> >>
> >> What happens if you drop this change,
> >>
> >>> select DMA_SHARED_BUFFER
> >>> - select IOMMU_IOVA if (IOMMU_SUPPORT || COMPILE_TEST)
> >>> + select IOMMU_IOVA if IOMMU_SUPPORT
> >>
> >> but keep this change?
> >>
> >> iova.h has stubs that are used if IOMMU_IOVA is not set, so it should
> >> work when compile testing this tegra-vde driver.
> >>
> >> Haven't tried it, but making sure that compile testing keep working is
> >> really important.
>
> The driver's code compilation works okay, it's the linkage stage which
> fails during compile-testing.
>
> > Yeah, that variant seems to work for me. I think it's also more correct
> > because the IOMMU_IOVA if IOMMU_SUPPORT dependency really says that the
> > IOVA usage is bound to IOMMU support. If IOMMU support is not enabled,
> > then IOVA is not needed either, so the dummies will do just fine.
>
> Am I understanding correctly that you're suggesting to revert [1][2] and
> get back to the other problem?
>
> [1]
> https://lore.kernel.org/linux-media/dd547b44-7abb-371f-aeee-a82b96f82...@gmail.com/T/
> [2] https://patchwork.ozlabs.org/patch/1136619/
>
> If we want to keep compile testing, I guess the only reasonable variant
> right now is to select IOMMU_IOVA unconditionally in all of the drivers
> (vde, host1x, drm and etc) and then just ignore that IOVA will be
> compiled-and-unused if IOMMU_SUPPORT=n (note that IOMMU_SUPPORT=y in all
> of default kernel configurations).
Agreed. I think we should just select IOMMU_IOVA unconditionally. We
really do want IOMMU_SUPPORT always as well, but it might be nice to be
able to switch it off for testing or so. In the cases that really matter
we will be enabling both IOMMU_SUPPORT and IOMMU_IOVA anyway, so might
as well select IOMMU_IOVA always. It's not terribly big and I can't
imagine anyone wanting to run a kernel without IOMMU_SUPPORT for
anything other than testing.
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] ARM: tegra: Enable Tegra VDE driver in tegra_defconfig
On Sun, Jun 23, 2019 at 08:07:25PM +0300, Dmitry Osipenko wrote: > The video decoder driver was tested by time and works absolutely fine. > The reason why it is in staging is because it doesn't provide common V4L > interface yet, this shouldn't stop driver enabling in the defconfig since > our userspace (libvdpau-tegra) provides combined acceleration of decoding > and displaying without use of V4L. > > Signed-off-by: Dmitry Osipenko > --- > > No changes since v1. > > arch/arm/configs/tegra_defconfig | 2 ++ > 1 file changed, 2 insertions(+) Applied to for-5.5/arm/defconfig, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] ARM: dts: tegra30: Connect SMMU with Video Decoder Engine
On Sun, Jun 23, 2019 at 08:07:24PM +0300, Dmitry Osipenko wrote: > Enable IOMMU support for the video decoder. > > Signed-off-by: Dmitry Osipenko > --- > > No changes since v1. > > arch/arm/boot/dts/tegra30.dtsi | 1 + > 1 file changed, 1 insertion(+) Applied to for-5.5/arm/dt, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: android: ion: Deletion of unnecessary checks before two function calls
On Sun, Nov 23, 2014 at 07:39:23PM +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sun, 23 Nov 2014 18:48:15 +0100 > > The functions ion_heap_destroy() and vfree() perform also input > parameter validation. Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/staging/android/ion/ion.c | 6 ++ > drivers/staging/android/ion/ion_dummy_driver.c | 6 ++ > drivers/staging/android/ion/tegra/tegra_ion.c | 6 ++ > 3 files changed, 6 insertions(+), 12 deletions(-) Acked-by: Thierry Reding pgp21exAuQEUm.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 2/2] ARM: dts: tegra20: Add video decoder node
On Thu, Oct 12, 2017 at 03:06:17PM +0300, Dmitry Osipenko wrote:
> Hello Vladimir,
>
> On 12.10.2017 10:43, Vladimir Zapolskiy wrote:
> > Hello Dmitry,
> >
> > On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
> >> Add a device node for the video decoder engine found on Tegra20.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> arch/arm/boot/dts/tegra20.dtsi | 17 +
> >> 1 file changed, 17 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/tegra20.dtsi
> >> b/arch/arm/boot/dts/tegra20.dtsi
> >> index 7c85f97f72ea..1b5d54b6c0cb 100644
> >> --- a/arch/arm/boot/dts/tegra20.dtsi
> >> +++ b/arch/arm/boot/dts/tegra20.dtsi
> >> @@ -249,6 +249,23 @@
> >>*/
> >>};
> >>
> >> + vde@6001a000 {
> >> + compatible = "nvidia,tegra20-vde";
> >> + reg = <0x6001a000 0x3D00/* VDE registers */
> >> + 0x4400 0x3FC00>; /* IRAM region */
> >
> > this notation of a used region in IRAM is non-standard and potentially it
> > may lead to conflicts for IRAM resource between users.
> >
> > My proposal is to add a valid device tree node to describe an IRAM region
> > firstly, then reserve a subregion in it by using a new "iram" property.
> >
>
> The defined in DT IRAM region used by VDE isn't exactly correct, actually it
> should be much smaller. I don't know exactly what parts of IRAM VDE uses, for
> now it is just safer to assign the rest of the IRAM region to VDE.
>
> I'm not sure whether it really worthy to use a dynamic allocator for a single
> static allocation, but maybe it would come handy later.. Stephen / Jon /
> Thierry, what do you think?
This sounds like a good idea. I agree that this currently doesn't seem
to be warranted, but consider what would happen if at some point we have
more devices requiring access to the IRAM. Spreading individual reg
properties all across the DT will make it very difficult to ensure they
don't overlap.
Presumably the mmio-sram driver will check that pool don't overlap. Or
even if it doesn't it will make it a lot easier to verify because it's
all in the same DT node and then consumers only reference it.
I like Vladimir's proposal. I also suspect that Rob may want us to stick
to a standardized way referencing such external memory.
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On Tue, Oct 17, 2017 at 03:13:54PM -0500, Rob Herring wrote: [...] > > diff --git > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt [...] > > +- resets : Must contain an entry for each entry in reset-names. > > + See ../reset/reset.txt for details. > > +- reset-names : Must include the following entries: > > + - vde > > -names is pointless when there is only one. I'd prefer to keep it. In the past we occasionally had to add clocks or resets to a device tree node where only one had been present (and hence no -names property) and that caused some awkwardness because verbiage had to be added to the bindings that clarified that one particular entry (the original one) always had to come first. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 2/2] ARM: dts: tegra30: Add Memory Client reset to VDE
On Sun, May 20, 2018 at 04:48:46PM +0300, Dmitry Osipenko wrote: > Hook up Memory Client reset of the Video Decoder to the decoders DT node. > > Signed-off-by: Dmitry Osipenko > --- > arch/arm/boot/dts/tegra30.dtsi | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Applied, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/2] ARM: dts: tegra20: Add Memory Client reset to VDE
On Sun, May 20, 2018 at 04:48:44PM +0300, Dmitry Osipenko wrote: > Hook up Memory Client reset of the Video Decoder to the decoders DT node. > > Signed-off-by: Dmitry Osipenko > --- > arch/arm/boot/dts/tegra20.dtsi | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 08/14] staging: media: tegra-vde: Track struct device *
From: Thierry Reding
The pointer to the struct device is frequently used, so store it in
struct tegra_vde. Also, pass around a pointer to a struct tegra_vde
instead of struct device in some cases to prepare for subsequent
patches referencing additional data from that structure.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 63 -
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 41cf86dc5dbd..2496a03fd158 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -71,6 +71,7 @@ struct tegra_vde_soc {
};
struct tegra_vde {
+ struct device *dev;
const struct tegra_vde_soc *soc;
void __iomem *sxe;
void __iomem *bsev;
@@ -644,7 +645,7 @@ static void tegra_vde_detach_and_put_dmabuf(struct
dma_buf_attachment *a,
dma_buf_put(dmabuf);
}
-static int tegra_vde_attach_dmabuf(struct device *dev,
+static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
int fd,
unsigned long offset,
size_t min_size,
@@ -662,38 +663,40 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf)) {
- dev_err(dev, "Invalid dmabuf FD: %d\n", fd);
+ dev_err(vde->dev, "Invalid dmabuf FD: %d\n", fd);
return PTR_ERR(dmabuf);
}
if (dmabuf->size & (align_size - 1)) {
- dev_err(dev, "Unaligned dmabuf 0x%zX, should be aligned to
0x%zX\n",
+ dev_err(vde->dev,
+ "Unaligned dmabuf 0x%zX, should be aligned to 0x%zX\n",
dmabuf->size, align_size);
return -EINVAL;
}
if ((u64)offset + min_size > dmabuf->size) {
- dev_err(dev, "Too small dmabuf size %zu @0x%lX, should be at
least %zu\n",
+ dev_err(vde->dev,
+ "Too small dmabuf size %zu @0x%lX, should be at least
%zu\n",
dmabuf->size, offset, min_size);
return -EINVAL;
}
- attachment = dma_buf_attach(dmabuf, dev);
+ attachment = dma_buf_attach(dmabuf, vde->dev);
if (IS_ERR(attachment)) {
- dev_err(dev, "Failed to attach dmabuf\n");
+ dev_err(vde->dev, "Failed to attach dmabuf\n");
err = PTR_ERR(attachment);
goto err_put;
}
sgt = dma_buf_map_attachment(attachment, dma_dir);
if (IS_ERR(sgt)) {
- dev_err(dev, "Failed to get dmabufs sg_table\n");
+ dev_err(vde->dev, "Failed to get dmabufs sg_table\n");
err = PTR_ERR(sgt);
goto err_detach;
}
if (sgt->nents != 1) {
- dev_err(dev, "Sparse DMA region is unsupported\n");
+ dev_err(vde->dev, "Sparse DMA region is unsupported\n");
err = -EINVAL;
goto err_unmap;
}
@@ -717,7 +720,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
return err;
}
-static int tegra_vde_attach_dmabufs_to_frame(struct device *dev,
+static int tegra_vde_attach_dmabufs_to_frame(struct tegra_vde *vde,
struct video_frame *frame,
struct tegra_vde_h264_frame *src,
enum dma_data_direction dma_dir,
@@ -726,7 +729,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device
*dev,
{
int err;
- err = tegra_vde_attach_dmabuf(dev, src->y_fd,
+ err = tegra_vde_attach_dmabuf(vde, src->y_fd,
src->y_offset, lsize, SZ_256,
&frame->y_dmabuf_attachment,
&frame->y_addr,
@@ -735,7 +738,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device
*dev,
if (err)
return err;
- err = tegra_vde_attach_dmabuf(dev, src->cb_fd,
+ err = tegra_vde_attach_dmabuf(vde, src->cb_fd,
src->cb_offset, csize, SZ_256,
&frame->cb_dmabuf_attachment,
&frame->cb_addr,
@@ -744,7 +747,7 @@ static int tegra_vde_attach_dmabufs_to_frame(struct device
*dev,
if (err)
goto err_release_y;
- err = tegra_vde_attach_dmabuf(dev, src->cr_fd,
+ err = tegra_vde_attach_dmabuf(vde, src->cr_fd,
[PATCH 05/14] staging: media: tegra-vde: Properly mark invalid entries
From: Thierry Reding
Entries in the reference picture list are marked as invalid by setting
the frame ID to 0x3f.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 275884e745df..0ce30c7ccb75 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -296,7 +296,7 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
*vde,
(frame->flags & FLAG_B_FRAME));
} else {
aux_addr = 0x6ADEAD00;
- value = 0;
+ value = 0x3f;
}
tegra_vde_setup_iram_entry(vde, num_ref_pics, 0, i, value,
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 13/14] ARM: tegra: Add BSEV clock and reset for VDE on Tegra30
From: Thierry Reding
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra30.dtsi | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index a6781f653310..492917d61bab 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -408,9 +408,13 @@
, /* BSE-V
interrupt */
; /* SXE interrupt
*/
interrupt-names = "sync-token", "bsev", "sxe";
- clocks = <&tegra_car TEGRA30_CLK_VDE>;
- reset-names = "vde", "mc";
- resets = <&tegra_car 61>, <&mc TEGRA30_MC_RESET_VDE>;
+ clocks = <&tegra_car TEGRA30_CLK_VDE>,
+<&tegra_car TEGRA30_CLK_BSEV>;
+ clock-names = "vde", "bsev";
+ resets = <&tegra_car 61>,
+<&tegra_car 63>,
+<&mc TEGRA30_MC_RESET_VDE>;
+ reset-names = "vde", "bsev", "mc";
};
apbmisc@7800 {
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 04/14] staging: media: tegra-vde: Use DRM/KMS framebuffer modifiers
From: Thierry Reding
VDE on Tegra20 through Tegra114 supports reading and writing frames in
16x16 tiled layout. Similarily, the various block-linear layouts that
are supported by the GPU on Tegra124 can also be read from and written
to by the Tegra124 VDE.
Enable userspace to specify the desired layout using the existing DRM
framebuffer modifiers.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 112 +---
drivers/staging/media/tegra-vde/uapi.h | 3 +-
2 files changed, 100 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 1a40f6dff7c8..275884e745df 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -24,6 +24,8 @@
#include
+#include
+
#include "uapi.h"
#define ICMDQUE_WR 0x00
@@ -58,12 +60,14 @@ struct video_frame {
dma_addr_t aux_addr;
u32 frame_num;
u32 flags;
+ u64 modifier;
};
struct tegra_vde_soc {
unsigned int num_ref_pics;
bool supports_ref_pic_marking;
bool supports_interlacing;
+ bool supports_block_linear;
};
struct tegra_vde {
@@ -202,6 +206,7 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
unsigned int frameid,
u32 mbs_width, u32 mbs_height)
{
+ u64 modifier = frame ? frame->modifier : DRM_FORMAT_MOD_LINEAR;
u32 y_addr = frame ? frame->y_addr : 0x6CDEAD00;
u32 cb_addr = frame ? frame->cb_addr : 0x6CDEAD00;
u32 cr_addr = frame ? frame->cr_addr : 0x6CDEAD00;
@@ -209,8 +214,12 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
u32 value2 = frame ? mbs_width + 1) >> 1) << 6) | 1) : 0;
u32 value = y_addr >> 8;
- if (vde->soc->supports_interlacing)
+ if (!vde->soc->supports_interlacing) {
+ if (modifier == DRM_FORMAT_MOD_NVIDIA_TEGRA_TILED)
+ value |= BIT(31);
+ } else {
value |= BIT(31);
+ }
VDE_WR(value,vde->frameid + 0x000 + frameid * 4);
VDE_WR(cb_addr >> 8, vde->frameid + 0x100 + frameid * 4);
@@ -349,6 +358,37 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
*vde,
}
}
+static int tegra_vde_get_block_height(u64 modifier, unsigned int *block_height)
+{
+ switch (modifier) {
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_ONE_GOB:
+ *block_height = 0;
+ return 0;
+
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_TWO_GOB:
+ *block_height = 1;
+ return 0;
+
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_FOUR_GOB:
+ *block_height = 2;
+ return 0;
+
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_EIGHT_GOB:
+ *block_height = 3;
+ return 0;
+
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_SIXTEEN_GOB:
+ *block_height = 4;
+ return 0;
+
+ case DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_THIRTYTWO_GOB:
+ *block_height = 5;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
struct tegra_vde_h264_decoder_ctx *ctx,
struct video_frame *dpb_frames,
@@ -383,7 +423,21 @@ static int tegra_vde_setup_hw_context(struct tegra_vde
*vde,
tegra_vde_set_bits(vde, 0x0005, vde->vdma + 0x04);
VDE_WR(0x, vde->vdma + 0x1C);
- VDE_WR(0x, vde->vdma + 0x00);
+
+ value = 0x;
+
+ if (vde->soc->supports_block_linear) {
+ unsigned int block_height;
+
+ err = tegra_vde_get_block_height(dpb_frames[0].modifier,
+&block_height);
+ if (err < 0)
+ return err;
+
+ value |= block_height << 10;
+ }
+
+ VDE_WR(value, vde->vdma + 0x00);
VDE_WR(0x0007, vde->vdma + 0x04);
VDE_WR(0x0007, vde->frameid + 0x200);
VDE_WR(0x0005, vde->tfe + 0x04);
@@ -730,11 +784,37 @@ static void tegra_vde_release_frame_dmabufs(struct
video_frame *frame,
static int tegra_vde_validate_frame(struct device *dev,
struct tegra_vde_h264_frame *frame)
{
+ struct tegra_vde *vde = dev_get_drvdata(dev);
+
if (frame->frame_num > 0x7F) {
dev_err(dev, "Bad frame_num %u\n", frame->frame_num);
return -EINVAL;
}
+ if (vde->soc->supports_block_linear) {
+ switch (frame->modifier) {
+
[PATCH 01/14] staging: media: tegra-vde: Support BSEV clock and reset
From: Thierry Reding
The BSEV clock has a separate gate bit and can not be assumed to be
always enabled. Add explicit handling for the BSEV clock and reset.
This fixes an issue on Tegra124 where the BSEV clock is not enabled
by default and therefore accessing the BSEV registers will hang the
CPU if the BSEV clock is not enabled and the reset not deasserted.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 35 +++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 6f06061a40d9..9d8f833744db 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -74,9 +74,11 @@ struct tegra_vde {
struct miscdevice miscdev;
struct reset_control *rst;
struct reset_control *rst_mc;
+ struct reset_control *rst_bsev;
struct gen_pool *iram_pool;
struct completion decode_completion;
struct clk *clk;
+ struct clk *clk_bsev;
dma_addr_t iram_lists_addr;
u32 *iram;
};
@@ -979,6 +981,11 @@ static int tegra_vde_runtime_suspend(struct device *dev)
return err;
}
+ reset_control_assert(vde->rst_bsev);
+
+ usleep_range(2000, 4000);
+
+ clk_disable_unprepare(vde->clk_bsev);
clk_disable_unprepare(vde->clk);
return 0;
@@ -996,6 +1003,16 @@ static int tegra_vde_runtime_resume(struct device *dev)
return err;
}
+ err = clk_prepare_enable(vde->clk_bsev);
+ if (err < 0)
+ return err;
+
+ err = reset_control_deassert(vde->rst_bsev);
+ if (err < 0)
+ return err;
+
+ usleep_range(2000, 4000);
+
return 0;
}
@@ -1084,14 +1101,21 @@ static int tegra_vde_probe(struct platform_device *pdev)
if (IS_ERR(vde->frameid))
return PTR_ERR(vde->frameid);
- vde->clk = devm_clk_get(dev, NULL);
+ vde->clk = devm_clk_get(dev, "vde");
if (IS_ERR(vde->clk)) {
err = PTR_ERR(vde->clk);
dev_err(dev, "Could not get VDE clk %d\n", err);
return err;
}
- vde->rst = devm_reset_control_get(dev, NULL);
+ vde->clk_bsev = devm_clk_get(dev, "bsev");
+ if (IS_ERR(vde->clk_bsev)) {
+ err = PTR_ERR(vde->clk_bsev);
+ dev_err(dev, "failed to get BSEV clock: %d\n", err);
+ return err;
+ }
+
+ vde->rst = devm_reset_control_get(dev, "vde");
if (IS_ERR(vde->rst)) {
err = PTR_ERR(vde->rst);
dev_err(dev, "Could not get VDE reset %d\n", err);
@@ -1105,6 +1129,13 @@ static int tegra_vde_probe(struct platform_device *pdev)
return err;
}
+ vde->rst_bsev = devm_reset_control_get(dev, "bsev");
+ if (IS_ERR(vde->rst_bsev)) {
+ err = PTR_ERR(vde->rst_bsev);
+ dev_err(dev, "failed to get BSEV reset: %d\n", err);
+ return err;
+ }
+
irq = platform_get_irq_byname(pdev, "sync-token");
if (irq < 0)
return irq;
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/14] staging: media: tegra-vde: Keep VDE in reset when unused
From: Thierry Reding There is no point in keeping the VDE module out of reset when it is not in use. Reset it on runtime suspend. Signed-off-by: Thierry Reding --- drivers/staging/media/tegra-vde/tegra-vde.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index 3bc0bfcfe34e..4b3c6ab3c77e 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -1226,6 +1226,7 @@ static int tegra_vde_runtime_suspend(struct device *dev) } reset_control_assert(vde->rst_bsev); + reset_control_assert(vde->rst); usleep_range(2000, 4000); -- 2.17.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 00/14] staging: media: tegra-vdea: Add Tegra124 support
From: Thierry Reding Hi, this set of patches perform a bit of cleanup and extend support to the VDE implementation found on Tegra114 and Tegra124. This requires adding handling for a clock and a reset for the BSEV block that is separate from the main VDE block. The new VDE revision also supports reference picture marking, which requires that the BSEV writes out some related data to a memory location. Since the supported tiling layouts have been changed in Tegra124, which supports only block-linear and no pitch- linear layouts, a new way is added to request a specific layout for the decoded frames. Both of the above changes require breaking the ABI to accomodate for the new data in the custom IOCTL. Finally this set also adds support for dealing with an IOMMU, which makes it more convenient to deal with imported buffers since they no longer need to be physically contiguous. Userspace changes for the updated ABI are available here: https://cgit.freedesktop.org/~tagr/libvdpau-tegra/commit/ Mauro, I'm sending the device tree changes as part of the series for completeness, but I expect to pick those up into the Tegra tree once this has been reviewed and you've applied the driver changes. Thanks, Thierry Thierry Reding (14): staging: media: tegra-vde: Support BSEV clock and reset staging: media: tegra-vde: Support reference picture marking staging: media: tegra-vde: Prepare for interlacing support staging: media: tegra-vde: Use DRM/KMS framebuffer modifiers staging: media: tegra-vde: Properly mark invalid entries staging: media: tegra-vde: Print out invalid FD staging: media: tegra-vde: Add some clarifying comments staging: media: tegra-vde: Track struct device * staging: media: tegra-vde: Add IOMMU support staging: media: tegra-vde: Keep VDE in reset when unused ARM: tegra: Enable VDE on Tegra124 ARM: tegra: Add BSEV clock and reset for VDE on Tegra20 ARM: tegra: Add BSEV clock and reset for VDE on Tegra30 ARM: tegra: Enable SMMU for VDE on Tegra124 arch/arm/boot/dts/tegra124.dtsi | 42 ++ arch/arm/boot/dts/tegra20.dtsi | 10 +- arch/arm/boot/dts/tegra30.dtsi | 10 +- drivers/staging/media/tegra-vde/tegra-vde.c | 528 +--- drivers/staging/media/tegra-vde/uapi.h | 6 +- 5 files changed, 511 insertions(+), 85 deletions(-) -- 2.17.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 11/14] ARM: tegra: Enable VDE on Tegra124
From: Thierry Reding
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra124.dtsi | 40 +
1 file changed, 40 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index b113e47b2b2a..8fdca4723205 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -83,6 +83,19 @@
};
};
+ iram@4000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x4000 0x0 0x4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x0 0x4000 0x4>;
+
+ vde_pool: pool@400 {
+ reg = <0x400 0x3fc00>;
+ pool;
+ };
+ };
+
host1x@5000 {
compatible = "nvidia,tegra124-host1x", "simple-bus";
reg = <0x0 0x5000 0x0 0x00034000>;
@@ -283,6 +296,33 @@
*/
};
+ vde@6003 {
+ compatible = "nvidia,tegra124-vde", "nvidia,tegra30-vde",
+"nvidia,tegra20-vde";
+ reg = <0x0 0x6003 0x0 0x1000 /* Syntax Engine */
+ 0x0 0x60031000 0x0 0x1000 /* Video Bitstream Engine */
+ 0x0 0x60032000 0x0 0x0100 /* Macroblock Engine */
+ 0x0 0x60032200 0x0 0x0100 /* Post-processing Engine */
+ 0x0 0x60032400 0x0 0x0100 /* Motion Compensation
Engine */
+ 0x0 0x60032600 0x0 0x0100 /* Transform Engine */
+ 0x0 0x60032800 0x0 0x0100 /* Pixel prediction block */
+ 0x0 0x60032a00 0x0 0x0100 /* Video DMA */
+ 0x0 0x60033800 0x0 0x0400>; /* Video frame controls */
+ reg-names = "sxe", "bsev", "mbe", "ppe", "mce",
+ "tfe", "ppb", "vdma", "frameid";
+ iram = <&vde_pool>; /* IRAM region */
+ interrupts = , /* Sync token
interrupt */
+, /* BSE-V
interrupt */
+; /* SXE interrupt
*/
+ interrupt-names = "sync-token", "bsev", "sxe";
+ clocks = <&tegra_car TEGRA124_CLK_VDE>,
+<&tegra_car TEGRA124_CLK_BSEV>;
+ clock-names = "vde", "bsev";
+ resets = <&tegra_car 61>,
+<&tegra_car 63>;
+ reset-names = "vde", "bsev";
+ };
+
apbdma: dma@6002 {
compatible = "nvidia,tegra124-apbdma", "nvidia,tegra148-apbdma";
reg = <0x0 0x6002 0x0 0x1400>;
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 14/14] ARM: tegra: Enable SMMU for VDE on Tegra124
From: Thierry Reding
The video decode engine can use the SMMU to use buffers that are not
physically contiguous in memory. This allows better memory usage for
video decoding, since fragmentation may cause contiguous allocations
to fail.
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra124.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 8fdca4723205..0713e0ed5fef 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -321,6 +321,8 @@
resets = <&tegra_car 61>,
<&tegra_car 63>;
reset-names = "vde", "bsev";
+
+ iommus = <&mc TEGRA_SWGROUP_VDE>;
};
apbdma: dma@6002 {
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 06/14] staging: media: tegra-vde: Print out invalid FD
From: Thierry Reding
Include the invalid file descriptor when reporting an error message to
help diagnosing why importing the buffer failed.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 0ce30c7ccb75..0adc603fa437 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -643,7 +643,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
dmabuf = dma_buf_get(fd);
if (IS_ERR(dmabuf)) {
- dev_err(dev, "Invalid dmabuf FD\n");
+ dev_err(dev, "Invalid dmabuf FD: %d\n", fd);
return PTR_ERR(dmabuf);
}
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 12/14] ARM: tegra: Add BSEV clock and reset for VDE on Tegra20
From: Thierry Reding
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra20.dtsi | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 15b73bd377f0..abb5738a0705 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -287,9 +287,13 @@
, /* BSE-V
interrupt */
; /* SXE interrupt
*/
interrupt-names = "sync-token", "bsev", "sxe";
- clocks = <&tegra_car TEGRA20_CLK_VDE>;
- reset-names = "vde", "mc";
- resets = <&tegra_car 61>, <&mc TEGRA20_MC_RESET_VDE>;
+ clocks = <&tegra_car TEGRA20_CLK_VDE>,
+<&tegra_car TEGRA20_CLK_BSEV>;
+ clock-names = "vde", "bsev";
+ resets = <&tegra_car 61>,
+<&tegra_car 63>,
+<&mc TEGRA20_MC_RESET_VDE>;
+ reset-names = "vde", "bsev", "mc";
};
apbmisc@7800 {
--
2.17.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 03/14] staging: media: tegra-vde: Prepare for interlacing support
From: Thierry Reding
The number of frames doubles when decoding interlaced content and the
structures describing the frames double in size. Take that into account
to prepare for interlacing support.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 73 -
1 file changed, 58 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 3027b11b11ae..1a40f6dff7c8 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -61,7 +61,9 @@ struct video_frame {
};
struct tegra_vde_soc {
+ unsigned int num_ref_pics;
bool supports_ref_pic_marking;
+ bool supports_interlacing;
};
struct tegra_vde {
@@ -205,8 +207,12 @@ static void tegra_vde_setup_frameid(struct tegra_vde *vde,
u32 cr_addr = frame ? frame->cr_addr : 0x6CDEAD00;
u32 value1 = frame ? ((mbs_width << 16) | mbs_height) : 0;
u32 value2 = frame ? mbs_width + 1) >> 1) << 6) | 1) : 0;
+ u32 value = y_addr >> 8;
- VDE_WR(y_addr >> 8, vde->frameid + 0x000 + frameid * 4);
+ if (vde->soc->supports_interlacing)
+ value |= BIT(31);
+
+ VDE_WR(value,vde->frameid + 0x000 + frameid * 4);
VDE_WR(cb_addr >> 8, vde->frameid + 0x100 + frameid * 4);
VDE_WR(cr_addr >> 8, vde->frameid + 0x180 + frameid * 4);
VDE_WR(value1, vde->frameid + 0x080 + frameid * 4);
@@ -229,20 +235,23 @@ static void tegra_setup_frameidx(struct tegra_vde *vde,
}
static void tegra_vde_setup_iram_entry(struct tegra_vde *vde,
+ unsigned int num_ref_pics,
unsigned int table,
unsigned int row,
u32 value1, u32 value2)
{
+ unsigned int entries = num_ref_pics * 2;
u32 *iram_tables = vde->iram;
dev_dbg(vde->miscdev.parent, "IRAM table %u: row %u: 0x%08X 0x%08X\n",
table, row, value1, value2);
- iram_tables[0x20 * table + row * 2] = value1;
- iram_tables[0x20 * table + row * 2 + 1] = value2;
+ iram_tables[entries * table + row * 2] = value1;
+ iram_tables[entries * table + row * 2 + 1] = value2;
}
static void tegra_vde_setup_iram_tables(struct tegra_vde *vde,
+ unsigned int num_ref_pics,
struct video_frame *dpb_frames,
unsigned int ref_frames_nb,
unsigned int with_earlier_poc_nb)
@@ -251,13 +260,17 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
*vde,
u32 value, aux_addr;
int with_later_poc_nb;
unsigned int i, k;
+ size_t size;
+
+ size = num_ref_pics * 4 * 8;
+ memset(vde->iram, 0, size);
dev_dbg(vde->miscdev.parent, "DPB: Frame 0: frame_num = %d\n",
dpb_frames[0].frame_num);
dev_dbg(vde->miscdev.parent, "REF L0:\n");
- for (i = 0; i < 16; i++) {
+ for (i = 0; i < num_ref_pics; i++) {
if (i < ref_frames_nb) {
frame = &dpb_frames[i + 1];
@@ -277,10 +290,14 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
*vde,
value = 0;
}
- tegra_vde_setup_iram_entry(vde, 0, i, value, aux_addr);
- tegra_vde_setup_iram_entry(vde, 1, i, value, aux_addr);
- tegra_vde_setup_iram_entry(vde, 2, i, value, aux_addr);
- tegra_vde_setup_iram_entry(vde, 3, i, value, aux_addr);
+ tegra_vde_setup_iram_entry(vde, num_ref_pics, 0, i, value,
+ aux_addr);
+ tegra_vde_setup_iram_entry(vde, num_ref_pics, 1, i, value,
+ aux_addr);
+ tegra_vde_setup_iram_entry(vde, num_ref_pics, 2, i, value,
+ aux_addr);
+ tegra_vde_setup_iram_entry(vde, num_ref_pics, 3, i, value,
+ aux_addr);
}
if (!(dpb_frames[0].flags & FLAG_B_FRAME))
@@ -309,7 +326,8 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde
*vde,
"\tFrame %d: frame_num = %d\n",
k + 1, frame->frame_num);
- tegra_vde_setup_iram_entry(vde, 2, i, value, aux_addr);
+ tegra_vde_setup_iram_entry(vde, num_ref_pics, 2, i, value,
+ aux_addr);
}
for (k = 0; i < ref_frames_nb; i++, k++) {
@@ -326,7 +344,8 @@ static void tegra_vde_s
[PATCH 07/14] staging: media: tegra-vde: Add some clarifying comments
From: Thierry Reding Add some comments specifying what tables are being set up in VRAM. Signed-off-by: Thierry Reding --- drivers/staging/media/tegra-vde/tegra-vde.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c index 0adc603fa437..41cf86dc5dbd 100644 --- a/drivers/staging/media/tegra-vde/tegra-vde.c +++ b/drivers/staging/media/tegra-vde/tegra-vde.c @@ -271,6 +271,7 @@ static void tegra_vde_setup_iram_tables(struct tegra_vde *vde, unsigned int i, k; size_t size; + /* clear H256RefPicList */ size = num_ref_pics * 4 * 8; memset(vde->iram, 0, size); @@ -453,6 +454,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, VDE_WR(0x, vde->bsev + 0x98); VDE_WR(0x0060, vde->bsev + 0x9C); + /* clear H264MB2SliceGroupMap, assuming no FMO */ memset(vde->iram + 1024, 0, macroblocks_nb / 2); tegra_setup_frameidx(vde, dpb_frames, ctx->dpb_frames_nb, @@ -480,6 +482,8 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, if (err) return err; + /* upload H264MB2SliceGroupMap */ + /* XXX don't hardcode map size? */ value = (0x20 << 26) | (0 << 25) | ((4096 >> 2) & 0x1fff); err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false); if (err) @@ -492,6 +496,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, if (err) return err; + /* clear H264MBInfo XXX don't hardcode size */ value = (0x21 << 26) | ((240 & 0x1fff) << 12) | (0x54c & 0xfff); err = tegra_vde_push_to_bsev_icmdqueue(vde, 0x840F054C, false); if (err) @@ -499,6 +504,16 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, size = num_ref_pics * 4 * 8; + /* clear H264RefPicList */ + /* + value = (0x21 << 26) | (((size >> 2) & 0x1fff) << 12) | 0xE34; + + err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false); + if (err) + return err; + */ + + /* upload H264RefPicList */ value = (0x20 << 26) | (0x0 << 25) | ((size >> 2) & 0x1fff); err = tegra_vde_push_to_bsev_icmdqueue(vde, value, false); if (err) @@ -584,7 +599,11 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde, tegra_vde_mbe_set_0xa_reg(vde, 0, 0x09FC); tegra_vde_mbe_set_0xa_reg(vde, 2, 0x61DEAD00); +#if 0 + tegra_vde_mbe_set_0xa_reg(vde, 4, dpb_frames[0].aux_addr); /* 0x62DEAD00 */ +#else tegra_vde_mbe_set_0xa_reg(vde, 4, 0x62DEAD00); +#endif tegra_vde_mbe_set_0xa_reg(vde, 6, 0x63DEAD00); tegra_vde_mbe_set_0xa_reg(vde, 8, dpb_frames[0].aux_addr); -- 2.17.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 09/14] staging: media: tegra-vde: Add IOMMU support
From: Thierry Reding
Implement support for using an IOMMU to map physically discontiguous
buffers into contiguous I/O virtual mappings that the VDE can use. This
allows importing arbitrary DMA-BUFs for use by the VDE.
While at it, make sure that the device is detached from any DMA/IOMMU
mapping that it might have automatically been attached to at boot. If
using the IOMMU API explicitly, detaching from any existing mapping is
required to avoid double mapping of buffers.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 171 +---
1 file changed, 153 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 2496a03fd158..3bc0bfcfe34e 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -13,7 +13,9 @@
#include
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -22,6 +24,10 @@
#include
#include
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
+#include
+#endif
+
#include
#include
@@ -61,6 +67,11 @@ struct video_frame {
u32 frame_num;
u32 flags;
u64 modifier;
+
+ struct iova *y_iova;
+ struct iova *cb_iova;
+ struct iova *cr_iova;
+ struct iova *aux_iova;
};
struct tegra_vde_soc {
@@ -93,6 +104,12 @@ struct tegra_vde {
struct clk *clk_bsev;
dma_addr_t iram_lists_addr;
u32 *iram;
+
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+ struct iova_domain iova;
+ unsigned long limit;
+ unsigned int shift;
};
static void tegra_vde_set_bits(struct tegra_vde *vde,
@@ -634,12 +651,22 @@ static void tegra_vde_decode_frame(struct tegra_vde *vde,
VDE_WR(0x2000 | (macroblocks_nb - 1), vde->sxe + 0x00);
}
-static void tegra_vde_detach_and_put_dmabuf(struct dma_buf_attachment *a,
+static void tegra_vde_detach_and_put_dmabuf(struct tegra_vde *vde,
+ struct dma_buf_attachment *a,
struct sg_table *sgt,
+ struct iova *iova,
enum dma_data_direction dma_dir)
{
struct dma_buf *dmabuf = a->dmabuf;
+ if (vde->domain) {
+ unsigned long size = iova_size(iova) << vde->shift;
+ dma_addr_t addr = iova_dma_addr(&vde->iova, iova);
+
+ iommu_unmap(vde->domain, addr, size);
+ __free_iova(&vde->iova, iova);
+ }
+
dma_buf_unmap_attachment(a, sgt, dma_dir);
dma_buf_detach(dmabuf, a);
dma_buf_put(dmabuf);
@@ -651,14 +678,16 @@ static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
size_t min_size,
size_t align_size,
struct dma_buf_attachment **a,
- dma_addr_t *addr,
+ dma_addr_t *addrp,
struct sg_table **s,
- size_t *size,
+ struct iova **iovap,
+ size_t *sizep,
enum dma_data_direction dma_dir)
{
struct dma_buf_attachment *attachment;
struct dma_buf *dmabuf;
struct sg_table *sgt;
+ size_t size;
int err;
dmabuf = dma_buf_get(fd);
@@ -695,18 +724,47 @@ static int tegra_vde_attach_dmabuf(struct tegra_vde *vde,
goto err_detach;
}
- if (sgt->nents != 1) {
+ if (sgt->nents > 1 && !vde->domain) {
dev_err(vde->dev, "Sparse DMA region is unsupported\n");
err = -EINVAL;
goto err_unmap;
}
- *addr = sg_dma_address(sgt->sgl) + offset;
+ if (vde->domain) {
+ int prot = IOMMU_READ | IOMMU_WRITE;
+ struct iova *iova;
+ dma_addr_t addr;
+
+ size = (dmabuf->size - offset) >> vde->shift;
+
+ iova = alloc_iova(&vde->iova, size, vde->limit - 1, true);
+ if (!iova) {
+ err = -ENOMEM;
+ goto err_unmap;
+ }
+
+ addr = iova_dma_addr(&vde->iova, iova);
+
+ size = iommu_map_sg(vde->domain, addr, sgt->sgl, sgt->nents,
+ prot);
+ if (!size) {
+ __free_iova(&vde->iova, iova);
+ err = -ENXIO;
+ goto err_unmap;
+ }
+
+ *addrp = addr;
+ *iovap = iova;
+ } else {
+ *addrp = sg_dma_address(
[PATCH 02/14] staging: media: tegra-vde: Support reference picture marking
From: Thierry Reding
Tegra114 and Tegra124 support reference picture marking, which will
cause BSEV to write picture marking data to SDRAM. Make sure there is
a valid destination address for that data to avoid error messages from
the memory controller.
Signed-off-by: Thierry Reding
---
drivers/staging/media/tegra-vde/tegra-vde.c | 54 -
drivers/staging/media/tegra-vde/uapi.h | 3 ++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c
b/drivers/staging/media/tegra-vde/tegra-vde.c
index 9d8f833744db..3027b11b11ae 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -60,7 +60,12 @@ struct video_frame {
u32 flags;
};
+struct tegra_vde_soc {
+ bool supports_ref_pic_marking;
+};
+
struct tegra_vde {
+ const struct tegra_vde_soc *soc;
void __iomem *sxe;
void __iomem *bsev;
void __iomem *mbe;
@@ -330,6 +335,7 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
struct video_frame *dpb_frames,
dma_addr_t bitstream_data_addr,
size_t bitstream_data_size,
+ dma_addr_t secure_addr,
unsigned int macroblocks_nb)
{
struct device *dev = vde->miscdev.parent;
@@ -454,6 +460,9 @@ static int tegra_vde_setup_hw_context(struct tegra_vde *vde,
VDE_WR(bitstream_data_addr, vde->sxe + 0x6C);
+ if (vde->soc->supports_ref_pic_marking)
+ VDE_WR(secure_addr, vde->sxe + 0x7c);
+
value = 0x1005;
value |= ctx->pic_width_in_mbs << 11;
value |= ctx->pic_height_in_mbs << 3;
@@ -772,12 +781,15 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde
*vde,
struct tegra_vde_h264_frame __user *frames_user;
struct video_frame *dpb_frames;
struct dma_buf_attachment *bitstream_data_dmabuf_attachment;
- struct sg_table *bitstream_sgt;
+ struct dma_buf_attachment *secure_attachment = NULL;
+ struct sg_table *bitstream_sgt, *secure_sgt;
enum dma_data_direction dma_dir;
dma_addr_t bitstream_data_addr;
+ dma_addr_t secure_addr;
dma_addr_t bsev_ptr;
size_t lsize, csize;
size_t bitstream_data_size;
+ size_t secure_size;
unsigned int macroblocks_nb;
unsigned int read_bytes;
unsigned int cstride;
@@ -803,6 +815,18 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde
*vde,
if (ret)
return ret;
+ if (vde->soc->supports_ref_pic_marking) {
+ ret = tegra_vde_attach_dmabuf(dev, ctx.secure_fd,
+ ctx.secure_offset, 0, SZ_256,
+ &secure_attachment,
+ &secure_addr,
+ &secure_sgt,
+ &secure_size,
+ DMA_TO_DEVICE);
+ if (ret)
+ goto release_bitstream_dmabuf;
+ }
+
dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
GFP_KERNEL);
if (!dpb_frames) {
@@ -876,6 +900,7 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde
*vde,
ret = tegra_vde_setup_hw_context(vde, &ctx, dpb_frames,
bitstream_data_addr,
bitstream_data_size,
+secure_addr,
macroblocks_nb);
if (ret)
goto put_runtime_pm;
@@ -929,6 +954,10 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde
*vde,
kfree(dpb_frames);
release_bitstream_dmabuf:
+ if (secure_attachment)
+ tegra_vde_detach_and_put_dmabuf(secure_attachment, secure_sgt,
+ DMA_TO_DEVICE);
+
tegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment,
bitstream_sgt, DMA_TO_DEVICE);
@@ -1029,6 +1058,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vde);
+ vde->soc = of_device_get_match_data(&pdev->dev);
+
regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sxe");
if (!regs)
return -ENODEV;
@@ -1258,8 +1289,27 @@ static const struct dev_pm_ops tegra_vde_pm_ops = {
tegra_vde_pm_resume)
};
+static const struct tegra_vde_soc tegra20_vde_soc = {
+ .supports_ref_pic_marking = false,
+};
+
+static const struct tegra_vde_soc tegra30_vd
Re: [PATCH 01/14] staging: media: tegra-vde: Support BSEV clock and reset
On Mon, Aug 13, 2018 at 06:09:46PM +0300, Dmitry Osipenko wrote: > On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote: > > From: Thierry Reding > > > > The BSEV clock has a separate gate bit and can not be assumed to be > > always enabled. Add explicit handling for the BSEV clock and reset. > > > > This fixes an issue on Tegra124 where the BSEV clock is not enabled > > by default and therefore accessing the BSEV registers will hang the > > CPU if the BSEV clock is not enabled and the reset not deasserted. > > > > Signed-off-by: Thierry Reding > > --- > > Are you sure that BSEV clock is really needed for T20/30? I've tried already > to disable the clock explicitly and everything kept working, though I'll try > again. I think you're right that these aren't strictly required for VDE to work on Tegra20 and Tegra30. However, the BSEV clock and reset do exist on those platforms, so I didn't see a reason why they shouldn't be handled uniformly across all generations. > The device-tree changes should be reflected in the binding documentation. Indeed, I forgot to update that. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/14] staging: media: tegra-vdea: Add Tegra124 support
On Mon, Sep 03, 2018 at 02:18:15PM +0200, Hans Verkuil wrote: > Hi Thierry, Dmitry, > > Dmitry found some issues, so I'll wait for a v2. > > Anyway, this driver is in staging with this TODO: > > - Implement V4L2 API once it gains support for stateless decoders. > > I just wanted to mention that the Request API is expected to be merged > for 4.20. A topic branch is here: > > https://git.linuxtv.org/media_tree.git/log/?h=request_api > > This patch series is expected to be added to the topic branch once > everyone agrees: > > https://www.spinics.net/lists/linux-media/msg139713.html > > The first Allwinner driver that will be using this API is here: > > https://lwn.net/Articles/763589/ > > It's expected to be merged for 4.20 as well. > > Preliminary H264 work for the Allwinner driver is here: > > https://lkml.org/lkml/2018/6/13/399 > > But this needs more work. > > HEVC support, on the other hand, is almost ready: > > https://lkml.org/lkml/2018/8/28/229 > > I hope these links give a good overview of the current status. Thanks for those links. I was aware of the ongoing efforts and was eagerly waiting for the various pieces to settle a bit. I will hopefully get around to porting the tegra-vde driver to this new infrastructure in the next couple of weeks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: nvec: cleanup USLEEP_RANGEcheckpatch checks
On Thu, Mar 02, 2017 at 03:57:01PM +0100, Marc Dietrich wrote: > Hi Simran, > > Am Donnerstag, 2. März 2017, 15:48:13 CET schrieb SIMRAN SINGHAL: > > On Thursday, March 2, 2017 at 8:06:40 PM UTC+5:30, Julia Lawall wrote: > > > On Thu, 2 Mar 2017, simran singhal wrote: > > > > Resolve strict checkpatch USLEEP_RANGE checks by converting delays and > > > > sleeps as described in ./Documentation/timers/timers-howto.txt. > > > > > > > > CHECK: usleep_range is preferred over udelay; see Documentation/ > > > > timers/timers-howto.txt > > > > > > > > Signed-off-by: simran singhal > > > I prefer not to change this. The whole interrupt routine is very wonky, and > changing some delays might break the communication with the i2c master. Also > this is in interrupt context, so a change to usleep_range may not by > justified. Yeah, I think this is going to trigger a WARN_ON from somewhere in the scheduler because of the interrupt context. I suppose checkpatch could be made smarter about this, though I doubt my perl skills would be up to it. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: nvec: Remove unnecessary cast on void pointer
On Fri, Mar 03, 2017 at 01:45:11AM +0530, simran singhal wrote: > The following Coccinelle script was used to detect this: > > @r@ > expression x; > void* e; > type T; > identifier f; > @@ > ( > *((T *)e) > | > ((T *)x)[...] > | > ((T*)x)->f > | > - (T*) > e > ) > > Signed-off-by: simran singhal > --- > drivers/staging/nvec/nvec_kbd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 0/3] Tegra GPIO: Minor code clean up
On Sun, Dec 15, 2019 at 09:30:44PM +0300, Dmitry Osipenko wrote: > Hello, > > I was investigating why CPU hangs during of GPIO driver suspend and in > the end it turned out that it is a Broadcom WiFi driver problem because > it keeps OOB wake-interrupt enabled while WLAN interface is DOWN and this > may cause a bit weird CPU hang on writing to INT_ENB register during of > GPIO driver suspend. Meanwhile I also noticed that a few things could be > improved in the driver, that's what this small series addresses. > > Dmitry Osipenko (3): > gpio: tegra: Use generic readl_relaxed/writel_relaxed accessors > gpio: tegra: Properly handle irq_set_irq_wake() error > gpio: tegra: Use NOIRQ phase for suspend/resume > > drivers/gpio/gpio-tegra.c | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) Patches look good: Reviewed-by: Thierry Reding I also applied this series on top of v5.5-rc1 and ran it through our test system: Test results: 13 builds: 13 pass, 0 fail 22 boots: 22 pass, 0 fail 34 tests: 34 pass, 0 fail Linux version: 5.5.0-rc1-g3d0b4fced39e Boards tested: tegra124-jetson-tk1, tegra186-p2771-, tegra194-p2972-, tegra20-ventana, tegra210-p2371-2180, tegra30-cardhu-a04 All tests passing, so: Tested-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] media: tegra: Make tegra210_video_formats static
On Mon, May 11, 2020 at 07:20:15PM +0800, Samuel Zou wrote:
> Fix the following sparse warning:
>
> drivers/staging/media/tegra-video/tegra210.c:589:33: warning: symbol
> 'tegra210_video_formats' was not declared.
>
> The tegra210_video_formats has only call site within tegra210.c
> It should be static
>
> Fixes: 423d10a99b30 ("media: tegra: Add Tegra210 Video input driver")
> Reported-by: Hulk Robot
> Signed-off-by: Samuel Zou
> ---
> drivers/staging/media/tegra-video/tegra210.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops
On Fri, Feb 07, 2014 at 04:55:00PM +0100, Jean-Francois Moine wrote:
> Some simple components don't need to do any specific action on
> bind to / unbind from a master component.
>
> This patch permits such components to omit the bind/unbind
> operations.
>
> Signed-off-by: Jean-Francois Moine
> ---
> drivers/base/component.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index c53efe6..0a39d7a 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -225,7 +225,8 @@ static void component_unbind(struct component *component,
> {
> WARN_ON(!component->bound);
>
> - component->ops->unbind(component->dev, master->dev, data);
> + if (component->ops)
> + component->ops->unbind(component->dev, master->dev, data);
This doesn't actually do what the commit message says. This makes
component->ops optional, not component->ops->unbind().
A more correct check would be:
if (component->ops && component->ops->unbind)
> component->bound = false;
>
> /* Release all resources claimed in the binding of this component */
> @@ -274,7 +275,11 @@ static int component_bind(struct component *component,
> struct master *master,
> dev_dbg(master->dev, "binding %s (ops %ps)\n",
> dev_name(component->dev), component->ops);
>
> - ret = component->ops->bind(component->dev, master->dev, data);
> + if (component->ops)
> + ret = component->ops->bind(component->dev, master->dev, data);
Same here.
Thierry
pgpMPGzPZV4wd.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12][ 09/12] drm/panel: Add Eukrea mbimxsd51 displays.
On Mon, Apr 07, 2014 at 02:44:48PM +0200, Denis Carikli wrote:
[...]
> +static const struct panel_desc eukrea_mbimxsd51_dvisvga = {
> + .modes = &eukrea_mbimxsd51_dvisvga_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 0,
> + .height = 0,
> + },
> +};
[...]
> +static const struct panel_desc eukrea_mbimxsd51_dvivga = {
> + .modes = &eukrea_mbimxsd51_dvivga_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 0,
> + .height = 0,
> + },
> +};
Surely these two panels have a physical size?
Thierry
pgpbZhZTg_TsE.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: fsl-mc: Do not allow building as a module
From: Thierry Reding This driver uses functionality (MSI IRQ domain) whose symbols aren't exported, and hence the modular build fails. While arguably there might be reasons to make these symbols available to modules, that change would be fairly involved and the set of exported functions should be carefully auditioned. Fix the build failure for now by marking the driver boolean. Cc: J. German Rivera Cc: Greg Kroah-Hartman Signed-off-by: Thierry Reding --- drivers/staging/fsl-mc/bus/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig index c498ac6a72d5..1f959339c671 100644 --- a/drivers/staging/fsl-mc/bus/Kconfig +++ b/drivers/staging/fsl-mc/bus/Kconfig @@ -7,7 +7,7 @@ # config FSL_MC_BUS - tristate "Freescale Management Complex (MC) bus driver" + bool "Freescale Management Complex (MC) bus driver" depends on OF && ARM64 select GENERIC_MSI_IRQ_DOMAIN help -- 2.7.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: fsl-mc: Avoid section mismatch
From: Thierry Reding
The fsl_mc_allocator_driver_exit() function is marked __exit, but is
called by the error handling code in fsl_mc_allocator_driver_init().
This results in a section mismatch, which in turn could lead to
executing random code.
Remove the __exit annotation to fix this.
Cc: J. German Rivera
Cc: Greg Kroah-Hartman
Signed-off-by: Thierry Reding
---
drivers/staging/fsl-mc/bus/mc-allocator.c | 2 +-
drivers/staging/fsl-mc/include/mc-private.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/fsl-mc/bus/mc-allocator.c
b/drivers/staging/fsl-mc/bus/mc-allocator.c
index c5fa628411ec..86f8543c2b9a 100644
--- a/drivers/staging/fsl-mc/bus/mc-allocator.c
+++ b/drivers/staging/fsl-mc/bus/mc-allocator.c
@@ -756,7 +756,7 @@ int __init fsl_mc_allocator_driver_init(void)
return fsl_mc_driver_register(&fsl_mc_allocator_driver);
}
-void __exit fsl_mc_allocator_driver_exit(void)
+void fsl_mc_allocator_driver_exit(void)
{
fsl_mc_driver_unregister(&fsl_mc_allocator_driver);
}
diff --git a/drivers/staging/fsl-mc/include/mc-private.h
b/drivers/staging/fsl-mc/include/mc-private.h
index be72a44ba064..ee5f1d2bf604 100644
--- a/drivers/staging/fsl-mc/include/mc-private.h
+++ b/drivers/staging/fsl-mc/include/mc-private.h
@@ -123,7 +123,7 @@ void dprc_driver_exit(void);
int __init fsl_mc_allocator_driver_init(void);
-void __exit fsl_mc_allocator_driver_exit(void);
+void fsl_mc_allocator_driver_exit(void);
int __must_check fsl_mc_resource_allocate(struct fsl_mc_bus *mc_bus,
enum fsl_mc_pool_type pool_type,
--
2.7.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: fsl-mc: Do not allow building as a module
On Mon, Feb 15, 2016 at 02:22:22PM +0100, Thierry Reding wrote: > From: Thierry Reding > > This driver uses functionality (MSI IRQ domain) whose symbols aren't > exported, and hence the modular build fails. While arguably there might > be reasons to make these symbols available to modules, that change would > be fairly involved and the set of exported functions should be carefully > auditioned. Fix the build failure for now by marking the driver boolean. > > Cc: J. German Rivera > Cc: Greg Kroah-Hartman > Signed-off-by: Thierry Reding > --- > drivers/staging/fsl-mc/bus/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Sorry for being pushy, but this has had allmodconfig builds broken in linux-next for a couple of days now. The driver has been marked BROKEN now in linux-next to remedy that, but having proper fixes seems better than that. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] pwm: Remove .can_sleep from struct pwm_chip
All PWM devices have been marked as "might sleep" since v4.5, there is
no longer a need to differentiate on a per-chip basis.
Signed-off-by: Thierry Reding
---
drivers/pwm/pwm-atmel-hlcdc.c | 1 -
drivers/pwm/pwm-atmel.c | 1 -
drivers/pwm/pwm-bcm-kona.c| 1 -
drivers/pwm/pwm-berlin.c | 1 -
drivers/pwm/pwm-brcmstb.c | 1 -
drivers/pwm/pwm-fsl-ftm.c | 1 -
drivers/pwm/pwm-imx.c | 1 -
drivers/pwm/pwm-lp3943.c | 1 -
drivers/pwm/pwm-mxs.c | 2 +-
drivers/pwm/pwm-pca9685.c | 1 -
drivers/pwm/pwm-sti.c | 1 -
drivers/pwm/pwm-sun4i.c | 1 -
drivers/pwm/pwm-twl-led.c | 1 -
drivers/pwm/pwm-twl.c | 1 -
drivers/staging/greybus/pwm.c | 1 -
include/linux/pwm.h | 3 ---
16 files changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index 14fc011faa32..999187277ea5 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -270,7 +270,6 @@ static int atmel_hlcdc_pwm_probe(struct platform_device
*pdev)
chip->chip.npwm = 1;
chip->chip.of_xlate = of_pwm_xlate_with_flags;
chip->chip.of_pwm_n_cells = 3;
- chip->chip.can_sleep = 1;
ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED);
if (ret) {
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index e6b8b1b7e6ba..67a7023be5c2 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -385,7 +385,6 @@ static int atmel_pwm_probe(struct platform_device *pdev)
atmel_pwm->chip.base = -1;
atmel_pwm->chip.npwm = 4;
- atmel_pwm->chip.can_sleep = true;
atmel_pwm->config = data->config;
atmel_pwm->updated_pwms = 0;
mutex_init(&atmel_pwm->isr_lock);
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c63418322023..09a95aeb3a70 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -276,7 +276,6 @@ static int kona_pwmc_probe(struct platform_device *pdev)
kp->chip.npwm = 6;
kp->chip.of_xlate = of_pwm_xlate_with_flags;
kp->chip.of_pwm_n_cells = 3;
- kp->chip.can_sleep = true;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
kp->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 01339c152ab0..771859aca4be 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -206,7 +206,6 @@ static int berlin_pwm_probe(struct platform_device *pdev)
pwm->chip.ops = &berlin_pwm_ops;
pwm->chip.base = -1;
pwm->chip.npwm = 4;
- pwm->chip.can_sleep = true;
pwm->chip.of_xlate = of_pwm_xlate_with_flags;
pwm->chip.of_pwm_n_cells = 3;
diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 5d5adee16886..8063cffa1c96 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -270,7 +270,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
p->chip.ops = &brcmstb_pwm_ops;
p->chip.base = -1;
p->chip.npwm = 2;
- p->chip.can_sleep = true;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
p->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index fad968eb75f6..557b4ea16796 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -446,7 +446,6 @@ static int fsl_pwm_probe(struct platform_device *pdev)
fpc->chip.of_pwm_n_cells = 3;
fpc->chip.base = -1;
fpc->chip.npwm = 8;
- fpc->chip.can_sleep = true;
ret = pwmchip_add(&fpc->chip);
if (ret < 0) {
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5cd4ba..1223187ad354 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -304,7 +304,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
imx->chip.dev = &pdev->dev;
imx->chip.base = -1;
imx->chip.npwm = 1;
- imx->chip.can_sleep = true;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
diff --git a/drivers/pwm/pwm-lp3943.c b/drivers/pwm/pwm-lp3943.c
index 872ea76a4f19..52584e9962ed 100644
--- a/drivers/pwm/pwm-lp3943.c
+++ b/drivers/pwm/pwm-lp3943.c
@@ -278,7 +278,6 @@ static int lp3943_pwm_probe(struct platform_device *pdev)
lp3943_pwm->chip.dev = &pdev->dev;
lp3943_pwm->chip.ops = &lp3943_pwm_ops;
lp3943_pwm->chip.npwm = LP3943_NUM_PWMS;
- lp3943_pwm->chip.can_sleep = true;
platform_set_drvdata(pdev, lp3943_pwm);
diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index 9a596324ebef..
[PATCH 1/2] pwm: Remove pwm_can_sleep()
The last user of this function has been removed, so it is no longer
needed.
Signed-off-by: Thierry Reding
---
drivers/pwm/core.c | 12
include/linux/pwm.h | 7 ---
2 files changed, 19 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 172ef8245811..78e114a11c4f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -960,18 +960,6 @@ void devm_pwm_put(struct device *dev, struct pwm_device
*pwm)
}
EXPORT_SYMBOL_GPL(devm_pwm_put);
-/**
- * pwm_can_sleep() - report whether PWM access will sleep
- * @pwm: PWM device
- *
- * Returns: True if accessing the PWM can sleep, false otherwise.
- */
-bool pwm_can_sleep(struct pwm_device *pwm)
-{
- return true;
-}
-EXPORT_SYMBOL_GPL(pwm_can_sleep);
-
#ifdef CONFIG_DEBUG_FS
static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
{
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2c6c5114c089..e15fd3ce6502 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -451,8 +451,6 @@ struct pwm_device *devm_pwm_get(struct device *dev, const
char *con_id);
struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
const char *con_id);
void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
-
-bool pwm_can_sleep(struct pwm_device *pwm);
#else
static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
{
@@ -566,11 +564,6 @@ static inline struct pwm_device *devm_of_pwm_get(struct
device *dev,
static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
{
}
-
-static inline bool pwm_can_sleep(struct pwm_device *pwm)
-{
- return false;
-}
#endif
static inline void pwm_apply_args(struct pwm_device *pwm)
--
2.11.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] [media] staging: media: Fix build for iMON LIRC driver
Add a missing , between parameters in a call to dev_info().
Signed-off-by: Thierry Reding
---
drivers/staging/media/lirc/lirc_imon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/lirc/lirc_imon.c
b/drivers/staging/media/lirc/lirc_imon.c
index 8180187..91c3e2d 100644
--- a/drivers/staging/media/lirc/lirc_imon.c
+++ b/drivers/staging/media/lirc/lirc_imon.c
@@ -925,7 +925,7 @@ static int imon_probe(struct usb_interface *interface,
if (usb_register_dev(interface, &imon_class)) {
/* Not a fatal error, so ignore */
dev_info(dev,
- "%s: could not get a minor number for display\n"
+ "%s: could not get a minor number for display\n",
__func__);
}
}
--
1.8.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 21/31] staging: nvec: use reset framework
On Fri, Nov 15, 2013 at 01:54:16PM -0700, Stephen Warren wrote: > From: Stephen Warren > > Tegra's clock driver now provides an implementation of the common > reset API (include/linux/reset.h). Use this instead of the old Tegra- > specific API; that will soon be removed. > > Cc: tred...@nvidia.com > Cc: pdeschrij...@nvidia.com > Cc: linux-te...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: Julian Andres Klode > Cc: Marc Dietrich > Cc: ac...@lists.launchpad.net > Cc: Greg Kroah-Hartman > Cc: de...@driverdev.osuosl.org > Signed-off-by: Stephen Warren > --- > This patch is part of a series with strong internal depdendencies. I'm > looking for an ack so that I can take the entire series through the Tegra > and arm-soc trees. The series will be part of a stable branch that can be > merged into other subsystems if needed to avoid/resolve dependencies. > --- > drivers/staging/nvec/nvec.c | 11 --- > drivers/staging/nvec/nvec.h | 5 - > 2 files changed, 12 insertions(+), 4 deletions(-) Reviewed-by: Thierry Reding pgpJY2PXFUGX6.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv5][ 3/8] staging: imx-drm: Correct BGR666 and the board's dts that use them.
On Thu, Dec 05, 2013 at 07:28:07PM +0100, Denis Carikli wrote:
[...]
> diff --git a/arch/arm/boot/dts/imx51-apf51dev.dts
> b/arch/arm/boot/dts/imx51-apf51dev.dts
> index f36a3aa..3b6de6a 100644
> --- a/arch/arm/boot/dts/imx51-apf51dev.dts
> +++ b/arch/arm/boot/dts/imx51-apf51dev.dts
> @@ -19,7 +19,7 @@
> display@di1 {
I know this isn't introduced by your patch, but WTF???
Thierry
pgpeRNOPkx7Jk.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv5][ 2/8] staging: imx-drm: Add RGB666 support for parallel display.
On Thu, Dec 05, 2013 at 07:28:06PM +0100, Denis Carikli wrote: [...] > diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c > b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c [...] > @@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt) > return IPU_DC_MAP_BGR666; > case V4L2_PIX_FMT_BGR24: > return IPU_DC_MAP_BGR24; > + case V4L2_PIX_FMT_RGB666: > + return IPU_DC_MAP_RGB666; Why is this DRM driver even using V4L2 pixel formats in the first place? Thierry pgpT_HOy9ANzp.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv5][ 5/8] staging: imx-drm: parallel display: add regulator support.
On Thu, Dec 05, 2013 at 07:28:09PM +0100, Denis Carikli wrote: [...] > diff --git a/drivers/staging/imx-drm/parallel-display.c > b/drivers/staging/imx-drm/parallel-display.c [...] > @@ -260,6 +275,13 @@ static int imx_pd_probe(struct platform_device *pdev) > if (ret) > return ret; > > + imxpd->disp_reg = devm_regulator_get(&pdev->dev, "display"); > + if (PTR_ERR(imxpd->disp_reg) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + if (IS_ERR(imxpd->disp_reg)) > + dev_dbg(&pdev->dev, "Operating without display regulator.\n"); I don't think this is necessary. There is code in the regulator core nowadays that supplies a dummy regulator if one hasn't been hooked up in devicetree explicitly. So any error that you get at this point is likely a valid one rather than just a missing regulator. The advantage is that you no longer have to check at every step of the way that the regulator is valid before calling the regulator API. Thierry pgpOzaX7uYTJz.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv5][ 2/8] staging: imx-drm: Add RGB666 support for parallel display.
On Fri, Dec 06, 2013 at 02:29:22PM +0100, Lucas Stach wrote: > Am Freitag, den 06.12.2013, 14:14 +0100 schrieb Thierry Reding: > > On Thu, Dec 05, 2013 at 07:28:06PM +0100, Denis Carikli wrote: > > [...] > > > diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c > > > b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c > > [...] > > > @@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt) > > > return IPU_DC_MAP_BGR666; > > > case V4L2_PIX_FMT_BGR24: > > > return IPU_DC_MAP_BGR24; > > > + case V4L2_PIX_FMT_RGB666: > > > + return IPU_DC_MAP_RGB666; > > > > Why is this DRM driver even using V4L2 pixel formats in the first place? > > > Because imx-drm is actually a misnomer. The i.MX IPU is a multifunction > device, which as one part has the display controllers, but also camera > interfaces and mem-to-mem scaler devices, which are hooked up via the > V4L2 interface. > > The generic IPU part, which is used for example for programming the DMA > channels is using V4L2 pixel formats as a common base. We have patches > to split this out and make this fact more visible. (The IPU core will be > placed aside the Tegra host1x driver) Have you considered splitting thing up further and move out the display controller driver to DRM and the camera driver to V4L2? I mean, if that is even possible with a reasonable amount of work. Is the "mem-to-mem" the same as the "DMA channels" you mentioned? If it only does DMA, why does it even need to worry about pixel formats? Thierry pgpEc5WPsAN7a.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 0/8] component helper improvements
On Sun, Apr 27, 2014 at 12:00:25AM +0100, Russell King - ARM Linux wrote:
> A while back, Laurent raised some comments about the component helper,
> which this patch set starts to address.
>
> The first point it addresses is the repeated parsing inefficiency when
> deferred probing occurs. When DT is used, the structure of the
> component helper today means that masters end up parsing the device
> tree for each attempt to re-bind the driver.
>
> We remove this inefficiency by creating an array of matching data and
> functions, which the component helper can use internally to match up
> components to their master.
>
> The second point was the inefficiency of destroying the list of
> components each time we saw a failure. We did this to ensure that
> we kept things correctly ordered: component bind order matters.
> As we have an array instead, the array is already ordered, so we
> use this array to store the component pointers instead of a list,
> and remember which are duplicates (and thus should be avoided.)
> Avoiding the right duplicates matters as we walk the array in the
> opposite direction at tear down.
I've been looking at converting the Tegra DRM driver to the component
helpers for a while now and had to make some changes to make it work for
that particular use-case. While updating the imx-drm and msm DRM drivers
for those changes I noticed an oddity. Both of the existing drivers use
the following pattern:
static int driver_component_bind(struct device *dev,
struct device *master,
void *data)
{
allocate memory
request resources
...
hook up to subsystem
...
enable hardware
}
static const struct component_ops driver_component_ops = {
.bind = driver_component_bind,
};
static int driver_probe(struct platform_device *pdev)
{
return component_add(&pdev->dev, &driver_component_ops);
}
While converting Tegra DRM, what I intuitively did (I didn't actually
look at the other drivers for inspiration) was something more along the
lines of the following:
static int driver_component_bind(struct device *dev,
struct device *master,
void *data)
{
hook up to subsystem
...
enable hardware
}
static const struct component_ops driver_component_ops = {
.bind = driver_component_bind,
};
static int driver_probe(struct platform_device *pdev)
{
allocate memory
request resources
...
return component_add(&pdev->dev, &driver_component_ops);
}
Since usually deferred probing is caused by resource allocations failing
this has the side-effect of handling deferred probing before the master
device is even bound (the component_add() happens as the very last step)
and therefore there is less risk for component_bind_all() to fail. I've
actually never seen it fail at all. Failure at that point is almost
certainly irrecoverable anyway.
It would seem to me that if other drivers followed the same pattern, the
second point above is solved by moving deferred probe handling one level
up and reduce the work of the component helpers to gluing together the
components on a subsystem level.
Another advantage to that pattern is that probe failure happens on a
much more granular level. It's handled by each component device rather
than all at once when the master is bound. By that time all components
will be ready and the heavy work of building the subsystem device will
usually not have to be undone as opposed to the former pattern where
that process is bound to be interrupted possibly many times be deferred
probing.
But perhaps I'm missing something. Was there another reason for choosing
this particular pattern?
Thierry
pgp4KMERDKRcu.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-drm: imx-hdmi: move memory and resource allocation into probe function
On Wed, May 14, 2014 at 11:24:14PM +0200, Philipp Zabel wrote:
> Move memory allocation and resource acquisition from the bind function into
> the probe function. This calls the devres managed functions once instead of
> possibly multiple times in the bind function and avoids leaking memory (as
> long as the hdmi platform device stays bound).
>
> While at it, request the irq only after the interrupt handler mutes are
> set up, to avoid spurious interrupts.
>
> Signed-off-by: Philipp Zabel
> ---
> drivers/staging/imx-drm/imx-hdmi.c | 163
> +++--
> 1 file changed, 83 insertions(+), 80 deletions(-)
Very nice. I like it. One comment below, though it's unrelated to what
you're trying to achieve here. It's primarily a note to myself.
> diff --git a/drivers/staging/imx-drm/imx-hdmi.c
> b/drivers/staging/imx-drm/imx-hdmi.c
[...]
> static int imx_hdmi_platform_probe(struct platform_device *pdev)
> {
[...]
> + ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
> + if (ddc_node) {
> + hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
> + if (!hdmi->ddc)
> + dev_dbg(hdmi->dev, "failed to read ddc node\n");
> +
> + of_node_put(ddc_node);
> + } else {
> + dev_dbg(hdmi->dev, "no ddc property found\n");
> + }
This seems to be emerging as a common pattern. Perhaps we should add a
common helper for this.
Thierry
pgpoaXVvRrJZX.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.
On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote: [...] > diff --git > a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt > b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt [...] > @@ -0,0 +1,7 @@ > +Eukrea DVI-SVGA (800x600 pixels) DVI output. [...] > diff --git > a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt > b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt [...] > @@ -0,0 +1,7 @@ > +Eukrea DVI-VGA (640x480 pixels) DVI output. DVI outputs shouldn't be using the panel framework and this binding at all. DVI usually has the means to determine all of this by itself. Why do you need to represent this as a panel in device tree? Thierry pgpidfDq8T7xh.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.
On Tue, Jun 24, 2014 at 02:52:11PM -0500, Rob Herring wrote:
> On Tue, Jun 24, 2014 at 10:06 AM, Russell King - ARM Linux
> wrote:
[...]
> > On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote:
[...]
> >> diff --git
> >> a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt
> >> b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt
[...]
> >> @@ -0,0 +1,7 @@
> >> +Eukrea DVI-VGA (640x480 pixels) DVI output.
> >> +
> >> +Required properties:
> >> +- compatible: should be "eukrea,mbimxsd51-dvi-vga"
> >> +
> >> +This binding is compatible with the simple-panel binding, which is
> >> specified
> >> +in simple-panel.txt in this directory.
>
> Seems like we could just have a list of compatible strings rather than
> a mostly duplicated file.
We've been doing it this way for all other panels.
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> >> b/drivers/gpu/drm/panel/panel-simple.c
> >> index a251361..adc40a7 100644
> >> --- a/drivers/gpu/drm/panel/panel-simple.c
> >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> >> @@ -403,6 +403,80 @@ static const struct panel_desc edt_etm0700g0dh6 = {
> >> },
> >> };
> >>
> >> +static const struct drm_display_mode eukrea_mbimxsd51_cmoqvga_mode = {
> >> + .clock = 6500,
> >> + .hdisplay = 320,
> >> + .hsync_start = 320 + 38,
> >> + .hsync_end = 320 + 38 + 20,
> >> + .htotal = 320 + 38 + 20 + 30,
> >> + .vdisplay = 240,
> >> + .vsync_start = 240 + 15,
> >> + .vsync_end = 240 + 15 + 4,
> >> + .vtotal = 240 + 15 + 4 + 3,
> >> + .vrefresh = 60,
> >> + .pol_flags = DRM_MODE_FLAG_POL_PIXDATA_NEGEDGE |
> >> + DRM_MODE_FLAG_POL_DE_LOW,
>
> Why aren't you using:
>
> Documentation/devicetree/bindings/video/display-timing.txt
Because it's redundant information. We need to have a compatible for the
panel in the device tree anyway and that already implicitly defines the
display mode.
Thierry
pgpAUDrLB6ar1.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.
On Tue, Jun 24, 2014 at 11:56:39PM +0200, Eric Bénard wrote: > Hi Thierry, > > Le Tue, 24 Jun 2014 23:49:37 +0200, > Thierry Reding a écrit : > > > On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote: > > [...] > > > diff --git > > > a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt > > > b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt > > [...] > > > @@ -0,0 +1,7 @@ > > > +Eukrea DVI-SVGA (800x600 pixels) DVI output. > > [...] > > > diff --git > > > a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt > > > b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt > > [...] > > > @@ -0,0 +1,7 @@ > > > +Eukrea DVI-VGA (640x480 pixels) DVI output. > > > > DVI outputs shouldn't be using the panel framework and this binding at > > all. DVI usually has the means to determine all of this by itself. Why > > do you need to represent this as a panel in device tree? > > > because on this very simple display board, we only have DVI LVDS signals > without the I2C to detect the display. That's unfortunate. In that case perhaps a better approach would be to add a video timings node to the device that provides the DVI output? The panel bindings are really for internal panels and should define all of their properties. That's also why they need a specific compatible string. What the above two bindings define are really "connectors" with a fixed resolution rather than panels. Thierry pgpMPKP72dOIG.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 06/10] drm: drm_display_mode: add signal polarity flags
On Tue, Jun 24, 2014 at 03:57:46PM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 16, 2014 at 12:11:20PM +0200, Denis Carikli wrote: > > We need a way to pass signal polarity informations > > between DRM panels, and the display drivers. > > > > To do that, a pol_flags field was added to drm_display_mode. > > > > Signed-off-by: Denis Carikli > > This patch needs an ack from the DRM people - can someone review it > please? This series has now been round 14 revisions and it's about > time it was properly reviewed - or a statement made if it's > unacceptable. I didn't follow all of the earlier discussions around this, but it seems to me like data-enable polarity and the pixel data edge flags are properties of the interface rather than the video mode. struct drm_display_mode represents the video timings and I'm not sure if it's a good idea to extend it with this type of information. Maybe we need to add a separate type of device to store these parameters (much like we've done for MIPI DSI devices). Thierry pgpJLEApSbzpD.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: nvec: remove unnecessary 'else' after 'return' statement
On Sat, Jul 05, 2014 at 10:30:55PM +0200, Pawel Lebioda wrote: > Fix the following warning reported by checkpatch.pl: > > WARNING: else is not generally useful after a break or return > 235: FILE: drivers/staging/nvec/nvec.c:235 > > Signed-off-by: Pawel Lebioda > --- > drivers/staging/nvec/nvec.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Thierry Reding pgp74td0RBKxI.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: nvec: remove unneccessary 'out of memory' message
On Sat, Jul 05, 2014 at 10:30:56PM +0200, Pawel Lebioda wrote: > Fix the following warning reported by checkpatch.pl: > > WARNING: Possible unnecessary 'out of memory' message > 811: FILE: drivers/staging/nvec/nvec.c:811 > > Signed-off-by: Pawel Lebioda > --- > drivers/staging/nvec/nvec.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Thierry Reding pgpcKewibAJKl.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
On Sun, Jul 13, 2014 at 12:22:02PM -0700, Greg Kroah-Hartman wrote: > On Sun, Jul 13, 2014 at 04:25:06PM +0200, Lars-Peter Clausen wrote: > > On 07/13/2014 04:03 PM, Richard Weinberger wrote: > > >Am 13.07.2014 15:56, schrieb Lars-Peter Clausen: > > >>On 07/13/2014 03:40 PM, Richard Weinberger wrote: > > >>>Am 13.07.2014 15:26, schrieb Lars-Peter Clausen: > > On 07/13/2014 11:45 AM, Richard Weinberger wrote: > > >Am 13.07.2014 11:27, schrieb Lennox Wu: > > >>As I said before, some configurations don't make sense. > > > > > >If such a configuration can be achieved using allmod/yesconfig it has > > >to be fixed. > > >Chen's fixes seem reasonable as not all architectures support iomem. > > > > Maybe we should stub out ioremap() and friends when COMPILE_TEST is > > enabled to avoid these linker errors. That's in my opinion better than > > turning most of the 'depends on > > COMPILE_TEST' into 'depends on COMPILE_TEST && HAS_IOMEM'. The issue > > comes up quite a lot and it is often overlooked when adding a driver > > that can be build when COMPILE_TEST is > > enabled. > > >>> > > >>>And what should this stub do? > > >>>Except calling BUG()... > > >> > > >>return NULL; > > >> > > >>It's for compile testing, it's not meant to work at runtime. > > > > > >Hm, I really don't like the idea of having a non-working kernel. > > >IMHO either it should build _and_ run and nothing else. > > >Greg, what do you think? > > > > The kernel will still be working fine and you can run it on a system. The > > drivers which use ioremap() or similar are probably not instantiated on a > > system that does not provide HAS_IOMEM. But even if it was the driver should > > handle ioremap() returning NULL gracefully and abort the driver probe. That > > said you'll probably not run a kernel that was built with COMPILE_TEST on > > your real hardware since it contains so many drivers that are completely > > useless on your hardware. The idea of COMPILE_TEST is to have as much > > compile test exposure as possible to the code that is enabled by > > COMPILE_TEST. Stubbing out ioremap() and friends when HAS_IOMEM is not set > > and COMPILE_TEST is set makes it easier to get there. > > I run my kernels with COMPILE_TEST enabled as I need to build test > things that I don't happen to use. > > I like the 'return NULL' option for this, it hits us all the time, might > as well fix it properly like this so that we don't have to deal with > Kconfig changes everywhere. I agree. One nit, though: devm_ioremap_resource() returns an ERR_PTR()- encoded error code, so the dummy should probably be returning something like ERR_PTR(-ENOSYS) instead of NULL. > Also put a big "This platform does not support IOMEM" error printk in > there, so that people have a chance to figure out what is going on if > they happen to run such a driver on a platform that can't support it. Yes, that sounds like a very good idea and should be indication enough of what exactly has gone wrong. Thierry pgpQEbFxoZ04h.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
On Thu, Jul 17, 2014 at 09:27:58AM +0800, Chen Gang wrote:
[...]
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c2421e0..a7500c3 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -630,7 +630,16 @@ extern unsigned long devm_get_free_pages(struct device
> *dev,
>gfp_t gfp_mask, unsigned int order);
> extern void devm_free_pages(struct device *dev, unsigned long addr);
>
> +#ifdef CONFIG_HAS_IOMEM
> void __iomem *devm_ioremap_resource(struct device *dev, struct resource
> *res);
> +#elif defined(CONFIG_COMPILE_TEST)
> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
> + struct resource *res)
> +{
> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
Maybe: "Hardware doesn't support memory-mapped I/O"? I'm not sure if
it's useful to include the reference to COMPILE_TEST, especially since
the #elif will be dropped in favour of a simple #else.
> + return (__force void __iomem *)ERR_PTR(-ENXIO);
There's apparently an IOMEM_ERR_PTR() for this nowadays...
> +}
> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
>
> /* allows to add/remove a custom action to devres stack */
> int devm_add_action(struct device *dev, void (*action)(void *), void *data);
> diff --git a/include/linux/io.h b/include/linux/io.h
> index b76e6e5..59128aa 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -58,14 +58,42 @@ static inline void devm_ioport_unmap(struct device *dev,
> void __iomem *addr)
> }
> #endif
>
> +#ifdef CONFIG_HAS_IOMEM
> +
> void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> unsigned long size);
> void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t
> offset,
> unsigned long size);
> void devm_iounmap(struct device *dev, void __iomem *addr);
> +void devm_ioremap_release(struct device *dev, void *res);
> +
> +#elif defined(CONFIG_COMPILE_TEST)
> +
> +static inline void __iomem *devm_ioremap(struct device *dev,
> + resource_size_t offset, unsigned long size)
For consistency with other functions above, please keep arguments on
subsequent lines aligned with the first argument on the first line, like
so:
static inline void __iomem *devm_ioremap(struct device *dev,
resource_size_t offset,
unsigned long size)
> +{
> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
> + return NULL;
> +}
> +static inline void __iomem *devm_ioremap_nocache(struct device *dev,
> + resource_size_t offset, unsigned long size)
> +{
> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
> + return NULL;
> +}
Perhaps this should call devm_ioremap() so we don't have to repeat the
same error message? Or maybe make it:
#define devm_ioremap_nocache devm_ioremap
?
Thierry
pgpd5bRIZqIRk.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
On Thu, Jul 17, 2014 at 05:29:31PM +0800, Chen Gang wrote:
>
>
> On 07/17/2014 05:20 PM, Arnd Bergmann wrote:
> > On Thursday 17 July 2014 09:27:58 Chen Gang wrote:
> >> gfp_t gfp_mask, unsigned int
> >> order);
> >> extern void devm_free_pages(struct device *dev, unsigned long addr);
> >>
> >> +#ifdef CONFIG_HAS_IOMEM
> >> void __iomem *devm_ioremap_resource(struct device *dev, struct resource
> >> *res);
> >> +#elif defined(CONFIG_COMPILE_TEST)
> >> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
> >> + struct resource *res)
> >> +{
> >> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
> >> + return (__force void __iomem *)ERR_PTR(-ENXIO);
> >> +}
> >> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
> >>
> >> /* allows to add/remove a custom action to devres stack */
> >
> > To be honest, I think it's a bad idea to introduce wrappers functions
> > that are only available when CONFIG_COMPILE_TEST is set.
> >
> > COMPILE_TEST is a great tool in general, but it has its limits.
> > In particular, the case for !CONFIG_IOMEM is completely obscure
> > and we won't find any bugs by allowing more drivers to be built
> > in those configurations, but attempting to do it would cause
> > endless churn by changing each instance of 'depends on HAS_IOMEM'
> > to 'depends on HAS_IOMEM || COMPILE_TEST'.
> >
>
> Architecture members and driver members really have different tastes,
> they are different roles. It really need additional discussion.
>
> For me, I only want to change devm_io*map*, not touch so much.
>
> Welcome any other members' idea or suggestions.
>
> > Note that s390 no has gained support for IOMEM, tile has it most
> > of the time (when PCI is enabled, so you get it in half the
> > test builds already), score should set HAS_IOMEM and doesn't
> > even have public compilers, and uml doesn't even compile in
> > latest mainline. Nothing else ever sets NO_IOMEM.
> >
>
> In latest gcc and binutils, can compile score cross compiler
> successfully for building kernel (but I am not quite sure whether the
> compiling result are really OK, but I guess so).
I was only able to ever build a partial bare-metal toolchain for score.
There's no uClibc, glibc or newlib support, so it becomes rather useless
as a Linux architecture.
Also when I run the cross-compiler on a score defconfig build, I get a
bunch of these:
score-unknown-elf-gcc-4.9.0: internal compiler error: Segmentation
fault (program as)
Thierry
pgpwGF6puxSqC.pgp
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
On Thu, Jul 17, 2014 at 11:20:36AM +0200, Arnd Bergmann wrote: [...] > score should set HAS_IOMEM and doesn't > even have public compilers This begs an interesting question. Should it be made a requirement to have publicly available compilers for new architectures so that they can at least be compile-tested? Preferably this would of course be in source form so that there aren't any dependencies on the distribution. Thierry pgpL5cRJmfsX7.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
On Thu, Jul 17, 2014 at 12:33:32PM +0200, Arnd Bergmann wrote: > On Thursday 17 July 2014 11:56:58 Thierry Reding wrote: > > On Thu, Jul 17, 2014 at 11:20:36AM +0200, Arnd Bergmann wrote: > > [...] > > > score should set HAS_IOMEM and doesn't > > > even have public compilers > > > > This begs an interesting question. Should it be made a requirement to > > have publicly available compilers for new architectures so that they can > > at least be compile-tested? Preferably this would of course be in source > > form so that there aren't any dependencies on the distribution. > > The question has come up a few times. I wouldn't mandate that the port > has an upstream gcc (you've got to start mainlining one of them first > after all), but having compilers available for download should probably be > required. It's hard to ask for a particular quality of that gcc port > though, or to expect it to stay available online. > > Where did you find the gcc port for score? It's upstream, though marked obsolete and to be removed in the next release... =) Thierry pgp2_qLMI3Awb.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging/nvec: Pass proper resource to mfd_add_devices()
From: Thierry Reding
The mfd_add_devices() parameter takes a struct resource * as fifth
argument, but the nvec driver passes in a void __iomem *. The driver
gets away with it because none of the subdevices ever directly access
the registers.
While at it, use platform_get_irq() instead of platform_get_resource()
to get the device's interrupt. This makes it easier to pass in the
register region since the variable is no longer reused.
Signed-off-by: Thierry Reding
---
Alternatively we could simply pass NULL into mfd_add_devices(), which
might be a slightly more accurate representation of what's going on.
Marc, Greg, any preferences?
drivers/staging/nvec/nvec.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index d32504844896..11f9e1c3447c 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -821,8 +821,8 @@ static int tegra_nvec_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res) {
+ nvec->irq = platform_get_irq(pdev, 0);
+ if (nvec->irq < 0) {
dev_err(&pdev->dev, "no irq resource?\n");
return -ENODEV;
}
@@ -840,7 +840,6 @@ static int tegra_nvec_probe(struct platform_device *pdev)
}
nvec->base = base;
- nvec->irq = res->start;
nvec->i2c_clk = i2c_clk;
nvec->rx = &nvec->msg_pool[0];
@@ -893,7 +892,7 @@ static int tegra_nvec_probe(struct platform_device *pdev)
}
ret = mfd_add_devices(nvec->dev, -1, nvec_devices,
- ARRAY_SIZE(nvec_devices), base, 0, NULL);
+ ARRAY_SIZE(nvec_devices), res, 0, NULL);
if (ret)
dev_err(nvec->dev, "error adding subdevices\n");
--
2.0.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging/nvec: Remove double const qualifier
From: Thierry Reding
The SIMPLE_DEV_PM_OPS macro already uses the const qualifier, so there's
no need to repeat it.
Signed-off-by: Thierry Reding
---
drivers/staging/nvec/nvec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 11f9e1c3447c..aef52306620a 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -959,7 +959,7 @@ static int nvec_resume(struct device *dev)
}
#endif
-static const SIMPLE_DEV_PM_OPS(nvec_pm_ops, nvec_suspend, nvec_resume);
+static SIMPLE_DEV_PM_OPS(nvec_pm_ops, nvec_suspend, nvec_resume);
/* Match table for of_platform binding */
static const struct of_device_id nvidia_nvec_of_match[] = {
--
2.0.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging/nvec: Pass proper resource to mfd_add_devices()
On Mon, Jul 21, 2014 at 02:39:15PM -0700, Greg Kroah-Hartman wrote: > On Mon, Jul 21, 2014 at 10:01:36PM +0200, Marc Dietrich wrote: > > Hi Thierry, > > > > On Mon, 21 Jul 2014 13:52:41 +0200 > > Thierry Reding wrote: > > > > > From: Thierry Reding > > > > > > The mfd_add_devices() parameter takes a struct resource * as fifth > > > argument, but the nvec driver passes in a void __iomem *. The driver > > > gets away with it because none of the subdevices ever directly access > > > the registers. > > > > you are right, this one looked bogus. > > > > > While at it, use platform_get_irq() instead of platform_get_resource() > > > to get the device's interrupt. This makes it easier to pass in the > > > register region since the variable is no longer reused. > > > > > > Signed-off-by: Thierry Reding > > > --- > > > Alternatively we could simply pass NULL into mfd_add_devices(), which > > > might be a slightly more accurate representation of what's going on. > > > > > > Marc, Greg, any preferences? > > > > I just tested with NULL as base and it seems to produce no harm. Even > > if we may get rid of mfd_add_devices in the future, passing NULL is > > cleaner for now and no children will ever make use of it. So please send > > a new version with this change. > > > > Otherwise, thanks for looking and cleaning up. > > Ok, can I get your ack for this patch then? I'll resend a patch which passes in NULL as base tomorrow as requested by Marc. Thierry pgpIoxMuG8nHX.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging/nvec: Use platform_get_irq()
From: Thierry Reding
As opposed to platform_get_resource(), the platform_get_irq() function
has special code to handle driver probe deferral when booting using DT
and where an interrupt provider hasn't been registered yet. While this
is unlikely to become an issue for nvec, platform_get_irq() is the
recommended way to get at interrupts.
Signed-off-by: Thierry Reding
---
drivers/staging/nvec/nvec.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 815065837ce7..a93208adbfcf 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -821,8 +821,8 @@ static int tegra_nvec_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!res) {
+ nvec->irq = platform_get_irq(pdev, 0);
+ if (nvec->irq < 0) {
dev_err(&pdev->dev, "no irq resource?\n");
return -ENODEV;
}
@@ -840,7 +840,6 @@ static int tegra_nvec_probe(struct platform_device *pdev)
}
nvec->base = base;
- nvec->irq = res->start;
nvec->i2c_clk = i2c_clk;
nvec->rx = &nvec->msg_pool[0];
--
2.0.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging/nvec: Do not pass resource to mfd_add_devices()
From: Thierry Reding The mfd_add_devices() function takes a struct resource * as fifth argument, but the nvec driver passes in a void __iomem *. The driver gets away with it because none of the subdevices ever directly access the registers. Since subdevices never need to access the registers we can simply pass NULL instead. Signed-off-by: Thierry Reding --- drivers/staging/nvec/nvec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index 0a5c84ad3f41..815065837ce7 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -893,7 +893,7 @@ static int tegra_nvec_probe(struct platform_device *pdev) } ret = mfd_add_devices(nvec->dev, -1, nvec_devices, - ARRAY_SIZE(nvec_devices), base, 0, NULL); + ARRAY_SIZE(nvec_devices), NULL, 0, NULL); if (ret) dev_err(nvec->dev, "error adding subdevices\n"); -- 2.0.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 3/4] ARM: dts: tegra20: Add device tree node to describe IRAM
On Tue, Dec 12, 2017 at 03:26:09AM +0300, Dmitry Osipenko wrote: > From: Vladimir Zapolskiy > > All Tegra20 SoCs contain 256KiB IRAM, which is used to store > resume code and by a video decoder engine. > > Signed-off-by: Vladimir Zapolskiy > Signed-off-by: Dmitry Osipenko > --- > arch/arm/boot/dts/tegra20.dtsi | 8 > 1 file changed, 8 insertions(+) Applied, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 4/4] ARM: dts: tegra20: Add video decoder node
On Tue, Dec 12, 2017 at 03:26:10AM +0300, Dmitry Osipenko wrote: > Add Video Decoder Engine device node. > > Signed-off-by: Dmitry Osipenko > --- > arch/arm/boot/dts/tegra20.dtsi | 27 +++ > 1 file changed, 27 insertions(+) Applied, thanks. Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH] media: staging: tegra-vde: fix runtime pm imbalance on error
On Thu, May 21, 2020 at 08:39:02PM +0300, Dan Carpenter wrote:
> On Thu, May 21, 2020 at 05:22:05PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 21, 2020 at 11:15 AM Dan Carpenter
> > wrote:
> > >
> > > On Thu, May 21, 2020 at 11:42:55AM +0800, dinghao@zju.edu.cn wrote:
> > > > Hi, Dan,
> > > >
> > > > I agree the best solution is to fix __pm_runtime_resume(). But there
> > > > are also
> > > > many cases that assume pm_runtime_get_sync() will change PM usage
> > > > counter on error. According to my static analysis results, the number
> > > > of these
> > > > "right" cases are larger. Adjusting __pm_runtime_resume() directly will
> > > > introduce
> > > > more new bugs. Therefore I think we should resolve the "bug" cases
> > > > individually.
> > > >
> > >
> > > That's why I was saying that we may need to introduce a new replacement
> > > function for pm_runtime_get_sync() that works as expected.
> > >
> > > There is no reason why we have to live with the old behavior.
> >
> > What exactly do you mean by "the old behavior"?
>
> I'm suggesting we leave pm_runtime_get_sync() alone but we add a new
> function which called pm_runtime_get_sync_resume() which does something
> like this:
>
> static inline int pm_runtime_get_sync_resume(struct device *dev)
> {
> int ret;
>
> ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> if (ret < 0) {
> pm_runtime_put(dev);
> return ret;
> }
> return 0;
> }
>
> I'm not sure if pm_runtime_put() is the correct thing to do? The other
> thing is that this always returns zero on success. I don't know that
> drivers ever care to differentiate between one and zero returns.
>
> Then if any of the caller expect that behavior we update them to use the
> new function.
Does that really have many benefits, though? I understand that this
would perhaps be easier to use because it is more in line with how other
functions operate. On the other hand, in some cases you may want to call
a different version of pm_runtime_put() on failure, as discussed in
other threads.
Even ignoring that issue, any existing callsites that are leaking the
reference would have to be updated to call the new function, which would
be pretty much the same amount of work as updating the callsites to fix
the leak, right?
So if instead we just fix up the leaks, we might have a case of an API
that doesn't work as some of us (myself included) expected it, but at
least it would be consistent. If we add another variant things become
fragmented and therefore even more complicated to use and review.
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Re: [PATCH] media: staging: tegra-vde: fix runtime pm imbalance on error
On Fri, May 22, 2020 at 04:23:18PM +0300, Dan Carpenter wrote:
> On Fri, May 22, 2020 at 03:10:31PM +0200, Thierry Reding wrote:
> > On Thu, May 21, 2020 at 08:39:02PM +0300, Dan Carpenter wrote:
> > > On Thu, May 21, 2020 at 05:22:05PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, May 21, 2020 at 11:15 AM Dan Carpenter
> > > > wrote:
> > > > >
> > > > > On Thu, May 21, 2020 at 11:42:55AM +0800, dinghao@zju.edu.cn
> > > > > wrote:
> > > > > > Hi, Dan,
> > > > > >
> > > > > > I agree the best solution is to fix __pm_runtime_resume(). But
> > > > > > there are also
> > > > > > many cases that assume pm_runtime_get_sync() will change PM usage
> > > > > > counter on error. According to my static analysis results, the
> > > > > > number of these
> > > > > > "right" cases are larger. Adjusting __pm_runtime_resume() directly
> > > > > > will introduce
> > > > > > more new bugs. Therefore I think we should resolve the "bug" cases
> > > > > > individually.
> > > > > >
> > > > >
> > > > > That's why I was saying that we may need to introduce a new
> > > > > replacement
> > > > > function for pm_runtime_get_sync() that works as expected.
> > > > >
> > > > > There is no reason why we have to live with the old behavior.
> > > >
> > > > What exactly do you mean by "the old behavior"?
> > >
> > > I'm suggesting we leave pm_runtime_get_sync() alone but we add a new
> > > function which called pm_runtime_get_sync_resume() which does something
> > > like this:
> > >
> > > static inline int pm_runtime_get_sync_resume(struct device *dev)
> > > {
> > > int ret;
> > >
> > > ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> > > if (ret < 0) {
> > > pm_runtime_put(dev);
> > > return ret;
> > > }
> > > return 0;
> > > }
> > >
> > > I'm not sure if pm_runtime_put() is the correct thing to do? The other
> > > thing is that this always returns zero on success. I don't know that
> > > drivers ever care to differentiate between one and zero returns.
> > >
> > > Then if any of the caller expect that behavior we update them to use the
> > > new function.
> >
> > Does that really have many benefits, though? I understand that this
> > would perhaps be easier to use because it is more in line with how other
> > functions operate. On the other hand, in some cases you may want to call
> > a different version of pm_runtime_put() on failure, as discussed in
> > other threads.
>
> I wasn't CC'd on the other threads so I don't know. :/
It was actually earlier in this thread, see here for example:
http://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao@zju.edu.cn/#2438776
> I have always assumed it was something like this but I don't know the
> details and there is no documentation.
Now, I don't know more than you do, but it sounds to me like there are
multiple valid ways that we can use to drop the runtime PM reference and
whatever we choose to do in this new function may not always be the
right thing.
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> You're essentially arguing that it's a #1 on Rusty's scale but ideally
> we would want to be at #7.
I think we could probably get it to at least a 3 or a 4 on that list if
we add a bit of documentation and fix all existing users.
Yes, 7 would be better than that, but I think we have to weigh the cost
of the added fragmentation versus the benefits that it gives us.
> > Even ignoring that issue, any existing callsites that are leaking the
> > reference would have to be updated to call the new function, which would
> > be pretty much the same amount of work as updating the callsites to fix
> > the leak, right?
>
> With the current API we're constantly adding bugs. I imagine that once
> we add a straight forward default and some documentation then we will
> solve this.
In my experience this stuff is often copy/pasted, so once we fix up all
of the bugs (and perhaps even add a coccinelle script) we shoudl be
seeing less bugs added all the time.
That said, I'm not opposed to adding a new function if we can make it
actually result in an overall improvement. What I'd hate to do
Re: [PATCH v2 3/4] media: staging: tegra-vde: Turn ON power domain on shutdown
On Wed, Jun 24, 2020 at 06:08:46PM +0300, Dmitry Osipenko wrote: > On some devices bootloader isn't ready to a clamped VDE power, and thus, > machine hangs on a warm reboot (CPU reset). The VDE power partition is > turned ON by default on a cold boot, hence VDE driver should keep power > partition enabled on system's reboot too. This fixes hang on a warm reboot > on a Tegra20 Acer A500 device, which is handy if Embedded Controller > driver is unavailable, i.e. cold reboot can't be performed. > > Signed-off-by: Dmitry Osipenko > --- > drivers/staging/media/tegra-vde/vde.c | 19 +++ > 1 file changed, 19 insertions(+) Unfortunate that we have to do this, but looks fine: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/4] media: staging: tegra-vde: Runtime PM is always available on Tegra
On Wed, Jun 24, 2020 at 06:08:45PM +0300, Dmitry Osipenko wrote:
> Runtime PM is always available on Tegra nowadays since commit 40b2bb1b132a
> ("ARM: tegra: enforce PM requirement"), hence the case of unavailable RPM
> doesn't need to be handled.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/staging/media/tegra-vde/vde.c | 16
> 1 file changed, 16 deletions(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/4] media: staging: tegra-vde: Power-cycle hardware on probe
On Wed, Jun 24, 2020 at 06:08:47PM +0300, Dmitry Osipenko wrote: > VDE partition is left turned ON after bootloader on most devices, hence > let's ensure that it's turned OFF in order to lower power leakage while > hardware is idling by turning it ON and OFF during of the driver's probe. > > Signed-off-by: Dmitry Osipenko > --- > drivers/staging/media/tegra-vde/vde.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/staging/media/tegra-vde/vde.c > b/drivers/staging/media/tegra-vde/vde.c > index b64e35b86fb4..3be96c36bf43 100644 > --- a/drivers/staging/media/tegra-vde/vde.c > +++ b/drivers/staging/media/tegra-vde/vde.c > @@ -1068,6 +1068,14 @@ static int tegra_vde_probe(struct platform_device > *pdev) > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 300); > > + /* > + * VDE partition may be left ON after bootloader, hence let's > + * power-cycle it in order to put hardware into a predictable lower > + * power state. > + */ > + pm_runtime_get_sync(dev); > + pm_runtime_put(dev); > + > return 0; > > err_deinit_iommu: Shouldn't this happen automatically? My understanding is that power domains are turned on automatically before ->probe() and then unless a runtime PM reference is taken during ->probe() it will get turned off again after ->probe()? Is that not happening? Is auto-suspend perhaps getting in the way somehow? Thierry signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
